Skip to content

Return WebAuthn credential record in create response#7145

Merged
iinuwa merged 2 commits intomainfrom
PM-33110/return-webauthn-credential-in-create-response
Mar 5, 2026
Merged

Return WebAuthn credential record in create response#7145
iinuwa merged 2 commits intomainfrom
PM-33110/return-webauthn-credential-in-create-response

Conversation

@iinuwa
Copy link
Contributor

@iinuwa iinuwa commented Mar 4, 2026

🎟️ Tracking

PM-33110

📔 Objective

Background

For the device auth key feature, we need to be able to delete a specific WebAuthn credential record from a mobile device when the user disables the setting or logs out. Right now, we cannot do that because the only ID that the caller has is the credential ID, which is generated by the caller. The delete endpoint only takes record IDs, which is distinct from the FIDO2 credential ID. The record ID is not returned from the create endpoint, so the caller cannot use it.

Currently, the delete method requires you to fetch all the credentials, and then the user can select which one they want to delete based on the name of the credential record, and pass the record ID to the delete endpoint. This works in the web vault, but not in the case of device auth keys on mobile, where it doesn’t make sense to list all the keys.

I would like to be able to delete a specific record by ID directly instead.

Changes

This PR modifies the WebAuthn create credential endpoint response to return the created object that is saved in the database, which includes the GUID of the generated record. This allows the mobile device to save the ID locally and use it to delete the credential later.

This is backwards compatible, because the response was empty before (unless a client is asserting the empty response). (For the mobile client implementation, I recommend that we require the model so that we can avoid having an optional field stored on devices forever, so the feature will be unavailable on clients connecting to older servers.)

@iinuwa iinuwa added the ai-review Request a Claude code review label Mar 4, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Logo
Checkmarx One – Scan Summary & Details3291f3e1-3ef2-4976-b099-0ce9b68fd5c4

Great job! No new security vulnerabilities introduced in this pull request

@iinuwa iinuwa marked this pull request as ready for review March 4, 2026 20:35
@iinuwa iinuwa requested a review from a team as a code owner March 4, 2026 20:35
@iinuwa iinuwa requested a review from ike-kottlowski March 4, 2026 20:35
@claude
Copy link
Contributor

claude bot commented Mar 4, 2026

Claude finished @iinuwa's task in 4m 35s —— View job


Code Review: Return WebAuthn credential record in create response

  • Read changed files and gather context
  • Review code for correctness, security, and best practices
  • Post review findings

Overall: This is a clean, well-scoped change. The approach of changing the return type from bool to WebAuthnCredential? is straightforward and the backwards compatibility argument in the PR description is sound.

Findings

1. Good: Null safety improvements

The PR removes the #nullable disable pragmas from both ICreateWebAuthnLoginCredentialCommand.cs and CreateWebAuthnLoginCredentialCommand.cs, and properly annotates the nullable parameters (string?). This aligns with ADR 0024 on nullable reference types.

2. Good: New null check on success.Result

The added null check at CreateWebAuthnLoginCredentialCommand.cs:34 is a worthwhile defensive addition. Previously, if MakeNewCredentialAsync returned a result with a null Result, the code would have thrown a NullReferenceException when accessing success.Result.CredentialId. Now it gracefully returns null.

3. Good: Test coverage

The new Post_CredentialCreationFailed_ThrowsBadRequestException test (WebAuthnControllerTests.cs:240-257) explicitly covers the null-return path, and existing tests are updated to assert on the returned credential. The cleanup of unnecessary mock setups in tests that throw before reaching the command (e.g., Post_RequireSsoPolicyApplicable_ThrowsBadRequestException) is also appropriate.

4. Observation: Response model does not include all entity fields

The WebAuthnCredentialResponseModel exposes Id, Name, PrfStatus, EncryptedUserKey, and EncryptedPublicKey. It does not expose CredentialId, PublicKey, Type, AaGuid, Counter, SupportsPrf, EncryptedPrivateKey, or timestamps. This is the existing model used by the GET endpoint as well, so it's consistent. Since the stated goal is to enable deletion by record ID, returning just the Id is sufficient. No security concern here — the response model already existed and was used for listing credentials.

5. Minor: WebAuthnCredential entity still has #nullable disable

The entity class at src/Core/Auth/Entities/WebAuthnCredential.cs:1-2 still has the // FIXME pragma. This is outside the scope of this PR but worth noting as a follow-up, since the command now properly uses nullable annotations and the entity's properties could benefit from the same treatment.

Security

  • The response model is the same one already used by the GET /webauthn list endpoint (WebAuthnController.cs:66-72), so this does not expose any new data — it just returns it at creation time as well.
  • The endpoint requires Policies.Application authorization, same as before.
  • No sensitive unencrypted data (vault data, raw keys) is leaked in the response.

Summary

No blocking issues. The change is minimal, backwards-compatible, well-tested, and achieves the stated goal of allowing callers to obtain the record ID at creation time. The null safety improvements are a nice bonus.

Verdict: Looks good to merge. 👍

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 4, 2026

@codecov
Copy link

codecov bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.69%. Comparing base (165117f) to head (b15ecf6).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...mentations/CreateWebAuthnLoginCredentialCommand.cs 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7145      +/-   ##
==========================================
+ Coverage   56.66%   56.69%   +0.03%     
==========================================
  Files        2024     2025       +1     
  Lines       88602    88644      +42     
  Branches     7905     7906       +1     
==========================================
+ Hits        50205    50257      +52     
+ Misses      36571    36560      -11     
- Partials     1826     1827       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@ike-kottlowski ike-kottlowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good.

You mention in the description that this should be fine as long as a client doesn't assert null. Can you follow up on that to ensure this change won't cause failures on the web client?

@iinuwa
Copy link
Contributor Author

iinuwa commented Mar 5, 2026

You mention in the description that this should be fine as long as a client doesn't assert null. Can you follow up on that to ensure this change won't cause failures on the web client?

Sorry, I should have been clear: I was referring to third-party clients there.

The web client does not assert an empty body, just an HTTP success code: https://github.com/bitwarden/clients/blob/main/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login-admin-api.service.ts#L43-L46. I have also confirmed locally that the changes work with the production client.

@iinuwa iinuwa requested a review from ike-kottlowski March 5, 2026 14:05
@iinuwa iinuwa merged commit dfc736f into main Mar 5, 2026
48 checks passed
@iinuwa iinuwa deleted the PM-33110/return-webauthn-credential-in-create-response branch March 5, 2026 14:47
prograhamming pushed a commit that referenced this pull request Mar 16, 2026
* Return WebAuthn credential record in create response

* Make CreateWebAuthnLoginCredentialCommand null-safe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants