Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SM-1256] Add BulkSecretAuthorizationHandler #4099

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

Thomas-Avery
Copy link
Contributor

Type of change

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

Objective

The purpose of this PR is to add a BulkSecretAuthorizationHandler and use it for the GetSecretsByIdsAsync endpoint.

Code changes

  • file.ext: Description of what was changed and why

  • bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs:
    src/Core/SecretsManager/AuthorizationRequirements/BulkSecretOperationRequirement.cs:
    Add new bulk authz handler.

  • bitwarden_license/src/Commercial.Core/SecretsManager/SecretsManagerCollectionExtensions.cs:
    Add handler to DI.

  • bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/SecretRepository.cs:
    src/Core/SecretsManager/Repositories/ISecretRepository.cs:
    src/Core/SecretsManager/Repositories/Noop/NoopSecretRepository.cs:
    Add AccessToSecretsAsync to the repository.

  • bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandlerTests.cs:
    Add authz handler unit tests.

  • src/Api/SecretsManager/Controllers/SecretsController.cs:
    Use the new bulk authz handler for the GetSecretsByIdsAsync endpoint.

  • test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs:
    Update unit tests

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

@Thomas-Avery Thomas-Avery self-assigned this May 17, 2024
Copy link

codecov bot commented May 17, 2024

Codecov Report

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

Project coverage is 39.34%. Comparing base (febc696) to head (d6f0992).

Files Patch % Lines
...sManager/Repositories/Noop/NoopSecretRepository.cs 0.00% 3 Missing ⚠️
...rk/SecretsManager/Repositories/SecretRepository.cs 94.11% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4099      +/-   ##
==========================================
+ Coverage   39.31%   39.34%   +0.03%     
==========================================
  Files        1201     1203       +2     
  Lines       57994    58030      +36     
  Branches     5336     5338       +2     
==========================================
+ Hits        22799    22832      +33     
- Misses      34139    34142       +3     
  Partials     1056     1056              

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

Copy link
Contributor

github-actions bot commented May 17, 2024

Logo
Checkmarx One – Scan Summary & Detailsa0650c68-80b2-478c-af82-87efdeff8952

No New Or Fixed Issues Found

@Thomas-Avery Thomas-Avery marked this pull request as ready for review May 17, 2024 19:47
@Thomas-Avery Thomas-Avery requested a review from a team as a code owner May 17, 2024 19:47
Copy link
Member

@coltonhurst coltonhurst left a comment

Choose a reason for hiding this comment

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

One small comment

_ => secrets.Select(s => new SecretAccess(s.Id, false, false))
};

private record SecretAccess(Guid Id, bool Read, bool Write);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Any reason this is at the bottom of the file instead of the top? There are a few other records we have in the middle of files, I'm used to type defs and fields at the top of a file or class. No big deal though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants