[PB-4737] feat(user): add public keys validation for account recovery #851
Conversation
douglas-xt
force-pushed
the
fix/validate-public-keys-on-recovery
branch
from
4ca295d to
212af49
Compare
douglas-xt
force-pushed
the
fix/validate-public-keys-on-recovery
branch
from
212af49 to
c635ea3
Compare
| @ApiProperty() | ||
| @IsOptional() | ||
| kyber?: string; | ||
| } |
There was a problem hiding this comment.
Add a line between this class and the next one
There was a problem hiding this comment.
Why in this class ecc and kyber are optional? Isn't it enough just to have optional public keys field in RecoverAccountDto? Because now we can have {ecc: undefined, kyber: underfined}, {ecc: , kyber: underfined}, {ecc: undefined, kyber: } and I don't think we should accept those as valid input
src/modules/user/user.usecase.ts
Outdated
| } | ||
|
|
||
| if (!withReset && !publicKeys) { | ||
| throw new BadRequestException('Invalid backup file'); |
There was a problem hiding this comment.
'Invalid keys' it's a better naming as you do not know if those come from a backup file or anything else in the future
src/modules/user/user.usecase.ts
Outdated
| publicKeys.kyber && existingKeys.kyber !== publicKeys.kyber; | ||
|
|
||
| if (eccMismatch || kyberMismatch) { | ||
| throw new BadRequestException('Invalid backup file'); |
There was a problem hiding this comment.
Same here. In this case it could be worth it to specify which key is failing.
|
WDYT of this PR @TamaraFinogina ? |
|
|
||
| const user = await this.userRepository.findByUuid(userUuid); | ||
|
|
||
| if (publicKeys) { |
There was a problem hiding this comment.
if withReset = true and mismatching public keys, then backup won't go through?
There was a problem hiding this comment.
@sg-gs Do we require/can send publicKeys for account reset? Becuase now we check public keys and if they fail we abort even if it's withReset=true
src/modules/user/user.usecase.ts
Outdated
|
|
||
| if (publicKeys) { | ||
| const existingKeys = await this.keyServerUseCases.getPublicKeys(user.id); | ||
| const eccMismatch = publicKeys.ecc && existingKeys.ecc !== publicKeys.ecc; |
There was a problem hiding this comment.
If we send publicKeys = {ecc: underfined, kyber: underfined}, then publicKeys.ecc would be false, right? so, the eccMismatch will be false without even checking existingKeys.ecc !== publicKeys.ecc; part, no?
I think we shouldn't accept those keys. Or at least use something like
! publicKeys.ecc || existingKeys.ecc !== publicKeys.ecc
then undefined, null or similar keeys will give a mismatch
src/modules/user/user.usecase.ts
Outdated
| const existingKeys = await this.keyServerUseCases.getPublicKeys(user.id); | ||
| const eccMismatch = publicKeys.ecc && existingKeys.ecc !== publicKeys.ecc; | ||
| const kyberMismatch = | ||
| publicKeys.kyber && existingKeys.kyber !== publicKeys.kyber; |
There was a problem hiding this comment.
@douglas-xt This check would pass for publicKeys = {ecc: underfined, kyber: underfined} without even checking what existingKeys are. I know that now they are not empty, but if we change that in the future this line becomes an issue. Can we remove publicKeys.kyber && and just do the comparison?
|
This PR adds validation of public keys during account recovery to prevent users from restoring their account using a backup file that doesn't belong to them.
When a user performs account recovery, the public keys included in the backup file are now compared against the keys stored in the database. If the keys don't match, the recovery is rejected with a clear error message indicating the backup file doesn't correspond to the account.
This change works in conjunction with updates in the SDK and drive-web that now include public keys in the backup file generation and send them during the recovery request.