[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 #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]

Did I write this?? I must've completely missed the fact that _fido2.MakeAssertionAsync can throw exceptions... Good catch!

❓ issue: So one thing we need to be careful with here is that we don't allow credential and/or user enumeration. If we have unique exception messages here then the caller will be able to differentiate between

• Invalid Credential -> Not found in database
• Unable to verify credential -> Found in database, but invalid assertion

[trmartin4]

I've adjusted the messages with bd46adc (#3873).

[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]

Checkmarx One – Scan Summary & Details – a9dbbb85-c9d0-45f5-a0ff-bbb5df70b195

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]

Codecov Report

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

Project coverage is 38.66%. Comparing base (fd173e8) to head (6c2f818).

Files	Patch %	Lines
src/Core/Services/Implementations/UserService.cs	0.00%	14 Missing ⚠️
...mentations/AssertWebAuthnLoginCredentialCommand.cs	73.33%	4 Missing ⚠️
...mentations/CreateWebAuthnLoginCredentialCommand.cs	76.47%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3873      +/-   ##
==========================================
- Coverage   38.66%   38.66%   -0.01%     
==========================================
  Files        1209     1209              
  Lines       58561    58591      +30     
  Branches     5594     5594              
==========================================
+ Hits        22643    22654      +11     
- Misses      34863    34883      +20     
+ Partials     1055     1054       -1     

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

[coroiu]

A blast from the past :)

[JaredSnider-Bitwarden]

Really nice error handling refactors! Do you think we should add tests for these commands / user service scenarios while we are in here?