Merge pull request #1935 from hedgedoc/feature/verify-password-change

src/api/private/auth/auth.controller.ts

@@ -9,10 +9,10 @@ import {
   ConflictException,
   Controller,
   Delete,
-  NotFoundException,
   Post,
   Put,
   Req,
+  UnauthorizedException,
   UseGuards,
 } from '@nestjs/common';
 import { Session } from 'express-session';
@@ -70,6 +70,10 @@ export class AuthController {
     @Body() changePasswordDto: UpdatePasswordDto,
   ): Promise<void> {
     try {
+      await this.identityService.checkLocalPassword(
+        user,
+        changePasswordDto.currentPassword,
+      );
       await this.identityService.updateLocalPassword(
         user,
         changePasswordDto.newPassword,
@@ -77,7 +81,9 @@ export class AuthController {
       return;
     } catch (e) {
       if (e instanceof NotInDBError) {
-        throw new NotFoundException(e.message);
+        throw new UnauthorizedException(
+          'Verifying your identity with the current password did not work.',
+        );
       }
       throw e;
     }

src/identity/identity.service.spec.ts

@@ -102,9 +102,9 @@ describe('IdentityService', () => {
       ) as Identity;
       identity.passwordHash = await hashPassword(password);
       user.identities = Promise.resolve([identity]);
-      await expect(
+      await expect(service.checkLocalPassword(user, password)).resolves.toEqual(
-        service.loginWithLocalIdentity(user, password),
+        undefined,
-      ).resolves.toEqual(undefined);
+      );
     });
     describe('fails', () => {
       it('when user has no local identity', async () => {

src/identity/identity.service.ts

@@ -73,20 +73,20 @@ export class IdentityService {
    * @param {string} password - the password to use
    * @throws {NotInDBError} the specified user can't be logged in
    */
-  async loginWithLocalIdentity(user: User, password: string): Promise<void> {
+  async checkLocalPassword(user: User, password: string): Promise<void> {
     const internalIdentity: Identity | undefined =
       await getFirstIdentityFromUser(user, ProviderType.LOCAL);
     if (internalIdentity === undefined) {
       this.logger.debug(
         `The user with the username ${user.username} does not have a internal identity.`,
-        'loginWithLocalIdentity',
+        'checkLocalPassword',
       );
       throw new NotInDBError();
     }
     if (!(await checkPassword(password, internalIdentity.passwordHash ?? ''))) {
       this.logger.debug(
         `Password check for ${user.username} did not succeed.`,
-        'loginWithLocalIdentity',
+        'checkLocalPassword',
       );
       throw new NotInDBError();
     }

src/identity/local/local.strategy.ts

@@ -30,7 +30,7 @@ export class LocalStrategy extends PassportStrategy(Strategy, 'local') {
       const user = await this.userService.getUserByUsername(username, [
         UserRelationEnum.IDENTITIES,
       ]);
-      await this.identityService.loginWithLocalIdentity(user, password);
+      await this.identityService.checkLocalPassword(user, password);
       return user;
     } catch (e) {
       if (e instanceof NotInDBError) {

src/identity/local/update-password.dto.ts

@@ -6,6 +6,8 @@
 import { IsString } from 'class-validator';
 
 export class UpdatePasswordDto {
+  @IsString()
+  currentPassword: string;
   @IsString()
   newPassword: string;
 }

test/private-api/auth.e2e-spec.ts

@@ -112,6 +112,7 @@ describe('Auth', () => {
     it('works', async () => {
       // Change password
       const changePasswordDto: UpdatePasswordDto = {
+        currentPassword: password,
         newPassword: newPassword,
       };
       await request(testSetup.app.getHttpServer())
@@ -133,6 +134,7 @@ describe('Auth', () => {
       cookie = response.get('Set-Cookie')[0];
       // Reset password
       const changePasswordBackDto: UpdatePasswordDto = {
+        currentPassword: newPassword,
         newPassword: password,
       };
       await request(testSetup.app.getHttpServer())
@@ -146,6 +148,7 @@ describe('Auth', () => {
       testSetup.configService.get('authConfig').local.enableLogin = false;
       // Try to change password
       const changePasswordDto: UpdatePasswordDto = {
+        currentPassword: password,
         newPassword: newPassword,
       };
       await request(testSetup.app.getHttpServer())
@@ -177,6 +180,29 @@ describe('Auth', () => {
         .send(JSON.stringify(loginOldPasswordDto))
         .expect(201);
     });
+    it('fails, when old password is wrong', async () => {
+      // Try to change password
+      const changePasswordDto: UpdatePasswordDto = {
+        currentPassword: 'wrong',
+        newPassword: newPassword,
+      };
+      await request(testSetup.app.getHttpServer())
+        .put('/api/private/auth/local')
+        .set('Content-Type', 'application/json')
+        .set('Cookie', cookie)
+        .send(JSON.stringify(changePasswordDto))
+        .expect(401);
+      // old password still does work for login
+      const loginOldPasswordDto: LoginDto = {
+        password: password,
+        username: username,
+      };
+      await request(testSetup.app.getHttpServer())
+        .post('/api/private/auth/local/login')
+        .set('Content-Type', 'application/json')
+        .send(JSON.stringify(loginOldPasswordDto))
+        .expect(201);
+    });
   });
 
   describe('POST /auth/local/login', () => {

test/private-api/register-and-login.e2e-spec.ts

@@ -114,6 +114,7 @@ describe('Register and Login', () => {
       .set('Content-Type', 'application/json')
       .send(
         JSON.stringify({
+          currentPassword: PASSWORD,
           newPassword: 'newPassword',
         }),
       )