[PM-6631] Handle Fido2VerificationException during passkey attestation and assertion

[trmartin4]

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

In https://github.com/bitwarden/server/pull/3615 we handled the Fido2VerificationException when asserting a WebAuthn credential for 2FA.

In this PR, we address the MakeNewCredentialAsync methods similarly, as well as the MakeAssertionAsync when asserting a WebAuthn credential for login, which was missed in #3615 .

📓 We have https://bitwarden.atlassian.net/browse/PM-4172 in the backlog to consolidate the implementations, at which point we should consider an abstraction.

Code changes

• AssertWebAuthnLoginCredentialCommand: Added try/catch around assertion that returns a BadRequestException instead of the unhandled exception returned previously. This will be handled on the client, as it is the pattern already established in the class for communicating assertion errors.
• CreateWebAuthnLoginCredentialCommand: Added try/catch around attestation that returns false along with a log message. I did this instead of throwing a BadRequestException as this is the pattern already established in this command for handling invalid data. I added a log here as returning false gives no indication of the root cause.
• UserService: Added try/catch around attestation that returns false along with a log message. I did this instead of throwing a BadRequestException as this is the pattern already established in this command for handling invalid data. I added a log here as returning false gives no indication of the root cause.

Before you submit

• Please check for formatting errors (dotnet format --verify-no-changes) (required)
• If making database changes - make sure you also update Entity Framework queries and/or migrations
• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team

[trmartin4]

@coroiu do you recall why the Assert... command handles validation issues by throwing a BadRequestException, whereas the Create... command returns false? Is it because we are already returning additional data in the assertion, and it was more flexible to throw the exceptions?

[coroiu]

@coroiu do you recall why the Assert... command handles validation issues by throwing a BadRequestException, whereas the Create... command returns false? Is it because we are already returning additional data in the assertion, and it was more flexible to throw the exceptions?

Yeah, I didn't like returning null to signify a failure so I ended up throwing exceptions. I probably should've changed the Create command to also throw exceptions, just for consistency.

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – 5c490b40-fac7-4e79-ac92-c3113c240fe4

No New Or Fixed Issues Found

[trmartin4]

I had to make the CreateWebAuthnLoginCredentialCommand public in order for the tests to pass, given that I had a dependency on ILogger<T>. I'm not quite sure why, which doesn't give me a good feeling about it - happy to change it back if you have any ideas about another solution.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 52.17391% with 22 lines in your changes missing coverage. Please review.

Project coverage is 41.76%. Comparing base (3ecb900) to head (dd73045). Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Core/Services/Implementations/UserService.cs	0.00%	14 Missing :warning:
...mentations/AssertWebAuthnLoginCredentialCommand.cs	73.33%	4 Missing :warning:
...mentations/CreateWebAuthnLoginCredentialCommand.cs	76.47%	4 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3873      +/-   ##
==========================================
- Coverage   41.76%   41.76%   -0.01%     
==========================================
  Files        1287     1287              
  Lines       61030    61060      +30     
  Branches     5627     5627              
==========================================
+ Hits        25489    25500      +11     
- Misses      34343    34362      +19     
  Partials     1198     1198              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.