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

[PM-7029] Remove conditional logic for KeyRotationImprovements feature flag #4002

Merged
merged 7 commits into from May 9, 2024

Conversation

trmartin4
Copy link
Member

@trmartin4 trmartin4 commented Apr 18, 2024

Type of change

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

Objective

Removed the conditional logic for the KeyRotationImprovements feature flag now that it has been 3 releases.

Note that the flag itself is kept as defined so that old clients will still be able to act on it.

This was causing bitwarden/clients#8453 for self-hosted customers with the flag off. Rather than try to fix the issue, I elected to remove the flag as the feature has already been enabled.

See corresponding clients PR: bitwarden/clients#8816.

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 trmartin4 requested a review from a team as a code owner April 18, 2024 18:58
@trmartin4 trmartin4 requested a review from jlf0dev April 18, 2024 18:58
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

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

Project coverage is 38.00%. Comparing base (f7aa56b) to head (af63d1e).
Report is 1 commits behind head on main.

Files Patch % Lines
src/Api/Auth/Controllers/AccountsController.cs 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4002      +/-   ##
==========================================
+ Coverage   37.86%   38.00%   +0.13%     
==========================================
  Files        1192     1192              
  Lines       57988    57781     -207     
  Branches     5549     5535      -14     
==========================================
  Hits        21960    21960              
+ Misses      34995    34788     -207     
  Partials     1033     1033              

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

Copy link
Contributor

github-actions bot commented Apr 18, 2024

Logo
Checkmarx One – Scan Summary & Details6a936494-31f2-4e1c-87e4-62231c25778e

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 231 Attack Vector
MEDIUM CSRF /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 71 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 443 Attack Vector
MEDIUM CSRF /src/Api/Billing/Public/Controllers/OrganizationController.cs: 44 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 457 Attack Vector
MEDIUM CSRF /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 97 Attack Vector
MEDIUM Privacy_Violation /src/Api/Controllers/DevicesController.cs: 155 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 703 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 650 Attack Vector
LOW Log_Forging /src/Api/Controllers/DevicesController.cs: 146 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 678 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 615 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProvidersController.cs: 141
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 284
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 145
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 145
MEDIUM CSRF /src/Api/Billing/Controllers/ProviderClientsController.cs: 30
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 563
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 563
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 563
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 563
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 318
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 318
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 702
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 678
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 891
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 173
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 744
MEDIUM CSRF /src/Api/Vault/Controllers/FoldersController.cs: 45
MEDIUM CSRF /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 51
MEDIUM CSRF /src/Api/Controllers/UsersController.cs: 22
MEDIUM CSRF /src/Api/Controllers/DevicesController.cs: 70
MEDIUM CSRF /src/Api/Controllers/DevicesController.cs: 57
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 69
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 49
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 92
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 49
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 142
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderOrganizationsController.cs: 52
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 148
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 78
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 61
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/UsersController.cs: 50
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 161
MEDIUM CSRF /src/Api/Auth/Controllers/EmergencyAccessController.cs: 159
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 98
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 88
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 231
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 271
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 144
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 188
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 744
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 175
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 613
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 669
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 222
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 858
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 807
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 288
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 410
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 191
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 187
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 526
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 362
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 174
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 128
MEDIUM CSRF /bitwarden_license/src/Sso/Controllers/AccountController.cs: 97
MEDIUM CSRF /bitwarden_license/src/Sso/Controllers/AccountController.cs: 164
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 308
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 207
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 77
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 114
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 230
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 331
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 85
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 203
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 285
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 303
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 286
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 374
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 403
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 231
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 220
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 408
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 900
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 571
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 271
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 970
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 970
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 726
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 825
MEDIUM CSRF /src/Api/Controllers/SettingsController.cs: 36
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 188
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 993
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 993
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 586
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 433
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 159
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1027
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 369
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 688
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 313
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 758
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 205
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 410
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 191
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 323
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 526
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 220
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1047
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 959
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 374
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 944
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 944
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 539
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 539
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 59
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 127
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 560
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 156
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 187
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 196
MEDIUM CSRF /src/Api/Public/Controllers/CollectionsController.cs: 64
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 50
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 66
MEDIUM

More results are available on AST platform

ike-kottlowski
ike-kottlowski previously approved these changes Apr 18, 2024
Comment on lines -484 to -491
result = await _userService.UpdateKeyAsync(
user,
model.MasterPasswordHash,
model.Key,
model.PrivateKey,
ciphers,
folders,
sends);
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this method too? I think this is only called here

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it, we also have code in the repository that needs to be removed as well. The good news is there's no actual sql, it should just be _cipherRepository.UpdateUserKeysAndCiphersAsync in both repos (assuming nothing else calls it of course)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Thank you. I've removed the methods.

@trmartin4 trmartin4 requested a review from jlf0dev April 25, 2024 01:43
Copy link
Member

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

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

vault changes look good

Copy link
Member

@jlf0dev jlf0dev left a comment

Choose a reason for hiding this comment

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

Thanks for this! May want to wait until rc is cut before merging

@trmartin4 trmartin4 merged commit 7f9d7c0 into main May 9, 2024
51 checks passed
@trmartin4 trmartin4 deleted the auth/pm-7029/remove-key-rotation-feature-flag branch May 9, 2024 17:24
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.

Encryption key rotation fails with 500 error
4 participants