-
Notifications
You must be signed in to change notification settings - Fork 18
[SBA-05] Introduce new password encryption abstractions #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { UnlockMethod, UnlockOptions } from '../types/lnc'; | ||
|
|
||
| /** | ||
| * Pure encryption service interface - no storage dependencies | ||
| */ | ||
| export interface EncryptionService { | ||
| /** | ||
| * Get the method this encryption service handles | ||
| */ | ||
| get method(): UnlockMethod; | ||
|
|
||
| /** | ||
| * Check if the encryption service is currently unlocked | ||
| */ | ||
| get isUnlocked(): boolean; | ||
|
|
||
| /** | ||
| * Encrypt data using the current encryption key | ||
| */ | ||
| encrypt(data: string): Promise<string>; | ||
|
|
||
| /** | ||
| * Decrypt data using the current encryption key | ||
| */ | ||
| decrypt(data: string): Promise<string>; | ||
|
|
||
| /** | ||
| * Unlock the encryption service with the provided options | ||
| */ | ||
| unlock(options: UnlockOptions): Promise<void>; | ||
|
|
||
| /** | ||
| * Lock the encryption service (clear sensitive data) | ||
| */ | ||
| lock(): void; | ||
|
|
||
| /** | ||
| * Check if this service can handle the given unlock method | ||
| */ | ||
| canHandle(method: UnlockMethod): boolean; | ||
|
|
||
| /** | ||
| * Check if this service has stored credentials/data available | ||
| */ | ||
| hasStoredData(): Promise<boolean>; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| import { beforeEach, describe, expect, it } from 'vitest'; | ||
| import { createTestCipher, generateSalt } from '../util/encryption'; | ||
| import { PasswordEncryptionService } from './passwordEncryptionService'; | ||
|
|
||
| describe('PasswordEncryptionService', () => { | ||
| let service: PasswordEncryptionService; | ||
|
|
||
| beforeEach(() => { | ||
| service = new PasswordEncryptionService(); | ||
| }); | ||
|
|
||
| describe('constructor', () => { | ||
| it('should create a locked service when no password is provided', () => { | ||
| expect(service.isUnlocked).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('unlock', () => { | ||
| it('should unlock with a new password (no salt)', async () => { | ||
| await service.unlock({ method: 'password', password: 'testPassword' }); | ||
| expect(service.isUnlocked).toBe(true); | ||
| }); | ||
|
|
||
| it('should unlock with existing salt and cipher', async () => { | ||
| const password = 'testPassword'; | ||
| const salt = generateSalt(); | ||
| const cipher = createTestCipher(password, salt); | ||
|
|
||
| await service.unlock({ method: 'password', password, salt, cipher }); | ||
| expect(service.isUnlocked).toBe(true); | ||
| }); | ||
|
|
||
| it('should throw error for invalid password with existing cipher', async () => { | ||
| const correctPassword = 'correctPassword'; | ||
| const wrongPassword = 'wrongPassword'; | ||
| const salt = generateSalt(); | ||
| const cipher = createTestCipher(correctPassword, salt); | ||
|
|
||
| await expect( | ||
| service.unlock({ | ||
| method: 'password', | ||
| password: wrongPassword, | ||
| salt, | ||
| cipher | ||
| }) | ||
| ).rejects.toThrow('Invalid password'); | ||
| }); | ||
|
|
||
| it('should throw error if password is missing', async () => { | ||
| await expect( | ||
| service.unlock({ method: 'password', password: '' }) | ||
| ).rejects.toThrow('Password is required for password unlock'); | ||
| }); | ||
|
|
||
| it('should throw error for non-password unlock method', async () => { | ||
| // Type assertion to test runtime behavior with invalid input | ||
| await expect( | ||
| service.unlock({ method: 'passkey' as 'password', password: 'test' }) | ||
| ).rejects.toThrow( | ||
| 'Password encryption service requires password unlock method' | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('encrypt/decrypt', () => { | ||
| beforeEach(async () => { | ||
| await service.unlock({ method: 'password', password: 'testPassword' }); | ||
| }); | ||
|
|
||
| it('should encrypt and decrypt data correctly', async () => { | ||
| const plaintext = 'Hello, World!'; | ||
| const encrypted = await service.encrypt(plaintext); | ||
| const decrypted = await service.decrypt(encrypted); | ||
| expect(decrypted).toBe(plaintext); | ||
| }); | ||
|
|
||
| it('should throw error when encrypting while locked', async () => { | ||
| service.lock(); | ||
| await expect(service.encrypt('test')).rejects.toThrow( | ||
| 'Encryption service is locked. Call unlock() first.' | ||
| ); | ||
| }); | ||
|
|
||
| it('should throw error when decrypting while locked', async () => { | ||
| const encrypted = await service.encrypt('test'); | ||
| service.lock(); | ||
| await expect(service.decrypt(encrypted)).rejects.toThrow( | ||
| 'Encryption service is locked. Call unlock() first.' | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('lock', () => { | ||
| it('should clear password and salt when locked', async () => { | ||
| await service.unlock({ method: 'password', password: 'testPassword' }); | ||
| expect(service.isUnlocked).toBe(true); | ||
| service.lock(); | ||
| expect(service.isUnlocked).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getMethod', () => { | ||
| it('should return password as the method', () => { | ||
| expect(service.method).toBe('password'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('canHandle', () => { | ||
| it('should return true for password method', () => { | ||
| expect(service.canHandle('password')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false for non-password methods', () => { | ||
| // Type assertion to test runtime behavior | ||
| expect(service.canHandle('passkey' as 'password')).toBe(false); | ||
| expect(service.canHandle('session' as 'password')).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('hasStoredData', () => { | ||
| it('should always return false (storage is at repository layer)', async () => { | ||
| expect(await service.hasStoredData()).toBe(false); | ||
| await service.unlock({ method: 'password', password: 'test' }); | ||
| expect(await service.hasStoredData()).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getSalt', () => { | ||
| it('should return the salt when unlocked', async () => { | ||
| await service.unlock({ method: 'password', password: 'testPassword' }); | ||
| const salt = service.getSalt(); | ||
| expect(salt).toBeDefined(); | ||
| expect(typeof salt).toBe('string'); | ||
| expect(salt.length).toBe(32); | ||
| }); | ||
|
|
||
| it('should throw error when locked', () => { | ||
| expect(() => service.getSalt()).toThrow( | ||
| 'No salt available - unlock first' | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('createTestCipher', () => { | ||
| it('should create a test cipher when unlocked', async () => { | ||
| await service.unlock({ method: 'password', password: 'testPassword' }); | ||
| const cipher = service.createTestCipher(); | ||
| expect(cipher).toBeDefined(); | ||
| expect(typeof cipher).toBe('string'); | ||
| }); | ||
|
|
||
| it('should throw error when locked', () => { | ||
| expect(() => service.createTestCipher()).toThrow( | ||
| 'No password/salt available - unlock first' | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| import { UnlockMethod, UnlockOptions } from '../types/lnc'; | ||
| import { | ||
| createTestCipher, | ||
| decrypt, | ||
| encrypt, | ||
| generateSalt, | ||
| verifyTestCipher | ||
| } from '../util/encryption'; | ||
| import { EncryptionService } from './encryptionService'; | ||
|
|
||
| /** | ||
| * Pure password-based encryption service. | ||
| * No storage dependencies - just crypto operations. | ||
| */ | ||
| export class PasswordEncryptionService implements EncryptionService { | ||
| private password?: string; | ||
| private salt?: string; | ||
| private isUnlockedState: boolean = false; | ||
|
|
||
| /** | ||
| * Get the unlock method handled by this service (`password`). | ||
| */ | ||
| get method(): UnlockMethod { | ||
| return 'password'; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when a password and salt are set and the service is unlocked. | ||
| */ | ||
| get isUnlocked(): boolean { | ||
| return this.isUnlockedState && !!this.password && !!this.salt; | ||
| } | ||
|
|
||
| /** | ||
| * Encrypt a plaintext string using the currently unlocked password and salt. | ||
| * Throws if the service has not been unlocked. | ||
| */ | ||
| async encrypt(data: string): Promise<string> { | ||
| if (!this.isUnlocked || !this.password || !this.salt) { | ||
| throw new Error('Encryption service is locked. Call unlock() first.'); | ||
| } | ||
| return encrypt(data, this.password, this.salt); | ||
| } | ||
|
|
||
| /** | ||
| * Decrypt a ciphertext string using the currently unlocked password and salt. | ||
| * Throws if the service has not been unlocked. | ||
| */ | ||
| async decrypt(data: string): Promise<string> { | ||
| if (!this.isUnlocked || !this.password || !this.salt) { | ||
| throw new Error('Encryption service is locked. Call unlock() first.'); | ||
| } | ||
| return decrypt(data, this.password, this.salt); | ||
| } | ||
|
|
||
| /** | ||
| * Unlock the service with the given password (and optional salt/cipher). | ||
| * For existing users, verifies the password using the stored test cipher. | ||
| */ | ||
| async unlock(options: UnlockOptions): Promise<void> { | ||
| if (options.method !== 'password') { | ||
| throw new Error( | ||
| 'Password encryption service requires password unlock method' | ||
| ); | ||
| } | ||
|
|
||
| if (!options.password) { | ||
| throw new Error('Password is required for password unlock'); | ||
jamaljsr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| this.password = options.password; | ||
|
|
||
| // If salt is provided (existing user), use it | ||
| if (options.salt) { | ||
| this.salt = options.salt; | ||
|
|
||
| // Verify password is correct by checking test cipher | ||
| if (options.cipher) { | ||
| try { | ||
| if (!verifyTestCipher(options.cipher, this.password, this.salt)) { | ||
| throw new Error('Invalid password'); | ||
| } | ||
| } catch { | ||
| throw new Error('Invalid password'); | ||
| } | ||
| } | ||
| } else { | ||
| // New user - generate new salt | ||
| this.salt = generateSalt(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this part of the code, we generate a salt for a new user. Does it make sense to do this in the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better here so consumers only call one method to unlock the encrypted data. It makes the public API easier to understand IMO. Don't set the |
||
| } | ||
|
|
||
| this.isUnlockedState = true; | ||
| } | ||
|
|
||
| /** | ||
| * Clear all in-memory password material and reset the unlocked state. | ||
| */ | ||
| lock(): void { | ||
| this.password = undefined; | ||
| this.salt = undefined; | ||
| this.isUnlockedState = false; | ||
| } | ||
|
|
||
| /** | ||
| * Return true if this service can handle the provided unlock method. | ||
| */ | ||
| canHandle(method: UnlockMethod): boolean { | ||
| return method === 'password'; | ||
| } | ||
|
|
||
| /** | ||
| * Return whether this service has any stored data of its own. | ||
| * Always false; storage is handled at the repository layer. | ||
| */ | ||
| async hasStoredData(): Promise<boolean> { | ||
| // Password encryption itself doesn't store data | ||
| // The repository layer will check for stored salt/cipher | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Get the current salt (for storage by repository) | ||
| */ | ||
| getSalt(): string { | ||
| if (!this.salt) { | ||
| throw new Error('No salt available - unlock first'); | ||
| } | ||
| return this.salt; | ||
| } | ||
|
|
||
| /** | ||
| * Generate a test cipher (for storage by repository) | ||
| */ | ||
| createTestCipher(): string { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this method needed? Is it for testing?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is used to verify the password is valid before trying to decrypt the actual data. We do this currently in |
||
| if (!this.password || !this.salt) { | ||
| throw new Error('No password/salt available - unlock first'); | ||
| } | ||
| return createTestCipher(this.password, this.salt); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see in the credentialRepository we have an interface, followed by a base class. There's a different pattern in
encryptionServicewhich is just an interface, whichpasswordEncryptionServiceextends. Why have the two patterns?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because
CredentialRepositoryhas a base class with shared functionality. I put the base class in the same file with the interface. TheEncryptionServicedoesn't have this inheritance structure.