Skip to content

[PM-39554] Create default collection for demoted admin #7918

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.

Sign up for GitHub

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

Open
BTreston wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[PM-39554] Create default collection for demoted admin #7918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5272670
create default collecion for demoted admin
BTreston Jul 2, 2026
27d921a
fix tests, add org ability check and policyRequirement check
BTreston Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Api/AdminConsole/Controllers/OrganizationUsersController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequ
var existingUserType = organizationUser.Type;

await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), existingUserType, userId,
collectionsToSave, groupsToSave);
collectionsToSave, groupsToSave, model.DefaultUserCollectionName);
}

[HttpPost("{id}")]
Expand Down
2 changes: 2 additions & 0 deletions src/Api/AdminConsole/Models/Request/Organizations/OrganizationUserRequestModels.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ public class OrganizationUserUpdateRequestModel

[StringLength(50)]
public string? Name { get; set; }

public string? DefaultUserCollectionName { get; set; }
#nullable disable

public OrganizationUser ToOrganizationUser(OrganizationUser existingUser)
Expand Down
3 changes: 2 additions & 1 deletion ...nsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interface
public interface IUpdateOrganizationUserCommand
{
Task UpdateUserAsync(OrganizationUser organizationUser, OrganizationUserType existingUserType, Guid? savingUserId,
List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess);
List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess,
string? defaultUserCollectionName = null);
}
24 changes: 22 additions & 2 deletions ...Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
ο»Ώ#nullable enable
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.Billing.Enums;
using Bit.Core.Billing.Pricing;
Expand Down Expand Up @@ -27,6 +29,7 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
private readonly IHasConfirmedOwnersExceptQuery _hasConfirmedOwnersExceptQuery;
private readonly IPricingClient _pricingClient;
private readonly TimeProvider _timeProvider;
private readonly IPolicyRequirementQuery _policyRequirementQuery;

public UpdateOrganizationUserCommand(
IEventService eventService,
Expand All @@ -39,7 +42,8 @@ public UpdateOrganizationUserCommand(
IGroupRepository groupRepository,
IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery,
IPricingClient pricingClient,
TimeProvider timeProvider)
TimeProvider timeProvider,
IPolicyRequirementQuery policyRequirementQuery)
{
_eventService = eventService;
_organizationService = organizationService;
Expand All @@ -52,6 +56,7 @@ public UpdateOrganizationUserCommand(
_hasConfirmedOwnersExceptQuery = hasConfirmedOwnersExceptQuery;
_pricingClient = pricingClient;
_timeProvider = timeProvider;
_policyRequirementQuery = policyRequirementQuery;
}

/// <summary>
Expand All @@ -65,7 +70,8 @@ public UpdateOrganizationUserCommand(
/// <exception cref="BadRequestException"></exception>
public async Task UpdateUserAsync(OrganizationUser organizationUser, OrganizationUserType existingUserType,
Guid? savingUserId,
List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess)
List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess,
string? defaultUserCollectionName = null)
{
// Avoid multiple enumeration
var collectionAccessList = collectionAccess?.ToList() ?? [];
Expand Down Expand Up @@ -145,6 +151,20 @@ public async Task UpdateUserAsync(OrganizationUser organizationUser, Organizatio
await _organizationUserRepository.UpdateGroupsAsync(organizationUser.Id, groupAccess, _timeProvider.GetUtcNow().UtcDateTime);
}

var isDemotedFromPrivilegedRole = existingUserType is OrganizationUserType.Admin or OrganizationUserType.Owner
&& organizationUser.Type is not (OrganizationUserType.Admin or OrganizationUserType.Owner);
if (isDemotedFromPrivilegedRole
&& organizationUser.UserId.HasValue
&& organization.UseMyItems
&& !string.IsNullOrWhiteSpace(defaultUserCollectionName)
&& (await _policyRequirementQuery.GetAsync<OrganizationDataOwnershipPolicyRequirement>(organizationUser.UserId.Value)).State == OrganizationDataOwnershipState.Enabled)
{
await _collectionRepository.CreateDefaultCollectionsAsync(
organizationUser.OrganizationId,
[organizationUser.Id],
defaultUserCollectionName);
}
Comment on lines +154 to +166

@claude claude Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT: Default collection is created without checking organization.UseMyItems or the Organization Data Ownership policy, unlike every other call site.

Details and fix

The three other places that call CreateDefaultCollectionsAsync all gate creation on both organization.UseMyItems and the OrganizationDataOwnershipPolicyRequirement, not just a non-empty name:

  • ConfirmOrganizationUserCommand.CreateManyDefaultCollectionsAsync (checks organization.UseMyItems + ShouldCreateDefaultCollection)
  • RestoreOrganizationUserCommand (checks organization.UseMyItems + policy State == Enabled)
  • BulkAutomaticallyConfirmOrganizationUsersCommand.CreateDefaultCollectionsForManyAsync (checks !organization.UseMyItems + ShouldCreateDefaultCollection)

Here, only !string.IsNullOrWhiteSpace(defaultUserCollectionName) is checked. Because the name comes from the client request, a demotion in an organization that has "My Items" disabled (or where the data-ownership policy does not apply to the user) will still create a DefaultUserCollection, contradicting the organization's configuration. Server-side enforcement should not depend on the client omitting the name.

The organization entity is already loaded at line 86, so at minimum the UseMyItems guard is a cheap addition:

var isDemotedFromPrivilegedRole = existingUserType is OrganizationUserType.Admin or OrganizationUserType.Owner
    && organizationUser.Type is not (OrganizationUserType.Admin or OrganizationUserType.Owner);
if (isDemotedFromPrivilegedRole
    && organization.UseMyItems
    && !string.IsNullOrWhiteSpace(defaultUserCollectionName))
{
    await _collectionRepository.CreateDefaultCollectionsAsync(...);
}

If the data-ownership policy should also gate this (as it does elsewhere), inject IPolicyRequirementQuery and mirror the sibling checks. If the divergence is intentional for the demotion flow, a brief comment explaining why would help future readers.


await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Updated);
}

Expand Down
12 changes: 8 additions & 4 deletions test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ await sutProvider.GetDependency<IUpdateOrganizationUserCommand>().Received(1).Up
savingUserId,
Arg.Is<List<CollectionAccessSelection>>(cas =>
cas.All(c => model.Collections.Any(m => m.Id == c.Id))),
model.Groups);
model.Groups,
model.DefaultUserCollectionName);
}

[Theory]
Expand Down Expand Up @@ -110,7 +111,8 @@ await sutProvider.GetDependency<IUpdateOrganizationUserCommand>().Received(1).Up
Arg.Is<List<CollectionAccessSelection>>(cas =>
cas.All(c => model.Collections.Any(m => m.Id == c.Id))),
// Main assertion: groups are not updated (are null)
null);
null,
model.DefaultUserCollectionName);
}

[Theory]
Expand Down Expand Up @@ -146,7 +148,8 @@ await sutProvider.GetDependency<IUpdateOrganizationUserCommand>().Received(1).Up
savingUserId,
Arg.Is<List<CollectionAccessSelection>>(cas =>
cas.All(c => model.Collections.Any(m => m.Id == c.Id))),
model.Groups);
model.Groups,
model.DefaultUserCollectionName);
}

[Theory]
Expand Down Expand Up @@ -227,7 +230,8 @@ await sutProvider.GetDependency<IUpdateOrganizationUserCommand>().Received(1).Up
cas.First(c => c.Id == editedCollectionId).Manage == true &&
cas.First(c => c.Id == editedCollectionId).ReadOnly == false &&
cas.First(c => c.Id == editedCollectionId).HidePasswords == false),
model.Groups);
model.Groups,
model.DefaultUserCollectionName);
}

[Theory]
Expand Down
117 changes: 117 additions & 0 deletions ...AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.Billing.Enums;
using Bit.Core.Entities;
Expand Down Expand Up @@ -282,6 +284,112 @@ await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).Repla
);
}

[Theory]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task UpdateUserAsync_WhenDemotingPrivilegedUserToUser_WithDefaultCollectionName_AndUseMyItemsEnabled_AndPolicyEnabled_CreatesDefaultCollection(
OrganizationUserType existingUserType,
Organization organization,
OrganizationUser oldUserData,
OrganizationUser newUserData,
string defaultUserCollectionName,
SutProvider<UpdateOrganizationUserCommand> sutProvider)
{
organization.UseMyItems = true;
newUserData.Type = OrganizationUserType.User;
Setup(sutProvider, organization, newUserData, oldUserData);
SetupDataOwnershipPolicy(sutProvider, newUserData.UserId!.Value, OrganizationDataOwnershipState.Enabled);

await sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null, defaultUserCollectionName);

await sutProvider.GetDependency<ICollectionRepository>().Received(1).CreateDefaultCollectionsAsync(
newUserData.OrganizationId,
Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(newUserData.Id)),
defaultUserCollectionName);
}

[Theory]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task UpdateUserAsync_WhenDemotingPrivilegedUserToUser_WithDefaultCollectionName_AndUseMyItemsDisabled_DoesNotCreateDefaultCollection(
OrganizationUserType existingUserType,
Organization organization,
OrganizationUser oldUserData,
OrganizationUser newUserData,
string defaultUserCollectionName,
SutProvider<UpdateOrganizationUserCommand> sutProvider)
{
organization.UseMyItems = false;
newUserData.Type = OrganizationUserType.User;
Setup(sutProvider, organization, newUserData, oldUserData);

await sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null, defaultUserCollectionName);

await sutProvider.GetDependency<ICollectionRepository>().DidNotReceive().CreateDefaultCollectionsAsync(
Arg.Any<Guid>(), Arg.Any<IEnumerable<Guid>>(), Arg.Any<string>());
}

[Theory]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task UpdateUserAsync_WhenDemotingPrivilegedUserToUser_WithDefaultCollectionName_AndPolicyDisabled_DoesNotCreateDefaultCollection(
OrganizationUserType existingUserType,
Organization organization,
OrganizationUser oldUserData,
OrganizationUser newUserData,
string defaultUserCollectionName,
SutProvider<UpdateOrganizationUserCommand> sutProvider)
{
organization.UseMyItems = true;
newUserData.Type = OrganizationUserType.User;
Setup(sutProvider, organization, newUserData, oldUserData);
SetupDataOwnershipPolicy(sutProvider, newUserData.UserId!.Value, OrganizationDataOwnershipState.Disabled);

await sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null, defaultUserCollectionName);

await sutProvider.GetDependency<ICollectionRepository>().DidNotReceive().CreateDefaultCollectionsAsync(
Arg.Any<Guid>(), Arg.Any<IEnumerable<Guid>>(), Arg.Any<string>());
}

[Theory]
[BitAutoData(OrganizationUserType.User)]
[BitAutoData(OrganizationUserType.Custom)]
public async Task UpdateUserAsync_WhenExistingUserIsNotPrivileged_WithDefaultCollectionName_DoesNotCreateDefaultCollection(
OrganizationUserType existingUserType,
Organization organization,
OrganizationUser oldUserData,
OrganizationUser newUserData,
string defaultUserCollectionName,
SutProvider<UpdateOrganizationUserCommand> sutProvider)
{
newUserData.Type = OrganizationUserType.User;
Setup(sutProvider, organization, newUserData, oldUserData);

await sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null, defaultUserCollectionName);

await sutProvider.GetDependency<ICollectionRepository>().DidNotReceive().CreateDefaultCollectionsAsync(
Arg.Any<Guid>(), Arg.Any<IEnumerable<Guid>>(), Arg.Any<string>());
}

[Theory]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task UpdateUserAsync_WhenDemotingPrivilegedUserToUser_WithoutDefaultCollectionName_DoesNotCreateDefaultCollection(
OrganizationUserType existingUserType,
Organization organization,
OrganizationUser oldUserData,
OrganizationUser newUserData,
SutProvider<UpdateOrganizationUserCommand> sutProvider)
{
newUserData.Type = OrganizationUserType.User;
Setup(sutProvider, organization, newUserData, oldUserData);

await sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null);

await sutProvider.GetDependency<ICollectionRepository>().DidNotReceive().CreateDefaultCollectionsAsync(
Arg.Any<Guid>(), Arg.Any<IEnumerable<Guid>>(), Arg.Any<string>());
}

private void Setup(SutProvider<UpdateOrganizationUserCommand> sutProvider, Organization organization,
OrganizationUser newUser, OrganizationUser oldUser)
{
Expand All @@ -301,4 +409,13 @@ private void Setup(SutProvider<UpdateOrganizationUserCommand> sutProvider, Organ
Arg.Is<IEnumerable<Guid>>(i => i.Contains(oldUser.Id)))
.Returns(true);
}

private void SetupDataOwnershipPolicy(SutProvider<UpdateOrganizationUserCommand> sutProvider,
Guid userId, OrganizationDataOwnershipState state)
{
var requirement = new OrganizationDataOwnershipPolicyRequirement(state, []);
sutProvider.GetDependency<IPolicyRequirementQuery>()
.GetAsync<OrganizationDataOwnershipPolicyRequirement>(userId)
.Returns(requirement);
}
}
Loading