Skip to content
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

feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials #3663

Merged
merged 28 commits into from
Aug 7, 2024

Conversation

panteliselef
Copy link
Member

@panteliselef panteliselef commented Jul 3, 2024

Description

This PR introduces a new hook API

import {
  useLocalCredentials,
} from "@clerk/clerk-expo";

const {
  hasCredentials, // boolean
  userOwnsCredentials, // boolean | null
  biometricType,  // "face-recognition" | "fingerprint" | null
  setCredentials, // ({ identifier?: string, password: string }) => void
  authenticate, // () => Promise<SignInResource>
  clearCredentials, // () => Promise<void>
} = useLocalCredentials();

Video of the new UX flow

RPReplay_Final1719504401.MP4

1.Example usage in a profile page

import { 
  useUser, 
  useClerk, 
  useLocalCredentials,
} from "@clerk/clerk-expo";

export default function Page() {
  const { user } = useUser();
  const { signOut } = useClerk();

  const {
    hasCredentials,
    clearCredentials,
  } = useLocalCredentials();

  return (
    <View>
      <Text>Settings, {user?.emailAddresses[0].emailAddress}</Text>
      <Button title="Sign out" onPress={() => signOut()} />
      {
        hasCredentials &&
        <Button title="Remove biometric credentials" onPress={() => clearCredentials()} />
      }
    </View>
  );
}

1.Example usage in a custom sign in flow

import {
  useSignIn,
  useLocalCredentials,
} from "@clerk/clerk-expo";
import { Link, Stack, useRouter } from "expo-router";
import {
  Text,
  TextInput,
  Button,
  View,
  TouchableOpacity,
  StyleSheet,
} from "react-native";
import React from "react";
import { SymbolView } from "expo-symbols";

export default function Page() {
  const { signIn, setActive, isLoaded } = useSignIn();
  const router = useRouter();

  const [emailAddress, setEmailAddress] = React.useState("");
  const [password, setPassword] = React.useState("");
  const { hasCredentials, setCredentials, authenticate, biometryType } =
    useLocalCredentials();

  const onSignInPress = React.useCallback(
    async (useLocal = false) => {
      if (!isLoaded) {
        return;
      }

      try {
        const signInAttempt =
          hasCredentials && useLocal
            ? await authenticate()
            : await signIn.create({
                identifier: emailAddress,
                password,
              });

        if (signInAttempt.status === "complete") {
          await setActive({ session: signInAttempt.createdSessionId });
          if (!(hasCredentials && useLocal)) {
            await setCredentials({
              identifier: emailAddress,
              password,
            })
          }

          // navigate away
        } else {
          // handle other statuses of sign in 
        }
      } catch (err: any) {
         // handle any other error
      }
    },
    [isLoaded, emailAddress, password]
  );

  return (
    <View>
      <TextInput
        value={emailAddress}
        onChangeText={(emailAddress) => setEmailAddress(emailAddress)}
      />
      <TextInput
        value={password}
        onChangeText={(password) => setPassword(password)}
      />
      <Button title="Sign In" onPress={() => onSignInPress()} />
      {hasCredentials && biometryType && (
        <TouchableOpacity
          onPress={() => onSignInPress(true)}
        >
          <SymbolView
            name={
              biometryType === "face-recognition" ? "faceid" : "touchid"
            }
            type="monochrome"
          />
        </TouchableOpacity>
      )}
    </View>
  );
}

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselef panteliselef self-assigned this Jul 3, 2024
Copy link

changeset-bot bot commented Jul 3, 2024

🦋 Changeset detected

Latest commit: 6b6308d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clerk/clerk-expo Minor

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

@panteliselef panteliselef force-pushed the elef/user-119-local-auth-for-expo branch from 44fb76b to ecce02d Compare July 5, 2024 11:31
@panteliselef panteliselef requested review from nikosdouvlis, BRKalow, octoper and LauraBeatris and removed request for nikosdouvlis July 5, 2024 13:00
@panteliselef
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

Hey @panteliselef - the snapshot version command generated the following package versions:

Package Version
@clerk/clerk-expo 1.3.0-snapshot.ve946474
gatsby-plugin-clerk 5.0.0-beta.45

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/clerk-expo

npm i @clerk/[email protected] --save-exact

gatsby-plugin-clerk

npm i [email protected] --save-exact

@panteliselef panteliselef marked this pull request as ready for review July 5, 2024 13:47
return;
}

if (numericTypes.includes(AuthenticationType.FINGERPRINT)) {
Copy link
Member

@LauraBeatris LauraBeatris Jul 5, 2024

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?

Copy link
Member Author

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`;
Copy link
Member

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;
Copy link
Member

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?

Copy link
Member Author

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.

packages/expo/package.json Outdated Show resolved Hide resolved
@panteliselef panteliselef changed the title feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials [DO NOT MERGE] feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials Jul 8, 2024
@panteliselef panteliselef changed the title [DO NOT MERGE] feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials Aug 6, 2024
@panteliselef
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

Hey @panteliselef - the snapshot version command generated the following package versions:

Package Version
@clerk/clerk-expo 2.1.0-snapshot.vff0337d

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/clerk-expo

npm i @clerk/[email protected] --save-exact

biometryType: BiometricType | null;
};

const LocalCredentialsInitValues: LocalCredentialsReturn = {
Copy link
Member

@octoper octoper Aug 6, 2024

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

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member

@BRKalow BRKalow left a 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 () => {
Copy link
Member

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

Copy link
Member

@octoper octoper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@panteliselef panteliselef requested a review from BRKalow August 7, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants