diff --git a/models/action.js b/models/action.js index f3414f3ea..773bccf7e 100644 --- a/models/action.js +++ b/models/action.js @@ -22,8 +22,8 @@ const ActionSchema = new Schema( item_id: String, user_id: String, - // The element that summaries will additionally group on in addtion to their action_type, item_type, and - // item_id. + // The element that summaries will additionally group on in addition to + // their action_type, item_type, and item_id. group_id: String, // Additional metadata stored on the field. diff --git a/perms/reducers/mutation.js b/perms/reducers/mutation.js index d0841ef99..f063fe1c2 100644 --- a/perms/reducers/mutation.js +++ b/perms/reducers/mutation.js @@ -1,4 +1,5 @@ -const { isString } = require('lodash'); +const { get, isString } = require('lodash'); +const moment = require('moment'); const { check } = require('../utils'); const types = require('../constants'); @@ -12,8 +13,22 @@ module.exports = (user, perm) => { isString(user.password) && user.password.length > 0 ); - case types.CHANGE_USERNAME: - return user.status.username.status === 'REJECTED'; + + case types.CHANGE_USERNAME: { + // Only users who have their usernames rejected or those users who + // not changed their usernames within 14 days can change their usernames. + const now = moment(); + return ( + user.status.username.status === 'REJECTED' || + get(user, 'status.username.history', []) + .filter(({ status }) => status === 'CHANGED') + .every(({ created_at }) => + moment(created_at) + .add(14, 'days') + .isAfter(now) + ) + ); + } case types.SET_USERNAME: return user.status.username.status === 'UNSET'; diff --git a/services/users.js b/services/users.js index 657be737a..b860acfeb 100644 --- a/services/users.js +++ b/services/users.js @@ -1,4 +1,5 @@ const uuid = require('uuid'); +const moment = require('moment'); const bcrypt = require('bcryptjs'); const { ErrMaxRateLimit, @@ -234,57 +235,64 @@ class Users { return user; } - static async _setUsername( - id, - username, - fromStatus, - toStatus, - assignedBy, - resetAllowed = false - ) { + static async setUsername(id, username, assignedBy) { try { + const oldestEditTime = moment().subtract(14, 'days'); + + // A username can be set if: + // + // - The previous status was 'UNSET' + // - The username has not been changed within the last 14 days. const query = { id, - 'status.username.status': fromStatus, - }; - if (!resetAllowed) { - query.username = { $ne: username }; - } - - let user = await User.findOneAndUpdate( - query, - { - $set: { - username, - lowercaseUsername: username.toLowerCase(), - 'status.username.status': toStatus, + $or: [ + { + 'status.username.status': 'UNSET', }, - $push: { - 'status.username.history': { - status: toStatus, - assigned_by: assignedBy, - created_at: Date.now(), + { + 'status.username.status': { $in: ['APPROVED', 'SET'] }, + 'status.username.history.created_at': { + $not: { + $gte: oldestEditTime, + }, }, }, + ], + }; + + const update = { + $set: { + username, + lowercaseUsername: username.toLowerCase(), + 'status.username.status': 'SET', }, - { - new: true, - } - ); + $push: { + 'status.username.history': { + status: 'SET', + assigned_by: assignedBy, + created_at: Date.now(), + }, + }, + }; + + let user = await User.findOneAndUpdate(query, update, { + new: true, + }); if (!user) { user = await Users.findById(id); if (user === null) { throw new ErrNotFound(); } - if (user.status.username.status !== fromStatus) { + if ( + !['UNSET', 'APPROVED', 'SET'].includes(user.status.username.status) || + !user.status.username.history.every(({ created_at }) => + oldestEditTime.isAfter(created_at) + ) + ) { throw new ErrPermissionUpdateUsername(); } - if (!resetAllowed && user.username === username) { - throw new ErrSameUsernameProvided(); - } - throw new Error('edit username failed for an unexpected reason'); } @@ -298,12 +306,57 @@ class Users { } } - static async setUsername(id, username, assignedBy) { - return Users._setUsername(id, username, 'UNSET', 'SET', assignedBy, true); - } - static async changeUsername(id, username, assignedBy) { - return Users._setUsername(id, username, 'REJECTED', 'CHANGED', assignedBy); + try { + const query = { + id, + username: { $ne: username }, + 'status.username.status': 'REJECTED', + }; + + const update = { + $set: { + username, + lowercaseUsername: username.toLowerCase(), + 'status.username.status': 'CHANGED', + }, + $push: { + 'status.username.history': { + status: 'CHANGED', + assigned_by: assignedBy, + created_at: Date.now(), + }, + }, + }; + + let user = await User.findOneAndUpdate(query, update, { + new: true, + }); + if (!user) { + user = await Users.findById(id); + if (user === null) { + throw new ErrNotFound(); + } + + if (user.status.username.status !== 'REJECTED') { + throw new ErrPermissionUpdateUsername(); + } + + if (user.username === username) { + throw new ErrSameUsernameProvided(); + } + + throw new Error('edit username failed for an unexpected reason'); + } + + return user; + } catch (err) { + if (err.code === 11000) { + throw new ErrUsernameTaken(); + } + + throw err; + } } /** diff --git a/test/server/graph/mutations/changeUsername.js b/test/server/graph/mutations/changeUsername.js index eaff85434..a5c3daed8 100644 --- a/test/server/graph/mutations/changeUsername.js +++ b/test/server/graph/mutations/changeUsername.js @@ -89,7 +89,7 @@ describe('graph.mutations.changeUsername', () => { expect(res.data.changeUsername.errors).to.have.length(1); expect(res.data.changeUsername.errors[0]).to.have.property( 'translation_key', - 'NOT_AUTHORIZED' + 'EDIT_USERNAME_NOT_AUTHORIZED' ); // Set the user to the desired status. diff --git a/test/server/services/users.js b/test/server/services/users.js index e8a0172f7..a7d7475e9 100644 --- a/test/server/services/users.js +++ b/test/server/services/users.js @@ -2,6 +2,8 @@ const UsersService = require('../../../services/users'); const SettingsService = require('../../../services/settings'); const mailer = require('../../../services/mailer'); const Context = require('../../../graph/context'); +const timekeeper = require('timekeeper'); +const moment = require('moment'); const chai = require('chai'); chai.use(require('chai-as-promised')); @@ -303,6 +305,62 @@ describe('services.UsersService', () => { } }); }); + + if (func === 'setUsername') { + it('should let a user set their username from UNSET', async () => { + const user = mockUsers[0]; + + // Set the user to the desired status. + await UsersService.setUsernameStatus(user.id, 'UNSET'); + await UsersService.setUsername(user.id, 'new_username', null); + }); + + describe('time based', () => { + afterEach(() => { + timekeeper.reset(); + }); + + ['SET', 'APPROVED'].forEach(status => { + it(`should not allow users to change their username if it was changed within 14 of today from ${status}`, async () => { + const user = mockUsers[0]; + + // Set the user to the desired status. + await UsersService.setUsernameStatus(user.id, status); + + timekeeper.travel( + moment() + .add(5, 'days') + .toDate() + ); + + try { + await UsersService.setUsername(user.id, 'new_username', null); + throw new Error('edit was processed successfully'); + } catch (err) { + expect(err).have.property( + 'translation_key', + 'EDIT_USERNAME_NOT_AUTHORIZED' + ); + } + }); + + it(`allows users to change their username if it was changed 14 days before today from ${status}`, async () => { + const user = mockUsers[0]; + + // Set the user to the desired status. + await UsersService.setUsernameStatus(user.id, status); + + timekeeper.travel( + moment() + .add(15, 'days') + .toDate() + ); + + await UsersService.setUsername(user.id, 'new_username', null); + }); + }); + }); + } }); describe('#isValidUsername', () => {