CONSOLE-4734: Multi-group impersonation Combined #15612
Conversation
…tion Support multiple Impersonate-Group subprotocols on WebSocket connections. Maintains backward compatibility for single-group impersonation.
…nation Handle X-Console-Impersonate-Groups header by splitting comma-separated groups into multiple Impersonate-Group headers for Kubernetes API.
Update GraphQL context to support array-valued headers for multi-group
impersonation. Change headers type from map[string]string to map[string]interface{}
to support both single string values and []string arrays.
Update fetch utilities to handle multi-group impersonation: - Support array-valued Impersonate-Group headers - Convert array to X-Console-Impersonate-Groups for fetch() API compatibility - Add UserWithGroups kind support in header generation
…ation Add multi-group support to Redux actions: - Update startImpersonate to accept groups parameter - Encode multiple groups as WebSocket subprotocols in UserWithGroups mode - Pass groups to beginImpersonate action - Expose store and UIActions for testing (temporary)
Add modal component for multi-group user impersonation: - TypeScript React component with group selection - Multi-select group input with search/filter - Chip display for selected groups - Form validation for username - Internationalization support
Integrate multi-group impersonation into the UI: - Update impersonation banner to display multiple groups - Add modal trigger in masthead toolbar - Update user component to show impersonated user - Add feature flag checks in app component - Add masthead menu items for start/stop impersonation - Add enabled parameter to usePackageManifestCheck hook
openshift-ci-robot
added
the
jira/valid-reference
|
@Leo6Leo: This pull request references CONSOLE-4734 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.21.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. |
openshift-ci
bot
added
component/backend
frontend/public/redux.ts
Outdated
| window.store = store; | ||
| } | ||
|
|
||
| // Temporary: Expose store for testing multi-group impersonation |
There was a problem hiding this comment.
This need to get removed whenever the PR is ready to get merged.
openshift-ci
bot
added
component/sdk
… in the multi-group impersonation modal.
|
/cc @logonoff |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
…y logonoff Co-authored-by: logonoff <git@logonoff.co>
…he test to reduce the flake
Leo6Leo
force-pushed
the
leo/CONSOLE-4782-all-changes-combined
branch
from
b35925e to
b97bafd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts (3)
40-63: Cleanup logic may fail silently if dropdown doesn't open.The
after()cleanup has nested.then()callbacks that could silently skip cleanup if the dropdown toggle click doesn't open the menu in time. Consider adding explicit waits or using.should()assertions to ensure the dropdown is open before checking for the stop button.after(() => { // Cleanup: stop any active impersonation using dropdown (more reliable) cy.get('body').then(($body) => { // Only clean up if there's an impersonation banner (not other cluster banners) if ($body.find('.pf-v6-c-banner:contains("You are impersonating")').length > 0) { // Check if dropdown toggle exists and is visible - cy.get('[data-test="user-dropdown-toggle"]').then(($toggle) => { - if ($toggle.length > 0 && $toggle.is(':visible')) { - cy.wrap($toggle).click(); - - // Wait for dropdown to open and stop button to appear - cy.get('body').then(($body2) => { - if ($body2.find('[data-test="stop-impersonate"]').length > 0) { - cy.byTestID('stop-impersonate').click(); - - // Wait for impersonation banner to disappear - cy.contains('You are impersonating', { timeout: 10000 }).should('not.exist'); - } - }); - } - }); + cy.byTestID('user-dropdown-toggle').should('be.visible').click(); + cy.byTestID('stop-impersonate').should('be.visible').click(); + cy.contains('You are impersonating', { timeout: 10000 }).should('not.exist'); } }); });
104-136: Test relies on state from previous test, reducing isolation.Line 106 explicitly states this test depends on state from the previous test ("Impersonation should be active from previous test"). This creates test coupling and can cause cascading failures. If the previous test fails or is skipped, this test will also fail for the wrong reason.
Consider making this test self-contained by starting impersonation at the beginning:
it('SMOKE: Banner persists across page navigation', () => { guidedTour.close(); - // Impersonation should be active from previous test - // Check specifically for the impersonation banner, not other banners (e.g., kubeadmin) - cy.contains('.pf-v6-c-banner', 'You are impersonating', { timeout: 5000 }).should('be.visible'); - cy.contains('smoke-test-user').should('be.visible'); + // Start impersonation for this test + cy.byTestID('user-dropdown-toggle').should('be.visible').click(); + cy.byTestID('impersonate-user').should('be.visible').click(); + cy.byTestID('username-input').should('be.visible').clear().type('smoke-test-user'); + cy.byTestID('impersonate-button').should('not.be.disabled').click(); + cy.wait(1000); + cy.contains('You are impersonating', { timeout: 10000 }).should('be.visible'); // Navigate to Workloads > Pods using nav link (preserves Redux state)
178-190: Conditional test logic with.then()may hide failures.Using
cy.get('body').then()to conditionally check for groups creates a code path where the test passes even if no groups are available. While the log message notes this is acceptable for smoke tests, this pattern makes it harder to detect regressions where groups should be available but aren't.Consider splitting into two tests: one that explicitly skips when groups unavailable, and another (like line 209) that uses mocked data to guarantee group availability.
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts (3)
44-104: Resource creation commands don't verify success before proceeding.The
cy.exec()calls for creating namespaces, groups, and rolebindings usefailOnNonZeroExit: falsebut don't verify the resources were actually created before proceeding. If any creation fails (e.g., due to quota or permissions), subsequent tests will fail with confusing errors.Consider adding verification after resource creation:
cy.exec(`oc create namespace ${testNamespaceA} --dry-run=client -o yaml | oc apply -f -`, { failOnNonZeroExit: false, + }).then((result) => { + if (result.code !== 0) { + cy.log(`⚠️ Namespace A creation may have failed: ${result.stderr}`); + } });Or add a verification step after all creations:
// Verify all resources exist before running tests cy.exec(`oc get namespace ${testNamespaceA} ${testNamespaceB} ${testNamespaceC}`); cy.exec(`oc get group ${testGroupA} ${testGroupB} ${testGroupC}`);
182-183: Hard-coded 10 second wait may be insufficient or excessive.The 10-second wait for pods to be created is a fixed delay that may be too short in slow environments or unnecessarily long in fast ones. This contributes to test flakiness and slow execution.
Consider polling for pod readiness instead:
- // Wait for pods to be created and potentially start - cy.wait(10000); + // Wait for pods to be created (poll with timeout) + cy.exec(`oc wait --for=condition=Ready pod/test-pod-a -n ${testNamespaceA} --timeout=60s || oc get pod test-pod-a -n ${testNamespaceA}`, { + failOnNonZeroExit: false, + timeout: 70000, + });
334-355: Conditional assertion doesn't fail on unexpected access.Lines 334-341 log a warning if namespace B is unexpectedly visible but don't fail the test. The actual access verification happens via
cy.exec()on lines 347-352, which is good, but the UI check should either be removed or made to fail if unexpected access is detected.// Namespace B should NOT be visible in the dropdown (no permissions) cy.get('body').then(($body) => { if ($body.text().includes(testNamespaceB)) { // If somehow visible, it's an error - user shouldn't see namespaces they don't have access to - cy.log('⚠️ WARNING: User can see namespace B when they should not have access'); + throw new Error(`Security issue: User can see namespace ${testNamespaceB} when they should not have access`); } else { cy.log('✅ Correctly not showing namespace B (no permissions)'); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts(1 hunks)frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.tsfrontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts
🧬 Code graph analysis (1)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts (1)
frontend/packages/integration-tests-cypress/support/index.ts (1)
checkErrors(84-90)
🔇 Additional comments (4)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts (1)
1-485: Comprehensive smoke test suite covering critical paths.The smoke tests provide good coverage of the impersonation feature's critical paths: modal interactions, user impersonation, group selection, banner persistence, and cleanup. The mocked data tests (lines 209-328) ensure deterministic UI validation. The documentation blocks clearly communicate test scope and expectations.
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts (3)
250-273: Helper functions improve readability and reduce duplication.The
startImpersonationandstopImpersonationhelper functions (lines 250-281) effectively encapsulate the repeated UI interactions for impersonation flows. This is good practice for test maintainability.
544-567: API verification with multiple groups provides strong RBAC validation.The test on lines 544-567 effectively validates combined group permissions by testing API access to all three namespaces with the correct
--as-groupflags. This provides reliable verification that the multi-group impersonation is working at the API level, not just in the UI.
1-750: Well-structured E2E test suite with comprehensive RBAC coverage.The test suite provides thorough coverage of the multi-group impersonation feature:
- Real resource creation/cleanup for authentic RBAC testing
- Single and multi-group permission verification
- Edge cases (special characters, empty groups)
- Both UI and API-level validation
The test organization into logical describe blocks (Single Group, Multi-Group, Search/Filter, Edge Cases) improves maintainability and makes test failures easier to diagnose.
| // Navigate to Build > Builds | ||
| cy.clickNavLink(['Builds', 'BuildConfigs']); | ||
| cy.wait(1500); | ||
|
|
||
| // Verify banner still persists on Nodes page | ||
| cy.contains('.pf-v6-c-banner', 'You are impersonating').should('be.visible'); | ||
| cy.contains('smoke-test-user').should('be.visible'); |
There was a problem hiding this comment.
Comment says "Nodes page" but code navigates to "BuildConfigs".
The comment on line 131 says "Verify banner still persists on Nodes page" but the navigation on line 128 goes to ['Builds', 'BuildConfigs']. This misleading comment could confuse future maintainers.
// Navigate to Build > Builds
cy.clickNavLink(['Builds', 'BuildConfigs']);
cy.wait(1500);
- // Verify banner still persists on Nodes page
+ // Verify banner still persists on BuildConfigs page
cy.contains('.pf-v6-c-banner', 'You are impersonating').should('be.visible');🤖 Prompt for AI Agents
In
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts
around lines 127 to 133, the inline comment says "Nodes page" but the test
navigates to ['Builds', 'BuildConfigs']; update the comment to accurately
describe the destination (e.g., "Verify banner still persists on BuildConfigs
page") or, if the intent was to check the Nodes page, change the navigation call
to the correct nav path (e.g., ['Infrastructure', 'Nodes']) so the comment and
navigation match.
| // Verify tooltip shows both groups | ||
| cy.get('[role="tooltip"]', { timeout: 5000 }) |
There was a problem hiding this comment.
Comment says "Navigate to Compute > Nodes" but navigates to "Builds > BuildConfigs".
Same issue as in the smoke test file - the comment doesn't match the actual navigation.
- // Navigate to Compute > Nodes
+ // Navigate to Builds > BuildConfigs
cy.clickNavLink(['Builds', 'BuildConfigs']);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts
around lines 413-414, the inline comment says "Navigate to Compute > Nodes" but
the test actually navigates to "Builds > BuildConfigs"; update the comment to
accurately reflect the navigation (e.g., "Navigate to Builds > BuildConfigs") or
if the test should go to Compute > Nodes, change the navigation code/selectors
to perform that navigation instead—make the comment and the navigation
consistent.
…mpersonation button is clicked: since after staritng the impersonation, the modal might show up and blocking the test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts (1)
405-427: Reduce coupling to PF CSS classes / literal copy and avoid fixedcy.waitwhere possibleAcross this file (e.g., here and in the group‑chip checks), the tests rely on:
- PatternFly CSS classes like
.pf-v6-c-banner,.pf-v6-c-label,.pf-v6-c-menu__list(-item).- Literal UI text like
'You are impersonating','with groups:', and'No results found'.- Fixed
cy.wait(...)calls after interactions.This works but is brittle: PF upgrades or minor copy/i18n changes will break tests, and hard waits can both slow the suite and still be flaky under load.
When you iterate on this suite, consider:
- Prefer
data-test/data-test-idselectors on key elements (banner, group chips, menus) instead of PF CSS classes and visible strings.- Where feasible, replace
cy.wait(...)with event‑driven waits (e.g.,should('be.visible'), checking for table rows, dropdown menus, or spinner disappearance) so Cypress’ retry logic handles timing.- Reserve text assertions for semantics you explicitly want to lock in (e.g., banner presence), and still back them with more stable attributes if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts(1 hunks)frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts
🧬 Code graph analysis (1)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts (3)
frontend/packages/integration-tests-cypress/support/index.ts (1)
checkErrors(84-90)frontend/public/components/RBAC/rules.jsx (1)
groups(58-58)frontend/packages/integration-tests-cypress/views/nav.ts (1)
nav(6-105)
🔇 Additional comments (1)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts (1)
25-199: Comprehensive RBAC coverage and setup/teardown look solidThe end‑to‑end flow (cluster setup via
oc, impersonation helpers, and the set of scenarios across single/multi‑group, search, selection, persistence, and edge cases) is well structured and matches the multi‑group impersonation feature’s intent. The global setup/cleanup andcheckErrorshook should keep these tests reliable in CI.Also applies to: 283-749
| // Namespace B should NOT be visible in the dropdown (no permissions) | ||
| cy.get('body').then(($body) => { | ||
| if ($body.text().includes(testNamespaceB)) { | ||
| // If somehow visible, it's an error - user shouldn't see namespaces they don't have access to | ||
| cy.log('⚠️ WARNING: User can see namespace B when they should not have access'); | ||
| } else { | ||
| cy.log('✅ Correctly not showing namespace B (no permissions)'); | ||
| } | ||
| }); | ||
|
|
||
| // Close dropdown | ||
| cy.get('body').click(0, 0); | ||
|
|
||
| // Verify via API that user has no access to namespace B | ||
| cy.exec(`oc get pods -n ${testNamespaceB} --as=${testUser} --as-group=${testGroupA}`, { | ||
| failOnNonZeroExit: false, | ||
| }).then((result) => { | ||
| expect(result.code).to.not.equal(0); | ||
| expect(result.stderr).to.include('cannot list resource "pods"'); | ||
| }); |
There was a problem hiding this comment.
Make namespace‑B visibility an assertion, not just a log
In the "should NOT have access to namespace B" test, the UI check only logs a warning when namespace B is visible:
cy.get('body').then(($body) => {
if ($body.text().includes(testNamespaceB)) {
// If somehow visible, it's an error - user shouldn't see namespaces they don't have access to
cy.log('⚠️ WARNING: User can see namespace B when they should not have access');
} else {
cy.log('✅ Correctly not showing namespace B (no permissions)');
}
});This never fails the test on that condition; only the later oc call can fail it. Given both the test name and comment, this should be an assertion.
You can make the intent enforceable with something like:
- cy.get('body').then(($body) => {
- if ($body.text().includes(testNamespaceB)) {
- // If somehow visible, it's an error - user shouldn't see namespaces they don't have access to
- cy.log('⚠️ WARNING: User can see namespace B when they should not have access');
- } else {
- cy.log('✅ Correctly not showing namespace B (no permissions)');
- }
- });
+ cy.get('body').then(($body) => {
+ const bodyText = $body.text();
+ // Namespace B must not be visible in the dropdown
+ expect(bodyText.includes(testNamespaceB)).to.be.false;
+ });(or equivalently, use cy.contains(testNamespaceB).should('not.exist') scoped to the dropdown container).
🤖 Prompt for AI Agents
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts
around lines 333 to 352: the test currently only logs a warning when namespace B
is visible instead of asserting it must not be visible, so replace the
conditional cy.get('body').then(...) logging block with a real Cypress assertion
that namespace B is not present in the dropdown (e.g., scope to the dropdown
element and use a negative assertion like contains(...).should('not.exist') or
should('not.be.visible')), preserving the subsequent click to close the dropdown
and the existing API verification.
|
--- FAIL: TestAsyncCache (4.00s) /retest backend |
|
@jhadvig: The The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
/test backend |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, jseseCCS, Leo6Leo, logonoff, 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 |
|
@Leo6Leo: The following test 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. |
|
/retest-required |
|
/verified by @yapei |
openshift-ci-robot
added
the
verified
|
@Leo6Leo: This PR has been marked as verified by 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. |
|
@Leo6Leo Thanks for sharing the demo and highlighting the various scenarios.
Hope this helps. Thank you! |
|
The follow up PR to fix all the issues mentioned in this PR is opened: #15916 |
10 participants
Description
As a cluster administrator want to impersonate a user with multiple group memberships simultaneously, so that I can accurately reproduce their effective permissions and troubleshoot RBAC issues.