Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #176 +/- ##
=============================================
- Coverage 46.64% 43.86% -2.78%
+ Complexity 1263 1216 -47
=============================================
Files 306 306
Lines 9215 9221 +6
Branches 1305 1308 +3
=============================================
- Hits 4298 4045 -253
- Misses 4347 4642 +295
+ Partials 570 534 -36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
witrisna
force-pushed
the
SDKS-4231
branch
2 times, most recently
from
1a128a4 to
e52de65
Compare
witrisna
requested review from
george-bafaloukas-forgerock,
spetrov and
vibhorgoswami
Contributor
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds Pushed Authorization Request (PAR) support across the OIDC module and integration tests: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthServer
participant App
rect rgba(0, 128, 255, 0.5)
Note over Client,AuthServer: PAR Flow (par = true)
Client->>AuthServer: POST /par (form: client_id, response_type, scope, redirect_uri, code_challenge, code_challenge_method, ...)
AuthServer-->>Client: 201 Created { "request_uri": "...", "expires_in": ... }
Client->>AuthServer: GET /authorize?request_uri=...&client_id=...
AuthServer-->>Client: 302 Redirect /callback?code=...
Client->>AuthServer: POST /token (grant_type=authorization_code, code_verifier, ...)
AuthServer-->>Client: { access_token, id_token, ... }
end
rect rgba(0, 200, 100, 0.5)
Note over Client,AuthServer: Standard Flow (par = false)
Client->>AuthServer: GET /authorize?client_id=...&response_type=...&scope=...&redirect_uri=...&code_challenge=...&code_challenge_method=...
AuthServer-->>Client: 302 Redirect /callback?code=...
Client->>AuthServer: POST /token (grant_type=authorization_code, code_verifier, ...)
AuthServer-->>Client: { access_token, id_token, ... }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.kt (1)
212-214: Avoid hard-coded reflection property-count assertions.
assertEquals(clazz.memberProperties.size, 22)is fragile and will break on unrelated field additions/removals. Prefer behavior-focused assertions (like the explicit per-property assertions already present below).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.kt` around lines 212 - 214, The test currently uses a brittle reflection count check (clazz.memberProperties.size == 22) which will fail when fields change; remove this hard-coded assertion and instead rely on the explicit per-property assertions already present (or replace it with a behavior-focused check that verifies required property names exist by comparing OidcClientConfig::class.memberProperties.map { it.name } against an explicit list of expected names). Update the test to reference OidcClientConfig and clazz/memberProperties when locating properties but do not assert a fixed numeric count.journey/src/test/kotlin/com/pingidentity/journey/JourneyTest.kt (1)
918-944: Similar index-based verification as DaVinciTest - consider using URL pattern matching.As noted in DaVinciTest review, relying on specific request history indices (
[3]for PAR,[4]for authorize) can be fragile if the flow changes. Consider finding requests by URL pattern for more robust assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@journey/src/test/kotlin/com/pingidentity/journey/JourneyTest.kt` around lines 918 - 944, Test uses brittle index-based lookups on mockEngine.requestHistory (e.g., mockEngine.requestHistory[3] and [4]) to find the PAR and authorize requests; change assertions to locate requests by URL pattern instead: search mockEngine.requestHistory for an entry whose url.toString() or url.encodedPath contains the PAR endpoint (e.g., "/par" or "par") to assign parRequest, and similarly find the authorize request by matching the authorize URL or presence of "request_uri" to assign authorizeRequest, then run the same body and query assertions against those found request objects (use the existing variables parRequest and authorizeRequest to keep rest of assertions unchanged).foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientTest.kt (1)
695-732: Consider asserting the specific error type for PAR failures.The test verifies that the result is
Failure<OidcError>but doesn't check the specific error type or message. SincepopulateRequestthrowsAuthorizeExceptionon PAR failure, consider asserting that the failure wraps this specific exception type.💡 Suggested improvement
val result = oidcClient.token() assertTrue(result is Failure<OidcError>) +assertTrue(result.value is OidcError.Authorize) +// Or verify the error message contains PAR-related info🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientTest.kt` around lines 695 - 732, The test currently only asserts that oidcClient.token() returns Failure<OidcError>; update the assertion to verify the failure wraps the specific AuthorizeException thrown on PAR failure (from populateRequest) — locate the test `token with PAR enabled fails when PAR endpoint returns an error`, call `oidcClient.token()`, then assert the result is a Failure and that its cause or contained error is an instance of `AuthorizeException` (and optionally assert the exception message) to ensure PAR-specific failures are validated; references: OidcClient.token(), populateRequest, AuthorizeException, PARTestAgent.foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt (1)
113-116: Clarifyresponse_modeextraction from pre-existing request URL.The code extracts
response_modefromrequest.url.toUri()before populating the PAR form. This assumes the caller may have pre-set a response_mode on the request URL.This pattern works for the DaVinci flow (which uses
pi.flowresponse mode), but the intent could be clearer. Consider adding a comment explaining when/why the request URL would already have parameters set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt` around lines 113 - 116, Add a short clarifying comment immediately above the form block where request.url.toUri().getQueryParameter(RESPONSE_MODE) is read (in OidcRequest.kt) explaining that we intentionally copy a pre-existing response_mode from the incoming request URL into the PAR form because some callers (e.g., the DaVinci flow using pi.flow response_mode) may have already set it on the request URL; keep the existing extraction logic (RESPONSE_MODE, buildAuthorizeParams(pkce, parameters) { k, v -> put(k, v) }) unchanged, just document the rationale and expected callers so intent is clear to future readers.davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.kt (1)
519-544: Verify request history indices match the actual flow sequence.The test assumes specific indices for request history (
[1]for PAR,[2]for authorize,[4]for token). If the flow sequence changes or additional requests are made, these indices could become incorrect and cause flaky tests.Consider using a more robust approach to find requests by URL pattern rather than relying on exact indices:
💡 Suggested alternative approach
val parRequest = mockEngine.requestHistory.first { it.url.encodedPath == "/par" } val authorizeRequest = mockEngine.requestHistory.first { it.url.encodedPath == "/authorize" } val tokenRequest = mockEngine.requestHistory.first { it.url.encodedPath == "/token" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.kt` around lines 519 - 544, The test is brittle because it indexes mockEngine.requestHistory by fixed positions (parRequest at [1], authorizeRequest at [2], tokenRequest at [4]); change the assignments to locate requests by their URL path instead of indices by searching mockEngine.requestHistory for the entry whose request URL path equals "/par" for parRequest, "/authorize" for authorizeRequest and "/token" for tokenRequest so the assertions (including extracting parBody from parRequest and the existing asserts on client_id, response_type, response_mode, scope, redirect_uri, code_challenge, and code_challenge_method) work regardless of additional or reordered requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.Response.kt`:
- Around line 25-26: The OpenID discovery mock JSON in DaVinciTest.Response.kt
is malformed: the object entries "revocation_endpoint" and
"pushed_authorization_request_endpoint" are missing a separating comma; update
the mocked discovery payload (the string containing "revocation_endpoint" and
"pushed_authorization_request_endpoint") to include a comma between those two
fields so the JSON deserializes correctly for PAR tests.
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`:
- Around line 118-129: The PAR handling currently defaults request_uri to an
empty string when the JSON lacks REQUEST_URI, which can mask server errors; in
the block inside the success branch (where response.body() is parsed and
requestUri is extracted), detect absence of REQUEST_URI (i.e., when
json[REQUEST_URI] or its jsonPrimitive/content is null/blank) and throw an
AuthorizeException with a clear message including the response.status and the
raw response body instead of setting an empty request.parameter; update the
logic around requestUri/REQUEST_URI and the downstream
request.parameter(CLIENT_ID, clientId) placement to only execute when a valid
request_uri is present.
---
Nitpick comments:
In `@davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.kt`:
- Around line 519-544: The test is brittle because it indexes
mockEngine.requestHistory by fixed positions (parRequest at [1],
authorizeRequest at [2], tokenRequest at [4]); change the assignments to locate
requests by their URL path instead of indices by searching
mockEngine.requestHistory for the entry whose request URL path equals "/par" for
parRequest, "/authorize" for authorizeRequest and "/token" for tokenRequest so
the assertions (including extracting parBody from parRequest and the existing
asserts on client_id, response_type, response_mode, scope, redirect_uri,
code_challenge, and code_challenge_method) work regardless of additional or
reordered requests.
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`:
- Around line 113-116: Add a short clarifying comment immediately above the form
block where request.url.toUri().getQueryParameter(RESPONSE_MODE) is read (in
OidcRequest.kt) explaining that we intentionally copy a pre-existing
response_mode from the incoming request URL into the PAR form because some
callers (e.g., the DaVinci flow using pi.flow response_mode) may have already
set it on the request URL; keep the existing extraction logic (RESPONSE_MODE,
buildAuthorizeParams(pkce, parameters) { k, v -> put(k, v) }) unchanged, just
document the rationale and expected callers so intent is clear to future
readers.
In
`@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.kt`:
- Around line 212-214: The test currently uses a brittle reflection count check
(clazz.memberProperties.size == 22) which will fail when fields change; remove
this hard-coded assertion and instead rely on the explicit per-property
assertions already present (or replace it with a behavior-focused check that
verifies required property names exist by comparing
OidcClientConfig::class.memberProperties.map { it.name } against an explicit
list of expected names). Update the test to reference OidcClientConfig and
clazz/memberProperties when locating properties but do not assert a fixed
numeric count.
In `@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientTest.kt`:
- Around line 695-732: The test currently only asserts that oidcClient.token()
returns Failure<OidcError>; update the assertion to verify the failure wraps the
specific AuthorizeException thrown on PAR failure (from populateRequest) —
locate the test `token with PAR enabled fails when PAR endpoint returns an
error`, call `oidcClient.token()`, then assert the result is a Failure and that
its cause or contained error is an instance of `AuthorizeException` (and
optionally assert the exception message) to ensure PAR-specific failures are
validated; references: OidcClient.token(), populateRequest, AuthorizeException,
PARTestAgent.
In `@journey/src/test/kotlin/com/pingidentity/journey/JourneyTest.kt`:
- Around line 918-944: Test uses brittle index-based lookups on
mockEngine.requestHistory (e.g., mockEngine.requestHistory[3] and [4]) to find
the PAR and authorize requests; change assertions to locate requests by URL
pattern instead: search mockEngine.requestHistory for an entry whose
url.toString() or url.encodedPath contains the PAR endpoint (e.g., "/par" or
"par") to assign parRequest, and similarly find the authorize request by
matching the authorize URL or presence of "request_uri" to assign
authorizeRequest, then run the same body and query assertions against those
found request objects (use the existing variables parRequest and
authorizeRequest to keep rest of assertions unchanged).
🪄 Autofix (Beta)
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
Run ID: 9b21a042-4060-4358-b0c0-8c654fe92256
📒 Files selected for processing (14)
davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.Response.ktdavinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/Constants.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OpenIdConfiguration.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcWebClientTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OpenIdConfigurationTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/Response.ktjourney/src/main/kotlin/com/pingidentity/journey/module/Agent.ktjourney/src/test/kotlin/com/pingidentity/journey/JourneyTest.Response.ktjourney/src/test/kotlin/com/pingidentity/journey/JourneyTest.kt
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt (1)
82-82: Minor:UI_LOCATESconstant appears to be a typo forUI_LOCALES.The constant
UI_LOCATES(line 82, referencingConstants.UI_LOCATESat line 27) is used for theui_localesOAuth parameter. While this is functionally correct since the constant value is"ui_locales", the constant name itself appears to be misspelled.This is an existing issue (not introduced in this PR) and doesn't affect runtime behavior, but consider renaming for clarity in a future cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt` at line 82, The constant Constants.UI_LOCATES is a misspelled identifier for the OAuth parameter "ui_locales" (used via onParam in OidcRequest.onParam(UI_LOCATES, it)); rename the constant to Constants.UI_LOCALES (update its declaration and all usages including the reference in OidcRequest) so the identifier matches the parameter name, and keep the constant value "ui_locales" unchanged to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`:
- Line 82: The constant Constants.UI_LOCATES is a misspelled identifier for the
OAuth parameter "ui_locales" (used via onParam in
OidcRequest.onParam(UI_LOCATES, it)); rename the constant to
Constants.UI_LOCALES (update its declaration and all usages including the
reference in OidcRequest) so the identifier matches the parameter name, and keep
the constant value "ui_locales" unchanged to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2f74ef7-3c45-49bb-8e42-04a6ca034b47
📒 Files selected for processing (14)
davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.Response.ktdavinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/Constants.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OpenIdConfiguration.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcWebClientTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OpenIdConfigurationTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/Response.ktjourney/src/main/kotlin/com/pingidentity/journey/module/Agent.ktjourney/src/test/kotlin/com/pingidentity/journey/JourneyTest.Response.ktjourney/src/test/kotlin/com/pingidentity/journey/JourneyTest.kt
✅ Files skipped from review due to trivial changes (3)
- journey/src/test/kotlin/com/pingidentity/journey/JourneyTest.Response.kt
- foundation/oidc/src/main/kotlin/com/pingidentity/oidc/Constants.kt
- foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OpenIdConfigurationTest.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OpenIdConfiguration.kt
- foundation/oidc/src/test/kotlin/com/pingidentity/oidc/Response.kt
- foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.kt
- foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcWebClientTest.kt
- davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.kt
Introduced support for PAR (RFC 9126) to improve security and handle large authorization request payloads by pushing parameters to the server prior to authorization. Key changes: - Added `par` configuration flag to `OidcClientConfig` and updated `OpenIdConfiguration` to include the `pushed_authorization_request_endpoint`. - Refactored authorization request building logic into `buildAuthorizeParams` and updated `populateRequest` to handle both standard and PAR flows. - Implemented PAR logic to POST parameters to the provider and utilize the returned `request_uri` in the subsequent authorization call. - Updated `Agent.kt` in the Journey module to use the centralized `populateRequest` logic, ensuring PAR support is available across modules. - Added comprehensive unit and integration tests in `DaVinciTest`, `JourneyTest`, and `OidcWebClientTest` to verify PAR execution and parameter handling. - Updated license headers to include 2026.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA Ticket
SDKS-4231
Description
Introduced support for PAR (RFC 9126) to improve security and handle large authorization request payloads by pushing parameters to the server prior to authorization.
Key changes:
parconfiguration flag toOidcClientConfigand updatedOpenIdConfigurationto include thepushed_authorization_request_endpoint.buildAuthorizeParamsand updatedpopulateRequestto handle both standard and PAR flows.request_uriin the subsequent authorization call.Agent.ktin the Journey module to use the centralizedpopulateRequestlogic, ensuring PAR support is available across modules.DaVinciTest,JourneyTest, andOidcWebClientTestto verify PAR execution and parameter handling.Summary by CodeRabbit
New Features
Tests