Pm 2032 troubleshoot actions#65
Open
lizard-boy wants to merge 56 commits into
Open
Conversation
* [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
…methods. (bitwarden#3284) * Added check for PasswordlessLogin feature flag on new controller and methods. * fix: build error from missing constructor argument --------- Co-authored-by: Andreas Coroiu <andreas.coroiu@gmail.com>
* [PM-4171] feat: update database to support PRF * [PM-4171] feat: rename `DescriptorId` to `CredentialId` * [PM-4171] feat: add PRF felds to domain object * [PM-4171] feat: add `SupportsPrf` column * [PM-4171] fix: add missing comma * [PM-4171] fix: add comma
…en#3331) * Added WebAuthnRepo to EF DI * updated config to match current grant types
…equireResidentKey`
* Add `src/Identity/flags.json` to .gitignore * Change to cover all OSS projects * Include `bitwarden_license` projects (cherry picked from commit e9be7f1)
![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, 59 comment(s)
Edit PR Review Bot Settings | Greptile
Comment on lines
+36
to
+43
| [HttpGet("")] | ||
| public async Task<ListResponseModel<WebAuthnCredentialResponseModel>> Get() | ||
| { | ||
| var user = await GetUserAsync(); | ||
| var credentials = await _credentialRepository.GetManyByUserIdAsync(user.Id); | ||
|
|
||
| return new ListResponseModel<WebAuthnCredentialResponseModel>(credentials.Select(c => new WebAuthnCredentialResponseModel(c))); | ||
| } |
There was a problem hiding this comment.
logic: Add authorization check to ensure the user can only access their own credentials
| throw new NotFoundException("Credential not found."); | ||
| } | ||
|
|
||
| await _credentialRepository.DeleteAsync(credential); |
There was a problem hiding this comment.
logic: Add error handling for the DeleteAsync operation
| var user = await GetUserAsync(); | ||
| if (!await _userService.VerifySecretAsync(user, model.Secret)) | ||
| { | ||
| await Task.Delay(Constants.FailedSecretVerificationDelay); |
There was a problem hiding this comment.
logic: Potential timing attack vulnerability. Consider using a constant-time comparison for secret verification
| using Bit.Core.Utilities; | ||
| using Fido2NetLib; | ||
|
|
||
| namespace Bit.Api.Auth.Models.Request.Webauthn; |
There was a problem hiding this comment.
style: Namespace 'Webauthn' is inconsistent with C# naming conventions. Consider changing to 'WebAuthn'.
|
|
||
| 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 of this class and its properties
| using (var scope = ServiceScopeFactory.CreateScope()) | ||
| { | ||
| var dbContext = GetDatabaseContext(scope); | ||
| var query = dbContext.WebAuthnCredentials.Where(d => d.Id == id && d.UserId == userId); |
There was a problem hiding this comment.
style: Consider adding .AsNoTracking() to improve query performance if entity tracking is not needed
| { | ||
| var dbContext = GetDatabaseContext(scope); | ||
| var query = dbContext.WebAuthnCredentials.Where(d => d.Id == id && d.UserId == userId); | ||
| var cred = await query.FirstOrDefaultAsync(); |
There was a problem hiding this comment.
logic: Handle case where cred is null to avoid potential NullReferenceException
| } | ||
| } | ||
|
|
||
| public async Task<ICollection<Core.Auth.Entities.WebAuthnCredential>> GetManyByUserIdAsync(Guid userId) |
There was a problem hiding this comment.
style: Add XML documentation for this method, including parameter description and return value
| using (var scope = ServiceScopeFactory.CreateScope()) | ||
| { | ||
| var dbContext = GetDatabaseContext(scope); | ||
| var query = dbContext.WebAuthnCredentials.Where(d => d.UserId == userId); |
There was a problem hiding this comment.
style: Consider adding .AsNoTracking() to improve query performance if entity tracking is not needed
Comment on lines
+18
to
+25
| 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: Consider extracting this using block into a separate method to reduce code duplication
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of change
Objective
Code changes
Before you submit
dotnet format --verify-no-changes) (required)Greptile Summary
This PR introduces WebAuthn functionality to the server, focusing on credential management and authentication flows.
WebAuthnControllerinsrc/Api/Auth/Controllers/WebAuthnController.csfor managing WebAuthn credentialssrc/Core/Auth/andsrc/Api/Auth/Models/WebAuthnCredentialRepositoryin both Dapper and EntityFramework implementations for data persistenceWebAuthnGrantValidatorinsrc/Identity/IdentityServer/WebAuthnGrantValidator.csfor handling WebAuthn authenticationUserDecryptionOptionsBuilderinsrc/Identity/IdentityServer/UserDecryptionOptionsBuilder.csfor constructing user decryption options