Conversation
witrisna
requested review from
spetrov and
vibhorgoswami
and removed request for
vibhorgoswami
📝 WalkthroughWalkthroughAdds RFC 8628 device authorization grant support: constants and models, an OidcDeviceClient with polling and authorize/user helpers, OIDC module and Journey wiring for device verification completion, request builder, broad unit/integration tests, and a small .gitignore update. ChangesRFC 8628 Device Authorization Grant Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.kt (1)
265-265:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential exception when merging uninitialized configs.
Line 265 directly accesses
other.openId, which will throwUninitializedPropertyAccessExceptionifother._openIdisnull. This differs from the defensive check fortokenStorageon Line 269.Consider adding a guard to prevent exceptions when cloning/merging configs before initialization:
🛡️ Suggested defensive check
operator fun plusAssign(other: OidcClientConfig) { - this.openId = other.openId + other._openId?.let { this.openId = it } this.refreshThreshold = other.refreshThreshold🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.kt` at line 265, The assignment this.openId = other.openId can throw UninitializedPropertyAccessException when other._openId is null; guard the copy by checking other._openId (or the equivalent initialization flag) before assigning so it mirrors the defensive check used for tokenStorage on tokenStorage lines—only set this.openId when other._openId is non-null/initialized during the merge/clone operation.
🧹 Nitpick comments (2)
journey/src/main/kotlin/com/pingidentity/journey/module/Oidc.kt (1)
70-73: ⚡ Quick winConsider simplifying session resolution logic.
The nested fallback logic works correctly but is difficult to parse at a glance. The multi-line conditional with two possible fallbacks could be expressed more clearly.
♻️ Optional refactor for clarity
- val existingSession = - if (success.session.value.isEmpty()) journey.session() - ?: success.session - else success.session + // Use journey session if success session is empty, otherwise use success session + val existingSession = + if (success.session.value.isEmpty()) { + journey.session() ?: success.session + } else { + success.session + }🤖 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 `@journey/src/main/kotlin/com/pingidentity/journey/module/Oidc.kt` around lines 70 - 73, The current nested conditional assigning existingSession is hard to read; replace it with a single clear expression that first prefers success.session when it has a value and otherwise falls back to journey.session() with a final fallback to success.session, e.g. set existingSession = success.session.takeIf { it.value.isNotEmpty() } ?: journey.session() ?: success.session, or use an explicit if/else: if (success.session.value.isNotEmpty()) success.session else journey.session() ?: success.session; update the assignment of existingSession accordingly to improve readability.foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.kt (1)
375-387: 💤 Low valueConsider strengthening the storage key verification.
The test comment states it verifies the default storage key is
com_pingidentity_sdk_v1_device_tokens, but the test only checks thatconfigis not null. While the current test ensures the code compiles, it doesn't actually verify the storage key value.Consider either:
- Accessing and asserting the actual storage key/filename if exposed by the API, or
- Updating the test comment to accurately reflect what is being verified (that the config initializes successfully with default settings).
🤖 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 `@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.kt` around lines 375 - 387, The test currently only asserts clientWithDefault.config is not null but claims to verify the default storage key; update the test to either (A) directly assert the storage key/filename on the created config (e.g., read clientWithDefault.config.storageOption or the exposed storageKey property and assert it equals "com_pingidentity_sdk_v1_device_tokens" or the library constant), or (B) if the API does not expose the storage key, change the test name/comment to state it only verifies the config initializes (keep OidcDeviceClient and clientWithDefault references), so the test and comment accurately reflect what is being validated.
🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`:
- Around line 156-159: The code currently always uses USER_CODE_CAMEL
("userCode") when building the OIDC request; change the logic in the OidcRequest
code that sets the device/user code query parameter to choose between USER_CODE
("user_code") and USER_CODE_CAMEL ("userCode") based on whether the endpoint
path starts with "/as/": compute paramName = if
(endpoint.path.startsWith("/as/")) USER_CODE else USER_CODE_CAMEL and use
paramName wherever the query parameter is added (replace the unconditional use
of USER_CODE_CAMEL).
- Around line 168-170: The code assumes deviceAuthEndpoint ends with
AS_DEVICE_AUTHORIZATION_PATH before calling removeSuffix; add a validation step
that checks deviceAuthEndpoint.endsWith(AS_DEVICE_AUTHORIZATION_PATH) (or
matches the expected PingOne/custom domain regex) and handle failure explicitly
(throw IllegalArgumentException or log and abort) before computing baseUrl and
setting request.url; reference deviceAuthEndpoint, AS_DEVICE_AUTHORIZATION_PATH,
baseUrl and request.url when adding the check so malformed endpoints don't
produce an incorrect "$baseUrl/applications/$clientId/deviceFlow" URL.
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.kt`:
- Around line 102-159: The loop in OidcDeviceClient calculates nextPollAt before
delaying and polling, so emitted DeviceFlowStatus.Polling timestamps are
effectively "now"; move the computation of nextPollAt (currently
`System.currentTimeMillis() + (pollInterval * 1000L)`) to just before each
emit(DeviceFlowStatus.Polling(...)) so it reflects the actual future next-poll
time—ensure you recalculate it after any pollInterval change (e.g., in the
ERROR_SLOW_DOWN branch) and after the delay/pollTokenEndpoint call in the
function that calls pollTokenEndpoint.
In `@journey/src/main/kotlin/com/pingidentity/journey/Journey.kt`:
- Around line 103-105: Custom parameters from option.parameters are currently
applied after the Journey control flags and can overwrite reserved flags
(FORCE_AUTH, NO_SESSION); change the merge so reserved keys remain authoritative
by either skipping keys named FORCE_AUTH and NO_SESSION when writing from
option.parameters or by applying option.parameters first and then re-setting the
Journey flags (e.g., ensure this[FORCE_AUTH] and this[NO_SESSION] are assigned
after option.parameters). Locate the loop using option.parameters.forEach and
implement the chosen approach so the reserved flags cannot be clobbered.
---
Outside diff comments:
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.kt`:
- Line 265: The assignment this.openId = other.openId can throw
UninitializedPropertyAccessException when other._openId is null; guard the copy
by checking other._openId (or the equivalent initialization flag) before
assigning so it mirrors the defensive check used for tokenStorage on
tokenStorage lines—only set this.openId when other._openId is
non-null/initialized during the merge/clone operation.
---
Nitpick comments:
In
`@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.kt`:
- Around line 375-387: The test currently only asserts clientWithDefault.config
is not null but claims to verify the default storage key; update the test to
either (A) directly assert the storage key/filename on the created config (e.g.,
read clientWithDefault.config.storageOption or the exposed storageKey property
and assert it equals "com_pingidentity_sdk_v1_device_tokens" or the library
constant), or (B) if the API does not expose the storage key, change the test
name/comment to state it only verifies the config initializes (keep
OidcDeviceClient and clientWithDefault references), so the test and comment
accurately reflect what is being validated.
In `@journey/src/main/kotlin/com/pingidentity/journey/module/Oidc.kt`:
- Around line 70-73: The current nested conditional assigning existingSession is
hard to read; replace it with a single clear expression that first prefers
success.session when it has a value and otherwise falls back to
journey.session() with a final fallback to success.session, e.g. set
existingSession = success.session.takeIf { it.value.isNotEmpty() } ?:
journey.session() ?: success.session, or use an explicit if/else: if
(success.session.value.isNotEmpty()) success.session else journey.session() ?:
success.session; update the assignment of existingSession accordingly to improve
readability.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae248f92-5348-4e07-9e91-086536fa0667
📒 Files selected for processing (21)
.gitignoredavinci/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/DeviceAuthorizationResponse.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/DeviceFlowStatus.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OpenIdConfiguration.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/Oidc.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/DeviceFlowStatusTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.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/Constants.ktjourney/src/main/kotlin/com/pingidentity/journey/Journey.ktjourney/src/main/kotlin/com/pingidentity/journey/module/Oidc.ktjourney/src/test/kotlin/com/pingidentity/journey/JourneyTest.Response.ktjourney/src/test/kotlin/com/pingidentity/journey/JourneyTest.kt
| * The distinction is made by checking whether the path of the endpoint starts with `/as/`: | ||
| * - Starts with `/as/` → custom domain → query param name is `user_code` | ||
| * - Does not start with `/as/` → PingOne format → query param name is `userCode` | ||
| * |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify constant value and check if conditional logic exists elsewhere
# 1. Check the value of USER_CODE_CAMEL constant
rg -n 'USER_CODE_CAMEL\s*=' --type=kotlin
# 2. Search for any path-checking logic related to device authorization
rg -nC3 'startsWith.*"/as/"' --type=kotlin -g '*oidc*'
# 3. Search for usage of both "user_code" and "userCode" parameter names
rg -n '"(user_code|userCode)"' --type=kotlin -g '*oidc*'Repository: ForgeRock/ping-android-sdk
Length of output: 499
🏁 Script executed:
# Read the OidcRequest.kt file to see the full function implementation
head -200 foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt | tail -100Repository: ForgeRock/ping-android-sdk
Length of output: 4389
🏁 Script executed:
# Also check if there's any usage of "user_code" string literal in the codebase
rg -n 'user_code' --type=kotlinRepository: ForgeRock/ping-android-sdk
Length of output: 3644
🏁 Script executed:
# Search for path-checking logic more broadly
rg -n 'startsWith.*as' --type=kotlinRepository: ForgeRock/ping-android-sdk
Length of output: 427
Implementation does not match documentation—missing conditional parameter name logic.
The KDoc (lines 151–158) clearly documents that the query parameter name should differ based on URL format:
- Custom domain (starts with
/as/) →user_code - PingOne format →
userCode
However, line 174 unconditionally uses USER_CODE_CAMEL ("userCode") without any path-based branching. Additionally, tests in JourneyTest.kt, DaVinciTest.kt, and DaVinciTest.kt expect the query parameter to be user_code, not userCode, indicating the implementation is incomplete.
The codebase already defines both USER_CODE ("user_code") and USER_CODE_CAMEL ("userCode") constants, but only USER_CODE_CAMEL is currently used.
Implement the documented conditional logic: check if the endpoint path starts with /as/ and use the appropriate parameter name.
🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`
around lines 156 - 159, The code currently always uses USER_CODE_CAMEL
("userCode") when building the OIDC request; change the logic in the OidcRequest
code that sets the device/user code query parameter to choose between USER_CODE
("user_code") and USER_CODE_CAMEL ("userCode") based on whether the endpoint
path starts with "/as/": compute paramName = if
(endpoint.path.startsWith("/as/")) USER_CODE else USER_CODE_CAMEL and use
paramName wherever the query parameter is added (replace the unconditional use
of USER_CODE_CAMEL).
| val baseUrl = deviceAuthEndpoint.removeSuffix(AS_DEVICE_AUTHORIZATION_PATH) | ||
|
|
||
| request.url = "$baseUrl/applications/$clientId/deviceFlow" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are tests covering different endpoint URL formats
# 1. Find device authorization endpoint usage in tests
rg -n 'device_authorization' --type=kotlin -g '*Test.kt' -A2 -B2
# 2. Look for endpoint format validation
rg -n 'removeSuffix.*device_authorization' --type=kotlin -C3Repository: ForgeRock/ping-android-sdk
Length of output: 8941
🏁 Script executed:
# Find the constant definition and the code context
rg -n 'AS_DEVICE_AUTHORIZATION_PATH' --type=kotlin -B2 -A2Repository: ForgeRock/ping-android-sdk
Length of output: 1196
🏁 Script executed:
# Read the full context around lines 168-170 in OidcRequest.kt
sed -n '150,185p' foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.ktRepository: ForgeRock/ping-android-sdk
Length of output: 1485
🏁 Script executed:
# Check where deviceAuthorizationEndpoint comes from and how it's used
rg -n 'deviceAuthorizationEndpoint' --type=kotlin -B1 -A1 | head -50Repository: ForgeRock/ping-android-sdk
Length of output: 5243
Add validation to confirm the endpoint format matches expected patterns before URL construction.
The code assumes deviceAuthorizationEndpoint ends with /as/device_authorization but doesn't validate this before calling removeSuffix() on line 168. If the endpoint has an unexpected format, the suffix removal fails silently and line 170 constructs an incorrect URL. While the endpoint should come from standards-compliant OpenID Discovery, consider adding a check to validate the endpoint matches the documented PingOne or custom domain format before building the device flow URL.
🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`
around lines 168 - 170, The code assumes deviceAuthEndpoint ends with
AS_DEVICE_AUTHORIZATION_PATH before calling removeSuffix; add a validation step
that checks deviceAuthEndpoint.endsWith(AS_DEVICE_AUTHORIZATION_PATH) (or
matches the expected PingOne/custom domain regex) and handle failure explicitly
(throw IllegalArgumentException or log and abort) before computing baseUrl and
setting request.url; reference deviceAuthEndpoint, AS_DEVICE_AUTHORIZATION_PATH,
baseUrl and request.url when adding the check so malformed endpoints don't
produce an incorrect "$baseUrl/applications/$clientId/deviceFlow" URL.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #199 +/- ##
=============================================
- Coverage 42.64% 42.16% -0.48%
- Complexity 1287 1288 +1
=============================================
Files 312 315 +3
Lines 9447 9604 +157
Branches 1403 1445 +42
=============================================
+ Hits 4029 4050 +21
- Misses 4862 4997 +135
- Partials 556 557 +1
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:
|
| * @param userCode The user code obtained from the device authorization response that needs to be verified. | ||
| * @return The populated [Request] ready for execution. | ||
| */ | ||
| val populateDeviceFlowVerificationRequest: suspend OidcClientConfig.(Request, String) -> Request = |
There was a problem hiding this comment.
Suggestion (critical): The caller already has the full VERIFICATION_URI_COMPLETE URI, but this function doesn't receive it. Instead it reconstructs the target URL by stripping /as/device_authorization from deviceAuthorizationEndpoint. This works for PingOne-format endpoints but silently breaks for AIC/AM endpoints like https://openam-sdks.forgeblocks.com/am/oauth2/realms/root/realms/alpha/device/code — removeSuffix is a no-op there, so the result becomes …/device/code/applications/{clientId}/deviceFlow, which will 404 with no useful error.
Worth noting: the Journey path in journey/module/Oidc.kt already does this correctly — it uses the caller-supplied VERIFICATION_URI_COMPLETE directly as the POST target. Would be nice to bring this in line with that: accept the complete URI as a parameter, use it as the request URL, and extract user_code from it. That way both paths behave consistently and custom-domain/AM endpoints work out of the box.
There was a problem hiding this comment.
This function applies to DaVinci Environment only. PingOne DaVinci uses specific URL to handle the device grant flow. Update the doc to reflect this special handling.
|
|
||
| // PingOne format paths look like "/{tenantId}/as/device_authorization", whereas | ||
| // custom-domain paths look like "/as/device_authorization". | ||
| request.parameter(USER_CODE_CAMEL, userCode) |
There was a problem hiding this comment.
Suggestion (critical): The KDoc just above describes two distinct behaviours — custom-domain paths use user_code (snake_case) while PingOne-format paths use userCode (camelCase). But the code unconditionally uses USER_CODE_CAMEL (userCode) for both cases. The conditional described in the docs was never implemented, so custom-domain deployments would silently send the wrong parameter name and the server would likely ignore or reject the request.
Either the conditional needs to be implemented (check whether the endpoint path starts with /as/ and pick the right constant), or if userCode is actually always correct, the KDoc should be updated to reflect that. Either way the docs and code should agree — happy to help figure out which behaviour is intended!
| // RFC 8628 §3.5: increase interval by 5 s on each slow_down response. | ||
| pollInterval += SLOW_DOWN_INCREMENT_SECONDS | ||
| logger.i("Slow down received; new interval: $pollInterval s") | ||
| emit(DeviceFlowStatus.Polling(pollCount, pollInterval, nextPollAt)) |
There was a problem hiding this comment.
Suggestion: Small timing inconsistency here — nextPollAt gets computed at the top of the loop (line 105) before the pollInterval increment on line 135. So when we emit Polling(pollCount, pollInterval, nextPollAt) after a slow_down, the emitted pollInterval reflects the new longer interval (e.g. 10s) but nextPollAt was computed using the old interval (e.g. 5s from the start of this iteration, which is already in the past). Any UI that drives a countdown from nextPollAt would immediately show 0s remaining.
Recomputing nextPollAt after the increment would fix this:
pollInterval += SLOW_DOWN_INCREMENT_SECONDS
val adjustedNextPollAt = System.currentTimeMillis() + (pollInterval * 1000L)
emit(DeviceFlowStatus.Polling(pollCount, pollInterval, adjustedNextPollAt))| data class OpenIdConfiguration( | ||
| @SerialName("authorization_endpoint") | ||
| val authorizationEndpoint: String = "", | ||
| var authorizationEndpoint: String = "", |
There was a problem hiding this comment.
Suggestion: Changing these from val to var breaks the immutability contract of the data class. The knock-on effect is in OidcClientConfig.clone() / plusAssign: it does this.openId = other.openId, which is a reference copy — so both the original and the clone share the same OpenIdConfiguration instance. Any post-clone mutation of one config's openId fields would affect the other.
The pre-existing val fields made this safe. To keep things mutable for the DSL while keeping clone isolation, you could leave the fields as var but fix plusAssign to copy the instance:
this.openId = other.openId.copy()Or alternatively keep fields val and use copy() in the DSL's openId { } builder.
|
Suggestion (token storage key collision): Worth noting: the test comment in |
| val existingSession = | ||
| if (success.session.value.isEmpty()) journey.session() | ||
| ?: success.session | ||
| else success.session |
There was a problem hiding this comment.
Suggestion: The comment says success.session may be empty due to NoSession or re-running an existing Journey. The fallback chain is: if empty → try journey.session(), else fall back to success.session.
The edge case worth considering: what happens when success.session.value.isEmpty() and journey.session() also returns null? The ?: success.session fallback means we'd land back on the empty session value, then use it as the CSRF token on line 83 — which would send an empty string and get a CSRF validation failure from AM. The error that comes back (line 87's ApiException) should surface it, but it's a confusing error to diagnose.
Might be worth an explicit guard here:
val existingSession = (if (success.session.value.isEmpty()) journey.session() else success.session)
?: return@success SuccessNode(success.input, prepareUser(journey, OidcUser(journey.oidcClient()), success.session as SSOToken))Or at least a log line so it's obvious what happened when the CSRF failure is hit. Totally up to you how you want to handle the nil case!
There was a problem hiding this comment.
Eventually, the device grant will be failed if no session, return ApiException to caller.
| val paths = mockEngine.requestHistory.map { it.url.encodedPath } | ||
| assertTrue(paths.none { it == "/token" }, "token endpoint should not be called in device flow") | ||
| // Token storage must remain empty because exchange was skipped | ||
| assertNull(tokenStorage.get()) |
There was a problem hiding this comment.
Nit/suggestion: This is intentional and correct behaviour — the approving device doesn't get a token, only the requesting device does. But it's the kind of thing that might surprise a future developer. The existing comment on line 625 is good; consider adding a short note that this is by design per RFC 8628 (the approving device completes auth in the browser, the token is held by the requesting device). Makes the intent clear to anyone who stumbles across this and wonders if the assertion is wrong.
| */ | ||
| suspend fun authorize(verificationUriComplete: String) { | ||
| try { | ||
| launch(URL(verificationUriComplete), redirectUri) |
There was a problem hiding this comment.
Suggestion: redirectUri here comes from BrowserLauncher.redirectUri (the global singleton). For a normal OIDC flow that value is set by the caller before launching, but for the device flow the verification URI opens in a browser without a redirect back to the app — so the redirect URI is semantically irrelevant here. Worth a short comment clarifying this is intentional, since AuthTabIntent uses the redirect URI for tab-completion detection and a reader might wonder why we're passing a global default rather than the app's redirectUri.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.kt`:
- Around line 590-593: The assertion in DaVinciTest.kt is using the wrong query
parameter name; update the check that inspects deviceFlowReq.url.parameters to
assert the "user_code" key (not "userCode") is present and equals "WDJB-MJHT" so
the test matches the actual device flow parameter naming used where
deviceFlowReq and mockEngine.requestHistory are examined.
In
`@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.kt`:
- Around line 375-387: The test currently only checks clientWithDefault.config
is non-null; instead assert the actual storage-key contract by inspecting the
config's storage key property on the OidcDeviceClient instance (e.g.,
clientWithDefault.config.storageKey or
clientWithDefault.config.storageOption.fileName / defaultStorageKey depending on
your config shape) and assert it equals "com_pingidentity_sdk_v1_device_tokens";
additionally, to guard against collisions, instantiate a standard OIDC client
(using OidcClient or equivalent) and assert its config.storageKey is different
from the device client's storage key; update the assertions to use assertEquals
for the expected device key and assertNotEquals for the cross-client check.
In `@journey/src/main/kotlin/com/pingidentity/journey/module/Oidc.kt`:
- Line 92: The success log currently prints the device authorization user_code
via logger.i("Device authorization succeeded for user_code=$userCode"); remove
or redact the user_code from that message to avoid logging credential-like
data—update the logger.i call in Oidc.kt (the statement that contains "Device
authorization succeeded for user_code") to a message without $userCode (e.g.,
"Device authorization succeeded" or mask the code) so no raw user_code is
written to logs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ede18e85-dca5-4ae5-820b-92a825912fb8
📒 Files selected for processing (21)
.gitignoredavinci/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/DeviceAuthorizationResponse.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/DeviceFlowStatus.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OpenIdConfiguration.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/Oidc.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/DeviceFlowStatusTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.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/Constants.ktjourney/src/main/kotlin/com/pingidentity/journey/Journey.ktjourney/src/main/kotlin/com/pingidentity/journey/module/Oidc.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 (2)
- .gitignore
- foundation/oidc/src/main/kotlin/com/pingidentity/oidc/DeviceAuthorizationResponse.kt
| // Verify the deviceFlow request has userCode as a query parameter | ||
| val deviceFlowReq = mockEngine.requestHistory.first { it.url.encodedPath == "/tenantId/applications/test/deviceFlow" } | ||
| assertEquals("WDJB-MJHT", deviceFlowReq.url.parameters["userCode"]) | ||
| } |
There was a problem hiding this comment.
Use the correct query parameter key in assertion.
The test checks userCode, but this flow passes user_code. This can fail for the right implementation or lock in the wrong contract.
Proposed fix
- assertEquals("WDJB-MJHT", deviceFlowReq.url.parameters["userCode"])
+ assertEquals("WDJB-MJHT", deviceFlowReq.url.parameters["user_code"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Verify the deviceFlow request has userCode as a query parameter | |
| val deviceFlowReq = mockEngine.requestHistory.first { it.url.encodedPath == "/tenantId/applications/test/deviceFlow" } | |
| assertEquals("WDJB-MJHT", deviceFlowReq.url.parameters["userCode"]) | |
| } | |
| // Verify the deviceFlow request has userCode as a query parameter | |
| val deviceFlowReq = mockEngine.requestHistory.first { it.url.encodedPath == "/tenantId/applications/test/deviceFlow" } | |
| assertEquals("WDJB-MJHT", deviceFlowReq.url.parameters["user_code"]) | |
| } |
🤖 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 `@davinci/src/test/kotlin/com/pingidentity/davinci/DaVinciTest.kt` around lines
590 - 593, The assertion in DaVinciTest.kt is using the wrong query parameter
name; update the check that inspects deviceFlowReq.url.parameters to assert the
"user_code" key (not "userCode") is present and equals "WDJB-MJHT" so the test
matches the actual device flow parameter naming used where deviceFlowReq and
mockEngine.requestHistory are examined.
| @Test | ||
| fun `default storage key is com_pingidentity_sdk_v1_device_tokens`() { | ||
| // We verify this by checking the config's storageOption default creates | ||
| // the expected filename. We do so by inspecting that a MemoryStorage override | ||
| // works and that the default is NOT the standard key. | ||
| val clientWithDefault = OidcDeviceClient { | ||
| clientId = "test" | ||
| } | ||
| // The config was created; we can't easily inspect the encrypted storage filename | ||
| // in a unit test, but we verify it compiles and the constant value is correct by | ||
| // accessing the config object. | ||
| assertNotNull(clientWithDefault.config) | ||
| } |
There was a problem hiding this comment.
Assert the storage-key contract instead of only asserting config creation.
Line 376 states a default storage-key requirement, but Lines 386-387 only check non-null config. This won’t catch token-store collisions between standard OIDC and device-flow clients.
Proposed test fix
+import com.pingidentity.storage.EncryptedDataStoreStorageConfig
@@
- fun `default storage key is com_pingidentity_sdk_v1_device_tokens`() {
- // We verify this by checking the config's storageOption default creates
- // the expected filename. We do so by inspecting that a MemoryStorage override
- // works and that the default is NOT the standard key.
- val clientWithDefault = OidcDeviceClient {
- clientId = "test"
- }
- // The config was created; we can't easily inspect the encrypted storage filename
- // in a unit test, but we verify it compiles and the constant value is correct by
- // accessing the config object.
- assertNotNull(clientWithDefault.config)
+ fun `default device storage key is distinct from standard OIDC key`() {
+ val deviceConfig = OidcDeviceClient { clientId = "test" }.config
+ val webConfig = OidcClientConfig()
+
+ val deviceStorage = EncryptedDataStoreStorageConfig().apply(deviceConfig.storageOption)
+ val webStorage = EncryptedDataStoreStorageConfig().apply(webConfig.storageOption)
+
+ assertTrue(deviceStorage.keyAlias != webStorage.keyAlias)
+ assertTrue(deviceStorage.fileName != webStorage.fileName)
}🤖 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
`@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.kt`
around lines 375 - 387, The test currently only checks clientWithDefault.config
is non-null; instead assert the actual storage-key contract by inspecting the
config's storage key property on the OidcDeviceClient instance (e.g.,
clientWithDefault.config.storageKey or
clientWithDefault.config.storageOption.fileName / defaultStorageKey depending on
your config shape) and assert it equals "com_pingidentity_sdk_v1_device_tokens";
additionally, to guard against collisions, instantiate a standard OIDC client
(using OidcClient or equivalent) and assert its config.storageKey is different
from the device client's storage key; update the assertions to use assertEquals
for the expected device key and assertNotEquals for the cross-client check.
| throw ApiException(response.status, response.body()) | ||
| } | ||
|
|
||
| logger.i("Device authorization succeeded for user_code=$userCode") |
There was a problem hiding this comment.
Avoid logging user_code in success logs.
Line 92 logs an authentication flow code. Remove/redact it to reduce credential-like data exposure in logs.
Suggested patch
- logger.i("Device authorization succeeded for user_code=$userCode")
+ logger.i("Device authorization succeeded")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.i("Device authorization succeeded for user_code=$userCode") | |
| logger.i("Device authorization succeeded") |
🤖 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 `@journey/src/main/kotlin/com/pingidentity/journey/module/Oidc.kt` at line 92,
The success log currently prints the device authorization user_code via
logger.i("Device authorization succeeded for user_code=$userCode"); remove or
redact the user_code from that message to avoid logging credential-like
data—update the logger.i call in Oidc.kt (the statement that contains "Device
authorization succeeded for user_code") to a message without $userCode (e.g.,
"Device authorization succeeded" or mask the code) so no raw user_code is
written to logs.
…upport - Implement `OidcDeviceClient` to manage the device authorization flow, including requesting device codes and polling the token endpoint. - Add `DeviceFlowStatus` sealed class to represent the states of the device flow: `Started`, `Polling`, `Success`, `Expired`, `AccessDenied`, and `Failure`. - Update `Oidc` module to detect the presence of `VERIFICATION_URI_COMPLETE` in the shared context to trigger device-code completion instead of the standard browser-redirect flow. - Modify `Journey` and `DaVinci` success logic to skip standard OIDC token exchange and instead perform device code verification when a `user_code` is present. - Implement `populateDeviceFlowVerificationRequest` to construct verification URLs supporting both standard and PingOne-specific tenant path formats. - Update `OpenIdConfiguration` to include `device_authorization_endpoint` and allow manual configuration via a new `openId` DSL block in `OidcClientConfig`. - Support passing custom parameters in `Journey.start` via an updated `Option` class. - Add comprehensive unit tests for device flow polling logic, error handling (e.g., `slow_down`, `expired_token`), and integration within DaVinci and Journey workflows.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt (1)
165-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate endpoint format before stripping suffix.
removeSuffix(AS_DEVICE_AUTHORIZATION_PATH)on line 165 silently produces an incorrectbaseUrlif the endpoint doesn't end with/as/device_authorization. For example, ifdeviceAuthorizationEndpointis a non-standard format or comes from a misconfigured discovery document, line 167 will construct a malformed URL without any error signal.Since the KDoc states this is DaVinci-only and the function expects a specific format, validate the format explicitly.
🛡️ Proposed validation
val deviceAuthEndpoint = openId.deviceAuthorizationEndpoint - // Strip "/as/device_authorization" to obtain the tenant-scoped base URL. + // Strip "/as/device_authorization" to obtain the tenant-scoped base URL. + require(deviceAuthEndpoint.endsWith(AS_DEVICE_AUTHORIZATION_PATH)) { + "DaVinci device flow requires endpoint ending with $AS_DEVICE_AUTHORIZATION_PATH, got: $deviceAuthEndpoint" + } val baseUrl = deviceAuthEndpoint.removeSuffix(AS_DEVICE_AUTHORIZATION_PATH)🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt` at line 165, The code currently computes baseUrl by blindly calling deviceAuthEndpoint.removeSuffix(AS_DEVICE_AUTHORIZATION_PATH); update the logic in OidcRequest (where baseUrl and deviceAuthEndpoint are used) to first validate that deviceAuthEndpoint.endsWith(AS_DEVICE_AUTHORIZATION_PATH) and that it parses as a valid URL; if the check fails, throw an explicit exception (e.g., IllegalArgumentException) or return an error rather than silently removing the suffix. Only call removeSuffix(AS_DEVICE_AUTHORIZATION_PATH) after the positive endsWith check and optionally validate the resulting baseUrl with java.net.URI/URL to ensure it is well-formed.
🧹 Nitpick comments (4)
foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt (1)
151-155: 💤 Low valueKDoc examples imply different URL formats but implementation doesn't differentiate.
The KDoc shows two example transformations (lines 151-155) that differ in path structure (
/{tenantId}/as/...vs/as/...), but both produce the same output format and the code doesn't branch based on path structure. This may confuse readers into thinking the function adapts to different endpoint formats.If both examples always produce
{base}/applications/{clientId}/deviceFlow?userCode=..., consider consolidating to a single representative example or clarifying that the path difference is transparent to the function.🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt` around lines 151 - 155, The KDoc in OidcRequest.kt shows two different input URL patterns but implies different outputs even though the implementation produces the same `{base}/applications/{clientId}/deviceFlow?userCode={userCode}` result; update the KDoc for the function/constructor that documents the device flow URL to either (a) consolidate to a single representative example showing the canonical input→output, or (b) add a clarifying sentence stating that the code treats both `/{tenantId}/as/...` and `/as/...` the same and normalizes them to `{base}/applications/{clientId}/deviceFlow?userCode=...`, so readers aren’t misled about branching behavior in OidcRequest.kt.foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.kt (1)
212-214: 💤 Low valueDocument which property was added or verify it explicitly.
The count incremented from 22 to 23, but the test doesn't verify which new property was added. Based on the PR summary,
OidcClientConfiglikely gained a new property related to device authorization, but without explicit verification, future developers won't know if the count is still accurate.📝 Recommended clarification
Add a comment documenting which property was added:
- //Ensure there are 23 properties in the class for now. + // Ensure there are 23 properties in the class (added device authorization support in SDKS-4784). val clazz: KClass<OidcClientConfig> = OidcClientConfig::class assertEquals(clazz.memberProperties.size, 23)🤖 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 `@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.kt` around lines 212 - 214, The test in OidcClientConfigTest currently only asserts clazz.memberProperties.size == 23; update it to explicitly verify the new property was added by either (a) adding a short comment naming the new property (e.g., device authorization property like "deviceAuthorizationEndpoint" or the exact property name added to OidcClientConfig) above the size assertion, or (b) replacing/augmenting the size-only check with an explicit assertion that OidcClientConfig::class.memberProperties.any { it.name == "<NEW_PROPERTY_NAME>" } is true; reference the class OidcClientConfig and the test variable clazz/memberProperties when making the change so future readers know which property caused the count bump.foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.kt (1)
75-80: ⚡ Quick winCorrect terminal-state KDoc for denied authorization.
Line 78 documents access denial under
Expired, but Lines 145-149 emitDeviceFlowStatus.AccessDenied. Update docs to match runtime behavior.Also applies to: 145-149
🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.kt` around lines 75 - 80, The KDoc in OidcDeviceClient currently lists access denial under DeviceFlowStatus.Expired but the runtime emits DeviceFlowStatus.AccessDenied; update the documentation to list the correct terminal states (Started, Polling, Success, Expired, AccessDenied, Failure) and change the description that currently ties "access denied" to Expired so it refers to DeviceFlowStatus.AccessDenied instead; also ensure any nearby KDoc/comments around the status emission logic in OidcDeviceClient (the block that emits DeviceFlowStatus.AccessDenied) are consistent with the updated docs.foundation/oidc/src/test/kotlin/com/pingidentity/oidc/DeviceFlowStatusTest.kt (1)
125-150: ⚡ Quick winMake the state-count test internally consistent.
Line 125 says “exactly five states,” but Lines 141-147 assert six sealed subclasses (including
Success). Keep one contract to avoid ambiguous failures.Proposed cleanup
- fun `DeviceFlowStatus has exactly five states`() { + fun `DeviceFlowStatus has exactly six sealed states`() { @@ - // Silence unused warning - assertEquals(5, statuses.size) + // Keep assertions aligned with sealed hierarchy contract. + assertEquals(6, sealed.size) }🤖 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 `@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/DeviceFlowStatusTest.kt` around lines 125 - 150, The test name and assertions are inconsistent: update the test's descriptive string from "DeviceFlowStatus has exactly five states" to "DeviceFlowStatus has exactly six states" so it matches the sealed-subclasses assertion (assertEquals(6, sealed.size)); keep the statuses list and the final assertEquals(5, statuses.size) as-is (Success is intentionally omitted), and ensure references to DeviceFlowStatus, DeviceAuthorizationResponse, and the test function name reflect this change.
🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/Oidc.kt`:
- Around line 50-57: deviceUserCode() only handles a stored Uri but callers may
store VERIFICATION_URI_COMPLETE as a String, so update the implementation of
deviceUserCode (on SharedContext) to accept both Uri and String from
getValue<Any?>(VERIFICATION_URI_COMPLETE): if the stored value is a Uri use
getQueryParameter(USER_CODE), if it's a String parse it to a Uri (e.g.
Uri.parse(...)) and then get the USER_CODE query parameter, otherwise return
null; reference functions/fields: deviceUserCode, SharedContext.getValue,
VERIFICATION_URI_COMPLETE, USER_CODE to locate and change the logic.
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.kt`:
- Around line 55-58: The factory OidcDeviceClient(block: OidcClientConfig.() ->
Unit = {}) creates a plain OidcClientConfig which allows device-flow tokens to
collide with other clients; after constructing config (val config =
OidcClientConfig().apply(block)) set a device-specific default token storage key
on that config (e.g. set config.tokenStorageKey to a device-suffixed value or
include "device" in the default key) only when no explicit override was
provided, so device-flow tokens are stored separately; update the factory to
mutate config.tokenStorageKey (or call the appropriate setter) before returning
OidcDeviceClient(config).
- Around line 99-104: The code uses deviceAuthResponse.interval directly as
pollInterval before calling delay, which can be zero or negative; clamp
pollInterval to a safe range (e.g., min 1 and a reasonable max like 60) before
each delay call to avoid rapid or invalid polling. Update the loop in
OidcDeviceClient (where pollInterval is set from deviceAuthResponse.interval and
delay(pollInterval * 1000L) is invoked) to compute a clampedPollInterval =
max(1, min(deviceAuthResponse.interval, MAX_POLL_INTERVAL)) and use that for
delay and any logic that updates pollInterval/pollCount.
---
Duplicate comments:
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`:
- Line 165: The code currently computes baseUrl by blindly calling
deviceAuthEndpoint.removeSuffix(AS_DEVICE_AUTHORIZATION_PATH); update the logic
in OidcRequest (where baseUrl and deviceAuthEndpoint are used) to first validate
that deviceAuthEndpoint.endsWith(AS_DEVICE_AUTHORIZATION_PATH) and that it
parses as a valid URL; if the check fails, throw an explicit exception (e.g.,
IllegalArgumentException) or return an error rather than silently removing the
suffix. Only call removeSuffix(AS_DEVICE_AUTHORIZATION_PATH) after the positive
endsWith check and optionally validate the resulting baseUrl with
java.net.URI/URL to ensure it is well-formed.
---
Nitpick comments:
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.kt`:
- Around line 151-155: The KDoc in OidcRequest.kt shows two different input URL
patterns but implies different outputs even though the implementation produces
the same `{base}/applications/{clientId}/deviceFlow?userCode={userCode}` result;
update the KDoc for the function/constructor that documents the device flow URL
to either (a) consolidate to a single representative example showing the
canonical input→output, or (b) add a clarifying sentence stating that the code
treats both `/{tenantId}/as/...` and `/as/...` the same and normalizes them to
`{base}/applications/{clientId}/deviceFlow?userCode=...`, so readers aren’t
misled about branching behavior in OidcRequest.kt.
In `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.kt`:
- Around line 75-80: The KDoc in OidcDeviceClient currently lists access denial
under DeviceFlowStatus.Expired but the runtime emits
DeviceFlowStatus.AccessDenied; update the documentation to list the correct
terminal states (Started, Polling, Success, Expired, AccessDenied, Failure) and
change the description that currently ties "access denied" to Expired so it
refers to DeviceFlowStatus.AccessDenied instead; also ensure any nearby
KDoc/comments around the status emission logic in OidcDeviceClient (the block
that emits DeviceFlowStatus.AccessDenied) are consistent with the updated docs.
In
`@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/DeviceFlowStatusTest.kt`:
- Around line 125-150: The test name and assertions are inconsistent: update the
test's descriptive string from "DeviceFlowStatus has exactly five states" to
"DeviceFlowStatus has exactly six states" so it matches the sealed-subclasses
assertion (assertEquals(6, sealed.size)); keep the statuses list and the final
assertEquals(5, statuses.size) as-is (Success is intentionally omitted), and
ensure references to DeviceFlowStatus, DeviceAuthorizationResponse, and the test
function name reflect this change.
In
`@foundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.kt`:
- Around line 212-214: The test in OidcClientConfigTest currently only asserts
clazz.memberProperties.size == 23; update it to explicitly verify the new
property was added by either (a) adding a short comment naming the new property
(e.g., device authorization property like "deviceAuthorizationEndpoint" or the
exact property name added to OidcClientConfig) above the size assertion, or (b)
replacing/augmenting the size-only check with an explicit assertion that
OidcClientConfig::class.memberProperties.any { it.name == "<NEW_PROPERTY_NAME>"
} is true; reference the class OidcClientConfig and the test variable
clazz/memberProperties when making the change so future readers know which
property caused the count bump.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c49ada04-537f-4fc8-86b5-bd054a5c7042
📒 Files selected for processing (21)
.gitignoredavinci/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/DeviceAuthorizationResponse.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/DeviceFlowStatus.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClientConfig.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/OpenIdConfiguration.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/Oidc.ktfoundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/OidcRequest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/DeviceFlowStatusTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcClientConfigTest.ktfoundation/oidc/src/test/kotlin/com/pingidentity/oidc/OidcDeviceClientTest.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/Constants.ktjourney/src/main/kotlin/com/pingidentity/journey/Journey.ktjourney/src/main/kotlin/com/pingidentity/journey/module/Oidc.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 (2)
- .gitignore
- journey/src/main/kotlin/com/pingidentity/journey/Constants.kt
| * Returns the `user_code` query parameter from the [VERIFICATION_URI_COMPLETE] URI stored in | ||
| * this context, or `null` if the URI is absent or carries no `user_code`. | ||
| * | ||
| * A non-null return value signals that the current flow is a device-code completion flow and | ||
| * that the normal browser-redirect authorization step should be skipped. | ||
| */ | ||
| fun SharedContext.deviceUserCode(): String? = | ||
| getValue<Uri>(VERIFICATION_URI_COMPLETE)?.getQueryParameter(USER_CODE) |
There was a problem hiding this comment.
deviceUserCode() only handles Uri, but the documented usage passes String.
If callers provide VERIFICATION_URI_COMPLETE as a string (as shown in the KDoc example), device mode is not detected and the module falls back to normal auth flow.
💡 Suggested fix
fun SharedContext.deviceUserCode(): String? =
- getValue<Uri>(VERIFICATION_URI_COMPLETE)?.getQueryParameter(USER_CODE)
+ (
+ getValue<Uri>(VERIFICATION_URI_COMPLETE)
+ ?: getValue<String>(VERIFICATION_URI_COMPLETE)?.let(Uri::parse)
+ )?.getQueryParameter(USER_CODE)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Returns the `user_code` query parameter from the [VERIFICATION_URI_COMPLETE] URI stored in | |
| * this context, or `null` if the URI is absent or carries no `user_code`. | |
| * | |
| * A non-null return value signals that the current flow is a device-code completion flow and | |
| * that the normal browser-redirect authorization step should be skipped. | |
| */ | |
| fun SharedContext.deviceUserCode(): String? = | |
| getValue<Uri>(VERIFICATION_URI_COMPLETE)?.getQueryParameter(USER_CODE) | |
| * Returns the `user_code` query parameter from the [VERIFICATION_URI_COMPLETE] URI stored in | |
| * this context, or `null` if the URI is absent or carries no `user_code`. | |
| * | |
| * A non-null return value signals that the current flow is a device-code completion flow and | |
| * that the normal browser-redirect authorization step should be skipped. | |
| */ | |
| fun SharedContext.deviceUserCode(): String? = | |
| ( | |
| getValue<Uri>(VERIFICATION_URI_COMPLETE) | |
| ?: getValue<String>(VERIFICATION_URI_COMPLETE)?.let(Uri::parse) | |
| )?.getQueryParameter(USER_CODE) |
🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/module/Oidc.kt` around
lines 50 - 57, deviceUserCode() only handles a stored Uri but callers may store
VERIFICATION_URI_COMPLETE as a String, so update the implementation of
deviceUserCode (on SharedContext) to accept both Uri and String from
getValue<Any?>(VERIFICATION_URI_COMPLETE): if the stored value is a Uri use
getQueryParameter(USER_CODE), if it's a String parse it to a Uri (e.g.
Uri.parse(...)) and then get the USER_CODE query parameter, otherwise return
null; reference functions/fields: deviceUserCode, SharedContext.getValue,
VERIFICATION_URI_COMPLETE, USER_CODE to locate and change the logic.
| fun OidcDeviceClient(block: OidcClientConfig.() -> Unit = {}): OidcDeviceClient { | ||
| val config = OidcClientConfig().apply(block) | ||
| return OidcDeviceClient(config) | ||
| } |
There was a problem hiding this comment.
Set a device-specific default token storage key in the factory.
Line 56 creates a plain OidcClientConfig with default storage behavior. This allows device-flow tokens and browser-flow tokens to overwrite each other when both clients are used against the same issuer and no override is provided.
🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.kt`
around lines 55 - 58, The factory OidcDeviceClient(block: OidcClientConfig.() ->
Unit = {}) creates a plain OidcClientConfig which allows device-flow tokens to
collide with other clients; after constructing config (val config =
OidcClientConfig().apply(block)) set a device-specific default token storage key
on that config (e.g. set config.tokenStorageKey to a device-suffixed value or
include "device" in the default key) only when no explicit override was
provided, so device-flow tokens are stored separately; update the factory to
mutate config.tokenStorageKey (or call the appropriate setter) before returning
OidcDeviceClient(config).
| var pollInterval = deviceAuthResponse.interval | ||
| var pollCount = 0 | ||
|
|
||
| while (System.currentTimeMillis() < expiresAt) { | ||
| delay(pollInterval * 1000L) | ||
|
|
There was a problem hiding this comment.
Clamp the poll interval before calling delay(...).
Line 99 trusts server-provided interval directly. A 0 or negative value can cause rapid-fire polling or fail the flow due to invalid delay, which is a reliability and endpoint-load risk.
Proposed fix
- var pollInterval = deviceAuthResponse.interval
+ var pollInterval = deviceAuthResponse.interval.coerceAtLeast(1)🤖 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 `@foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcDeviceClient.kt`
around lines 99 - 104, The code uses deviceAuthResponse.interval directly as
pollInterval before calling delay, which can be zero or negative; clamp
pollInterval to a safe range (e.g., min 1 and a reasonable max like 60) before
each delay call to avoid rapid or invalid polling. Update the loop in
OidcDeviceClient (where pollInterval is set from deviceAuthResponse.interval and
delay(pollInterval * 1000L) is invoked) to compute a clampedPollInterval =
max(1, min(deviceAuthResponse.interval, MAX_POLL_INTERVAL)) and use that for
delay and any logic that updates pollInterval/pollCount.
2 participants
JIRA Ticket
SDKS-4784
Description
OidcDeviceClientto manage the device authorization flow, including requesting device codes and polling the token endpoint.DeviceFlowStatussealed class to represent the states of the device flow:Started,Polling,Success,Expired,AccessDenied, andFailure.Oidcmodule to detect the presence ofVERIFICATION_URI_COMPLETEin the shared context to trigger device-code completion instead of the standard browser-redirect flow.JourneyandDaVincisuccess logic to skip standard OIDC token exchange and instead perform device code verification when auser_codeis present.populateDeviceFlowVerificationRequestto construct verification URLs supporting both standard and PingOne-specific tenant path formats.OpenIdConfigurationto includedevice_authorization_endpointand allow manual configuration via a newopenIdDSL block inOidcClientConfig.Journey.startvia an updatedOptionclass.slow_down,expired_token), and integration within DaVinci and Journey workflows.Summary by CodeRabbit
New Features
Tests
Chores