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. In 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. |
WalkthroughTwo new Cypress test files are added for multi-group impersonation testing. The smoke test validates basic impersonation modal interactions, validation states, and banner visibility. The comprehensive suite tests RBAC scenarios across Kubernetes namespaces with helper utilities for starting/stopping impersonation and includes single-group, multi-group, and edge case validations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
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
In 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.
Needs 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 |
|
@Leo6Leo: The following test failed, say
Full PR test history. Your PR dashboard. 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. 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