Support resetting passwords via email #2
Conversation
mia-pi-git
force-pushed
the
master
branch
2 times, most recently
from
a165e71
to
75eafa2
Compare
| // eslint-disable-next-line | ||
| const EMAIL_REGEX = /(?:[a-z0-9!#$%&\'*+\/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&\'*+\/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])/i; |
There was a problem hiding this comment.
Does this regex come from somewhere or did you create it yourself? It's hard to tell what this is doing; I assume it's to validate an email address.
There was a problem hiding this comment.
I stole it from usermodlog.
src/actions.ts
Outdated
| throw new ActionError(`You must be logged in to set an email.`); | ||
| } | ||
| if (!params.email || typeof params.email !== 'string') { | ||
| throw new ActionError(`You must send an email.`); |
There was a problem hiding this comment.
| throw new ActionError(`You must send an email.`); | |
| throw new ActionError(`You must send an email address.`); |
| const data = await tables.users.selectOne()`WHERE email = ${email}`; | ||
| if (!data) { | ||
| // no user associated with that email. | ||
| // ...pretend like it succeeded (we don't wanna leak that it's in use, after all) | ||
| return {success: true}; | ||
| } | ||
| if (!data.email) { | ||
| // should literally never happen | ||
| throw new Error(`Account data found with no email, but had an email match`); | ||
| } | ||
| if (data.email.endsWith('@')) { | ||
| throw new ActionError(`You have 2FA, and so do not need a password reset.`); | ||
| } | ||
| const token = await this.session.createPasswordResetToken(data.username); |
There was a problem hiding this comment.
Will this work if an email address is associated with multiple user accounts?
There was a problem hiding this comment.
I was thinking about this. I think as a matter of internet standard, we probably shouldn't allow emails to be keyed to more than one account.
There was a problem hiding this comment.
That sounds inconvenient for people who have multiple accounts they care about not losing the password to. Also, if we do decide to run things this way, are we normalizing email addresses?
src/actions.ts
Outdated
| success: !!result.changedRows, | ||
| curuser: {loggedin: true, userid: this.user.id, username: data.username, email}, |
There was a problem hiding this comment.
If 0 rows are altered, but there's no error from SQL, wouldn't that just mean they already had that email on their account? Would that warrant success: false?
src/actions.ts
Outdated
|
|
||
| delete (data as any).passwordhash; | ||
| return { | ||
| actionsuccess: !!result.changedRows, |
There was a problem hiding this comment.
Why do some of these actions use success and others actionsuccess? Apparently it got started from a typo? I understand it on older actions, since we need to be compatible with PHP, but we should really pick one or the other to use exclusively on new actions.
| } | ||
| const email = EMAIL_REGEX.exec(params.email)?.[0]; | ||
| if (!email) { | ||
| throw new ActionError(`Invalid email sent.`); |
There was a problem hiding this comment.
This error reads like it refers to sending an email, rather than sending an email address in a HTTP request.
I really hate that local linter doesn't catch that.
* WIP * rebase kinda borked, handle it * linter stuff * Dammit i swear I had linted this * Don't require a challstr * add schemas * modify some stuff * remove register action * Remove the inner action too * Prettify page * Shift stuff down * Fix jquery loading * specify message * Make client typing more consistent in token table * Sanitize redirect URL, also make the page html a top-level constant * Escape client information too * Escape URI, fix checks
The code sample had multiple problems. The most important one was that it did not run because the call to `checkIfUpdated` was incorrect and the code didn't take into account that a DOMException is thrown when trying to read `popup.location.href` before the redirect happened. Apart from that I added url encoding of the query parameters and some simple sanity checks on the received `token` and `assertion` values in order to make the sample a bit more robust.
Needs UI, full deploy of TS.
(Transferred from old repo)