[LFXV2-920] Policy Abstraction for Fine-Grained Authorization #24
Conversation
…synchronization Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920 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-920 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds fine-grained policy support: a Policy domain model and tests, a PolicyHandler service and tests, a RelationshipSynchronizer interface, and committee handler integration that stores Policies on committeeStub and evaluates each policy during access sync. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CommitteeHandler as "Committee Handler"
participant PolicyHandler as "Policy Handler"
participant Sync as "Relationship Synchronizer"
participant FGA as "FGA Service"
Client->>CommitteeHandler: Update Committee Access
CommitteeHandler->>Sync: SyncObjectTuples(..., "member")
Sync->>FGA: Write/Delete Tuples
FGA-->>Sync: OK
Sync-->>CommitteeHandler: OK
rect rgba(230,245,255,1)
Note over CommitteeHandler,PolicyHandler: For each policy in committee.Policies
CommitteeHandler->>PolicyHandler: EvaluatePolicy(ctx, policy, objectID, userRel)
PolicyHandler->>PolicyHandler: Validate & build tuple keys
alt Object→Policy link
PolicyHandler->>Sync: ReadObjectTuples(objectID)
Sync->>FGA: Read Tuples
FGA-->>Sync: Tuples
Sync-->>PolicyHandler: Tuples
PolicyHandler->>Sync: WriteAndDeleteTuples(writes, deletes)
Sync->>FGA: Write/Delete Tuples
FGA-->>Sync: OK
end
alt Policy→User relation link
PolicyHandler->>Sync: ReadObjectTuples(policy.ObjectID())
Sync->>FGA: Read Tuples
FGA-->>Sync: Tuples
Sync-->>PolicyHandler: Tuples
PolicyHandler->>Sync: WriteAndDeleteTuples(writes, deletes)
Sync->>FGA: Write/Delete Tuples
FGA-->>Sync: OK
end
PolicyHandler-->>CommitteeHandler: Success / Error
end
CommitteeHandler-->>Client: Update Complete / Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/domain/policy.go (1)
37-42: Consider makingUserRelationa standalone function.This method doesn't reference any fields of the
Policyreceiver—it's purely a utility that formatsobjectID#relation. Attaching it toPolicymay mislead readers into thinking it uses policy state.🔎 Alternative: standalone function
// FormatUserRelation returns the user relation string for a given object. // Format: objectID#relation // Example: "committee:123#member" func FormatUserRelation(objectID, relation string) string { return fmt.Sprintf("%s#%s", objectID, relation) }handler_committee.go (2)
85-98: Consider reusingPolicyHandlerinstance at handler service level.Creating a new
PolicyHandlerfor each message with policies adds minor overhead. SincePolicyHandleris stateless (only holds logger and synchronizer references), it could be instantiated once inHandlerServiceand reused.🔎 Suggested approach
type HandlerService struct { fgaService FgaService + policyHandler service.PolicyHandler // ... other fields } // In constructor or initialization: +h.policyHandler = service.NewPolicyHandler(logger, h.fgaService) // In committeeUpdateAccessHandler: -policyEval := service.NewPolicyHandler(logger, h.fgaService) for _, policy := range committee.Policies { - err = policyEval.EvaluatePolicy(ctx, policy, object) + err = h.policyHandler.EvaluatePolicy(ctx, policy, object)
85-98: Policy evaluation errors should be logged with additional context.When policy evaluation fails, the log includes
policyandobject, but consider also logging the committee UID for easier debugging and correlation with other log entries.🔎 Enhanced logging
err = policyEval.EvaluatePolicy(ctx, policy, object) if err != nil { - logger.With(errKey, err, "policy", policy, "object", object).ErrorContext(ctx, "failed to evaluate policy") + logger.With(errKey, err, "policy", policy, "object", object, "committeeUID", committee.UID).ErrorContext(ctx, "failed to evaluate policy") return err }internal/service/policy_handler_test.go (1)
329-337: Consider usingerrors.Isor checking for wrapped error.The exact error message comparison (
"failed to write policy tuples: write error") is fragile if the message format changes. Usingerrors.Isorerrors.Asprovides more robust error checking.🔎 More robust error check
-if !errors.Is(err, expectedError) && err.Error() != "failed to write policy tuples: write error" { - t.Errorf("Expected write error, got: %v", err) -} +if !errors.Is(err, expectedError) { + // Check if the error wraps our expected error + var unwrapped error + if errors.As(err, &unwrapped) { + if !errors.Is(unwrapped, expectedError) { + t.Errorf("Expected write error, got: %v", err) + } + } else { + t.Errorf("Expected write error, got: %v", err) + } +}Alternatively, if the production code wraps errors with
fmt.Errorf("...: %w", err), simply checkingerrors.Is(err, expectedError)should suffice.internal/service/policy_handler.go (1)
116-116: Hardcoded"member"relation limits reusability.The
"member"relation is hardcoded, but other policy types may need different user relations (e.g.,"viewer","admin"). Consider making this configurable via thePolicystruct or as a parameter.🔎 Suggested approach
Option 1: Add a
UserRelationTypefield toPolicy:type Policy struct { Name string `json:"name"` Relation string `json:"relation"` Value string `json:"value"` UserRelationType string `json:"user_relation_type"` // e.g., "member", "viewer" }Option 2: Pass the relation type as a parameter to
EvaluatePolicy:EvaluatePolicy(ctx context.Context, policy domain.Policy, objectID string, userRelationType string) errorFor now, adding a comment documenting the "member" assumption would help:
// Default to "member" relation for committee visibility policies. // TODO: Make configurable when supporting other policy types. userRelation := policy.UserRelation(objectID, "member")
📜 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 (6)
handler_committee.gointernal/domain/policy.gointernal/domain/policy_test.gointernal/service/policy_handler.gointernal/service/policy_handler_test.gointernal/service/relationship_sync_service.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use handler functions dedicated to each message type (accessCheckHandler, projectUpdateHandler, projectDeleteHandler, committeeUpdateAccessHandler, etc.) following the handler pattern architecture
Implement cache strategy using Base32-encoded relation tuples as cache keys (e.g.,rel.{encoded-relation}) and JSON values withallowedboolean andcreated_attimestamp
Use timestamp-based cache invalidation with configurable staleness tolerance and implement fallback to direct OpenFGA queries on cache miss
Implement structured logging with context, graceful degradation on cache miss, message replies with error details, and ensure service continues running on individual message failures
Use preallocated slices to reduce garbage collection, batch OpenFGA operations up to 100 tuples per request, employ cache-first approach, and use efficient string parsing withbytes.Cut
Implement health endpoints for Kubernetes-ready liveness and readiness probes at/livezand/readyzpaths
Expose expvar metrics at/debug/varsendpoint including cache hits, misses, and stale hits for monitoring
Use structured JSON logging for observability across the service
Files:
internal/domain/policy_test.gointernal/domain/policy.gointernal/service/relationship_sync_service.gohandler_committee.gointernal/service/policy_handler.gointernal/service/policy_handler_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Write test cases in
*_test.gofiles containing unit tests for each handler, with full test structure including integration tests using docker-compose.test.yml
Files:
internal/domain/policy_test.gointernal/service/policy_handler_test.go
🧬 Code graph analysis (4)
internal/domain/policy_test.go (1)
internal/domain/policy.go (1)
Policy(9-13)
handler_committee.go (2)
internal/domain/policy.go (1)
Policy(9-13)internal/service/policy_handler.go (1)
NewPolicyHandler(148-153)
internal/service/policy_handler.go (2)
internal/domain/policy.go (1)
Policy(9-13)internal/service/relationship_sync_service.go (1)
RelationshipSynchronizer(18-23)
internal/service/policy_handler_test.go (2)
internal/service/policy_handler.go (2)
NewPolicyHandler(148-153)PolicyHandler(16-18)internal/domain/policy.go (1)
Policy(9-13)
⏰ 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: Agent
🔇 Additional comments (13)
internal/domain/policy.go (2)
8-13: LGTM!Clean domain model with appropriate JSON tags for serialization. The struct captures the essential policy attributes needed for OpenFGA relationship construction.
15-28: LGTM!Validation logic is clear and returns specific error messages for each missing field. The validation order (Name → Value → Relation) is consistent with field declaration order.
internal/domain/policy_test.go (4)
10-80: LGTM!Comprehensive table-driven tests for
Validate()covering all field combinations. Error message assertions ensure consistent user-facing errors.
82-129: LGTM!Good coverage of
ObjectID()including edge cases with empty values and special characters. The ":" output for empty values documents current behavior even if not recommended for production use.
131-183: LGTM!Tests verify
UserRelation()formatting correctly. Edge case tests for empty objectID and empty relation document the method's behavior with invalid inputs.
185-212: LGTM!Integration test validates the typical usage flow: validate → get object ID → get user relation. Uses realistic UUID format for committee ID.
internal/service/relationship_sync_service.go (1)
13-23: LGTM - pragmatic interface design with documented trade-off.The interface successfully decouples
PolicyHandlerfromFgaServicewhile acknowledging the SDK type coupling. The inline comment (line 17) documenting the anti-pattern and intent to refactor is good practice for future maintainability.internal/service/policy_handler_test.go (3)
23-66: LGTM!Well-designed mock with call tracking, error injection, and captured write/delete tuples. This enables thorough verification of
PolicyHandlerbehavior.
140-198: LGTM!Thorough verification of the happy path: validates tuple structure, write count, delete count, and
ReadObjectTuplescall count. The expected tuple assertions ensure the two-level relationship structure is correctly formed.
408-418: LGTM!Constructor test verifies non-nil return and interface compliance via compile-time check.
internal/service/policy_handler.go (2)
25-47: Excellent documentation!The detailed ASCII diagram and examples clearly explain the two-level relationship structure. This significantly aids maintainability and onboarding for developers unfamiliar with the OpenFGA tuple model.
58-93: LGTM on idempotent tuple sync logic.The
checkTuplehelper correctly:
- Identifies exact matches to skip redundant writes
- Marks conflicting tuples (same user, different relation) for deletion
- Prepares new tuples only when needed
This ensures idempotent behavior on repeated calls.
handler_committee.go (1)
78-83: The new"member"parameter inSyncObjectTuplesis correctly used and all callers are consistent.The function signature supports variadic
excludeRelations ...string, allowing zero or more relation names to be passed as arguments. The call in handler_committee.go with"member"as the fourth argument is valid, and other handlers already use this pattern with 0-3 excludeRelations arguments (e.g., handler_past_meeting.go passes 3 relations:RelationOrganizer, RelationHost, RelationInvitee; handler_meeting.go passes 2:RelationParticipant, RelationHost). All callers are properly aligned with the function signature.
There was a problem hiding this comment.
Pull request overview
This PR introduces a clean architecture foundation for fine-grained authorization policy evaluation in the FGA sync service. It establishes a reusable abstraction layer that enables policy-based access control through a two-level relationship structure in OpenFGA, starting with committee member visibility policies.
Key Changes:
- Introduces a domain-driven
Policyentity with validation and helper methods for constructing OpenFGA tuple identifiers - Creates a
PolicyHandlerservice interface with clean separation from OpenFGA SDK dependencies via theRelationshipSynchronizerinterface - Integrates policy evaluation into the committee handler, allowing committees to specify policies that control member visibility
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/domain/policy.go | Defines the Policy domain entity with validation and helper methods for OpenFGA tuple construction |
| internal/domain/policy_test.go | Comprehensive unit tests for the Policy domain entity covering validation, object ID generation, and user relation formatting |
| internal/service/relationship_sync_service.go | Defines the RelationshipSynchronizer interface to decouple policy logic from OpenFGA SDK implementation |
| internal/service/policy_handler.go | Implements the PolicyHandler service that evaluates policies and creates two-level tuple relationships in OpenFGA |
| internal/service/policy_handler_test.go | Comprehensive unit tests for PolicyHandler covering validation, tuple management, conflict resolution, and error scenarios |
| handler_committee.go | Updates committee handler to support policy evaluation through the new policies field and excludes member relations from standard sync |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920 Reviewed with [GitHub Copilot](https://github.com/features/copilot) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…ttee policies Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920 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: 0
🧹 Nitpick comments (2)
handler_committee.go (1)
85-96: Consider caching PolicyHandler instance for efficiency.Creating a new
PolicyHandleron every committee update adds unnecessary allocation overhead. SincePolicyHandlerhas no mutable state, consider creating it once (e.g., as a field inHandlerService) and reusing it across invocations.🔎 Example refactor to reuse PolicyHandler
Add a field to HandlerService:
type HandlerService struct { fgaService *FGAService policyHandler service.PolicyHandler // ... other fields }Initialize it once during service construction:
func NewHandlerService(fgaService *FGAService, logger *slog.Logger) *HandlerService { return &HandlerService{ fgaService: fgaService, policyHandler: service.NewPolicyHandler(logger, fgaService), // ... other fields } }Then reuse it here:
if len(committee.Policies) > 0 { - policyEval := service.NewPolicyHandler(logger, h.fgaService) - // Evaluate each policy associated with the committee for _, policy := range committee.Policies { - errEvaluatePolicy := policyEval.EvaluatePolicy(ctx, policy, object, "member") + errEvaluatePolicy := h.policyHandler.EvaluatePolicy(ctx, policy, object, "member") if errEvaluatePolicy != nil { logger.With(errKey, errEvaluatePolicy, "policy", policy, "object", object).ErrorContext(ctx, "failed to evaluate policy") return errEvaluatePolicy } } }internal/service/policy_handler.go (1)
96-146: LGTM! Correct two-level policy relationship implementation.The policy evaluation correctly implements the two-level relationship structure described in the documentation:
- Object → Policy link
- Policy → User Relation link
The conditional write optimization (lines 133-143) ensures tuples are only written when changes are needed.
Minor: Misleading comment at line 117.
The comment says "Default to 'member' relation" but
userObjectRelationis passed as a parameter without any defaulting logic. Consider updating the comment for clarity:- // Format: policy.Name:policy.Value -> policy.Relation -> objectID#userRelation - // Example: visibility_policy:basic_profile#allows_basic_profile@committee:C#member - userRelation := policy.UserRelation(objectID, userObjectRelation) // Default to "member" relation + // Format: policy.Name:policy.Value -> policy.Relation -> objectID#userObjectRelation + // Example: visibility_policy:basic_profile#allows_basic_profile@committee:C#member + userRelation := policy.UserRelation(objectID, userObjectRelation)
📜 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 (3)
handler_committee.gointernal/service/policy_handler.gointernal/service/policy_handler_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use handler functions dedicated to each message type (accessCheckHandler, projectUpdateHandler, projectDeleteHandler, committeeUpdateAccessHandler, etc.) following the handler pattern architecture
Implement cache strategy using Base32-encoded relation tuples as cache keys (e.g.,rel.{encoded-relation}) and JSON values withallowedboolean andcreated_attimestamp
Use timestamp-based cache invalidation with configurable staleness tolerance and implement fallback to direct OpenFGA queries on cache miss
Implement structured logging with context, graceful degradation on cache miss, message replies with error details, and ensure service continues running on individual message failures
Use preallocated slices to reduce garbage collection, batch OpenFGA operations up to 100 tuples per request, employ cache-first approach, and use efficient string parsing withbytes.Cut
Implement health endpoints for Kubernetes-ready liveness and readiness probes at/livezand/readyzpaths
Expose expvar metrics at/debug/varsendpoint including cache hits, misses, and stale hits for monitoring
Use structured JSON logging for observability across the service
Files:
handler_committee.gointernal/service/policy_handler_test.gointernal/service/policy_handler.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Write test cases in
*_test.gofiles containing unit tests for each handler, with full test structure including integration tests using docker-compose.test.yml
Files:
internal/service/policy_handler_test.go
🧬 Code graph analysis (3)
handler_committee.go (2)
internal/domain/policy.go (1)
Policy(9-13)internal/service/policy_handler.go (1)
NewPolicyHandler(149-154)
internal/service/policy_handler_test.go (2)
internal/service/policy_handler.go (2)
NewPolicyHandler(149-154)PolicyHandler(16-18)internal/domain/policy.go (1)
Policy(9-13)
internal/service/policy_handler.go (2)
internal/domain/policy.go (1)
Policy(9-13)internal/service/relationship_sync_service.go (1)
RelationshipSynchronizer(18-27)
🔇 Additional comments (7)
handler_committee.go (2)
13-14: LGTM! Imports support policy evaluation.The new imports are necessary for the Policy domain type and PolicyHandler service integration.
25-25: LGTM! Policies field enables policy-based authorization.The field addition properly supports the policy abstraction architecture described in the PR objectives.
internal/service/policy_handler_test.go (1)
1-418: Excellent test coverage for PolicyHandler!The test suite is comprehensive and well-structured:
- Thorough validation error coverage (empty name, value, relation, objectID)
- Happy path verification with no existing tuples
- Idempotency check with existing tuples
- Conflict resolution with tuple deletions
- Error propagation for both read and write operations
- Multi-policy scenarios validating reusability
- Constructor and interface compliance verification
The mock implementation properly tracks calls, writes, deletes, and simulates errors, enabling thorough behavior validation without OpenFGA dependencies.
internal/service/policy_handler.go (4)
15-23: LGTM! Clean interface design.The
PolicyHandlerinterface provides a focused API for policy evaluation, and thepolicyHandlerstruct maintains minimal, immutable state, making it safe for concurrent use and reuse across multiple invocations.
48-56: LGTM! Proper validation before processing.The validation logic correctly delegates to the domain model's validation method and explicitly checks for required parameters before proceeding with policy evaluation.
58-94: LGTM! Idempotent tuple management.The
checkTuplehelper correctly implements idempotent tuple synchronization:
- Detects exact matches and avoids redundant writes
- Identifies conflicts (same user, different relation) and marks them for deletion
- Prepares new tuples only when needed
The logic properly handles multiple conflicting tuples by adding all of them to the deletion list.
148-154: LGTM! Clean constructor following dependency injection.The constructor properly follows the dependency injection pattern and returns the interface type, which enables testing and maintains good separation of concerns.
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…ittee access handler Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920 Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…bility Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920 Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920 Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
2 participants
Overview
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-920
This PR introduces a clean architecture foundation for fine-grained authorization policy evaluation. It creates a reusable abstraction layer for policy handling, starting with committee member visibility but designed to be extended to other policies with minimal adjustments.
Key Changes
Policydomain entity with validation and helper methodsPolicyHandlerinterface with clean separation of concernsRelationshipSynchronizerinterface to decouple policy logic from OpenFGA SDKpoliciesfieldArchitecture
Two-Level Policy Relationship Structure
The implementation creates a flexible two-level relationship structure for policy evaluation:
This structure allows OpenFGA to evaluate complex policies by traversing relationships:
Clean Architecture Benefits
Future Improvements
As noted in code comments, the
RelationshipSynchronizerinterface is currently tightly coupled to the OpenFGA client package. This is intentional for this first iteration, but we can gradually refactor to:client.ClientTupleKeyMigration Path
This PR establishes the foundation. Existing handlers continue to work unchanged. We can now: