OCPBUGS-66201: Add impersonation user tests for multi-group RBAC functionality #15809
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@Leo6Leo: This pull request references Jira Issue OCPBUGS-66201, 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. |
WalkthroughAdds two Cypress test files: a smoke test for impersonation modal and banner behavior, and a comprehensive E2E suite exercising multi-group impersonation and RBAC across namespaces with setup/teardown and helper utilities. (47 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Leo6Leo: This pull request references Jira Issue OCPBUGS-66201, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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
the
kind/cypress
|
[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 |
![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 (7)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts (3)
98-129: Test interdependency creates fragility.This test relies on impersonation state from the previous test (
'SMOKE: Can impersonate a user'). If that test fails or is skipped, this test will also fail. Each test should be independent and set up its own state.Additionally, the comment on line 124 says "Nodes page" but the code navigates to "BuildConfigs".
- it('SMOKE: Banner persists across page navigation', () => { - // 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'); + it('SMOKE: Banner persists across page navigation', () => { + // Start impersonation to ensure test independence + 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.contains('You are impersonating', { timeout: 10000 }).should('be.visible');// 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');
40-63: Duplicated cleanup logic betweenafter()hook and test body.The cleanup logic for stopping impersonation is nearly identical in the
after()hook (lines 40-63) and within the'SMOKE: Cancel does not start impersonation'test (lines 440-457). Extract this to a reusable helper function to improve maintainability and reduce duplication.Consider adding a helper at the top of the describe block:
const cleanupImpersonation = () => { cy.get('body').then(($body) => { if ($body.find('.pf-v6-c-banner:contains("You are impersonating")').length > 0) { cy.get('[data-test="user-dropdown-toggle"]').then(($toggle) => { if ($toggle.length > 0 && $toggle.is(':visible')) { cy.wrap($toggle).click(); cy.get('body').then(($body2) => { if ($body2.find('[data-test="stop-impersonate"]').length > 0) { cy.byTestID('stop-impersonate').click(); cy.contains('You are impersonating', { timeout: 10000 }).should('not.exist'); } }); } }); } }); };Also applies to: 440-457
91-92: Excessive use of hardcodedcy.wait()leads to flaky tests.The test suite uses many hardcoded
cy.wait()calls (e.g.,cy.wait(1000),cy.wait(1500),cy.wait(500)). While the eslint-disable acknowledges this, these fixed waits can cause flakiness—tests may fail on slower CI environments or pass when they shouldn't on fast machines.Where possible, prefer waiting for specific conditions:
- cy.wait(1000); - - // Verify impersonation banner appears - cy.contains('You are impersonating', { timeout: 10000 }).should('be.visible'); + // Wait for impersonation banner to appear (indicates page reload complete) + cy.contains('You are impersonating', { timeout: 10000 }).should('be.visible');For navigation waits, consider waiting for a page-specific element or URL change instead of fixed delays.
Also applies to: 106-107, 114-115, 122-123, 187-188, 301-302, 362-363, 367-368, 376-377, 385-386, 394-395, 403-404, 425-426, 437-438, 453-454
frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts (4)
44-104: Silent failures in resource setup may cause misleading test results.All
cy.execcalls usefailOnNonZeroExit: false, which means resource creation failures are silently ignored. If namespace or group creation fails, subsequent tests will fail for unclear reasons.Consider adding validation that critical resources were created successfully:
cy.exec(`oc create namespace ${testNamespaceA} --dry-run=client -o yaml | oc apply -f -`, { failOnNonZeroExit: false, - }); + }).then((result) => { + if (result.code !== 0 && !result.stderr.includes('already exists')) { + throw new Error(`Failed to create namespace ${testNamespaceA}: ${result.stderr}`); + } + });Alternatively, add a verification step after resource creation to confirm essential resources exist before running tests.
182-183: 10-second hardcoded wait is excessive and may not be sufficient.A 10-second wait for pods to be created is arbitrary—pods may take longer on a slow cluster or be ready much sooner. Consider polling for pod readiness instead:
- // Wait for pods to be created and potentially start - cy.wait(10000); + // Wait for pods to exist (not necessarily Running, just created) + cy.exec(`oc wait --for=condition=Ready pod/test-pod-a -n ${testNamespaceA} --timeout=60s`, { + failOnNonZeroExit: false, + }); + cy.exec(`oc wait --for=condition=Ready pod/test-pod-b -n ${testNamespaceB} --timeout=60s`, { + failOnNonZeroExit: false, + });If pods don't need to be Running (just created), use
oc get podwith retry logic instead.
250-272: Helper functions could be extracted and shared with smoke tests.Both test files implement similar impersonation start/stop patterns. The helpers defined here (
startImpersonation,stopImpersonation) could be extracted to a shared utility file (e.g.,views/impersonation.ts) and reused in the smoke test file to reduce duplication and ensure consistency.// frontend/packages/integration-tests-cypress/views/impersonation.ts export const startImpersonation = (username: string, groups: string[] = []) => { 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(username); // ... rest of implementation }; export const stopImpersonation = () => { 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'); };Also applies to: 274-279
35-199: Consider extracting resource setup to a separate utility or fixture.The
before()hook spans ~165 lines with resource creation logic. This makes the test file harder to maintain. Consider extracting resource setup to:
- A separate helper module (e.g.,
support/rbac-setup.ts)- Pre-created fixtures that can be applied via a single command
- A shell script that Cypress can invoke
This would improve readability and allow reuse of the setup logic in other RBAC-related test suites.
📜 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-smoke.cy.tsfrontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts
🧬 Code graph analysis (2)
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)
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)
1-23: Well-structured RBAC test coverage.The test suite provides comprehensive coverage for multi-group impersonation scenarios:
- Single group access control validation
- Multi-group combined permissions
- API-level verification via
oc auth can-iandoc get pods- Edge cases including special characters and empty groups
The use of real Kubernetes resources and API verification (not just UI checks) provides strong confidence in the feature's correctness.
Also applies to: 281-389, 391-563, 654-685
| /** | ||
| * SMOKE TEST SUMMARY | ||
| * | ||
| * These 10 tests verify the critical path: | ||
| * 1. Modal opens | ||
| * 2. User impersonation works | ||
| * 3. Banner persists across navigation (without groups) | ||
| * 4. Stop works | ||
| * 5. Validation works | ||
| * 6. Group selection works (if groups available) | ||
| * 7. Group selection UI with mocked data (guaranteed groups for UI testing) | ||
| * 8. Banner persists across navigation with groups | ||
| * 9. Stop from dropdown works | ||
| * 10. Cancel works | ||
| * | ||
| * If all smoke tests pass → Feature is working! | ||
| * If any smoke test fails → Something is seriously broken! | ||
| * | ||
| * Run these tests: | ||
| * - After every build | ||
| * - Before deployment | ||
| * - As a gate before running full E2E suite | ||
| * | ||
| * Expected run time: 3-4 minutes | ||
| */ |
There was a problem hiding this comment.
Summary comment claims "10 tests" but actual count differs.
The summary states "These 10 tests verify the critical path" but the describe block contains 9 it() blocks. Update the comment to match the actual test count.
🤖 Prompt for AI Agents
In
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts
around lines 473 to 497, the header comment incorrectly states "These 10 tests
verify the critical path" while the file contains 9 it() blocks; update the
comment to match the actual count by changing "10" to "9" (or alternatively add
a missing test if you intended 10), and ensure the numbered list and summary
reflect the corrected test count.
| // 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)'); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Conditional assertion weakens test reliability.
This test checks whether namespace B is visible but only logs a warning if it is, rather than failing. Since the test is meant to verify the user does NOT have access to namespace B, finding it visible should fail the test:
// 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');
+ // User shouldn't see namespaces they don't have access to
+ throw new Error(`Security issue: User can see namespace ${testNamespaceB} without permissions`);
} else {
cy.log('✅ Correctly not showing namespace B (no permissions)');
}
});📝 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.
| // 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)'); | |
| } | |
| }); | |
| // Namespace B should NOT be visible in the dropdown (no permissions) | |
| cy.get('body').then(($body) => { | |
| if ($body.text().includes(testNamespaceB)) { | |
| // User shouldn't see namespaces they don't have access to | |
| throw new Error(`Security issue: User can see namespace ${testNamespaceB} without permissions`); | |
| } else { | |
| cy.log('✅ Correctly not showing namespace B (no permissions)'); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In frontend/packages/integration-tests-cypress/tests/app/impersonate-user.cy.ts
around lines 329 to 338, the test currently only logs a warning if Namespace B
is present which weakens the test; change the conditional logging to a hard
assertion that Namespace B is not present (e.g., use
cy.contains(testNamespaceB).should('not.exist') or
cy.get(...).should('not.be.visible') as appropriate for the dropdown element) so
the test fails when Namespace B is visible, and remove the if/else logging
branch.
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts (1)
471-495: Summary comment claims "10 tests" but actual count is 9.The describe block contains 9
it()blocks, not 10. Update the summary to match.-/** - * SMOKE TEST SUMMARY - * - * These 10 tests verify the critical path: +/** + * SMOKE TEST SUMMARY + * + * These 9 tests verify the critical path:
🧹 Nitpick comments (3)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts (3)
96-127: Test depends on state from previous test, creating fragility.This test assumes impersonation is active from the previous test. If "Can impersonate a user" fails, this test will fail for the wrong reason, making debugging harder. Consider starting impersonation within this test or using a
beforeEachpattern for tests that require active impersonation.Also, the inline comments don't match the actual navigation targets:
- Line 118: Comment says "Navigate to Build > Builds" but navigates to
BuildConfigs- Line 122: Comment says "Verify banner still persists on Nodes page" but we're on
BuildConfigs- // Navigate to Build > Builds + // Navigate to Builds > BuildConfigs 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');
399-406: Comment does not match navigation action.The comment says "Navigate to Compute > Nodes" but the code navigates to
['Builds', 'BuildConfigs'].- // Navigate to Compute > Nodes + // Navigate to Builds > BuildConfigs cy.clickNavLink(['Builds', 'BuildConfigs']);
38-61: Cleanup logic uses nested conditionals that could leave impersonation active.The
after()hook has deeply nested conditionals. If the dropdown opens butstop-impersonateisn't found immediately, cleanup silently skips. Consider using Cypress retry-ability with proper assertions rather than synchronous jQuery checks, or add logging to indicate when cleanup is skipped.
📜 Review details
Configuration used: Organization 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.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-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 (1)
frontend/packages/integration-tests-cypress/tests/app/impersonate-user-smoke.cy.ts (1)
1-36: LGTM overall — good coverage of critical impersonation flows.The tests cover the essential happy paths: modal interaction, impersonation start/stop, banner persistence, validation states, and group selection with both real and mocked data. The use of
cy.intercept()for deterministic group testing is a solid approach.Also applies to: 63-94, 129-154, 156-195, 197-315, 317-414, 416-436, 438-468
|
/retest-required |
|
/retest-required |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
Updated impersonation logic to wait for dropdown rendering and use text selectors for menu items.
|
/retest-required |
|
@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. |
2 participants
Description
Adding the e2e cypress test as a follow up for the multi-group impersonation feature completed in #15612