fix(oidc-client): always pass prompt=none on background authorize calls - #748
fix(oidc-client): always pass prompt=none on background authorize calls#748ryanbas21 wants to merge 4 commits into
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: 9bd12a5 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 9bd12a5
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
15f8987 to
6f3c67f
Compare
Codecov Report❌ Patch coverage is Please upload reports for the commit 6f3c67f to get more accurate results. ❌ Your project status has failed because the head coverage (23.64%) 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.64% +5.57%
==========================================
Files 155 162 +7
Lines 24398 25716 +1318
Branches 1203 1660 +457
==========================================
+ Hits 4410 6081 +1671
+ Misses 19988 19635 -353
🚀 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
@forgerock/sdk-wellknown
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. |
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
We classified this failure as an environment issue rather than a code regression. The failing test exercises the interactive PAR redirect login flow, which is unrelated to the background() changes introduced by this PR. The external ForgeBlocks auth server successfully received the PAR request but did not redirect back within the timeout, indicating an external service dependency failure.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
I think we need to enforce prompt: 'none' in the authorizeOptions here as well.
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.
Background authorize flows (both standard and PAR) must include prompt=none so the authorization server does not prompt the user for interaction. The standard flow already enforced this inside createAuthorizeUrlµ; the PAR flow was not injecting it, meaning any background PAR call without an explicit prompt option would silently omit the required parameter.
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
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