Pm 2023 fido2 authentication #73
Conversation
Missed staging them when commiting
* [PM-2014] chore: rename `IWebAuthnRespository` to `IWebAuthnCredentialRepository` * [PM-2014] fix: add missing service registration * [PM-2014] feat: add user verification when fetching options * [PM-2014] feat: create migration script for mssql * [PM-2014] chore: append to todo comment * [PM-2014] feat: add support for creation token * [PM-2014] feat: implement credential saving * [PM-2014] chore: add resident key TODO comment * [PM-2014] feat: implement passkey listing * [PM-2014] feat: implement deletion without user verification * [PM-2014] feat: add user verification to delete * [PM-2014] feat: implement passkey limit * [PM-2014] chore: clean up todo comments * [PM-2014] fix: add missing sql scripts Missed staging them when commiting * [PM-2014] feat: include options response model in swagger docs * [PM-2014] chore: move properties after ctor * [PM-2014] feat: use `Guid` directly as input paramter * [PM-2014] feat: use nullable guid in token * [PM-2014] chore: add new-line * [PM-2014] feat: add support for feature flag * [PM-2014] feat: start adding controller tests * [PM-2014] feat: add user verification test * [PM-2014] feat: add controller tests for token interaction * [PM-2014] feat: add tokenable tests * [PM-2014] chore: clean up commented premium check * [PM-2014] feat: add user service test for credential limit * [PM-2014] fix: run `dotnet format` * [PM-2014] chore: remove trailing comma * [PM-2014] chore: add `Async` suffix * [PM-2014] chore: move delay to constant * [PM-2014] chore: change `default` to `null` * [PM-2014] chore: remove autogenerated weirdness * [PM-2014] fix: lint
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
30 file(s) reviewed, 53 comment(s)
Edit PR Review Bot Settings | Greptile
| if (!await _userService.VerifySecretAsync(user, model.Secret)) | ||
| { | ||
| await Task.Delay(Constants.FailedSecretVerificationDelay); | ||
| throw new BadRequestException(string.Empty, "User verification failed."); | ||
| } |
There was a problem hiding this comment.
logic: Potential timing attack vulnerability. Consider using a constant-time comparison
| using System.ComponentModel.DataAnnotations; | ||
| using Fido2NetLib; | ||
|
|
||
| namespace Bit.Api.Auth.Models.Request.Webauthn; |
There was a problem hiding this comment.
style: Consider using 'WebAuthn' instead of 'Webauthn' in the namespace for consistency with the class name
| public AuthenticatorAttestationRawResponse DeviceResponse { get; set; } | ||
|
|
||
| [Required] | ||
| public string Name { get; set; } |
There was a problem hiding this comment.
style: Add length constraint to Name property
|
|
||
| namespace Bit.Api.Auth.Models.Response.WebAuthn; | ||
|
|
||
| public class WebAuthnCredentialCreateOptionsResponseModel : ResponseModel |
There was a problem hiding this comment.
style: Consider adding XML documentation comments to describe the purpose and usage of this class
|
|
||
| public WebAuthnCredentialResponseModel(WebAuthnCredential credential) : base(ResponseObj) | ||
| { | ||
| Id = credential.Id.ToString(); |
There was a problem hiding this comment.
style: Consider using Guid.ToString("N") for a more compact string representation without hyphens
| using (var scope = ServiceScopeFactory.CreateScope()) | ||
| { | ||
| var dbContext = GetDatabaseContext(scope); | ||
| var query = dbContext.WebAuthnCredentials.Where(d => d.Id == id && d.UserId == userId); | ||
| var cred = await query.FirstOrDefaultAsync(); | ||
| return Mapper.Map<Core.Auth.Entities.WebAuthnCredential>(cred); | ||
| } |
There was a problem hiding this comment.
style: Use 'await using' instead of 'using' for better asynchronous resource management
| using (var scope = ServiceScopeFactory.CreateScope()) | ||
| { | ||
| var dbContext = GetDatabaseContext(scope); | ||
| var query = dbContext.WebAuthnCredentials.Where(d => d.UserId == userId); | ||
| var creds = await query.ToListAsync(); | ||
| return Mapper.Map<List<Core.Auth.Entities.WebAuthnCredential>>(creds); | ||
| } |
There was a problem hiding this comment.
style: Use 'await using' instead of 'using' for better asynchronous resource management
| { | ||
| var dbContext = GetDatabaseContext(scope); | ||
| var query = dbContext.WebAuthnCredentials.Where(d => d.UserId == userId); | ||
| var creds = await query.ToListAsync(); |
There was a problem hiding this comment.
style: Consider using ToListAsync() for more explicit type conversion
| @@ -98,6 +99,7 @@ protected override void OnModelCreating(ModelBuilder builder) | |||
| var eOrganizationApiKey = builder.Entity<OrganizationApiKey>(); | |||
| var eOrganizationConnection = builder.Entity<OrganizationConnection>(); | |||
| var eOrganizationDomain = builder.Entity<OrganizationDomain>(); | |||
| var aWebAuthnCredential = builder.Entity<WebAuthnCredential>(); | |||
There was a problem hiding this comment.
style: Variable name 'aWebAuthnCredential' inconsistent with naming convention. Consider 'eWebAuthnCredential' for consistency.
| @RevisionDate DATETIME2(7) | ||
| AS | ||
| BEGIN | ||
| SET NOCOUNT ON |
There was a problem hiding this comment.
style: Add error handling using TRY...CATCH blocks
Type of change
Objective
Code changes
Before you submit
dotnet format --verify-no-changes) (required)Greptile Summary
This pull request introduces WebAuthn (FIDO2) authentication functionality to the server, enhancing the security and flexibility of user authentication options.
WebAuthnControllerinsrc/Api/Auth/Controllers/WebAuthnController.csto handle WebAuthn operationssrc/Api/Auth/Models/Request/WebAuthnandsrc/Api/Auth/Models/Response/WebAuthnWebAuthnCredentialentity and repository interfaces insrc/Core/Auth/Entitiesandsrc/Core/Auth/RepositoriesExtensionGrantValidatorinsrc/Identity/IdentityServer/ExtensionGrantValidator.csfor WebAuthn token validationsrc/Core/Auth/Models/Business/Tokenablesfor managing authentication tokens