-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
Login with magic code #1818
Login with magic code #1818
Conversation
62e4f47
to
6954342
Compare
We could also enable magic code for non-PWA users, imho it would be handy, token is 6-digits anyway and you can still click the link |
3930597
to
19296ab
Compare
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.
Turns out I didn't submit this review two days ago 👀
I didn't test yet, only looked at the code. Left some questions.
We could also enable magic code for non-PWA users, imho it would be handy, token is 6-digits anyway and you can still click the link
I think this is the way to go. Less complexity in our code, less confusing for users and would allow to use emails on mobile to login on desktop and vice versa.
components/pull-to-refresh.js
Outdated
|
||
useEffect(checkPWA, []) | ||
useEffect(() => { | ||
typeof window !== 'undefined' && setIsPWA(checkPWA(window)) |
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.
Do you need to check typeof window !== undefined
? Do effects not always run on the client?
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.
That's what I thought and still believe, I had to include typeof window !== undefined
because it seems that the code is being ran before client exposes window or on server while compiling. I might be saying some big bullshit here but that was the error I was getting.
pages/api/auth/[...nextauth].js
Outdated
function randomizeToken () { | ||
const words = bech32.toWords(Buffer.from(randomBytes(3))) | ||
return bech32.encode('token', words).slice(6, 12) | ||
} |
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 need to think more if 24 bits is strong enough even if the link is only valid for 5 mins. Need to do some research how other services deal with the low entropy 🤔
Using bech32 encoding to go from random bytes to guaranteed alphanumeric characters is also an interesting choice. I think I know why you decided to use bech32 but can you elaborate?
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 more a product of better safe than sorry
. Discussing with @huumn we agreed that bech32 encoding would produce a safer token than a standard 6-digit or even an 8-digit one.
The real reason is the resulting entropy, there are 32 possible characters per every 'digit', 32^6 = 1.073.741.824 (auto calc came in clutch lol).
24-bit or 32-bit entropy will result anyway in 32^6 after slicing!1
Edit:
go from random bytes to guaranteed alphanumeric characters
I realized I didn't address this, it's just one way to randomize the starting seed, we could choose whatever in place of that
Footnotes
-
if I'm not mistaken ↩
Redrafting to give this full priority to 'complete' the PR as everything else is ready for review |
pages/auth/error.js
Outdated
@@ -27,8 +27,8 @@ export default function AuthError ({ error }) { | |||
return ( | |||
<StaticLayout> | |||
<Image className='rounded-1 shadow-sm' width='500' height='375' src={`${process.env.NEXT_PUBLIC_ASSET_PREFIX}/double.gif`} fluid /> | |||
<h2 className='pt-4'>This magic link has expired.</h2> | |||
<h4 className='text-muted pt-2'>Get another by logging in.</h4> | |||
<h2 className='pt-4'>Where did the magic go?</h2> |
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 a placeholder for eventual stackernews-themed sentences 👀
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'll QA and stuff first thing tomorrow, but we'll want to change the usage of the bech32
lib.
pages/api/auth/[...nextauth].js
Outdated
@@ -366,9 +403,15 @@ export default async (req, res) => { | |||
await NextAuth(req, res, getAuthOptions(req, res)) | |||
} | |||
|
|||
function randomizeToken () { | |||
const words = bech32.toWords(Buffer.from(randomBytes(3))) | |||
return bech32.encode('token', words).slice(6, 12) |
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 don't think we want to use the bech32 encoding lib:
- I don't understand it
- it has features that we don't need like prefix and checksum
I suggested bech32 for the alphanumeric character set because in lower case, none of the characters can be confused for other numbers/letters. For upper case, there might be a better character set, but basically we want something like the following to generate the string (we have no need for a checksum):
function generateRandomString(length = 6, charset) {
// Default charset if none provided
charset = charset || 'qpzry9x8gf2tvdw0s3jn54khce6mua7l';
const bytes = crypto.randomBytes(length);
let result = '';
// Map each byte to a character in the charset
for (let i = 0; i < length; i++) {
result += charset[bytes[i] % charset.length];
}
return result;
}
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.
Okay! I juggled around the bech32 library because we were already using it (appropriately) for other stuff. I'll gladly switch to manual generation and avoid shenanigans.
Didn't know about avoiding confusion with uppercase characters but I see it now, switching asap!
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.
FYI Probably not going to get to QA until later today. Troubleshooting weird lightning stuff.
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 didn't get to it at all today - add to #1853 that the lnd cert expired, paginated comments deployment caused a meltdown and had weird ux issues.
The fixes you made look good though. I'll QA first thing tomorrow hopefully.
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.
A signal from the universe that paginated comments are 🔥🔥🔥, jokes aside don't worry, this PR ain't going somewhere else while we sleep ^^
pages/api/auth/[...nextauth].js
Outdated
function generateRandomString (length = 6, charset = BECH32_CHARSET) { | ||
const bytes = randomBytes(length) | ||
const result = new Array(length) | ||
|
||
// Map each byte to a character in the charset | ||
for (let i = 0; i < length; i++) { | ||
result[i] = charset[bytes[i] % charset.length] | ||
} | ||
|
||
return result.join('') | ||
} |
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.
Just added the bech32 charset to constants, made it default and switched to array building instead of string concatenation (it would allocate a string per addition, actually not a problem with 6 characters but it's better to be safe than sorry)
edit: my opinion on array join vs string concatenation is something that I learnt but I might be wrong, the performance change is impossible to perceive anyway!
I'll test performance asap
Okay I was wrong, string concatenation in this use case is 0.17% faster, this edit is basically useless... reverting
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 haven't done a line by line review yet but this works well in my QA so far.
Is it intentional that the code must be typed manually? The email says "copy it" but each input only takes one character so I can't copy then paste. I have to manually input each character.
Also it might be nice if the input autofocused to assist copy and pasting.
I'll continue QAing and let you know if anything else comes up
Oh, didn't think about it! I'm going to push it right now.
That's a bug, autofocus works on desktop hmm... on it, thanks k00b! |
Copy/paste works now and it QAs great. I'll do a line-by-line and finer UI/UX crit tomorrow, but unless there's something too big for me to change easily, you can probably consider this done. Nice work! 🎆 |
}, | ||
orderBy: { | ||
createdAt: 'desc' | ||
} | ||
}) | ||
|
||
if (!verificationRequest) return null |
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 forces a redirect to the login page (my change that is) if all 3 attempts have been tried which lets us avoid a the "go back or login again" on the error page. Instead, the button on the error page just goes back in the browser history.
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 a way better UX, thanks!
await prisma.verificationToken.update({ | ||
where: { id: verificationRequest.id }, | ||
data: { attempts: { increment: 1 } } | ||
}) |
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.
Your changes were perfectly fine, but the increment wasn't an atomic read-write; it read from the db in one tx, then updated it in another. While it's still possible with my fixes for someone to get more than 3 attempts in quick succession, this makes it a little less likely.
Ideally, this code would not have any races at all, but it's a bit tricky because identifier
isn't unique. We could wrap it all in a transaction and do an optimistic lock, or make it seriallizable, but our attempt limit is so low they won't get many more attempts anyway.
Great work! Couldn't fault except for relative nitpicks. |
Woa, this is very neat!! Nice solution for fixing the loggin in with the PWA 👍🏻 |
Description
Partially fixes #727
Introduces a 6-character bech32 magic code (token) that expires in 5 minutes and can be used to login with email on PWA, especially iOS since atm there's no way to open a PWA from a link.
On
signIn
, the email and the callbackUrl will be temporarily saved on sessionStorageUpon submitting the magic code, it will build and redirect to a magic link (as we are used to) with the user inserted token and
email, callbackUrl
from sessionStorageAfter 3 failed attempts, a custom
useVerificationToken
will delete email and token from the database, requiring the user to get a new one.Introduces
MultiInput
, a customizable component that allows for n inputs and supports keyboard traveling, can prepend a sequence (1-input, 2-input, etc.)Screenshots
code input page:
![image](https://private-user-images.githubusercontent.com/6390896/407281288-0f94a66d-66e5-46c2-8142-3ef0c37e0368.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwOTQyNzQsIm5iZiI6MTczOTA5Mzk3NCwicGF0aCI6Ii82MzkwODk2LzQwNzI4MTI4OC0wZjk0YTY2ZC02NmU1LTQ2YzItODE0Mi0zZWYwYzM3ZTAzNjgucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMDkzOTM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NTFlOGRlYzYyNDgwYzE5YjgzYjdjYmUzYTFmNzhlOTg0Yjk5OWFiMGI3ZTg5MGNiY2Q1MjgwNzIyNDQ5NmVjNSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.Qs0dB_tSdIku4bMSEuNzzdl-VtEMN2ohe-DBdufTacw)
invalid magic code:
![image](https://private-user-images.githubusercontent.com/6390896/407281097-2fd8a05f-504d-41e5-9c70-adbe155fb20c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwOTQyNzQsIm5iZiI6MTczOTA5Mzk3NCwicGF0aCI6Ii82MzkwODk2LzQwNzI4MTA5Ny0yZmQ4YTA1Zi01MDRkLTQxZTUtOWM3MC1hZGJlMTU1ZmIyMGMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMDkzOTM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ODUxN2U4Y2QxNmRlNDU4OWYzMjc2Zjc3MWFmYzk0NTg2NjZlYzRjOWE3YjI2ODJjMDZlYTM5NzU0YTIxMjBhMiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.5pqUOUV9SSYql3WQDVV40BPm_HHZR5dF958hRKIy-b0)
if no callback url or email
![image](https://private-user-images.githubusercontent.com/6390896/407291227-5d14959a-5a06-4ac8-8267-4cd478c5ab59.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwOTQyNzQsIm5iZiI6MTczOTA5Mzk3NCwicGF0aCI6Ii82MzkwODk2LzQwNzI5MTIyNy01ZDE0OTU5YS01YTA2LTRhYzgtODI2Ny00Y2Q0NzhjNWFiNTkucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMDkzOTM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ODZhYjI5NWU4YWI3NDUyNjE1NThjMGUxODBkMWY4OTY4YWExZGI1ZTU5OGI0ZDM0YTQ1Njk3ZWMzODc0ZmE2ZiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.4Ee__Bv_Il4AFuwe3z9n2xSaUYDWogQNAkrXrjGG4_o)
email template:
![image](https://private-user-images.githubusercontent.com/6390896/405195522-25869d29-9d55-4c1c-9605-906688c7b3cd.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwOTQyNzQsIm5iZiI6MTczOTA5Mzk3NCwicGF0aCI6Ii82MzkwODk2LzQwNTE5NTUyMi0yNTg2OWQyOS05ZDU1LTRjMWMtOTYwNS05MDY2ODhjN2IzY2QucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMDkzOTM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MWJkOTg4NWMzNzc1ZWZjYmVmYTY2MjM5Njc3YWUzNDE3Mzg1NDViNjVkYWI2OGM1NGZiZThhMWE2NDQ3M2Q1ZCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.ZzDL8FHm_gth-8ud8ZLt4O868UAVEQ397veEfUh7eMo)
error page:
![image](https://private-user-images.githubusercontent.com/6390896/403837932-d50d247a-e1e2-4823-ba76-3d44db6dcb3d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwOTQyNzQsIm5iZiI6MTczOTA5Mzk3NCwicGF0aCI6Ii82MzkwODk2LzQwMzgzNzkzMi1kNTBkMjQ3YS1lMWUyLTQ4MjMtYmE3Ni0zZDQ0ZGI2ZGNiM2QucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMDkzOTM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NmY1ZmRjMDc3NTBiYzBhMTc0ZDk2ZTliYWViMjIxMzkxMTNjZTBhZTBkZjVmNTlhZTNiNDZmYTcwZGI5MDA0ZCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.1CYln3XcP_dURjDUu_ULaouYkKPGVuMbreo4UdPE7cc)
Additional Context
afaik in this version of NextAuth we can't send parameters from signIn to /verify-request to /email without risking to be disruptive, thus I had to resort to sessionStorage
Also we can't send the default token AND the new custom token
Changed from 'login as [email protected]' to 'login with [email protected]' to make it independent from context (if login or setting an email); removed any reference to links
InputInner component has been modified to check for
hideError
, if true error messages will be showed by MultiInput instead of InputInnerChecklist
Are your changes backwards compatible? Please answer below:
Changes token generation
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
7: login, signup, link email, emails
7: token invalidation occurs on success and on 4th failed attempt
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Yes
Did you introduce any new environment variables? If so, call them out explicitly here:
No
Progress
Removed checkboxes, was lagging too much
pull-to-refresh.js
;email.js
)