-
Notifications
You must be signed in to change notification settings - Fork 426
feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials #3663
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
Conversation
🦋 Changeset detectedLatest commit: 6b6308d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
44fb76b to
ecce02d
Compare
|
!snapshot |
|
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i [email protected] --save-exact |
| return; | ||
| } | ||
|
|
||
| if (numericTypes.includes(AuthenticationType.FINGERPRINT)) { |
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.
❓If the device supports multiple authentication methods (fingerprint and facial recognition), are we prioritizing fingerprint here?
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.
That's a very nice question, tbh my intention here was to have AuthenticationType.IRIS behave like face-recognition so i might need to reverse the logic, in order for iris/face have priority
| export function LocalCredentialsProvider(props: PropsWithChildren): JSX.Element { | ||
| const { isLoaded, signIn } = useSignIn(); | ||
| const { publishableKey } = useClerk(); | ||
| const key = `__clerk_local_auth_${publishableKey}_identifier`; |
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.
💡Nit: Could we extract this interpolation to a separate function? Or perhaps type it, to avoid accidentally changing it or misspelling between individual variables.
| const [isEnrolled, setIsEnrolled] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| let ignore = false; |
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.
❓Is the purpose of ignore to not re-fetch isEnrolledAsync?
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.
So the intention is to avoid race conditions in a situation where the component gets unmounted but the promise resolves after the component is already unmounted.
|
!snapshot |
|
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact |
| biometryType: BiometricType | null; | ||
| }; | ||
|
|
||
| const LocalCredentialsInitValues: LocalCredentialsReturn = { |
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.
❓ Should we have something like isPlatformSupported in LocalCredentialsInitValues in order to be easier to check if the hook can be used on specific platforms like Web or a device that does not has any biometric support, as now it seems you will have to check if biometryType is null which is not so clear if you don't take a look at the code
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.
For devices that do not support biometric, android and iOS fallback to PIN, and if a pin does not exist then biometricType will be null.
with biometricType you get the available authenticator and it is useful mostly for UI purposes. Displaying a Face ID or a touch ID icon for example.
Maybe simply changing the name to supportedBiometricType would do ?
We don't wanna indicate that the device can use biometric, but instead if we can use local authentication (biometric or not) in order to store credentials.
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.
I was mostly referring for platforms that this will not work at all like web,the biometric support was just an example as I didn't know if this fallbacks to pin
brkalow
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.
Need some JSDoc comments on exported methods, and left a few non-blocking comments. Nice work!
Feel free to dismiss if you address the comments during your day tomorrow.
| setUserOwnsCredentials(false); | ||
| }; | ||
|
|
||
| const authenticate = async () => { |
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.
💡 It would be nice to (optionally?) set the credentials after a successful auth
packages/expo/src/hooks/useLocalCredentials/useLocalCredentials.ts
Outdated
Show resolved
Hide resolved
octoper
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.
💯
Description
This PR introduces a new hook API
Video of the new UX flow
RPReplay_Final1719504401.MP4
1.Example usage in a profile page
1.Example usage in a custom sign in flow
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change