fix(oidc-client): append query params when provided #471
Conversation
🦋 Changeset detectedLatest commit: ef4ab6f The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
WalkthroughRefactors authorize URL construction by removing the explicit OidcConfig from the createAuthorizeUrlµ signature and returned tuple, updates related tests, and adds behavior to spread optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant authorize.request
participant authorize.utils
participant authorize.effects
Caller->>authorize.request: authorize(url, options)
activate authorize.request
note over authorize.request: flatMap now calls\ncreateAuthorizeUrlµ(url, options)
authorize.request->>authorize.utils: createAuthorizeUrlµ(url, options)
activate authorize.utils
authorize.utils->>authorize.utils: buildAuthorizeOptionsµ()
note over authorize.utils: returns [string, GetAuthorizationUrlOptions]
authorize.utils-->>authorize.request: [url, options]
deactivate authorize.utils
authorize.request->>authorize.effects: buildAuthorizeURL(url, options)
activate authorize.effects
note over authorize.effects: Spreads options.query into\nURLSearchParams (NEW)
note over authorize.effects: Then adds mandatory params\n(code_challenge, client_id, etc.)
authorize.effects-->>authorize.request: authorizeURL (with query params)
deactivate authorize.effects
authorize.request-->>Caller: authorizeURL
deactivate authorize.request
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)packages/sdk-effects/oidc/src/lib/authorize.test.ts (2)
packages/oidc-client/src/lib/authorize.request.utils.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (5)
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 |
|
View your CI Pipeline Execution ↗ for commit ef4ab6f
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/sdk-effects/oidc/src/lib/authorize.effects.ts (1)
42-53: LGTM! Query parameter spreading implemented correctly.The implementation correctly spreads
options.queryintoURLSearchParamsbefore adding standard OAuth parameters. This ordering ensures that required OAuth parameters (e.g.,client_id,redirect_uri) cannot be overridden by custom query parameters, which is the correct security posture.Consider adding a brief comment explaining the parameter precedence:
const requestParams = new URLSearchParams({ + // Spread custom query params first; standard OAuth params below will override any conflicts ...options.query, code_challenge: challenge,packages/sdk-effects/oidc/src/lib/authorize.test.ts (1)
86-102: LGTM! Good test coverage for the new feature.The test correctly verifies that custom query parameters are appended to the authorization URL when provided via
options.query.Consider adding a test case that verifies standard OAuth parameters take precedence over query parameters in case of naming conflicts:
it('should ensure standard OAuth params override conflicting query params', async () => { const optionsWithConflict: GenerateAndStoreAuthUrlValues = { ...mockOptions, query: { client_id: 'malicious-client', custom_param: 'value', }, }; const url = await createAuthorizeUrl(baseUrl, optionsWithConflict); const params = new URL(url).searchParams; // Standard param should override query param expect(params.get('client_id')).toBe(mockOptions.clientId); // Custom param should be preserved expect(params.get('custom_param')).toBe('value'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/vast-dogs-make.md(1 hunks)packages/oidc-client/src/lib/authorize.request.ts(1 hunks)packages/oidc-client/src/lib/authorize.request.utils.test.ts(0 hunks)packages/oidc-client/src/lib/authorize.request.utils.ts(2 hunks)packages/sdk-effects/oidc/src/lib/authorize.effects.ts(1 hunks)packages/sdk-effects/oidc/src/lib/authorize.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/oidc-client/src/lib/authorize.request.utils.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/oidc-client/src/lib/authorize.request.ts (1)
packages/oidc-client/src/lib/authorize.request.utils.ts (1)
createAuthorizeUrlµ(95-119)
packages/sdk-effects/oidc/src/lib/authorize.test.ts (2)
packages/sdk-types/src/lib/authorize.types.ts (1)
GenerateAndStoreAuthUrlValues(44-48)packages/sdk-effects/oidc/src/lib/authorize.effects.ts (1)
createAuthorizeUrl(23-60)
packages/oidc-client/src/lib/authorize.request.utils.ts (1)
packages/sdk-types/src/lib/authorize.types.ts (1)
GetAuthorizationUrlOptions(19-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pr
- GitHub Check: Mend Code Security Check
🔇 Additional comments (5)
.changeset/vast-dogs-make.md (1)
1-6: LGTM!The changeset correctly documents this patch-level feature addition for both affected packages.
packages/oidc-client/src/lib/authorize.request.utils.ts (3)
15-15: LGTM! Type simplification improves data flow.Removing
OidcConfigfrom theBuildAuthorizationDatatuple is a good refactoring that reduces redundancy. All necessary config fields are now embedded directly in theGetAuthorizationUrlOptionsobject.
25-44: LGTM! Config fields correctly embedded in options.The function now constructs authorization options by extracting necessary fields from
configand embedding them directly in the options object, which aligns with the simplified tuple structure.
95-98: LGTM! Function signature correctly updated.The removal of the
configparameter is appropriate since all necessary data is now available in theoptionsobject. The signature change is consistent with the updatedBuildAuthorizationDatatype.packages/oidc-client/src/lib/authorize.request.ts (1)
40-40: LGTM! Call site correctly updated.The function call correctly reflects the updated
createAuthorizeUrlµsignature, passing onlyurlandoptionsparameters.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (18.52%) 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 #471 +/- ##
==========================================
- Coverage 18.52% 18.52% -0.01%
==========================================
Files 138 138
Lines 27402 27401 -1
Branches 963 963
==========================================
- Hits 5076 5075 -1
Misses 22326 22326
🚀 New features to boost your workflow:
|
@forgerock/davinci-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: |
|
Deployed 7e23c86 to https://ForgeRock.github.io/ping-javascript-sdk/pr-471/7e23c86eab3d412b13139bcc364dfe7c1348d820 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.0 KB, -100.0%) 📊 Minor Changes📉 @forgerock/oidc-client - 22.9 KB (-0.0 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ancheetah
force-pushed
the
SDKS-4445-oidc-query-params
branch
from
52c2b48 to
ef4ab6f
Compare
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4445
Description
Bug fix. Appends query params to authorization url when provided in options to
oidcClient.tokens.getoroidcClient.authorize. Adds unit test.Includes patch changeset
Summary by CodeRabbit
New Features
Tests
Chores