Skip to content
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

Support resetting passwords via email #2

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
23c3905
Support resetting passwords via email
mia-pi-git Nov 3, 2021
530d6d3
Fix queries
mia-pi-git Mar 27, 2023
75583b5
curuser stuff
mia-pi-git Jul 9, 2023
ab6540a
limit emails to one account
mia-pi-git Jul 9, 2023
c4a5cba
Fix standing actions
mia-pi-git Mar 27, 2023
8ca40ef
Fix GXE rounding
mia-pi-git Mar 27, 2023
7dc7786
Use async signing instead of sync
mia-pi-git Mar 27, 2023
27f0491
Further optimize string encoding
mia-pi-git Mar 27, 2023
a669a2c
Set CORS headers when sid is sent directly in the URL
mia-pi-git Mar 29, 2023
62ff3dd
Remove trailing space from comment
mia-pi-git Mar 29, 2023
d13c1a4
Fix setting CORS for body sids
mia-pi-git Apr 3, 2023
d8f4ecb
Support rebuilding the client through an action (#15)
mia-pi-git Apr 14, 2023
6b635de
Support full rebuilds in the rebuildclient action
mia-pi-git Apr 14, 2023
f82d81a
Actions: Ensure the right commands are executed in rebuildclient
mia-pi-git Apr 21, 2023
895819e
Ensure pulls error correctly
mia-pi-git Apr 21, 2023
49cc574
Session: Improve existing user check to fix crash in addUser
mia-pi-git May 17, 2023
ae2b617
Set current gen to 9 for ladder Elo decay purposes
adrivrie Apr 2, 2023
d964001
Add missing truncation
gigalh128 Jul 25, 2023
ecbf23b
Support OAuth (#12)
mia-pi-git Aug 15, 2023
a70e7d5
Implement origin checking on OAuth clients
mia-pi-git Aug 15, 2023
136d864
OAuth: Add refresh token endpoint
mia-pi-git Aug 16, 2023
fecafa8
Add documentation for OAuth2 functionality
mia-pi-git Aug 18, 2023
9991844
OAuth: Inform users what account they're logged in as
mia-pi-git Aug 18, 2023
0eb3886
OAuth: Add a page to list authorized applicationons for a user
mia-pi-git Aug 18, 2023
895a2b1
Fix typo
mia-pi-git Aug 18, 2023
ba1060d
Ensure CORS headers are always set on oauth endpoints
mia-pi-git Aug 21, 2023
7e740de
OAuth: Ensure the getassertion action accepts a challstr properly
mia-pi-git Aug 21, 2023
3f56dde
Fix code sample for getting a token (#18)
Async10 Sep 11, 2023
9377d9e
Fix bugs in OAuth UI
mia-pi-git Sep 25, 2023
8f14db8
Add an action to query teams from a database (#19)
mia-pi-git Sep 25, 2023
8b536e7
Actions: Split up getteams to use less bandwidth
mia-pi-git Sep 29, 2023
59f87bf
Support resetting passwords via email
mia-pi-git Nov 3, 2021
7c79f52
Merge remote-tracking branch 'upstream/master' into pass-resets
mia-pi-git Oct 18, 2023
fb15ce2
Require confirmation
mia-pi-git Oct 18, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/config-example.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ exports.passwordSalt = 10;
/** @type {Record<string, string>} */
exports.routes = {
root: "pokemonshowdown.com",
client: "play.pokemonshowdown.com",
};

/** @type {string} */
Expand Down Expand Up @@ -150,3 +151,8 @@ exports.standings = {
"30": "Permaban",
"100": "Disabled",
};
/** @type {{transportOpts: import('nodemailer').TransportOptions, from: string}} */
exports.passwordemails = {
transportOpts: {},
from: 'passwords@pokemonshowdown.com',
};
31 changes: 24 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
},
"dependencies": {
"@types/node": "^15.12.4",
"@types/nodemailer": "^6.4.4",
"bcrypt": "^5.0.1",
"eslint-plugin-import": "^2.24.2",
"google-auth-library": "^3.1.2",
Expand All @@ -31,7 +32,7 @@
"eslint": "^7.32.0",
"eslint-plugin-import": "^2.22.1",
"mocha": "^6.0.2",
"nodemailer": "^6.6.5",
"nodemailer": "^6.9.1",
"typescript": "^4.4.3"
},
"private": true
Expand Down
93 changes: 93 additions & 0 deletions src/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ import {toID, updateserver, bash, time} from './utils';
import * as tables from './tables';
import * as pathModule from 'path';
import IPTools from './ip-tools';
import nodemailer from 'nodemailer';

// 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;
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stole it from usermodlog.


const mailer = nodemailer.createTransport(Config.passwordemails.transportOpts);


export const actions: {[k: string]: QueryHandler} = {
async register(params) {
Expand Down Expand Up @@ -467,6 +474,92 @@ export const actions: {[k: string]: QueryHandler} = {
matches: await tables.users.selectAll(['userid', 'banstate'])`ip = ${res.ip}`,
};
},
async setemail(params) {
if (!this.user.loggedIn) {
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 address.`);
}
const email = EMAIL_REGEX.exec(params.email)?.[0];
if (!email) throw new ActionError(`Email is invalid or already taken.`);
const data = await tables.users.get(this.user.id);
if (!data) throw new ActionError(`You are not registered.`);
if (data.email?.endsWith('@')) {
throw new ActionError(`You have 2FA, and do not need to set an email for password resets.`);
}
const emailUsed = await tables.users.selectAll(['userid'])`WHERE email = ${email}`;
if (emailUsed.length) {
throw new ActionError(`Email is invalid or already taken.`);
}
const result = await tables.users.update(this.user.id, {email});

delete (data as any).passwordhash;
return {
success: !!result.changedRows,
curuser: {loggedin: true, userid: this.user.id, username: data.username, email},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

};
},
async clearemail() {
if (!this.user.loggedIn) {
throw new ActionError(`You must be logged in to edit your email.`);
}
const data = await tables.users.get(this.user.id);
if (!data) throw new ActionError(`You are not registered.`);
if (data.email?.endsWith('@')) {
throw new ActionError(
`You have 2FA, and need an administrator to set/unset your email manually.`
);
}
const result = await tables.users.update(this.user.id, {email: null});

delete (data as any).passwordhash;
return {
actionsuccess: !!result.changedRows,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

curuser: {loggedin: true, userid: this.user.id, username: data.username, email: null},
};
},
async resetpassword(params) {
if (typeof params.email !== 'string' || !params.email) {
throw new ActionError(`You must provide an email address.`);
}
const email = EMAIL_REGEX.exec(params.email)?.[0];
if (!email) {
throw new ActionError(`Invalid email sent.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error reads like it refers to sending an email, rather than sending an email address in a HTTP request.

}
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);
Comment on lines +623 to +636
Copy link
Contributor

@AnnikaCodes AnnikaCodes Jul 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work if an email address is associated with multiple user accounts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


await mailer.sendMail({
from: Config.passwordemails.from,
to: data.email,
subject: "Pokemon Showdown account password reset",
text: (
`You requested a password reset for the Pokemon Showdown account ${data.userid}. Click this link https://${Config.routes.root}/resetpassword/${token} and follow the instructions to change your password.\n` +
`Not you? Please contact staff by typing /ht in any chatroom on Pokemon Showdown. \n` +
`If you are unable to do so, visit the Help chatroom.`
),
html: (
`You requested a password reset for the Pokemon Showdown account ${data.userid}. ` +
`Click <a href="https://${Config.routes.root}/resetpassword/${token}">this link</a> and follow the instructions to change your password.<br />` +
`Not you? Please contact staff by typing <code>/ht</code> in any chatroom on Pokemon Showdown. <br />` +
`If you are unable to do so, visit the <a href="${Config.routes.client}/help">Help</a> chatroom.`
),
});
return {success: true};
},
};

if (Config.actions) {
Expand Down
32 changes: 32 additions & 0 deletions src/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {ladder, loginthrottle, sessions, users, usermodlog} from './tables';

const SID_DURATION = 2 * 7 * 24 * 60 * 60;
const LOGINTIME_INTERVAL = 24 * 60 * 60;
const PASSWORD_RESET_TOKEN_SIZE = 10;

export class User {
name = 'Guest';
Expand Down Expand Up @@ -485,4 +486,35 @@ export class Session {
}
return pass;
}

async createPasswordResetToken(name: string, timeout: null | number = null) {
const ctime = time();
const userid = toID(name);
if (!timeout) {
timeout = 7 * 24 * 60 * 60;
}
timeout += ctime;
// todo throttle by checking to see if pending token exists in sid table?
if (await this.findPendingReset(name)) {
throw new ActionError(`A reset token is already pending to that account.`);
}

await usermodlog.insert({
userid, actorid: userid, ip: this.context.getIp(),
date: ctime, entry: "Password reset token requested",
});

// magical character string...
const token = crypto.randomBytes(PASSWORD_RESET_TOKEN_SIZE).toString('hex');
await sessions.insert({
userid, sid: token, time: ctime, timeout, ip: this.context.getIp(),
});
return token;
}
async findPendingReset(name: string) {
const id = toID(name);
const sids = await sessions.selectAll()`WHERE userid = ${id}`;
// not a fan of this but sids are normally different lengths. have to be, iirc.
return sids.some(({sid}) => sid.length === (PASSWORD_RESET_TOKEN_SIZE * 2));
}
AnnikaCodes marked this conversation as resolved.
Show resolved Hide resolved
}