[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 – abe04035-da68-452b-b3ed-bfa4c1f0629d

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 72

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.15%. Comparing base (0ea9e2e) to head (6ff1b2f).
⚠️ Report is 1 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   52.11%   52.15%   +0.04%     
==========================================
  Files        1908     1908              
  Lines       84415    84413       -2     
  Branches     7537     7537              
==========================================
+ Hits        43992    44026      +34     
+ Misses      38714    38675      -39     
- Partials     1709     1712       +3     

☔ 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

---

PR #6503 Review Complete

Summary of Changes

This PR successfully implements the TODO to include collection data in the Public Members List endpoint (GET /public/members). Key changes:

1. MembersController.cs - Modified List() to fetch and return collection associations
2. AssociationWithPermissionsResponseModel.cs - Added parameterless constructor for JSON deserialization
3. Test files - Enhanced integration tests with comprehensive collection verification
4. Code cleanup - Removed unused dependencies and enabled nullable reference types

---

Critical Issues

🔴 Missing Nullable Reference Type Migration

Location: src/Api/AdminConsole/Public/Models/Response/MemberResponseModel.cs:1-2

While MembersController.cs had its nullable disable directive removed, the related MemberResponseModel.cs still has:

// FIXME: Update this file to be null safe and then delete the line below
#nullable disable

Issue: This creates inconsistency. The controller now uses nullable reference types (e.g., _currentContext.OrganizationId!.Value), but the response model doesn't. This could mask potential null reference issues.

Recommendation: Either:

1. Complete the nullable migration for MemberResponseModel.cs as a follow-up task, OR
2. Document why this file was intentionally excluded from this PR's nullable migration

---

Suggested Improvements

📝 API Documentation Update Needed

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

The XML documentation was updated but could be more explicit:

/// Member objects listed in this call include information about their associated collections.

Suggestion: Be more specific about what collection information is included:

/// Member objects listed in this call include information about their associated collections,
/// including collection IDs and permission settings (ReadOnly, HidePasswords, Manage).

🎨 Unnecessary Parameterless Constructor?

Location: src/Api/AdminConsole/Public/Models/Response/AssociationWithPermissionsResponseModel.cs:8-11

A [JsonConstructor] parameterless constructor was added:

[JsonConstructor]
public AssociationWithPermissionsResponseModel() : base()
{
}

Question: Is this constructor actually needed? The model already has a constructor taking CollectionAccessSelection, and response models typically don't need deserialization support (they're only serialized for output).

Evidence: Looking at the codebase, this is a Response model, not a Request model. Response models generally don't need JSON constructors unless they're also used for deserialization somewhere.

Recommendation: Verify if this constructor is actually required. If it's not needed for deserialization, consider removing it to keep the code minimal.

🤔 Test Coverage Gap - Empty Collections Serialization

Location: test/Api.IntegrationTest/AdminConsole/Public/Controllers/MembersControllerTests.cs:87

The test verifies empty collections:

Assert.Empty(ownerResult.Collections);

Observation: While this tests that the collections property exists and is empty, it doesn't verify that it serializes as "collections": [] rather than "collections": null in the JSON response. Given that the PR objective shows empty arrays in the example, this distinction matters for API consumers.

Suggestion: Consider adding an assertion that explicitly checks the JSON serialization matches the expected format from the PR description.

---

Security Considerations

✅ No security issues identified. The changes:

• Don't introduce new authentication/authorization logic
• Use existing repository methods with proper organization context validation
• Don't expose sensitive data beyond what was already accessible via GET /public/members/{id}
• Maintain the existing authorization pattern ([Authorize("Organization")])

The collection data was already accessible through individual member queries; this PR simply optimizes retrieval for bulk operations.

---

Performance Considerations

✅ Performance improved. The implementation:

• Uses includeCollections: true parameter to fetch collections in a single query (likely uses JOIN)
• Avoids N+1 query problem that would occur if collections were fetched per-member
• Properly leverages the repository layer's optimized query

Note: The 2 missing coverage lines reported by Codecov appear to be in non-critical paths and don't indicate a functional issue.

---

Good Practices Observed

• Comprehensive integration test with multiple scenarios (users with 0, 1, and 2 collections)
• Added helper method CreateCollectionAsync for test reusability
• Proper use of null-forgiving operator (!) after null checks
• Removed unused dependencies (IUserService, IApplicationCacheService)
• Updated XML documentation to reflect new behavior

---

Action Items for Author

1. Required: Address or document the nullable reference type inconsistency with MemberResponseModel.cs
2. Recommended: Verify if the [JsonConstructor] parameterless constructor is actually needed
3. Optional: Enhance XML documentation to be more specific about collection data returned
4. Optional: Consider explicit JSON serialization test for empty collections array

---

Overall Assessment

This is a solid implementation that successfully resolves the TODO. The code is clean, well-tested, and maintains consistency with existing patterns. The main concern is the partial nullable reference type migration which should be addressed for maintainability.

Status: ✅ Approved with minor suggestions