Hierarchical Namespace Entitlement Revocation #69
Conversation
Instead of removing all namespace access when revoking a permission, downgrade to the next lower permission level in the hierarchy: Admin → Write → Read → remove access entirely. Also validates that the user actually has the permission being revoked.
WalkthroughIntroduces a hierarchical permission-downgrade helper function for namespace access and modifies the Revoke operation to validate entitlement IDs, check current access levels, and conditionally downgrade permissions (ADMIN→WRITE→READ→removal) rather than immediately revoking access. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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
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: 0
🧹 Nitpick comments (3)
pkg/connector/helpers.go (1)
238-249: Make downgrade mapping explicit (READ/UNSPECIFIED) to match comment and reduce ambiguity.
Current implementation works, butdefaultimplicitly includes READ and any future enum values; being explicit keeps intent clear and safer to maintain.Proposed diff
func nextLowerNamespacePermission(current identityv1.NamespaceAccess_Permission) identityv1.NamespaceAccess_Permission { switch current { case identityv1.NamespaceAccess_PERMISSION_ADMIN: return identityv1.NamespaceAccess_PERMISSION_WRITE case identityv1.NamespaceAccess_PERMISSION_WRITE: return identityv1.NamespaceAccess_PERMISSION_READ - default: + case identityv1.NamespaceAccess_PERMISSION_READ: + return identityv1.NamespaceAccess_PERMISSION_UNSPECIFIED + case identityv1.NamespaceAccess_PERMISSION_UNSPECIFIED: + return identityv1.NamespaceAccess_PERMISSION_UNSPECIFIED + default: return identityv1.NamespaceAccess_PERMISSION_UNSPECIFIED } }pkg/connector/namespaces.go (2)
219-229: Tighten entitlement ID validation (prefix + namespace match), not just “3 parts”.
Right now anyx:y:zwith a valid permission inzpasses; validatingenIDParts[0]andenIDParts[1]prevents accidental mismatches betweenentitlementIDandnamespaceID.Proposed diff
enIDParts := strings.Split(entitlementID, ":") - if len(enIDParts) != 3 { + if len(enIDParts) != 3 || enIDParts[0] != "namespace" || enIDParts[1] != namespaceID { return nil, fmt.Errorf("baton-temporalcloud: invalid entitlement ID %s", entitlementID) }
236-258: Add nil-guard forcurrentAccessand prefer mutating existing entry on downgrade.
This avoids a potential panic if the map value is nil, and future-proofs ifNamespaceAccessgains additional fields beyondPermission.Proposed diff
currentAccess, ok := spec.GetAccess().GetNamespaceAccesses()[namespaceID] - if !ok { + if !ok || currentAccess == nil { annos := annotations.New(&v2.GrantAlreadyRevoked{}) return annos, fmt.Errorf("baton-temporalcloud: grant does not exist for user") } @@ nextPerm := nextLowerNamespacePermission(revokedPerm) if nextPerm == identityv1.NamespaceAccess_PERMISSION_UNSPECIFIED { // Read permission: remove access entirely delete(spec.Access.NamespaceAccesses, namespaceID) } else { // Downgrade to the next lower permission level - spec.Access.NamespaceAccesses[namespaceID] = &identityv1.NamespaceAccess{Permission: nextPerm} + currentAccess.Permission = nextPerm }The updated async failure message at Line 280 looks good/clearer for the revoke path.
Also applies to: 280-281
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/connector/helpers.gopkg/connector/helpers_test.gopkg/connector/namespaces.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: test
🔇 Additional comments (2)
pkg/connector/helpers_test.go (2)
72-111: No change needed: Go 1.25 includes the range-variable capture fix.The repository targets Go 1.25, which fixed the range-variable capture bug introduced in earlier Go versions. The code is already safe as written. The defensive
tc := tccapture is not required.Likely an incorrect or invalid review comment.
113-167: This is not a bug in Go 1.22 and later versions.Go 1.22 (released March 2024) automatically fixed the loop variable capture issue. Each iteration now gets its own scope for the loop variable, eliminating the need for manual shadowing (
tc := tc). Since this project uses Go 1.25, the code is correct as written and does not require the proposed fix.Likely an incorrect or invalid review comment.
| // If they have a different permission level, this grant doesn't exist. | ||
| if currentAccess.GetPermission() != revokedPerm { | ||
| annos := annotations.New(&v2.GrantAlreadyRevoked{}) | ||
| return annos, fmt.Errorf("baton-temporalcloud: user does not have %s permission on namespace", revokedPermStr) |
There was a problem hiding this comment.
I don't think we generally return an error when returning a GrantAlreadyRevoked annotation
https://github.com/ConductorOne/baton-okta/blob/38a984cbc03bd1055f2bf6f63efcb28fc0aa3d83/pkg/connector/role.go#L644
https://github.com/ConductorOne/baton-contentful/blob/795bc6fed1d25464a293edbce65fa8248dfcbac4/pkg/connector/teams.go#L191
When revoking namespace entitlements, "downgrade" to the next entitlement in the hierarchy instead of removing access entirely. For example, when revoking write access, downgrade to read instead of removing access entirely. This is consistent with how other connectors such as
baton-githubhandle hierarchical entitlements.Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.