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

[AC-2513] Scaling PM seat count with SM seat count #4040

Merged

Conversation

cturnbull-bitwarden
Copy link
Contributor

@cturnbull-bitwarden cturnbull-bitwarden commented May 1, 2024

Type of change

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

Objective

For organizations who have signed up for the Secrets Manager free trial via https://vault.bitwarden.com/#/register?org=teams&layout=secretsManager, this PR scales the organization's Password Manager seat count in step with adjustments to the organization's Secrets Manager seat count. We know when an organization has signed up to trial secrets manager because we're already applying a discount to the Password Manager subscription item. By checking for that discount, we can decide if we should scale Password Manager seats along with Secrets Manager.

This is due to a couple of factors:

  1. On the frontend, we're now hiding the Password Manager subscription adjustment form from the "subscriptions" tab in the Admin Console for organizations who are trialing Secrets Manager in this way
  2. We have a requirement where Secrets Manager seats cannot exceed Password Manager seats
  3. We don't want to remove this requirement yet

We'll be splitting Secrets Manager to be a standalone product later this year, such that a user won't have to sign up for Password Manager in order to sign up for Secrets Manager. Until we do that, we need to do some goofy things like this to simulate that they're currently different products.

In addition, I've separated out endpoints from the Admin Console OrganizationsController.cs that are billing related into a new Billing owned OrganizationsController (same routes). As a result, the diff will be hard to read. I recommend first looking at the commit history, as I've committed in logical steps to the above description. The first commit shows the business change as requested by product, the subsequent commits are the codeowner changes. Feel free to comment on any of it.

Client-side PR: bitwarden/clients#9011

Code changes

  • src\Api\AdminConsole\Controllers\OrganizationsController.cs: Moved Billing endpoints to the billing-owned controller of the same route
  • src\Api\Billing\Controllers\OrganizationBillingController.cs: Moved GetBilling endpoint to this controller since it matches the base route
  • src\Api\Billing\Controllers\OrganizationsController.cs: New billing-owned controller to house organization-level endpoints that relate to billing, like GET {id}/billing-status. Added new business logic to POST {id}/sm-subscription that scales PM seat counts with SM seat changes for SM standalone organizations
  • test\Api.Test\AdminConsole\Controllers\OrganizationsControllerTests.cs: Moved billing unit tests to new billing-owned test file
  • test\Api.Test\Billing\Controllers\OrganizationsControllerTests.cs: New billing-owned test file that tests organization controller
Recording.2024-05-02.091734.mp4

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

Copy link

codecov bot commented May 1, 2024

Codecov Report

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

❗ No coverage uploaded for pull request base (main@ea49ff7). Click here to learn what that means.

Files Patch % Lines
...Api/Billing/Controllers/OrganizationsController.cs 24.18% 163 Missing ⚠️
...lling/Controllers/OrganizationBillingController.cs 26.66% 11 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4040   +/-   ##
=======================================
  Coverage        ?   38.39%           
=======================================
  Files           ?     1210           
  Lines           ?    58676           
  Branches        ?     5589           
=======================================
  Hits            ?    22531           
  Misses          ?    35102           
  Partials        ?     1043           

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

Copy link
Contributor

github-actions bot commented May 1, 2024

Logo
Checkmarx One – Scan Summary & Details5d63fb8b-0f71-42c2-b958-b3483d53d0ff

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/Public/Controllers/CollectionsController.cs: 87 Attack Vector
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 270 Attack Vector
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 270 Attack Vector
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 212 Attack Vector
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 212 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 518 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 655 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 955 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 937 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 929 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 947 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 510 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 647 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 587
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 587
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 587
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 587
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 132
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProvidersController.cs: 141
MEDIUM CSRF /src/Api/SecretsManager/Controllers/AccessPoliciesController.cs: 229
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 309
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 161
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 161
MEDIUM CSRF /src/Api/Billing/Controllers/ProviderClientsController.cs: 30
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 190
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 331
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 331
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 710
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 686
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: 752
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/Sso/Controllers/AccountController.cs: 163
MEDIUM CSRF /bitwarden_license/src/Sso/Controllers/AccountController.cs: 96
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: 284
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 908
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 244
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 550
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 274
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 586
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 144
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 175
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 222
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 222
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 375
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 222
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 791
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 308
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 232
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 81
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: 86
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 216
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 298
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 316
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 72
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 50
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 942
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 125
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 111
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 66
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 50
MEDIUM CSRF /src/Api/Public/Controllers/CollectionsController.cs: 64
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 118
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 323
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/Auth/Controllers/AccountsController.cs: 288
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 728
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 188
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1046
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1046
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 411
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 313
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 174
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 997
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 997
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1100
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 299
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: 805
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1023
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1023
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/Controllers/GroupsController.cs: 257
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 911
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 408
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 815
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Controllers/SettingsController.cs: 36
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 571
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/Controllers/CollectionsController.cs: 411
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 187
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 284
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 244
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 550
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 447
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 362
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 403
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 702
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 375
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 433
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 878
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 159
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 613
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 766
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF

More results are available on AST platform

r-tome
r-tome previously approved these changes May 2, 2024
Copy link
Contributor

@r-tome r-tome left a comment

Choose a reason for hiding this comment

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

Great work! I left a few suggestions but none are blockers.
Also, thanks for suggesting to look through the commit history, that helped to review this 😀

Copy link
Contributor

@r-tome r-tome 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 making these changes!

@cturnbull-bitwarden cturnbull-bitwarden merged commit a601802 into main May 17, 2024
49 checks passed
@cturnbull-bitwarden cturnbull-bitwarden deleted the billing/AC-2513/scale-pm-with-sm-seat-updates branch May 17, 2024 18:16
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

4 participants