fix: mfa settings encryption version (standardnotes#57)

Author: karolsojko

migrations/1630661830850-fix_encryption_version_on_mfa_settings.ts

@@ -0,0 +1,11 @@
+import { MigrationInterface, QueryRunner } from 'typeorm'
+
+export class fixEncryptionVersionOnMfaSettings1630661830850 implements MigrationInterface {
+  public async up(queryRunner: QueryRunner): Promise<void> {
+    await queryRunner.query('UPDATE `settings` SET `server_encryption_version` = 1 WHERE `server_encryption_version` = 2')
+  }
+
+  public async down(): Promise<void> {
+    return
+  }
+}

src/Controller/SettingsController.spec.ts

@@ -136,7 +136,7 @@ describe('SettingsController', () => {
     expect(result.statusCode).toEqual(400)
   })
 
-  it('should update user mfa setting with default encoded and encrypted setting', async () => {
+  it('should update user mfa setting with default encrypted setting', async () => {
     request.params.userUuid = '1-2-3'
     request.body = {
       uuid: '2-3-4',
@@ -154,7 +154,7 @@ describe('SettingsController', () => {
       props: {
         createdAt: 123,
         name: 'MFA_SECRET',
-        serverEncryptionVersion: Setting.ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED,
+        serverEncryptionVersion: Setting.ENCRYPTION_VERSION_DEFAULT,
         sensitive: true,
         updatedAt: 234,
         uuid: '2-3-4',
@@ -203,7 +203,7 @@ describe('SettingsController', () => {
     request.body = {
       uuid: '2-3-4',
       value: 'test',
-      serverEncryptionVersion: Setting.ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED,
+      serverEncryptionVersion: Setting.ENCRYPTION_VERSION_DEFAULT,
       createdAt: 123,
       updatedAt: 234,
     }
@@ -217,7 +217,7 @@ describe('SettingsController', () => {
       props: {
         createdAt: 123,
         name: 'MFA_SECRET',
-        serverEncryptionVersion: 2,
+        serverEncryptionVersion: 1,
         updatedAt: 234,
         uuid: '2-3-4',
         value: 'test',

src/Controller/SettingsController.ts

@@ -85,7 +85,7 @@ export class SettingsController extends BaseHttpController {
     const {
       uuid,
       value,
-      serverEncryptionVersion = Setting.ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED,
+      serverEncryptionVersion = Setting.ENCRYPTION_VERSION_DEFAULT,
       createdAt,
       updatedAt,
     } = request.body

src/Domain/Encryption/CrypterNode.spec.ts

@@ -2,18 +2,16 @@ import { Aes256GcmEncrypted } from '@standardnotes/sncrypto-common'
 import { SnCryptoNode } from '@standardnotes/sncrypto-node'
 import { Logger } from 'winston'
 import { User } from '../User/User'
-import { UserRepositoryInterface } from '../User/UserRepositoryInterface'
 import { CrypterNode } from './CrypterNode'
 
 describe('CrypterNode', () => {
   let crypto: SnCryptoNode
   let user: User
-  let userRepository: UserRepositoryInterface
   let logger: Logger
 
   const iv = 'iv'
 
-  const createCrypter = () => new CrypterNode(serverKey, crypto, userRepository, logger)
+  const createCrypter = () => new CrypterNode(serverKey, crypto, logger)
 
   const makeEncrypted = (ciphertext: string): Aes256GcmEncrypted<string> => {
     return {
@@ -48,15 +46,12 @@ describe('CrypterNode', () => {
     user = {} as jest.Mocked<User>
     user.encryptedServerKey = version(encryptedUserKey)
 
-    userRepository = {} as jest.Mocked<UserRepositoryInterface>
-    userRepository.save = jest.fn()
-
     logger = {} as jest.Mocked<Logger>
     logger.debug = jest.fn()
   })
 
   it('should fail to instantiate on non-32-byte key', async () => {
-    expect(() => new CrypterNode('short-key', crypto, userRepository, logger)).toThrow()
+    expect(() => new CrypterNode('short-key', crypto, logger)).toThrow()
   })
 
   it('should encrypt a value for user', async () => {
@@ -79,17 +74,6 @@ describe('CrypterNode', () => {
     expect(crypto.aes256GcmDecrypt).toHaveBeenNthCalledWith(2, encrypted, decrypted)
   })
 
-  it('should generate an encrypted server key for user during decryption if one does not exist', async () => {
-    user.encryptedServerKey = null
-    user.serverEncryptionVersion = 0
-
-    expect(await createCrypter().decryptForUser(version(encrypted), user)).toEqual(decrypted)
-
-    expect(crypto.aes256GcmDecrypt).toHaveBeenCalledTimes(2)
-
-    expect(userRepository.save).toHaveBeenCalled()
-  })
-
   it('should generate an encrypted user server key', async () => {
     const anotherUserKey = 'anotherUserKey'
     crypto.generateRandomKey = jest.fn()

src/Domain/Encryption/CrypterNode.ts

@@ -4,15 +4,13 @@ import { inject, injectable } from 'inversify'
 import { Logger } from 'winston'
 import TYPES from '../../Bootstrap/Types'
 import { User } from '../User/User'
-import { UserRepositoryInterface } from '../User/UserRepositoryInterface'
 import { CrypterInterface } from './CrypterInterface'
 
 @injectable()
 export class CrypterNode implements CrypterInterface {
   constructor (
     @inject(TYPES.ENCRYPTION_SERVER_KEY) private encryptionServerKey: string,
     @inject(TYPES.SnCryptoNode) private cryptoNode: SnCryptoNode,
-    @inject(TYPES.UserRepository) private userRepository: UserRepositoryInterface,
     @inject(TYPES.Logger) private logger: Logger,
   ) {
     const keyBuffer = Buffer.from(encryptionServerKey, 'hex')
@@ -64,12 +62,6 @@ export class CrypterNode implements CrypterInterface {
   }
 
   async decryptUserServerKey(user: User): Promise<string> {
-    if (!user.encryptedServerKey) {
-      user.encryptedServerKey = await this.generateEncryptedUserServerKey()
-      user.serverEncryptionVersion = User.DEFAULT_ENCRYPTION_VERSION
-      await this.userRepository.save(user)
-    }
-
     const encrypted = this.parseVersionedEncrypted(user.encryptedServerKey as string)
 
     return this.cryptoNode.aes256GcmDecrypt(encrypted, this.encryptionServerKey)

src/Domain/Setting/Setting.ts

@@ -6,7 +6,6 @@ import { User } from '../User/User'
 export class Setting {
   static readonly ENCRYPTION_VERSION_UNENCRYPTED = 0
   static readonly ENCRYPTION_VERSION_DEFAULT = 1
-  static readonly ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED = 2
 
   @PrimaryGeneratedColumn('uuid')
   uuid: string

src/Domain/Setting/SettingFactory.ts

@@ -69,7 +69,6 @@ export class SettingFactory {
     case Setting.ENCRYPTION_VERSION_UNENCRYPTED:
       return value
     case Setting.ENCRYPTION_VERSION_DEFAULT:
-    case Setting.ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED:
       return this.crypter.encryptForUser(value as string, user)
     default:
       throw Error(`Unrecognized encryption version: ${serverEncryptionVersion}!`)

src/Domain/Setting/SettingService.spec.ts

@@ -132,20 +132,4 @@ describe('SettingService', () => {
 
     expect(await createService().findSetting({ userUuid: '1-2-3', settingName: 'test' as SettingName })).toEqual(undefined)
   })
-
-  it('should decrypt a encoded value of a setting for user', async () => {
-    setting = {
-      value: 'encoded_and_encrypted',
-      serverEncryptionVersion: Setting.ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED,
-    } as jest.Mocked<Setting>
-
-    settingRepository.findLastByNameAndUserUuid = jest.fn().mockReturnValue(setting)
-
-    crypter.decryptForUser = jest.fn().mockReturnValue('encoded_and_decrypted')
-
-    expect(await createService().findSetting({ userUuid: '1-2-3', settingName: 'test' as SettingName })).toEqual({
-      serverEncryptionVersion: 2,
-      value: 'encoded_and_decrypted',
-    })
-  })
 })

src/Domain/Setting/SettingService.ts

@@ -35,12 +35,7 @@ export class SettingService implements SettingServiceInterface {
       return undefined
     }
 
-    if (setting.value !== null &&
-      (
-        setting.serverEncryptionVersion === Setting.ENCRYPTION_VERSION_DEFAULT ||
-        setting.serverEncryptionVersion === Setting.ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED
-      )
-    ) {
+    if (setting.value !== null && setting.serverEncryptionVersion === Setting.ENCRYPTION_VERSION_DEFAULT) {
       const user = await this.userRepository.findOneByUuid(dto.userUuid)
 
       if (user === undefined) {

src/Domain/UseCase/GetSettings/GetSettings.ts

@@ -48,12 +48,7 @@ export class GetSettings implements UseCaseInterface {
     }
 
     for (const setting of settings) {
-      if (setting.value !== null &&
-        (
-          setting.serverEncryptionVersion === Setting.ENCRYPTION_VERSION_DEFAULT ||
-          setting.serverEncryptionVersion === Setting.ENCRYPTION_VERSION_CLIENT_ENCODED_AND_SERVER_ENCRYPTED
-        )
-      ) {
+      if (setting.value !== null && setting.serverEncryptionVersion === Setting.ENCRYPTION_VERSION_DEFAULT) {
         setting.value = await this.crypter.decryptForUser(setting.value, user)
       }
     }