From b426edd4854adc6e65844d8c54b8998e792b5778 Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Mon, 11 Feb 2019 09:30:29 +0100 Subject: [PATCH] Cleanup reset user password by admin And add some tests --- client/package.json | 3 +- .../users/user-edit/user-edit.component.html | 16 +++++---- .../users/user-edit/user-edit.component.scss | 13 +++++++ .../app/+admin/users/user-edit/user-edit.ts | 5 +++ .../user-edit/user-password.component.html | 16 ++++----- .../user-edit/user-password.component.scss | 1 + .../user-edit/user-password.component.ts | 35 +++---------------- .../users/user-edit/user-update.component.ts | 5 ++- client/src/app/shared/users/user.service.ts | 5 --- server/controllers/api/users/index.ts | 20 +++++------ server/controllers/api/users/me.ts | 2 +- server/initializers/constants.ts | 2 ++ server/lib/emailer.ts | 20 ++--------- server/middlewares/validators/users.ts | 2 ++ server/tests/api/check-params/users.ts | 18 ++++++++++ server/tests/api/users/users.ts | 16 +++++++++ shared/models/users/user-update.model.ts | 1 + shared/utils/users/users.ts | 2 ++ 18 files changed, 97 insertions(+), 85 deletions(-) diff --git a/client/package.json b/client/package.json index 9e5e87d4a..3eea661f1 100644 --- a/client/package.json +++ b/client/package.json @@ -164,7 +164,6 @@ "webpack-cli": "^3.0.8", "webtorrent": "https://github.com/webtorrent/webtorrent#e9b209c7970816fc29e0cc871157a4918d66001d", "whatwg-fetch": "^3.0.0", - "zone.js": "~0.8.5", - "generate-password-browser": "^1.0.2" + "zone.js": "~0.8.5" } } diff --git a/client/src/app/+admin/users/user-edit/user-edit.component.html b/client/src/app/+admin/users/user-edit/user-edit.component.html index 3ce246771..c6566da24 100644 --- a/client/src/app/+admin/users/user-edit/user-edit.component.html +++ b/client/src/app/+admin/users/user-edit/user-edit.component.html @@ -82,12 +82,16 @@ -
+
-

Send a link to reset the password by mail to the user.

- +
+ + +
-

Manually set the user password

- -
\ No newline at end of file +
+ + +
+
diff --git a/client/src/app/+admin/users/user-edit/user-edit.component.scss b/client/src/app/+admin/users/user-edit/user-edit.component.scss index 2b4aae83c..c1cc4ca45 100644 --- a/client/src/app/+admin/users/user-edit/user-edit.component.scss +++ b/client/src/app/+admin/users/user-edit/user-edit.component.scss @@ -32,3 +32,16 @@ input[type=submit], button { margin-top: 55px; margin-bottom: 30px; } + +.danger-zone { + .reset-password-email { + margin-bottom: 30px; + padding-bottom: 30px; + border-bottom: 1px solid rgba(0, 0, 0, 0.1); + + button { + display: block; + margin-top: 0; + } + } +} diff --git a/client/src/app/+admin/users/user-edit/user-edit.ts b/client/src/app/+admin/users/user-edit/user-edit.ts index 021b1feb4..649b35b0c 100644 --- a/client/src/app/+admin/users/user-edit/user-edit.ts +++ b/client/src/app/+admin/users/user-edit/user-edit.ts @@ -8,6 +8,7 @@ export abstract class UserEdit extends FormReactive { videoQuotaDailyOptions: { value: string, label: string }[] = [] roles = Object.keys(USER_ROLE_LABELS).map(key => ({ value: key.toString(), label: USER_ROLE_LABELS[key] })) username: string + userId: number protected abstract serverService: ServerService protected abstract configService: ConfigService @@ -37,6 +38,10 @@ export abstract class UserEdit extends FormReactive { return multiplier * parseInt(this.form.value['videoQuota'], 10) } + resetPassword () { + return + } + protected buildQuotaOptions () { // These are used by a HTML select, so convert key into strings this.videoQuotaOptions = this.configService diff --git a/client/src/app/+admin/users/user-edit/user-password.component.html b/client/src/app/+admin/users/user-edit/user-password.component.html index 822e4688e..a1e1f6216 100644 --- a/client/src/app/+admin/users/user-edit/user-password.component.html +++ b/client/src/app/+admin/users/user-edit/user-password.component.html @@ -1,19 +1,15 @@
-
-
-
- -
-
- +
- +
diff --git a/client/src/app/+admin/users/user-edit/user-password.component.scss b/client/src/app/+admin/users/user-edit/user-password.component.scss index 9185e787c..217d585af 100644 --- a/client/src/app/+admin/users/user-edit/user-password.component.scss +++ b/client/src/app/+admin/users/user-edit/user-password.component.scss @@ -3,6 +3,7 @@ input:not([type=submit]):not([type=checkbox]) { @include peertube-input-text(340px); + display: block; border-top-right-radius: 0; border-bottom-right-radius: 0; diff --git a/client/src/app/+admin/users/user-edit/user-password.component.ts b/client/src/app/+admin/users/user-edit/user-password.component.ts index 30cd21ccd..5b3040440 100644 --- a/client/src/app/+admin/users/user-edit/user-password.component.ts +++ b/client/src/app/+admin/users/user-edit/user-password.component.ts @@ -1,14 +1,11 @@ -import { Component, OnDestroy, OnInit, Input } from '@angular/core' +import { Component, Input, OnInit } from '@angular/core' import { ActivatedRoute, Router } from '@angular/router' -import * as generator from 'generate-password-browser' -import { NotificationsService } from 'angular2-notifications' import { UserService } from '@app/shared/users/user.service' -import { ServerService } from '../../../core' +import { Notifier } from '../../../core' import { User, UserUpdate } from '../../../../../../shared' import { I18n } from '@ngx-translate/i18n-polyfill' import { FormValidatorService } from '@app/shared/forms/form-validators/form-validator.service' import { UserValidatorsService } from '@app/shared/forms/form-validators/user-validators.service' -import { ConfigService } from '@app/+admin/config/shared/config.service' import { FormReactive } from '../../../shared' @Component({ @@ -16,7 +13,7 @@ import { FormReactive } from '../../../shared' templateUrl: './user-password.component.html', styleUrls: [ './user-password.component.scss' ] }) -export class UserPasswordComponent extends FormReactive implements OnInit, OnDestroy { +export class UserPasswordComponent extends FormReactive implements OnInit { error: string username: string showPassword = false @@ -25,12 +22,10 @@ export class UserPasswordComponent extends FormReactive implements OnInit, OnDes constructor ( protected formValidatorService: FormValidatorService, - protected serverService: ServerService, - protected configService: ConfigService, private userValidatorsService: UserValidatorsService, private route: ActivatedRoute, private router: Router, - private notificationsService: NotificationsService, + private notifier: Notifier, private userService: UserService, private i18n: I18n ) { @@ -43,10 +38,6 @@ export class UserPasswordComponent extends FormReactive implements OnInit, OnDes }) } - ngOnDestroy () { - // - } - formValidated () { this.error = undefined @@ -54,8 +45,7 @@ export class UserPasswordComponent extends FormReactive implements OnInit, OnDes this.userService.updateUser(this.userId, userUpdate).subscribe( () => { - this.notificationsService.success( - this.i18n('Success'), + this.notifier.success( this.i18n('Password changed for user {{username}}.', { username: this.username }) ) }, @@ -64,16 +54,6 @@ export class UserPasswordComponent extends FormReactive implements OnInit, OnDes ) } - generatePassword () { - this.form.patchValue({ - password: generator.generate({ - length: 16, - excludeSimilarCharacters: true, - strict: true - }) - }) - } - togglePasswordVisibility () { this.showPassword = !this.showPassword } @@ -81,9 +61,4 @@ export class UserPasswordComponent extends FormReactive implements OnInit, OnDes getFormButtonTitle () { return this.i18n('Update user password') } - - private onUserFetched (userJson: User) { - this.userId = userJson.id - this.username = userJson.username - } } diff --git a/client/src/app/+admin/users/user-edit/user-update.component.ts b/client/src/app/+admin/users/user-edit/user-update.component.ts index 4e4002a73..94ef87b08 100644 --- a/client/src/app/+admin/users/user-edit/user-update.component.ts +++ b/client/src/app/+admin/users/user-edit/user-update.component.ts @@ -1,4 +1,4 @@ -import { Component, OnDestroy, OnInit, Input } from '@angular/core' +import { Component, OnDestroy, OnInit } from '@angular/core' import { ActivatedRoute, Router } from '@angular/router' import { Subscription } from 'rxjs' import { Notifier } from '@app/core' @@ -93,8 +93,7 @@ export class UserUpdateComponent extends UserEdit implements OnInit, OnDestroy { resetPassword () { this.userService.askResetPassword(this.userEmail).subscribe( () => { - this.notificationsService.success( - this.i18n('Success'), + this.notifier.success( this.i18n('An email asking for password reset has been sent to {{username}}.', { username: this.username }) ) }, diff --git a/client/src/app/shared/users/user.service.ts b/client/src/app/shared/users/user.service.ts index d0abc7def..cc5c051f1 100644 --- a/client/src/app/shared/users/user.service.ts +++ b/client/src/app/shared/users/user.service.ts @@ -103,11 +103,6 @@ export class UserService { ) } - resetUserPassword (userId: number) { - return this.authHttp.post(UserService.BASE_USERS_URL + userId + '/reset-password', {}) - .pipe(catchError(err => this.restExtractor.handleError(err))) - } - verifyEmail (userId: number, verificationString: string) { const url = `${UserService.BASE_USERS_URL}/${userId}/verify-email` const body = { diff --git a/server/controllers/api/users/index.ts b/server/controllers/api/users/index.ts index beac6d8b1..e3533a7f6 100644 --- a/server/controllers/api/users/index.ts +++ b/server/controllers/api/users/index.ts @@ -3,7 +3,6 @@ import * as RateLimit from 'express-rate-limit' import { UserCreate, UserRight, UserRole, UserUpdate } from '../../../../shared' import { logger } from '../../../helpers/logger' import { getFormattedObjects } from '../../../helpers/utils' -import { pseudoRandomBytesPromise } from '../../../helpers/core-utils' import { CONFIG, RATES_LIMIT, sequelizeTypescript } from '../../../initializers' import { Emailer } from '../../../lib/emailer' import { Redis } from '../../../lib/redis' @@ -230,7 +229,7 @@ async function unblockUser (req: express.Request, res: express.Response, next: e return res.status(204).end() } -async function blockUser (req: express.Request, res: express.Response, next: express.NextFunction) { +async function blockUser (req: express.Request, res: express.Response) { const user: UserModel = res.locals.user const reason = req.body.reason @@ -239,23 +238,23 @@ async function blockUser (req: express.Request, res: express.Response, next: exp return res.status(204).end() } -function getUser (req: express.Request, res: express.Response, next: express.NextFunction) { +function getUser (req: express.Request, res: express.Response) { return res.json((res.locals.user as UserModel).toFormattedJSON()) } -async function autocompleteUsers (req: express.Request, res: express.Response, next: express.NextFunction) { +async function autocompleteUsers (req: express.Request, res: express.Response) { const resultList = await UserModel.autoComplete(req.query.search as string) return res.json(resultList) } -async function listUsers (req: express.Request, res: express.Response, next: express.NextFunction) { +async function listUsers (req: express.Request, res: express.Response) { const resultList = await UserModel.listForApi(req.query.start, req.query.count, req.query.sort, req.query.search) return res.json(getFormattedObjects(resultList.data, resultList.total)) } -async function removeUser (req: express.Request, res: express.Response, next: express.NextFunction) { +async function removeUser (req: express.Request, res: express.Response) { const user: UserModel = res.locals.user await user.destroy() @@ -265,12 +264,13 @@ async function removeUser (req: express.Request, res: express.Response, next: ex return res.sendStatus(204) } -async function updateUser (req: express.Request, res: express.Response, next: express.NextFunction) { +async function updateUser (req: express.Request, res: express.Response) { const body: UserUpdate = req.body const userToUpdate = res.locals.user as UserModel const oldUserAuditView = new UserAuditView(userToUpdate.toFormattedJSON()) const roleChanged = body.role !== undefined && body.role !== userToUpdate.role + if (body.password !== undefined) userToUpdate.password = body.password if (body.email !== undefined) userToUpdate.email = body.email if (body.emailVerified !== undefined) userToUpdate.emailVerified = body.emailVerified if (body.videoQuota !== undefined) userToUpdate.videoQuota = body.videoQuota @@ -280,11 +280,11 @@ async function updateUser (req: express.Request, res: express.Response, next: ex const user = await userToUpdate.save() // Destroy user token to refresh rights - if (roleChanged) await deleteUserToken(userToUpdate.id) + if (roleChanged || body.password !== undefined) await deleteUserToken(userToUpdate.id) auditLogger.update(getAuditIdFromRes(res), new UserAuditView(user.toFormattedJSON()), oldUserAuditView) - // Don't need to send this update to followers, these attributes are not propagated + // Don't need to send this update to followers, these attributes are not federated return res.sendStatus(204) } @@ -294,7 +294,7 @@ async function askResetUserPassword (req: express.Request, res: express.Response const verificationString = await Redis.Instance.setResetPasswordVerificationString(user.id) const url = CONFIG.WEBSERVER.URL + '/reset-password?userId=' + user.id + '&verificationString=' + verificationString - await Emailer.Instance.addForgetPasswordEmailJob(user.email, url) + await Emailer.Instance.addPasswordResetEmailJob(user.email, url) return res.status(204).end() } diff --git a/server/controllers/api/users/me.ts b/server/controllers/api/users/me.ts index 94a2b8732..d5e154869 100644 --- a/server/controllers/api/users/me.ts +++ b/server/controllers/api/users/me.ts @@ -167,7 +167,7 @@ async function deleteMe (req: express.Request, res: express.Response) { return res.sendStatus(204) } -async function updateMe (req: express.Request, res: express.Response, next: express.NextFunction) { +async function updateMe (req: express.Request, res: express.Response) { const body: UserUpdateMe = req.body const user: UserModel = res.locals.oauth.token.user diff --git a/server/initializers/constants.ts b/server/initializers/constants.ts index 98f8f8694..e5c4c4e63 100644 --- a/server/initializers/constants.ts +++ b/server/initializers/constants.ts @@ -711,6 +711,8 @@ if (isTestInstance() === true) { CACHE.VIDEO_CAPTIONS.MAX_AGE = 3000 MEMOIZE_TTL.OVERVIEWS_SAMPLE = 1 ROUTE_CACHE_LIFETIME.OVERVIEWS.VIDEOS = '0ms' + + RATES_LIMIT.LOGIN.MAX = 20 } updateWebserverUrls() diff --git a/server/lib/emailer.ts b/server/lib/emailer.ts index 7681164b3..672414cc0 100644 --- a/server/lib/emailer.ts +++ b/server/lib/emailer.ts @@ -101,22 +101,6 @@ class Emailer { return JobQueue.Instance.createJob({ type: 'email', payload: emailPayload }) } - addForceResetPasswordEmailJob (to: string, resetPasswordUrl: string) { - const text = `Hi dear user,\n\n` + - `Your password has been reset on ${CONFIG.WEBSERVER.HOST}! ` + - `Please follow this link to reset it: ${resetPasswordUrl}\n\n` + - `Cheers,\n` + - `PeerTube.` - - const emailPayload: EmailPayload = { - to: [ to ], - subject: 'Reset of your PeerTube password', - text - } - - return JobQueue.Instance.createJob({ type: 'email', payload: emailPayload }) - } - addNewFollowNotification (to: string[], actorFollow: ActorFollowModel, followType: 'account' | 'channel') { const followerName = actorFollow.ActorFollower.Account.getDisplayName() const followingName = (actorFollow.ActorFollowing.VideoChannel || actorFollow.ActorFollowing.Account).getDisplayName() @@ -312,9 +296,9 @@ class Emailer { return JobQueue.Instance.createJob({ type: 'email', payload: emailPayload }) } - addForgetPasswordEmailJob (to: string, resetPasswordUrl: string) { + addPasswordResetEmailJob (to: string, resetPasswordUrl: string) { const text = `Hi dear user,\n\n` + - `It seems you forgot your password on ${CONFIG.WEBSERVER.HOST}! ` + + `A reset password procedure for your account ${to} has been requested on ${CONFIG.WEBSERVER.HOST} ` + `Please follow this link to reset it: ${resetPasswordUrl}\n\n` + `If you are not the person who initiated this request, please ignore this email.\n\n` + `Cheers,\n` + diff --git a/server/middlewares/validators/users.ts b/server/middlewares/validators/users.ts index 1bb0bfb1b..a52e3060a 100644 --- a/server/middlewares/validators/users.ts +++ b/server/middlewares/validators/users.ts @@ -113,6 +113,7 @@ const deleteMeValidator = [ const usersUpdateValidator = [ param('id').isInt().not().isEmpty().withMessage('Should have a valid id'), + body('password').optional().custom(isUserPasswordValid).withMessage('Should have a valid password'), body('email').optional().isEmail().withMessage('Should have a valid email attribute'), body('emailVerified').optional().isBoolean().withMessage('Should have a valid email verified attribute'), body('videoQuota').optional().custom(isUserVideoQuotaValid).withMessage('Should have a valid user quota'), @@ -233,6 +234,7 @@ const usersAskResetPasswordValidator = [ logger.debug('Checking usersAskResetPassword parameters', { parameters: req.body }) if (areValidationErrors(req, res)) return + const exists = await checkUserEmailExist(req.body.email, res, false) if (!exists) { logger.debug('User with email %s does not exist (asking reset password).', req.body.email) diff --git a/server/tests/api/check-params/users.ts b/server/tests/api/check-params/users.ts index a3e8e2e9c..13be8b460 100644 --- a/server/tests/api/check-params/users.ts +++ b/server/tests/api/check-params/users.ts @@ -464,6 +464,24 @@ describe('Test users API validators', function () { await makePutBodyRequest({ url: server.url, path: path + userId, token: server.accessToken, fields }) }) + it('Should fail with a too small password', async function () { + const fields = { + currentPassword: 'my super password', + password: 'bla' + } + + await makePutBodyRequest({ url: server.url, path: path + userId, token: server.accessToken, fields }) + }) + + it('Should fail with a too long password', async function () { + const fields = { + currentPassword: 'my super password', + password: 'super'.repeat(61) + } + + await makePutBodyRequest({ url: server.url, path: path + userId, token: server.accessToken, fields }) + }) + it('Should fail with an non authenticated user', async function () { const fields = { videoQuota: 42 diff --git a/server/tests/api/users/users.ts b/server/tests/api/users/users.ts index ad98ab1c7..c4465d541 100644 --- a/server/tests/api/users/users.ts +++ b/server/tests/api/users/users.ts @@ -501,6 +501,22 @@ describe('Test users', function () { accessTokenUser = await userLogin(server, user) }) + it('Should be able to update another user password', async function () { + await updateUser({ + url: server.url, + userId, + accessToken, + password: 'password updated' + }) + + await getMyUserVideoQuotaUsed(server.url, accessTokenUser, 401) + + await userLogin(server, user, 400) + + user.password = 'password updated' + accessTokenUser = await userLogin(server, user) + }) + it('Should be able to list video blacklist by a moderator', async function () { await getBlacklistedVideosList(server.url, accessTokenUser) }) diff --git a/shared/models/users/user-update.model.ts b/shared/models/users/user-update.model.ts index abde51321..cd215bab3 100644 --- a/shared/models/users/user-update.model.ts +++ b/shared/models/users/user-update.model.ts @@ -1,6 +1,7 @@ import { UserRole } from './user-role' export interface UserUpdate { + password?: string email?: string emailVerified?: boolean videoQuota?: number diff --git a/shared/utils/users/users.ts b/shared/utils/users/users.ts index 61a7e3757..7191b263e 100644 --- a/shared/utils/users/users.ts +++ b/shared/utils/users/users.ts @@ -213,11 +213,13 @@ function updateUser (options: { emailVerified?: boolean, videoQuota?: number, videoQuotaDaily?: number, + password?: string, role?: UserRole }) { const path = '/api/v1/users/' + options.userId const toSend = {} + if (options.password !== undefined && options.password !== null) toSend['password'] = options.password if (options.email !== undefined && options.email !== null) toSend['email'] = options.email if (options.emailVerified !== undefined && options.emailVerified !== null) toSend['emailVerified'] = options.emailVerified if (options.videoQuota !== undefined && options.videoQuota !== null) toSend['videoQuota'] = options.videoQuota