Fix super-admin role #516
Fix super-admin role #516
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe pull request introduces updates to the versioning of the tenant application and enhances the access control model by adding a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)packages/apps/tenant/templates/tenant.yaml (3)
The current super-admin role configuration has some limitations:
As mentioned in a previous review, consider adding these permissions for a complete super-admin role:
- apiGroups: [rbac.authorization.k8s.io]
resources:
- roles
+ - rolebindings
verbs:
- get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete
# ... existing permissions ...
+ - apiGroups: [""]
+ resources:
+ - events
+ - pods/log
+ - pods/exec
+ verbs:
+ - get
+ - list
+ - watch
+ - create
The role permissions in the cozy-public namespace are well-scoped to helm-related resources.
The RoleBinding configuration correctly implements the hierarchical access requirement, ensuring that super-admins of parent tenants automatically get super-admin access to child tenants through the template logic. Let's verify the template rendering for a nested tenant: ✅ Verification successfulThe previous attempt failed because Hierarchical super-admin access implementation verified The template logic correctly implements hierarchical access for super-admins:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify RoleBinding template rendering for nested tenants
# Create a temporary values file
cat << EOF > /tmp/values.yaml
tenant:
name: tenant-parent-child
EOF
# Render the template and check RoleBinding subjects
helm template -f /tmp/values.yaml . | grep -A 10 "kind: RoleBinding" | grep -A 10 "name: tenant-parent-child-super-admin"
Length of output: 225 Script: #!/bin/bash
# Let's verify the template logic directly from the file
cat packages/apps/tenant/templates/tenant.yaml | grep -A 40 "kind: RoleBinding" | grep -B 40 "roleRef:"
Length of output: 5050 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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/apps/tenant/Chart.yaml(1 hunks)packages/apps/tenant/templates/tenant.yaml(1 hunks)packages/apps/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/tenant/Chart.yaml
🔇 Additional comments (3)
packages/apps/versions_map (1)
91-92: LGTM: Version updates are consistent with the changes
The version progression from 1.6.1 to 1.6.2 appropriately reflects the addition of the super-admin role functionality.
packages/apps/tenant/templates/tenant.yaml (2)
392-414: LGTM: Appropriate permissions for helm operations
The super-admin role in cozy-public namespace has the correct unrestricted access to helm charts, which is appropriate for this role level.
415-441: Verify the existence of super-admin groups
The RoleBindings are correctly configured, but we should verify that the corresponding groups exist in the authentication system.
✅ Verification successful
Super-admin group is properly defined in the authentication system
The super-admin group is correctly configured in both the RBAC RoleBindings and the Keycloak authentication system:
- The group is defined as a KeycloakRealmGroup in
keycloakgroups.yaml - It's properly linked to the Keycloak realm via
realmRef - The group name matches exactly with what's used in the RoleBindings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to super-admin groups in the codebase
# Search for group definitions or references
rg -l "{{ include \"tenant.name\" . }}-super-admin" --type yaml
# Check for any existing group configurations
ast-grep --pattern 'kind: Group
name: $$$super-admin$$$'
Length of output: 241
Script:
#!/bin/bash
# Search for group definitions or references using escaped quotes
rg -l "\\{\\{ include \"tenant.name\" . \\}\\}-super-admin" --type yaml
# Search for any Group kind definitions
rg -A 3 "kind: Group" --type yaml
# Search for any auth/group configurations
fd -e yaml -e yml | xargs rg -l "groups?" --type yaml
Length of output: 17102
Script:
#!/bin/bash
# Check the keycloakgroups.yaml file for group definitions
cat packages/apps/tenant/templates/keycloakgroups.yaml
# Check for any Keycloak group-related configurations
rg -A 5 "super-admin" packages/apps/tenant/templates/keycloakgroups.yaml
Length of output: 1652
| --- | ||
| kind: Role | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| metadata: | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| namespace: {{ include "tenant.name" . }} | ||
| rules: | ||
| - apiGroups: [rbac.authorization.k8s.io] | ||
| resources: | ||
| - roles | ||
| verbs: | ||
| - get | ||
| - apiGroups: [""] | ||
| resources: | ||
| - "*" | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - delete | ||
| - apiGroups: ["helm.toolkit.fluxcd.io"] | ||
| resources: | ||
| - helmreleases | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - apiGroups: ["kubevirt.io"] | ||
| resources: | ||
| - virtualmachines | ||
| verbs: | ||
| - get | ||
| - list | ||
| - apiGroups: ["subresources.kubevirt.io"] | ||
| resources: | ||
| - virtualmachineinstances/console | ||
| - virtualmachineinstances/vnc | ||
| verbs: | ||
| - get | ||
| - list | ||
| - apiGroups: ["apps.cozystack.io"] | ||
| resources: | ||
| - '*' | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding super-admin permissions for complete control
While the role provides comprehensive access to apps.cozystack.io resources, consider adding these permissions for a true super-admin role:
- Full access to
rolesandrolebindingsfor user management - Full access to
eventsandpods/logfor monitoring - Full access to
pods/execfor troubleshooting
- apiGroups: [rbac.authorization.k8s.io]
resources:
- roles
+ - rolebindings
verbs:
- get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete
# ... existing permissions ...
+ - apiGroups: [""]
+ resources:
+ - events
+ - pods/log
+ - pods/exec
+ verbs:
+ - get
+ - list
+ - watch
+ - create📝 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.
| --- | |
| kind: Role | |
| apiVersion: rbac.authorization.k8s.io/v1 | |
| metadata: | |
| name: {{ include "tenant.name" . }}-super-admin | |
| namespace: {{ include "tenant.name" . }} | |
| rules: | |
| - apiGroups: [rbac.authorization.k8s.io] | |
| resources: | |
| - roles | |
| verbs: | |
| - get | |
| - apiGroups: [""] | |
| resources: | |
| - "*" | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - delete | |
| - apiGroups: ["helm.toolkit.fluxcd.io"] | |
| resources: | |
| - helmreleases | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - apiGroups: ["kubevirt.io"] | |
| resources: | |
| - virtualmachines | |
| verbs: | |
| - get | |
| - list | |
| - apiGroups: ["subresources.kubevirt.io"] | |
| resources: | |
| - virtualmachineinstances/console | |
| - virtualmachineinstances/vnc | |
| verbs: | |
| - get | |
| - list | |
| - apiGroups: ["apps.cozystack.io"] | |
| resources: | |
| - '*' | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - create | |
| - update | |
| - patch | |
| - delete | |
| --- | |
| kind: Role | |
| apiVersion: rbac.authorization.k8s.io/v1 | |
| metadata: | |
| name: {{ include "tenant.name" . }}-super-admin | |
| namespace: {{ include "tenant.name" . }} | |
| rules: | |
| - apiGroups: [rbac.authorization.k8s.io] | |
| resources: | |
| - roles | |
| - rolebindings | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - create | |
| - update | |
| - patch | |
| - delete | |
| - apiGroups: [""] | |
| resources: | |
| - "*" | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - delete | |
| - apiGroups: [""] | |
| resources: | |
| - events | |
| - pods/log | |
| - pods/exec | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - create | |
| - apiGroups: ["helm.toolkit.fluxcd.io"] | |
| resources: | |
| - helmreleases | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - apiGroups: ["kubevirt.io"] | |
| resources: | |
| - virtualmachines | |
| verbs: | |
| - get | |
| - list | |
| - apiGroups: ["subresources.kubevirt.io"] | |
| resources: | |
| - virtualmachineinstances/console | |
| - virtualmachineinstances/vnc | |
| verbs: | |
| - get | |
| - list | |
| - apiGroups: ["apps.cozystack.io"] | |
| resources: | |
| - '*' | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - create | |
| - update | |
| - patch | |
| - delete |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/apps/tenant/templates/tenant.yaml (1)
338-425: Consider implementing additional security controlsWhile introducing a super-admin role, consider these security recommendations:
- Implement audit logging for super-admin actions
- Consider adding resource quotas or limits even for super-admins
- Document the process for granting/revoking super-admin access
- Consider implementing break-glass procedures for super-admin access
Would you like assistance in implementing any of these security controls?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/apps/tenant/templates/tenant.yaml(1 hunks)
🔇 Additional comments (3)
packages/apps/tenant/templates/tenant.yaml (3)
383-398: LGTM: Appropriate permissions for helm operations
The permissions granted in the cozy-public namespace are well-scoped for helm chart and repository management.
338-380:
Expand RBAC permissions for comprehensive super-admin control
The super-admin role has limited RBAC permissions which may hinder administrative tasks. While it has full access to apps.cozystack.io resources, it lacks essential permissions for complete cluster management.
Consider adding these permissions:
- apiGroups: [rbac.authorization.k8s.io]
resources:
- roles
+ - rolebindings
verbs:
- get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete
# ... existing permissions ...
+ - apiGroups: [""]
+ resources:
+ - events
+ - pods/log
+ - pods/exec
+ verbs:
+ - get
+ - list
+ - watch
+ - create399-425: LGTM: Verify group existence and membership
The RoleBindings are correctly structured and follow consistent naming patterns. However, ensure that the corresponding groups exist and have appropriate membership controls.
✅ Verification successful
Groups are properly defined and managed through Keycloak integration
The super-admin group is correctly defined and managed:
- KeycloakRealmGroup resource creates the group in Keycloak for OIDC-based authentication
- RoleBindings in both namespaces correctly reference this group
- Part of a complete RBAC hierarchy (view, use, admin, super-admin groups)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of super-admin groups and their memberships
# Search for group definitions or references
rg -l "kind:\s+Group.*super-admin"
# Search for group membership management code
rg -l "group.*super-admin.*member"
Length of output: 73
Script:
#!/bin/bash
# Let's try a broader search for group-related configurations and references
# Search for any Group kind definitions
rg -l "kind:\s*Group"
# Search for any mentions of super-admin in yaml files
fd -e yaml -e yml -x rg -l "super-admin" {}
# Search for any RBAC-related configurations
fd -e yaml -e yml -x rg -l "rbac.authorization.k8s.io" {}
Length of output: 16323
Script:
#!/bin/bash
# Let's examine the keycloakgroups.yaml file since it was found in the super-admin search
cat packages/apps/tenant/templates/keycloakgroups.yaml
# Also check the tenant.yaml file for group definitions
cat packages/apps/tenant/templates/tenant.yaml | grep -A 10 "kind: Group"
Length of output: 3024
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| namespace: cozy-public | ||
| subjects: | ||
| - kind: Group | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io | ||
| roleRef: | ||
| kind: Role | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io | ||
| --- | ||
| kind: RoleBinding | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| metadata: | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| namespace: {{ include "tenant.name" . }} | ||
| subjects: | ||
| - kind: Group | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io | ||
| roleRef: | ||
| kind: Role | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
Super admin of one tenant should be super-admin of any sub-tenants
| - kind: Group | ||
| name: tenant-root-super-admin | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
Super admin of one tenant should be super-admin of any sub-tenants
| - kind: Group | ||
| name: {{ join "-" (slice $parts 0 (add $i 1)) }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
Super admin of one tenant should be super-admin of any sub-tenants
| - kind: Group | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
Super admin of one tenant should be super-admin of any sub-tenants
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/apps/tenant/templates/tenant.yaml(1 hunks)
🔇 Additional comments (4)
packages/apps/tenant/templates/tenant.yaml (4)
338-380: Expand super-admin role permissions for complete control
The current super-admin role has limited permissions for certain critical resources. As previously suggested, consider expanding the permissions for comprehensive control:
- apiGroups: [rbac.authorization.k8s.io]
resources:
- roles
+ - rolebindings
verbs:
- get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete
# ... existing permissions ...
+ - apiGroups: [""]
+ resources:
+ - events
+ - pods/log
+ - pods/exec
+ verbs:
+ - get
+ - list
+ - watch
+ - create383-398: LGTM: Appropriate permissions for Helm operations
The role permissions in the cozy-public namespace are well-scoped for Helm chart and repository operations.
399-412: LGTM: Clean and correct RoleBinding configuration
The RoleBinding correctly associates the super-admin role with the corresponding group.
338-435: Verify role hierarchy and permissions across the codebase
Let's verify the implementation and its impact:
✅ Verification successful
Let me analyze the results and generate a final response based on the verification of role hierarchy and permissions across the codebase.
Super-admin role implementation is consistent and properly scoped
The verification shows that:
- The super-admin role is properly defined with specific permissions in the tenant namespace
- The role bindings are correctly scoped to both tenant and cozy-public namespaces
- No conflicts found with other role definitions in the codebase
- The role hierarchy (super-admin > admin > view) is maintained consistently
The implementation follows Kubernetes RBAC best practices with:
- Clear separation of concerns between different role levels
- Explicit permission grants rather than wildcards where possible
- Proper namespace scoping for role bindings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify role hierarchy and permissions
# Check for any other RoleBindings that might conflict with the new super-admin role
echo "Checking for potential role conflicts..."
rg -l 'kind:\s+RoleBinding' | while read -r file; do
echo "=== $file ==="
rg -A 10 'kind:\s+RoleBinding' "$file"
done
# Check for any direct references to the old admin roles that might need updating
echo "Checking for admin role references..."
rg -l 'tenant-.*-admin' | while read -r file; do
echo "=== $file ==="
rg 'tenant-.*-admin' "$file"
done
# Look for any potential security implications in the codebase
echo "Checking for security-related code..."
rg -l 'rbac|role|permission|admin' | while read -r file; do
echo "=== $file ==="
rg 'rbac|role|permission|admin' "$file"
done
Length of output: 131070
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| namespace: {{ include "tenant.name" . }} | ||
| subjects: | ||
| {{- if hasPrefix "tenant-" .Release.Namespace }} | ||
| {{- $parts := splitList "-" .Release.Namespace }} | ||
| {{- range $i, $v := $parts }} | ||
| {{- if ne $i 0 }} | ||
| - kind: Group | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| - kind: ServiceAccount | ||
| name: {{ include "tenant.name" . }} | ||
| namespace: {{ include "tenant.name" . }} | ||
| roleRef: | ||
| kind: Role | ||
| name: {{ include "tenant.name" . }}-super-admin | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
Implement hierarchical super-admin access for sub-tenants
The current RoleBinding doesn't fully implement the requirement that a super-admin of one tenant should be super-admin of any sub-tenants. Consider modifying the template to include parent tenant super-admin groups:
subjects:
{{- if hasPrefix "tenant-" .Release.Namespace }}
{{- $parts := splitList "-" .Release.Namespace }}
{{- range $i, $v := $parts }}
{{- if ne $i 0 }}
- kind: Group
name: {{ include "tenant.name" . }}-super-admin
apiGroup: rbac.authorization.k8s.io
+ # Add parent tenant's super-admin group
+ - kind: Group
+ name: {{ join "-" (slice $parts 0 (add $i 1)) }}-super-admin
+ apiGroup: rbac.authorization.k8s.io
{{- end }}
{{- end }}
{{- end }}This change ensures that super-admins of parent tenants automatically get super-admin access to child tenants.
📝 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.
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: RoleBinding | |
| metadata: | |
| name: {{ include "tenant.name" . }}-super-admin | |
| namespace: {{ include "tenant.name" . }} | |
| subjects: | |
| {{- if hasPrefix "tenant-" .Release.Namespace }} | |
| {{- $parts := splitList "-" .Release.Namespace }} | |
| {{- range $i, $v := $parts }} | |
| {{- if ne $i 0 }} | |
| - kind: Group | |
| name: {{ include "tenant.name" . }}-super-admin | |
| apiGroup: rbac.authorization.k8s.io | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} | |
| - kind: ServiceAccount | |
| name: {{ include "tenant.name" . }} | |
| namespace: {{ include "tenant.name" . }} | |
| roleRef: | |
| kind: Role | |
| name: {{ include "tenant.name" . }}-super-admin | |
| apiGroup: rbac.authorization.k8s.io | |
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: RoleBinding | |
| metadata: | |
| name: {{ include "tenant.name" . }}-super-admin | |
| namespace: {{ include "tenant.name" . }} | |
| subjects: | |
| {{- if hasPrefix "tenant-" .Release.Namespace }} | |
| {{- $parts := splitList "-" .Release.Namespace }} | |
| {{- range $i, $v := $parts }} | |
| {{- if ne $i 0 }} | |
| - kind: Group | |
| name: {{ include "tenant.name" . }}-super-admin | |
| apiGroup: rbac.authorization.k8s.io | |
| - kind: Group | |
| name: {{ join "-" (slice $parts 0 (add $i 1)) }}-super-admin | |
| apiGroup: rbac.authorization.k8s.io | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} | |
| - kind: ServiceAccount | |
| name: {{ include "tenant.name" . }} | |
| namespace: {{ include "tenant.name" . }} | |
| roleRef: | |
| kind: Role | |
| name: {{ include "tenant.name" . }}-super-admin | |
| apiGroup: rbac.authorization.k8s.io |
klinch0
force-pushed
the
bugfix/fix-tenant-super-admin
branch
from
64e79cc to
b63d721
Compare
Summary by CodeRabbit
New Features
super-adminrole with comprehensive permissions across resources, enhancing access control.Version Updates
1.6.1to1.6.2.tenant, updated to reflect new version identifiers.These updates improve user access management and ensure the application is running on the latest version.