feat(sample-app): integrate OpenID4VP flow and expand sample wallet #2298
Open
Kaushikgupta469 wants to merge 6 commits into
Open
feat(sample-app): integrate OpenID4VP flow and expand sample wallet #2298Kaushikgupta469 wants to merge 6 commits into
Kaushikgupta469 wants to merge 6 commits into
Conversation
…apabilities Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntegrates OpenID4VP into the sample credential wallet: build/dependency updates, OpenID4VP manager, key management and signing, credential-VC bridging and matching, new VP sharing UI and ViewModel, navigation/deeplink updates, keystore policy migration, and supporting strings/logging. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant App as Wallet App
participant Scanner as QR Scanner
participant OVP as OpenID4VPManager
participant Store as Credential Store
participant Verifier as Verifier
User->>App: Tap "Share credentials"
App->>Scanner: Start camera & scan QR
Scanner->>App: QR text (urlEncoded auth request)
App->>OVP: authenticateVerifier(urlEncodedAuthRequest)
OVP->>Verifier: Validate & parse auth request
Verifier-->>OVP: AuthorizationRequest
OVP-->>App: Parsed presentationDefinition
App->>Store: getAllCredentialMetadata()
Store-->>App: List<VCMetadata>
App->>App: MatchingVcsHelper.getVcsMatchingAuthRequest(...)
App-->>User: Show matching credentials UI
User->>App: Select & confirm share
App->>OVP: shareVerifiablePresentation(selectedItems)
OVP->>OVP: buildSelectedVCsMapPlain() & sign VPs
OVP->>Verifier: Submit VP payload
Verifier-->>OVP: VerifierResponse (maybe redirect)
OVP-->>App: onResult(VerifierResponse)
App->>App: Handle redirect → navigate to VPSuccess
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
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 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/MainActivity.kt (1)
30-36:⚠️ Potential issue | 🟠 MajorStartup still skips the new key-policy migration for upgraded installs.
The migration now lives inside
SecureKeystoreManager.initializeKeystore(), butinitializeKeystoreIfNeeded()still returns early wheneverkeys_generatedis alreadytrue. With the new VP share path reachable directly from Home, an upgraded install can enter presentation flow on first launch with the old auth-gated keys and still hitKEY_USER_NOT_AUTHENTICATED. CallinitializeKeystore()unconditionally here and let the manager decide whether regeneration is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/MainActivity.kt` around lines 30 - 36, The startup path in MainActivity.kt currently early-returns from initializeKeystoreIfNeeded() when keys_generated is true, skipping the new key-policy migration; instead, always call SecureKeystoreManager.initializeKeystore() on launch so the manager can decide whether regeneration/migration is needed. Replace or modify the initializeKeystoreIfNeeded() invocation to call keystoreManager.initializeKeystore() unconditionally (or refactor initializeKeystoreIfNeeded() to always invoke SecureKeystoreManager.initializeKeystore()), ensuring the manager handles the keys_generated check and migration logic.
🧹 Nitpick comments (2)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPConstants.kt (1)
4-5: Move user-facing error text to Android string resources.These are UI-visible messages; keeping them in
strings.xmlimproves localization and keeps copy centralized.♻️ Proposed refactor
object OVPConstants { - const val ERR_DECLINED = "The user has declined to share their credentials at this time." - const val ERR_NO_MATCHING_VCS = "No matching credentials found to fulfill the request." + const val ERR_DECLINED = "err_declined" + const val ERR_NO_MATCHING_VCS = "err_no_matching_vcs" }<!-- res/values/strings.xml --> <string name="err_declined">The user has declined to share their credentials at this time.</string> <string name="err_no_matching_vcs">No matching credentials found to fulfill the request.</string>// when rendering message context.getString(R.string.err_declined)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPConstants.kt` around lines 4 - 5, These two user-facing constants in OVPConstants.kt (ERR_DECLINED and ERR_NO_MATCHING_VCS) should be moved into Android string resources and references updated: add <string> entries for err_declined and err_no_matching_vcs in res/values/strings.xml, then update any code that currently uses OVPConstants.ERR_DECLINED or OVPConstants.ERR_NO_MATCHING_VCS to call context.getString(R.string.err_declined) / context.getString(R.string.err_no_matching_vcs) (or pass a Context/Resources to the caller), and finally remove the hardcoded constants from OVPConstants.kt. Ensure usages in UI rendering or error displays (search for ERR_DECLINED and ERR_NO_MATCHING_VCS) are replaced accordingly.sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/IssuerConfiguration.kt (1)
125-131: Consider consolidating duplicated issuer metadata across V1/V2 repositories.
StayProtected(and other issuers) is defined in bothIssuerRepositoryV2andIssuerRepository, which can drift over time. A shared source model with V1/V2 adapters would reduce maintenance risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/IssuerConfiguration.kt` around lines 125 - 131, The duplicate issuer metadata (e.g., the "StayProtected" IssuerConfiguration instance) exists in both IssuerRepositoryV2 and IssuerRepository; extract the canonical issuer definitions into a single shared source (e.g., a new IssuerRepositoryCommon or IssuerConfigurations object) that exposes a map of issuer keys to IssuerConfiguration, then refactor IssuerRepositoryV2 and IssuerRepository to read from that shared source or implement thin adapter methods that transform the shared IssuerConfiguration into the specific V1/V2 shapes expected; update references to the IssuerConfiguration instances (the "StayProtected" entry and any others) so they are defined only once and both repositories use the shared adapter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sample-application/sample-application/app/build.gradle.kts`:
- Line 106: Replace the SNAPSHOT dependency with the released RC artifact to
improve build reproducibility: locate the implementation(...) dependency
declaration that currently references
"io.inji:inji-openid4vp-aar:0.7.0-SNAPSHOT" in the build.gradle.kts and update
the version to "0.7.0-RC1" (i.e., change the string inside the
implementation(...) call); after updating, run your build and tests to confirm
the RC1 artifact is compatible with the project.
- Around line 59-66: The global exclusions in configurations.configureEach are
too broad and may remove transitive deps for test/aux configs; remove only the
two lines that exclude group "io.mosip", module "vcverifier-jar" and group
"com.apicatalog", module "titanium-json-ld" from the
configurations.configureEach block, leaving the org.bouncycastle and bcpkix
excludes in place, and rely on the existing dependency-level exclusions in
inji-vci-client-aar, vcverifier-aar, inji-openid4vp-aar, and pixelpass-aar to
prevent those artifacts from being brought in.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.kt`:
- Around line 307-317: Wrap the call to OpenID4VPManager.sendErrorToVerifier
inside a try/catch (or try/catch/finally) within the coroutineScope.launch block
so any thrown exception is caught and does not cancel the coroutine; ensure that
onDeclineConfirmed() is always invoked (e.g., call it in finally or after catch)
and still attempt to call handleVerifierResponse(verifierResponse,
navController) only if a response was obtained (or handle a null/failed response
path). Modify the coroutine that calls sendErrorToVerifier, referencing
coroutineScope.launch, OpenID4VPManager.sendErrorToVerifier,
handleVerifierResponse, and onDeclineConfirmed to guarantee navigation/decline
proceeds even when sendErrorToVerifier throws.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt`:
- Around line 118-189: The camera setup leaks resources and binds to the wrong
lifecycle; fix by using LocalLifecycleOwner.current as the lifecycle owner
(replace casts of context as LifecycleOwner), switch cameraProviderFuture.get()
to cameraProviderFuture.addListener(...) and perform bindToLifecycle inside that
listener using the lifecycleOwner, move executor creation into remember {
Executors.newSingleThreadExecutor() } and dispose it in a DisposableEffect tied
to the composable, create the BarcodeScanning.getClient(...) outside the
AndroidView factory (e.g., as a remembered val) and close it in a
DisposableEffect, and ensure any created analyzers/use-cases are released when
the DisposableEffect is disposed so bind/unbind and resource cleanup mirror the
pattern in AppNavHost.QrScannerScreen.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/CredentialStoreOVPBridge.kt`:
- Around line 36-40: toVCMetadata currently parses credentialJson into a
JsonObject and sets VCMetadata(rawCBORData = null), which drops MSO_MDOC
credentials because MatchingVcsHelper.buildSelectedVCsMapPlain expects
rawCBORData for mdoc entries; update toVCMetadata to detect when
detectFormat(vcObject) == MSO_MDOC and, if so, preserve the original CBOR
payload by setting VCMetadata.rawCBORData to the original credentialJson (or the
store-provided CBOR bytes when available) and otherwise leave it null; ensure
the change references toVCMetadata, detectFormat, VCMetadata.rawCBORData and
retains existing behavior for non-MSO_MDOC formats so mdoc credentials are not
silently omitted from VP responses.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt`:
- Around line 66-82: The current logic treats lenientInclude (computed from
matchesFormat && !matchesConstraints && constraints?.has("fields") == true) as a
valid match and adds it to shouldInclude, which lets MatchingCredentialsScreen
show credentials that fail verifier constraints; change shouldInclude to only
use strictInclude (i.e., set shouldInclude = strictInclude) so failed constraint
checks are not considered matches, remove lenientInclude from the match
decision, and instead surface such interoperability candidates separately (e.g.,
create a separate flag or list like interopCandidates/interopWarning associated
with descriptorId and vcMetadata.format that the UI can display as a warning)
while keeping the existing Log.w about lenient cases for diagnostics.
- Around line 48-55: The matcher currently only checks format/constraints and
sets hasFormatOrConstraints, which ignores input_descriptor.schema; update
MatchingVcsHelper (the matching routine that reads inputDescriptor, format,
constraints, descriptorId and uses hasFormatOrConstraints) to also evaluate
inputDescriptor.get("schema")/getAsJsonArray("schema") and apply schema-aware
filtering against stored credential types/schema URIs (e.g., match
credential.type or credential.schema.id to the descriptor schema entries); if
schema matches are absent treat the descriptor as restrictive (fail closed)
instead of using the "attach all credentials" fallback—i.e., incorporate schema
checks into the same branch that sets hasFormatOrConstraints and into the
credential-filtering logic currently used where fallback occurs.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.kt`:
- Around line 42-51: The shareVerifiablePresentation function currently launches
a detached CoroutineScope(Dispatchers.IO).launch which causes onResult/onError
to be invoked on a background thread and can break Compose state updates (e.g.,
isSharing, shareErrorMessage); update shareVerifiablePresentation to either be
declared suspend and remove the internal CoroutineScope launch so callers
control the scope, or keep it non-suspending but ensure you switch back to
Dispatchers.Main (e.g., withContext(Dispatchers.Main)) before calling
onResult/onError after sendVP (note sendVP already uses Dispatchers.IO), and
reference shareVerifiablePresentation, sendVP, onResult, onError, isSharing and
shareErrorMessage when making the change.
- Around line 116-118: OpenID4VPManager currently calls
OVPKeyManager.generateKeyPair(mdocKeyType) for MSO_MDOC device authentication
which breaks device-bound mDOC verification; instead persist and reuse the
device key created at issuance: extend the credential metadata to store a device
key reference (alias or persistent id) when issuing the credential, then in the
verification/signing path (where mdocKeyType and mdocKeyPair are set) look up
that alias in your keystore via OVPKeyManager (e.g., a new
OVPKeyManager.getKeyPairByAlias(alias) or equivalent) and use the retrieved
public/private key for DeviceAuthentication signing; remove the
generateKeyPair() call for MSO_MDOC and add null/exists checks and clear error
logging if the referenced key is missing so verification fails cleanly.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPKeyManager.kt`:
- Around line 92-100: The JWS header algorithm is hardcoded to ES256 which
mismatches the RSASSASigner when keyType == OVPKeyType.RSA; update the header
creation in signDeviceAuthentication (in OVPKeyManager) to choose JWSAlgorithm
based on keyType (e.g., ES256 for OVPKeyType.ES256, RS256 for OVPKeyType.RSA)
and keep the signer selection as-is, and ensure any returned signatureAlgorithm
or metadata uses the same chosen algorithm so alg and signer remain consistent.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/viewmodel/OVPViewModel.kt`:
- Around line 24-33: In storeMatchResult and clearMatchResult, remove the
unnecessary viewModelScope.launch wrappers and set _matchingResult.value
synchronously (e.g., _matchingResult.value = result and _matchingResult.value =
null) because MutableStateFlow.value is thread-safe; then remove any
compensating delays or workarounds (such as delay(100) in VPShareScreen) that
were added to mask the race so that navigation reads the updated state
immediately.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/auth/AuthWebViewScreen.kt`:
- Around line 101-110: The handleRedirect function currently uses
startsWith(redirectUri) which accepts prefix lookalikes and treats an empty
redirectUri as matching everything; fix it by rejecting blank/empty redirectUri
early and by parsing both the configured redirectUri and the incoming url with
Uri.parse and comparing their components (scheme, authority/host+port, and path
or pathSegments) for equality instead of prefix matching; update handleRedirect
to return false if redirectUri is null/blank, parse val expected =
Uri.parse(redirectUri) and val actual = Uri.parse(url), then only proceed when
expected.scheme == actual.scheme && expected.authority == actual.authority &&
expected.path == actual.path (or equivalent pathSegments comparison) before
treating it as a redirect.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/SecureKeyStoreManager.kt`:
- Around line 74-83: The migration currently swallows exceptions from
keystore.removeAllKeys() and proceeds to regenerate keys and
markKeysAsGenerated(), which can incorrectly treat migration as successful;
change the flow in the needsKeyPolicyMigration() block so that when
keystore.removeAllKeys() throws you abort the migration (either rethrow the
exception or return/exit the migration function) and do not set
prefs.edit().putBoolean(KEY_KEYS_GENERATED, true)/call markKeysAsGenerated() or
run generateKeyPair*()—ensure the failure is logged and propagated so callers
can retry migration instead of stamping policy version 2.
---
Outside diff comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/MainActivity.kt`:
- Around line 30-36: The startup path in MainActivity.kt currently early-returns
from initializeKeystoreIfNeeded() when keys_generated is true, skipping the new
key-policy migration; instead, always call
SecureKeystoreManager.initializeKeystore() on launch so the manager can decide
whether regeneration/migration is needed. Replace or modify the
initializeKeystoreIfNeeded() invocation to call
keystoreManager.initializeKeystore() unconditionally (or refactor
initializeKeystoreIfNeeded() to always invoke
SecureKeystoreManager.initializeKeystore()), ensuring the manager handles the
keys_generated check and migration logic.
---
Nitpick comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPConstants.kt`:
- Around line 4-5: These two user-facing constants in OVPConstants.kt
(ERR_DECLINED and ERR_NO_MATCHING_VCS) should be moved into Android string
resources and references updated: add <string> entries for err_declined and
err_no_matching_vcs in res/values/strings.xml, then update any code that
currently uses OVPConstants.ERR_DECLINED or OVPConstants.ERR_NO_MATCHING_VCS to
call context.getString(R.string.err_declined) /
context.getString(R.string.err_no_matching_vcs) (or pass a Context/Resources to
the caller), and finally remove the hardcoded constants from OVPConstants.kt.
Ensure usages in UI rendering or error displays (search for ERR_DECLINED and
ERR_NO_MATCHING_VCS) are replaced accordingly.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/IssuerConfiguration.kt`:
- Around line 125-131: The duplicate issuer metadata (e.g., the "StayProtected"
IssuerConfiguration instance) exists in both IssuerRepositoryV2 and
IssuerRepository; extract the canonical issuer definitions into a single shared
source (e.g., a new IssuerRepositoryCommon or IssuerConfigurations object) that
exposes a map of issuer keys to IssuerConfiguration, then refactor
IssuerRepositoryV2 and IssuerRepository to read from that shared source or
implement thin adapter methods that transform the shared IssuerConfiguration
into the specific V1/V2 shapes expected; update references to the
IssuerConfiguration instances (the "StayProtected" entry and any others) so they
are defined only once and both repositories use the shared adapter.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38c0b4cf-e9f9-4be6-83e0-ecb36f5f6666
📒 Files selected for processing (21)
sample-application/sample-application/app/build.gradle.ktssample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/MainActivity.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/navigation/AppNavHost.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/data/OVPData.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/data/VCMetadata.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPSuccessScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/CredentialStoreOVPBridge.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPConstants.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPKeyManager.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/viewmodel/OVPViewModel.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/auth/AuthWebViewScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/credential/CredentialDownloadScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/home/HomeScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialDownloadManager.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/IssuerConfiguration.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/SecureKeyStoreManager.ktsample-application/sample-application/app/src/main/res/values/strings.xml
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/home/HomeScreen.kt (1)
330-376:⚠️ Potential issue | 🟡 MinorUnreachable
falsebranch inwhenexpression.The
whenblock at lines 330-376 hastrue, false ->as the first branch (line 331), which matches bothtrueandfalse. The subsequentfalse ->branch (line 346) is unreachable dead code and will never execute.🐛 Proposed fix
when (isValid) { - true, false -> { + true -> { Icon( imageVector = Icons.Default.CheckCircle, contentDescription = "Verified", tint = Color(0xFF4CAF50), modifier = Modifier.size(14.dp) ) Spacer(modifier = Modifier.width(4.dp)) Text( text = "Valid", style = MaterialTheme.typography.bodySmall, color = Color.White, fontSize = 11.sp ) } false -> { Icon( imageVector = Icons.Default.Info, contentDescription = "Not Verified", tint = Color(0xFFFF9800), modifier = Modifier.size(14.dp) ) Spacer(modifier = Modifier.width(4.dp)) Text( text = "Unverified", style = MaterialTheme.typography.bodySmall, color = Color.White, fontSize = 11.sp ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/home/HomeScreen.kt` around lines 330 - 376, The when expression handling isValid in HomeScreen.kt incorrectly uses "true, false" in the first branch, making the later "false" branch unreachable; update the when on isValid (in the HomeScreen composable) so each branch is distinct—use "true ->" for the Valid UI, "false ->" for the Unverified UI, and "null ->" for the Checking... UI (remove the combined "true, false" pattern).
🧹 Nitpick comments (3)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt (1)
146-153: Fallback assigns all VCs when no format/constraints/schema are specified.When
hasFormatOrConstraintsis false (meaning no descriptor has format, constraint fields, or schema), all credentials are assigned to the first descriptor (lines 149-151). While this may be intentional for permissive verifier requests, it could inadvertently share unrelated credentials.Consider logging a warning or documenting this behavior explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt` around lines 146 - 153, The current fallback branch in MatchingVcsHelper (when hasFormatOrConstraints is false) assigns all credentials from vcList to the first input descriptor (using inputDescriptors[0] and fallbackId) which can unintentionally expose unrelated VCs; update the code in the block that builds matchingVCs[fallbackId] to (1) add a clear warning log via your logger (or processLogger) mentioning fallback assignment of all VCs to the first descriptor (include fallbackId and vcList.size()), and (2) add an inline comment documenting this permissive behavior so future maintainers notice the security/privacy implications; keep the existing assignment logic unless an explicit narrowing is required.sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.kt (1)
263-289: Extract hardcoded dialog strings to string resources.The "Sharing in progress" (line 266), "Please wait while we submit your VP response" (line 271), and "Share failed" (line 281) strings should use
stringResource(R.string.*)for consistency with the rest of the UI and to support localization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.kt` around lines 263 - 289, In MatchingCredentialsScreen replace the hardcoded dialog strings with string resources: change the title "Sharing in progress" to stringResource(R.string.sharing_in_progress), the body "Please wait while we submit your VP response" to stringResource(R.string.sharing_in_progress_message), and the error title "Share failed" to stringResource(R.string.share_failed); add those keys to strings.xml (sharing_in_progress, sharing_in_progress_message, share_failed) and ensure the file imports stringResource (androidx.compose.ui.res.stringResource) so the AlertDialog title/text Text() calls use stringResource(...) instead of literal strings.sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPKeyManager.kt (1)
59-77: Consider narrowing the return type or documenting the mixed-type contract.
generateKeyPairreturnsAny, which requires callers to cast based onkeyType. This is acceptable for internal use, but adding a KDoc comment clarifying thatRSA/ES256returnKeyPairwhileEd25519returnsOctetKeyPairwould improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPKeyManager.kt` around lines 59 - 77, generateKeyPair currently returns Any forcing callers to cast; replace this with a clear typed result by introducing a sealed class (e.g., GeneratedKey) with variants for RSA/ES256 that carry a java.security.KeyPair (e.g., GeneratedKey.KeyPairResult(val keyPair: KeyPair)) and for Ed25519 that carries the OctetKeyPair (e.g., GeneratedKey.OctetKeyPairResult(val key: OctetKeyPair)), update generateKeyPair(keyType: OVPKeyType): GeneratedKey to return the appropriate variant, and add KDoc on the generateKeyPair function and the GeneratedKey sealed class documenting which variant maps to which OVPKeyType (reference symbols: generateKeyPair, OVPKeyType, KeyPair, OctetKeyPair).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt`:
- Around line 219-233: The LaunchedEffect(Unit) inside the if (showErrorDialog)
block (in VPShareScreen) can run multiple times across recompositions; add a
dedicated boolean state (e.g., errorSent or hasSentVerifierError) private to the
composable and set it to true after sending, or move the send logic out of the
UI conditional so you call OpenID4VPManager.sendErrorToVerifier only once;
update the code paths that reference showErrorDialog and LaunchedEffect(Unit) to
check/set this flag (or invoke send logic once) so that
OpenID4VPManager.sendErrorToVerifier /
OpenID4VPExceptions.InvalidTransactionData is not called repeatedly.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.kt`:
- Around line 194-206: The code currently keeps the first device-key alias seen
for a given mdoc docType in the selectedItems loop (in OpenID4VPManager.kt) and
silently ignores later conflicting aliases; change the logic in the processing
loop that uses extractMdocDocType and aliasesByDocType so that when a docType is
already present and the new metadata.deviceKeyAlias (trimmed) is non-blank and
differs from the stored alias, you reject the selection by throwing or returning
an explicit error (e.g., IllegalStateException or a Domain-specific error)
indicating a conflicting device-key alias for that docType; ensure the check
handles null/blank aliases correctly (ignore blank new aliases) and includes the
docType and both aliases in the error message for debugging.
- Around line 42-51: The code reads a live SnapshotStateList on a background
thread; before launching the IO coroutine in shareVerifiablePresentation, create
an immutable snapshot (e.g., val items = selectedItems.toList()) and use that
copy inside the coroutine and when calling sendVP; update sendVP's signature
from accepting SnapshotStateList<Pair<String, VCMetadata>> to List<Pair<String,
VCMetadata>> and adjust its implementation and any call sites (including the
other call path currently passing selectedItems) to accept the immutable List so
no SnapshotStateList is accessed off the main snapshot context.
---
Outside diff comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/home/HomeScreen.kt`:
- Around line 330-376: The when expression handling isValid in HomeScreen.kt
incorrectly uses "true, false" in the first branch, making the later "false"
branch unreachable; update the when on isValid (in the HomeScreen composable) so
each branch is distinct—use "true ->" for the Valid UI, "false ->" for the
Unverified UI, and "null ->" for the Checking... UI (remove the combined "true,
false" pattern).
---
Nitpick comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.kt`:
- Around line 263-289: In MatchingCredentialsScreen replace the hardcoded dialog
strings with string resources: change the title "Sharing in progress" to
stringResource(R.string.sharing_in_progress), the body "Please wait while we
submit your VP response" to
stringResource(R.string.sharing_in_progress_message), and the error title "Share
failed" to stringResource(R.string.share_failed); add those keys to strings.xml
(sharing_in_progress, sharing_in_progress_message, share_failed) and ensure the
file imports stringResource (androidx.compose.ui.res.stringResource) so the
AlertDialog title/text Text() calls use stringResource(...) instead of literal
strings.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt`:
- Around line 146-153: The current fallback branch in MatchingVcsHelper (when
hasFormatOrConstraints is false) assigns all credentials from vcList to the
first input descriptor (using inputDescriptors[0] and fallbackId) which can
unintentionally expose unrelated VCs; update the code in the block that builds
matchingVCs[fallbackId] to (1) add a clear warning log via your logger (or
processLogger) mentioning fallback assignment of all VCs to the first descriptor
(include fallbackId and vcList.size()), and (2) add an inline comment
documenting this permissive behavior so future maintainers notice the
security/privacy implications; keep the existing assignment logic unless an
explicit narrowing is required.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPKeyManager.kt`:
- Around line 59-77: generateKeyPair currently returns Any forcing callers to
cast; replace this with a clear typed result by introducing a sealed class
(e.g., GeneratedKey) with variants for RSA/ES256 that carry a
java.security.KeyPair (e.g., GeneratedKey.KeyPairResult(val keyPair: KeyPair))
and for Ed25519 that carries the OctetKeyPair (e.g.,
GeneratedKey.OctetKeyPairResult(val key: OctetKeyPair)), update
generateKeyPair(keyType: OVPKeyType): GeneratedKey to return the appropriate
variant, and add KDoc on the generateKeyPair function and the GeneratedKey
sealed class documenting which variant maps to which OVPKeyType (reference
symbols: generateKeyPair, OVPKeyType, KeyPair, OctetKeyPair).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0abea1a2-7bea-49fa-82be-3f490ed06136
📒 Files selected for processing (16)
sample-application/sample-application/app/build.gradle.ktssample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/navigation/AppNavHost.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/data/VCMetadata.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPSuccessScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/CredentialStoreOVPBridge.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OVPKeyManager.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/viewmodel/OVPViewModel.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/auth/AuthWebViewScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/credential/CredentialDownloadScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/home/HomeScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialDownloadManager.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/SecureKeyStoreManager.kt
✅ Files skipped from review due to trivial changes (1)
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPSuccessScreen.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/data/VCMetadata.kt
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/credential/CredentialDownloadScreen.kt
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/auth/AuthWebViewScreen.kt
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialDownloadManager.kt
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/viewmodel/OVPViewModel.kt
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/CredentialStoreOVPBridge.kt
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.kt (1)
41-72: Prefer caller-owned coroutine scope for VP sharing.
CoroutineScope(Dispatchers.IO).launchcreates unmanaged work that can outlive the screen passingonResult/onError, so navigation away cannot cancel the 45s operation. Consider accepting a caller/lifecycle scope or returning the launchedJobso the UI can cancel it on disposal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.kt` around lines 41 - 72, The shareVerifiablePresentation function currently creates an internal CoroutineScope(Dispatchers.IO).launch which produces unmanaged work; change its signature to either accept a caller-provided CoroutineScope (e.g., add parameter scope: CoroutineScope) or return the launched Job so callers can cancel it; inside, replace CoroutineScope(Dispatchers.IO).launch with scope.launch(Dispatchers.IO) (or assign and return the Job from that launch), keeping the withTimeout/withContext logic unchanged and ensuring callers (UI/lifecycleScope) can cancel the operation when the screen is destroyed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt`:
- Around line 147-151: When scanningEnabled is toggled off the code calls
analysisUseCase?.clearAnalyzer() and boundCameraProvider?.unbindAll(), but
there's no rebind when scanningEnabled becomes true so the camera never resumes;
update the LaunchedEffect(scanningEnabled) block to handle both states: on false
keep the camera bound or clear only the analyzer (remove
boundCameraProvider?.unbindAll()), or if you prefer unbinding keep it but add a
rebind path when scanningEnabled is true that re-attaches the analyzer and
re-binds use cases via the same setup used in the AndroidView factory (mirror
the bind logic used when the preview/Analyzer is initially created), ensuring
analysisUseCase and boundCameraProvider are restored so QR scanning can resume.
---
Nitpick comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.kt`:
- Around line 41-72: The shareVerifiablePresentation function currently creates
an internal CoroutineScope(Dispatchers.IO).launch which produces unmanaged work;
change its signature to either accept a caller-provided CoroutineScope (e.g.,
add parameter scope: CoroutineScope) or return the launched Job so callers can
cancel it; inside, replace CoroutineScope(Dispatchers.IO).launch with
scope.launch(Dispatchers.IO) (or assign and return the Job from that launch),
keeping the withTimeout/withContext logic unchanged and ensuring callers
(UI/lifecycleScope) can cancel the operation when the screen is destroyed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2984a4b-fa40-4d73-a6b6-17bb94c9baf3
📒 Files selected for processing (2)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/OpenID4VPManager.kt
Kaushikgupta469
force-pushed
the
sample-ovp-feature
branch
3 times, most recently
from
ef54877 to
cb83ced
Compare
✅ Actions performedReview triggered.
|
…eature Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt (1)
519-523:⚠️ Potential issue | 🟠 MajorDon’t allow failed non-type constraints through the fallback.
When constraints target claims other than
type, Line 523 currently allows any format-matched VC even though full constraint evaluation already failed. That can disclose credentials that do not satisfy the verifier request.Suggested fix
if (!hasTypeConstraint) { - // No type constraint in the fields — the constraint fields are about - // other claims. Since format already matches, allow it through. - Log.d("OVP_MATCH", "typeAwareFallback: no type constraint in fields, allowing format-matched VC") - return true + Log.d("OVP_MATCH", "typeAwareFallback: no type constraint in fields, failing closed") + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt` around lines 519 - 523, The fallback in typeAwareFallback currently returns true whenever hasTypeConstraint is false, even if full constraint evaluation failed for non-type claims; change the logic so you only allow format-matched VCs through when hasTypeConstraint is false AND there were no failing non-type constraint checks. Concretely, in the typeAwareFallback function (and where the full constraint evaluation result is computed), replace the unconditional return true at the hasTypeConstraint check with a guard that checks the prior constraint-evaluation result (e.g., a boolean like nonTypeConstraintsPassed or constraintEvaluationFailed) and only return true when that indicates no failed non-type constraints; otherwise return false.sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt (1)
169-242:⚠️ Potential issue | 🟠 MajorSize the camera preview explicitly.
AndroidViewhas no modifier here, so the preview may not occupy the scanner screen reliably. Match the existing QR scanner pattern and make it fill the container.Suggested fix
- AndroidView(factory = { ctx -> + AndroidView( + modifier = Modifier.fillMaxSize(), + factory = { ctx -> val previewView = PreviewView(ctx) previewView.implementationMode = PreviewView.ImplementationMode.COMPATIBLE @@ previewView - }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt` around lines 169 - 242, The AndroidView created in VPShareScreen does not have a Modifier so the camera preview may not fill the scanner area; update the AndroidView invocation to include a layout modifier (e.g., Modifier.fillMaxSize()) and ensure the created PreviewView is sized to match its parent by setting its layout params to match_parent (or equivalent) when you construct previewView inside the AndroidView factory; this ensures PreviewView (the previewView variable used with Preview.Builder and setSurfaceProvider) reliably fills the container when binding cameraProvider in the cameraProviderFuture listener.
🧹 Nitpick comments (2)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt (2)
49-54: Type selection here picks blindly by last index; use the preferred-type helper.
types.getString(types.length() - 1)can return"VerifiableCredential"when the array ordering puts it last (not uncommon for mdoc/JSON-LD variants), which then renders as the generic "Verifiable Credential" label even though a more specific type is available. The resolver already exposesextractPreferredTypefor exactly this case — using it keepsparseCredential["type"]consistent with the display-name logic thatresolveFromJsonuses.Note this method is
privateinCredentialDisplayNameResolver; you'd need to expose it (or add a thin public wrapper) to reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt` around lines 49 - 54, The current type selection in CredentialParser (using types.getString(types.length() - 1) to pick rawType and passing it to addField("type", CredentialDisplayNameResolver.toDisplayName(rawType))) can return the generic "VerifiableCredential"; replace this blind-last-index logic with the resolver's preferred-type helper: call CredentialDisplayNameResolver.extractPreferredType(...) (or add a thin public wrapper around that private method) to obtain the preferred type from the JSONArray/JSONObject and then pass that result into toDisplayName before calling addField("type"). Ensure you update any visibility (make extractPreferredType public or add a small public wrapper) so CredentialParser can call it and remove the getString(...last) usage.
205-249: FourextractPreferredTypeoverloads duplicate the same logic acrossJSONArrayandJsonArray.The two "skip VerifiableCredential, otherwise return last seen" implementations are identical save for the iteration API. Consider collapsing to a single generic helper that takes a
Sequence<String>(orList<String>) of candidate type strings, and have theJSONArray/JsonArrayentry points only handle extraction-to-strings. Reduces the maintenance surface if the preference rule ever changes (e.g., adding more "generic" types to skip).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt` around lines 205 - 249, The four extractPreferredType overloads duplicate the same selection logic; refactor by creating a single helper function (e.g., extractPreferredTypeFromCandidates or extractPreferredType(sequence: Sequence<String>)) that implements the "skip 'VerifiableCredential' and otherwise return last seen" rule from a Sequence/List of Strings, then change the two JSONArray and JsonArray entrypoints (the extractPreferredType(vararg typeArrays: JSONArray?), extractPreferredType(typeArray: JSONArray?), extractPreferredType(vararg typeArrays: JsonArray?), extractPreferredType(typeArray: JsonArray?)) to each convert their elements to a Sequence/List<String> and delegate to that helper; keep the public signatures the same and ensure trimming/empty checks happen before delegation so behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.kt`:
- Around line 195-219: The click handler can leave isSharing true if
OpenID4VPManager.shareVerifiablePresentation throws synchronously or if
handleVerifierResponse throws inside the onResult coroutine; wrap the call to
OpenID4VPManager.shareVerifiablePresentation in a try/catch that sets isSharing
= false, logs the exception and updates shareErrorMessage, and inside the
onResult coroutineScope.launch wrap handleVerifierResponse(...) in its own
try/catch that also sets isSharing = false, logs the error, updates
shareErrorMessage and only clears selectedItems and call onSuccess() on success;
reference TextButton click handler, isSharing,
OpenID4VPManager.shareVerifiablePresentation, coroutineScope.launch,
handleVerifierResponse, selectedItems, shareErrorMessage and onSuccess when
applying the change.
- Around line 111-172: The share button currently enables whenever selectedItems
is non-empty; change this to require coverage of all requested input descriptors
by ensuring at least one selected VC per descriptor key: compute
coveredDescriptors = selectedItems.map { it.first }.toSet() and require
coveredDescriptors.size == matchResult!!.matchingVCs.size (or another
descriptor-count field if present) before enabling the Button and before setting
showConsentDialog in the Button onClick; update the Button's enabled check and
add a final validation before opening showConsentDialog (using selectedItems and
matchResult.matchingVCs) so sharing is blocked unless every descriptor has at
least one selection.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt`:
- Around line 333-355: The code currently uses hasMatchingVCs (any non-empty
match) before navigating; instead require a match for every requested input
descriptor: compute expectedDescriptorCount from the parsed presentation
definition (pd/inputDescriptors) and ensure matchingVcsResult.matchingVCs
contains an entry for each descriptor and that each list is non-empty (or
equivalently matchedDescriptorCount == expectedDescriptorCount and no value
lists are empty) before proceeding; update the condition that currently uses
hasMatchingVCs (and related logs) to this stricter check, keep storing the
result via ovpViewModel.storeMatchResult(matchingVcsResult), and log a clear
message when the check fails so the flow does not continue to consent with
partial matches (see MatchingVcsHelper.getVcsMatchingAuthRequest and
matchingVcsResult.matchingVCs).
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt`:
- Around line 622-668: evaluateFilter currently ignores unknown JSON Schema
keywords which widens matches; add a "fail-closed" guard: collect
filter.keySet() and compare against an allowedKeys set (e.g., "type", "pattern",
"const", "enum", "contains"); if any key is not in allowedKeys return false
immediately. Also apply the same guard recursively for the JsonObject returned
by filter.getAsJsonObject("contains") (or call the same validation before using
evaluatePrimitiveMatch) so nested contains filters with unsupported keywords
also cause failure. Use the existing function name evaluateFilter and the
containsFilter variable to locate where to insert this check.
- Around line 280-287: The current matching logic in MatchingVcsHelper uses
extractSchemaTokens and stringsEquivalent to consider any shared token as a
schema match, which allows generic URI tokens (e.g., "http", "https", "www",
"schema", "vc") to cause false positives; update the matching to skip generic
tokens by adding an isGenericSchemaToken helper (as suggested) and apply it when
iterating descriptorTokens and credentialTokens so that both descriptorToken and
credentialToken are ignored if isGenericSchemaToken(token) returns true before
calling stringsEquivalent, ensuring only meaningful schema tokens drive matches.
- Around line 414-418: The current check in MatchingVcsHelper.kt reads proofType
from vc and returns true when it's null/blank, which accepts format-only
matches; instead, when the verifier requested a proof_type constraint, change
this branch to return false (and log that proof.type is missing) so missing
proof.type fails proof-type matching; locate the block that reads val proofType
= vc.getAsJsonObject("proof")?.get("type")?.asString and replace the
early-accept behavior with an early-fail when a proof_type was requested by the
verifier (use the existing verifier/requested constraints context in the same
method to decide), and update the log message accordingly.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt`:
- Around line 133-139: resolveFromJson currently calls
optString("credentialName") which returns the JSON text for localized-object
values and causes false-positive matches; update resolveFromJson to detect when
credentialName is a JSONObject (check credentialJson.opt("credentialName") is
JSONObject) and, instead of using optString directly, call or reuse
extractCredentialNameFromSubject to parse both plain strings and
localized-object shapes; do the same for the nested vc path (inspect
credentialJson.optJSONObject("vc") and its credentialName field) so localized
credentialName objects don't short-circuit the fallback logic.
---
Duplicate comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt`:
- Around line 169-242: The AndroidView created in VPShareScreen does not have a
Modifier so the camera preview may not fill the scanner area; update the
AndroidView invocation to include a layout modifier (e.g.,
Modifier.fillMaxSize()) and ensure the created PreviewView is sized to match its
parent by setting its layout params to match_parent (or equivalent) when you
construct previewView inside the AndroidView factory; this ensures PreviewView
(the previewView variable used with Preview.Builder and setSurfaceProvider)
reliably fills the container when binding cameraProvider in the
cameraProviderFuture listener.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt`:
- Around line 519-523: The fallback in typeAwareFallback currently returns true
whenever hasTypeConstraint is false, even if full constraint evaluation failed
for non-type claims; change the logic so you only allow format-matched VCs
through when hasTypeConstraint is false AND there were no failing non-type
constraint checks. Concretely, in the typeAwareFallback function (and where the
full constraint evaluation result is computed), replace the unconditional return
true at the hasTypeConstraint check with a guard that checks the prior
constraint-evaluation result (e.g., a boolean like nonTypeConstraintsPassed or
constraintEvaluationFailed) and only return true when that indicates no failed
non-type constraints; otherwise return false.
---
Nitpick comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt`:
- Around line 49-54: The current type selection in CredentialParser (using
types.getString(types.length() - 1) to pick rawType and passing it to
addField("type", CredentialDisplayNameResolver.toDisplayName(rawType))) can
return the generic "VerifiableCredential"; replace this blind-last-index logic
with the resolver's preferred-type helper: call
CredentialDisplayNameResolver.extractPreferredType(...) (or add a thin public
wrapper around that private method) to obtain the preferred type from the
JSONArray/JSONObject and then pass that result into toDisplayName before calling
addField("type"). Ensure you update any visibility (make extractPreferredType
public or add a small public wrapper) so CredentialParser can call it and remove
the getString(...last) usage.
- Around line 205-249: The four extractPreferredType overloads duplicate the
same selection logic; refactor by creating a single helper function (e.g.,
extractPreferredTypeFromCandidates or extractPreferredType(sequence:
Sequence<String>)) that implements the "skip 'VerifiableCredential' and
otherwise return last seen" rule from a Sequence/List of Strings, then change
the two JSONArray and JsonArray entrypoints (the extractPreferredType(vararg
typeArrays: JSONArray?), extractPreferredType(typeArray: JSONArray?),
extractPreferredType(vararg typeArrays: JsonArray?),
extractPreferredType(typeArray: JsonArray?)) to each convert their elements to a
Sequence/List<String> and delegate to that helper; keep the public signatures
the same and ensure trimming/empty checks happen before delegation so behavior
is preserved.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67ddb458-9e47-4757-a7f2-0a4d75c4aae4
📒 Files selected for processing (9)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/navigation/AppNavHost.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/credential/CredentialDownloadScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/credential/CredentialListScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/home/HomeScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.ktsample-application/sample-application/app/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- sample-application/sample-application/app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/home/HomeScreen.kt
- sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ui/credential/CredentialDownloadScreen.kt
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
swatigoel
suggested changes
Apr 23, 2026
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (4)
sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt (1)
158-164:⚠️ Potential issue | 🟡 MinorHandle localized
credentialNameobjects in the Gson overload too.The
JSONObjectoverload now supportscredentialName: { "value": ... }, but thisJsonObjectoverload still only reads primitive strings, so the same credential parsed through Gson falls back to type/docType instead of using the localized display name.🛠️ Proposed fix
fun resolveFromJson(vcObject: JsonObject): String? { - val direct = vcObject.getAsSafeString("credentialName") + val direct = extractCredentialNameFromSubject(vcObject) if (!direct.isNullOrBlank()) return direct val nestedVc = vcObject.getAsSafeObject("vc") - val nestedDirect = nestedVc?.getAsSafeString("credentialName") + val nestedDirect = extractCredentialNameFromSubject(nestedVc) if (!nestedDirect.isNullOrBlank()) return nestedDirect🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt` around lines 158 - 164, In resolveFromJson, extend the JsonObject handling to accept localized credentialName objects (e.g., credentialName: { "value": "..." }) as well as primitive strings: after reading vcObject.getAsSafeString("credentialName") and vcObject.getAsSafeObject("credentialName"), check if the object exists and extract its "value" string (falling back to nested vc handling), and similarly for the nestedVc variable; update the resolution order in resolveFromJson to return the extracted "value" when present before falling back to type/docType.sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt (2)
314-318:⚠️ Potential issue | 🟠 MajorTreat
credentialURI path tokens as generic schema tokens.Schema URLs that only share path components like
credential/credentialscan still match unrelated credential types. Add those to the generic-token denylist so schema matching is driven by meaningful type-specific tokens.Suggested tightening
private fun isGenericSchemaToken(token: String): Boolean { return when (token.trim().lowercase()) { - "http", "https", "www", "schema", "schemas", "vc" -> true + "http", "https", "www", "schema", "schemas", "credential", "credentials", "vc" -> true else -> false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt` around lines 314 - 318, The isGenericSchemaToken function currently treats common host/protocol tokens as generic; extend its denylist to also include path tokens like "credential" and "credentials" so that schema matching ignores those generic path components; update the when-clause in isGenericSchemaToken (referenced by function name) to include "credential" and "credentials" alongside the existing "http", "https", "www", "schema", "schemas", "vc".
537-541:⚠️ Potential issue | 🟠 MajorDo not let failed non-type constraints pass via fallback.
When constraint fields do not target
type, this returnstrue, so a format/schema-matched VC that failed required claim constraints can still be presented. Make this fail closed unless the fallback can verify the specific failed constraint.Suggested tightening
if (!hasTypeConstraint) { // No type constraint in the fields — the constraint fields are about - // other claims. Since format already matches, allow it through. - Log.d("OVP_MATCH", "typeAwareFallback: no type constraint in fields, allowing format-matched VC") - return true + // other claims. Do not override a failed constraint match. + Log.d("OVP_MATCH", "typeAwareFallback: no type constraint in fields; failing closed") + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt` around lines 537 - 541, The typeAwareFallback logic in MatchingVcsHelper.kt currently returns true when hasTypeConstraint is false, which allows VCs that failed non-type claim constraints to slip through; update the typeAwareFallback method to fail closed by returning false when hasTypeConstraint is false (or only return true if the fallback explicitly verifies the specific failed non-type constraint), and ensure the change references the hasTypeConstraint boolean and the typeAwareFallback function so callers of type/schema matching cannot bypass failed claim constraints.sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt (1)
244-263:⚠️ Potential issue | 🟠 MajorMark the verifier-error send as attempted before the IO call.
If
sendErrorToVerifier(...)throws,hasSentVerifierErrorstaysfalse, so recomposition while the overlay is visible can retry the external call repeatedly. Set the guard on the main coroutine before switching toDispatchers.IO.Suggested fix
LaunchedEffect(showErrorDialog, hasSentVerifierError) { if (!showErrorDialog || hasSentVerifierError) { return@LaunchedEffect } + hasSentVerifierError = true withContext(Dispatchers.IO) { try { OpenID4VPManager.sendErrorToVerifier( OpenID4VPExceptions.InvalidTransactionData( OVPConstants.ERR_NO_MATCHING_VCS, @@ "VPShareScreen" ) ) - hasSentVerifierError = true } catch (e: Exception) { Log.e("VPShareScreen", "Failed to send error to verifier", e) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt` around lines 244 - 263, The current LaunchedEffect may retry sending verifier errors if OpenID4VPManager.sendErrorToVerifier(...) throws because hasSentVerifierError is only set after the IO call; move the guard update to before the IO switch: inside the LaunchedEffect (guarded by showErrorDialog and !hasSentVerifierError) set hasSentVerifierError = true on the main coroutine, then call withContext(Dispatchers.IO) to invoke OpenID4VPManager.sendErrorToVerifier(...) and handle/log any exception without resetting hasSentVerifierError; reference showErrorDialog, hasSentVerifierError, LaunchedEffect, and OpenID4VPManager.sendErrorToVerifier when locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.kt`:
- Around line 244-263: The current LaunchedEffect may retry sending verifier
errors if OpenID4VPManager.sendErrorToVerifier(...) throws because
hasSentVerifierError is only set after the IO call; move the guard update to
before the IO switch: inside the LaunchedEffect (guarded by showErrorDialog and
!hasSentVerifierError) set hasSentVerifierError = true on the main coroutine,
then call withContext(Dispatchers.IO) to invoke
OpenID4VPManager.sendErrorToVerifier(...) and handle/log any exception without
resetting hasSentVerifierError; reference showErrorDialog, hasSentVerifierError,
LaunchedEffect, and OpenID4VPManager.sendErrorToVerifier when locating the
change.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.kt`:
- Around line 314-318: The isGenericSchemaToken function currently treats common
host/protocol tokens as generic; extend its denylist to also include path tokens
like "credential" and "credentials" so that schema matching ignores those
generic path components; update the when-clause in isGenericSchemaToken
(referenced by function name) to include "credential" and "credentials"
alongside the existing "http", "https", "www", "schema", "schemas", "vc".
- Around line 537-541: The typeAwareFallback logic in MatchingVcsHelper.kt
currently returns true when hasTypeConstraint is false, which allows VCs that
failed non-type claim constraints to slip through; update the typeAwareFallback
method to fail closed by returning false when hasTypeConstraint is false (or
only return true if the fallback explicitly verifies the specific failed
non-type constraint), and ensure the change references the hasTypeConstraint
boolean and the typeAwareFallback function so callers of type/schema matching
cannot bypass failed claim constraints.
In
`@sample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt`:
- Around line 158-164: In resolveFromJson, extend the JsonObject handling to
accept localized credentialName objects (e.g., credentialName: { "value": "..."
}) as well as primitive strings: after reading
vcObject.getAsSafeString("credentialName") and
vcObject.getAsSafeObject("credentialName"), check if the object exists and
extract its "value" string (falling back to nested vc handling), and similarly
for the nestedVc variable; update the resolution order in resolveFromJson to
return the extracted "value" when present before falling back to type/docType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9541e940-5ad8-4dd0-9a0d-9bce2c0eade3
📒 Files selected for processing (6)
sample-application/sample-application/.kotlin/sessions/kotlin-compiler-16268922388281170111.salivesample-application/sample-application/app/build.gradle.ktssample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/MatchingCredentialsScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/ui/VPShareScreen.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/ovp/utils/MatchingVcsHelper.ktsample-application/sample-application/app/src/main/java/com/example/samplecredentialwallet/utils/CredentialParser.kt
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue ticket number and link
Attaching video for reference
sample-ovp.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Chores