Conversation
…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.
witrisna
requested review from
spetrov and
vibhorgoswami
and removed request for
vibhorgoswami
📝 WalkthroughWalkthroughThis PR implements OAuth 2.0 Device Authorization Grant (RFC 8628) support, introducing a state-machine-based device client, configuration updates, OIDC and Journey module integration, and comprehensive test coverage for device-initiated authentication flows. ChangesRFC 8628 Device Authorization Grant Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
| while (System.currentTimeMillis() < expiresAt) { | ||
| // Capture next-poll time before the delay so the emitted value reflects when the | ||
| // *next* attempt will occur, not when the current delay ends. | ||
| val nextPollAt = System.currentTimeMillis() + (pollInterval * 1000L) | ||
| delay(pollInterval * 1000L) | ||
|
|
||
| if (System.currentTimeMillis() >= expiresAt) { | ||
| logger.i("Device code expired (wall-clock)") | ||
| emit(DeviceFlowStatus.Expired) | ||
| return@flow | ||
| } | ||
|
|
||
| pollCount++ | ||
| logger.i("Polling token endpoint (attempt $pollCount)") | ||
|
|
||
| val tokenResponse = | ||
| pollTokenEndpoint(deviceAuthResponse.deviceCode, config.openId.tokenEndpoint) | ||
|
|
||
| when { | ||
| tokenResponse.isSuccess -> { | ||
| val token = json.decodeFromString<Token>(tokenResponse.body) | ||
| config.tokenStorage.save(token) | ||
| logger.i("Device flow succeeded") | ||
| emit(DeviceFlowStatus.Success(OidcUser(config))) | ||
| return@flow | ||
| } | ||
|
|
||
| tokenResponse.error == ERROR_AUTHORIZATION_PENDING -> { | ||
| emit(DeviceFlowStatus.Polling(pollCount, pollInterval, nextPollAt)) | ||
| } | ||
|
|
||
| tokenResponse.error == ERROR_SLOW_DOWN -> { | ||
| // 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)) | ||
| } | ||
|
|
||
| tokenResponse.error == ERROR_EXPIRED_TOKEN -> { | ||
| logger.i("Device code expired") | ||
| emit(DeviceFlowStatus.Expired) | ||
| return@flow | ||
| } | ||
|
|
||
| tokenResponse.error == ERROR_ACCESS_DENIED -> { | ||
| logger.i("Device flow access denied") | ||
| emit(DeviceFlowStatus.AccessDenied) | ||
| return@flow | ||
| } | ||
|
|
||
| else -> { | ||
| val message = tokenResponse.error ?: "Unknown token endpoint error" | ||
| logger.w("Device flow failed: $message") | ||
| emit(DeviceFlowStatus.Failure(IllegalStateException(message))) | ||
| return@flow | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Recalculate nextPollAt after polling to reflect the actual next poll time.
The current implementation calculates nextPollAt at line 105 before the delay, but emits it at lines 130 and 137 after both the delay and the poll have completed. By emission time, nextPollAt ≈ System.currentTimeMillis() (approximately now), which is semantically confusing for consumers expecting it to indicate when the next poll will occur (a future timestamp).
Consider moving the calculation to after the poll result handling so that emitted nextPollAt values represent the future time of the next attempt:
val nextPollAt = System.currentTimeMillis() + (pollInterval * 1000L)should be calculated just before each emit(DeviceFlowStatus.Polling(...)) statement, rather than at the top of the loop.
🤖 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 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.
| option.parameters.forEach { (key, value) -> | ||
| this[key] = value | ||
| } |
There was a problem hiding this comment.
Prevent custom parameters from overriding reserved Journey flags.
On Line 103, custom entries are applied after FORCE_AUTH/NO_SESSION, so a key collision can replace those booleans with arbitrary values. Reserve these keys (or set them last) to keep control flags authoritative.
Suggested patch
private fun option(context: SharedContext, block: Option.() -> Unit = {}) {
val option = Option().apply(block)
context.apply {
- FORCE_AUTH to option.forceAuth
- NO_SESSION to option.noSession
- option.parameters.forEach { (key, value) ->
+ option.parameters
+ .filterKeys { it != FORCE_AUTH && it != NO_SESSION }
+ .forEach { (key, value) ->
this[key] = value
- }
+ }
+ FORCE_AUTH to option.forceAuth
+ NO_SESSION to option.noSession
}
}🤖 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/Journey.kt` around lines 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #199 +/- ##
==============================================
- Coverage 44.46% 25.69% -18.77%
+ Complexity 1296 933 -363
==============================================
Files 312 309 -3
Lines 9447 9358 -89
Branches 1403 1411 +8
==============================================
- Hits 4201 2405 -1796
- Misses 4649 6637 +1988
+ Partials 597 316 -281
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.
|
|
||
| // 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!
| 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.
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