OCPBUGS-81749: Add SSAR checks to gate OLM actions#16554
Conversation
Added permission checks (useAccessReview/accessReview) to OLM actions and UI buttons to hide them when users lack required permissions. Previously, actions were visible to all users regardless of permissions, leading to failed operations when clicked. Actions fixed: - Edit Subscription action (UPDATE permission) - View ClusterServiceVersion action (GET permission) - Enable catalog source button (PATCH OperatorHub permission) - Subscription update channel/approval buttons (UPDATE permission) - Descriptor edit buttons: Boolean, Checkbox, PodCount, UpdateStrategy (UPDATE permission) Added unit tests verifying accessReview on actions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@TheRealJon: An error was encountered searching for bug OCPBUGS-81749 on the Jira server at https://redhat.atlassian.net. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://redhat.atlassian.net/rest/api/2/issue/OCPBUGS-81749": GET https://redhat.atlassian.net/rest/api/2/issue/OCPBUGS-81749 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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 integrates Kubernetes access review checks throughout the Operator Lifecycle Manager (OLM) package. It adds permission validation to action hooks and gates UI controls based on user permissions for subscription updates, catalog source patching, and descriptor editing. ChangesAccess review integration for OLM actions and components
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TheRealJon 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 |
openshift-ci
Bot
added
the
approved
|
@TheRealJon: An error was encountered searching for bug OCPBUGS-81749 on the Jira server at https://redhat.atlassian.net. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://redhat.atlassian.net/rest/api/2/issue/OCPBUGS-81749": GET https://redhat.atlassian.net/rest/api/2/issue/OCPBUGS-81749 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
![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
🤖 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
`@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx`:
- Around line 414-418: The useAccessReview call that sets canPatchOperatorHub is
missing resourceAttributes.name so the SSAR checks patch permission on all
OperatorHub resources instead of the specific instance; update the
useAccessReview invocation (the one used by DisabledPopover /
canPatchOperatorHub) to include resourceAttributes: { name:
operatorHub?.metadata?.name } (or the appropriate instance name variable) so the
access review is scoped to the single OperatorHub being enabled.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 13013632-2e6d-46ff-8b78-f08d8aa1bcd5
📒 Files selected for processing (7)
frontend/packages/operator-lifecycle-manager/src/actions/__tests__/useOperatorActions.spec.tsfrontend/packages/operator-lifecycle-manager/src/actions/hooks/__tests__/useSubscriptionActions.spec.tsfrontend/packages/operator-lifecycle-manager/src/actions/hooks/useSubscriptionActions.tsfrontend/packages/operator-lifecycle-manager/src/actions/useOperatorActions.tsfrontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsxfrontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/index.tsxfrontend/packages/operator-lifecycle-manager/src/components/subscription.tsx
| const [canPatchOperatorHub] = useAccessReview({ | ||
| group: OperatorHubModel.apiGroup, | ||
| resource: OperatorHubModel.plural, | ||
| verb: 'patch', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes RBAC SelfSubjectAccessReview, what is the effect of omitting resourceAttributes.nameversus setting it to a specific resource name? Does omission require broader permission than name-scopedresourceNames rules?
💡 Result:
In a Kubernetes SelfSubjectAccessReview, the resourceAttributes.name field determines the scope of the access check. When this field is omitted or left as an empty string (""), it signifies "all" resources of the specified type within the requested namespace (or cluster, if applicable) [1][2][3]. Conversely, setting it to a specific string restricts the check to a single, named resource instance [1][4][2]. Yes, omitting the name requires broader permissions than specifying a particular name. If you omit the name, the authorization check evaluates whether the user has permission to access any resource of that type (an "all" request) [1][3]. This requires that the user possesses a role or cluster role that explicitly grants access to that resource type across the entire scope (e.g., a rule with resourceNames omitted or a wildcard), whereas specifying a name allows the system to verify access specifically granted to that single resource instance [1][4]. In effect, an "all" request is a superset of a name-scoped request, necessitating that the user's permissions are not restricted to specific named resources [1][4].
Citations:
- 1: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/authorization/types.go
- 2: https://dev-k8sref-io.web.app/docs/authorization/selfsubjectaccessreview-v1/
- 3: https://docs.redhat.com/en/documentation/openshift_container_platform/4.15/html/authorization_apis/selfsubjectaccessreview-authorization-k8s-io-v1
- 4: https://github.com/kubernetes/api/blob/master/authorization/v1/types.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx"
# Show the referenced area
sed -n '380,460p' "$FILE" | nl -ba | sed -n '1,120p'
# Find all access review hooks in this file
rg -n "useAccessReview\\(" "$FILE"
# Try to locate where useAccessReview is implemented (search in repo)
rg -n "function useAccessReview|const useAccessReview|useAccessReview\\s*=\\s*" frontend/packages/operator-lifecycle-manager/src -S || true
# Fallback: search repo-wide for useAccessReview definition (limited)
rg -n "useAccessReview\\(" frontend/packages -S | head -n 50Repository: openshift/console
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx"
echo "== Snippet around lines 410-430 =="
awk 'NR>=395 && NR<=435 {print NR ":" $0}' "$FILE"
echo
echo "== useAccessReview calls in file =="
rg -n "useAccessReview\\(" "$FILE" || true
echo
echo "== Locate useAccessReview implementation/signature in repo =="
rg -n "function useAccessReview|const useAccessReview\\s*=|useAccessReview\\s*:\\s*|export\\s+function\\s+useAccessReview|export\\s+const\\s+useAccessReview" frontend/packages -S || true
echo
echo "== Also search whole frontend for useAccessReview definition =="
rg -n "useAccessReview" frontend/packages -S | head -n 200Repository: openshift/console
Length of output: 36111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect catalog-source.tsx around enableSource usage/definition
FILE="frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx"
echo "== catalog-source.tsx: around enableSource call/props =="
awk 'NR>=300 && NR<=460 {print NR ":" $0}' "$FILE"
echo
echo "== Search for enableSource definition/usage in file =="
rg -n "enableSource\\(" "$FILE"
rg -n "const enableSource|function enableSource|enableSource\\s*=" "$FILE" || true
echo
echo "== Locate operatorHub usage in file (metadata.name etc.) =="
rg -n "operatorHub\\??\\.metadata\\??\\.name|metadata\\.name|resourceAttributes" "$FILE" || true
# 2) Inspect useAccessReview implementation that consumes {group, resource, verb, name?}
RBAC_FILE="frontend/packages/console-dynamic-plugin-sdk/src/app/components/utils/rbac.tsx"
echo
echo "== rbac.tsx: inspect useAccessReview implementation =="
sed -n '1,260p' "$RBAC_FILE" | nl -ba | head -n 260
echo
echo "== rbac.tsx: inspect SelfSubjectAccessReview construction area =="
# narrower search for SelfSubjectAccessReview or resourceAttributes.name
rg -n "SelfSubjectAccessReview|resourceAttributes\\.name|resourceAttributes" "$RBAC_FILE"Repository: openshift/console
Length of output: 6824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RBAC_FILE="frontend/packages/console-dynamic-plugin-sdk/src/app/components/utils/rbac.tsx"
echo "== Show useAccessReview implementation (approx. 80-220) =="
sed -n '80,240p' "$RBAC_FILE"
echo
echo "== Search for SelfSubjectAccessReview construction =="
rg -n "SelfSubjectAccessReview" "$RBAC_FILE"
echo
echo "== Show surrounding code for SelfSubjectAccessReview =="
# print some context around each match
rg -n "SelfSubjectAccessReview" "$RBAC_FILE" | while read -r line; do
lineno="${line%%:*}"
start=$((lineno-25))
end=$((lineno+80))
echo "---- around line $lineno ----"
sed -n "${start},${end}p" "$RBAC_FILE"
done
echo
echo "== Search for resourceAttributes.name usage =="
rg -n "resourceAttributes\\.name|resourceAttributes\\s*:\\s*\\{|name\\s*:" "$RBAC_FILE" || trueRepository: openshift/console
Length of output: 5396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RBAC_FILE="frontend/packages/console-dynamic-plugin-sdk/src/app/components/utils/rbac.tsx"
CAT_FILE="frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx"
echo "== rbac.tsx: inspect checkAccessInternal / ssar payload =="
awk 'NR>=1 && NR<=120 {print NR ":" $0}' "$RBAC_FILE" | sed -n '1,120p'
echo
echo "== rbac.tsx: inspect around line(s) that build ssar.kind/ResourceAttributes =="
# Narrow: show more but still bounded
awk 'NR>=30 && NR<=110 {print NR ":" $0}' "$RBAC_FILE"
echo
echo "== catalog-source.tsx: inspect enableSource implementation =="
awk 'NR>=1 && NR<=140 {print NR ":" $0}' "$CAT_FILE" | sed -n '1,160p'Repository: openshift/console
Length of output: 13169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find OperatorHubModel definition
rg -n "export const OperatorHubModel|OperatorHubModel\\s*=" frontend/packages/operator-lifecycle-manager/src -S
rg -n "OperatorHubModel" frontend/packages/operator-lifecycle-manager/src/models -S || true
# If above doesn't show, search in whole operator-lifecycle-manager package
rg -n "OperatorHubModel" frontend/packages/operator-lifecycle-manager/src -S | head -n 50
# 2) Inspect asAccessReview implementation
rg -n "asAccessReview\\s*\\(" frontend/packages -S | head -n 50
rg -n "export .*asAccessReview|function asAccessReview|const asAccessReview" frontend/packages -S | head -n 50
# 3) Show the asAccessReview usage context in catalog-source.tsx
awk 'NR>=40 && NR<=110 {print NR ":" $0}' frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsxRepository: openshift/console
Length of output: 9760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) OperatorHubModel definition
MODEL_FILE="frontend/packages/operator-lifecycle-manager/src/models.ts"
if [ -f "$MODEL_FILE" ]; then
echo "== OperatorHubModel in $MODEL_FILE =="
rg -n "export const OperatorHubModel" "$MODEL_FILE"
sed -n '90,160p' "$MODEL_FILE"
else
echo "Missing $MODEL_FILE"
fi
echo
# 2) Locate asAccessReview implementation
echo "== Locate asAccessReview definition =="
rg -n "asAccessReview\\s*=\\s*\\(|function\\s+asAccessReview\\s*\\(|export\\s+.*asAccessReview" frontend/packages -S | head -n 50
# 3) Print asAccessReview implementation (best match)
# Try a few likely files explicitly by searching for its name then showing around the match.
AS_FILE=$(rg -l "asAccessReview\\s*\\(" frontend/packages/operator-lifecycle-manager/src -S | head -n 1 || true)
if [ -n "${AS_FILE:-}" ]; then
echo
echo "== First asAccessReview usage file (operator-lifecycle-manager) =="
echo "$AS_FILE"
fi
# Prefer the actual definition in a shared util; search and then print around the definition line.
DEF_FILE=$(rg -l "asAccessReview\\s*=|function\\s+asAccessReview|export\\s+const\\s+asAccessReview|export\\s+function\\s+asAccessReview" frontend/packages -S | head -n 20 | head -n 1 || true)
if [ -n "${DEF_FILE:-}" ]; then
echo
echo "== asAccessReview definition file =="
echo "$DEF_FILE"
DEF_LINE=$(rg -n "asAccessReview\\s*=|function\\s+asAccessReview|export\\s+const\\s+asAccessReview|export\\s+function\\s+asAccessReview" "$DEF_FILE" | head -n 1 | cut -d: -f1)
start=$((DEF_LINE-40))
end=$((DEF_LINE+80))
sed -n "${start},${end}p" "$DEF_FILE"
else
echo "No asAccessReview definition found with the current search."
fiRepository: openshift/console
Length of output: 1299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where asAccessReview is defined
rg -n "asAccessReview\\s*=" frontend/packages -S | head -n 50
rg -n "function\\s+asAccessReview\\b|export\\s+function\\s+asAccessReview\\b|export\\s+const\\s+asAccessReview\\b|export\\s+\\{[^}]*asAccessReview" frontend/packages -S | head -n 50
# Print matching files
rg -l "asAccessReview\\b" frontend/packages -S | head -n 50
echo
echo "== Show the first few definition candidates =="
for f in $(rg -l "asAccessReview\\b" frontend/packages -S | head -n 10); do
echo "---- $f ----"
rg -n "asAccessReview\\b" "$f" | head -n 20
doneRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for asAccessReview definition across repo =="
rg -n "export\\s+(const|function)\\s+asAccessReview\\b|asAccessReview\\s*=\\s*\\(" . -S | head -n 50
echo
echo "== Search for asAccessReview usage sites to infer signature =="
rg -n "asAccessReview\\(" frontend/packages -S | head -n 50Repository: openshift/console
Length of output: 7610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RBAC_FILE="frontend/public/components/utils/rbac.tsx"
echo "== asAccessReview definition =="
rg -n "export const asAccessReview" "$RBAC_FILE"
LINE=$(rg -n "export const asAccessReview" "$RBAC_FILE" | head -n 1 | cut -d: -f1)
start=$((LINE-20))
end=$((LINE+120))
sed -n "${start},${end}p" "$RBAC_FILE" | nl -ba | head -n 200Repository: openshift/console
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RBAC_FILE="frontend/public/components/utils/rbac.tsx"
LINE=$(rg -n "export const asAccessReview" "$RBAC_FILE" | head -n 1 | cut -d: -f1)
start=$((LINE-10))
end=$((LINE+160))
sed -n "${start},${end}p" "$RBAC_FILE"Repository: openshift/console
Length of output: 750
Scope the patch access review to the specific OperatorHub being enabled
DisabledPopover gates the “Enable source” button with a useAccessReview call that omits resourceAttributes.name, so the SSAR is effectively evaluated for “any/all” operatorhubs (not the single operatorHub instance being patched). With RBAC resourceNames scoping, this can incorrectly hide the action even when patching that named operatorHub is allowed.
Suggested fix
const [canPatchOperatorHub] = useAccessReview({
group: OperatorHubModel.apiGroup,
resource: OperatorHubModel.plural,
+ name: operatorHub?.metadata?.name,
verb: 'patch',
});🤖 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
`@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx`
around lines 414 - 418, The useAccessReview call that sets canPatchOperatorHub
is missing resourceAttributes.name so the SSAR checks patch permission on all
OperatorHub resources instead of the specific instance; update the
useAccessReview invocation (the one used by DisabledPopover /
canPatchOperatorHub) to include resourceAttributes: { name:
operatorHub?.metadata?.name } (or the appropriate instance name variable) so the
access review is scoped to the single OperatorHub being enabled.
|
@TheRealJon: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
Summary
Adds permission checks to OLM actions and UI buttons to hide them when users lack required permissions. Server still enforces permissions, but actions now properly gated in UI.
Fixes:
accessReviewaccessReviewuseAccessReviewhookuseAccessReviewhookuseAccessReviewhooksTests:
accessReviewchecksTest plan
Automated
useOperatorActions.spec.ts,useSubscriptionActions.spec.tsManual verification
See
frontend/MANUAL_TEST_INSTRUCTIONS.mdfor detailed test procedure.Quick test:
oc apply -f frontend/test-rbac-readonly-user.yamlExpected: All edit/delete actions hidden for read-only user.
Expected: All actions visible for admin user.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests