-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[PM-28555] Add DefaultCollectionSemaphore table #6791
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
Changes from all commits
f069faf
a91f096
31eebe2
e67727b
ae9d18a
a53bb23
9cc89eb
843fd7a
90f2e2b
7aef3da
53bc100
36aad90
f668a0c
7ea237f
a6fcee7
21d7276
6bcc17f
8212af5
a9e8409
ebbdc94
897719e
38bab44
2bf8487
8ab5ad1
415182a
8427367
5180092
0face3a
1a42f6f
c84dd78
aa1c0a4
2b7caf1
2fc0257
fc351ce
8f345e0
91c51fd
dc5c06f
1f4fc9b
4c32e20
32dd846
3d801be
9d86aff
88036ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Utilities; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.Collections; | ||
|
|
||
| public static class CollectionUtils | ||
| { | ||
| /// <summary> | ||
| /// Arranges Collection and CollectionUser objects to create default user collections. | ||
| /// </summary> | ||
| /// <param name="organizationId">The organization ID.</param> | ||
| /// <param name="organizationUserIds">The IDs for organization users who need default collections.</param> | ||
| /// <param name="defaultCollectionName">The encrypted string to use as the default collection name.</param> | ||
| /// <returns>A tuple containing the collections and collection users.</returns> | ||
| public static (ICollection<Collection> collections, ICollection<CollectionUser> collectionUsers) | ||
| BuildDefaultUserCollections(Guid organizationId, IEnumerable<Guid> organizationUserIds, | ||
| string defaultCollectionName) | ||
| { | ||
| var now = DateTime.UtcNow; | ||
|
|
||
| var collectionUsers = new List<CollectionUser>(); | ||
| var collections = new List<Collection>(); | ||
|
|
||
| foreach (var orgUserId in organizationUserIds) | ||
| { | ||
| var collectionId = CoreHelpers.GenerateComb(); | ||
|
|
||
| collections.Add(new Collection | ||
| { | ||
| Id = collectionId, | ||
| OrganizationId = organizationId, | ||
| Name = defaultCollectionName, | ||
| CreationDate = now, | ||
| RevisionDate = now, | ||
| Type = CollectionType.DefaultUserCollection, | ||
| DefaultUserCollectionEmail = null | ||
|
|
||
| }); | ||
|
|
||
| collectionUsers.Add(new CollectionUser | ||
| { | ||
| CollectionId = collectionId, | ||
| OrganizationUserId = orgUserId, | ||
| ReadOnly = false, | ||
| HidePasswords = false, | ||
| Manage = true, | ||
| }); | ||
| } | ||
|
|
||
| return (collections, collectionUsers); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| ๏ปฟusing System.Data; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Text.Json; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.Collections; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
|
|
@@ -360,7 +361,45 @@ public async Task<ICollection<CollectionAccessSelection>> GetManyUsersByIdAsync( | |
| } | ||
| } | ||
|
|
||
| public async Task UpsertDefaultCollectionsAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, string defaultCollectionName) | ||
| public async Task CreateDefaultCollectionsAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, string defaultCollectionName) | ||
| { | ||
| organizationUserIds = organizationUserIds.ToList(); | ||
| if (!organizationUserIds.Any()) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var organizationUserCollectionIds = organizationUserIds | ||
| .Select(ou => (ou, CoreHelpers.GenerateComb())) | ||
| .ToTwoGuidIdArrayTVP(); | ||
|
|
||
| await using var connection = new SqlConnection(ConnectionString); | ||
| await connection.OpenAsync(); | ||
| await using var transaction = connection.BeginTransaction(); | ||
|
|
||
| try | ||
| { | ||
| await connection.ExecuteAsync( | ||
| "[dbo].[Collection_CreateDefaultCollections]", | ||
| new | ||
| { | ||
| OrganizationId = organizationId, | ||
| DefaultCollectionName = defaultCollectionName, | ||
| OrganizationUserCollectionIds = organizationUserCollectionIds | ||
| }, | ||
| commandType: CommandType.StoredProcedure, | ||
| transaction: transaction); | ||
|
|
||
| await transaction.CommitAsync(); | ||
| } | ||
| catch | ||
| { | ||
| await transaction.RollbackAsync(); | ||
| throw; | ||
| } | ||
| } | ||
|
|
||
| public async Task CreateDefaultCollectionsBulkAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, string defaultCollectionName) | ||
| { | ||
| organizationUserIds = organizationUserIds.ToList(); | ||
| if (!organizationUserIds.Any()) | ||
|
|
@@ -377,7 +416,8 @@ public async Task UpsertDefaultCollectionsAsync(Guid organizationId, IEnumerable | |
|
|
||
| var missingDefaultCollectionUserIds = organizationUserIds.Except(orgUserIdWithDefaultCollection); | ||
|
|
||
| var (collectionUsers, collections) = BuildDefaultCollectionForUsers(organizationId, missingDefaultCollectionUserIds, defaultCollectionName); | ||
| var (collections, collectionUsers) = | ||
| CollectionUtils.BuildDefaultUserCollections(organizationId, missingDefaultCollectionUserIds, defaultCollectionName); | ||
|
|
||
| if (!collectionUsers.Any() || !collections.Any()) | ||
| { | ||
|
|
@@ -387,11 +427,11 @@ public async Task UpsertDefaultCollectionsAsync(Guid organizationId, IEnumerable | |
| await BulkResourceCreationService.CreateCollectionsAsync(connection, transaction, collections); | ||
| await BulkResourceCreationService.CreateCollectionsUsersAsync(connection, transaction, collectionUsers); | ||
|
|
||
| transaction.Commit(); | ||
| await transaction.CommitAsync(); | ||
| } | ||
| catch | ||
| { | ||
| transaction.Rollback(); | ||
| await transaction.RollbackAsync(); | ||
| throw; | ||
| } | ||
| } | ||
|
Comment on lines
402
to
437
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method still has a TOCTOU race condition:
Race condition scenario: Why the transaction doesn't prevent this: While the transaction ensures atomicity of the inserts within a single call, it doesn't provide isolation between concurrent calls at the SERIALIZABLE level needed to prevent this race. Comparison with non-bulk method: The Recommendation:
|
||
|
|
@@ -421,40 +461,6 @@ INNER JOIN | |
| return organizationUserIds.ToHashSet(); | ||
| } | ||
|
|
||
| private (List<CollectionUser> collectionUser, List<Collection> collection) BuildDefaultCollectionForUsers(Guid organizationId, IEnumerable<Guid> missingDefaultCollectionUserIds, string defaultCollectionName) | ||
| { | ||
| var collectionUsers = new List<CollectionUser>(); | ||
| var collections = new List<Collection>(); | ||
|
|
||
| foreach (var orgUserId in missingDefaultCollectionUserIds) | ||
| { | ||
| var collectionId = CoreHelpers.GenerateComb(); | ||
|
|
||
| collections.Add(new Collection | ||
| { | ||
| Id = collectionId, | ||
| OrganizationId = organizationId, | ||
| Name = defaultCollectionName, | ||
| CreationDate = DateTime.UtcNow, | ||
| RevisionDate = DateTime.UtcNow, | ||
| Type = CollectionType.DefaultUserCollection, | ||
| DefaultUserCollectionEmail = null | ||
|
|
||
| }); | ||
|
|
||
| collectionUsers.Add(new CollectionUser | ||
| { | ||
| CollectionId = collectionId, | ||
| OrganizationUserId = orgUserId, | ||
| ReadOnly = false, | ||
| HidePasswords = false, | ||
| Manage = true, | ||
| }); | ||
| } | ||
|
|
||
| return (collectionUsers, collections); | ||
| } | ||
|
|
||
| public class CollectionWithGroupsAndUsers : Collection | ||
| { | ||
| public CollectionWithGroupsAndUsers() { } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| ๏ปฟusing AutoMapper; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.Collections; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Utilities; | ||
| using Bit.Infrastructure.EntityFramework.Models; | ||
| using Bit.Infrastructure.EntityFramework.Repositories.Queries; | ||
| using LinqToDB.EntityFrameworkCore; | ||
|
|
@@ -794,7 +794,7 @@ private static async Task ReplaceCollectionUsersAsync(DatabaseContext dbContext, | |
| // SaveChangesAsync is expected to be called outside this method | ||
| } | ||
|
|
||
| public async Task UpsertDefaultCollectionsAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, string defaultCollectionName) | ||
| public async Task CreateDefaultCollectionsAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, string defaultCollectionName) | ||
| { | ||
| organizationUserIds = organizationUserIds.ToList(); | ||
| if (!organizationUserIds.Any()) | ||
|
|
@@ -808,15 +808,15 @@ public async Task UpsertDefaultCollectionsAsync(Guid organizationId, IEnumerable | |
| var orgUserIdWithDefaultCollection = await GetOrgUserIdsWithDefaultCollectionAsync(dbContext, organizationId); | ||
| var missingDefaultCollectionUserIds = organizationUserIds.Except(orgUserIdWithDefaultCollection); | ||
|
|
||
| var (collectionUsers, collections) = BuildDefaultCollectionForUsers(organizationId, missingDefaultCollectionUserIds, defaultCollectionName); | ||
| var (collections, collectionUsers) = CollectionUtils.BuildDefaultUserCollections(organizationId, missingDefaultCollectionUserIds, defaultCollectionName); | ||
|
|
||
| if (!collectionUsers.Any() || !collections.Any()) | ||
| if (!collections.Any() || !collectionUsers.Any()) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| await dbContext.BulkCopyAsync(collections); | ||
| await dbContext.BulkCopyAsync(collectionUsers); | ||
| await dbContext.BulkCopyAsync(Mapper.Map<IEnumerable<Collection>>(collections)); | ||
| await dbContext.BulkCopyAsync(Mapper.Map<IEnumerable<CollectionUser>>(collectionUsers)); | ||
|
|
||
| await dbContext.SaveChangesAsync(); | ||
| } | ||
|
Comment on lines
797
to
822
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. โ CRITICAL: Missing transaction wrapper for atomic operations This method performs multiple database operations (BulkCopyAsync for Collections, BulkCopyAsync for CollectionUsers) without wrapping them in an explicit transaction. This violates the atomicity requirement stated in the stored procedure comment: "this MUST be executed in a single transaction to ensure consistency." Risk: Partial failures could leave the database in an inconsistent state where collections are created without their corresponding collection users, or vice versa. Evidence: The Dapper implementation correctly wraps these operations in an explicit transaction (lines 410-436 in Dapper CollectionRepository.cs): await using var transaction = connection.BeginTransaction();
try {
// operations
await transaction.CommitAsync();
} catch {
await transaction.RollbackAsync();
throw;
}Fix: Wrap the entire method body in a transaction similar to the Dapper implementation, or document why EF's implicit transaction handling is sufficient here (though BulkCopyAsync typically bypasses EF's change tracker). |
||
|
|
@@ -844,37 +844,7 @@ private async Task<HashSet<Guid>> GetOrgUserIdsWithDefaultCollectionAsync(Databa | |
| return results.ToHashSet(); | ||
| } | ||
|
|
||
| private (List<CollectionUser> collectionUser, List<Collection> collection) BuildDefaultCollectionForUsers(Guid organizationId, IEnumerable<Guid> missingDefaultCollectionUserIds, string defaultCollectionName) | ||
| { | ||
| var collectionUsers = new List<CollectionUser>(); | ||
| var collections = new List<Collection>(); | ||
|
|
||
| foreach (var orgUserId in missingDefaultCollectionUserIds) | ||
| { | ||
| var collectionId = CoreHelpers.GenerateComb(); | ||
|
|
||
| collections.Add(new Collection | ||
| { | ||
| Id = collectionId, | ||
| OrganizationId = organizationId, | ||
| Name = defaultCollectionName, | ||
| CreationDate = DateTime.UtcNow, | ||
| RevisionDate = DateTime.UtcNow, | ||
| Type = CollectionType.DefaultUserCollection, | ||
| DefaultUserCollectionEmail = null | ||
|
|
||
| }); | ||
|
|
||
| collectionUsers.Add(new CollectionUser | ||
| { | ||
| CollectionId = collectionId, | ||
| OrganizationUserId = orgUserId, | ||
| ReadOnly = false, | ||
| HidePasswords = false, | ||
| Manage = true, | ||
| }); | ||
| } | ||
|
|
||
| return (collectionUsers, collections); | ||
| } | ||
| public Task CreateDefaultCollectionsBulkAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, | ||
| string defaultCollectionName) => | ||
| CreateDefaultCollectionsAsync(organizationId, organizationUserIds, defaultCollectionName); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.