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-7004] Org Admin Initiate Delete #3905

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

[PM-7004] Org Admin Initiate Delete #3905

wants to merge 26 commits into from

Conversation

kspearrin
Copy link
Member

@kspearrin kspearrin commented Mar 15, 2024

Type of change

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

Objective

This PR adds support for support to initiate a delete of an organization that is then confirmed by the org admin via email.

image

image

Code changes

TODO

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 Mar 15, 2024

Codecov Report

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

Project coverage is 39.24%. Comparing base (0b5c21a) to head (74ac919).

Files Patch % Lines
...dminConsole/Controllers/OrganizationsController.cs 12.00% 22 Missing ⚠️
.../Services/Implementations/HandlebarsMailService.cs 0.00% 22 Missing ⚠️
...dminConsole/Controllers/OrganizationsController.cs 0.00% 21 Missing ⚠️
...e/Models/Business/Tokenables/OrgDeleteTokenable.cs 0.00% 15 Missing ⚠️
...ore/Models/Mail/OrganizationInitiateDeleteModel.cs 0.00% 15 Missing ⚠️
...le/Services/Implementations/OrganizationService.cs 13.33% 13 Missing ⚠️
src/Core/Utilities/ModelStateExtensions.cs 0.00% 7 Missing ⚠️
src/Admin/Views/Shared/_Layout.cshtml 0.00% 3 Missing ⚠️
...re/Services/NoopImplementations/NoopMailService.cs 0.00% 3 Missing ⚠️
...nConsole/Models/OrganizationInitiateDeleteModel.cs 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3905      +/-   ##
==========================================
- Coverage   39.31%   39.24%   -0.08%     
==========================================
  Files        1200     1205       +5     
  Lines       57988    58122     +134     
  Branches     5337     5348      +11     
==========================================
+ Hits        22797    22808      +11     
- Misses      34135    34258     +123     
  Partials     1056     1056              

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

@@ -579,6 +579,18 @@
}
}

[HttpPost("delete-recover-token")]
[AllowAnonymous]
public async Task PostDeleteRecoverToken([FromBody] OrganizationVerifyDeleteRecoverRequestModel model)

Check warning

Code scanning / Checkmarx One

Log Forging

Log Forging
@@ -579,6 +579,18 @@
}
}

[HttpPost("delete-recover-token")]
[AllowAnonymous]
public async Task PostDeleteRecoverToken([FromBody] OrganizationVerifyDeleteRecoverRequestModel model)

Check warning

Code scanning / Checkmarx One

Log Forging

Log Forging
</tr>
<tr style="margin: 0; box-sizing: border-box; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
<td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none; text-align: center;" valign="top" align="center">
<a href="{{{Url}}}" clicktracking=off target="_blank" style="color: #ffffff; text-decoration: none; text-align: center; cursor: pointer; display: inline-block; border-radius: 5px; background-color: #175DDC; border-color: #175DDC; border-style: solid; border-width: 10px 20px; margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">

Check warning

Code scanning / Checkmarx One

Unsafe Use Of Target blank Medium

Unsafe Use Of Target blank
@@ -579,6 +579,18 @@
}
}

[HttpPost("delete-recover-token")]
[AllowAnonymous]
public async Task PostDeleteRecoverToken([FromBody] OrganizationVerifyDeleteRecoverRequestModel model)

Check warning

Code scanning / Checkmarx One

Log Forging

Log Forging
@kspearrin kspearrin changed the title Org Admin Initiate Delete [PM-7004] Org Admin Initiate Delete Mar 22, 2024
@kspearrin kspearrin marked this pull request as ready for review March 22, 2024 13:44
@kspearrin kspearrin requested review from a team as code owners March 22, 2024 13:44
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

I'll leave the general review to @vincentsalucci , just wanted to throw in my 2 cents on OrganizationService.

@vvolkgang vvolkgang requested review from r-tome and removed request for vincentsalucci March 26, 2024 16:25
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.

The implementation looks good to me but there are some changes needed including feedback from the Product team.
Ideally the OrganizationService methods should be on a new command, along with unit tests. However, if time constraints exist, this can be addressed at a later stage.

kspearrin and others added 2 commits March 29, 2024 12:20
Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com>
Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com>
Copy link
Contributor

github-actions bot commented Mar 29, 2024

Logo
Checkmarx One – Scan Summary & Details05b276ec-78f7-468f-b9da-73347437dff1

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 Missing_HSTS_Header /bitwarden_license/src/Sso/Startup.cs: 57 Attack Vector
MEDIUM Privacy_Violation /src/Api/Controllers/DevicesController.cs: 155 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 739 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 686 Attack Vector
LOW Log_Forging /src/Api/Vault/Controllers/CiphersController.cs: 222 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 596 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 596 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 596 Attack Vector
LOW Log_Forging /src/Api/Controllers/DevicesController.cs: 146 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 651 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 714 Attack Vector
LOW Missing_CSP_Header /src/Core/MailTemplates/Handlebars/InitiateDeleteOrganzation.html.hbs: 10 Attack Vector
LOW Unsafe_Use_Of_Target_blank /src/Core/MailTemplates/Handlebars/InitiateDeleteOrganzation.html.hbs: 32 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: 891
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 711
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 173
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/AdminConsole/Controllers/OrganizationUsersController.cs: 246
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 375
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
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: 277
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1120
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 774
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 159
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 931
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/Controllers/SettingsController.cs: 36
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/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: 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: 218
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 300
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 318
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 411
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 722
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 530
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 627
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 627
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1066
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1066
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 260
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 174
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 403
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 926
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 287
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 786
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1043
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1043
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
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/Vault/Controllers/CiphersController.cs: 811
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/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/Controllers/CollectionsController.cs: 323
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/AdminConsole/Controllers/OrganizationUsersController.cs: 246
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 144
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/Vault/Controllers/CiphersController.cs: 222
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 572
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 361
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 570
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 748
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderOrganizationsController.cs: 53
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: 519
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 CSRF /src/Api/Vault/Controllers/CiphersController.cs: 111
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 125
MEDIUM CSRF

More results are available on AST platform

kspearrin

This comment was marked as resolved.

kspearrin

This comment was marked as resolved.

Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com>
@kspearrin
Copy link
Member Author

The implementation looks good to me but there are some changes needed including feedback from the Product team. Ideally the OrganizationService methods should be on a new command, along with unit tests. However, if time constraints exist, this can be addressed at a later stage.

Where can I find examples of using commands?

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.

While testing this I noticed an issue, more info on the comments below

kspearrin and others added 2 commits April 1, 2024 10:39
Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com>
@andrebispo5 andrebispo5 requested a review from jlf0dev April 8, 2024 08:45
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.

The build is broken, we just need to fix the orgId variable

r-tome
r-tome previously approved these changes Apr 9, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Does this belong in Auth? Feels like it might be more Admin Console

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, the clients code was already moved to AC ownership so I also moved this.

# Conflicts:
#	src/Core/Services/IMailService.cs
#	src/Core/Services/NoopImplementations/NoopMailService.cs
Copy link
Member

@eliykat eliykat 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! Just a couple of questions about our security practices, otherwise good to go.


Click the link below to complete the deletion of your organization.

If you did not request this email to delete your Bitwarden organization, you can safely ignore it.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a nit, but if you didn't request this email you should probably be worried! Someone is trying to delete your organization. Maybe "If you did not request this email to delete your Bitwarden organization, please contact us." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I am not opposed to changing that.

@r-tome r-tome requested a review from eliykat April 25, 2024 13:54
eliykat
eliykat previously approved these changes Apr 25, 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

6 participants