Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(javascript-sdk): include raw response to OAuth token return #400

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

feat(javascript-sdk): include raw response to OAuth token return #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions e2e/autoscript-apps/src/authn-oauth/autoscript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ function autoscript() {
} else {
throw new Error('Session_Error');
}
console.log(tokens.rawResponse);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a log of the raw response, so the test suite can pick it up and ensure the original response has the correct values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use the Logger class?

return tokens;
}),
rxDelay(delay),
Expand Down
26 changes: 20 additions & 6 deletions e2e/autoscript-suites/src/suites/authn-oauth.lc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,28 @@ test.describe('Test OAuth login flow', () => {
test(`should login successfully and then log out with`, async ({ page, browserName }) => {
const { messageArray, networkArray } = await setupAndGo(page, browserName, 'authn-oauth/');

let rawResponse = '';

// Test assertions
// Test log messages
expect(messageArray.includes('OAuth login successful')).toBe(true);
expect(messageArray.includes('Logout successful')).toBe(true);
expect(messageArray.includes('Calling authorize endpoint')).toBe(true);
expect(messageArray.includes('Calling access token exchange endpoint')).toBe(true);
expect(messageArray.includes('Get user info from OAuth endpoint')).toBe(true);
expect(messageArray.includes('New OAuth tokens retrieved')).toBe(true);
expect(messageArray.includes('OAuth login successful'), 'oauth success').toBe(true);
expect(messageArray.includes('Logout successful'), 'logout success').toBe(true);
expect(messageArray.includes('Calling authorize endpoint'), 'call /authorize').toBe(true);
expect(
messageArray.includes('Calling access token exchange endpoint'),
'call /access_token',
).toBe(true);
expect(messageArray.includes('Get user info from OAuth endpoint'), 'call /userinfo').toBe(true);
expect(messageArray.includes('New OAuth tokens retrieved'), 'tokens received').toBe(true);
Comment on lines +22 to +30
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding secondary messages here, so if the test fails, it will print the message to help inform which expectation failed. Without this, the test just says, "expected true and got false" or something like that.


// Test rawResponse on token object
messageArray.forEach((message) => {
if (message.includes('access_token')) {
rawResponse = message;
}
});
Comment on lines +33 to +37
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new code that tests the existence of the raw response, which uses snake_case. Because this is a live capable test, I just want to test the existence of keys, not values.


expect(rawResponse.includes('access_token')).toBe(true);

// Test network requests
// Make sure revoke request is made twice, one for force renew and one for logout
Expand Down
20 changes: 16 additions & 4 deletions e2e/autoscript-suites/src/suites/authz-txn-basic-json.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,21 @@ test.describe('Test Transaction Authorization flow using JSON response', () => {
const { messageArray } = await setupAndGo(page, browserName, 'authz-txn-basic-json/');

// Test assertions
expect(messageArray.includes('IG resource requires additional authorization')).toBe(true);
expect(messageArray.includes('Request to IG resource successfully responded')).toBe(true);
expect(messageArray.includes('Starting authentication with composite advice')).toBe(true);
expect(messageArray.includes('Continuing authentication with composite advice')).toBe(true);
expect(
messageArray.includes('IG resource requires additional authorization'),
'add. auth required',
).toBe(true);
expect(
messageArray.includes('Request to IG resource successfully responded'),
'successful response',
).toBe(true);
expect(
messageArray.includes('Starting authentication with composite advice'),
'start auth with advice',
).toBe(true);
expect(
messageArray.includes('Continuing authentication with composite advice'),
'continue with advice',
).toBe(true);
Comment on lines +19 to +34
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This randomly failed, and I was motivated to add the secondary messages here to, but there's no actual code change here.

});
});
2 changes: 2 additions & 0 deletions packages/javascript-sdk/src/oauth2-client/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ abstract class OAuth2Client {
};

const response = await this.request('accessToken', undefined, false, init, options);
const responseClone = response.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloning the response here since we want to both grab the response text as well as create parsed JSON out of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to what you mean? I don't understand why the clone is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! This is something that took me a bit to wrap my head around (I had to do this a lot in the Token Vault plugin). The tl;dr of is any time you call a method off of body, you "attach" a "reader" to the underlying ReadableStream of the body. Only one reader can be attached to a stream at a time, so if you have two different places in the code that need to "attach" to the stream of the body, then you have to clone it.

The other location where we read the body is here: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/oauth2-client/index.ts#L301.

Ref for the above: https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_readable_streams#attaching_a_reader

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow havent used this before at all. interesting stuff

const responseBody = await this.getBody<unknown>(response);

if (response.status !== 200) {
Expand All @@ -185,6 +186,7 @@ abstract class OAuth2Client {
idToken: responseObject.id_token,
refreshToken: responseObject.refresh_token,
tokenExpiry: tokenExpiry,
rawResponse: await responseClone.text(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the basic ask is to just include the entire response object in addition to how we structure it normally.

i'm curious if we should include this as an opt in configuration? it may be overkill to add as a config but i also think we are adding a lot to what was previously a very thin api that could add noise for many who just don't need this info

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we discussed enabling/disabling this with a config flag, but since this was just a test branch, we decided to minimize the work. If this is brought in as an official feature, we can consider more sophisticated implementation.

};
}

Expand Down
1 change: 1 addition & 0 deletions packages/javascript-sdk/src/oauth2-client/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { ResponseType } from './enums';
interface OAuth2Tokens {
accessToken: string;
idToken?: string;
rawResponse: unknown;
refreshToken?: string;
tokenExpiry?: number;
}
Expand Down
1 change: 1 addition & 0 deletions packages/javascript-sdk/src/shared/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ interface StringDict<T> {
interface Tokens {
accessToken: string;
idToken?: string;
rawResponse: unknown;
refreshToken?: string;
tokenExpiry?: number;
}
Expand Down
9 changes: 5 additions & 4 deletions packages/javascript-sdk/src/token-manager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ abstract class TokenManager {
const tokens = await TokenStorage.get();

/**
* If tokens are stored, no option for `forceRenew` or `query` object with `code`, and do not expire within the configured threshold,
* immediately return the stored tokens
* If tokens are stored, no option for `forceRenew` or `query` object with `code`,
* and do not expire within the configured threshold, immediately return the stored tokens
*/
if (
tokens &&
Expand All @@ -87,8 +87,9 @@ abstract class TokenManager {
}

/**
* If we are still here because of forceRenew or we have an authorization code, or the tokens expire within the configured threshold,
* revoke and delete existing tokens to prepare for the new ones
* If we are still here because of forceRenew or we have an authorization code,
* or the tokens expire within the configured threshold, revoke and delete existing
* tokens to prepare for the new ones
*/
if (tokens) {
try {
Expand Down
Loading