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-713] Add database support for secret access policies #3681

Merged
merged 18 commits into from
Feb 22, 2024
Merged

Conversation

Thomas-Avery
Copy link
Contributor

@Thomas-Avery Thomas-Avery commented Jan 17, 2024

Type of change

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

Objective

The purpose of this PR is to add database support for individual secret permissions for the Secrets Manager project.
This includes changes to EF Core code to clean up secret access policies on deletion events.

Code changes

  • bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ServiceAccountRepository.cs:
    Move to using a transaction, ExecuteDeleteAsync and added cleanup for secret access policies.

  • src/Core/SecretsManager/Entities/AccessPolicy.cs:
    Add secret access policy entities.

  • src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs:
    Add secret access policy cleanup.

  • src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs:
    Add secret access policy cleanup for single delete.
    Bulk delete was broken swapped to using a transaction, ExecuteDeleteAsync, and cleanup code.

  • src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs:
    Add new access policy types to database context.

  • src/Infrastructure.EntityFramework/SecretsManager/Configurations/AccessPolicyEntityTypeConfiguration.cs:
    Add new access policy types to database configuration.

  • src/Infrastructure.EntityFramework/SecretsManager/Discriminators/AccessPolicyDiscriminator.cs:
    Add new discriminators for TPH mappings.

  • src/Infrastructure.EntityFramework/SecretsManager/Models/AccessPolicy.cs
    Add mappings for new entities into EF models.

  • src/Infrastructure.EntityFramework/SecretsManager/Models/Secret.cs
    Add access policy EF navigation properties.

  • src/Sql/SecretsManager/dbo/Tables/AccessPolicy.sql:
    Add column, FK, and index.
    Run the mssql formatter on this file to be more inline with our other SQL files.

  • util/Migrator/DbScripts/2024-01-10_00_AddSecretAccessPolicies.sql:
    Add mssql migration script.

  • util/MySqlMigrations/Migrations/*:
    MySql EF migrations

  • util/PostgresMigrations/Migrations/*:
    Postgres EF migrations

  • util/SqliteMigrations/Migrations/*:
    Sqlite EF migrations

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 Jan 17, 2024
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 54 lines in your changes are missing coverage. Please review.

Comparison is base (374b59b) 36.36% compared to head (caff7a1) 36.42%.

Files Patch % Lines
...Console/Repositories/OrganizationUserRepository.cs 7.69% 24 Missing ⚠️
src/Core/SecretsManager/Entities/AccessPolicy.cs 0.00% 13 Missing ⚠️
...ityFramework/SecretsManager/Models/AccessPolicy.cs 52.00% 12 Missing ⚠️
...re.EntityFramework/SecretsManager/Models/Secret.cs 25.00% 3 Missing ⚠️
...dminConsole/Repositories/OrganizationRepository.cs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3681      +/-   ##
==========================================
+ Coverage   36.36%   36.42%   +0.05%     
==========================================
  Files        1158     1158              
  Lines       55885    55998     +113     
  Branches     5376     5376              
==========================================
+ Hits        20325    20395      +70     
- Misses      34614    34657      +43     
  Partials      946      946              

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

@Thomas-Avery Thomas-Avery marked this pull request as ready for review January 19, 2024 21:07
@Thomas-Avery Thomas-Avery requested review from a team as code owners January 19, 2024 21:07
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.

Overall looks good, thank you for all the work on this! Only one comment.

@bitwarden-bot
Copy link

bitwarden-bot commented Jan 26, 2024

Logo
Checkmarx One – Scan Summary & Details02c1a8b4-f23c-4b7a-8e54-9657df595c48

No New Or Fixed Issues Found

coltonhurst
coltonhurst previously approved these changes Jan 29, 2024
addisonbeck
addisonbeck previously approved these changes Feb 1, 2024
rkac-bw
rkac-bw previously approved these changes Feb 1, 2024
Copy link
Contributor

@rkac-bw rkac-bw left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link

sonarcloud bot commented Feb 21, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
21.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Contributor

@rkac-bw rkac-bw left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Thomas-Avery Thomas-Avery merged commit 1499d1e into main Feb 22, 2024
49 of 51 checks passed
@Thomas-Avery Thomas-Avery deleted the sm/sm-713 branch February 22, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants