OCPBUGS-72526: Impersonating user loads extra pages that user not authorized to view#16088
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Leo6Leo: This pull request references CONSOLE-5070 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 story to target the "4.22.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Leo6Leo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Leo6Leo
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@Leo6Leo: This pull request references Jira Issue OCPBUGS-72526, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
…n handling Made-with: Cursor
Leo6Leo
force-pushed
the
leo/OCPBUGS-72526/impersonation-access-hide-tabs
branch
from
bf49972 to
0426766
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Leo6Leo: This pull request references Jira Issue OCPBUGS-72526, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
📝 WalkthroughWalkthroughThis pull request implements impersonation header handling across the GraphQL layer. The frontend introduces a setForceHTTP mechanism to switch transport between WebSocket and HTTP when entering or exiting impersonation mode, with calls placed in the refreshFeaturesAfterImpersonation and stopImpersonate flows. The backend GraphQL HTTP handler was refactored to extract impersonation headers from requests (Impersonate-User and Impersonate-Group variants, with conditional system:authenticated group appending) and propagate them into the resolver context, enabling downstream logic to access impersonation state. A minor error handling adjustment was made in the features action callback. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
pkg/server/server.go (1)
390-396: Normalize forwarded impersonation groups before sending downstream.Line 391 and Line 395 forward raw split values. Trimming whitespace and dropping empty entries prevents malformed
Impersonate-Groupvalues from accidental spacing/trailing commas.♻️ Suggested hardening
if consoleGroups := r.Header.Get("X-Console-Impersonate-Groups"); consoleGroups != "" { - groups := strings.Split(consoleGroups, ",") + rawGroups := strings.Split(consoleGroups, ",") + groups := make([]string, 0, len(rawGroups)+1) + for _, g := range rawGroups { + g = strings.TrimSpace(g) + if g != "" { + groups = append(groups, g) + } + } groups = append(groups, "system:authenticated") headers["Impersonate-Group"] = groups } else if impGroups := r.Header.Values("Impersonate-Group"); len(impGroups) > 0 { + for i := range impGroups { + impGroups[i] = strings.TrimSpace(impGroups[i]) + } impGroups = append(impGroups, "system:authenticated") headers["Impersonate-Group"] = impGroups }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/server.go` around lines 390 - 396, When forwarding impersonation groups from r.Header.Get("X-Console-Impersonate-Groups") (variable groups) or r.Header.Values("Impersonate-Group") (variable impGroups) normalize entries by trimming whitespace and removing empty strings before assigning to headers["Impersonate-Group"]; keep the existing append of "system:authenticated". In practice iterate over the split or values, apply strings.TrimSpace, skip if the trimmed entry == "", collect cleaned entries and then append "system:authenticated" and set headers["Impersonate-Group"] = cleanedSlice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/server/server.go`:
- Around line 398-399: The code creates a new background context and loses
request cancellation/deadline and upstream values; replace
context.WithValue(context.Background(), resolver.HeadersKey, headers) with using
the request's context as the parent (i.e., context.WithValue(r.Context(),
resolver.HeadersKey, headers)) so that graphQLHandler receives
r.WithContext(ctx) that preserves cancellation and other request-scoped values;
update the usage around resolver.HeadersKey and graphQLHandler accordingly.
---
Nitpick comments:
In `@pkg/server/server.go`:
- Around line 390-396: When forwarding impersonation groups from
r.Header.Get("X-Console-Impersonate-Groups") (variable groups) or
r.Header.Values("Impersonate-Group") (variable impGroups) normalize entries by
trimming whitespace and removing empty strings before assigning to
headers["Impersonate-Group"]; keep the existing append of
"system:authenticated". In practice iterate over the split or values, apply
strings.TrimSpace, skip if the trimmed entry == "", collect cleaned entries and
then append "system:authenticated" and set headers["Impersonate-Group"] =
cleanedSlice.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
frontend/public/actions/features.tsfrontend/public/actions/ui.tsfrontend/public/graphql/client.tspkg/server/server.go
| ctx := context.WithValue(context.Background(), resolver.HeadersKey, headers) | ||
| graphQLHandler(w, r.WithContext(ctx)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the actual code at lines 398-399
head -n 410 pkg/server/server.go | tail -n 25Repository: openshift/console
Length of output: 1281
🏁 Script executed:
#!/bin/bash
# Search for the specific pattern mentioned in the review
rg -nP 'context\.WithValue\(context\.Background\(\),\s*resolver\.HeadersKey' pkg/server/server.goRepository: openshift/console
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Let's also check the broader context of this handler to understand the function signature
sed -n '380,410p' pkg/server/server.goRepository: openshift/console
Length of output: 1641
🏁 Script executed:
#!/bin/bash
# Check for other similar patterns of context.Background() in handler contexts
rg -n 'context\.Background\(\)' pkg/server/server.go | head -20Repository: openshift/console
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Look for how graphQLHandler is defined and uses context
rg -n 'graphQLHandler\|NewHandlerFunc' pkg/server/server.go -B 2 -A 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there are other handlers in the codebase that do this correctly
rg -n 'context\.WithValue\(r\.Context\(\)' pkg/ -t go | head -10Repository: openshift/console
Length of output: 160
Preserve request context when injecting resolver headers.
Line 398 creates a fresh background context, which drops request cancellation/deadline and upstream context values. The codebase already uses the correct pattern elsewhere (e.g., pkg/middleware/middleware.go). Use r.Context() as the parent instead, particularly important since this handler wraps a GraphQL WebSocket connection that may be long-lived.
🔧 Proposed fix
- ctx := context.WithValue(context.Background(), resolver.HeadersKey, headers)
+ ctx := context.WithValue(r.Context(), resolver.HeadersKey, headers)
graphQLHandler(w, r.WithContext(ctx))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/server/server.go` around lines 398 - 399, The code creates a new
background context and loses request cancellation/deadline and upstream values;
replace context.WithValue(context.Background(), resolver.HeadersKey, headers)
with using the request's context as the parent (i.e.,
context.WithValue(r.Context(), resolver.HeadersKey, headers)) so that
graphQLHandler receives r.WithContext(ctx) that preserves cancellation and other
request-scoped values; update the usage around resolver.HeadersKey and
graphQLHandler accordingly.
|
/retest-required |
|
@Leo6Leo: all tests passed! 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
Description
The issue is that when impersonating a user, the SelfSubjectAccessReview checks that determine page visibility are sent via GraphQL but the backend GraphQL HTTP handler wasn't forwarding impersonation headers to the Kubernetes API. So the K8s API always evaluated permissions as kube:admin, not the impersonated user
Steps to QA
You are impersonating User testuser-1. You are viewing all resources and roles this User can access. Stop Impersonatingappear in masthead, then check the pagestestuser-1doesn't have enough access to view, such as: Home -> Overview, Compute, Administration -> Cluster Settings, Namespaces pages etc.Summary by CodeRabbit
New Features
Bug Fixes