fix: correct code / type in getTokens call #330

ryanbas21 · 2023-02-16T15:27:37Z

JIRA Ticket

https://bugster.forgerock.org/jira/browse/SDKS-2375

Description

the options call is a GetTokens but the options we spread in should only be a ConfigOptions. so we can remove the types from the call that are not config options and pass them into authorization url creation

Type of Change

Please Delete options that are not relevant

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Definition of Done

Check all that apply

Documentation

Acceptance criteria met
Spell-check run
Peer reviewed
Proofread

nx-cloud · 2023-02-16T15:31:05Z

☁️ Nx Cloud Report

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

📂 See all runs for this branch

✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

github-actions · 2023-02-16T15:36:00Z

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

Preview environment endpoints are available at:

ryanbas21 · 2023-02-16T16:15:52Z

.github/workflows/merge.yml

    branches:
      - develop
      - master
+  workflow_dispatch:


allows this to be run wihtout a pr (mostly for me sanity checking or re reunning things or if we want to cut a release without a pr)

I had a look at the file separately as it looked odd - l shouldn't the workflow_dispatch key have a value, or some child keys indented below it? At first glance I assumed env: was a child of this but now seeing it without this review block in the way I don't quite understand how this is valid.

So we don't really pass any inputs into this workflow, so i don't think we need anything. Truthfully im not entirely sure we need this to begin with, i was adding it as a way of running CI without needing a PR up. so if you just need to sanity check things or something odd fails and you want to re run the branch

ryanbas21 · 2023-02-16T16:16:01Z

.github/workflows/merge.yml

      - uses: actions/checkout@v3
        with:
          fetch-depth: 0
+          ssh-key: ${{ secrets.SSH_PUBLIC_KEY }}


authenticate clone the repo

ryanbas21 · 2023-02-16T16:16:14Z

.github/workflows/merge.yml

          node-version: '16.x'
          scope: '@forgerock'
+      - name: Import GPG
+        uses: crazy-max/ghaction-import-gpg@v5.2.0


sign the commits. its locked to v5.2.0 as per our old discussion

ryanbas21 · 2023-02-16T16:16:22Z

.github/workflows/merge.yml

+      - name: Import GPG
+        uses: crazy-max/ghaction-import-gpg@v5.2.0
+        with:
+          gpg_private_key: ${{ secrets.GPG_PRIVATE_KEY }}


this is my gpg private key

ryanbas21 · 2023-02-16T16:16:35Z

package.json

    "ts-node": "10.9.1",
    "typedoc": "^0.17.7",
-    "typescript": "4.8.4",
+    "typescript": "4.9.5",


satisfies operator and newer ts stuff

Just wondering we have something for this project like dependabot that we use (whether manual or automated), to identify which of these dependencies need updating and when?

we have dependabot but not configured or setup really like i did in the other repo

ryanbas21 · 2023-02-16T16:16:57Z

packages/javascript-sdk/src/token-manager/index.ts

     */
    const verifier = PKCE.createVerifier();
    const state = PKCE.createState();
+    const { forceRenew, login, query, ...config } = options;


unsure if theres a better way to remove the lint warnings on unused variables but this felt like the cleanest option.

Ok, think I understand this now after reading through for a little bit. Not sure how we can remove the lint warnings but will have a think. What you're doing makes sense though.

I think we need to pass query into the authorizeUrlOrptions as well, yeah? It's used here: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/oauth2-client/index.ts#L53.

Yes I think you're right, good spot. Yeah it looks like GetTokensOptions defines query on top of ConfigOptions.

I quickly pulled the code down and ran an e2e test which passes a query parameter into the authorize URL options, and I can confirm that I got a failure on authn-central-login which seems to be caused by the fact that the query param for ACR values is ignored / not added to the request. So I agree with Justin, we need to also pass query into authorizeUrlOptions here.

the options call is a GetTokens but the options we spread in should only be a ConfigOptions. so we can remove the types from the call that are not config options and pass them into authorization url creation

daveadams56

Initial look - seems good, I had to think a bit to understand what we are doing here but it makes sense to me now. I'll have a quick think about linting warnings if I get a chance.

daveadams56 · 2023-02-16T20:40:32Z

packages/javascript-sdk/src/token-manager/index.ts

     */
    const verifier = PKCE.createVerifier();
    const state = PKCE.createState();
+    const { forceRenew, login, query, ...config } = options;


Ok, think I understand this now after reading through for a little bit. Not sure how we can remove the lint warnings but will have a think. What you're doing makes sense though.

daveadams56

Another pass through this - on the CI side just a quick question about what we're adding to project.json, for my understanding. I also pulled down the code and ran a test that I thought would exercise the options change and test Justin's assertion that we need to pass query in as well. From my perspective we need to add query to the authorizeUrlOptions before this can be merged. Once this is done, I'm happy with this.

daveadams56 · 2023-02-22T14:08:43Z

packages/javascript-sdk/project.json

          "remote": "origin",
          "push": true,
-          "postTargets": ["javascript-sdk:deploy:beta"],
+          "postTargets": ["javascript-sdk:deploy:beta", "javascript-sdk:github:beta"],


Are you able to explain what we're adding here, with javascript-sdk:github:beta and javascript-sdk:github? It's not obvious to me and would like to know more. Thanks.

its a post target, so after this runs, it will run that nx target.

daveadams56 · 2023-02-22T14:33:35Z

packages/javascript-sdk/src/token-manager/index.ts

     */
    const verifier = PKCE.createVerifier();
    const state = PKCE.createState();
+    const { forceRenew, login, query, ...config } = options;


I quickly pulled the code down and ran an e2e test which passes a query parameter into the authorize URL options, and I can confirm that I got a failure on authn-central-login which seems to be caused by the fact that the query param for ACR values is ignored / not added to the request. So I agree with Justin, we need to also pass query into authorizeUrlOptions here.

fix query removal from the token index authorize-url

ryanbas21 temporarily deployed to Preview February 16, 2023 15:27 — with GitHub Actions Inactive

github-actions bot added CI NX root sdk labels Feb 16, 2023

ryanbas21 force-pushed the sdk-fix-token-type branch from 3e3c2e2 to 3f84475 Compare February 16, 2023 16:06

ryanbas21 temporarily deployed to Preview February 16, 2023 16:06 — with GitHub Actions Inactive

ryanbas21 commented Feb 16, 2023

View reviewed changes

fix(javascript-sdk): correct code / type in getTokens call

abd3e66

the options call is a GetTokens but the options we spread in should only be a ConfigOptions. so we can remove the types from the call that are not config options and pass them into authorization url creation

ryanbas21 force-pushed the sdk-fix-token-type branch from 3f84475 to abd3e66 Compare February 16, 2023 19:06

ryanbas21 temporarily deployed to Preview February 16, 2023 19:06 — with GitHub Actions Inactive

daveadams56 reviewed Feb 16, 2023

View reviewed changes

daveadams56 requested changes Feb 22, 2023

View reviewed changes

chore(javascript-sdk): pr-review

41f28fe

fix query removal from the token index authorize-url

ryanbas21 temporarily deployed to Preview February 24, 2023 15:44 — with GitHub Actions Inactive

cerebrl approved these changes Mar 15, 2023

View reviewed changes

ryanbas21 merged commit 7e0bdaf into develop Mar 15, 2023

ryanbas21 deleted the sdk-fix-token-type branch March 15, 2023 16:57

fix: correct code / type in getTokens call #330

fix: correct code / type in getTokens call #330

Uh oh!

Conversation

ryanbas21 commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA Ticket

Description

Type of Change

How Has This Been Tested?

Definition of Done

Documentation

Uh oh!

nx-cloud bot commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

github-actions bot commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daveadams56 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daveadams56 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ryanbas21 commented Feb 16, 2023 •

edited

Loading

nx-cloud bot commented Feb 16, 2023 •

edited

Loading