[core] Support magic links in `SignInPage` #4085

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)

Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Bharat Kashyap <[email protected]>

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

              <Alert severity="error">{error}</Alert>
            ) : 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

              <Alert severity="error">{error}</Alert>
            ) : null}
            {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 enhancement This is not a bug, nor 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 and others added 4 commits October 16, 2024 01:17

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

16f2a3b

Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Bharat Kashyap <[email protected]>

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

5a1b0b4

Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Bharat Kashyap <[email protected]>

fix: Jan review

040fcaf

fix: CI

dc9f688

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

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

d534daa

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 16, 2024

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

22de185

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

fix: lint

ffde5f0

bharatkashyap requested a review from mnajdova October 18, 2024 12:29

bharatkashyap changed the title ~~[feat] Support magic links in SignInPage~~ [core] Support magic links in SignInPage Oct 18, 2024

bharatkashyap requested review from Janpot and removed request for Janpot October 18, 2024 18:06

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Support magic links in `SignInPage` #4085

[core] Support magic links in `SignInPage` #4085

bharatkashyap commented Sep 13, 2024 •

edited by prakhargupta1

Loading

apedroferreira left a comment

apedroferreira Oct 10, 2024

bharatkashyap Oct 16, 2024

apedroferreira Oct 16, 2024 •

edited

Loading

apedroferreira Oct 10, 2024

apedroferreira Oct 10, 2024

bharatkashyap Oct 11, 2024 •

edited

Loading

apedroferreira Oct 11, 2024

apedroferreira Oct 10, 2024

bharatkashyap Oct 11, 2024

apedroferreira Oct 11, 2024

apedroferreira Oct 10, 2024

bharatkashyap Oct 11, 2024

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 22, 2024

jakobmerrild commented Oct 22, 2024

apedroferreira left a comment •

edited

Loading

apedroferreira Oct 22, 2024 •

edited

Loading

apedroferreira Oct 22, 2024

apedroferreira Oct 22, 2024

apedroferreira Oct 22, 2024 •

edited

Loading


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

[core] Support magic links in SignInPage #4085

Are you sure you want to change the base?

[core] Support magic links in SignInPage #4085

Conversation

bharatkashyap commented Sep 13, 2024 • edited by prakhargupta1 Loading

apedroferreira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharatkashyap Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 21, 2024

jakobmerrild commented Oct 21, 2024

bharatkashyap commented Oct 22, 2024

jakobmerrild commented Oct 22, 2024

apedroferreira left a comment • edited Loading

Choose a reason for hiding this comment

apedroferreira Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

[core] Support magic links in `SignInPage` #4085

[core] Support magic links in `SignInPage` #4085

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