-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
[core] Support magic links in SignInPage
#4085
base: master
Are you sure you want to change the base?
Conversation
nodemailer
provider to SignInPage
SignInPage
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.
Cool, seems to work well, the demos look clean!
I wrote down some thoughts and ideas.
|
||
Find details on how to set up each provider in the [Auth.js documentation](https://authjs.dev/getting-started/authentication/oauth). | ||
::: | ||
|
||
## Magic Link |
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 there be a default success alert that would already show in this demo?
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 have a separate demo on the Alerts just below this one, so I think this one is okay? If you think strongly that we should merge them, I can do that
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.
Not a huge deal, it's just that the first demo doesn't provide any feedback when you press the submit button so maybe that will feel a bit weird if it's the first thing a user tries.
But I just noticed that other demos in the same page work the same... maybe they could always show a standard browser alert or something (instead of a console log that most people will miss), but I guess that's something you can add at any time, doesn't need to block this PR.
|
||
{{"demo": "MagicLinkAlertSignInPage.js", "iframe": true, "height": 500}} | ||
|
||
To get the magic link working, you need to add the following code to your custom sign-in page: |
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 guess they don't really "need" to but it's a way that users can do it, right? So maybe we can say they "can" add that code instead, as this is an example of how they can do it with Auth.js?
if (provider.id === 'nodemailer' && | ||
(error as any).digest?.includes('verify-request')) { | ||
return { | ||
success: 'Check your email for a verification link.', |
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.
Would a more specific name like successAlert
or successMessage
be better?
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.
Perhaps, but that would mean we also rename the error
prop in AuthResponse
to errorMessage
which – while better – would be a breaking change on the AuthResponse
interface. What do you think?
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.
Got it, just a thought, I think error
shouldn't be renamed so we can keep success
if it's more consistent with that.
// For the nodemailer provider, we want to return a success message | ||
// if the error is a 'verify-request' error, instead of redirecting | ||
// to a `verify-request` page | ||
if (provider.id === 'nodemailer' && (error as any).digest?.includes('verify-request')) { |
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 guess they don't provide types that we can use to type this error with the digest
?
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 agree that this is quite fragile, but I couldn't find any way to detect exclusively that the redirect is to verify-request
using an Error type. I'll investigate a bit more
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.
Ok, no big deal if you can't find it, was just wondering.
redirectTo: callbackUrl ?? '/', | ||
}); | ||
} catch (error) { | ||
if (error instanceof Error && error.message === 'NEXT_REDIRECT') { |
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.
Not super clean that this is how "success" works (by catching an error) but if that's just how Auth.js does it I guess there's nothing we can do?
Ideally this code would be easier to follow.
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.
This is how Next.js + Auth.js server compoents redirects work - throwing a NEXT_REDIRECT
which we must catch, I'm basing this on vercel/next.js#49298 (comment)
@@ -220,4 +238,4 @@ The `SignInPage` component has versions with different layouts for authenticatio | |||
|
|||
## 🚧 Other authentication Flows | |||
|
|||
The `SignInPage` will be accompanied by other components to allow users to sign up, verify emails and reset passwords. This is in progress. | |||
The `SignInPage` will be accompanied by other components to allow users to sign up, and reset passwords. This is in progress. |
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.
Address vale comment
The `SignInPage` will be accompanied by other components to allow users to sign up, and reset passwords. This is in progress. | |
Besides the `SignInPage` , the team is planning work on several other components that enable new workflows such as sign up and password reset. |
Perhaps we need links to issues?
Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Bharat Kashyap <[email protected]>
Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Bharat Kashyap <[email protected]>
SignInPage
SignInPage
@bharatkashyap I was looking for any open issues w.r.t. supporting one-time passwords when using the |
@jakobmerrild Not with this PR (I should edit the title of that issue) but the plan is to implement support for OTPs as an immediate follow up of this PR, including adding an example app. I'll create a separate issue: #4292 |
Sounds great! |
Actually, looking at the new issue it sounds like you are planning to support one time codes sent to the user's email. What I'm looking for is 2FA support via one-time passwords, basically an additional optional input field on the |
Understood, that isn't part of the roadmap yet but feel free to open an issue/comment with your requirements on that issue! |
Thanks for letting me know. I have opened an issue. In the mean time is there a way for me to create my own sign-in page and have it integrate with the |
Thanks for the detailed issue, I'll add it to the consideration for our future versions. For the meantime, you could use the |
Thanks! I managed to get it to work using your example and the basic page from |
This is because |
Gotcha! Thanks for all the help and sorry for spamming your PR 🙈 |
Docs preview: https://deploy-preview-4085--mui-toolpad-docs.netlify.app/toolpad/core/react-sign-in-page/#magic-link