Skip to content

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

Open
BTreston wants to merge 2 commits into
mainfrom
ac/pm-39554
Open

[PM-39554] Create default collection for demoted admin#7918
BTreston wants to merge 2 commits into
mainfrom
ac/pm-39554

Conversation

@BTreston

@BTreston BTreston commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-39554

📔 Objective

This PR fixes an issue when an admin's role is demoted to a user, no "My Items" collection is created for them.

📸 Screenshots

@BTreston BTreston requested a review from a team as a code owner July 2, 2026 16:23
@BTreston BTreston requested a review from sven-bitwarden July 2, 2026 16:23
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed the change that creates a default ("My Items") collection when an Admin/Owner is demoted to a lower role in UpdateOrganizationUserCommand. The demotion detection, the plumbing of DefaultUserCollectionName through the request model, interface, and controller, and the unit tests are all sound. One consistency gap stands out: the new call to CreateDefaultCollectionsAsync skips the UseMyItems and Organization Data Ownership policy guards that every other call site enforces.

Code Review Details
  • ⚠️ : Default collection created without checking organization.UseMyItems or the data-ownership policy, unlike the three sibling call sites (confirm, restore, auto-confirm)
    • src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs:149

Comment on lines +149 to +157
var isDemotedFromPrivilegedRole = existingUserType is OrganizationUserType.Admin or OrganizationUserType.Owner
&& organizationUser.Type is not (OrganizationUserType.Admin or OrganizationUserType.Owner);
if (isDemotedFromPrivilegedRole && !string.IsNullOrWhiteSpace(defaultUserCollectionName))
{
await _collectionRepository.CreateDefaultCollectionsAsync(
organizationUser.OrganizationId,
[organizationUser.Id],
defaultUserCollectionName);
}

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.

@BTreston BTreston added the t:bugfix Change Type - Bugfix label Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.36%. Comparing base (b9876f7) to head (27d921a).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...OrganizationUsers/UpdateOrganizationUserCommand.cs 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7918      +/-   ##
==========================================
+ Coverage   61.34%   61.36%   +0.02%     
==========================================
  Files        2236     2236              
  Lines       98517    98563      +46     
  Branches     8907     8913       +6     
==========================================
+ Hits        60437    60486      +49     
+ Misses      35943    35941       -2     
+ Partials     2137     2136       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants