[LFXV2-932] Restrict Committee Member Email Visibility for Non-Privileged Users#53
Conversation
- Updated OpenAPI schema to replace `CommitteeMemberFullWithReadonlyAttributes` with `CommitteeMemberBasicWithReadonlyAttributes`, removing the email field from the basic representation. - Introduced `CommitteeMemberSensitive` schema to encapsulate sensitive information, specifically the email address. - Modified `CommitteeMember` struct to include `CommitteeMemberSensitive` for better separation of concerns. - Updated mock repository to reflect changes in committee member structure, ensuring sensitive information is handled appropriately. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…ndex subject Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…vices Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…ncurrency and error handling Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…mitteeWriterOrchestrator Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughEmail was moved from base member attributes into a new CommitteeMemberSensitive type; API get/create now return a basic member shape. A new GET /committees/{uid}/members/{member_uid}/contact endpoint was added. Member message publishing was refactored into helper builders and a consolidated concurrent publish flow. Tests and mocks updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Committee API
participant Service as Committee Service
participant Writer as Member Writer
participant Helpers as Message Helpers
participant Pool as Worker Pool
participant Indexer as Indexer
participant EventBus as Event Bus
participant ACL as Access Control
Client->>API: create/update/delete member
API->>Service: handle request (validate, persist, convert)
Service->>Writer: publishMemberMessages(ctx, action, data)
Writer->>Helpers: build indexer funcs, event func, acl func
Helpers-->>Writer: publish funcs (or error)
Writer->>Pool: submit publish funcs
Pool->>Indexer: execute indexer publish funcs
Pool->>EventBus: execute event publish func
Pool->>ACL: execute acl publish func
alt any publish error
Pool-->>Writer: error
Writer-->>Service: return error
Service-->>API: return error to Client
else all succeed
Pool-->>Writer: success
Writer-->>Service: success (+ ETag where applicable)
Service-->>API: success
Service-->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (17)
📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-01-02T18:06:31.356ZApplied to files:
🧬 Code graph analysis (3)cmd/committee-api/service/committee_service_response.go (3)
cmd/committee-api/service/committee_service.go (1)
cmd/committee-api/design/committee.go (2)
🔇 Additional comments (11)
✏️ Tip: You can disable this entire section by setting Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements attribute-level visibility control for committee member profiles by splitting the CommitteeMember model to separate sensitive data (email) from basic information. The GET member endpoint now returns only basic information, while sensitive data is indexed separately in OpenSearch with restricted access (auditor/writer privileges).
Purpose: Restrict email visibility for non-privileged users while maintaining full access for create/update operations and privileged users.
Key Changes:
- Split
CommitteeMemberintoCommitteeMemberBaseandCommitteeMemberSensitive - Created
CommitteeMemberBasicWithReadonlyAttributesresponse type excluding email - Updated GET endpoint to return basic info only
- Split indexer messages into two subjects with different access controls
- Updated all tests to use the new structure
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pkg/constants/subjects.go |
Added IndexCommitteeMemberSensitiveSubject constant for sensitive data indexing |
internal/domain/model/committee_member.go |
Split model into CommitteeMemberBase and CommitteeMemberSensitive with embedded structure |
internal/service/committee_member_writer.go |
Refactored message publishing to create separate indexer messages for base and sensitive data |
cmd/committee-api/service/committee_service.go |
Updated GET endpoint to use convertMemberDomainBasicResponse instead of full response |
cmd/committee-api/design/type.go |
Added new types for basic member info and sensitive attributes |
gen/http/openapi3.yaml |
Added CommitteeMemberBasicWithReadonlyAttributes schema and updated GET response |
| Test files | Updated all test data to use new CommitteeMemberSensitive structure |
No issues were identified. The implementation is clean, well-tested, and follows the stated design approach of separating sensitive data at the model layer for access control in the indexer.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/committee_member_writer.go (1)
878-911: Thesyncparameter is unused; event publishing always runs async.The function accepts
sync boolbut hardcodesfalseon line 910. If synchronous event publishing is expected whensync=true, this is a bug.🔎 Proposed fix
return func() error { - return uc.committeePublisher.Event(ctx, eventMessageBuild.Subject, eventMessageBuild, false) + return uc.committeePublisher.Event(ctx, eventMessageBuild.Subject, eventMessageBuild, sync) }, nil
🧹 Nitpick comments (2)
internal/domain/model/committee_member.go (1)
21-25: Consider clarifying or removing the UID duplication comment.The comment on line 24 mentions "uid is duplicated here for storing sensitive info separately," but
CommitteeMemberSensitive(lines 48-51) only contains the🔎 Suggested fix
// CommitteeMember represents the complete committee member business entity type CommitteeMember struct { // CommitteeMemberBase represents the basic profile information CommitteeMemberBase - // CommitteeMemberSensitive represents sensitive profile information with - // embedded sensitive information - uid is duplicated here for storing sensitive info separately + // CommitteeMemberSensitive represents sensitive profile information (e.g., email) + // that requires restricted access control CommitteeMemberSensitive }internal/service/committee_member_writer.go (1)
790-793: Consider computing capacity dynamically.The capacity
4is a magic number that assumes exactly 2 indexer messages + 1 event + 1 access control message. IfmemberMessageIndexeris extended to produce more messages, this will silently under-allocate.🔎 Suggested fix
- messages := make([]func() error, 0, 4) + messages := make([]func() error, 0, len(indexerMessages)+2) messages = append(messages, indexerMessages...) messages = append(messages, eventMessage) messages = append(messages, accessControlMessage)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (8)
gen/committee_service/service.gois excluded by!**/gen/**gen/http/committee_service/client/cli.gois excluded by!**/gen/**gen/http/committee_service/client/types.gois excluded by!**/gen/**gen/http/committee_service/server/types.gois excluded by!**/gen/**gen/http/openapi.jsonis excluded by!**/gen/**gen/http/openapi.yamlis excluded by!**/gen/**gen/http/openapi3.jsonis excluded by!**/gen/**gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (13)
cmd/committee-api/design/committee.gocmd/committee-api/design/type.gocmd/committee-api/service/committee_service.gocmd/committee-api/service/committee_service_response.gocmd/committee-api/service/committee_service_response_test.gocmd/committee-api/service/committee_service_test.gointernal/domain/model/committee_member.gointernal/domain/model/committee_member_test.gointernal/infrastructure/mock/committee.gointernal/service/committee_member_writer.gointernal/service/committee_member_writer_test.gointernal/service/committee_reader_test.gopkg/constants/subjects.go
🧰 Additional context used
🧬 Code graph analysis (8)
cmd/committee-api/design/committee.go (2)
cmd/committee-api/design/type.go (1)
CommitteeMemberBasicWithReadonlyAttributes(424-434)gen/committee_service/service.go (1)
CommitteeMemberBasicWithReadonlyAttributes(170-224)
cmd/committee-api/service/committee_service_response_test.go (2)
internal/domain/model/committee_member.go (1)
CommitteeMemberSensitive(49-51)cmd/committee-api/design/type.go (1)
CommitteeMemberSensitive(391-394)
cmd/committee-api/service/committee_service_test.go (1)
cmd/committee-api/design/type.go (1)
CommitteeMemberSensitive(391-394)
internal/service/committee_member_writer_test.go (6)
internal/domain/model/committee_member.go (4)
CommitteeMemberSensitive(49-51)CommitteeMember(20-26)CommitteeMemberBase(29-46)CommitteeMemberOrganization(68-72)cmd/committee-api/design/type.go (2)
CommitteeMemberSensitive(391-394)CommitteeMemberBase(385-388)internal/domain/model/committee_message.go (4)
MessageAction(19-19)ActionCreated(24-24)ActionUpdated(26-26)ActionDeleted(28-28)pkg/constants/subjects.go (2)
PutMemberCommitteeSubject(57-57)RemoveMemberCommitteeSubject(61-61)pkg/constants/http.go (2)
AuthorizationContextID(27-27)PrincipalContextID(14-14)internal/infrastructure/mock/committee.go (1)
NewMockCommitteePublisher(809-811)
internal/domain/model/committee_member_test.go (2)
internal/domain/model/committee_member.go (3)
CommitteeMemberSensitive(49-51)CommitteeMember(20-26)CommitteeMemberBase(29-46)cmd/committee-api/design/type.go (2)
CommitteeMemberSensitive(391-394)CommitteeMemberBase(385-388)
internal/domain/model/committee_member.go (1)
cmd/committee-api/design/type.go (2)
CommitteeMemberBase(385-388)CommitteeMemberSensitive(391-394)
internal/infrastructure/mock/committee.go (2)
internal/domain/model/committee_member.go (1)
CommitteeMemberSensitive(49-51)cmd/committee-api/design/type.go (1)
CommitteeMemberSensitive(391-394)
cmd/committee-api/service/committee_service_response.go (3)
internal/domain/model/committee_member.go (2)
CommitteeMemberSensitive(49-51)CommitteeMember(20-26)cmd/committee-api/design/type.go (3)
CommitteeMemberSensitive(391-394)CommitteeMemberFullWithReadonlyAttributes(437-448)CommitteeMemberBasicWithReadonlyAttributes(424-434)gen/committee_service/service.go (2)
CommitteeMemberFullWithReadonlyAttributes(228-284)CommitteeMemberBasicWithReadonlyAttributes(170-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: MegaLinter
🔇 Additional comments (22)
pkg/constants/subjects.go (1)
43-45: LGTM!The new constant for sensitive data indexing follows the established naming and documentation patterns. The value is consistent with the separation strategy outlined in the PR objectives.
cmd/committee-api/service/committee_service_test.go (1)
329-331: LGTM!The test data structure correctly reflects the new domain model where
CommitteeMemberSensitive. The test assertions continue to work correctly through Go's struct embedding.internal/domain/model/committee_member_test.go (2)
52-54: LGTM!The test fixtures have been systematically updated to use the new
CommitteeMemberSensitivestructure for theAlso applies to: 128-130, 151-153, 172-174, 189-191, 213-215, 239-241
278-280: LGTM!The
BuildIndexKeytests have been updated to useCommitteeMemberSensitiveand continue to correctly verify that the index key is computed from the committee UID and email combination. The tests validate both consistency and uniqueness.Also applies to: 292-294, 305-307, 318-320, 358-360, 368-370, 377-379
cmd/committee-api/service/committee_service_response_test.go (4)
700-702: LGTM!The
TestConvertMemberPayloadToDomaintest cases have been updated to useCommitteeMemberSensitivefor the email field. The tests cover complete, minimal, and partial payload scenarios and correctly reflect the new domain model structure.Also applies to: 719-721, 745-747, 778-780, 809-812
870-872: LGTM!The
TestConvertMemberDomainToFullResponsetest cases correctly verify that the email field from the nestedCommitteeMemberSensitivestructure is properly mapped to the response. This aligns with the current behavior where Create/Update operations return full member data.Also applies to: 924-926, 955-957, 996-998
1115-1117: LGTM!The
TestConvertPayloadToUpdateMembertest cases have been updated to correctly place the email field in theCommitteeMemberSensitivestructure. The tests cover various update scenarios including complete, minimal, and partial payloads.Also applies to: 1136-1138, 1169-1171, 1203-1205
1243-1245: LGTM!The LinkedIn profile test cases have been updated to use
CommitteeMemberSensitivefor the email field, maintaining consistency with the rest of the test suite. All scenarios (with profile, without profile, empty profile, etc.) correctly reflect the new domain model structure.Also applies to: 1266-1268, 1289-1291, 1312-1314, 1335-1337, 1374-1376, 1402-1404, 1452-1454, 1475-1477, 1498-1500
internal/infrastructure/mock/committee.go (1)
155-157: LGTM! Mock data correctly updated to use the new CommitteeMemberSensitive structure.The Email field is now properly initialized under
CommitteeMemberSensitivefor both sample members, aligning with the domain model refactor that separates sensitive attributes from base attributes.Also applies to: 184-186
internal/service/committee_member_writer_test.go (3)
240-243: LGTM! Test fixtures correctly updated for the new structure.All test member initializations now properly nest Email under
CommitteeMemberSensitive, maintaining consistency with the domain model changes.Also applies to: 261-264, 289-291, 333-335
1112-1283: Good test coverage for the dual-indexing message flow.The
memberMessageIndexertests correctly validate that 2 messages are generated (one for base data, one for sensitive data), which aligns with the PR objective of creating separate OpenSearch documents for different access control policies.
1735-1857: Comprehensive integration test for message functions.The integration test thoroughly validates all three message functions (indexer, event, access control) across create, update, and delete actions. This ensures the message publishing refactor works correctly end-to-end.
internal/domain/model/committee_member.go (1)
48-51: LGTM! Clean separation of sensitive data.The new
CommitteeMemberSensitivestruct properly encapsulates the Email field, enabling attribute-level visibility control as required by the PR objectives.cmd/committee-api/design/committee.go (1)
351-355: LGTM! API response correctly restricts sensitive data for GET requests.The
get-committee-memberendpoint now returnsCommitteeMemberBasicWithReadonlyAttributeswhich excludes the email field, implementing the attribute-level visibility control for non-privileged users. This aligns with the PR objective to hide sensitive attributes based on policy logic.internal/service/committee_reader_test.go (1)
726-728: LGTM! Test fixtures correctly updated for the new model structure.The test member fixtures properly initialize Email within
CommitteeMemberSensitive, and the existing assertions (e.g.,member.Email) continue to work correctly due to Go's embedded field promotion.Also applies to: 833-835
cmd/committee-api/service/committee_service_response.go (1)
329-331: LGTM! Payload conversion correctly populates the new sensitive structure.Email is now properly initialized within
CommitteeMemberSensitivewhen converting from GOA payloads to domain models.Also applies to: 415-417
cmd/committee-api/design/type.go (3)
390-394: LGTM! Clean separation of sensitive attributes in DSL definitions.The new
CommitteeMemberSensitivetype andCommitteeMemberSensitiveAttributes()function properly encapsulate the email field, enabling clear separation between base and sensitive data in the API schema.Also applies to: 410-413
423-434: LGTM! Basic response type correctly excludes sensitive attributes.
CommitteeMemberBasicWithReadonlyAttributesincludes all base member attributes and readonly fields but intentionally omitsCommitteeMemberSensitiveAttributes(), ensuring the email is not exposed to non-privileged users.
445-445: LGTM! Full response and mutation types correctly include sensitive attributes.The
CommitteeMemberFullWithReadonlyAttributes,CommitteeMemberCreateAttributes, andCommitteeMemberUpdateAttributesall includeCommitteeMemberSensitiveAttributes(), ensuring email is available for create/update operations and privileged access scenarios.Also applies to: 453-453, 459-459
internal/service/committee_member_writer.go (3)
816-842: LGTM with a minor observation.The helper function properly handles different actions. Note that for
ActionDeleted, thememberDataparameter is intentionally ignored since only the UID is needed for deletion indexing. This is correct behavior, but a brief comment would clarify this intent for future maintainers.
915-936: LGTM!The access control message helper is clean and correctly passes through the
syncparameter for proper synchronous/asynchronous behavior.
855-873: Good design for sensitive data indexing.The inline struct correctly embeds sensitive fields along with the necessary identifiers (
UID,CommitteeUID) to enable proper document correlation and access control in OpenSearch.
…ganization details conditionally Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Reviewed with [GitHub Copilot](https://github.com/features/copilot) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmd/committee-api/service/committee_service_response.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
🔇 Additional comments (3)
cmd/committee-api/service/committee_service_response.go (3)
329-331: LGTM! Email properly stored in sensitive struct.The Email field is now correctly stored in the
CommitteeMemberSensitivestruct, aligning with the PR's objective to separate sensitive attributes from base member data.
415-417: LGTM! Consistent Email handling in update path.The update conversion correctly mirrors the create path by storing Email in
CommitteeMemberSensitive.
497-497: No action needed. The code is correct.The
member.EmailbecauseCommitteeMemberSensitiveis embedded as an anonymous field inCommitteeMember, which enables Go's field promotion mechanism. The Email field from the embedded struct is automatically promoted to the parent struct level, making direct access valid.Likely an incorrect or invalid review comment.
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Reviewed with [GitHub Copilot](https://github.com/features/copilot) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932 Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/lfx-v2-committee-service/Chart.yaml (1)
8-8: Consider a minor version bump for API contract changes.The PR introduces significant changes to the API response structure (new
CommitteeMemberBasicWithReadonlyAttributestype that excludes email, dual-document indexing). A minor version bump (e.g.,0.3.0) would more clearly signal these API contract modifications compared to a patch increment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
charts/lfx-v2-committee-service/Chart.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
- Introduced a new GET endpoint `/committees/{uid}/members/{member_uid}/contact` to fetch contact details for a specific committee member.
- Added parameters for API version, committee UID, and member UID.
- Defined response schemas for successful and error responses, including 200, 400, 404, 500, and 503 status codes.
- Created a new schema `CommitteeMemberContactWithReadonlyAttributes` to structure the contact information response.
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-932
Assisted by [Claude Code](https://claude.ai/code)
Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
Overview
Jira ticket https://linuxfoundation.atlassian.net/browse/LFXV2-932
Building on the member visibility implementation from LFXV2-920, we need to implement attribute-level visibility control for user profiles. Specifically, we need the ability to hide certain profile attributes (like email) from being displayed based on policy logic.
Changes
Important Note
No dedicated endpoint was created to return sensitive data (email) for writers/auditors at this time. This will be evaluated in a follow-up to determine if a separate endpoint is necessary.
Implementation Notes
Data is kept in a single KV bucket rather than splitting into separate buckets (basic/sensitive). This approach provides:
The separation exists primarily at the model layer to enable different access control policies in the Indexer (OpenSearch), where sensitive fields can be restricted based on user permissions
Tests
1. Create Committee Member
Request:
Response:
Note: Create/Update operations return full member data (including email) for now.
2. Verify OpenSearch Indexing
Request:
Response:
Key Observations:
Two separate OpenSearch documents are created:
committee_member_sensitivedocument:uid,committee_uid,emailaccess_check_relation:auditor- Requires auditor/writer permissioncommittee_memberdocument:access_check_relation:viewer- Accessible to any committee viewer3. Delete Committee Member
Request:
Response:
4. Verify Deletion in OpenSearch
Request:
Response:
Key Observations:
committee_memberandcommittee_member_sensitivedocuments are marked as deleteddeleted_attimestamp setlatest: trueflag for audit purposesSummary
This implementation successfully separates sensitive committee member data (email) from basic information at both the API and indexing layers. The separation enables: