[PM-26099] Update public list members endpoint to include collections

[r-tome]

🎟️ Tracking

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

📔 Objective

Implement the big TODO on the Public Member’s Controller

Returned data example:

{
    "object": "list",
    "data": [
        {
            "object": "member",
            "id": "4339708f-5e9e-4db5-a111-b32f00fb1301",
            "userId": "3cb3996d-21bc-4f8f-8f3b-b32f00faee5f",
            "name": null,
            "email": "[email protected]",
            "twoFactorEnabled": false,
            "status": 2,
            "collections": [
                {
                    "id": "09721a9f-1658-4776-a995-b32f00fb131b",
                    "readOnly": false,
                    "hidePasswords": false,
                    "manage": true
                },
                {
                    "id": "ee2c9dc4-c106-4ca6-8d4b-b34d0093bc1e",
                    "readOnly": true,
                    "hidePasswords": false,
                    "manage": false
                }
            ],
            "resetPasswordEnrolled": false,
            "ssoExternalId": null,
            "type": 0,
            "externalId": null,
            "permissions": null
        },
        {
            "object": "member",
            "id": "eca6d8e8-ca9f-4e3f-b1b7-b36200a79d88",
            "userId": "e86d3a6f-e55e-45a4-9231-b36200a7a960",
            "name": null,
            "email": "[email protected]",
            "twoFactorEnabled": false,
            "status": 2,
            "collections": [],
            "resetPasswordEnrolled": true,
            "ssoExternalId": null,
            "type": 2,
            "externalId": null,
            "permissions": null
        }
    ]
}

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – ed31b52b-6281-4c20-8d75-c112f8c6ad35

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.03%. Comparing base (9b313d9) to head (a154db2).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...minConsole/Public/Controllers/MembersController.cs	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6503      +/-   ##
==========================================
- Coverage   56.04%   52.03%   -4.02%     
==========================================
  Files        1877     1877              
  Lines       82616    82614       -2     
  Branches     7315     7315              
==========================================
- Hits        46299    42985    -3314     
- Misses      34585    37971    +3386     
+ Partials     1732     1658      -74     

☔ View full report in Codecov by Sentry.
📢 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.

[claude]

Claude finished @r-tome's task —— View job

---

Code Review Complete

Summary of Changes

This PR successfully implements the TODO in the Public Members Controller to include collection associations in the List endpoint response. The changes include:

1. MembersController.cs - Updated List() method to fetch and return collection associations
2. AssociationWithPermissionsResponseModel.cs - Added parameterless constructor with [JsonConstructor] attribute
3. MembersControllerTests.cs - Enhanced test coverage with collection validation
4. OrganizationTestHelpers.cs - Added helper method for creating collections with user/group associations

Additionally, the PR includes cleanup work: removed unused dependencies (IUserService, IApplicationCacheService) and removed the #nullable disable directive to enable nullable reference types.

---

Critical Issues

1. ⚠️ Missing Coverage on New Code Path

File: src/Api/AdminConsole/Public/Controllers/MembersController.cs:115-120

According to Codecov, 2 lines are missing coverage with 80% patch coverage. Looking at the diff, the uncovered lines are likely in the List() method. While there is a test (List_Member_Success), it may not cover all execution paths, particularly error scenarios.

Recommendation: Investigate which specific lines lack coverage and add tests for edge cases such as:

• Organizations with no members
• Members with null/empty collection lists
• Error handling if the repository call fails

---

Suggested Improvements

2. 🎨 Potential Performance Concern - N+1 Query Pattern

File: src/Api/AdminConsole/Public/Controllers/MembersController.cs:115

The call to GetManyDetailsByOrganizationAsync(_currentContext.OrganizationId!.Value, includeCollections: true) now includes collections. While this is the correct approach, it's important to verify that the underlying repository method efficiently fetches collections in a single query (or minimal queries) rather than executing a separate query per user.

Recommendation:

• Review the implementation of IOrganizationUserRepository.GetManyDetailsByOrganizationAsync() to ensure it uses JOIN operations or efficient batching
• Consider adding performance testing for organizations with many members (e.g., 100+ users with 50+ collections each)
• Document expected query complexity in code comments if not already done

Example concern pattern

// BAD: N+1 pattern
foreach (var user in users) {
    user.Collections = await GetCollectionsForUser(user.Id); // Separate query per user
}

// GOOD: Single/batched query
var allCollections = await GetAllCollectionsForOrganization(orgId); // Single query
// Map collections to users in memory

3. 🤔 Consider Null-Safety in Lambda Expression

File: src/Api/AdminConsole/Public/Controllers/MembersController.cs:120

return new MemberResponseModel(u, orgUsersTwoFactorIsEnabled.FirstOrDefault(tuple => tuple.user == u).twoFactorIsEnabled, u.Collections);

The FirstOrDefault call returns a tuple that could theoretically be default, which would cause a runtime error when accessing .twoFactorIsEnabled on a default value tuple.

Recommendation: Add null-safety check or use a more defensive pattern:

var twoFactorEnabled = orgUsersTwoFactorIsEnabled.FirstOrDefault(tuple => tuple.user == u).twoFactorIsEnabled;
// Or more defensive:
var twoFactorTuple = orgUsersTwoFactorIsEnabled.FirstOrDefault(tuple => tuple.user == u);
return new MemberResponseModel(u, twoFactorTuple.twoFactorIsEnabled, u.Collections);

However, this may be acceptable if the logic guarantees that every user in organizationUserUserDetails will have a corresponding entry in orgUsersTwoFactorIsEnabled. Consider adding a debug assertion or comment explaining this invariant.

4. 📝 API Documentation Update

File: src/Api/AdminConsole/Public/Controllers/MembersController.cs:108

The XML doc comment was updated from "do not include" to "include information about their associated collections", which is good. However, consider enhancing the documentation:

Recommendation: Add information about:

• What collection permissions are included (ReadOnly, HidePasswords, Manage)
• Performance implications for large organizations
• That owners/admins may have empty collections arrays (they have implicit access)

Suggested documentation

/// <summary>
/// List all members.
/// </summary>
/// <remarks>
/// Returns a list of your organization's members.
/// Member objects listed in this call include information about their associated collections,
/// including the collection ID and permission flags (ReadOnly, HidePasswords, Manage).
/// Note: Members with Owner or Admin roles may have empty collection arrays as they have
/// implicit access to all collections through their role.
/// </remarks>

5. ⛏ JsonConstructor Placement

File: src/Api/AdminConsole/Public/Models/Response/AssociationWithPermissionsResponseModel.cs:8

The [JsonConstructor] attribute is correctly placed, but the parameterless constructor is now duplicating functionality.

Minor suggestion: Consider whether the parameterless constructor is necessary or if the attribute alone suffices. If the base class already has a parameterless constructor, this may be redundant. However, if it's required for deserialization clarity, it's fine as-is.

6. 🎨 Test Organization and Readability

File: test/Api.IntegrationTest/AdminConsole/Public/Controllers/MembersControllerTests.cs:56-126

The List_Member_Success test is comprehensive but quite long (70+ lines). While the test coverage is excellent, consider:

Recommendation:

• Breaking into smaller, focused tests (e.g., List_Members_IncludesCollections_Success, List_Members_WithoutCollections_Success)
• Or extracting assertion helper methods to reduce repetition

This is a minor suggestion - the current test is well-structured and thorough.

---

Good Practices Observed

• ✅ Removed unused dependencies (IUserService, IApplicationCacheService)
• ✅ Enabled nullable reference types by removing #nullable disable
• ✅ Comprehensive integration test coverage with multiple scenarios
• ✅ Added reusable CreateCollectionAsync helper for test infrastructure
• ✅ Updated API documentation to reflect new behavior
• ✅ Proper null-forgiving operators where appropriate (OrganizationId!.Value)
• ✅ No sensitive data exposure concerns
• ✅ Security scan (Checkmarx) passed

---

Security Assessment

✅ No security concerns identified

• Collections are properly scoped to the authenticated organization via _currentContext.OrganizationId
• No PII or sensitive vault data is being exposed (collections are organization metadata)
• Proper authorization is enforced via [Authorize("Organization")] attribute
• Repository methods appear to properly filter by organization context

---

Action Items for Author

1. HIGH PRIORITY: Investigate and address the 2 lines with missing test coverage (80% patch coverage per Codecov)
2. MEDIUM PRIORITY: Verify that GetManyDetailsByOrganizationAsync(includeCollections: true) uses efficient queries (no N+1 pattern)
3. LOW PRIORITY: Consider enhancing API documentation with details about collection permissions
4. LOW PRIORITY: Review null-safety pattern in the lambda expression at line 120
5. OPTIONAL: Consider breaking up the long integration test into smaller focused tests

---

Overall Assessment: This is a solid implementation that successfully addresses the TODO. The main concerns are around test coverage (per Codecov report) and verifying query efficiency for the new collections inclusion. The code quality is good, security practices are sound, and the test coverage demonstrates thorough validation of the feature.

---