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-787] Extract authorization from project delete command #2987

Merged
merged 13 commits into from
Jul 11, 2023

Conversation

Thomas-Avery
Copy link
Contributor

@Thomas-Avery Thomas-Avery commented May 31, 2023

Type of change

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

Objective

The purpose of this PR is to refactor authorization out of the DeleteProjectCommand.

Code changes

  • bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandler.cs:
    Adding delete to project authorization handler.

  • bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Projects/DeleteProjectCommand.cs:

  • src/Core/SecretsManager/Commands/Projects/Interfaces/IDeleteProjectCommand.cs:
    Extracting authorization from the delete project command.
    Removing the extra call to update revision dates in favor of encapsulating the update all in one transaction via DeleteManyByIdAsync.

  • bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ProjectRepository.cs:
    Adding in updating the revision date of secrets associated with the projects being deleted, so this can all be in one database transaction.

  • bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandlerTests.cs:
    Adding in unit test coverage for the delete operation and consolidating update boolean mapping tests.

  • bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/Projects/DeleteProjectCommandTests.cs:
    Updating unit test coverage for the DeleteProjectCommand since authorization is now extracted out.

  • src/Api/SecretsManager/Controllers/ProjectsController.cs:
    Updating the bulk delete controller to utilize the project authorization handler.

  • src/Core/SecretsManager/AuthorizationRequirements/ProjectOperationRequirement.cs:
    Adding delete as an option for ProjectOperationRequirement

  • test/Api.Test/SecretsManager/Controllers/ProjectsControllerTests.cs:
    Adding unit test coverage for authorization handler and controller code updates.

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 May 31, 2023
@Thomas-Avery Thomas-Avery marked this pull request as ready for review June 1, 2023 18:41
@Thomas-Avery Thomas-Avery requested a review from a team as a code owner June 1, 2023 18:41
@Thomas-Avery Thomas-Avery requested a review from Hinton June 8, 2023 15:16
coltonhurst
coltonhurst previously approved these changes Jun 13, 2023
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.

This looks great, thanks @Thomas-Avery

@Thomas-Avery
Copy link
Contributor Author

Thomas-Avery commented Jun 28, 2023

This will get merged into master after #3021.

So I've updated this PR to support write access for service accounts on project delete. 67e9f2d

@bitwarden-bot
Copy link

bitwarden-bot commented Jul 7, 2023

Logo
Checkmarx One – Scan Summary & Details5aee9b75-d942-4702-8f19-1efcf23b9e81

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/SecretsManager/Controllers/ProjectsController.cs: 131 Attack Vector

Fixed Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/SecretsManager/Controllers/ProjectsController.cs: 130 Attack Vector

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.

This looks good, thanks @Thomas-Avery

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

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

Minor things not needing change right now. Looks good!

projectSecret.RevisionDate = utcNow;
}

dbContext.Remove(project);
Copy link
Contributor

Choose a reason for hiding this comment

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

if (authorizationResult.Succeeded)
{
projectsToDelete.Add(project);
results.Add((project, ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ I prefer the cleanliness of string.Empty vs. the literal here.

@@ -4,5 +4,5 @@ public enum PermissionType
{
RunAsAdmin,
RunAsUserWithPermission,
RunAsServiceAccountWithPermission
RunAsServiceAccountWithPermission,
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Errant comma.

@Thomas-Avery Thomas-Avery merged commit b629c31 into master Jul 11, 2023
41 checks passed
@Thomas-Avery Thomas-Avery deleted the sm/SM-787 branch July 11, 2023 20:15
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.

4 participants