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-2576] Replace Billing commands and queries with services #4070

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

Conversation

amorask-bitwarden
Copy link
Contributor

@amorask-bitwarden amorask-bitwarden commented May 9, 2024

Type of change

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

Objective

I wrote this PR due to ongoing conversations in the #code channel regarding how Bitwarden handles "CQRS" and the issue of having commands invoke other commands. Since Billing as a business function is more of an intermediary layer that processes side effects of other user actions, we've decided to refactor the business logic we've been writing to be service-oriented rather than formatted as commands or queries.

(Sorry for the insane commit chain. I've outlined the changes by team below)

This resulted in the movement of the following logic:

  • AssignSeatsToClientOrganizationCommand -> Commercial.ProviderBillingService
  • CancelSubscriptionCommand -> SubscriberService
  • CreateCustomerCommand -> Commercial.ProviderBillingService
  • RemovePaymentMethodCommand -> SubscriberService
  • ScaleSeatsCommand -> Commercial.ProviderBillingService
  • StartSubscriptionCommand -> Commercial.ProviderBillingService
  • ProviderBillingQueries -> Commercial.ProviderBillingService
  • OrganizationBillingQueries -> OrganizationBillingService
  • SubscriberQueries -> SubscriberService

@bitwarden/team-admin-console-dev:
As far as changes to your domain go, the big one is in RemoveOrganizationFromProviderCommand. Since RemovePaymentMethod is no longer a command, we can now invoke it from its service within this command. That update is in this PR and I've added tests for it.

The updates to your API Controllers are just wiring up the new services with the same logic instead of the commands and queries. The only actual change to billing business logic represented in AC code is in the API ProvidersController. You'll see StartSubscriptionCommand has been split into 2 methods: CreateCustomer and StartSubscription. They do the same thing as before, but I wanted method control to be more granular.

@bitwarden/team-auth-dev:
I think you only have one file changed in here and it's just replacing the previous command invocation for cancelling a subscription with the new service method; should be pretty straight forward.

@bitwarden/team-billing-dev:
If needed, we can sync as a team on the changes. Jet let me know.

Copy link
Contributor

github-actions bot commented May 9, 2024

Logo
Checkmarx One – Scan Summary & Details9f70af6b-3896-4cbe-a968-2c24eac488b2

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProvidersController.cs: 82 Attack Vector
MEDIUM Privacy_Violation /src/Api/Controllers/DevicesController.cs: 155 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 343 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 396 Attack Vector
LOW Log_Forging /src/Api/Controllers/DevicesController.cs: 146 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 371 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 308 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
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: 320
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 163
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 163
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: 333
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 333
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 669
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 645
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 535
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 711
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/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/Controllers/CollectionsController.cs: 411
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 375
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 286
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 572
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 570
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1120
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 748
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 246
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 411
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 375
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 931
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 286
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 188
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 260
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 774
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 308
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 243
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 81
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 118
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 117
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 72
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 50
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 283
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 962
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 86
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 218
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 300
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 318
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/AdminConsole/Controllers/ProviderOrganizationsController.cs: 53
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 50
MEDIUM CSRF /src/Api/Public/Controllers/CollectionsController.cs: 64
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 519
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 159
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 283
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 403
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 926
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/Controllers/SettingsController.cs: 36
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 786
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 407
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1100
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 432
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1066
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1066
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 811
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 722
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 627
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 627
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 530
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 174
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 287
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 283
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 283
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 283
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1043
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1043
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1017
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1017
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 222
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 312
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 301
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 449
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 545
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 825
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 221
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: 323
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 222
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 187
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 898
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 867
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 570
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 175
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 361
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 277
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 246
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 144
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 196
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 187
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 156
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 59
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 127
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProviderOrganizationsController.cs: 40
MEDIUM CSRF /src/Api/Auth/Controllers/WebAuthnController.cs: 130
MEDIUM CSRF /src/Api/Auth/Controllers/WebAuthnController.cs: 101
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 679
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1120
MEDIUM CSRF

More results are available on AST platform

r-tome
r-tome previously approved these changes May 13, 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.

AC changes look good!


organizationRepository.GetOwnerEmailAddressesById(organization.Id).Returns(organizationOwnerEmails);
organizationRepository.GetOwnerEmailAddressesById(organization.Id).Returns([
"a@b.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I prefer to use the domain example.com that is reserved for testing purposes

jlf0dev
jlf0dev previously approved these changes May 13, 2024
Copy link

codecov bot commented May 13, 2024

Codecov Report

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

Project coverage is 39.07%. Comparing base (febc696) to head (5d77b73).

Files Patch % Lines
.../Commercial.Core/Billing/ProviderBillingService.cs 95.20% 6 Missing and 9 partials ⚠️
...ling/Services/Implementations/SubscriberService.cs 96.66% 3 Missing and 4 partials ⚠️
...pi/AdminConsole/Controllers/ProvidersController.cs 0.00% 5 Missing ⚠️
...lling/Controllers/OrganizationBillingController.cs 33.33% 4 Missing ⚠️
...dminConsole/Controllers/OrganizationsController.cs 0.00% 3 Missing ⚠️
...Providers/RemoveOrganizationFromProviderCommand.cs 96.15% 0 Missing and 1 partial ⚠️
...ole/Controllers/ProviderOrganizationsController.cs 0.00% 1 Missing ⚠️
...dminConsole/Controllers/OrganizationsController.cs 66.66% 0 Missing and 1 partial ⚠️
src/Api/Auth/Controllers/AccountsController.cs 66.66% 1 Missing ⚠️
...Api/Billing/Controllers/OrganizationsController.cs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4070      +/-   ##
==========================================
- Coverage   39.31%   39.07%   -0.25%     
==========================================
  Files        1201     1195       -6     
  Lines       57994    57831     -163     
  Branches     5336     5321      -15     
==========================================
- Hits        22799    22595     -204     
- Misses      34139    34180      +41     
  Partials     1056     1056              

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

@amorask-bitwarden
Copy link
Contributor Author

@jlf0dev @r-tome @cturnbull-bitwarden Hey everyone, sorry had to fix something up in the RemoveOrganizationFromProviderCommand. That change is represented here: 734e77c

If I could get another look, would greatly appreciate it.

jlf0dev
jlf0dev previously approved these changes May 13, 2024
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.

Auth looks good 👍

r-tome
r-tome previously approved these changes May 14, 2024
r-tome
r-tome previously approved these changes May 15, 2024
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