1
0
mirror of https://github.com/bitwarden/server synced 2025-12-26 05:03:18 +00:00

[PM-24055] - Collection Users and Groups null on Public response (#6713)

* Integration test around getting and saving collection with group/user permissions

* This adds groups to the collections returned.

* Added new stored procedures so we don't accidentally wipe out access due to null parameters.

* wrapping all calls in transaction in the event that there is an error.
This commit is contained in:
Jared McCannon
2025-12-17 11:34:17 -06:00
committed by GitHub
parent 886ba9ae6d
commit de504d800b
10 changed files with 609 additions and 17 deletions

View File

@@ -2,6 +2,7 @@
#nullable disable
using System.ComponentModel.DataAnnotations;
using System.Text.Json.Serialization;
using Bit.Api.Models.Public.Response;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.Models.Data;
@@ -13,6 +14,12 @@ namespace Bit.Api.AdminConsole.Public.Models.Response;
/// </summary>
public class GroupResponseModel : GroupBaseModel, IResponseModel
{
[JsonConstructor]
public GroupResponseModel()
{
}
public GroupResponseModel(Group group, IEnumerable<CollectionAccessSelection> collections)
{
if (group == null)

View File

@@ -2,6 +2,7 @@
#nullable disable
using System.ComponentModel.DataAnnotations;
using System.Text.Json.Serialization;
using Bit.Api.AdminConsole.Public.Models.Response;
using Bit.Core.Entities;
using Bit.Core.Models.Data;
@@ -13,6 +14,12 @@ namespace Bit.Api.Models.Public.Response;
/// </summary>
public class CollectionResponseModel : CollectionBaseModel, IResponseModel
{
[JsonConstructor]
public CollectionResponseModel()
{
}
public CollectionResponseModel(Collection collection, IEnumerable<CollectionAccessSelection> groups)
{
if (collection == null)

View File

@@ -65,10 +65,11 @@ public class CollectionsController : Controller
[ProducesResponseType(typeof(ListResponseModel<CollectionResponseModel>), (int)HttpStatusCode.OK)]
public async Task<IActionResult> List()
{
var collections = await _collectionRepository.GetManySharedCollectionsByOrganizationIdAsync(
_currentContext.OrganizationId.Value);
// 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 collections = await _collectionRepository.GetManyByOrganizationIdWithAccessAsync(_currentContext.OrganizationId.Value);
var collectionResponses = collections.Select(c =>
new CollectionResponseModel(c.Item1, c.Item2.Groups));
var response = new ListResponseModel<CollectionResponseModel>(collectionResponses);
return new JsonResult(response);
}

View File

@@ -226,7 +226,6 @@ public class CollectionRepository : Repository<Collection, Guid>, ICollectionRep
{
obj.SetNewId();
var objWithGroupsAndUsers = JsonSerializer.Deserialize<CollectionWithGroupsAndUsers>(JsonSerializer.Serialize(obj))!;
objWithGroupsAndUsers.Groups = groups != null ? groups.ToArrayTVP() : Enumerable.Empty<CollectionAccessSelection>().ToArrayTVP();
@@ -243,18 +242,52 @@ public class CollectionRepository : Repository<Collection, Guid>, ICollectionRep
public async Task ReplaceAsync(Collection obj, IEnumerable<CollectionAccessSelection>? groups, IEnumerable<CollectionAccessSelection>? users)
{
var objWithGroupsAndUsers = JsonSerializer.Deserialize<CollectionWithGroupsAndUsers>(JsonSerializer.Serialize(obj))!;
objWithGroupsAndUsers.Groups = groups != null ? groups.ToArrayTVP() : Enumerable.Empty<CollectionAccessSelection>().ToArrayTVP();
objWithGroupsAndUsers.Users = users != null ? users.ToArrayTVP() : Enumerable.Empty<CollectionAccessSelection>().ToArrayTVP();
using (var connection = new SqlConnection(ConnectionString))
await using var connection = new SqlConnection(ConnectionString);
await connection.OpenAsync();
await using var transaction = await connection.BeginTransactionAsync();
try
{
var results = await connection.ExecuteAsync(
$"[{Schema}].[Collection_UpdateWithGroupsAndUsers]",
objWithGroupsAndUsers,
commandType: CommandType.StoredProcedure);
if (groups == null && users == null)
{
await connection.ExecuteAsync(
$"[{Schema}].[Collection_Update]",
obj,
commandType: CommandType.StoredProcedure,
transaction: transaction);
}
else if (groups != null && users == null)
{
await connection.ExecuteAsync(
$"[{Schema}].[Collection_UpdateWithGroups]",
new CollectionWithGroups(obj, groups),
commandType: CommandType.StoredProcedure,
transaction: transaction);
}
else if (groups == null && users != null)
{
await connection.ExecuteAsync(
$"[{Schema}].[Collection_UpdateWithUsers]",
new CollectionWithUsers(obj, users),
commandType: CommandType.StoredProcedure,
transaction: transaction);
}
else if (groups != null && users != null)
{
await connection.ExecuteAsync(
$"[{Schema}].[Collection_UpdateWithGroupsAndUsers]",
new CollectionWithGroupsAndUsers(obj, groups, users),
commandType: CommandType.StoredProcedure,
transaction: transaction);
}
await transaction.CommitAsync();
}
catch
{
await transaction.RollbackAsync();
throw;
}
}
public async Task DeleteManyAsync(IEnumerable<Guid> collectionIds)
@@ -424,9 +457,70 @@ public class CollectionRepository : Repository<Collection, Guid>, ICollectionRep
public class CollectionWithGroupsAndUsers : Collection
{
public CollectionWithGroupsAndUsers() { }
public CollectionWithGroupsAndUsers(Collection collection,
IEnumerable<CollectionAccessSelection> groups,
IEnumerable<CollectionAccessSelection> users)
{
Id = collection.Id;
Name = collection.Name;
OrganizationId = collection.OrganizationId;
CreationDate = collection.CreationDate;
RevisionDate = collection.RevisionDate;
Type = collection.Type;
ExternalId = collection.ExternalId;
DefaultUserCollectionEmail = collection.DefaultUserCollectionEmail;
Groups = groups.ToArrayTVP();
Users = users.ToArrayTVP();
}
[DisallowNull]
public DataTable? Groups { get; set; }
[DisallowNull]
public DataTable? Users { get; set; }
}
public class CollectionWithGroups : Collection
{
public CollectionWithGroups() { }
public CollectionWithGroups(Collection collection, IEnumerable<CollectionAccessSelection> groups)
{
Id = collection.Id;
Name = collection.Name;
OrganizationId = collection.OrganizationId;
CreationDate = collection.CreationDate;
RevisionDate = collection.RevisionDate;
Type = collection.Type;
ExternalId = collection.ExternalId;
DefaultUserCollectionEmail = collection.DefaultUserCollectionEmail;
Groups = groups.ToArrayTVP();
}
[DisallowNull]
public DataTable? Groups { get; set; }
}
public class CollectionWithUsers : Collection
{
public CollectionWithUsers() { }
public CollectionWithUsers(Collection collection, IEnumerable<CollectionAccessSelection> users)
{
Id = collection.Id;
Name = collection.Name;
OrganizationId = collection.OrganizationId;
CreationDate = collection.CreationDate;
RevisionDate = collection.RevisionDate;
Type = collection.Type;
ExternalId = collection.ExternalId;
DefaultUserCollectionEmail = collection.DefaultUserCollectionEmail;
Users = users.ToArrayTVP();
}
[DisallowNull]
public DataTable? Users { get; set; }
}
}

View File

@@ -0,0 +1,74 @@
CREATE PROCEDURE [dbo].[Collection_UpdateWithGroups]
@Id UNIQUEIDENTIFIER,
@OrganizationId UNIQUEIDENTIFIER,
@Name VARCHAR(MAX),
@ExternalId NVARCHAR(300),
@CreationDate DATETIME2(7),
@RevisionDate DATETIME2(7),
@Groups AS [dbo].[CollectionAccessSelectionType] READONLY,
@DefaultUserCollectionEmail NVARCHAR(256) = NULL,
@Type TINYINT = 0
AS
BEGIN
SET NOCOUNT ON
EXEC [dbo].[Collection_Update] @Id, @OrganizationId, @Name, @ExternalId, @CreationDate, @RevisionDate, @DefaultUserCollectionEmail, @Type
-- Groups
-- Delete groups that are no longer in source
DELETE
cg
FROM
[dbo].[CollectionGroup] cg
LEFT JOIN
@Groups g ON cg.GroupId = g.Id
WHERE
cg.CollectionId = @Id
AND g.Id IS NULL;
-- Update existing groups
UPDATE
cg
SET
cg.ReadOnly = g.ReadOnly,
cg.HidePasswords = g.HidePasswords,
cg.Manage = g.Manage
FROM
[dbo].[CollectionGroup] cg
INNER JOIN
@Groups g ON cg.GroupId = g.Id
WHERE
cg.CollectionId = @Id
AND (
cg.ReadOnly != g.ReadOnly
OR cg.HidePasswords != g.HidePasswords
OR cg.Manage != g.Manage
);
-- Insert new groups
INSERT INTO [dbo].[CollectionGroup]
(
[CollectionId],
[GroupId],
[ReadOnly],
[HidePasswords],
[Manage]
)
SELECT
@Id,
g.Id,
g.ReadOnly,
g.HidePasswords,
g.Manage
FROM
@Groups g
INNER JOIN
[dbo].[Group] grp ON grp.Id = g.Id
LEFT JOIN
[dbo].[CollectionGroup] cg ON cg.CollectionId = @Id AND cg.GroupId = g.Id
WHERE
grp.OrganizationId = @OrganizationId
AND cg.CollectionId IS NULL;
EXEC [dbo].[User_BumpAccountRevisionDateByCollectionId] @Id, @OrganizationId
END

View File

@@ -0,0 +1,74 @@
CREATE PROCEDURE [dbo].[Collection_UpdateWithUsers]
@Id UNIQUEIDENTIFIER,
@OrganizationId UNIQUEIDENTIFIER,
@Name VARCHAR(MAX),
@ExternalId NVARCHAR(300),
@CreationDate DATETIME2(7),
@RevisionDate DATETIME2(7),
@Users AS [dbo].[CollectionAccessSelectionType] READONLY,
@DefaultUserCollectionEmail NVARCHAR(256) = NULL,
@Type TINYINT = 0
AS
BEGIN
SET NOCOUNT ON
EXEC [dbo].[Collection_Update] @Id, @OrganizationId, @Name, @ExternalId, @CreationDate, @RevisionDate, @DefaultUserCollectionEmail, @Type
-- Users
-- Delete users that are no longer in source
DELETE
cu
FROM
[dbo].[CollectionUser] cu
LEFT JOIN
@Users u ON cu.OrganizationUserId = u.Id
WHERE
cu.CollectionId = @Id
AND u.Id IS NULL;
-- Update existing users
UPDATE
cu
SET
cu.ReadOnly = u.ReadOnly,
cu.HidePasswords = u.HidePasswords,
cu.Manage = u.Manage
FROM
[dbo].[CollectionUser] cu
INNER JOIN
@Users u ON cu.OrganizationUserId = u.Id
WHERE
cu.CollectionId = @Id
AND (
cu.ReadOnly != u.ReadOnly
OR cu.HidePasswords != u.HidePasswords
OR cu.Manage != u.Manage
);
-- Insert new users
INSERT INTO [dbo].[CollectionUser]
(
[CollectionId],
[OrganizationUserId],
[ReadOnly],
[HidePasswords],
[Manage]
)
SELECT
@Id,
u.Id,
u.ReadOnly,
u.HidePasswords,
u.Manage
FROM
@Users u
INNER JOIN
[dbo].[OrganizationUser] ou ON ou.Id = u.Id
LEFT JOIN
[dbo].[CollectionUser] cu ON cu.CollectionId = @Id AND cu.OrganizationUserId = u.Id
WHERE
ou.OrganizationId = @OrganizationId
AND cu.CollectionId IS NULL;
EXEC [dbo].[User_BumpAccountRevisionDateByCollectionId] @Id, @OrganizationId
END

View File

@@ -0,0 +1,117 @@
using Bit.Api.AdminConsole.Public.Models.Request;
using Bit.Api.IntegrationTest.Factories;
using Bit.Api.IntegrationTest.Helpers;
using Bit.Api.Models.Public.Request;
using Bit.Api.Models.Public.Response;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.Billing.Enums;
using Bit.Core.Enums;
using Bit.Core.Models.Data;
using Bit.Core.Platform.Push;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Xunit;
namespace Bit.Api.IntegrationTest.Controllers.Public;
public class CollectionsControllerTests : IClassFixture<ApiApplicationFactory>, IAsyncLifetime
{
private readonly HttpClient _client;
private readonly ApiApplicationFactory _factory;
private readonly LoginHelper _loginHelper;
private string _ownerEmail = null!;
private Organization _organization = null!;
public CollectionsControllerTests(ApiApplicationFactory factory)
{
_factory = factory;
_factory.SubstituteService<IPushNotificationService>(_ => { });
_factory.SubstituteService<IFeatureService>(_ => { });
_client = factory.CreateClient();
_loginHelper = new LoginHelper(_factory, _client);
}
public async Task InitializeAsync()
{
_ownerEmail = $"integration-test{Guid.NewGuid()}@bitwarden.com";
await _factory.LoginWithNewAccount(_ownerEmail);
(_organization, _) = await OrganizationTestHelpers.SignUpAsync(_factory,
plan: PlanType.EnterpriseAnnually,
ownerEmail: _ownerEmail,
passwordManagerSeats: 10,
paymentMethod: PaymentMethodType.Card);
await _loginHelper.LoginWithOrganizationApiKeyAsync(_organization.Id);
}
public Task DisposeAsync()
{
_client.Dispose();
return Task.CompletedTask;
}
[Fact]
public async Task CreateCollectionWithMultipleUsersAndVariedPermissions_Success()
{
// Arrange
_organization.AllowAdminAccessToAllCollectionItems = true;
await _factory.GetService<IOrganizationRepository>().UpsertAsync(_organization);
var groupRepository = _factory.GetService<IGroupRepository>();
var group = await groupRepository.CreateAsync(new Group
{
OrganizationId = _organization.Id,
Name = "CollectionControllerTests.CreateCollectionWithMultipleUsersAndVariedPermissions_Success",
ExternalId = $"CollectionControllerTests.CreateCollectionWithMultipleUsersAndVariedPermissions_Success{Guid.NewGuid()}",
});
var (_, user) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(
_factory,
_organization.Id,
OrganizationUserType.User);
var collection = await OrganizationTestHelpers.CreateCollectionAsync(
_factory,
_organization.Id,
"Shared Collection with a group",
externalId: "shared-collection-with-group",
groups:
[
new CollectionAccessSelection { Id = group.Id, ReadOnly = false, HidePasswords = false, Manage = true }
],
users:
[
new CollectionAccessSelection { Id = user.Id, ReadOnly = false, HidePasswords = false, Manage = true }
]);
var getCollectionsResponse = await _client.GetFromJsonAsync<ListResponseModel<CollectionResponseModel>>("public/collections");
var getCollectionResponse = await _client.GetFromJsonAsync<CollectionResponseModel>($"public/collections/{collection.Id}");
var firstCollection = getCollectionsResponse.Data.First(x => x.ExternalId == "shared-collection-with-group");
var update = new CollectionUpdateRequestModel
{
ExternalId = firstCollection.ExternalId,
Groups = firstCollection.Groups?.Select(x => new AssociationWithPermissionsRequestModel
{
Id = x.Id,
ReadOnly = x.ReadOnly,
HidePasswords = x.HidePasswords,
Manage = x.Manage
}),
};
await _client.PutAsJsonAsync($"public/collections/{firstCollection.Id}", update);
var result = await _factory.GetService<ICollectionRepository>()
.GetByIdWithAccessAsync(firstCollection.Id);
Assert.NotNull(result);
Assert.NotEmpty(result.Item2.Groups);
Assert.NotEmpty(result.Item2.Users);
}
}

View File

@@ -159,14 +159,16 @@ public static class OrganizationTestHelpers
Guid organizationId,
string name,
IEnumerable<CollectionAccessSelection>? users = null,
IEnumerable<CollectionAccessSelection>? groups = null)
IEnumerable<CollectionAccessSelection>? groups = null,
string? externalId = null)
{
var collectionRepository = factory.GetService<ICollectionRepository>();
var collection = new Collection
{
OrganizationId = organizationId,
Name = name,
Type = CollectionType.SharedCollection
Type = CollectionType.SharedCollection,
ExternalId = externalId
};
await collectionRepository.CreateAsync(collection, groups, users);

View File

@@ -144,4 +144,69 @@ public class CollectionRepositoryReplaceTests
await userRepository.DeleteAsync(user);
await organizationRepository.DeleteAsync(organization);
}
[Theory, DatabaseData]
public async Task ReplaceAsync_WhenNotPassingGroupsOrUsers_DoesNotDeleteAccess(
IUserRepository userRepository,
IOrganizationRepository organizationRepository,
IOrganizationUserRepository organizationUserRepository,
IGroupRepository groupRepository,
ICollectionRepository collectionRepository)
{
// Arrange
var organization = await organizationRepository.CreateTestOrganizationAsync();
var user1 = await userRepository.CreateTestUserAsync();
var orgUser1 = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user1);
var user2 = await userRepository.CreateTestUserAsync();
var orgUser2 = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user2);
var group1 = await groupRepository.CreateTestGroupAsync(organization);
var group2 = await groupRepository.CreateTestGroupAsync(organization);
var collection = new Collection
{
Name = "Test Collection Name",
OrganizationId = organization.Id,
};
await collectionRepository.CreateAsync(collection,
[
new CollectionAccessSelection { Id = group1.Id, Manage = true, HidePasswords = true, ReadOnly = false, },
new CollectionAccessSelection { Id = group2.Id, Manage = false, HidePasswords = false, ReadOnly = true, },
],
[
new CollectionAccessSelection { Id = orgUser1.Id, Manage = true, HidePasswords = false, ReadOnly = true },
new CollectionAccessSelection { Id = orgUser2.Id, Manage = false, HidePasswords = true, ReadOnly = false },
]
);
// Act
collection.Name = "Updated Collection Name";
await collectionRepository.ReplaceAsync(collection, null, null);
// Assert
var (actualCollection, actualAccess) = await collectionRepository.GetByIdWithAccessAsync(collection.Id);
Assert.NotNull(actualCollection);
Assert.Equal("Updated Collection Name", actualCollection.Name);
var groups = actualAccess.Groups.ToArray();
Assert.Equal(2, groups.Length);
Assert.Single(groups, g => g.Id == group1.Id && g.Manage && g.HidePasswords && !g.ReadOnly);
Assert.Single(groups, g => g.Id == group2.Id && !g.Manage && !g.HidePasswords && g.ReadOnly);
var users = actualAccess.Users.ToArray();
Assert.Equal(2, users.Length);
Assert.Single(users, u => u.Id == orgUser1.Id && u.Manage && !u.HidePasswords && u.ReadOnly);
Assert.Single(users, u => u.Id == orgUser2.Id && !u.Manage && u.HidePasswords && !u.ReadOnly);
// Clean up data
await userRepository.DeleteAsync(user1);
await userRepository.DeleteAsync(user2);
await organizationRepository.DeleteAsync(organization);
}
}

View File

@@ -0,0 +1,151 @@
CREATE OR ALTER PROCEDURE [dbo].[Collection_UpdateWithUsers]
@Id UNIQUEIDENTIFIER,
@OrganizationId UNIQUEIDENTIFIER,
@Name VARCHAR(MAX),
@ExternalId NVARCHAR(300),
@CreationDate DATETIME2(7),
@RevisionDate DATETIME2(7),
@Users AS [dbo].[CollectionAccessSelectionType] READONLY,
@DefaultUserCollectionEmail NVARCHAR(256) = NULL,
@Type TINYINT = 0
AS
BEGIN
SET NOCOUNT ON
EXEC [dbo].[Collection_Update] @Id, @OrganizationId, @Name, @ExternalId, @CreationDate, @RevisionDate, @DefaultUserCollectionEmail, @Type
-- Users
-- Delete users that are no longer in source
DELETE
cu
FROM
[dbo].[CollectionUser] cu
LEFT JOIN
@Users u ON cu.OrganizationUserId = u.Id
WHERE
cu.CollectionId = @Id
AND u.Id IS NULL;
-- Update existing users
UPDATE
cu
SET
cu.ReadOnly = u.ReadOnly,
cu.HidePasswords = u.HidePasswords,
cu.Manage = u.Manage
FROM
[dbo].[CollectionUser] cu
INNER JOIN
@Users u ON cu.OrganizationUserId = u.Id
WHERE
cu.CollectionId = @Id
AND (
cu.ReadOnly != u.ReadOnly
OR cu.HidePasswords != u.HidePasswords
OR cu.Manage != u.Manage
);
-- Insert new users
INSERT INTO [dbo].[CollectionUser]
(
[CollectionId],
[OrganizationUserId],
[ReadOnly],
[HidePasswords],
[Manage]
)
SELECT
@Id,
u.Id,
u.ReadOnly,
u.HidePasswords,
u.Manage
FROM
@Users u
INNER JOIN
[dbo].[OrganizationUser] ou ON ou.Id = u.Id
LEFT JOIN
[dbo].[CollectionUser] cu ON cu.CollectionId = @Id AND cu.OrganizationUserId = u.Id
WHERE
ou.OrganizationId = @OrganizationId
AND cu.CollectionId IS NULL;
EXEC [dbo].[User_BumpAccountRevisionDateByCollectionId] @Id, @OrganizationId
END
GO
CREATE OR ALTER PROCEDURE [dbo].[Collection_UpdateWithGroups]
@Id UNIQUEIDENTIFIER,
@OrganizationId UNIQUEIDENTIFIER,
@Name VARCHAR(MAX),
@ExternalId NVARCHAR(300),
@CreationDate DATETIME2(7),
@RevisionDate DATETIME2(7),
@Groups AS [dbo].[CollectionAccessSelectionType] READONLY,
@DefaultUserCollectionEmail NVARCHAR(256) = NULL,
@Type TINYINT = 0
AS
BEGIN
SET NOCOUNT ON
EXEC [dbo].[Collection_Update] @Id, @OrganizationId, @Name, @ExternalId, @CreationDate, @RevisionDate, @DefaultUserCollectionEmail, @Type
-- Groups
-- Delete groups that are no longer in source
DELETE
cg
FROM
[dbo].[CollectionGroup] cg
LEFT JOIN
@Groups g ON cg.GroupId = g.Id
WHERE
cg.CollectionId = @Id
AND g.Id IS NULL;
-- Update existing groups
UPDATE
cg
SET
cg.ReadOnly = g.ReadOnly,
cg.HidePasswords = g.HidePasswords,
cg.Manage = g.Manage
FROM
[dbo].[CollectionGroup] cg
INNER JOIN
@Groups g ON cg.GroupId = g.Id
WHERE
cg.CollectionId = @Id
AND (
cg.ReadOnly != g.ReadOnly
OR cg.HidePasswords != g.HidePasswords
OR cg.Manage != g.Manage
);
-- Insert new groups
INSERT INTO [dbo].[CollectionGroup]
(
[CollectionId],
[GroupId],
[ReadOnly],
[HidePasswords],
[Manage]
)
SELECT
@Id,
g.Id,
g.ReadOnly,
g.HidePasswords,
g.Manage
FROM
@Groups g
INNER JOIN
[dbo].[Group] grp ON grp.Id = g.Id
LEFT JOIN
[dbo].[CollectionGroup] cg ON cg.CollectionId = @Id AND cg.GroupId = g.Id
WHERE
grp.OrganizationId = @OrganizationId
AND cg.CollectionId IS NULL;
EXEC [dbo].[User_BumpAccountRevisionDateByCollectionId] @Id, @OrganizationId
END
GO