Make expanded grants immutable. #278
Conversation
WalkthroughThe pull request introduces a modification to the grant creation process in the Changes
Sequence DiagramsequenceDiagram
participant Syncer
participant Entitlement
participant Grant
participant Annotations
Syncer->>Entitlement: Expand group membership
Syncer->>Grant: Create new grant
Syncer->>Annotations: Add GrantImmutable annotation
Annotations-->>Grant: Attach annotations
Grant-->>Syncer: Return immutable grant
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -81,6 +81,107 @@ func TestExpandGrants(t *testing.T) { | |||
| _ = os.Remove(c1zpath) | |||
| } | |||
|
|
|||
| func TestExpandGrantImmutable(t *testing.T) { | |||
There was a problem hiding this comment.
is it possible to add a test for a circular group expansion? i think this current test is a fine validation, but i was wondering about the behavoir of full circles, eg group A has member B which has member group C which has member A -> and a user could be attached at any point, but only have direct in a single group?
There was a problem hiding this comment.
I added a test but it looks like we'll need more changes to correctly support immutable in circular group membership. We currently fail to mark them immutable. I think that's OK for now.
If we expand a grant and there isn't already a direct grant for the same entitlement and principal, mark the grant as immutable. Also add tests.
ggreer
force-pushed
the
ggreer/immutable-grant-expansion
branch
from
693a322 to
f7bd344
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/sync/syncer_test.go (2)
84-182: LGTM with minor suggestions for improved documentation.The test function effectively verifies the grant immutability behavior for both direct and indirect grants. The test setup and assertions are comprehensive.
Consider adding documentation to clarify the test scenario:
+// TestExpandGrantImmutable verifies that: +// 1. Direct grants (user1 -> group1) remain mutable +// 2. Indirect grants through group expansion (user2 -> group2 -> group1) are marked as immutable +// This ensures that only expanded grants without existing direct grants are immutable. func TestExpandGrantImmutable(t *testing.T) {Also, consider adding more descriptive assertion messages:
-require.False(t, hasImmutable) // Direct grant should not be immutable +require.False(t, hasImmutable, "Direct grant (user1 -> group1) should not be immutable") -require.True(t, hasImmutable) // Expanded indirect grant should be immutable +require.True(t, hasImmutable, "Indirect grant (user2 -> group2 -> group1) should be immutable")
184-296: LGTM with action items for cyclic grant expansion.The test effectively sets up a cyclic group membership scenario and verifies direct grant behavior. However, there's a TODO indicating that indirect grant immutability in cyclic scenarios needs to be implemented.
The TODO comment at line 291 indicates that cyclic grant expansion needs to be fixed. Would you like me to:
- Help implement the fix for cyclic grant expansion?
- Open a GitHub issue to track this enhancement?
Consider adding documentation to clarify the cyclic test scenario:
+// TestExpandGrantImmutableCycle verifies grant immutability behavior in cyclic group memberships: +// - Group cycle: group1 -> group2 -> group3 -> group1 +// - Direct grant: user1 -> group1 (should remain mutable) +// - Indirect grant through cycle: user2 -> group2 -> group1 (should be immutable) +// TODO: Currently, indirect grants in cyclic scenarios are not properly handled. func TestExpandGrantImmutableCycle(t *testing.T) {Also, consider adding more descriptive assertion messages:
-require.False(t, hasImmutable) // Direct grant should not be immutable +require.False(t, hasImmutable, "Direct grant (user1 -> group1) should not be immutable even in cyclic scenario") -// require.True(t, hasImmutable) // Expanded indirect grant should be immutable -require.False(t, hasImmutable) // TODO: delete this and fix the code so the above line passes +// TODO: Enable this assertion after fixing cyclic grant expansion +// require.True(t, hasImmutable, "Indirect grant through cycle (user2 -> group2 -> group1) should be immutable") +require.False(t, hasImmutable, "Temporary assertion until cyclic grant expansion is fixed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/sync/syncer.go(1 hunks)pkg/sync/syncer_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/sync/syncer.go
🔇 Additional comments (1)
pkg/sync/syncer_test.go (1)
12-14: LGTM!
The new imports are necessary for reading and verifying grant annotations in the test functions.
If we expand a grant and there isn't already a direct grant for the same entitlement and principal, mark the grant as immutable.
Also add tests.
Summary by CodeRabbit
New Features
Tests