[LFXV2-1476] feat(fga): add granular committee member access — roster_viewer and email_viewer#79
Open
andrest50 wants to merge 12 commits into
Open
[LFXV2-1476] feat(fga): add granular committee member access — roster_viewer and email_viewer#79andrest50 wants to merge 12 commits into
andrest50 wants to merge 12 commits into
Conversation
…ittee access control - Add RelationRosterViewer, RelationEmailViewer, RelationCommitteeForMemberRosterAccess, MemberVisibilityBasicProfile, and MemberVisibilityHidden constants to pkg/constants/access_control.go - committee_writer.go: set roster_viewer: ["*"] on public committees; set committee_for_member_roster_access self-pointer when member_visibility = "basic_profile" - committee_member_writer.go: switch access_check_relation from "viewer" to constants.RelationRosterViewer; remove email from fulltext - Update docs/fga-contract.md and docs/indexer-contract.md to reflect the new relations and the email-exclusion rationale Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…ranular roster access
- Split committee member response: CommitteeMemberFullWithReadonlyAttributes no longer
includes email; email is now returned only via the new contact endpoint
- Add CommitteeMemberBasicBaseAttributes() Goa design function (all non-email fields)
- Add CommitteeMemberContactWithReadonlyAttributes Goa type (uid, committee_uid, email)
- Add get-committee-member-contact endpoint: GET /committees/{uid}/members/{member_uid}/contact
- Add GetCommitteeMemberContact service handler (reuses GetMember orchestrator)
- Add convertMemberDomainToContactResponse conversion (uid + committee_uid + email only)
- Update ruleset: committee_members:get now checks roster_viewer relation
- Add ruleset rule: committee_members:get_contact checks email_viewer relation
- Regenerate all gen/ files via make apigen
- Update tests to remove Email field from CommitteeMemberFullWithReadonlyAttributes assertions
Generated with [Claude Code](https://claude.ai/code)
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSplits member email into a new sensitive struct, adds GET /committees/{uid}/members/{member_uid}/contact, replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Committee API
participant FGA as OpenFGA
participant DB as Storage/Indexer
Client->>API: GET /committees/{uid}/members/{member_uid}/contact (Bearer)
API->>FGA: Check relation "email_viewer" on object committee:{uid}
alt FGA allowed
FGA-->>API: allow
API->>DB: Read member (includes CommitteeMemberSensitive.Email)
DB-->>API: Member with sensitive email
API-->>Client: 200 OK (uid, committee_uid, email)
else FGA denied
FGA-->>API: deny
API-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…s for roster_viewer Public committees now include roster_viewer: ["*"] in the FGA message. Update the two test cases with Public: true to include this relation. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns the committee-service API and indexing behavior with updated FGA relations by splitting committee member access into (1) roster visibility (non-sensitive profile/role info) and (2) email visibility via a new contact endpoint.
Changes:
- Removes
emailfrom the primary committee member “full” response and introducesGET /committees/{uid}/members/{member_uid}/contactfor email retrieval. - Adds new FGA relations/constants (
roster_viewer,email_viewer, and self-referentialcommittee_for_member_roster_access) and updates ruleset/docs accordingly. - Updates committee-member indexing configuration to use
roster_viewerand removes email from thefulltextfield.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/constants/subjects.go | Adds a new index subject constant for sensitive committee member indexing. |
| pkg/constants/access_control.go | Introduces new FGA relation constants and member_visibility constants. |
| internal/service/committee_writer.go | Updates committee access-control message construction to set roster_viewer for public committees and member-visibility references. |
| internal/service/committee_reader_test.go | Updates tests to move email into CommitteeMemberSensitive. |
| internal/service/committee_member_writer_test.go | Updates writer tests for the new sensitive email struct placement. |
| internal/service/committee_member_writer.go | Switches indexing access relation to roster_viewer and removes email from fulltext. |
| internal/infrastructure/mock/committee.go | Updates mock data to place email under CommitteeMemberSensitive. |
| internal/domain/model/committee_member_test.go | Adjusts validation/tags/index-key tests for sensitive email separation. |
| internal/domain/model/committee_member.go | Introduces CommitteeMemberSensitive and removes email from the base struct; removes email tags. |
| gen/http/openapi3.yaml | Updates OpenAPI v3 docs: removes email from roster response and adds contact endpoint/schema. |
| gen/http/openapi.yaml | Updates Swagger v2 YAML docs similarly (new contact endpoint + schema). |
| gen/http/openapi.json | Updates generated Swagger JSON (new endpoint and removal of email from roster response schema). |
| gen/http/committee_service/server/types.go | Updates server transport types to remove email from roster responses and add contact response types. |
| gen/http/committee_service/server/server.go | Mounts/initializes the new get-committee-member-contact handler. |
| gen/http/committee_service/server/paths.go | Adds path helper for the contact endpoint. |
| gen/http/committee_service/server/encode_decode.go | Adds encoder/decoder + error encoding for the contact endpoint. |
| gen/http/committee_service/client/types.go | Updates client transport types for removed email fields and new contact response types. |
| gen/http/committee_service/client/paths.go | Adds client path helper for the contact endpoint. |
| gen/http/committee_service/client/encode_decode.go | Adds client request builder/encoder/decoder for the contact endpoint. |
| gen/http/committee_service/client/client.go | Adds client endpoint method for get-committee-member-contact. |
| gen/http/committee_service/client/cli.go | Adds CLI payload builder for the contact endpoint. |
| gen/http/cli/committee/cli.go | Wires CLI parsing/usage for get-committee-member-contact. |
| gen/committee_service/service.go | Adds service interface + payload/result types for get-committee-member-contact; removes email from roster result type. |
| gen/committee_service/endpoints.go | Adds endpoint wiring for get-committee-member-contact. |
| gen/committee_service/client.go | Adds strongly-typed service client method for get-committee-member-contact. |
| docs/indexer-contract.md | Updates indexer contract docs for roster_viewer and removes email from fulltext description. |
| docs/fga-contract.md | Documents the new roster_viewer relation and self-referential member visibility reference. |
| cmd/committee-api/service/committee_service_test.go | Updates service tests to reflect email moving into the sensitive struct. |
| cmd/committee-api/service/committee_service_response_test.go | Updates response conversion tests for sensitive email separation. |
| cmd/committee-api/service/committee_service_response.go | Updates payload/domain conversions and adds contact-response conversion helper. |
| cmd/committee-api/service/committee_service.go | Adds GetCommitteeMemberContact endpoint implementation and updates member creation flows for sensitive email. |
| cmd/committee-api/design/type.go | Adds design types/attribute grouping for roster vs contact shapes. |
| cmd/committee-api/design/committee.go | Adds the new get-committee-member-contact endpoint to the Goa design. |
| charts/lfx-v2-committee-service/templates/ruleset.yaml | Updates ruleset: GET member uses roster_viewer; adds GET contact rule using email_viewer. |
💡 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
🧹 Nitpick comments (1)
internal/domain/model/committee_member_test.go (1)
410-482: Use theexpectedhashes or remove them.This table now carries
expectedvalues and “SHA-256 of …” comments, but the test never comparesresultagainst them. As written, it only proves “deterministic 64-char hex”, not that the key is actually derived fromcommittee_uid|email. Either assert the expected hash or compute it inline so this contract stays covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/domain/model/committee_member_test.go` around lines 410 - 482, The table includes expected SHA-256 strings but the test never asserts them; update the subtest for each case to assert that result == expected (or remove the unused expected fields). Specifically, in the loop where you call CommitteeMember.BuildIndexKey(ctx), add an assertion comparing the returned result to the test case's expected value (tt.expected) or, if you prefer, remove the expected field from the test cases; if you keep expected, ensure the strings are the actual SHA-256 hex of CommitteeMemberBase.CommitteeUID + "|" + CommitteeMemberSensitive.Email so the check verifies the intended contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/committee-api/service/committee_service.go`:
- Around line 295-301: The new GetCommitteeMemberContact handler currently lacks
unit tests to assert that GetMember (committeeReaderOrchestrator.GetMember)
returns a CommitteeMember with CommitteeMemberSensitive.Email and that the
handler returns that email via convertMemberDomainToContactResponse; add tests
for the GetCommitteeMemberContact endpoint that mock
committeeReaderOrchestrator.GetMember to return a CommitteeMember containing
CommitteeMemberSensitive.Email, verify the handler calls GetMember with ctx,
p.UID and p.MemberUID, and assert the HTTP/response payload contains the
expected email and proper error handling when GetMember returns an error.
In `@internal/service/committee_member_writer.go`:
- Around line 790-803: The roster-level indexing branch currently sets
indexerMessage.IndexingConfig and still publishes the full data.Member and
data.Member.Tags(), which can leak emails; update the roster_viewer branch (the
code that builds indexerMessage.IndexingConfig and assigns memberData) to
publish a sanitized member payload (or filtered Tags) that removes email-related
fields and tags before sending to the roster index, and ensure any email value
is only emitted to the separate sensitive index
(lfx.index.committee_member_sensitive) with AccessCheckRelation/email_viewer;
specifically modify the logic around indexerMessage.IndexingConfig, memberData,
and data.Member.Tags() so roster viewers never receive email-bearing fields
while keeping the sensitive email-only index with access_check_relation =
"email_viewer".
---
Nitpick comments:
In `@internal/domain/model/committee_member_test.go`:
- Around line 410-482: The table includes expected SHA-256 strings but the test
never asserts them; update the subtest for each case to assert that result ==
expected (or remove the unused expected fields). Specifically, in the loop where
you call CommitteeMember.BuildIndexKey(ctx), add an assertion comparing the
returned result to the test case's expected value (tt.expected) or, if you
prefer, remove the expected field from the test cases; if you keep expected,
ensure the strings are the actual SHA-256 hex of
CommitteeMemberBase.CommitteeUID + "|" + CommitteeMemberSensitive.Email so the
check verifies the intended contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca4b964c-fc6a-408d-a00f-0d2571201784
⛔ Files ignored due to path filters (17)
gen/committee_service/client.gois excluded by!**/gen/**gen/committee_service/endpoints.gois excluded by!**/gen/**gen/committee_service/service.gois excluded by!**/gen/**gen/http/cli/committee/cli.gois excluded by!**/gen/**gen/http/committee_service/client/cli.gois excluded by!**/gen/**gen/http/committee_service/client/client.gois excluded by!**/gen/**gen/http/committee_service/client/encode_decode.gois excluded by!**/gen/**gen/http/committee_service/client/paths.gois excluded by!**/gen/**gen/http/committee_service/client/types.gois excluded by!**/gen/**gen/http/committee_service/server/encode_decode.gois excluded by!**/gen/**gen/http/committee_service/server/paths.gois excluded by!**/gen/**gen/http/committee_service/server/server.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 (18)
charts/lfx-v2-committee-service/templates/ruleset.yamlcmd/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.godocs/fga-contract.mddocs/indexer-contract.mdinternal/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.gointernal/service/committee_writer.gopkg/constants/access_control.gopkg/constants/subjects.go
…tact endpoint tests - Prevent email leak in roster index: use CommitteeMemberBase (no email) for the lfx.index.committee_member payload; publish a second sensitive indexer message to lfx.index.committee_member_sensitive gated by email_viewer - Fix TestCommitteeMember_BuildIndexKey: replace placeholder expected strings with real SHA-256 hashes and add result assertions - Add TestGetCommitteeMemberContact: covers happy-path email return, not-found propagation, and unexpected-error propagation; verifies GetMember is called with correct committeeUID and memberUID Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
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)
821-889:⚠️ Potential issue | 🟠 MajorEmit a delete to the sensitive index too.
lfx.index.committee_member_sensitiveis a separate subject, but this path only builds/publishes it for create/update. On member delete, the roster document is removed while the last email document can remain indexed underemail_viewer.Suggested fix
- var sensitiveIndexerMessageBuild *model.CommitteeIndexerMessage - if action == model.ActionCreated || action == model.ActionUpdated { + var sensitiveIndexerMessageBuild *model.CommitteeIndexerMessage + if action == model.ActionCreated || action == model.ActionUpdated || action == model.ActionDeleted { sensitiveIndexerMessage := model.CommitteeIndexerMessage{ Action: action, IndexingConfig: &indexerTypes.IndexingConfig{ ObjectID: data.Member.UID, AccessCheckObject: fmt.Sprintf("committee:%s", data.Member.CommitteeUID), AccessCheckRelation: constants.RelationEmailViewer, ParentRefs: []string{fmt.Sprintf("committee:%s", data.Member.CommitteeUID)}, }, } - sensitivePayload := struct { - UID string `json:"uid"` - CommitteeUID string `json:"committee_uid"` - Email string `json:"email"` - }{ - UID: data.Member.UID, - CommitteeUID: data.Member.CommitteeUID, - Email: data.Member.Email, - } + var sensitivePayload any = data.Member.UID + if action != model.ActionDeleted { + sensitivePayload = struct { + UID string `json:"uid"` + CommitteeUID string `json:"committee_uid"` + Email string `json:"email"` + }{ + UID: data.Member.UID, + CommitteeUID: data.Member.CommitteeUID, + Email: data.Member.Email, + } + } built, errBuildSensitive := sensitiveIndexerMessage.Build(ctx, sensitivePayload) if errBuildSensitive != nil { slog.ErrorContext(ctx, "failed to build sensitive member indexer message", "error", errBuildSensitive, "action", action, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/committee_member_writer.go` around lines 821 - 889, The sensitive indexer message is only built for model.ActionCreated and model.ActionUpdated, so deletes never emit to the sensitive subject; expand the build condition to include model.ActionDeleted (i.e., change the if that creates sensitiveIndexerMessage/sensitivePayload to run for ActionCreated || ActionUpdated || ActionDeleted) and ensure the sensitivePayload still includes UID and CommitteeUID (and Email if available, or empty string) so that sensitiveIndexerMessageBuild is non-nil for deletes and will be published by the existing publisher goroutine. Use the same symbols shown (sensitiveIndexerMessage, sensitivePayload, sensitiveIndexerMessageBuild, action, model.ActionDeleted) to locate and update the code.
🧹 Nitpick comments (2)
cmd/committee-api/service/committee_service_test.go (1)
1603-1611: Assert concrete GOA error types in failure-path contact tests.The cases named “propagates not-found/unexpected” currently only check
require.Error(Line 1657). Addrequire.ErrorAsto lock in*committeeservice.NotFoundErrorand*committeeservice.InternalServerErrormapping, otherwise a wrong error translation could still pass.💡 Tighten failure assertions
func TestGetCommitteeMemberContact(t *testing.T) { tests := []struct { name string payload *committeeservice.GetCommitteeMemberContactPayload setupMock func(*mockCommitteeReaderOrchestrator) expectError bool + validateError func(*testing.T, error) validateResult func(*testing.T, *committeeservice.CommitteeMemberContactWithReadonlyAttributes) validateCalls func(*testing.T, []getMemberCall) }{ ... { name: "propagates not-found error from GetMember", ... expectError: true, + validateError: func(t *testing.T, err error) { + var nfErr *committeeservice.NotFoundError + require.ErrorAs(t, err, &nfErr) + }, validateResult: func(t *testing.T, res *committeeservice.CommitteeMemberContactWithReadonlyAttributes) { assert.Nil(t, res) }, ... { name: "propagates unexpected error from GetMember", ... expectError: true, + validateError: func(t *testing.T, err error) { + var internalErr *committeeservice.InternalServerError + require.ErrorAs(t, err, &internalErr) + }, validateResult: func(t *testing.T, res *committeeservice.CommitteeMemberContactWithReadonlyAttributes) { assert.Nil(t, res) }, ... if tt.expectError { require.Error(t, err) + if tt.validateError != nil { + tt.validateError(t, err) + } } else { require.NoError(t, err) }Also applies to: 1622-1630, 1657-1661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/committee-api/service/committee_service_test.go` around lines 1603 - 1611, The failing-path tests for GetCommitteeMemberContact (cases like "propagates not-found error from GetMember" that set mockCommitteeReaderOrchestrator.getMemberErr) only call require.Error; update those cases to also assert the concrete Goa error type using require.ErrorAs (e.g. require.ErrorAs(t, err, new(*committeeservice.NotFoundError)) for not-found cases and require.ErrorAs(t, err, new(*committeeservice.InternalServerError)) for unexpected/internal cases) so the test locks in the exact translation of errors when invoking the handler for GetCommitteeMemberContactPayload.internal/domain/model/committee_member_test.go (1)
415-425: Add one normalization case forBuildIndexKey().
BuildIndexKey()trims and lowercases bothCommitteeUIDandinternal/domain/model/committee_member.go:80-88), but every case here is already normalized. Adding a" Committee-123 "/" Test@Example.com "case that expects the same digest as the basic member would lock in the dedupe contract and catch casing/whitespace regressions.Suggested test case
{ name: "basic member", member: &CommitteeMember{ CommitteeMemberBase: CommitteeMemberBase{ CommitteeUID: "committee-123", }, CommitteeMemberSensitive: CommitteeMemberSensitive{Email: "test@example.com"}, }, // SHA-256 of "committee-123|test@example.com" expected: "93548eeb4f04488dfe77d98d56f0642fff5e1c9637314866d07e9f289cc4343a", }, + { + name: "normalizes committee uid and email", + member: &CommitteeMember{ + CommitteeMemberBase: CommitteeMemberBase{ + CommitteeUID: " Committee-123 ", + }, + CommitteeMemberSensitive: CommitteeMemberSensitive{Email: " Test@Example.com "}, + }, + // Normalizes to "committee-123|test@example.com" + expected: "93548eeb4f04488dfe77d98d56f0642fff5e1c9637314866d07e9f289cc4343a", + },Also applies to: 427-447, 449-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/domain/model/committee_member_test.go` around lines 415 - 425, Add a normalization test case for BuildIndexKey(): create a new CommitteeMember instance with CommitteeMemberBase.CommitteeUID set to " Committee-123 " and CommitteeMemberSensitive.Email set to " Test@Example.com " and assert that BuildIndexKey() returns the same digest as the existing basic member ("93548eeb4f04488dfe77d98d56f0642fff5e1c9637314866d07e9f289cc4343a"); this verifies the trimming/lowercasing logic in BuildIndexKey() and should be added alongside the other cases that reference CommitteeMember, CommitteeMemberBase, and CommitteeMemberSensitive so it fails if normalization regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/service/committee_member_writer.go`:
- Around line 821-889: The sensitive indexer message is only built for
model.ActionCreated and model.ActionUpdated, so deletes never emit to the
sensitive subject; expand the build condition to include model.ActionDeleted
(i.e., change the if that creates sensitiveIndexerMessage/sensitivePayload to
run for ActionCreated || ActionUpdated || ActionDeleted) and ensure the
sensitivePayload still includes UID and CommitteeUID (and Email if available, or
empty string) so that sensitiveIndexerMessageBuild is non-nil for deletes and
will be published by the existing publisher goroutine. Use the same symbols
shown (sensitiveIndexerMessage, sensitivePayload, sensitiveIndexerMessageBuild,
action, model.ActionDeleted) to locate and update the code.
---
Nitpick comments:
In `@cmd/committee-api/service/committee_service_test.go`:
- Around line 1603-1611: The failing-path tests for GetCommitteeMemberContact
(cases like "propagates not-found error from GetMember" that set
mockCommitteeReaderOrchestrator.getMemberErr) only call require.Error; update
those cases to also assert the concrete Goa error type using require.ErrorAs
(e.g. require.ErrorAs(t, err, new(*committeeservice.NotFoundError)) for
not-found cases and require.ErrorAs(t, err,
new(*committeeservice.InternalServerError)) for unexpected/internal cases) so
the test locks in the exact translation of errors when invoking the handler for
GetCommitteeMemberContactPayload.
In `@internal/domain/model/committee_member_test.go`:
- Around line 415-425: Add a normalization test case for BuildIndexKey(): create
a new CommitteeMember instance with CommitteeMemberBase.CommitteeUID set to "
Committee-123 " and CommitteeMemberSensitive.Email set to " Test@Example.com "
and assert that BuildIndexKey() returns the same digest as the existing basic
member ("93548eeb4f04488dfe77d98d56f0642fff5e1c9637314866d07e9f289cc4343a");
this verifies the trimming/lowercasing logic in BuildIndexKey() and should be
added alongside the other cases that reference CommitteeMember,
CommitteeMemberBase, and CommitteeMemberSensitive so it fails if normalization
regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce450a0f-4e5f-485b-b608-ebc38bd94e9d
📒 Files selected for processing (3)
cmd/committee-api/service/committee_service_test.gointernal/domain/model/committee_member_test.gointernal/service/committee_member_writer.go
… access Splits member_visibility into three tiers: - hidden: no member-to-member visibility (default) - basic_profile: members can view each other's names & roles (roster only) - full_profile: members can view each other's names, roles, and email addresses Adds RelationCommitteeForMemberEmailAccess constant and wires it into buildAccessControlMessage so full_profile sets both the roster and email self-referential FGA relations. Adds test coverage for both new cases. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…cessControlMessage Adds a test case verifying that member_visibility=hidden leaves both committee_for_member_roster_access and committee_for_member_email_access unset, so members cannot see each other at all. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/committee_writer_test.go (1)
641-695: Consider adding an explicithiddenvisibility test case.You already cover default/no-visibility paths, but an explicit
MemberVisibilityHiddencase would make this contract clearer and more regression-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/committee_writer_test.go` around lines 641 - 695, Add an explicit test case in committee_writer_test.go that covers MemberVisibilityHidden: create a model.Committee with CommitteeSettings.MemberVisibility set to constants.MemberVisibilityHidden (e.g., UID "committee-hidden", ProjectUID "project-hidden") and assert the produced fgatypes.GenericFGAMessage (GenericAccessData) contains only the project reference (no "committee_for_member_roster_access" or "committee_for_member_email_access") and still includes ExcludeRelations: []string{"member"}; mirror the structure of the existing "basic_profile" / "full_profile" cases and name it something like "hidden_profile sets no member access" so the behavior is explicit and regression-tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/committee-api/design/type.go`:
- Around line 747-750: Fix the spelling typo in the description string for the
member_visibility attribute: change "Dertermines the visibility level of members
profiles to other members of the same committee" to "Determines the visibility
level of members' profiles to other members of the same committee". Update the
description in the MemberVisibilityAttribute function (the dsl.Attribute call
for "member_visibility") and while editing also add the missing apostrophe in
"members' profiles" for correct possessive form.
---
Nitpick comments:
In `@internal/service/committee_writer_test.go`:
- Around line 641-695: Add an explicit test case in committee_writer_test.go
that covers MemberVisibilityHidden: create a model.Committee with
CommitteeSettings.MemberVisibility set to constants.MemberVisibilityHidden
(e.g., UID "committee-hidden", ProjectUID "project-hidden") and assert the
produced fgatypes.GenericFGAMessage (GenericAccessData) contains only the
project reference (no "committee_for_member_roster_access" or
"committee_for_member_email_access") and still includes ExcludeRelations:
[]string{"member"}; mirror the structure of the existing "basic_profile" /
"full_profile" cases and name it something like "hidden_profile sets no member
access" so the behavior is explicit and regression-tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c5feaaa-436e-47f5-93e3-7e36365336d4
⛔ Files ignored due to path filters (7)
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 (4)
cmd/committee-api/design/type.gointernal/service/committee_writer.gointernal/service/committee_writer_test.gopkg/constants/access_control.go
✅ Files skipped from review due to trivial changes (1)
- pkg/constants/access_control.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/committee_writer.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 36 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix typo in member_visibility attribute description (Dertermines → Determines) - Update fga-contract.md to document committee_for_member_email_access separately from committee_for_member_roster_access with correct conditions per visibility tier - Publish delete event to sensitive index subject on member deletion so email documents are not left accessible to email_viewer after removal Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…lds to sensitive index
- Remove email from committee_member roster index (CommitteeMemberBase has no email field)
- Add missing committee_member_sensitive section documenting the email-only index
- Add fulltext, name_and_aliases, sort_name, tags, and parent_refs to sensitive IndexingConfig
- Set history_check_object/relation (committee:{uid} / auditor) on sensitive index
Generated with [Claude Code](https://claude.ai/claude-code)
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 36 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add explicit case for MemberVisibilityHidden and "" in buildAccessControlMessage so the "no self-referential references" behavior is documented intent rather than implicit switch fall-through. Pre-feature records stored as "" are treated identically to "hidden", preventing any silent behavior change if a default case is added in the future. Generated with [Claude Code](https://claude.ai/claude-code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…s and add Version to test payloads - committee_member_writer.go: extract sensitiveMemberTags variable and set both CommitteeIndexerMessage.Tags and IndexingConfig.Tags from the same slice, ensuring the top-level Tags field is not left empty on the sensitive index message - committee_service_test.go: add Version: "1" to all three GetCommitteeMemberContactPayload test instances in TestGetCommitteeMemberContact Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 36 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
425
to
439
| @@ -437,26 +438,52 @@ func CommitteeMemberBaseAttributes() { | |||
| OrganizationInfoAttributes() | |||
| } | |||
There was a problem hiding this comment.
CommitteeMemberBaseAttributes() is still used for update payloads and includes EmailAttribute() (and the update method requires email). Since the roster-view member endpoints no longer return email, clients that can update a member but don’t have email_viewer may have no way to obtain the current email to satisfy PUT “complete replacement” semantics. Consider making email optional for updates (use stored email when omitted), switching to PATCH semantics for non-email updates, or explicitly ensuring the update permission implies email_viewer so callers can fetch /contact first.
Re-publishes all committee members into the new two-index structure (roster_viewer + email_viewer) introduced by the committee_member index split. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
This PR splits committee member data access into two distinct scopes, aligning the API with the FGA model changes in
lfx-v2-helm:roster_viewer— can see member names, roles, voting status, and org infoemail_viewer— can see email addresses (gated separately)Previously
GET /committees/{uid}/members/{member_uid}returned everything including email, and was checked against a singleviewerrelation. Now email lives behind its own endpoint and its own FGA relation.Response type split
GET /committees/{uid}/members/{member_uid}→ checked againstroster_viewerReturns profile and role data — no email:
{ "uid": "...", "committee_uid": "...", "committee_name": "...", "committee_category": "...", "username": "...", "first_name": "...", "last_name": "...", "job_title": "...", "linkedin_profile": "...", "appointed_by": "...", "status": "...", "role": { "name": "...", "start_date": "...", "end_date": "..." }, "voting": { "status": "...", "start_date": "...", "end_date": "..." }, "organization": { "id": "...", "name": "...", "website": "..." }, "created_at": "...", "updated_at": "..." }GET /committees/{uid}/members/{member_uid}/contact→ checked againstemail_viewer(new)Returns only the identifiers needed to look up contact info:
{ "uid": "...", "committee_uid": "...", "email": "user@example.com" }Per-committee member visibility (
member_visibility)A new
member_visibilitysetting onCommitteeSettingscontrols whether committee members can see each other, without affecting what staff/auditors can see:hidden(default)roster_viewernoremail_vieweris setbasic_profilecommittee_for_member_roster_accessto the committee itselffull_profilecommittee_for_member_roster_accessandcommittee_for_member_email_accessto the committee itselfThese are self-referential FGA references (analogous to
vote_for_participant_result_access): when set, the FGA model grants allmembers of the committee the corresponding viewer relation on each other.OpenSearch indexing
A new index subject
lfx.index.committee_member_sensitivehas been added alongside the existinglfx.index.committee_member. This allows the indexer to store email in a separate document type gated by theemail_viewerrelation, keeping sensitive fields out of the general member index.On member deletion, a delete event is now published to both index subjects so that the email document is removed from the sensitive index and does not remain accessible.
Ruleset changes
Ticket
LFXV2-1476
🤖 Generated with Claude Code