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

Missing PKCE is success in email verification flow #1145

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

scotttrinh
Copy link
Collaborator

Since end users might verify their email on a different device than the user agent they initiated the sign up (or sign in) flow with, treat this as a success condition. The application will need to detect this case and show a message that confirms that the email is verified, but that the user will need to sign in to complete.

Note: this is a breaking change, so we need to bump the minor (given this is a 0.x release)

@scotttrinh scotttrinh requested a review from jaclarke December 10, 2024 12:48
Since end users might verify their email on a different device than
the user agent they initiated the sign up (or sign in) flow with, treat
this as a success condition. The application will need to detect this
case and show a message that confirms that the email is verified, but
that the user will need to sign in to complete.
@scotttrinh scotttrinh force-pushed the email-verify-can-skip-pkce branch from 01428bd to d6c1053 Compare December 11, 2024 02:03
@scotttrinh scotttrinh requested a review from jaclarke December 11, 2024 02:03
@scotttrinh
Copy link
Collaborator Author

@jaclarke Sorry about missing this: the built-in UI needed this update, too, so: d6c1053 (#1145)

error: null,
tokenData: null,
provider: null,
isSignUp: false,
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this probably should be true, as email verification is part of the sign up flow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good point. I think we should actually use the isSignUp search param here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, actually, I don't think this works as-is: it's impossible to tell the difference between a sign in attempt from an unverified email vs. a successful verification without the PKCE verifier since they have the same shape.

Let me do a little design work here. I think we might need to introduce a discriminant into this callback payload type to make it less ambiguous for the consumer.

@scotttrinh
Copy link
Collaborator Author

Going to put this back in draft, it's not nearly ready:

  • The core verify methods do not verify emails without a pkce verifier. They should be updated.
  • We don't really have a way to signal that a sign-up -> verify --missing pkce--> sign in is really a sign up with more steps. I plan on adding the identity_id to the sign up response from the server so you can at least create your user before email verification. This will impact this interface.

@scotttrinh scotttrinh marked this pull request as draft December 16, 2024 16:07
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.

2 participants