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

cerebrl · 2023-11-01T21:21:56Z

DO NOT MERGE

JIRA Ticket

Description

Adjust OAuth2Client class to attach the raw response from the server as an additional property that propagates through to the application layer and also is stored along with the traditional tokens.

Details

add raw response to returned value in OAuth2Client.getOAuth2Tokens
this raw response is the original serialized JSON body
this new property will be persisted with the tokens in Web Storage

Type of Change

Please Delete options that are not relevant

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Manually tested with the e2e autoscript apps (screenshot included) as well as added a test assertion for our automated e2e test suite as well.

Screenshot of manual test with localStorage with rawResponse containing the original response body

Definition of Done

Check all that apply

Documentation

Acceptance criteria met
Spell-check run
Peer reviewed
Proofread

- add raw response to returned value in OAuth2Client - this raw response is the original serialized JSON body - this new property will be persisted with the tokens in WebStorage

changeset-bot · 2023-11-01T21:21:59Z

⚠️ No Changeset found

Latest commit: b4ba91e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

nx-cloud · 2023-11-01T21:27:21Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b4ba91e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

cerebrl · 2023-11-01T21:30:03Z

e2e/autoscript-apps/src/authn-oauth/autoscript.ts

@@ -88,6 +88,7 @@ function autoscript() {
        } else {
          throw new Error('Session_Error');
        }
+        console.log(tokens.rawResponse);


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

Should probably use the Logger class?

cerebrl · 2023-11-01T21:32:08Z

e2e/autoscript-suites/src/suites/authn-oauth.lc.test.ts

+    messageArray.forEach((message) => {
+      if (message.includes('access_token')) {
+        rawResponse = message;
+      }
+    });


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.

cerebrl · 2023-11-01T21:33:53Z

e2e/autoscript-suites/src/suites/authn-oauth.lc.test.ts

+    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);


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.

cerebrl · 2023-11-01T21:34:27Z

e2e/autoscript-suites/src/suites/authz-txn-basic-json.test.ts

+    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);


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

cerebrl · 2023-11-01T21:35:20Z

packages/javascript-sdk/src/oauth2-client/index.ts

@@ -160,6 +160,7 @@ abstract class OAuth2Client {
    };

    const response = await this.request('accessToken', undefined, false, init, options);
+    const responseClone = response.clone();


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

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

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

wow havent used this before at all. interesting stuff

github-actions · 2023-11-01T21:35:57Z

Your preview environment pr-400-ryanbas21 has been deployed.

Preview environment endpoints are available at:

ryanbas21

thoughts for discussion

ryanbas21 · 2023-11-13T21:08:16Z

e2e/autoscript-apps/src/authn-oauth/autoscript.ts

@@ -88,6 +88,7 @@ function autoscript() {
        } else {
          throw new Error('Session_Error');
        }
+        console.log(tokens.rawResponse);


Should probably use the Logger class?

ryanbas21 · 2023-11-13T21:15:02Z

packages/javascript-sdk/src/oauth2-client/index.ts

@@ -160,6 +160,7 @@ abstract class OAuth2Client {
    };

    const response = await this.request('accessToken', undefined, false, init, options);
+    const responseClone = response.clone();


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

ryanbas21 · 2023-11-13T21:22:12Z

packages/javascript-sdk/src/oauth2-client/index.ts

@@ -185,6 +186,7 @@ abstract class OAuth2Client {
      idToken: responseObject.id_token,
      refreshToken: responseObject.refresh_token,
      tokenExpiry: tokenExpiry,
+      rawResponse: await responseClone.text(),


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

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.

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

b4ba91e

- add raw response to returned value in OAuth2Client - this raw response is the original serialized JSON body - this new property will be persisted with the tokens in WebStorage

cerebrl temporarily deployed to Preview November 1, 2023 21:22 — with GitHub Actions Inactive

cerebrl requested a review from ryanbas21 November 1, 2023 21:22

github-actions bot added sdk test labels Nov 1, 2023

cerebrl commented Nov 1, 2023

View reviewed changes

ryanbas21 requested changes Nov 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cerebrl commented Nov 1, 2023 •

edited

Loading

changeset-bot bot commented Nov 1, 2023

nx-cloud bot commented Nov 1, 2023 •

edited

Loading

cerebrl Nov 1, 2023

ryanbas21 Nov 13, 2023

cerebrl Nov 1, 2023

cerebrl Nov 1, 2023

cerebrl Nov 1, 2023

cerebrl Nov 1, 2023

ryanbas21 Nov 13, 2023

cerebrl Nov 14, 2023

ryanbas21 Nov 14, 2023

github-actions bot commented Nov 1, 2023

ryanbas21 left a comment

ryanbas21 Nov 13, 2023

ryanbas21 Nov 13, 2023

ryanbas21 Nov 13, 2023

cerebrl Nov 14, 2023

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

Are you sure you want to change the base?

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

Conversation

cerebrl commented Nov 1, 2023 • edited Loading

DO NOT MERGE

JIRA Ticket

Description

Details

Type of Change

How Has This Been Tested?

Definition of Done

Documentation

changeset-bot bot commented Nov 1, 2023

⚠️ No Changeset found

nx-cloud bot commented Nov 1, 2023 • edited Loading

☁️ Nx Cloud Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 1, 2023

ryanbas21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cerebrl commented Nov 1, 2023 •

edited

Loading

nx-cloud bot commented Nov 1, 2023 •

edited

Loading