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

Account creation #48

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
24 changes: 24 additions & 0 deletions emailTemplates/verification.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<html>
<head> </head>
<body>
<p>Hi there!</p>
<p>You are receiving this message to verify the email associated with your new SCCS account! Your username is: <strong>${username}</strong>.</p>
<p>
To access SCCS services, confirm your email by clicking:
<!-- insert link with proper id and key -->
<a href="${domain}/account/init?id=${verifyId}&key=${verifyKey}">this link</a>. Or, copy and
<!-- account/init -->
paste this link into your browser:
</p>
<p>${domain}/account/init?id=${resetId}&key=${resetKey}</p>
<p>
(This link will stay active for seven days; after that, you'll have to
<a href="${domain}/account/create">request a new link</a>.)
</p>
<p>
Enjoy your new SCCS account! We hope you'll find our services useful. If you have any
questions, comments, or suggestionsx, feel free to contact us at
<a href="mailto:[email protected]">[email protected]</a>
</p>
</body>
</html>
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@
"prettier --write"
],
"*.js": [
"eslint --fix",
"prettier --write"
],
"*.pug": [
Expand Down
55 changes: 34 additions & 21 deletions src/controllers/accountController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@ import { HttpException } from '../error/httpException';
import { mailTransporter } from '../integration/email';
import { ldapClient } from '../integration/ldap';
import { modifyForwardFile } from '../integration/localAgent';
import { PasswordResetRequest, PasswordResetRequestModel, TaskModel } from '../integration/models';
import {
PasswordResetRequest,
PasswordResetRequestModel,
TaskModel,
VerifyEmailRequest,
VerifyEmailRequestModel,
} from '../integration/models';
import { generateEmail } from '../util/emailTemplates';
import { sendTaskNotification } from '../util/emailUtils';
import { modifyLdap, searchAsync, searchAsyncUid } from '../util/ldapUtils';
Expand All @@ -27,36 +33,19 @@ const USERNAME_REGEX = /^[a-z][-a-z0-9]*$/;
export class CreateAccountReq {
// usernames must comply with debian/ubuntu standards so we can give them
// Heron accounts
@jf
.string()
.regex(/^[a-z][-a-z0-9]*$/, 'POSIX username')
.required()
username: string;

@jf
.string()
.email()
.regex(/.+@swarthmore\.edu/, 'Swarthmore email address')
.required()
email: string;

@jf.string().required()
name: string;

// FIXME would be nice to not manually update this every year
// TODO for that matter, can't we pull this from Cygnet?
@jf.string().valid(VALID_CLASSES).required()
classYear: string;
}

export const submitCreateAccountRequest = async (req: CreateAccountReq) => {
// TODO how do we handle someone going to create an account if they're
// already logged in?

if (!(await isUsernameAvailable(req.username))) {
throw new HttpException(400, { message: `Username ${req.username} already exists` });
}

if (!(await isEmailAvailable(req.email))) {
throw new HttpException(400, {
message: `Email ${req.email} is already associated with an account`,
Expand Down Expand Up @@ -125,7 +114,7 @@ export const doPasswordResetRequest = async (identifier: string) => {

/**
*/
export class PasswordResetCredentials {
export class ResetCredentials {
@jf.string().required()
id: string;
@jf.string().required()
Expand All @@ -134,7 +123,7 @@ export class PasswordResetCredentials {

/**
*/
export class PasswordResetRequestParams extends PasswordResetCredentials {
export class PasswordResetRequestParams extends ResetCredentials {
// we'll properly validate it later
@jf.string().required()
password: string;
Expand All @@ -146,7 +135,7 @@ export class PasswordResetRequestParams extends PasswordResetCredentials {
* @param {PasswordResetCredentials} creds The credentials to check
*/
export const verifyPasswordReset = async (
creds: PasswordResetCredentials,
creds: ResetCredentials,
): Promise<PasswordResetRequest> => {
const invalidProps = {
friendlyMessage:
Expand All @@ -170,6 +159,29 @@ export const verifyPasswordReset = async (
return resetRequest;
};

export const verifyEmail = async (creds: ResetCredentials): Promise<VerifyEmailRequest> => {
const invalidProps = {
friendlyMessage:
'This email verification link is invalid or expired. <a href="/account/create">Request a new one</a>.',
};

const verifyRequest = await VerifyEmailRequestModel.findById(creds.id);
if (!verifyRequest) {
throw new HttpException(400, {
...invalidProps,
message: `Email verification ID ${creds.id} did not match any request`,
});
}

if (!(await argon2.verify(verifyRequest.key, creds.key as string))) {
throw new HttpException(400, {
...invalidProps,
message: 'Email verification key did not match database',
});
}
return verifyRequest;
};

export const doPasswordReset = async (params: PasswordResetRequestParams) => {
const resetRequest = await verifyPasswordReset(params);

Expand Down Expand Up @@ -268,6 +280,7 @@ export const isEmailAvailable = async (email: string): Promise<boolean> => {
const [inDatabase, inPending] = await Promise.all([
Copy link
Member

Choose a reason for hiding this comment

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

I realize this code needs slightly more tweaking - needs to capture the results in three different variables

Suggested change
const [inDatabase, inPending] = await Promise.all([
const [inDatabase, inPending, inNeedsVerify] = await Promise.all([

searchAsync(ldapClient, ldapEscape.filter`(swatmail=${email})`),
TaskModel.exists({ 'data.email': email, status: 'pending' }),
VerifyEmailRequestModel.exists({ 'data.email': email }),
]);

if (inDatabase || inPending) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (inDatabase || inPending) {
if (inDatabase || inPending || inNeedsVerify) {

Expand Down
19 changes: 19 additions & 0 deletions src/functions/createAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { generateEmail } from '../util/emailTemplates';
import { addLdap, searchAsyncMultiple } from '../util/ldapUtils';
import { logger } from '../util/logging';
import { createPasswordResetRequest } from '../util/passwordReset';
import { createVerifyAccountRequest } from '../util/accountVerify';

export interface CreateAccountData {
username: string;
Expand Down Expand Up @@ -96,4 +97,22 @@ export const createAccount = async (data: any) => {
} else {
throw new Error('CreateAccount data not properly formatted');
}

const [verifyId, verifyKey] = await createVerifyAccountRequest(data.email, 24 * 7, true);
const [emailText, transporter] = await Promise.all([
generateEmail('verification.html', {
email: data.email,
domain: process.env.EXTERNAL_ADDRESS,
verifyKey: verifyKey,
verifyId: verifyId,
}),
mailTransporter,
]);

const info = await transporter.sendMail({
from: process.env.EMAIL_FROM,
to: data.email,
subject: 'Verify your SCCS account',
html: emailText,
});
};
43 changes: 43 additions & 0 deletions src/integration/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,49 @@ export const PasswordResetRequestModel = mongoose.model<PasswordResetRequest>(
passwordResetRequestSchema,
);

// new interface for email verification
export interface VerifyEmailRequest {
// verification email links generate two keys: the ID, which is stored plain, and the key, which is
// hashed with argon2 before being stored.
// Both are provided to the user and they make a request;
// then we look up the request for the ID and compare hashed keys, basically exactly like a normal
// username/password login flow.
_id: string;
key: string;
email: string;
timestamp: Date;
suppressEmail?: boolean;
}

const verifyEmailRequestSchema = new mongoose.Schema<VerifyEmailRequest>({
_id: {
type: String,
required: true,
},
key: {
type: String,
required: true,
},
email: {
type: String,
required: true,
index: true,
},
timestamp: {
type: Date,
expires: 0,
},
suppressEmail: {
type: Boolean,
required: false,
},
Comment on lines +119 to +122
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary for email verification requests, since there isn't really a "confirmation" of the email being verified like there is for password resets. We have this option in the password reset model because there's two scenarios where we do a password reset: 1) explicitly creating a new account and 2) a user-requested password reset. In scenario 2 we send a confirmation email after the change happens (see here), but not in scenario 1.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, why was this marked as resolved? This hasn't been fixed.

});

export const VerifyEmailRequestModel = mongoose.model<VerifyEmailRequest>(
'VerifyEmail',
verifyEmailRequestSchema,
);

export interface MinecraftWhitelist {
_id: string;
mcUuid: string;
Expand Down
39 changes: 37 additions & 2 deletions src/routes/account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,16 @@ router.post(
throw new HttpException(400, { message: `Invalid request: ${error.message}` });
}



await controller.submitCreateAccountRequest(value);

res.render('createAccountSuccess', { email: value.email });
}),
);



router.get(
'/username-ok/:username',
catchErrors(async (req: any, res, next) => {
Expand Down Expand Up @@ -80,15 +84,15 @@ router.post(
router.get(
'/reset',
catchErrors(async (req, res, next) => {
const { error, value } = jf.validateAsClass(req.query, controller.PasswordResetCredentials);
const { error, value } = jf.validateAsClass(req.query, controller.ResetCredentials);

if (error) {
throw new HttpException(400, { message: `Invalid request: ${error.message}` });
}

// I have no idea why joiful throws a fit here but this is the necessary workaround
const resetRequest = await controller.verifyPasswordReset(
value as unknown as controller.PasswordResetCredentials,
value as unknown as controller.ResetCredentials,
);

return res.render('resetPassword', {
Expand All @@ -99,6 +103,37 @@ router.get(
}),
);

// router.get(
// '/init',
// catchErrors(async (req, res, next) => {
// return res.render('initAccount',);
// }),
// );

// router.post(
// '/init',
// catchErrors(async (req, res, next) => {
// const { error, value } = jf.validateAsClass(req.query, controller.ResetCredentials);

// if (error) {
// throw new HttpException(400, { message: `Invalid request: ${error.message}` });
// }

// // I have no idea why joiful throws a fit here but this is the necessary workaround
// const verifyEmail = await controller.verifyEmail(
// value as unknown as controller.ResetCredentials,
// );

// return res.render('initAccountSuccess', {
// id: value.id,
// key: value.key,
// email: verifyEmail.email,
// });
// }),
// );



Copy link
Member

Choose a reason for hiding this comment

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

Not sure why the commented block here but of course let me know if you need any help w/this

/**
*
*/
Expand Down
30 changes: 30 additions & 0 deletions src/util/accountVerify.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import argon2 from 'argon2';
import { nanoid } from 'nanoid';

import { VerifyEmailRequestModel } from '../integration/models';
import { logger } from './logging';

const USERNAME_REGEX = /^[a-z][-a-z0-9]*$/;

export const createVerifyAccountRequest = async (
email: string,
expireHours = 12 * 7,
suppressEmail = false,
): Promise<[string, string]> => {
logger.debug(`Creating account verification ID/key pair for ${email}`);
const verifyId = nanoid();
const verifyKey = nanoid();

const expireDate = new Date();
expireDate.setHours(expireDate.getHours() + expireHours);

await new VerifyEmailRequestModel({
_id: verifyId,
key: await argon2.hash(verifyKey, { raw: false }),
email: email,
timestamp: expireDate,
suppressEmail: suppressEmail,
}).save();

return [verifyId, verifyKey];
};
37 changes: 4 additions & 33 deletions views/createAccount.pug
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,15 @@ include include/sauce-common
| SCCS accounts are available to all Swarthmore students, faculty,
| and staff&mdash;and you'll still be able to access it after
| graduation.
form#createForm.form-signin.needs-validation(action='/account/create', method='post', novalidate)
form#createForm.form-signin.needs-validation(action='/account/init', method='post', novalidate)
input(type='hidden', name='id', value=id)
input(type='hidden', name='key', value=key)
Copy link
Member

Choose a reason for hiding this comment

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

The createAccount page itself (the one that doesn't contain Cygnet data) doesn't need id and key hidden form parameters. Those will go in the other page that requires you to actually click a reset link.

.form-group.mb-3
.form-floating
script(type='text/javascript').
function validateUsername() {
console.log(this);
}
input#usernameInput.form-control(
type='text',
pattern='^[a-z][-a-z0-9]*$',
name='username',
placeholder='Username',
aria-describedby='usernameHelp',
onchange='validateUsername',
required
)
label(for='usernameInput') Username
.valid-feedback Username is available
#usernameFeedback.invalid-feedback
small#usernameHelp.form-text
| This should be at least vaguely
| related to your real name, and contain only lowercase
| letters, numbers, and hyphens.
.form-group.mb-3
.form-floating
input#emailInput.form-control(
Expand All @@ -41,23 +27,8 @@ include include/sauce-common
)
label(for='emailInput') Swarthmore email
#emailFeedback.invalid-feedback Please provide a valid @swarthmore.edu email
.form-group.mb-3
.form-floating
input#nameInput.form-control(type='text', name='name', placeholder='Name', required)
label(for='nameInput') Full name
.invalid-feedback Please provide a valid name
.form-group.mb-3
each val in classes
.form-check.form-check-inline
input.form-check-input(
type='radio',
name='classYear',
id='classYear1' + val,
value=val,
required
)
label.form-check-label(for='classYear1' + val, style='text-transform: capitalize')= /^\d+$/.test(val) ? '\u2019' + val : val
button#submitButton.btn.btn-large.btn-primary.mb-3(type='submit') Request Your Account
button#submitButton.btn.btn-large.btn-primary.mb-3(type='submit') Get Your Account
p.text-muted.mb-0 Already have an account? #[a(href='/login') Sign in].
script(type='text/javascript', src='/dist/js/createAccount.js')
//- .col
Loading