SDKS-5026: Device Authorization Grant Feature Testing#202
Open
vibhorgoswami wants to merge 1 commit into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## SDKS-4784 #202 +/- ##
===============================================
- Coverage 42.16% 42.15% -0.02%
Complexity 1288 1288
===============================================
Files 315 315
Lines 9604 9604
Branches 1445 1445
===============================================
- Hits 4050 4049 -1
Misses 4997 4997
- Partials 557 558 +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:
|
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
vibhorgoswami
force-pushed
the
SDKS-5026
branch
2 times, most recently
from
d6c9a7b to
bb07373
Compare
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt (1)
346-364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize Device Auth client during ViewModel bootstrap
daAppliedis loaded and assigned (Line 360), but no SDK instance is built in the same init path. This can leaveoidcDeviceClientout of sync with applied config until a manual selection happens.Suggested fix
withContext(Dispatchers.Main) { customJourneyConfigs = jCustom customDaVinciConfigs = dvCustom customWebConfigs = wCustom customDeviceAuthConfigs = daCustom appliedJourneyConfig = jApplied appliedDaVinciConfig = dvApplied appliedWebConfig = wApplied appliedDeviceAuthConfig = daApplied buildJourneyInstance(jApplied) buildDaVinciInstance(dvApplied) buildWebInstance(wApplied) + buildDeviceAuthClient(daApplied) }🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt` around lines 346 - 364, The device auth applied config is loaded into appliedDeviceAuthConfig but the SDK client isn't initialized during ViewModel bootstrap; call the method that builds the Device Auth SDK instance (e.g., buildDeviceAuthInstance) with daApplied inside the same withContext block after setting appliedDeviceAuthConfig so oidcDeviceClient is initialized and stays in sync with the applied config.
🧹 Nitpick comments (1)
samples/pingsampleapp/src/main/res/values/strings.xml (1)
25-26: 💤 Low valueConsider using consistent terminology in title and subtitle.
The title uses the full term "Device Authorization Grant" while the subtitle abbreviates to "Device Auth". For improved clarity and consistency, consider using the full term in the subtitle as well.
📝 Suggested refinement
- <string name="text_device_authorization_grant_subtitle">Start or verify Device Auth</string> + <string name="text_device_authorization_grant_subtitle">Start or verify Device Authorization</string>🤖 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 `@samples/pingsampleapp/src/main/res/values/strings.xml` around lines 25 - 26, Update the subtitle string to use the full term to match the title: change the value of the resource named text_device_authorization_grant_subtitle so it reads "Start or verify Device Authorization Grant" (matching the wording of text_device_authorization_grant_title) to keep terminology consistent across the UI.
🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt`:
- Around line 207-211: The onClick handler for the Approve with Browser Button
calls context.startActivity(Intent(Intent.ACTION_VIEW,
initialVerificationUri.toUri())) without guarding for ActivityNotFoundException;
wrap the launch in a safe check (use
context.packageManager?.resolveActivity(intent, 0) to ensure an app can handle
the intent or run startActivity inside a try/catch for
ActivityNotFoundException) and only call
onApproveWithBrowser(initialVerificationUri) after a successful launch; on
failure, log or show a user-facing message instead of dismissing UI so the flow
doesn't crash.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/DeviceAuthorizationGrantViewModel.kt`:
- Around line 98-127: In the DeviceFlowStatus.Expired,
DeviceFlowStatus.AccessDenied and is DeviceFlowStatus.Failure branches update
the _uiState to also clear any stale device-auth artifacts: set userCode = ""
(or null if your UI state uses nullable), verificationUri = "" (or null), and
isSuccess = false along with the existing
hasStarted/statusMessage/errorMessage/isLoading changes so previous codes/URIs
and success flags are not retained after terminal error states; modify the calls
to _uiState.update in those branches (the ones shown handling Expired,
AccessDenied and Failure) to include these fields.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt`:
- Around line 470-472: The code compares appliedDeviceAuthConfig.display to
deleted.display and then injects a hard-coded DeviceAuthConfigState; instead,
compare a stable identifier (e.g., appliedDeviceAuthConfig.id or
appliedDeviceAuthConfig.issuer) to deleted.id/issuer and reuse an existing
preset from your presets list before falling back; update the branch around
appliedDeviceAuthConfig, deleted and selectDeviceAuthConfig to search your
presets for a matching config (by id or issuer) and call
selectDeviceAuthConfig(matchingPreset) if found, otherwise clear the selection
or select a safe default from presets rather than instantiating a new hard-coded
DeviceAuthConfigState.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/logout/Logout.kt`:
- Around line 159-173: The aggregate session predicates that drive the “Logout
All” enabled state and the “No Active Sessions” message do not include the
device-auth session (state.oidcDeviceClient); update the aggregate checks (the
functions/expressions that compute whether any session is active and whether
“logout all” should be enabled—e.g., the code in the composable/logic that reads
state.oidcUserSession, state.oidcBackchannel, state.samlSession, etc.) to also
consider state.oidcDeviceClient, and ensure the same updated predicate is used
where the “No Active Sessions” label and the Logout All button enabled/disabled
logic are computed (update the related ViewModel/composable methods such as
listLogoutOptions(), the logout-all handler predicate, and the UI
visibility/enablement checks to include oidcDeviceClient).
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/logout/LogoutViewModel.kt`:
- Around line 62-67: Wrap the logout calls in try/catch/finally so failures
don't prevent onCompleted() or subsequent logouts: in logoutOidcDeviceClient,
call oidcDeviceClient?.user()?.logout() inside a try block, log or handle the
exception in catch, and call onCompleted() in finally; do the same pattern for
the other logout helpers referenced in this file (e.g., the logoutAll sequence
and any methods around lines 69-77) so each logout is resilient and errors don't
stop later calls.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/Token.kt`:
- Around line 115-122: The Tab composable currently hardcodes the label "Auth
Grant" (seen where Tab is created with selected = tokenState.selectedTab ==
TokenType.AUTH_GRANT and onClick calling
tokenViewModel.selectTab(TokenType.AUTH_GRANT) /
tokenViewModel.loadAllTokens()); extract this and other user-facing strings in
the Token composable (also around lines 153-206) into resources by adding
entries like auth_grant, access_token, clear, refresh, revoke to strings.xml and
replace hardcoded Text("Auth Grant") (and other hardcoded Text(...) usages) with
Text(stringResource(R.string.<name>)) to enable i18n consistency.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.kt`:
- Around line 180-196: In authGrantUserInfo, when oidcDeviceClient?.user() is
null the function currently returns without clearing previous values; update the
logic in authGrantUserInfo to call state.update and set authGrantUser = null and
authGrantError = null (or appropriate cleared values) when
oidcDeviceClient?.user() returns null so stale profile data is removed; locate
this in the authGrantUserInfo function and ensure both the null-branch and
existing Result.Failure/Result.Success branches consistently update state via
state.update.
---
Outside diff comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt`:
- Around line 346-364: The device auth applied config is loaded into
appliedDeviceAuthConfig but the SDK client isn't initialized during ViewModel
bootstrap; call the method that builds the Device Auth SDK instance (e.g.,
buildDeviceAuthInstance) with daApplied inside the same withContext block after
setting appliedDeviceAuthConfig so oidcDeviceClient is initialized and stays in
sync with the applied config.
---
Nitpick comments:
In `@samples/pingsampleapp/src/main/res/values/strings.xml`:
- Around line 25-26: Update the subtitle string to use the full term to match
the title: change the value of the resource named
text_device_authorization_grant_subtitle so it reads "Start or verify Device
Authorization Grant" (matching the wording of
text_device_authorization_grant_title) to keep terminology consistent across the
UI.
🪄 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: 12e2be9e-a913-490c-b230-974b82dd3767
📒 Files selected for processing (19)
gradle/libs.versions.tomlsamples/pingsampleapp/build.gradle.ktssamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/DeviceAuthorizationGrantScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/DeviceAuthorizationGrantViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/Env.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/DaVinci.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/DaVinciViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/home/HomeApp.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/logout/Logout.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/logout/LogoutViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/navigation/Navigation.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/Token.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/TokenState.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/TokenViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfile.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.ktsamples/pingsampleapp/src/main/res/values/strings.xml
Comment on lines
+207
to
+211
| Button( | ||
| onClick = { | ||
| context.startActivity(Intent(Intent.ACTION_VIEW, initialVerificationUri.toUri())) | ||
| onApproveWithBrowser(initialVerificationUri) | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify and show the relevant section around the referenced lines
FILE="samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE ==="
# Show around lines 180-250 to capture the full click handler and surrounding context
nl -ba "$FILE" | sed -n '180,250p'Repository: ForgeRock/ping-android-sdk
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE ==="
# Print with line numbers and focus around the referenced section
cat -n "$FILE" | sed -n '180,250p'Repository: ForgeRock/ping-android-sdk
Length of output: 2690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore -S "onApproveWithBrowser\\(" samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt
rg -n --hidden --no-ignore -S "startActivity\\(Intent\\(Intent\\.ACTION_VIEW" samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt
rg -n --hidden --no-ignore -S "resolveActivity\\(" samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt
# Check for similar patterns elsewhere in the repo
rg -n --hidden --no-ignore -S "startActivity\\(Intent\\(Intent\\.ACTION_VIEW" samples/pingsampleapp/src/main/java | head -n 50
rg -n --hidden --no-ignore -S "resolveActivity\\(" samples/pingsampleapp/src/main/java | head -n 50Repository: ForgeRock/ping-android-sdk
Length of output: 237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt"
# Show the top of the file to understand parameters and imports
sed -n '1,120p' "$FILE" | cat -n
# Find where ApproveDeviceScreen is used
rg -n --hidden --no-ignore -S "ApproveDeviceScreen\\(" samples/pingsampleapp/src/main/java | head -n 50
# Search for the onApproveWithBrowser handler implementation (likely in parent screen)
rg -n --hidden --no-ignore -S "onApproveWithBrowser" samples/pingsampleapp/src/main/java | head -n 50Repository: ForgeRock/ping-android-sdk
Length of output: 6935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/DeviceAuthorizationGrantScreen.kt"
sed -n '1,240p' "$FILE" | cat -n
# Focus around the onApproveWithBrowser wiring
rg -n --hidden --no-ignore -S "onApproveWithBrowser\\s*=\\s*\\{" "$FILE" -n || trueRepository: ForgeRock/ping-android-sdk
Length of output: 11160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/DeviceAuthorizationGrantScreen.kt"
# Print around the onApproveWithBrowser wiring and further context
sed -n '220,190p' "$FILE" >/dev/null 2>&1 || true
# The file is likely longer; print a wider range around where it mentions onApproveWithBrowser
cat -n "$FILE" | sed -n '200,260p'
# Search the authgrant sample package for any startActivity/ACTION_VIEW handling
rg -n --hidden --no-ignore -S "startActivity\\(" samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant
rg -n --hidden --no-ignore -S "Intent\\.ACTION_VIEW" samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrantRepository: ForgeRock/ping-android-sdk
Length of output: 2981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate viewmodel / polling logic for device authorization grant approval
rg -n --hidden --no-ignore -S "class DeviceAuthorizationGrantViewModel" -S samples/pingsampleapp/src/main/java
FILE_VM=$(fd -t f "DeviceAuthorizationGrantViewModel.*" samples/pingsampleapp/src/main/java | head -n 5 | head -n 1)
echo "VM file guess: $FILE_VM"
if [ -n "${FILE_VM:-}" ] && [ -f "$FILE_VM" ]; then
echo "=== VM snippet around approve/poll ==="
rg -n --hidden --no-ignore -S "approve|poll|verificationUri|ACTION_VIEW|browser" "$FILE_VM" | head -n 80
sed -n '1,260p' "$FILE_VM" | cat -n | head -n 260
fi
# Search for polling/approval in authgrant package
rg -n --hidden --no-ignore -S "poll|Polling|startPolling|verificationUri|hasStarted|approve" samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant | head -n 200Repository: ForgeRock/ping-android-sdk
Length of output: 14514
Guard browser intent launch to avoid ActivityNotFoundException crashes.
The “Approve with Browser” button directly calls context.startActivity(Intent(Intent.ACTION_VIEW, initialVerificationUri.toUri())) and then onApproveWithBrowser(...). There’s no check or error handling in this flow (the onApproveWithBrowser callback just dismisses UI), so if no app can handle the URI, startActivity can throw and break the flow.
Button(
onClick = {
context.startActivity(Intent(Intent.ACTION_VIEW, initialVerificationUri.toUri()))
onApproveWithBrowser(initialVerificationUri)
},🛠️ Suggested fix
Button(
onClick = {
- context.startActivity(Intent(Intent.ACTION_VIEW, initialVerificationUri.toUri()))
- onApproveWithBrowser(initialVerificationUri)
+ val intent = Intent(Intent.ACTION_VIEW, initialVerificationUri.toUri())
+ val canHandle = intent.resolveActivity(context.packageManager) != null
+ if (canHandle) {
+ context.startActivity(intent)
+ onApproveWithBrowser(initialVerificationUri)
+ }
},
enabled = initialVerificationUri.isNotBlank(),
modifier = Modifier.fillMaxWidth(),🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/ApproveDeviceScreen.kt`
around lines 207 - 211, The onClick handler for the Approve with Browser Button
calls context.startActivity(Intent(Intent.ACTION_VIEW,
initialVerificationUri.toUri())) without guarding for ActivityNotFoundException;
wrap the launch in a safe check (use
context.packageManager?.resolveActivity(intent, 0) to ensure an app can handle
the intent or run startActivity inside a try/catch for
ActivityNotFoundException) and only call
onApproveWithBrowser(initialVerificationUri) after a successful launch; on
failure, log or show a user-facing message instead of dismissing UI so the flow
doesn't crash.
Comment on lines
+98
to
+127
| DeviceFlowStatus.Expired -> { | ||
| _uiState.update { | ||
| it.copy( | ||
| hasStarted = false, | ||
| statusMessage = "", | ||
| errorMessage = "The device code has expired. Please start again.", | ||
| isLoading = false, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| DeviceFlowStatus.AccessDenied -> { | ||
| _uiState.update { | ||
| it.copy( | ||
| hasStarted = false, | ||
| statusMessage = "", | ||
| errorMessage = "Access denied by user.", | ||
| isLoading = false, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| is DeviceFlowStatus.Failure -> { | ||
| _uiState.update { | ||
| it.copy( | ||
| hasStarted = false, | ||
| statusMessage = "", | ||
| errorMessage = status.exception.message ?: "Device authorization failed.", | ||
| isLoading = false, | ||
| ) |
Contributor
There was a problem hiding this comment.
Clear stale device-auth artifacts in terminal error states.
After expiry/denial/failure, the state still retains prior userCode/verificationUri (and potentially isSuccess). That leaves stale data available for follow-up actions and retries.
Suggested fix
DeviceFlowStatus.Expired -> {
_uiState.update {
it.copy(
hasStarted = false,
+ userCode = "",
+ verificationUri = "",
statusMessage = "",
errorMessage = "The device code has expired. Please start again.",
isLoading = false,
+ isSuccess = false,
)
}
}
DeviceFlowStatus.AccessDenied -> {
_uiState.update {
it.copy(
hasStarted = false,
+ userCode = "",
+ verificationUri = "",
statusMessage = "",
errorMessage = "Access denied by user.",
isLoading = false,
+ isSuccess = false,
)
}
}
is DeviceFlowStatus.Failure -> {
_uiState.update {
it.copy(
hasStarted = false,
+ userCode = "",
+ verificationUri = "",
statusMessage = "",
errorMessage = status.exception.message ?: "Device authorization failed.",
isLoading = false,
+ isSuccess = false,
)
}
}📝 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.
Suggested change
| DeviceFlowStatus.Expired -> { | |
| _uiState.update { | |
| it.copy( | |
| hasStarted = false, | |
| statusMessage = "", | |
| errorMessage = "The device code has expired. Please start again.", | |
| isLoading = false, | |
| ) | |
| } | |
| } | |
| DeviceFlowStatus.AccessDenied -> { | |
| _uiState.update { | |
| it.copy( | |
| hasStarted = false, | |
| statusMessage = "", | |
| errorMessage = "Access denied by user.", | |
| isLoading = false, | |
| ) | |
| } | |
| } | |
| is DeviceFlowStatus.Failure -> { | |
| _uiState.update { | |
| it.copy( | |
| hasStarted = false, | |
| statusMessage = "", | |
| errorMessage = status.exception.message ?: "Device authorization failed.", | |
| isLoading = false, | |
| ) | |
| DeviceFlowStatus.Expired -> { | |
| _uiState.update { | |
| it.copy( | |
| hasStarted = false, | |
| userCode = "", | |
| verificationUri = "", | |
| statusMessage = "", | |
| errorMessage = "The device code has expired. Please start again.", | |
| isLoading = false, | |
| isSuccess = false, | |
| ) | |
| } | |
| } | |
| DeviceFlowStatus.AccessDenied -> { | |
| _uiState.update { | |
| it.copy( | |
| hasStarted = false, | |
| userCode = "", | |
| verificationUri = "", | |
| statusMessage = "", | |
| errorMessage = "Access denied by user.", | |
| isLoading = false, | |
| isSuccess = false, | |
| ) | |
| } | |
| } | |
| is DeviceFlowStatus.Failure -> { | |
| _uiState.update { | |
| it.copy( | |
| hasStarted = false, | |
| userCode = "", | |
| verificationUri = "", | |
| statusMessage = "", | |
| errorMessage = status.exception.message ?: "Device authorization failed.", | |
| isLoading = false, | |
| isSuccess = false, | |
| ) | |
| } | |
| } |
🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authgrant/DeviceAuthorizationGrantViewModel.kt`
around lines 98 - 127, In the DeviceFlowStatus.Expired,
DeviceFlowStatus.AccessDenied and is DeviceFlowStatus.Failure branches update
the _uiState to also clear any stale device-auth artifacts: set userCode = ""
(or null if your UI state uses nullable), verificationUri = "" (or null), and
isSuccess = false along with the existing
hasStarted/statusMessage/errorMessage/isLoading changes so previous codes/URIs
and success flags are not retained after terminal error states; modify the calls
to _uiState.update in those branches (the ones shown handling Expired,
AccessDenied and Failure) to include these fields.
Comment on lines
+159
to
+173
| // OIDC Device Client Session | ||
| if (state.oidcDeviceClient) { | ||
| item { | ||
| LogoutOptionCard( | ||
| title = "Device Auth Grant Session", | ||
| description = "Logout from OIDC Device Client authentication", | ||
| onLogout = { | ||
| logoutViewModel.logoutOidcDeviceClient { | ||
| // Refresh the list after logout | ||
| logoutViewModel.listLogoutOptions() | ||
| } | ||
| } | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Include device-auth session in aggregate session checks.
Adding this session type also requires updating the aggregate predicates at Line 87 and Line 176. Right now, “Logout All” can be disabled and “No Active Sessions” can appear even when device-auth session is active.
🛠️ Suggested fix
- val hasActiveSessions = state.journey || state.daVinci || state.oidc
+ val hasActiveSessions = state.journey || state.daVinci || state.oidc || state.oidcDeviceClient
...
- if (!state.journey && !state.daVinci && !state.oidc) {
+ if (!state.journey && !state.daVinci && !state.oidc && !state.oidcDeviceClient) {🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/logout/Logout.kt`
around lines 159 - 173, The aggregate session predicates that drive the “Logout
All” enabled state and the “No Active Sessions” message do not include the
device-auth session (state.oidcDeviceClient); update the aggregate checks (the
functions/expressions that compute whether any session is active and whether
“logout all” should be enabled—e.g., the code in the composable/logic that reads
state.oidcUserSession, state.oidcBackchannel, state.samlSession, etc.) to also
consider state.oidcDeviceClient, and ensure the same updated predicate is used
where the “No Active Sessions” label and the Logout All button enabled/disabled
logic are computed (update the related ViewModel/composable methods such as
listLogoutOptions(), the logout-all handler predicate, and the UI
visibility/enablement checks to include oidcDeviceClient).
Comment on lines
+62
to
+67
| fun logoutOidcDeviceClient(onCompleted: () -> Unit) { | ||
| viewModelScope.launch { | ||
| oidcDeviceClient?.user()?.logout() | ||
| onCompleted() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Make logout callbacks resilient to failures.
If any logout call throws, onCompleted() may never run; in logoutAll, later logout calls may be skipped too.
🛠️ Suggested fix
fun logoutOidcDeviceClient(onCompleted: () -> Unit) {
viewModelScope.launch {
- oidcDeviceClient?.user()?.logout()
- onCompleted()
+ try {
+ oidcDeviceClient?.user()?.logout()
+ } finally {
+ onCompleted()
+ }
}
}
fun logoutAll(onCompleted: () -> Unit) {
viewModelScope.launch {
- // Logout from all active sessions
- journey.journeyUser()?.logout()
- daVinci?.davinciUser()?.logout()
- web?.user()?.logout()
- oidcDeviceClient?.user()?.logout()
- onCompleted()
+ try {
+ runCatching { journey.journeyUser()?.logout() }
+ runCatching { daVinci?.davinciUser()?.logout() }
+ runCatching { web?.user()?.logout() }
+ runCatching { oidcDeviceClient?.user()?.logout() }
+ } finally {
+ onCompleted()
+ }
}
}Also applies to: 69-77
🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/logout/LogoutViewModel.kt`
around lines 62 - 67, Wrap the logout calls in try/catch/finally so failures
don't prevent onCompleted() or subsequent logouts: in logoutOidcDeviceClient,
call oidcDeviceClient?.user()?.logout() inside a try block, log or handle the
exception in catch, and call onCompleted() in finally; do the same pattern for
the other logout helpers referenced in this file (e.g., the logoutAll sequence
and any methods around lines 69-77) so each logout is resilient and errors don't
stop later calls.
Comment on lines
+115
to
+122
| Tab( | ||
| selected = tokenState.selectedTab == TokenType.AUTH_GRANT, | ||
| onClick = { | ||
| tokenViewModel.selectTab(TokenType.AUTH_GRANT) | ||
| tokenViewModel.loadAllTokens() | ||
| }, | ||
| text = { Text("Auth Grant") } | ||
| ) |
Contributor
There was a problem hiding this comment.
Externalize new/edited UI labels to resources.
New/edited user-facing labels are hardcoded in the composable. Please move them to strings.xml and use stringResource(...) for i18n consistency.
Proposed fix
+import androidx.compose.ui.res.stringResource
@@
- text = { Text("Auth Grant") }
+ text = { Text(stringResource(R.string.auth_grant)) }
@@
- text = { Text("Refresh") },
+ text = { Text(stringResource(R.string.refresh)) },
@@
- text = { Text("Revoke") },
+ text = { Text(stringResource(R.string.revoke)) },
@@
- Text(text = "AccessToken")
+ Text(text = stringResource(R.string.access_token))
@@
- Text(text = "Clear")
+ Text(text = stringResource(R.string.clear))
@@
- Text(text = "Refresh")
+ Text(text = stringResource(R.string.refresh))
@@
- Text(text = "Revoke")
+ Text(text = stringResource(R.string.revoke))<!-- samples/pingsampleapp/src/main/res/values/strings.xml -->
<string name="auth_grant">Auth Grant</string>
<string name="access_token">Access token</string>
<string name="clear">Clear</string>
<string name="refresh">Refresh</string>
<string name="revoke">Revoke</string>Also applies to: 153-206
🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/Token.kt`
around lines 115 - 122, The Tab composable currently hardcodes the label "Auth
Grant" (seen where Tab is created with selected = tokenState.selectedTab ==
TokenType.AUTH_GRANT and onClick calling
tokenViewModel.selectTab(TokenType.AUTH_GRANT) /
tokenViewModel.loadAllTokens()); extract this and other user-facing strings in
the Token composable (also around lines 153-206) into resources by adding
entries like auth_grant, access_token, clear, refresh, revoke to strings.xml and
replace hardcoded Text("Auth Grant") (and other hardcoded Text(...) usages) with
Text(stringResource(R.string.<name>)) to enable i18n consistency.
Comment on lines
+180
to
+196
| fun authGrantUserInfo() { | ||
| viewModelScope.launch { | ||
| oidcDeviceClient?.user()?.let { user -> | ||
| when (val result = user.userinfo(false)) { | ||
| is Result.Failure -> | ||
| state.update { s -> | ||
| s.copy(authGrantUser = null, authGrantError = result.value) | ||
| } | ||
|
|
||
| is Result.Success -> { | ||
| state.update { s -> | ||
| s.copy(authGrantUser = result.value, authGrantError = null) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Clear auth-grant state when there is no active device-client user.
When oidcDeviceClient?.user() is null, the function exits without resetting authGrantUser/authGrantError, so stale profile data can remain visible.
🛠️ Suggested fix
fun authGrantUserInfo() {
viewModelScope.launch {
- oidcDeviceClient?.user()?.let { user ->
- when (val result = user.userinfo(false)) {
- is Result.Failure ->
- state.update { s ->
- s.copy(authGrantUser = null, authGrantError = result.value)
- }
-
- is Result.Success -> {
- state.update { s ->
- s.copy(authGrantUser = result.value, authGrantError = null)
- }
- }
- }
- }
+ val user = oidcDeviceClient?.user()
+ if (user == null) {
+ state.update { s ->
+ s.copy(
+ authGrantUser = null,
+ authGrantError = null,
+ showRawAuthGrantUserInfo = false,
+ )
+ }
+ return@launch
+ }
+ when (val result = user.userinfo(false)) {
+ is Result.Failure ->
+ state.update { s ->
+ s.copy(authGrantUser = null, authGrantError = result.value)
+ }
+ is Result.Success ->
+ state.update { s ->
+ s.copy(authGrantUser = result.value, authGrantError = null)
+ }
+ }
}
}🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.kt`
around lines 180 - 196, In authGrantUserInfo, when oidcDeviceClient?.user() is
null the function currently returns without clearing previous values; update the
logic in authGrantUserInfo to call state.update and set authGrantUser = null and
authGrantError = null (or appropriate cleared values) when
oidcDeviceClient?.user() returns null so stale profile data is removed; locate
this in the authGrantUserInfo function and ensure both the null-branch and
existing Result.Failure/Result.Success branches consistently update state via
state.update.
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
witrisna
force-pushed
the
SDKS-4784
branch
2 times, most recently
from
14b1925 to
f8980b3
Compare
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA Ticket
SDKS-5026
Description
Introduces the test feature for Device Authorization Grant. Shows the option for QR code/User Code/Verification URL to authenticate the device.
Summary by CodeRabbit