OSAC-1283: Add functions and types for policies and permissions#665
Open
CrystalChun wants to merge 1 commit into
Open
OSAC-1283: Add functions and types for policies and permissions#665CrystalChun wants to merge 1 commit into
CrystalChun wants to merge 1 commit into
Conversation
Adds functionality to CRUD policies and permissions in Keycloak for fine-grained access control for projects. Assisted-by: Claude Code <noreply@anthropic.com>
|
@CrystalChun: This pull request references OSAC-1283 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR extends the Keycloak IDP integration with authorization policy and scope permission management for project access control. It introduces domain types, client interface methods, REST API implementations, and integrates policy/permission creation into project authorization group setup with coordinated cleanup on failure. ChangesKeycloak Authorization Policy and Scope Permission Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/idp/keycloak/authz_policies.go`:
- Around line 51-52: Remove the no-op assignment that sets policy.GroupsClaim to
an empty string when it's already empty: delete the if block that checks if
policy.GroupsClaim == "" and reassigns it to ""; alternatively, if a default is
intended, set policy.GroupsClaim to a meaningful default (e.g., "groups") where
policy.GroupsClaim is initialized/validated so the code uses that default
instead of the redundant assignment.
In `@internal/idp/manager_test.go`:
- Around line 270-280: The mock CreateGroupPolicy implementation in mockClient
returns an AuthorizationPolicy but omits copying the Groups field from the
input, so update mockClient.CreateGroupPolicy to include Groups: policy.Groups
in the returned AuthorizationPolicy (preserving all other fields like ID, Name,
Type, Logic, DecisionStrategy, GroupsClaim) so the mock mirrors real behavior.
In `@internal/idp/resource_manager.go`:
- Around line 496-526: Both getPolicyIDByName and getPermissionIDByName
duplicate the same type-assert + call pattern; extract a generic helper (e.g.,
getResourceIDByName) on ResourceManager that accepts ctx, resourceType string,
name string and a lookupFunc func(context.Context,string)(string,error) and
returns (string,error); update getPolicyIDByName and getPermissionIDByName to
build their specific lookupFunc by type-asserting m.client to the existing
policyIDGetter/permissionIDGetter and pass that function to getResourceIDByName,
and ensure getResourceIDByName returns a clear error like "client does not
support getting <resourceType> ID by name" when lookupFunc is nil.
- Around line 293-304: The cleanupGroupsOnFailure helper currently swallows
errors from getGroupIDByPath and DeleteAuthorizationGroup; update
cleanupGroupsOnFailure to log debug-level messages when either
m.getGroupIDByPath or m.client.DeleteAuthorizationGroup return an error (include
the organizationName, group path, and the returned error), and log when a group
ID is empty to aid tracing; keep the "best-effort" behavior (do not return the
error) but ensure debug logs call the existing logger (e.g., m.logger or process
logger used elsewhere) so failures are visible during troubleshooting.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 56714416-4519-4088-a5dd-4c7370b4ed22
📒 Files selected for processing (9)
internal/controllers/project/project_reconciler_function_test.gointernal/idp/client.gointernal/idp/client_mock.gointernal/idp/keycloak/authz_policies.gointernal/idp/keycloak/types.gointernal/idp/manager_test.gointernal/idp/resource_manager.gointernal/idp/resource_manager_test.gointernal/idp/types.go
Comment on lines
+51
to
+52
| if policy.GroupsClaim == "" { | ||
| policy.GroupsClaim = "" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove no-op assignment.
Setting GroupsClaim to an empty string when it's already empty serves no purpose. Either remove this block or set a meaningful default value.
♻️ Suggested fix
- if policy.GroupsClaim == "" {
- policy.GroupsClaim = ""
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if policy.GroupsClaim == "" { | |
| policy.GroupsClaim = "" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/idp/keycloak/authz_policies.go` around lines 51 - 52, Remove the
no-op assignment that sets policy.GroupsClaim to an empty string when it's
already empty: delete the if block that checks if policy.GroupsClaim == "" and
reassigns it to ""; alternatively, if a default is intended, set
policy.GroupsClaim to a meaningful default (e.g., "groups") where
policy.GroupsClaim is initialized/validated so the code uses that default
instead of the redundant assignment.
Comment on lines
+270
to
+280
| func (m *mockClient) CreateGroupPolicy(ctx context.Context, policy *AuthorizationPolicy) (*AuthorizationPolicy, error) { | ||
| // Return the policy with an ID assigned | ||
| return &AuthorizationPolicy{ | ||
| ID: "test-policy-id", | ||
| Name: policy.Name, | ||
| Type: policy.Type, | ||
| Logic: policy.Logic, | ||
| DecisionStrategy: policy.DecisionStrategy, | ||
| GroupsClaim: policy.GroupsClaim, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Mock should copy Groups field to match real behavior.
The CreateGroupPolicy mock copies most fields from the input policy but omits the Groups field (line 279). This could cause tests to miss issues with group path handling. Real implementations (like Keycloak) would preserve this field in the response.
🔧 Suggested fix to include Groups field
func (m *mockClient) CreateGroupPolicy(ctx context.Context, policy *AuthorizationPolicy) (*AuthorizationPolicy, error) {
// Return the policy with an ID assigned
return &AuthorizationPolicy{
ID: "test-policy-id",
Name: policy.Name,
Type: policy.Type,
Logic: policy.Logic,
DecisionStrategy: policy.DecisionStrategy,
GroupsClaim: policy.GroupsClaim,
+ Groups: policy.Groups,
}, nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/idp/manager_test.go` around lines 270 - 280, The mock
CreateGroupPolicy implementation in mockClient returns an AuthorizationPolicy
but omits copying the Groups field from the input, so update
mockClient.CreateGroupPolicy to include Groups: policy.Groups in the returned
AuthorizationPolicy (preserving all other fields like ID, Name, Type, Logic,
DecisionStrategy, GroupsClaim) so the mock mirrors real behavior.
Comment on lines
+293
to
+304
| // cleanupGroupsOnFailure is a helper to clean up groups when policy or permission creation fails. | ||
| func (m *ResourceManager) cleanupGroupsOnFailure(ctx context.Context, organizationName, viewersGroupPath, managersGroupPath string) { | ||
| viewersGroupID, _ := m.getGroupIDByPath(ctx, organizationName, viewersGroupPath) | ||
| if viewersGroupID != "" { | ||
| _ = m.client.DeleteAuthorizationGroup(ctx, organizationName, viewersGroupID) | ||
| } | ||
|
|
||
| managersGroupID, _ := m.getGroupIDByPath(ctx, organizationName, managersGroupPath) | ||
| if managersGroupID != "" { | ||
| _ = m.client.DeleteAuthorizationGroup(ctx, organizationName, managersGroupID) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider logging cleanup failures for debugging.
The best-effort cleanup silently ignores all errors. While this is appropriate for rollback scenarios, it could make debugging difficult if the cleanup fails. Consider adding debug-level logging when getGroupIDByPath or DeleteAuthorizationGroup fail.
💡 Optional enhancement to add debug logging
func (m *ResourceManager) cleanupGroupsOnFailure(ctx context.Context, organizationName, viewersGroupPath, managersGroupPath string) {
- viewersGroupID, _ := m.getGroupIDByPath(ctx, organizationName, viewersGroupPath)
+ viewersGroupID, err := m.getGroupIDByPath(ctx, organizationName, viewersGroupPath)
+ if err != nil {
+ m.logger.DebugContext(ctx, "Failed to get viewers group ID during cleanup",
+ slog.String("group_path", viewersGroupPath),
+ slog.Any("error", err),
+ )
+ }
if viewersGroupID != "" {
- _ = m.client.DeleteAuthorizationGroup(ctx, organizationName, viewersGroupID)
+ if err := m.client.DeleteAuthorizationGroup(ctx, organizationName, viewersGroupID); err != nil {
+ m.logger.DebugContext(ctx, "Failed to delete viewers group during cleanup",
+ slog.String("group_id", viewersGroupID),
+ slog.Any("error", err),
+ )
+ }
}
- managersGroupID, _ := m.getGroupIDByPath(ctx, organizationName, managersGroupPath)
+ managersGroupID, err := m.getGroupIDByPath(ctx, organizationName, managersGroupPath)
+ if err != nil {
+ m.logger.DebugContext(ctx, "Failed to get managers group ID during cleanup",
+ slog.String("group_path", managersGroupPath),
+ slog.Any("error", err),
+ )
+ }
if managersGroupID != "" {
- _ = m.client.DeleteAuthorizationGroup(ctx, organizationName, managersGroupID)
+ if err := m.client.DeleteAuthorizationGroup(ctx, organizationName, managersGroupID); err != nil {
+ m.logger.DebugContext(ctx, "Failed to delete managers group during cleanup",
+ slog.String("group_id", managersGroupID),
+ slog.Any("error", err),
+ )
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/idp/resource_manager.go` around lines 293 - 304, The
cleanupGroupsOnFailure helper currently swallows errors from getGroupIDByPath
and DeleteAuthorizationGroup; update cleanupGroupsOnFailure to log debug-level
messages when either m.getGroupIDByPath or m.client.DeleteAuthorizationGroup
return an error (include the organizationName, group path, and the returned
error), and log when a group ID is empty to aid tracing; keep the "best-effort"
behavior (do not return the error) but ensure debug logs call the existing
logger (e.g., m.logger or process logger used elsewhere) so failures are visible
during troubleshooting.
Comment on lines
+496
to
+526
| // getPolicyIDByName is a helper to get the policy ID from a policy name. | ||
| // This is a Keycloak-specific operation and may not be available on all IdP clients. | ||
| func (m *ResourceManager) getPolicyIDByName(ctx context.Context, policyName string) (string, error) { | ||
| // This relies on the Keycloak client implementation | ||
| // If the client doesn't support this, it will return an error | ||
| type policyIDGetter interface { | ||
| GetPolicyIDByName(ctx context.Context, policyName string) (string, error) | ||
| } | ||
|
|
||
| if getter, ok := m.client.(policyIDGetter); ok { | ||
| return getter.GetPolicyIDByName(ctx, policyName) | ||
| } | ||
|
|
||
| return "", fmt.Errorf("client does not support getting policy ID by name") | ||
| } | ||
|
|
||
| // getPermissionIDByName is a helper to get the permission ID from a permission name. | ||
| // This is a Keycloak-specific operation and may not be available on all IdP clients. | ||
| func (m *ResourceManager) getPermissionIDByName(ctx context.Context, permissionName string) (string, error) { | ||
| // This relies on the Keycloak client implementation | ||
| // If the client doesn't support this, it will return an error | ||
| type permissionIDGetter interface { | ||
| GetPermissionIDByName(ctx context.Context, permissionName string) (string, error) | ||
| } | ||
|
|
||
| if getter, ok := m.client.(permissionIDGetter); ok { | ||
| return getter.GetPermissionIDByName(ctx, permissionName) | ||
| } | ||
|
|
||
| return "", fmt.Errorf("client does not support getting permission ID by name") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Consider extracting common lookup pattern to reduce duplication.
The getPolicyIDByName and getPermissionIDByName functions follow an identical pattern (interface definition, type assertion, method call). While the duplication is minimal and the current approach is clear, extracting a generic helper could improve maintainability if more lookup methods are added in the future.
🔄 Optional refactor using a generic helper
Note: This is a more complex change and may reduce clarity, so only consider if multiple similar lookup methods will be added.
// getResourceIDByName is a generic helper for Keycloak name-to-ID lookups.
func (m *ResourceManager) getResourceIDByName(
ctx context.Context,
resourceType string,
name string,
lookupFunc func(ctx context.Context, name string) (string, error),
) (string, error) {
if lookupFunc == nil {
return "", fmt.Errorf("client does not support getting %s ID by name", resourceType)
}
return lookupFunc(ctx, name)
}
// Then use it like:
func (m *ResourceManager) getPolicyIDByName(ctx context.Context, policyName string) (string, error) {
type policyIDGetter interface {
GetPolicyIDByName(ctx context.Context, policyName string) (string, error)
}
var lookupFunc func(context.Context, string) (string, error)
if getter, ok := m.client.(policyIDGetter); ok {
lookupFunc = getter.GetPolicyIDByName
}
return m.getResourceIDByName(ctx, "policy", policyName, lookupFunc)
}However, the current explicit approach is clearer and has minimal duplication, so this refactoring is optional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/idp/resource_manager.go` around lines 496 - 526, Both
getPolicyIDByName and getPermissionIDByName duplicate the same type-assert +
call pattern; extract a generic helper (e.g., getResourceIDByName) on
ResourceManager that accepts ctx, resourceType string, name string and a
lookupFunc func(context.Context,string)(string,error) and returns
(string,error); update getPolicyIDByName and getPermissionIDByName to build
their specific lookupFunc by type-asserting m.client to the existing
policyIDGetter/permissionIDGetter and pass that function to getResourceIDByName,
and ensure getResourceIDByName returns a clear error like "client does not
support getting <resourceType> ID by name" when lookupFunc is nil.
jhernand
approved these changes
Jun 9, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
Author
|
/retest |
Contributor
Author
|
/hold |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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
3 participants
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.
Description
Adds functionality to CRUD policies and permissions in Keycloak for fine-grained access control for projects.
Testing
go build ./...Assisted-by: Claude Code noreply@anthropic.com
/cc @jhernand
Summary by CodeRabbit