From cb708c720f79507e5c60183119bb8685fab4e70e Mon Sep 17 00:00:00 2001 From: John Lyon-Smith Date: Sat, 24 Mar 2018 10:53:34 -0700 Subject: [PATCH] Fixing last couple of auth dialogs --- server/src/api/routes/AuthRoutes.js | 235 +++++++++++++---------- website/src/API.js | 3 + website/src/App.js | 6 +- website/src/Auth/ConfirmEmail.js | 10 +- website/src/Auth/ForgotPassword.js | 9 +- website/src/Auth/ProtectedRoute.js | 7 +- website/src/Auth/ResetPassword.js | 16 +- website/src/Home/Home.js | 6 +- website/src/Modal/ChangeEmailModal.js | 32 +-- website/src/Modal/ChangePasswordModal.js | 96 +++++---- website/src/Profile/Profile.js | 74 +++---- website/src/Profile/ProfileForm.js | 76 ++++---- website/src/Users/UserForm.js | 4 +- website/src/Users/Users.js | 6 +- website/src/ui/Button.js | 1 + website/src/ui/style.js | 5 +- 16 files changed, 312 insertions(+), 274 deletions(-) diff --git a/server/src/api/routes/AuthRoutes.js b/server/src/api/routes/AuthRoutes.js index b90fe6f..82eaabf 100644 --- a/server/src/api/routes/AuthRoutes.js +++ b/server/src/api/routes/AuthRoutes.js @@ -43,6 +43,10 @@ export class AuthRoutes { app.route('/auth/password/send') .post(this.sendPasswordResetEmail) + // Confirm a password reset token is valid + app.route('/auth/password/confirm') + .post(this.confirmPasswordToken) + // Finish a password reset app.route('/auth/password/reset') .post(this.resetPassword) @@ -52,110 +56,104 @@ export class AuthRoutes { .get(passport.authenticate('bearer', { session: false }), this.whoAmI) } - login(req, res, next) { + async login(req, res, next) { const email = req.body.email const password = req.body.password - - if (!email || !password) { - return next(new createError.BadRequest('Must supply user name and password')) - } - let User = this.db.User - // Lookup the user - User.findOne({ email }).then((user) => { + try { + if (!email || !password) { + createError.BadRequest('Must supply user name and password') + } + + // Lookup the user + const user = await User.findOne({ email }) + if (!user) { // NOTE: Don't return NotFound as that gives too much information away to hackers - return Promise.reject(createError.BadRequest("Email or password incorrect")) + throw createError.BadRequest("Email or password incorrect") } else if (user.emailToken || !user.passwordHash) { - return Promise.reject(createError.Forbidden("Must confirm email and set password")) - } else { - let cr = credential() - - return Promise.all([ - Promise.resolve(user), - cr.verify(JSON.stringify(user.passwordHash), req.body.password) - ]) + throw createError.Forbidden("Must confirm email and set password") } - }).then((arr) => { - const [user, isValid] = arr + + let cr = credential() + const isValid = cr.verify(JSON.stringify(user.passwordHash), req.body.password) + if (isValid) { user.loginToken = loginToken.pack(user._id.toString(), user.email) } else { user.loginToken = null // A bad login removes existing token for this user... } - return user.save() - }).then((savedUser) => { + + const savedUser = await user.save() + if (savedUser.loginToken) { res.set('Authorization', `Bearer ${savedUser.loginToken}`) res.json(savedUser.toClient()) } else { - return Promise.reject(createError.BadRequest('email or password incorrect')) + throw createError.BadRequest('email or password incorrect') } - }).catch((err) => { + } catch(err) { if (err instanceof createError.HttpError) { next(err) } else { - next(createError.InternalServerError(`${err ? err.message : ''}`)) + next(createError.InternalServerError(err.message)) } - }) + } } - logout(req, res, next) { + async logout(req, res, next) { let User = this.db.User - User.findById({ _id: req.user._id }).then((user) => { - if (!user) { - return next(createError.BadRequest()) + try { + const user = await User.findById({ _id: req.user._id }) + + if (user) { + user.loginToken = null + await user.save() } - user.loginToken = null - user.save().then((savedUser) => { - res.json({}) - }) - }).catch((err) => { - next(createError.InternalServerError(`Unable to login. ${err ? err.message : ''}`)) - }) + res.json({}) + } catch(err) { + next(createError.InternalServerError(err.message)) + } } whoAmI(req, res, next) { res.json(req.user.toClient()) } - sendChangeEmailEmail(req, res, next) { + async sendChangeEmailEmail(req, res, next) { let existingEmail = req.body.existingEmail const newEmail = req.body.newEmail let User = this.db.User const isAdmin = !!req.user.administrator - if (existingEmail) { - if (!isAdmin) { - return next(createError.Forbidden('Only admins can resend change email to any user')) + try { + if (existingEmail) { + if (!isAdmin) { + throw createError.Forbidden('Only admins can resend change email to any user') + } + } else { + existingEmail = req.user.email } - } else { - existingEmail = req.user.email - } - let promiseArray = [User.findOne({ email: existingEmail })] + const user = await User.findOne({ email: existingEmail }) + let conflictingUser = null - if (newEmail) { - promiseArray.push(User.findOne({ email: newEmail })) - } - - Promise.all(promiseArray).then((arr) => { - const [user, conflictingUser] = arr + if (newEmail) { + conflictingUser = await User.findOne({ email: newEmail }) + } if (!user) { - return Promise.reject(createError.NotFound(`User with email '${existingEmail}' was not found`)) + throw createError.NotFound(`User with email '${existingEmail}' was not found`) } else if (conflictingUser) { - return Promise.reject(createError.BadRequest(`A user with '${newEmail}' already exists`)) + throw createError.BadRequest(`A user with '${newEmail}' already exists`) } else if (!isAdmin && user.emailToken && (new Date() - user.emailToken.created) < this.sendEmailDelayInSeconds) { - return Promise.reject(createError.BadRequest('Cannot request email confirmation again so soon')) + throw createError.BadRequest('Cannot request email confirmation again so soon') } - return Promise.all([Promise.resolve(user), util.promisify(crypto.randomBytes)(32)]) - }).then((arr) => { - let [ user, buf ] = arr + const buf = await util.promisify(crypto.randomBytes)(32) user.emailToken = { value: urlSafeBase64.encode(buf), @@ -166,8 +164,7 @@ export class AuthRoutes { user.newEmail = newEmail } - return user.save() - }).then((savedUser) => { + const savedUser = await user.save() const userFullName = `${savedUser.firstName} ${savedUser.lastName}` const siteUrl = url.parse(req.headers.referer) let msgs = [] @@ -193,36 +190,41 @@ export class AuthRoutes { supportEmail: this.supportEmail } }) - return this.sendEmail ? this.mq.request('dar-email', 'sendEmail', msgs) : Promise.resolve() - }).then(() => { + + if (this.sendEmail) { + await this.mq.request('dar-email', 'sendEmail', msgs) + } + res.json({}) - }).catch((err) => { + } catch(err) { if (err instanceof createError.HttpError) { next(err) } else { - next(createError.InternalServerError(`Unable to send change email email. ${err.message}`)) + next(createError.InternalServerError(err.message)) } - }) + } } - confirmEmail(req, res, next) { + async confirmEmail(req, res, next) { const token = req.body.emailToken let User = this.db.User - if (!token) { - return next(createError.BadRequest('Invalid request parameters')) - } + try { + if (!token) { + throw createError.BadRequest('Invalid request parameters') + } + + const user = await User.findOne({ 'emailToken.value': token }) - User.findOne({ 'emailToken.value': token }).then((user) => { if (!user) { - return Promise.reject(createError.BadRequest(`The token was not found`)) + throw createError.BadRequest(`The token was not found`) } // Token must not be too old const ageInHours = (new Date() - user.emailToken.created) / 3600000 if (ageInHours > this.maxEmailTokenAgeInHours) { - return Promise.reject(createError.BadRequest(`Token has expired`)) + throw createError.BadRequest(`Token has expired`) } // Remove the email token & any login token as it will become invalid @@ -235,86 +237,109 @@ export class AuthRoutes { user.newEmail = undefined } - let promiseArray = [ Promise.resolve(user) ] + let buf = null + // If user has no password, create reset token for them if (!user.passwordHash) { - // User has no password, create reset token for them - promiseArray.push(util.promisify(crypto.randomBytes)(32)) - } + buf = await util.promisify(crypto.randomBytes)(32) - return Promise.all(promiseArray) - }).then((arr) => { - let [ user, buf ] = arr - - if (buf) { user.passwordToken = { value: urlSafeBase64.encode(buf), created: new Date() } } - return user.save() - }).then((savedUser) => { + const savedUser = await user.save() let obj = {} - // Only because the user has sent us a valid email reset token can we respond with an password reset token + // Only because the user has sent us a valid email reset token + // can we respond with an password reset token. if (savedUser.passwordToken) { obj.passwordToken = savedUser.passwordToken.value } res.json(obj) - }).catch((err) => { + } catch(err) { if (err instanceof createError.HttpError) { next(err) } else { - next(createError.InternalServerError(`Unable to confirm set email token. ${err.message}`)) + next(createError.InternalServerError(err.message)) } - }) + } } - resetPassword(req, res, next) { + async confirmPasswordToken(req, res, next) { const token = req.body.passwordToken - const newPassword = req.body.newPassword let User = this.db.User - let cr = credential() - if (!token || !newPassword) { - return next(createError.BadRequest('Invalid request parameters')) - } + try { + if (!token) { + throw createError.BadRequest('Invalid request parameters') + } + + const user = await User.findOne({ 'passwordToken.value': token }) - User.findOne({ 'passwordToken.value': token }).then((user) => { if (!user) { - return Promise.reject(createError.BadRequest(`The token was not found`)) + throw createError.BadRequest(`The token was not found`) } // Token must not be too old const ageInHours = (new Date() - user.passwordToken.created) / (3600 * 1000) if (ageInHours > this.maxPasswordTokenAgeInHours) { - return Promise.reject(createError.BadRequest(`Token has expired`)) + throw createError.BadRequest(`Token has expired`) + } + + res.json({}) + } catch (err) { + if (err instanceof createError.HttpError) { + next(err) + } else { + next(createError.InternalServerError(err.message)) + } + } + } + + async resetPassword(req, res, next) { + const token = req.body.passwordToken + const newPassword = req.body.newPassword + let User = this.db.User + let cr = credential() + + try { + if (!token || !newPassword) { + throw createError.BadRequest('Invalid request parameters') + } + + const user = await User.findOne({ 'passwordToken.value': token }) + + if (!user) { + throw createError.BadRequest(`The token was not found`) + } + + // Token must not be too old + const ageInHours = (new Date() - user.passwordToken.created) / (3600 * 1000) + + if (ageInHours > this.maxPasswordTokenAgeInHours) { + throw createError.BadRequest(`Token has expired`) } // Remove the password token & any login token user.passwordToken = undefined user.loginToken = undefined - return Promise.all([ - Promise.resolve(user), - cr.hash(newPassword) - ]) - }).then((arr) => { - const [user, json] = arr + const json = await cr.hash(newPassword) + user.passwordHash = JSON.parse(json) - return user.save() - }).then((savedUser) => { + await user.save() res.json({}) - }).catch((err) => { + } catch(err) { if (err instanceof createError.HttpError) { next(err) } else { - next(createError.InternalServerError(`Unable to confirm password reset token. ${err.message}`)) + next(createError.InternalServerError(err.message)) } - }) + } } async changePassword(req, res, next) { diff --git a/website/src/API.js b/website/src/API.js index 32b8435..81aa944 100644 --- a/website/src/API.js +++ b/website/src/API.js @@ -230,6 +230,9 @@ class API extends EventEmitter { sendResetPassword(email) { return this.post('/auth/password/send', { email }) } + confirmResetPassword(passwordToken) { + return this.post('/auth/password/confirm', { passwordToken }) + } resetPassword(passwords) { return this.post('/auth/password/reset', passwords) } diff --git a/website/src/App.js b/website/src/App.js index b647e0f..fc6bea7 100644 --- a/website/src/App.js +++ b/website/src/App.js @@ -97,11 +97,11 @@ export class App extends Component { - - ()} /> + ()} /> + ()} /> - ()} /> + ()} /> diff --git a/website/src/Auth/ConfirmEmail.js b/website/src/Auth/ConfirmEmail.js index ad583d4..eceb687 100644 --- a/website/src/Auth/ConfirmEmail.js +++ b/website/src/Auth/ConfirmEmail.js @@ -2,13 +2,13 @@ import React from 'react' import { api } from 'src/API' import PropTypes from 'prop-types' import { MessageModal, WaitModal } from '../Modal' -import { Logout } from '.' import autobind from 'autobind-decorator' export class ConfirmEmail extends React.Component { static propTypes = { history: PropTypes.oneOfType([PropTypes.array, PropTypes.object]) } + constructor() { super() this.state = { @@ -22,7 +22,9 @@ export class ConfirmEmail extends React.Component { this.setState({ waitModal: { message: 'Validating Email...' } }) if (emailToken) { - api.confirmEmail(emailToken).then((response) => { + api.logout().then(() => { + return api.confirmEmail(emailToken) + }).then((response) => { this.setState({ waitModal: null }) if (response && response.passwordToken) { // API will send a password reset token if this is the first time loggin on @@ -54,10 +56,6 @@ export class ConfirmEmail extends React.Component { render() { const { messageModal, waitModal } = this.state - if (api.loggedInUser) { - return - } - return (
- } - return ( diff --git a/website/src/Auth/ProtectedRoute.js b/website/src/Auth/ProtectedRoute.js index 5c1f4b0..11d9a52 100644 --- a/website/src/Auth/ProtectedRoute.js +++ b/website/src/Auth/ProtectedRoute.js @@ -7,6 +7,7 @@ import autobind from 'autobind-decorator' export class ProtectedRoute extends React.Component { static propTypes = { location: PropTypes.shape({ pathname: PropTypes.string, search: PropTypes.string }), + admin: PropTypes.bool, } @autobind @@ -30,11 +31,11 @@ export class ProtectedRoute extends React.Component { // The API might be in the middle of fetching the user information // Return something and wait for login evint to fire to re-render return
- } else if (user.administrator) { + } else if (!this.props.admin || (this.props.admin && user.administrator)) { return } - } else { - return } + + return } } diff --git a/website/src/Auth/ResetPassword.js b/website/src/Auth/ResetPassword.js index b8873e0..83b4d6a 100644 --- a/website/src/Auth/ResetPassword.js +++ b/website/src/Auth/ResetPassword.js @@ -1,7 +1,6 @@ import React, { Component, Fragment } from 'react' import PropTypes from 'prop-types' import { Box, Text, Image, Column, Row, BoundInput, BoundButton } from 'ui' -import { Logout } from '.' import { MessageModal, WaitModal } from '../Modal' import { api } from 'src/API' import { FormBinder } from 'react-form-binder' @@ -47,13 +46,10 @@ export class ResetPassword extends Component { this.setState({ waitModal: { message: 'Confirming password reset...' } }) if (passwordToken) { - api.confirmResetPassword(passwordToken).then((response) => { - this.setState({ waitModal: null }) - if (response && response.valid) { - this.setState({ tokenConfirmed: true }) - } else { - this.props.history.replace('/') - } + api.logout().then(() => { + return api.confirmResetPassword(passwordToken) + }).then((response) => { + this.setState({ waitModal: null, tokenConfirmed: true }) }).catch((err) => { this.setState({ waitModal: null, @@ -107,10 +103,6 @@ export class ResetPassword extends Component { render() { const { messageModal, waitModal, binder } = this.state - if (api.loggedInUser) { - return - } - return ( diff --git a/website/src/Home/Home.js b/website/src/Home/Home.js index d780ee7..e6ce9ad 100644 --- a/website/src/Home/Home.js +++ b/website/src/Home/Home.js @@ -6,15 +6,15 @@ import { sizeInfo } from 'ui/style' export class Home extends Component { static propTypes = { history: PropTypes.object, - onChangeTitle: PropTypes.func.isRequired, + changeTitle: PropTypes.func.isRequired, } componentDidMount() { - this.props.onChangeTitle('Home') + this.props.changeTitle('Home') } componentWillUnmount() { - this.props.onChangeTitle('') + this.props.changeTitle('') } render() { diff --git a/website/src/Modal/ChangeEmailModal.js b/website/src/Modal/ChangeEmailModal.js index 6c77cec..6996543 100644 --- a/website/src/Modal/ChangeEmailModal.js +++ b/website/src/Modal/ChangeEmailModal.js @@ -14,8 +14,11 @@ export class ChangeEmailModal extends React.Component { } static bindings = { + oldEmail: { + noValue: true, + }, newEmail: { - isValid: (r, v) => (v !== '' && regExpPattern.email.test(v)) + isValid: (r, v) => (v !== '' && regExpPattern.email.test(v) && v !== r.getFieldValue('oldEmail')) }, submit: { isDisabled: (r) => (!r.allValid), @@ -26,7 +29,9 @@ export class ChangeEmailModal extends React.Component { constructor(props) { super(props) this.state = { - binder: new FormBinder({}, ChangeEmailModal.bindings) + binder: new FormBinder({ + oldEmail: props.oldEmail, + }, ChangeEmailModal.bindings) } } @@ -43,12 +48,7 @@ export class ChangeEmailModal extends React.Component { @autobind handleSubmit(e) { e.preventDefault() - let newEmail = null - - if (this.state.binder.anyModified && this.state.binder.allValid) { - newEmail = this.state.binder.getFieldValue('newEmail') - } - + let newEmail = this.state.binder.getFieldValue('newEmail') this.close(newEmail) } @@ -58,10 +58,11 @@ export class ChangeEmailModal extends React.Component { } render() { + const { binder } = this.state + return ( - -
+ + @@ -69,6 +70,11 @@ export class ChangeEmailModal extends React.Component { + + + Change Password + + {this.props.oldEmail} @@ -76,7 +82,7 @@ export class ChangeEmailModal extends React.Component { + binder={binder} /> @@ -87,7 +93,7 @@ export class ChangeEmailModal extends React.Component { - + diff --git a/website/src/Modal/ChangePasswordModal.js b/website/src/Modal/ChangePasswordModal.js index 5edd632..fa2b369 100644 --- a/website/src/Modal/ChangePasswordModal.js +++ b/website/src/Modal/ChangePasswordModal.js @@ -1,7 +1,7 @@ import React from 'react' import PropTypes from 'prop-types' import autobind from 'autobind-decorator' -import { Modal, Button, Icon, Column, Row, Text, BoundInput, BoundButton } from 'ui' +import { Modal, Button, Column, Row, Text, BoundInput, BoundButton } from 'ui' import { FormBinder } from 'react-form-binder' import { sizeInfo } from 'ui/style' @@ -21,8 +21,7 @@ export class ChangePasswordModal extends React.Component { isValid: (r, v) => (v !== '' && v !== r.fields.oldPassword.value) }, reenteredNewPassword: { - alwaysGet: true, - isValid: (r, v) => (v !== '' && v === r.fields.newPassword.value) + isValid: (r, v) => (v !== '' && v === r.getFieldValue('newPassword')), }, submit: { isDisabled: (r) => (!r.allValid), @@ -51,10 +50,11 @@ export class ChangePasswordModal extends React.Component { handleSubmit(e) { e.preventDefault() let passwords = null + const { binder } = this.state - if (this.state.binder.allValid) { - const oldPassword = this.state.binder.getField('oldPassword').value - const newPassword = this.state.binder.getField('newPassword').value + if (binder.allValid) { + const oldPassword = binder.getFieldValue('oldPassword') + const newPassword = binder.getFieldValue('newPassword') passwords = { oldPassword, newPassword } } this.close(passwords) @@ -66,41 +66,57 @@ export class ChangePasswordModal extends React.Component { } render() { + const { binder } = this.state + return ( - - - - Change Password - - - - - - - - - - - - - - - - - - OK - - - - + + + + + + + + + Change Password + + + + + + + + + + + + + + + + + + + + +