-
Notifications
You must be signed in to change notification settings - Fork 26
feat: adds an ios.requireBiometrics config flag #56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: adds an ios.requireBiometrics config flag #56
Conversation
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant TS_Module as "TS Module"
participant RN_Native as "iOS Native Module"
participant LAContext
participant BiometricSystem
App->>TS_Module: create/get(request, requireBiometrics?)
TS_Module->>RN_Native: call native create/get(with flag on iOS)
RN_Native->>LAContext: build LAContext and choose policy
alt requireBiometrics = true
RN_Native->>LAContext: deviceOwnerAuthenticationWithBiometrics
else
RN_Native->>LAContext: deviceOwnerAuthentication
end
RN_Native->>LAContext: canEvaluatePolicy()
LAContext->>BiometricSystem: check hardware/credentials
alt policy evaluable
BiometricSystem-->>LAContext: success
LAContext-->>RN_Native: ok
RN_Native-->>TS_Module: proceed with auth flow / result
TS_Module-->>App: success response
else
BiometricSystem-->>LAContext: failure
LAContext-->>RN_Native: BiometricException
RN_Native-->>TS_Module: error
TS_Module-->>App: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
ios/ReactNativePasskeysModule.swift(4 hunks)src/ReactNativePasskeysModule.ts(3 hunks)src/ReactNativePasskeysModule.web.ts(1 hunks)src/index.ts(3 hunks)src/utils/warn-user-of-missing-webauthn-extensions.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (1)
src/ReactNativePasskeys.types.ts (2)
CreationResponse(245-258)AuthenticationResponseJSON(96-103)
src/ReactNativePasskeysModule.ts (2)
src/ReactNativePasskeys.types.ts (4)
PublicKeyCredentialCreationOptionsJSON(51-61)CreationResponse(245-258)PublicKeyCredentialRequestOptionsJSON(66-73)AuthenticationResponseJSON(96-103)src/errors.ts (1)
NotSupportedError(16-20)
| if (!this.isSupported) throw new NotSupportedError(); | ||
|
|
||
| const credential = await passkeys.create(request); | ||
| const credential = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call the support check
isSupported is a function. Without invoking it, this guard is always truthy and NotSupportedError will never surface; the code falls straight through to the native call even on unsupported platforms (which can surface unrelated native errors). Please invoke the check before continuing.
- if (!this.isSupported) throw new NotSupportedError();
+ if (!this.isSupported()) throw new NotSupportedError();📝 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.
| if (!this.isSupported) throw new NotSupportedError(); | |
| const credential = await passkeys.create(request); | |
| const credential = | |
| if (!this.isSupported()) throw new NotSupportedError(); | |
| const credential = |
🤖 Prompt for AI Agents
In src/ReactNativePasskeysModule.ts around lines 22 to 24, the support guard
currently references isSupported as a value; change it to invoke the function
and throw when it indicates unsupported. Call isSupported() and if it returns a
Promise, await it (e.g., await this.isSupported()); then use the boolean result
in the if-check and throw NotSupportedError when false so native calls are only
made on supported platforms.
|
@ronickg you're fine! Thanks for picking this up and rebasing 🎉 |
lucasloisp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Do believe we need a changeset file here!
src/ReactNativePasskeysModule.web.ts
Outdated
| if (clientExtensionResults) { | ||
| for (const key in requestedExtensions) { | ||
| if (typeof clientExtensionResults[key] === "undefined") { | ||
| if (typeof (clientExtensionResults as Record<string, unknown>)[key] === "undefined") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this change was needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasloisp Ah my ts was showing up with errors here for some reason. I can revert it.
| if (clientExtensionResults) { | ||
| for (const key in requestedExtensions) { | ||
| if (typeof clientExtensionResults[key] === "undefined") { | ||
| if (typeof (clientExtensionResults as Record<string, unknown>)[key] === "undefined") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same Q
This PR allows one to disable the need for requiring biometrics on iOS, thanks to the work from https://github.com/lucasloisp in the #43
I tried forking the repo and merging, but found it easier to use the latest version and include the changes from there. Ideally, credit for the work should go to @lucasloisp. Or maybe he can update his PR.
Summary by CodeRabbit
New Features
Bug Fixes