-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Passkey new finalize enrollment #15163
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: passkey-new-start-enrollment
Are you sure you want to change the base?
Passkey new finalize enrollment #15163
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback. |
Generated by 🚫 Danger |
708f4d8
to
6bf5556
Compare
|
||
let request = FinalizePasskeyEnrollmentRequest( | ||
idToken: rawAccessToken(), | ||
name: passkeyName ?? "Unnamed account (Apple)", |
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.
We would have already set this in start, do we really need to this check again?
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.
in this case, since passkeyName is an optional string, we either need to provide a default value like this, or force-unwrap using '!' to abort execution if the optional value contains nil. There's no compiler guarantee that startEnrollment would be called and will set the passkeyName. Although, we can add guard else throw{..}
in the beginning to confirm that passkeyName is set but as we know in the passkey flow, finalizePasskeyEnrollment will always be called after startPasskeyEnrollment hence I didnt use an explicit error handling here and used the default name which could be the only case where passkeyName is nil. Please let me know if I should add a guard statement here also, in that case we can remove "Unnamed account (Apple)" from 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.
I think it's okay if we need to keep it, you can update this to use the same constant as you used in the StartEnrollment flow once you merge
No description provided.