PM-32858: Feat: Set and Reset Password flows should use newer request models

[david-livefront]

🎟️ Tracking

PM-32858

📔 Objective

This PR updates the ResetPasswordRequestJson and SetPasswordRequestJson to use the new request standard. The old standard is going to be removed in the future.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed the migration of ResetPasswordRequestJson and SetPasswordRequestJson to the new authentication/unlock-data request format, the new toKdfRequestModel() extension, and the corresponding repository and test updates. The reset-password flow change (logout instead of storing the master password hash) and the PBKDF2 fallback in toKdfRequestModel() look correct, and the two earlier blocking concerns (PBKDF2 iteration type-mismatch and a non-compiling test) appear resolved in the current revision. One previously raised concern remains open and unaddressed.

Code Review Details

• ⚠️ : Making SetPasswordRequestJson a sealed class causes the Retrofit @Body (statically typed as the sealed base) to serialize via the polymorphic serializer, injecting a type class discriminator into the body sent to /accounts/set-password. Tests only assert result.isSuccess against an empty mock response, so they do not catch the extra field. Already flagged in an existing unresolved thread — not re-posting.
  • network/src/main/kotlin/com/bitwarden/network/model/SetPasswordRequestJson.kt:10

[claude]

⚠️ IMPORTANT: iterations fallback is type-inconsistent — PBKDF2 default would be the Argon2 iteration count (3).

Details and fix

When kdfType is null it falls back to PBKDF2_SHA256, but when kdfIterations is null it falls back to DEFAULT_ARGON2_ITERATIONS (3). For PBKDF2 the correct default is DEFAULT_PBKDF2_ITERATIONS (600000). If both fields were ever null, this sends a PBKDF2 master-password hash computed with 3 iterations — a severely weakened hash (CWE-916).

The sibling toSdkParams() above already picks the iteration default based on type. Consider mirroring that here so the fallback iterations match the fallback type:

fun AccountJson.Profile.toKdfRequestModel(): KdfJson {
    val type = this.kdfType ?: KdfTypeJson.PBKDF2_SHA256
    val defaultIterations = when (type) {
        KdfTypeJson.ARGON2_ID -> KdfParamsConstants.DEFAULT_ARGON2_ITERATIONS
        KdfTypeJson.PBKDF2_SHA256 -> KdfParamsConstants.DEFAULT_PBKDF2_ITERATIONS
    }
    return KdfJson(
        kdfType = type,
        iterations = this.kdfIterations ?: defaultIterations,
        memory = this.kdfMemory,
        parallelism = this.kdfParallelism,
    )
}

In the set/reset flows the active account's KDF params are normally populated from the login response, so this is defensive, but the fallback combination is internally inconsistent and fails open to a weak hash.

[claude]

❌ CRITICAL: This test will not compile — it uses constructor parameters and field names removed/renamed in this PR.

Details and fix

After this PR, AccountKeys renamed publicKey/encryptedPrivateKey to accountPublicKey/userKeyEncryptedAccountPrivateKey (lines 213-214 still use the old names). Additionally, SetPasswordRequestJson no longer has passwordHash, kdfIterations, kdfMemory, kdfParallelism, kdfType, or key parameters (lines 204-211). The same problem affects ResetPasswordRequestJson at lines 173-178 and 188-193, which still pass the removed newPasswordHash and key parameters.

Update these call sites to the new constructor shape, e.g.:

body = SetPasswordRequestJson(
    passwordHint = "passwordHint",
    organizationIdentifier = "organizationId",
    accountKeys = SetPasswordRequestJson.AccountKeys(
        accountPublicKey = "public",
        userKeyEncryptedAccountPrivateKey = "private",
    ),
    kdf = KdfJson(
        kdfType = KdfTypeJson.PBKDF2_SHA256,
        iterations = 7,
        memory = null,
        parallelism = null,
    ),
    salt = "email",
    masterPasswordAuthenticationHash = "passwordHash",
    masterKeyWrappedUserKey = "encryptedUserKey",
),

Without this, the :network module test compilation breaks, blocking CI. This was flagged in a prior review and remains unaddressed.

[claude]

⚠️ IMPORTANT: Making this request body a sealed class will inject an unexpected type discriminator field into the serialized request sent to /accounts/set-password.

Details and fix

AuthenticatedAccountsApi.setPassword and AccountsService.setPassword both declare the @Body as the sealed base type SetPasswordRequestJson. With the shared Json config in CoreModule (default classDiscriminator = "type", default classDiscriminatorMode = POLYMORPHIC), kotlinx-serialization resolves the serializer from the declared (sealed) type and emits a class discriminator.

Because the V1/V2 subclasses have no @SerialName, the body becomes:

{ "type": "com.bitwarden.network.model.SetPasswordRequestJson.V1", "kdf": ..., "key": ... }

The server does not model a type field, so this is an extra (and FQCN-leaking) property in the outbound request. This is the only sealed request body in the network module — the other sealed *Json types are all response models, where the discriminator is harmless on the inbound path.

The existing setPassword with v1/v2 request ... tests do not catch this because they only assert result.isSuccess against an empty MockWebServer response; they never inspect the serialized request body.

Consider serializing each concrete subtype directly (e.g. accept V1/V2 at the call boundary and dispatch to distinct API methods or pass the concrete type as @Body), or add a RequestBody/custom-serializer step that strips the discriminator, so the wire format matches what the server expects.

[david-livefront]

The JIT flow uses the V1 model still, this entire flow will be removed once we enable the V2EncryptionJitPassword feature flag.