mirror of
https://github.com/bitwarden/server.git
synced 2025-12-10 00:42:07 -06:00
[PM-23987] Fix saving to default collections by updating collection lookup (#6122)
* Refactor ICollectionRepository.GetManyByOrganizationIdAsync logic to include default user collections * Add stored procedure Collection_ReadSharedCollectionsByOrganizationId to retrieve collections by organization ID, excluding default user collections. * Add GetManySharedCollectionsByOrganizationIdAsync method to ICollectionRepository and its implementations to retrieve collections excluding default user collections. * Add unit test for GetManySharedCollectionsByOrganizationIdAsync method in CollectionRepositoryTests to verify retrieval of collections excluding default user collections. * Refactor controllers to use GetManySharedCollectionsByOrganizationIdAsync for retrieving shared collections * Update unit tests to use GetManySharedCollectionsByOrganizationIdAsync for verifying shared collections retrieval * Revert CiphersController.CanEditItemsInCollections to use GetManyByOrganizationIdAsync for retrieving organization collections * Update stored procedures to retrieve only DefaultUserCollection by modifying the WHERE clause in Collection_ReadSharedCollectionsByOrganizationId.sql and its corresponding migration script. * Update EF CollectionRepository.GetManySharedCollectionsByOrganizationIdAsync to filter collections by SharedCollection * Update OrganizationUserRepository.GetManyDetailsByOrganizationAsync_vNext to only include Shared collections * Update comments in stored procedure and migration script to clarify filtering for SharedCollections only
This commit is contained in:
parent
52ef3ef7a5
commit
6dea40c868
@ -109,7 +109,7 @@ public class CollectionsController : Controller
|
|||||||
var readAllAuthorized = (await _authorizationService.AuthorizeAsync(User, CollectionOperations.ReadAll(orgId))).Succeeded;
|
var readAllAuthorized = (await _authorizationService.AuthorizeAsync(User, CollectionOperations.ReadAll(orgId))).Succeeded;
|
||||||
if (readAllAuthorized)
|
if (readAllAuthorized)
|
||||||
{
|
{
|
||||||
orgCollections = await _collectionRepository.GetManyByOrganizationIdAsync(orgId);
|
orgCollections = await _collectionRepository.GetManySharedCollectionsByOrganizationIdAsync(orgId);
|
||||||
}
|
}
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
|
|||||||
@ -65,7 +65,7 @@ public class CollectionsController : Controller
|
|||||||
[ProducesResponseType(typeof(ListResponseModel<CollectionResponseModel>), (int)HttpStatusCode.OK)]
|
[ProducesResponseType(typeof(ListResponseModel<CollectionResponseModel>), (int)HttpStatusCode.OK)]
|
||||||
public async Task<IActionResult> List()
|
public async Task<IActionResult> List()
|
||||||
{
|
{
|
||||||
var collections = await _collectionRepository.GetManyByOrganizationIdAsync(
|
var collections = await _collectionRepository.GetManySharedCollectionsByOrganizationIdAsync(
|
||||||
_currentContext.OrganizationId.Value);
|
_currentContext.OrganizationId.Value);
|
||||||
// TODO: Get all CollectionGroup associations for the organization and marry them up here for the response.
|
// TODO: Get all CollectionGroup associations for the organization and marry them up here for the response.
|
||||||
var collectionResponses = collections.Select(c => new CollectionResponseModel(c, null));
|
var collectionResponses = collections.Select(c => new CollectionResponseModel(c, null));
|
||||||
|
|||||||
@ -16,10 +16,16 @@ public interface ICollectionRepository : IRepository<Collection, Guid>
|
|||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Return all collections that belong to the organization. Does not include any permission details or group/user
|
/// Return all collections that belong to the organization. Does not include any permission details or group/user
|
||||||
/// access relationships. Excludes default collections (My Items collections).
|
/// access relationships.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
Task<ICollection<Collection>> GetManyByOrganizationIdAsync(Guid organizationId);
|
Task<ICollection<Collection>> GetManyByOrganizationIdAsync(Guid organizationId);
|
||||||
|
|
||||||
|
/// <inheritdoc cref="GetManyByOrganizationIdAsync"/>
|
||||||
|
/// <remarks>
|
||||||
|
/// Excludes default collections (My Items collections) - used by Admin Console Collections tab.
|
||||||
|
/// </remarks>
|
||||||
|
Task<ICollection<Collection>> GetManySharedCollectionsByOrganizationIdAsync(Guid organizationId);
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Return all shared collections that belong to the organization. Includes group/user access relationships for each collection.
|
/// Return all shared collections that belong to the organization. Includes group/user access relationships for each collection.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
|
|||||||
@ -79,6 +79,19 @@ public class CollectionRepository : Repository<Collection, Guid>, ICollectionRep
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public async Task<ICollection<Collection>> GetManySharedCollectionsByOrganizationIdAsync(Guid organizationId)
|
||||||
|
{
|
||||||
|
using (var connection = new SqlConnection(ConnectionString))
|
||||||
|
{
|
||||||
|
var results = await connection.QueryAsync<Collection>(
|
||||||
|
$"[{Schema}].[{Table}_ReadSharedCollectionsByOrganizationId]",
|
||||||
|
new { OrganizationId = organizationId },
|
||||||
|
commandType: CommandType.StoredProcedure);
|
||||||
|
|
||||||
|
return results.ToList();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
public async Task<ICollection<Tuple<Collection, CollectionAccessDetails>>> GetManyByOrganizationIdWithAccessAsync(Guid organizationId)
|
public async Task<ICollection<Tuple<Collection, CollectionAccessDetails>>> GetManyByOrganizationIdWithAccessAsync(Guid organizationId)
|
||||||
{
|
{
|
||||||
using (var connection = new SqlConnection(ConnectionString))
|
using (var connection = new SqlConnection(ConnectionString))
|
||||||
|
|||||||
@ -441,13 +441,15 @@ public class OrganizationUserRepository : Repository<Core.Entities.OrganizationU
|
|||||||
: new List<Guid>(),
|
: new List<Guid>(),
|
||||||
|
|
||||||
Collections = includeCollections
|
Collections = includeCollections
|
||||||
? ou.CollectionUsers.Select(cu => new CollectionAccessSelection
|
? ou.CollectionUsers
|
||||||
{
|
.Where(cu => cu.Collection.Type == CollectionType.SharedCollection)
|
||||||
Id = cu.CollectionId,
|
.Select(cu => new CollectionAccessSelection
|
||||||
ReadOnly = cu.ReadOnly,
|
{
|
||||||
HidePasswords = cu.HidePasswords,
|
Id = cu.CollectionId,
|
||||||
Manage = cu.Manage
|
ReadOnly = cu.ReadOnly,
|
||||||
}).ToList()
|
HidePasswords = cu.HidePasswords,
|
||||||
|
Manage = cu.Manage
|
||||||
|
}).ToList()
|
||||||
: new List<CollectionAccessSelection>()
|
: new List<CollectionAccessSelection>()
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
@ -212,13 +212,26 @@ public class CollectionRepository : Repository<Core.Entities.Collection, Collect
|
|||||||
}
|
}
|
||||||
|
|
||||||
public async Task<ICollection<Core.Entities.Collection>> GetManyByOrganizationIdAsync(Guid organizationId)
|
public async Task<ICollection<Core.Entities.Collection>> GetManyByOrganizationIdAsync(Guid organizationId)
|
||||||
|
{
|
||||||
|
using (var scope = ServiceScopeFactory.CreateScope())
|
||||||
|
{
|
||||||
|
var dbContext = GetDatabaseContext(scope);
|
||||||
|
var query = from c in dbContext.Collections
|
||||||
|
where c.OrganizationId == organizationId
|
||||||
|
select c;
|
||||||
|
var collections = await query.ToArrayAsync();
|
||||||
|
return collections;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
public async Task<ICollection<Core.Entities.Collection>> GetManySharedCollectionsByOrganizationIdAsync(Guid organizationId)
|
||||||
{
|
{
|
||||||
using (var scope = ServiceScopeFactory.CreateScope())
|
using (var scope = ServiceScopeFactory.CreateScope())
|
||||||
{
|
{
|
||||||
var dbContext = GetDatabaseContext(scope);
|
var dbContext = GetDatabaseContext(scope);
|
||||||
var query = from c in dbContext.Collections
|
var query = from c in dbContext.Collections
|
||||||
where c.OrganizationId == organizationId &&
|
where c.OrganizationId == organizationId &&
|
||||||
c.Type != CollectionType.DefaultUserCollection
|
c.Type == CollectionType.SharedCollection
|
||||||
select c;
|
select c;
|
||||||
var collections = await query.ToArrayAsync();
|
var collections = await query.ToArrayAsync();
|
||||||
return collections;
|
return collections;
|
||||||
|
|||||||
@ -9,6 +9,5 @@ BEGIN
|
|||||||
FROM
|
FROM
|
||||||
[dbo].[CollectionView]
|
[dbo].[CollectionView]
|
||||||
WHERE
|
WHERE
|
||||||
[OrganizationId] = @OrganizationId AND
|
[OrganizationId] = @OrganizationId
|
||||||
[Type] != 1 -- Exclude DefaultUserCollection
|
|
||||||
END
|
END
|
||||||
@ -0,0 +1,14 @@
|
|||||||
|
CREATE PROCEDURE [dbo].[Collection_ReadSharedCollectionsByOrganizationId]
|
||||||
|
@OrganizationId UNIQUEIDENTIFIER
|
||||||
|
AS
|
||||||
|
BEGIN
|
||||||
|
SET NOCOUNT ON
|
||||||
|
|
||||||
|
SELECT
|
||||||
|
*
|
||||||
|
FROM
|
||||||
|
[dbo].[CollectionView]
|
||||||
|
WHERE
|
||||||
|
[OrganizationId] = @OrganizationId AND
|
||||||
|
[Type] = 0 -- SharedCollections only
|
||||||
|
END
|
||||||
@ -26,6 +26,8 @@ BEGIN
|
|||||||
SELECT cu.*
|
SELECT cu.*
|
||||||
FROM [dbo].[CollectionUser] cu
|
FROM [dbo].[CollectionUser] cu
|
||||||
INNER JOIN [dbo].[OrganizationUser] ou ON cu.OrganizationUserId = ou.Id
|
INNER JOIN [dbo].[OrganizationUser] ou ON cu.OrganizationUserId = ou.Id
|
||||||
WHERE ou.OrganizationId = @OrganizationId
|
INNER JOIN [dbo].[Collection] c ON cu.CollectionId = c.Id
|
||||||
|
WHERE ou.OrganizationId = @OrganizationId
|
||||||
|
AND c.Type = 0 -- SharedCollections only
|
||||||
END
|
END
|
||||||
END
|
END
|
||||||
|
|||||||
@ -173,12 +173,12 @@ public class CollectionsControllerTests
|
|||||||
.Returns(AuthorizationResult.Success());
|
.Returns(AuthorizationResult.Success());
|
||||||
|
|
||||||
sutProvider.GetDependency<ICollectionRepository>()
|
sutProvider.GetDependency<ICollectionRepository>()
|
||||||
.GetManyByOrganizationIdAsync(organization.Id)
|
.GetManySharedCollectionsByOrganizationIdAsync(organization.Id)
|
||||||
.Returns(collections);
|
.Returns(collections);
|
||||||
|
|
||||||
var response = await sutProvider.Sut.Get(organization.Id);
|
var response = await sutProvider.Sut.Get(organization.Id);
|
||||||
|
|
||||||
await sutProvider.GetDependency<ICollectionRepository>().Received(1).GetManyByOrganizationIdAsync(organization.Id);
|
await sutProvider.GetDependency<ICollectionRepository>().Received(1).GetManySharedCollectionsByOrganizationIdAsync(organization.Id);
|
||||||
|
|
||||||
Assert.Equal(collections.Count, response.Data.Count());
|
Assert.Equal(collections.Count, response.Data.Count());
|
||||||
}
|
}
|
||||||
|
|||||||
@ -525,7 +525,7 @@ public class CollectionRepositoryTests
|
|||||||
var collection3 = new Collection { Name = "Collection 3", OrganizationId = organization.Id, };
|
var collection3 = new Collection { Name = "Collection 3", OrganizationId = organization.Id, };
|
||||||
await collectionRepository.CreateAsync(collection3, null, null);
|
await collectionRepository.CreateAsync(collection3, null, null);
|
||||||
|
|
||||||
// Create a default user collection (should not be returned by this method)
|
// Create a default user collection
|
||||||
var defaultCollection = new Collection
|
var defaultCollection = new Collection
|
||||||
{
|
{
|
||||||
Name = "My Items",
|
Name = "My Items",
|
||||||
@ -536,12 +536,73 @@ public class CollectionRepositoryTests
|
|||||||
|
|
||||||
var collections = await collectionRepository.GetManyByOrganizationIdAsync(organization.Id);
|
var collections = await collectionRepository.GetManyByOrganizationIdAsync(organization.Id);
|
||||||
|
|
||||||
|
Assert.NotNull(collections);
|
||||||
|
Assert.Equal(4, collections.Count);
|
||||||
|
Assert.All(collections, c => Assert.Equal(organization.Id, c.OrganizationId));
|
||||||
|
|
||||||
|
Assert.Contains(collections, c => c.Name == "Collection 1");
|
||||||
|
Assert.Contains(collections, c => c.Name == "Collection 2");
|
||||||
|
Assert.Contains(collections, c => c.Name == "Collection 3");
|
||||||
|
Assert.Contains(collections, c => c.Name == "My Items");
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Test to ensure organization properly retrieves shared collections
|
||||||
|
/// </summary>
|
||||||
|
[DatabaseTheory, DatabaseData]
|
||||||
|
public async Task GetManySharedCollectionsByOrganizationIdAsync_Success(
|
||||||
|
IUserRepository userRepository,
|
||||||
|
IOrganizationRepository organizationRepository,
|
||||||
|
ICollectionRepository collectionRepository,
|
||||||
|
IOrganizationUserRepository organizationUserRepository)
|
||||||
|
{
|
||||||
|
var user = await userRepository.CreateAsync(new User
|
||||||
|
{
|
||||||
|
Name = "Test User",
|
||||||
|
Email = $"test+{Guid.NewGuid()}@email.com",
|
||||||
|
ApiKey = "TEST",
|
||||||
|
SecurityStamp = "stamp",
|
||||||
|
});
|
||||||
|
|
||||||
|
var organization = await organizationRepository.CreateAsync(new Organization
|
||||||
|
{
|
||||||
|
Name = "Test Org",
|
||||||
|
PlanType = PlanType.EnterpriseAnnually,
|
||||||
|
Plan = "Test Plan",
|
||||||
|
BillingEmail = "billing@email.com"
|
||||||
|
});
|
||||||
|
|
||||||
|
var orgUser = await organizationUserRepository.CreateAsync(new OrganizationUser
|
||||||
|
{
|
||||||
|
OrganizationId = organization.Id,
|
||||||
|
UserId = user.Id,
|
||||||
|
Status = OrganizationUserStatusType.Confirmed,
|
||||||
|
});
|
||||||
|
|
||||||
|
var collection1 = new Collection { Name = "Collection 1", OrganizationId = organization.Id, };
|
||||||
|
await collectionRepository.CreateAsync(collection1, null, null);
|
||||||
|
|
||||||
|
var collection2 = new Collection { Name = "Collection 2", OrganizationId = organization.Id, };
|
||||||
|
await collectionRepository.CreateAsync(collection2, null, null);
|
||||||
|
|
||||||
|
var collection3 = new Collection { Name = "Collection 3", OrganizationId = organization.Id, };
|
||||||
|
await collectionRepository.CreateAsync(collection3, null, null);
|
||||||
|
|
||||||
|
// Create a default user collection (should not be returned by this method)
|
||||||
|
var defaultCollection = new Collection
|
||||||
|
{
|
||||||
|
Name = "My Items",
|
||||||
|
OrganizationId = organization.Id,
|
||||||
|
Type = CollectionType.DefaultUserCollection
|
||||||
|
};
|
||||||
|
await collectionRepository.CreateAsync(defaultCollection, null, null);
|
||||||
|
|
||||||
|
var collections = await collectionRepository.GetManySharedCollectionsByOrganizationIdAsync(organization.Id);
|
||||||
|
|
||||||
Assert.NotNull(collections);
|
Assert.NotNull(collections);
|
||||||
Assert.Equal(3, collections.Count); // Should only return the 3 shared collections, excluding the default user collection
|
Assert.Equal(3, collections.Count); // Should only return the 3 shared collections, excluding the default user collection
|
||||||
Assert.All(collections, c => Assert.Equal(organization.Id, c.OrganizationId));
|
Assert.All(collections, c => Assert.Equal(organization.Id, c.OrganizationId));
|
||||||
Assert.All(collections, c => Assert.NotEqual(CollectionType.DefaultUserCollection, c.Type));
|
|
||||||
|
|
||||||
// Verify specific collections are returned
|
|
||||||
Assert.Contains(collections, c => c.Name == "Collection 1");
|
Assert.Contains(collections, c => c.Name == "Collection 1");
|
||||||
Assert.Contains(collections, c => c.Name == "Collection 2");
|
Assert.Contains(collections, c => c.Name == "Collection 2");
|
||||||
Assert.Contains(collections, c => c.Name == "Collection 3");
|
Assert.Contains(collections, c => c.Name == "Collection 3");
|
||||||
|
|||||||
@ -911,6 +911,17 @@ public class OrganizationUserRepositoryTests
|
|||||||
RevisionDate = requestTime
|
RevisionDate = requestTime
|
||||||
});
|
});
|
||||||
|
|
||||||
|
var defaultUserCollection = await collectionRepository.CreateAsync(new Collection
|
||||||
|
{
|
||||||
|
Id = CoreHelpers.GenerateComb(),
|
||||||
|
OrganizationId = organization.Id,
|
||||||
|
Name = "My Items",
|
||||||
|
Type = CollectionType.DefaultUserCollection,
|
||||||
|
DefaultUserCollectionEmail = user1.Email,
|
||||||
|
CreationDate = requestTime,
|
||||||
|
RevisionDate = requestTime
|
||||||
|
});
|
||||||
|
|
||||||
// Create organization user with both groups and collections using CreateManyAsync
|
// Create organization user with both groups and collections using CreateManyAsync
|
||||||
var createOrgUserWithCollections = new List<CreateOrganizationUser>
|
var createOrgUserWithCollections = new List<CreateOrganizationUser>
|
||||||
{
|
{
|
||||||
@ -940,6 +951,13 @@ public class OrganizationUserRepositoryTests
|
|||||||
ReadOnly = false,
|
ReadOnly = false,
|
||||||
HidePasswords = true,
|
HidePasswords = true,
|
||||||
Manage = true
|
Manage = true
|
||||||
|
},
|
||||||
|
new CollectionAccessSelection
|
||||||
|
{
|
||||||
|
Id = defaultUserCollection.Id,
|
||||||
|
ReadOnly = false,
|
||||||
|
HidePasswords = false,
|
||||||
|
Manage = true
|
||||||
}
|
}
|
||||||
],
|
],
|
||||||
Groups = [group1.Id, group2.Id]
|
Groups = [group1.Id, group2.Id]
|
||||||
@ -969,6 +987,7 @@ public class OrganizationUserRepositoryTests
|
|||||||
Assert.Equal(2, user1Result.Collections.Count());
|
Assert.Equal(2, user1Result.Collections.Count());
|
||||||
Assert.Contains(user1Result.Collections, c => c.Id == collection1.Id);
|
Assert.Contains(user1Result.Collections, c => c.Id == collection1.Id);
|
||||||
Assert.Contains(user1Result.Collections, c => c.Id == collection2.Id);
|
Assert.Contains(user1Result.Collections, c => c.Id == collection2.Id);
|
||||||
|
Assert.DoesNotContain(user1Result.Collections, c => c.Id == defaultUserCollection.Id);
|
||||||
}
|
}
|
||||||
|
|
||||||
[DatabaseTheory, DatabaseData]
|
[DatabaseTheory, DatabaseData]
|
||||||
|
|||||||
@ -0,0 +1,65 @@
|
|||||||
|
CREATE OR ALTER PROCEDURE [dbo].[Collection_ReadByOrganizationId]
|
||||||
|
@OrganizationId UNIQUEIDENTIFIER
|
||||||
|
AS
|
||||||
|
BEGIN
|
||||||
|
SET NOCOUNT ON
|
||||||
|
|
||||||
|
SELECT
|
||||||
|
*
|
||||||
|
FROM
|
||||||
|
[dbo].[CollectionView]
|
||||||
|
WHERE
|
||||||
|
[OrganizationId] = @OrganizationId
|
||||||
|
END
|
||||||
|
GO
|
||||||
|
|
||||||
|
CREATE OR ALTER PROCEDURE [dbo].[Collection_ReadSharedCollectionsByOrganizationId]
|
||||||
|
@OrganizationId UNIQUEIDENTIFIER
|
||||||
|
AS
|
||||||
|
BEGIN
|
||||||
|
SET NOCOUNT ON
|
||||||
|
|
||||||
|
SELECT
|
||||||
|
*
|
||||||
|
FROM
|
||||||
|
[dbo].[CollectionView]
|
||||||
|
WHERE
|
||||||
|
[OrganizationId] = @OrganizationId AND
|
||||||
|
[Type] = 0 -- SharedCollections only
|
||||||
|
END
|
||||||
|
GO
|
||||||
|
|
||||||
|
CREATE OR ALTER PROCEDURE [dbo].[OrganizationUserUserDetails_ReadByOrganizationId_V2]
|
||||||
|
@OrganizationId UNIQUEIDENTIFIER,
|
||||||
|
@IncludeGroups BIT = 0,
|
||||||
|
@IncludeCollections BIT = 0
|
||||||
|
AS
|
||||||
|
BEGIN
|
||||||
|
SET NOCOUNT ON
|
||||||
|
|
||||||
|
-- Result Set 1: User Details (always returned)
|
||||||
|
SELECT *
|
||||||
|
FROM [dbo].[OrganizationUserUserDetailsView]
|
||||||
|
WHERE OrganizationId = @OrganizationId
|
||||||
|
|
||||||
|
-- Result Set 2: Group associations (if requested)
|
||||||
|
IF @IncludeGroups = 1
|
||||||
|
BEGIN
|
||||||
|
SELECT gu.*
|
||||||
|
FROM [dbo].[GroupUser] gu
|
||||||
|
INNER JOIN [dbo].[OrganizationUser] ou ON gu.OrganizationUserId = ou.Id
|
||||||
|
WHERE ou.OrganizationId = @OrganizationId
|
||||||
|
END
|
||||||
|
|
||||||
|
-- Result Set 3: Collection associations (if requested)
|
||||||
|
IF @IncludeCollections = 1
|
||||||
|
BEGIN
|
||||||
|
SELECT cu.*
|
||||||
|
FROM [dbo].[CollectionUser] cu
|
||||||
|
INNER JOIN [dbo].[OrganizationUser] ou ON cu.OrganizationUserId = ou.Id
|
||||||
|
INNER JOIN [dbo].[Collection] c ON cu.CollectionId = c.Id
|
||||||
|
WHERE ou.OrganizationId = @OrganizationId
|
||||||
|
AND c.Type = 0 -- SharedCollections only
|
||||||
|
END
|
||||||
|
END
|
||||||
|
GO
|
||||||
Loading…
x
Reference in New Issue
Block a user