[core] Support magic links in `SignInPage` by bharatkashyap · Pull Request #4085 · mui/toolpad

bharatkashyap · 2024-09-13T13:46:24Z

Closes [SignInPage] Add magic link support #3990

Docs preview: https://deploy-preview-4085--mui-toolpad-docs.netlify.app/toolpad/core/react-sign-in-page/#magic-link

…d into feat/magic-link

apedroferreira

Cool, seems to work well, the demos look clean!
I wrote down some thoughts and ideas.

apedroferreira · 2024-10-10T12:22:29Z

docs/data/toolpad/core/components/sign-in-page/sign-in-page.md

 Find details on how to set up each provider in the [Auth.js documentation](https://authjs.dev/getting-started/authentication/oauth).
 :::

+## Magic Link


Should there be a default success alert that would already show in this demo?

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

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.

apedroferreira · 2024-10-10T12:25:10Z

docs/data/toolpad/core/components/sign-in-page/sign-in-page.md

+
+{{"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:


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?

apedroferreira · 2024-10-10T12:27:40Z

docs/data/toolpad/core/components/sign-in-page/sign-in-page.md

+            if (provider.id === 'nodemailer' &&
+            (error as any).digest?.includes('verify-request')) {
+            return {
+              success: 'Check your email for a verification link.',


Would a more specific name like successAlert or successMessage be better?

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?

Got it, just a thought, I think error shouldn't be renamed so we can keep success if it's more consistent with that.

apedroferreira · 2024-10-10T12:29:27Z

examples/core-auth-nextjs-email/src/app/auth/signin/actions.ts

+      // 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')) {


I guess they don't provide types that we can use to type this error with the digest?

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

Ok, no big deal if you can't find it, was just wondering.

apedroferreira · 2024-10-10T12:39:53Z

docs/data/toolpad/core/components/sign-in-page/sign-in-page.md

+              redirectTo: callbackUrl ?? '/',
+            });
+        } catch (error) {
+          if (error instanceof Error && error.message === 'NEXT_REDIRECT') {


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.

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)

jakobmerrild · 2024-10-21T09:40:20Z

@bharatkashyap I was looking for any open issues w.r.t. supporting one-time passwords when using the credentials provider and found your issue. However, looking at the code in this PR it doesn't seem to solve that part of the issue.
Do you have any plans to include support for one-time passwords in the credentials provider with this PR?

bharatkashyap · 2024-10-21T10:09:08Z

@bharatkashyap I was looking for any open issues w.r.t. supporting one-time passwords when using the credentials provider and found your issue. However, looking at the code in this PR it doesn't seem to solve that part of the issue. Do you have any plans to include support for one-time passwords in the credentials provider with this PR?

@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

jakobmerrild · 2024-10-21T10:55:12Z

Sounds great!

jakobmerrild · 2024-10-21T10:59:02Z

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 SignInPage.

bharatkashyap · 2024-10-21T11:23:47Z

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 SignInPage.

Understood, that isn't part of the roadmap yet but feel free to open an issue/comment with your requirements on that issue!

jakobmerrild · 2024-10-21T12:00:49Z

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 Account component?

bharatkashyap · 2024-10-21T13:21:28Z

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 Account component?

Thanks for the detailed issue, I'll add it to the consideration for our future versions.

For the meantime, you could use the Account component separately like the demos in this page: https://mui.com/toolpad/core/react-account/ while building your own component for the sign in page.

jakobmerrild · 2024-10-21T17:11:01Z

Thanks! I managed to get it to work using your example and the basic page from authjs. Should then be able to just create a new custom page and pass it into the pages prop for the NextAuth I believe.
Though I am curious why your example uses the signIn and signOut functions from next-auth/react instead the ones exported from auth.ts.

bharatkashyap · 2024-10-22T09:32:28Z

Though I am curious why your example uses the signIn and signOut functions from next-auth/react instead the ones exported from auth.ts.

This is because SignInPage is a client component - see https://authjs.dev/getting-started/migrating-to-v5#details

jakobmerrild · 2024-10-22T09:48:21Z

Gotcha! Thanks for all the help and sorry for spamming your PR 🙈

apedroferreira

No major changes since last time I approved, right? Let me know if there's anything I should check more in specific.
LGTM, added some comments that sound worth doing before merging, but nothing blocking!

apedroferreira · 2024-10-22T14:54:08Z

packages/toolpad-core/src/SignInPage/SignInPage.test.tsx

+    await userEvent.click(signInButton);
+
+    expect(signIn).toHaveBeenCalled();
+    expect(signIn.mock.calls[0][0]).toHaveProperty('id', 'nodemailer');


I guess that none of those usual methods like toHaveBeenLastCalledWith works for these checks? Or would it? They're just more readable.

apedroferreira · 2024-10-22T14:55:59Z

packages/toolpad-core/src/SignInPage/SignInPage.tsx

+                  fullWidth
+                  slotProps={{
+                    htmlInput: {
+                      sx: { paddingTop: '12px', paddingBottom: '12px' },


Maybe try to use theme units if possible unless we really need the 12px specifically here?

apedroferreira · 2024-10-22T14:57:08Z

packages/toolpad-core/src/SignInPage/SignInPage.tsx

            ) : null}
            {Object.values(providers ?? {}).map((provider) => {
-              if (provider.id === 'credentials' || provider.id === 'passkey') {
+              if (


A .filter here would probably be cleaner, before using .map?

apedroferreira · 2024-10-22T14:58:30Z

packages/toolpad-core/src/SignInPage/SignInPage.tsx

            {Object.values(providers ?? {}).map((provider) => {
-              if (provider.id === 'credentials' || provider.id === 'passkey') {
+              if (
+                provider.id === 'credentials' ||


You can also just create a single boolean variable for these provider id checks, might end up being useful for other situations later too.

feat: Add nodemailer provider to SignInPage

31ea476

bharatkashyap added the type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. label Sep 13, 2024

Merge branch 'master' into feat/magic-link

c952172

bharatkashyap changed the title ~~[feat] Add nodemailer provider to SignInPage~~ [feat] Support magic links in SignInPage Sep 13, 2024

bharatkashyap added 3 commits September 13, 2024 19:34

fix: eslint

131b443

Merge branch 'feat/magic-link' of github.com:bharatkashyap/mui-toolpa…

65e0949

…d into feat/magic-link

Merge branch 'master' into feat/magic-link

c9e3956

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 15, 2024

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

2c61b88

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 17, 2024

bharatkashyap added 6 commits September 17, 2024 15:47

Merge branch 'master' into feat/magic-link

bbdddef

Merge branch 'master' into feat/magic-link

4d7a335

Merge branch 'master' into feat/magic-link

c0796ab

Merge branch 'master' into feat/magic-link

000f694

Merge branch 'master' into feat/magic-link

7882158

Merge branch 'master' into feat/magic-link

3b6309d

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 22, 2024

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

2dd6554

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged. and removed PR: out-of-date The pull request has merge conflicts and can't be merged. labels Sep 30, 2024

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

31ba678

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 10, 2024

fix: Add example link to docs

70ef2aa

apedroferreira approved these changes Oct 10, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 10, 2024

bharatkashyap added 2 commits October 11, 2024 15:09

fix: CI

d390f71

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

4532fb2

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged. and removed PR: out-of-date The pull request has merge conflicts and can't be merged. labels Oct 11, 2024

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

110a06e

bharatkashyap removed the request for review from Janpot October 18, 2024 19:43

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 18, 2024

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

c72c511

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 21, 2024

bharatkashyap requested a review from apedroferreira October 22, 2024 04:16

apedroferreira approved these changes Oct 22, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 22, 2024

bharatkashyap added 4 commits October 23, 2024 17:28

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

b87bdc6

fix: Merge

745221e

fix: Pedro review

4ba4168

fix: Add example to docs page

6d540a7

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 23, 2024

bharatkashyap added 2 commits October 23, 2024 17:48

fix: test

5949071

Merge branch 'master' into feat/magic-link

c598903

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 29, 2024

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

8b09542

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 30, 2024

bharatkashyap added 2 commits October 30, 2024 16:03

Merge branch 'master' of github.com:mui/mui-toolpad into feat/magic-link

f09dd8f

fix: Merge with separate examples

4f64dc8

bharatkashyap merged commit 3be37c8 into mui:master Oct 30, 2024


		{{"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:

Uh oh!

Conversation

bharatkashyap commented Sep 13, 2024 • edited by prakhargupta1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apedroferreira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apedroferreira Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatkashyap Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobmerrild commented Oct 21, 2024

Uh oh!

bharatkashyap commented Oct 21, 2024

Uh oh!

jakobmerrild commented Oct 21, 2024

Uh oh!

jakobmerrild commented Oct 21, 2024

Uh oh!

bharatkashyap commented Oct 21, 2024

Uh oh!

jakobmerrild commented Oct 21, 2024

Uh oh!

bharatkashyap commented Oct 21, 2024

Uh oh!

jakobmerrild commented Oct 21, 2024

Uh oh!

bharatkashyap commented Oct 22, 2024

Uh oh!

jakobmerrild commented Oct 22, 2024

Uh oh!

apedroferreira left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apedroferreira Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apedroferreira Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bharatkashyap commented Sep 13, 2024 •

edited by prakhargupta1

Loading

apedroferreira Oct 16, 2024 •

edited

Loading

bharatkashyap Oct 11, 2024 •

edited

Loading

apedroferreira left a comment •

edited

Loading

apedroferreira Oct 22, 2024 •

edited

Loading

apedroferreira Oct 22, 2024 •

edited

Loading