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

🛂 Passkey Authentication #21

Closed
wants to merge 9 commits into from
Closed

🛂 Passkey Authentication #21

wants to merge 9 commits into from

Conversation

sebipap
Copy link
Contributor

@sebipap sebipap commented Jan 8, 2024

  1. last commit 🤡 set up initial mocked ui can be ignored for review, it was only used to test the features detailed in PR.
  2. you may see there're a lot of TODO comments and questions. This change is still WIP as there are some details to be discussed with team.
  3. this was tested in web environment

Copy link

vercel bot commented Jan 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
pomelo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2024 6:31pm

jgalat
jgalat previously approved these changes Jan 9, 2024
@sebipap sebipap changed the title 🚸 Initial UI setup 🛂 Passkey Authentication Feb 6, 2024
Copy link

@NicolasMontone NicolasMontone left a comment

Choose a reason for hiding this comment

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

LGTM so far :)

@@ -0,0 +1 @@
.vercel

Choose a reason for hiding this comment

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

This could be moved to the global .gitignore, right?

import rpId from "../../utils/rpId.js";

async function handler(request: VercelRequest, response: VercelResponse) {
const { challengeID } = request.query as { challengeID: string };

Choose a reason for hiding this comment

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

You can use zod to validate this input (same with the other files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

about validation: I'll fix after agreeing the core functionality is approved

@@ -8,13 +8,15 @@ if (!process.env.EXPO_PUBLIC_ALCHEMY_GAS_POLICY_ID) throw new Error("missing alc
if (!process.env.EXPO_PUBLIC_TURNKEY_ORGANIZATION_ID) throw new Error("missing turnkey organization id");
if (!process.env.EXPO_PUBLIC_TURNKEY_API_PUBLIC_KEY) throw new Error("missing turnkey api public key");
if (!process.env.EXPO_PUBLIC_TURNKEY_API_PRIVATE_KEY) throw new Error("missing turnkey api private key");
if (!process.env.EXPO_PUBLIC_BACKEND_URL) throw new Error("missing backend url");

Choose a reason for hiding this comment

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

Nothing to do with the PR, but a cool idea is to validate the env with zod https://catalins.tech/validate-environment-variables-with-zod/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

export default async function createAccount() {
const challengeID = base64URLEncode(generateRandomBuffer());
const { challenge } = await registrationOptions(challengeID);
const name = `exactly, ${new Date().toISOString()}`;

Choose a reason for hiding this comment

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

Why having multiples keys make sense? This may confuse the user, IMO, having 1 key per phone is ok.
I suggest the name:

Suggested change
const name = `exactly, ${new Date().toISOString()}`;
const name = 'exa-app-key';

Copy link
Member

Choose a reason for hiding this comment

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

this key is not per device, it's global to the user's google/apple account

Choose a reason for hiding this comment

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

Cool thanks for replying! But it make sense having the date in the name? The user will see this names, right? Maybe it could be improved so the user can differentiate between keys. Just a suggestion :)

Copy link
Contributor Author

@sebipap sebipap Feb 15, 2024

Choose a reason for hiding this comment

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

this key is not per device, it's global to the user's google/apple account

having this in mind, doesn't it make sense to only have 1 passkey per user google/apple account?
It could be problematic if the user creates multiple passkeys when they uninstall and reinstall app and doing the "normal" onboarding flow, creating an extra passkey.
Maybe we could make a distinct and clearly different flow for "creating a second account", where user on purpose creates a second account for whatever purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants