[SM-999] Add Bulk Move to Project Endpoint#66
Conversation
- Use EntityType for Join Table
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
26 file(s) reviewed, 24 comment(s)
Edit PR Review Bot Settings | Greptile
| BulkSecretOperationRequirement requirement, | ||
| IReadOnlyList<Secret> resource) | ||
| { | ||
| var secretsByOrganizationId = resource.GroupBy(s => s.OrganizationId).ToArray(); |
There was a problem hiding this comment.
style: Consider using FirstOrDefault() instead of ToArray() for better performance when only checking for a single group.
| var secretAccesses = await _secretRepository.AccessToSecretsAsync( | ||
| secrets.Select(s => s.Id).ToArray(), userId, accessClientType); |
There was a problem hiding this comment.
style: This could potentially be a performance bottleneck for large numbers of secrets. Consider implementing a batch operation in the repository.
| secrets.Select(s => s.Id).ToArray(), userId, accessClientType); | ||
|
|
||
| // If we don't have the write permission | ||
| return secretAccesses.All(a => a.Value.Write); |
There was a problem hiding this comment.
logic: Ensure that secretAccesses contains an entry for every secret, otherwise this check might pass incorrectly.
| await dbContext.ProjectSecrets | ||
| .Where(ps => secretIds.Contains(ps.SecretsId)) | ||
| .ExecuteDeleteAsync(); |
There was a problem hiding this comment.
logic: This operation deletes all existing project-secret relationships. Ensure this is the intended behavior, as it may have unintended consequences
| Guid userId, | ||
| AccessClientType accessType) | ||
| { | ||
| await using var scope = ServiceScopeFactory.CreateAsyncScope(); |
There was a problem hiding this comment.
style: Use 'using' instead of 'await using' for consistency with other methods
| modelBuilder.Entity("Bit.Infrastructure.EntityFramework.SecretsManager.Models.ProjectSecret", b => | ||
| { | ||
| b.HasOne("Bit.Infrastructure.EntityFramework.SecretsManager.Models.Project", null) | ||
| .WithMany() | ||
| .HasForeignKey("ProjectsId") | ||
| .OnDelete(DeleteBehavior.Cascade) | ||
| .IsRequired(); | ||
|
|
||
| b.HasOne("Bit.Infrastructure.EntityFramework.SecretsManager.Models.Secret", null) | ||
| .WithMany() | ||
| .HasForeignKey("SecretsId") | ||
| .OnDelete(DeleteBehavior.Cascade) | ||
| .IsRequired(); | ||
| }); |
There was a problem hiding this comment.
logic: Cascade delete behavior for ProjectSecret relationships may cause unintended data loss if not carefully managed
| protected override void Up(MigrationBuilder migrationBuilder) | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
logic: Up method is empty. Implement table creation, indexes, and foreign keys for ProjectSecret entity.
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
logic: Down method is empty. Implement logic to revert changes made in Up method.
| protected override void Up(MigrationBuilder migrationBuilder) | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
logic: The Up method is empty. It should create the ProjectSecret table with appropriate columns.
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
logic: The Down method is empty. It should drop the ProjectSecret table to revert the migration.
2 participants
Type of change
Objective
Add new endpoint for updating the projects of many secrets to the same project.
Clients PR: bitwarden/clients#6665
Code changes
ProjectSecretto avoid making large changes in the code base or to make this update one row at a time. It deletes all current relationships for the given secrets and then creates new relationships for them based on the supplied project ids.Before you submit
dotnet format --verify-no-changes) (required)Greptile Summary
This pull request adds a new endpoint for bulk moving secrets to a project in the Secrets Manager, including necessary authorization, command implementation, and unit tests.
BulkMoveToProjectAsyncmethod inSecretsController.csto handle the new bulk move endpointMoveSecretsCommandandBulkSecretAuthorizationHandlerfor executing and authorizing bulk secret operationsProjectSecretentity to represent many-to-many relationships between projects and secretsAccessToSecretsAsyncandMoveSecretsAsynctoISecretRepositoryinterface and implementations