-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
There was a problem hiding this 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
New Issues
Fixed Issues
|
There was a problem hiding this 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
There was a problem hiding this 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); |
There was a problem hiding this comment.
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, "")); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Errant comma.
Type of change
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
dotnet format --verify-no-changes
) (required)