fix(oidc-client): always pass prompt=none on background authorize calls - #748
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBackground authorization now enforces ChangesBackground authorization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🦋 Changeset detectedLatest commit: 2ff7b6a The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit 976a3bc
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
15f8987 to
6f3c67f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (23.94%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #748 +/- ##
==========================================
+ Coverage 18.07% 23.94% +5.87%
==========================================
Files 155 162 +7
Lines 24398 25722 +1324
Branches 1203 1668 +465
==========================================
+ Hits 4410 6160 +1750
+ Misses 19988 19562 -426
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/oidc-client/src/lib/client.store.ts`:
- Around line 210-213: Update the background authorization flow around
authorizeµ so bgOptions is always an options object containing prompt: 'none',
while preserving any caller-provided options. Add regression coverage for
authorize.background() without arguments in both PAR and standard authorization
flows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40de47a1-7d61-4cc0-a5d3-e6ac03c6c004
📒 Files selected for processing (3)
.changeset/oidc-prompt-none-background.mdpackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.ts
| options !== undefined ? { ...options, prompt: 'none' as const } : undefined; | ||
| const result = await Micro.runPromiseExit( | ||
| authorizeµ(wellknown, config, log, store, options, useParFlow), | ||
| authorizeµ(wellknown, config, log, store, bgOptions, useParFlow), |
There was a problem hiding this comment.
Can we simplify the code to authorizeµ(wellknown, config, log, store, { ...options, prompt: 'none'}, useParFlow), instead of creating a new bgOptions variable?
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/oidc-client/src/lib/client.store.test.ts (1)
689-773: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest caller-provided
promptprecedence.Add PAR and standard-flow cases that call
background({ prompt: 'login' }). Assert that each outbound request still containsprompt=none. A reversed merge order would pass the current tests because they only omitprompt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/client.store.test.ts` around lines 689 - 773, The authorize.background() enforcement test only covers omitted prompts; extend it with PAR and standard-flow calls that provide prompt: 'login', then assert each outbound request still sends prompt=none. Update the existing test around authorize.background() and its captured PAR and authorize request assertions, preserving the current omission cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/oidc-client/src/lib/client.store.test.ts`:
- Around line 689-773: The authorize.background() enforcement test only covers
omitted prompts; extend it with PAR and standard-flow calls that provide prompt:
'login', then assert each outbound request still sends prompt=none. Update the
existing test around authorize.background() and its captured PAR and authorize
request assertions, preserving the current omission cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02c52d78-d137-410b-b577-493fc4edaa23
📒 Files selected for processing (2)
packages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/oidc-client/src/lib/client.store.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b361bfe to
08edab1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/oidc-client/src/lib/client.store.ts (1)
210-212: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
background()no longer forcesprompt=none.Line 211 passes
optionsstraight toauthorizeµ. The PR objectives state thatbackground()must mergeprompt: 'none'into the options for both the standard and the PAR flow. Withoutprompt=none, the authorization server can return an interactive login page for the hidden background request. The call then hangs or fails instead of returninglogin_required. Reviewers raised this point on earlier commits.🐛 Proposed fix
const result = await Micro.runPromiseExit( - authorizeµ(wellknown, config, log, store, options, useParFlow), + authorizeµ(wellknown, config, log, store, { ...options, prompt: 'none' }, useParFlow), );Run the following script to check the related tests and the
authorizeµoption handling:#!/bin/bash # Find prompt=none expectations in oidc-client tests and the authorizeµ option path. rg -n -C4 "prompt" --glob 'packages/oidc-client/src/**' -g '*.ts'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/client.store.ts` around lines 210 - 212, Update background() at the authorizeµ invocation to merge prompt: 'none' into the options before passing them to Micro.runPromiseExit, preserving all existing options. Ensure this merged option is used for both standard and PAR authorization flows.
🧹 Nitpick comments (3)
e2e/oidc-suites/src/par.spec.ts (1)
90-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack the server-side defect before keeping this test skipped.
Add an upstream issue reference and a re-enable condition to this TODO. The skip removes end-to-end coverage for the PAR redirect-login and token flow. The existing unit tests only cover client-side PAR URL construction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/oidc-suites/src/par.spec.ts` around lines 90 - 91, Update the skipped test declaration for “redirect login with PAR enabled (ParClient)” to include a reference to the tracked upstream server-side issue and an explicit condition or TODO mechanism for re-enabling it once that defect is fixed, while preserving the test’s existing end-to-end coverage and assertions.packages/oidc-client/src/lib/session.micros.test.ts (1)
206-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailure assertions can be skipped in both migrated test files. The migration from
it.effectreplaced typed failure assertions with a combined guard that returns early whenMicro.causeIsFailisfalse. A defect (Die) cause still satisfiesMicro.exitIsFailure, so the error-field assertions never run and the test passes.
packages/oidc-client/src/lib/session.micros.test.ts#L206-L211: addexpect(Micro.causeIsFail(exit.cause)).toBe(true)before the early return, and apply the same change to every failure case in the file.packages/oidc-client/src/lib/exchange.utils.test.ts#L97-L104: addexpect(Micro.causeIsFail(result.cause)).toBe(true)before the early return, and apply the same change at lines 126-133 and 147-154.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/session.micros.test.ts` around lines 206 - 211, Ensure every failure-case assertion in packages/oidc-client/src/lib/session.micros.test.ts at lines 206-211 and throughout the file, plus packages/oidc-client/src/lib/exchange.utils.test.ts at lines 97-104, 126-133, and 147-154, explicitly expects Micro.causeIsFail(exit.cause/result.cause) to be true before any early return; retain the existing typed error-field assertions so defect causes cannot silently pass.e2e/mock-api-v2/src/services/session.service.ts (1)
16-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new session contract and failure paths.
Test successful
refreshSessioncalls and assert that the returnedSessionDatacontains the updated expiry. Test missing-sessionupdateSessioncalls and missing or expiredrefreshSessioncalls. Assert the failure messages and removal of expired sessions. The PR coverage report identifies changed lines without coverage.Also applies to: 68-68, 81-87, 89-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/mock-api-v2/src/services/session.service.ts` around lines 16 - 19, Add regression tests for the session service contract around refreshSession and updateSession: verify successful refreshSession returns SessionData with the updated expiry, missing-session updateSession fails with the expected message, and missing or expired refreshSession calls fail with the expected messages while removing expired sessions. Cover the changed branches and declarations associated with refreshSession, updateSession, and expired-session handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/davinci-client/src/lib/client.store.effects.ts`:
- Around line 189-191: Update the completed-challenge branch handling
PollDispatchResult.data to validate status at runtime: accept only string values
matching an allowed PollingStatus member, and return {_tag: 'error'} for
non-string or unsupported statuses. Remove the unchecked cast in this path and
add tests covering both a non-string status and an unsupported string status.
In `@packages/oidc-client/src/lib/authorize.request.micros.ts`:
- Line 122: Update background() in authorize.request.micros.ts to merge prompt:
'none' into the options passed to both PAR and standard authorization flows,
including when called without arguments, while preserving caller-provided
options otherwise. Update the related assertions in client.store.test.ts to
expect prompt=none and revise .changeset/oidc-prompt-none-background.md
accordingly.
---
Outside diff comments:
In `@packages/oidc-client/src/lib/client.store.ts`:
- Around line 210-212: Update background() at the authorizeµ invocation to merge
prompt: 'none' into the options before passing them to Micro.runPromiseExit,
preserving all existing options. Ensure this merged option is used for both
standard and PAR authorization flows.
---
Nitpick comments:
In `@e2e/mock-api-v2/src/services/session.service.ts`:
- Around line 16-19: Add regression tests for the session service contract
around refreshSession and updateSession: verify successful refreshSession
returns SessionData with the updated expiry, missing-session updateSession fails
with the expected message, and missing or expired refreshSession calls fail with
the expected messages while removing expired sessions. Cover the changed
branches and declarations associated with refreshSession, updateSession, and
expired-session handling.
In `@e2e/oidc-suites/src/par.spec.ts`:
- Around line 90-91: Update the skipped test declaration for “redirect login
with PAR enabled (ParClient)” to include a reference to the tracked upstream
server-side issue and an explicit condition or TODO mechanism for re-enabling it
once that defect is fixed, while preserving the test’s existing end-to-end
coverage and assertions.
In `@packages/oidc-client/src/lib/session.micros.test.ts`:
- Around line 206-211: Ensure every failure-case assertion in
packages/oidc-client/src/lib/session.micros.test.ts at lines 206-211 and
throughout the file, plus packages/oidc-client/src/lib/exchange.utils.test.ts at
lines 97-104, 126-133, and 147-154, explicitly expects
Micro.causeIsFail(exit.cause/result.cause) to be true before any early return;
retain the existing typed error-field assertions so defect causes cannot
silently pass.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7757ba1c-b2e9-4b5c-b6e0-c6f5ae65bde6
📒 Files selected for processing (13)
.changeset/oidc-prompt-none-background.mde2e/mock-api-v2/src/services/mock-env-helpers/index.tse2e/mock-api-v2/src/services/session.service.tse2e/oidc-suites/src/par.spec.tspackages/davinci-client/src/lib/client.store.effects.test.tspackages/davinci-client/src/lib/client.store.effects.tspackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/exchange.utils.test.tspackages/oidc-client/src/lib/session.micros.test.ts
| if (data['isChallengeComplete'] === true) { | ||
| const pollStatus = data['status']; | ||
| return pollStatus ? (pollStatus as PollingStatus) : 'error'; | ||
| const status = data['status']; | ||
| return status ? { _tag: 'complete', status: status as PollingStatus } : { _tag: 'error' }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the completed challenge status at runtime.
PollDispatchResult.data is unknown. The truthiness check accepts any non-empty value and casts it to PollingStatus. A response with an unsupported status stops polling and returns an invalid terminal status.
Validate the status type and allowed PollingStatus values. Classify invalid values as error. Add tests for a non-string status and an unsupported string status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/davinci-client/src/lib/client.store.effects.ts` around lines 189 -
191, Update the completed-challenge branch handling PollDispatchResult.data to
validate status at runtime: accept only string values matching an allowed
PollingStatus member, and return {_tag: 'error'} for non-string or unsupported
statuses. Remove the unchecked cast in this path and add tests covering both a
non-string status and an unsupported string status.
bbafd97 to
485e735
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/oidc-client/src/lib/client.store.ts`:
- Around line 210-213: Update the background authorization flow around
authorizeµ so bgOptions is always an options object containing prompt: 'none',
including when authorize.background() is called without arguments. Preserve any
supplied options while enforcing that prompt value, and add coverage for
no-argument background calls in both PAR and standard flows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76024ef7-0c5e-4eeb-a4f5-8124849b2370
📒 Files selected for processing (4)
.changeset/oidc-prompt-none-background.mdpackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/oidc-client/src/lib/authorize.request.micros.ts
Address PR #748 review comments: - bgOptions always created as object (never undefined) so prompt=none is enforced even when background() is called without arguments - token.get() background renewal also enforces prompt=none via bgAuthorizeOptions - add no-argument background() regression tests for both PAR and standard flows
Address PR #748 review comments: - bgOptions always created as object (never undefined) so prompt=none is enforced even when background() is called without arguments - token.get() background renewal also enforces prompt=none via bgAuthorizeOptions - add no-argument background() regression tests for both PAR and standard flows
b4409a2 to
9bd12a5
Compare
There was a problem hiding this comment.
Some e2e tests are failing in the oidc suites and we are unable to publish a beta either. There might be a bigger refactor needed here to make sure those tests pass, to remove prompt none from some places.
We need to investigate this further before the PR can be approved.
pnpm nx e2e @forgerock/oidc-suites is failing at the moment.
https://github.com/ForgeRock/ping-javascript-sdk/actions/runs/31130463395/job/92717945620#step:4:3506
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
This CI failure appears to be related to the environment or external dependencies rather than your code changes.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
|
Deployed d62f47b to https://ForgeRock.github.io/ping-javascript-sdk/pr-748/d62f47b68240d5c5835990fcb00aa861809e9a59 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/davinci-client - 56.7 KB (new) 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
44c35f8 to
bc0ac85
Compare
- Restore prompt='none' injection in createAuthorizeUrlµ (background/silent dispatch path only — iframe and fetch, never redirect) - Remove erroneous prompt='none' injection from token.get() shared path, which was causing login_required/interaction_required on redirect flows - Strip prompt from fallback redirectUrl in buildAuthorizeRedirectUrlµ so interactive redirect still works after a failed silent auth - Skip flaky PAR redirect e2e test (external ForgeBlocks AM timeout) - Bump pnpm.overrides for CVE fixes: fast-uri, brace-expansion, undici, nanoid, postcss
bc0ac85 to
2ff7b6a
Compare
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
The need for removing prompt none in buildAuthorizeRedirectUrlµ seems weird. I've mentioned this in the line comment. We can do the refactor later after the patch release.
| return Micro.tryPromise({ | ||
| try: () => createAuthorizeUrl(wellknown.authorization_endpoint, { ...options }), | ||
| try: () => { | ||
| const { prompt: _prompt, ...interactiveOptions } = options; |
There was a problem hiding this comment.
This looks like code smell to me, that we need to add an exception to remove prompt none so deep in the code.
Architecturally we should handle the redirect case in the client.store file so we can correctly route any failures. Building fallback / redirect case in dispatchAuthorizeFetchµ / dispatchAuthorizeIframeµ means we need to add exceptions like above.
We can remove buildAuthorizeRedirectUrlµ and handleDispatchErrorµ functions entirely and build the authorize url for redirect in client.store file. That will simplify the logic.
We can do the refactoring later because that looks like it's going to affect several files. As far as this PR is concerned, I think it's okay to add this exception and continue with the patch release.
There was a problem hiding this comment.
I had to add this because of a failing test.
- authorize.background() now injects prompt: 'none' before calling authorizeµ
- authorizeµ passes those options all the way down to dispatchAuthorizeFetchµ / dispatchAuthorizeIframeµ
- When background auth fails (e.g. no active session), those dispatch functions call handleDispatchErrorµ → buildAuthorizeRedirectUrlµ to construct a fallback interactive redirect URL
- Without the fix, that fallback URL was being built with prompt: 'none' still in the options — so the "fallback to interactive login" redirect would immediately fail with interaction_required at the server
1aab424 to
2ff7b6a
Compare
4 participants
Summary
Background authorize flows (both standard and PAR) must include
prompt=noneso the authorization server does not prompt the user for interaction during silent/background token acquisition.Bug
The standard authorize flow already enforced
prompt=noneinsidecreateAuthorizeUrlµ(hardcoded). The PAR flow did not —createParAuthorizeUrlµpassedpromptfrom the caller's options, which meant any background PAR call without an explicitpromptoption would silently omit the required parameter.Fix
In
background()(client.store.ts), mergeprompt: 'none'into options before delegating toauthorizeµ. This is the correct enforcement point: it covers both flow paths (standard and PAR) and mirrors how the OIDC spec expects silent authentication to work.Tests
prompt=nonein the PAR POST bodybackground() always includes prompt=none even when options omit it(covers both PAR-enabled and standard flow paths)Summary by CodeRabbit
Bug Fixes
prompt=nonefor standard and PAR flows.Tests