fix(android): always set Kotlin jvmTarget to 17#59
fix(android): always set Kotlin jvmTarget to 17#59christopherwxyz wants to merge 7 commits intopeterferguson:mainfrom
Conversation
The kotlinOptions block was gated behind `agpVersion < 8`, so on AGP 8+ the Kotlin JVM target defaulted to whatever JDK was running Gradle. With Kotlin 2.1.20 and JDK 20, this caused a JVM target mismatch (Java 17 vs Kotlin 20) that fails the build. Remove the AGP version check so jvmTarget=17 is always set.
|
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:
📝 WalkthroughWalkthroughRemoved AGP-version conditional and always set Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant JS as "JS Module"
participant iOS as "iOS Module"
participant ASAuth as "ASAuthorizationController"
JS->>iOS: request assertion (includes request.allowCredentials?)
alt allowCredentials present & non-empty
iOS->>ASAuth: create controller with platformKeyAssertionRequest
else
iOS->>ASAuth: create controller with platformKeyAssertionRequest + crossPlatformKeyAssertionRequest
end
ASAuth->>iOS: delegate results
iOS->>JS: return passkey result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Regenerate pnpm-lock.yaml to match current peerDependencies. Fix TypeScript error where local AuthenticationExtensionsLargeBlobInputs type's `write: string` conflicts with DOM's `write: BufferSource`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ReactNativePasskeysModule.web.ts (1)
51-55: The@ts-expect-errorscope is broader than ideal, but restructuring would require unwanted changes.The suppression at line 51 covers the entire
extensions:property, including theprffield. This is wider than thegetflow's scoping (line 127), which narrows to thelargeBlobproperty by constructing it explicitly with type-checked conversions.However, narrowing the
createflow's suppression to matchget's pattern would require explicitly handlinglargeBlob.write(converting Base64URL toBufferSource). Per the WebAuthn spec,largeBlob.writeis only valid in theget(assertion) flow, notcreate(attestation), making such conversion unnecessary and semantically incorrect.The current broad suppression is a pragmatic trade-off: because
largeBlobarrives opaquely via spread rather than through explicit construction, keeping the suppression at the property level is simpler than restructuring the code. The suppression will self-clean when the upstream type mismatch is resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ReactNativePasskeysModule.web.ts` around lines 51 - 55, The `@ts-expect-error` currently precedes the entire extensions object (around the extensions: { ...request.extensions, prf: normalizePRFInputs(request) } construction) which is broader than necessary; either move the suppression to only the prf line (apply `@ts-expect-error` directly to the normalizePRFInputs(request) expression or its assignment) so you don't suppress other extension fields, or if narrowing is impossible without restructuring, replace the blanket suppression with a short explanatory comment referencing the deliberate trade-off (largeBlob.write only applies to get/assertion and not create/attestation) and keep the suppression scoped to the extensions property; locate this change in ReactNativePasskeysModule.web.ts around the extensions spread and the normalizePRFInputs(request) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ReactNativePasskeysModule.web.ts`:
- Around line 51-55: The `@ts-expect-error` currently precedes the entire
extensions object (around the extensions: { ...request.extensions, prf:
normalizePRFInputs(request) } construction) which is broader than necessary;
either move the suppression to only the prf line (apply `@ts-expect-error`
directly to the normalizePRFInputs(request) expression or its assignment) so you
don't suppress other extension fields, or if narrowing is impossible without
restructuring, replace the blanket suppression with a short explanatory comment
referencing the deliberate trade-off (largeBlob.write only applies to
get/assertion and not create/attestation) and keep the suppression scoped to the
extensions property; locate this change in ReactNativePasskeysModule.web.ts
around the extensions spread and the normalizePRFInputs(request) call.
When allowCredentials is specified, only include the platform assertion request in ASAuthorizationController. Including the cross-platform (SecurityKey) provider alongside the platform provider causes iOS to show the expanded "choose how to sign in" UI instead of auto-selecting the credential, breaking targeted assertions with third-party providers like Chrome and 1Password. Discoverable assertions (empty allowCredentials) still include both providers for full compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/ReactNativePasskeysModule.swift`:
- Around line 52-61: The current branch that checks request.allowCredentials
only creates platformKeyAssertionRequest, preventing external/security-key
authenticators from matching; always include both platformKeyAssertionRequest
and the crossPlatformKeyAssertionRequest (from
prepareCrossPlatformAssertionRequest) in the ASAuthorizationController requests
when allowCredentials is non-empty, and set the allowedCredentials (credential
descriptors) on both request objects using request.allowCredentials so either
authenticator type can satisfy the assertion; only reduce to platform-only or
cross-platform-only when you have explicit verified metadata indicating the
credentials are exclusively one authenticator type.
| if let allowCredentials = request.allowCredentials, !allowCredentials.isEmpty { | ||
| authController = ASAuthorizationController(authorizationRequests: [ | ||
| platformKeyAssertionRequest, | ||
| ]) | ||
| } else { | ||
| let crossPlatformKeyAssertionRequest = prepareCrossPlatformAssertionRequest( | ||
| challenge: challengeData, request: request) | ||
| authController = ASAuthorizationController(authorizationRequests: [ | ||
| platformKeyAssertionRequest, crossPlatformKeyAssertionRequest, | ||
| ]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Apple ASAuthorizationPlatformPublicKeyCredentialAssertionRequest security key external credentials allowCredentials
💡 Result:
In Apple’s passkeys APIs, ASAuthorizationPlatformPublicKeyCredentialAssertionRequest.allowedCredentials is the native equivalent of WebAuthn allowCredentials: it lets you restrict an assertion to a specific set of credential IDs (non‑discoverable / “username-first” style sign-in). If you omit it or set it to an empty array, you enable the discoverable (passkey) experience where the system can show an account selector / pick from available passkeys without you pre-supplying credential IDs. [1], [2], [3]
Important distinction: platform vs external (security key) credentials
ASAuthorizationPlatformPublicKeyCredentialAssertionRequestis for the platform authenticator (passkeys in iCloud Keychain on Apple devices). [1]- For external / roaming authenticators (USB/NFC/BLE security keys), use
ASAuthorizationSecurityKeyPublicKeyCredentialProviderand itsASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest, which also supports an allow-list viaallowedCredentials(and some bindings expose additional related properties likeSecurityAllowedCredentials). [4]
Typical patterns
1) Discoverable passkey sign-in (recommended UX): don’t set allowedCredentials
let provider = ASAuthorizationPlatformPublicKeyCredentialProvider(relyingPartyIdentifier: rpId)
let req = provider.createCredentialAssertionRequest(challenge: challenge)
// req.allowedCredentials = nil // or []Omitting/empty allowCredentials is what enables discoverable credential flows in WebAuthn/passkeys. [2], [3]
2) Restrict to known credential IDs (allow-list)
req.allowedCredentials = credentialIDs.map {
ASAuthorizationPlatformPublicKeyCredentialDescriptor(credentialID: $0)
}This mirrors WebAuthn allowCredentials usage. [2], [3]
3) External security key assertion: use the security-key provider
Conceptually:
- create an
ASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest - set its
allowedCredentialsto the security-key credential descriptors you want to permit (optionally including transport hints), then run it viaASAuthorizationController. [4]
Sources: Apple “Supporting passkeys” overview for the platform flow [1], WebAuthn/passkeys behavior of allowCredentials (omit/empty for discoverable credentials) [2], [3], and the security-key assertion request API surface showing AllowedCredentials for security keys [4].
🌐 Web query:
Apple AuthenticationServices ASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest vs platform request
💡 Result:
In AuthenticationServices, the assertion request type mainly selects which kind of WebAuthn authenticator you’re targeting:
-
ASAuthorizationPlatformPublicKeyCredentialAssertionRequest(“platform request”)- Targets a platform authenticator: a passkey stored on/managed by the Apple platform (e.g., iCloud Keychain / device-bound passkeys).
- Can also surface hybrid / “use a phone” flows (QR/Bluetooth) on supported OS versions via platform-only UI controls like
shouldShowHybridTransport(seen used only on the platform assertion request). [2] - In practice, this is the request you use for “Sign in with passkey” on Apple devices. [1]
-
ASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest(“security key request”)- Targets a cross‑platform external authenticator (a “security key”): USB/NFC/BLE FIDO2 keys (e.g., YubiKey).
- Uses security‑key descriptors (
ASAuthorizationSecurityKeyPublicKeyCredentialDescriptor) rather than platform descriptors, and does not participate in the platform’s hybrid/passkey-on-phone UX toggles. [3][2]
What’s the practical difference in code?
- You create them from different providers:
ASAuthorizationPlatformPublicKeyCredentialProvider(...).createCredentialAssertionRequest(challenge:)for platform/passkeys. [1][2]ASAuthorizationSecurityKeyPublicKeyCredentialProvider(...).createCredentialAssertionRequest(challenge:)for external security keys. [2]
Common pattern (support both)
Many implementations create both a platform assertion request and a security-key assertion request (same RP ID + challenge), set allowedCredentials appropriately for each descriptor type, then pass both to ASAuthorizationController so the user can choose the authenticator. [2][3]
Sources: [1] [2] [3] [4]
🌐 Web query:
WebAuthn allowCredentials attribute effect on authenticator selection
💡 Result:
allowCredentials (in PublicKeyCredentialRequestOptions passed to navigator.credentials.get()) directly filters which authenticators are eligible by telling the user agent “only these credential IDs are acceptable for this assertion.”
- If
allowCredentialsis non-empty: the browser will only let the user complete authentication using an authenticator that can produce an assertion for one of the listed credential IDs (and matching RP ID, etc.). If no available authenticator can satisfy any entry, the ceremony fails (commonly surfaced asNotAllowedError). [1], [2] - If
allowCredentialsis empty (or omitted): it means any credential is acceptable, which enables “username-less” / discoverable credential flows (the authenticator can choose an RP-matching discoverable credential and the browser may show an account picker). [1], [3] transportsinside each allowCredentials entry: are hints the platform can use to decide which authenticators could work (e.g., “internal” vs “usb/nfc/ble”), potentially avoiding presenting options that can’t satisfy the request. [2]
Practical effect on authenticator selection:
- Populate
allowCredentialswhen you already know the user (e.g., after username entry) to constrain authentication to that account’s registered credentials. [3] - Leave it empty/omitted only if you intend to support discoverable credentials and account selection on the authenticator/UI. [3]
Sources:
[1] MDN: PublicKeyCredentialRequestOptions.allowCredentials (restriction + empty array meaning “any credential”) (developer.mozilla.org)
[2] W3C WebAuthn Level 2: ineligible authenticators + transports hint and “no eligible credential” failure behavior (w3.org)
[3] W3C WebAuthn Level 3: definition/purpose of allowCredentials (“find authenticators eligible…”, recommended when account identified) (w3.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredentialRequestOptions?utm_source=openai
- 2: https://www.w3.org/TR/webauthn-2/?utm_source=openai
- 3: https://www.w3.org/TR/webauthn-3/?utm_source=openai
Functional regression: non-empty allowCredentials prevents security-key authentication.
When allowCredentials is non-empty, the code now creates only a platform assertion request. If the allowCredentials list contains a credential ID registered to an external/security-key authenticator, that authenticator cannot fulfill the assertion—the platform request cannot produce a valid assertion for security-key credentials. This causes NotAllowedError.
Fix: Always provide both platform and security-key assertion requests when allowCredentials is set. Set allowedCredentials on each request type with appropriate descriptors, allowing either authenticator type to satisfy the credentials in the allow-list. Only use platform-only or security-key-only paths if there is explicit, verifiable information about which authenticator types the credentials belong to.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/ReactNativePasskeysModule.swift` around lines 52 - 61, The current branch
that checks request.allowCredentials only creates platformKeyAssertionRequest,
preventing external/security-key authenticators from matching; always include
both platformKeyAssertionRequest and the crossPlatformKeyAssertionRequest (from
prepareCrossPlatformAssertionRequest) in the ASAuthorizationController requests
when allowCredentials is non-empty, and set the allowedCredentials (credential
descriptors) on both request objects using request.allowCredentials so either
authenticator type can satisfy the assertion; only reduce to platform-only or
cross-platform-only when you have explicit verified metadata indicating the
credentials are exclusively one authenticator type.
Use GetCredentialRequest.Builder with setAutoSelectAllowed(true) when allowCredentials is non-empty. This lets Android Credential Manager skip the picker and go straight to biometric verification when a single credential matches, preventing the hidden picker hang. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
android/src/main/java/expo/modules/passkeys/ReactNativePasskeysModule.kt (1)
78-78: Consider usingisNullOrEmpty()for cleaner null-safe check.The current pattern works correctly due to short-circuit evaluation, but Kotlin's standard library provides a more idiomatic approach.
♻️ Suggested improvement
- val hasAllowCredentials = request.allowCredentials != null && request.allowCredentials!!.isNotEmpty() + val hasAllowCredentials = !request.allowCredentials.isNullOrEmpty()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/expo/modules/passkeys/ReactNativePasskeysModule.kt` at line 78, Replace the null-and-empty check for allowCredentials with Kotlin's idiomatic null-safe method: update the computation of hasAllowCredentials (which currently uses request.allowCredentials != null && request.allowCredentials!!.isNotEmpty()) to call request.allowCredentials.isNullOrEmpty() and invert as needed so hasAllowCredentials remains true when there are credentials; reference the hasAllowCredentials variable and request.allowCredentials to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/expo/modules/passkeys/ReactNativePasskeysModule.kt`:
- Around line 79-82: Remove the invalid setAutoSelectAllowed(...) call on
GetCredentialRequest.Builder in the block that builds getCredentialRequest;
instead only add GetPublicKeyCredentialOption(json) to the request builder, and
if you intended to enable client-side auto-select for a Google credential use
GetGoogleIdOption.Builder().setAutoSelectEnabled(true) and add that option to
the GetCredentialRequest via addCredentialOption(...) (or otherwise configure
auto-select on the appropriate provider entry builder), referencing
GetCredentialRequest.Builder, GetPublicKeyCredentialOption, and
GetGoogleIdOption.Builder.setAutoSelectEnabled.
---
Nitpick comments:
In `@android/src/main/java/expo/modules/passkeys/ReactNativePasskeysModule.kt`:
- Line 78: Replace the null-and-empty check for allowCredentials with Kotlin's
idiomatic null-safe method: update the computation of hasAllowCredentials (which
currently uses request.allowCredentials != null &&
request.allowCredentials!!.isNotEmpty()) to call
request.allowCredentials.isNullOrEmpty() and invert as needed so
hasAllowCredentials remains true when there are credentials; reference the
hasAllowCredentials variable and request.allowCredentials to locate the change.
android/src/main/java/expo/modules/passkeys/ReactNativePasskeysModule.kt
Outdated
Show resolved
Hide resolved
The previous version (1.3.0-alpha01) did not have setAutoSelectAllowed on GetCredentialRequest.Builder. Version 1.5.0 (stable) includes this API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/build.gradle`:
- Around line 89-90: The project currently uses compileSdkVersion 33 while the
added dependencies implementation "androidx.credentials:credentials:1.5.0" and
implementation "androidx.credentials:credentials-play-services-auth:1.5.0"
require minCompileSdk=35; fix by either updating compileSdkVersion to 35 in the
Android module's build.gradle (update the compileSdkVersion property) or replace
those dependency lines with versions compatible with compileSdkVersion 33
(downgrade the two androidx.credentials artifacts to a supported version);
ensure you update only the compileSdkVersion or the two dependency coordinates
(implementation "androidx.credentials:credentials:..." and implementation
"androidx.credentials:credentials-play-services-auth:...") so consumer builds no
longer fail.
|
looking for this too 👍 |
|
Yes, completely agree that the jvm target needs to be fixed, but why does this PR contain 7 commits and 2,540 insertions/3,009 deletions? It should be a simple build.gradle fix. |
|
#60 contains the targeted fix (using the simpler |
4 participants
Summary
The
kotlinOptionsblock inandroid/build.gradlewas gated behindagpVersion < 8, so on AGP 8+ the Kotlin JVM target defaulted to whatever JDK was running Gradle. With Kotlin 2.1.20 and JDK 20, this causes a build failure:This PR removes the AGP version check so
jvmTarget = 17is always set, matching thecompileOptionsblock above it.Changes
if (agpVersion < 8)guard aroundkotlinOptionscompileOptionsblock that was inside the guard (already set unconditionally above)Summary by CodeRabbit