fix(e2e): fix orchestrator RBAC tests

[testetson22]

Description

Fixes failing Orchestrator RBAC tests in nightly job(s).

New File: e2e-tests/playwright/support/api/orchestrator-rbac-helper.ts

A reusable helper class that:

• Provides removeGenericOrchestratorPermissions() - finds and removes any generic orchestrator.workflow permissions for a user, saving them for later restoration
• Provides restoreGenericOrchestratorPermissions() - restores previously saved permissions
• Exports SavedRolePolicy interface

We can roll more permission management into this, but I kept the scope narrow.

Modified: orchestrator-rbac.spec.ts

• Added import for OrchestratorRbacHelper
• In "Individual Workflow Denied Access" test:
  • Replaced inline interface and logic with OrchestratorRbacHelper instance
  • Simplified the "Remove any generic orchestrator.workflow permissions" test to use helper
  • Simplified afterAll restoration to use helper

Modified: orchestrator-entity-rbac.spec.ts

• Added import for OrchestratorRbacHelper
• Added testUser constant for "user:default/rhdh-qe"
• In "RHIDP-11840: Template run WITH workflow permissions" test:
  • Replaced inline interface and logic with OrchestratorRbacHelper instance
  • Simplified the setup test to use helper
  • Simplified afterAll restoration to use helper
• Added test.setTimeout(150000) to handle workflow execution timeout

Which issue(s) does this PR fix

• Fixes RHDHBUGS-2807

PR acceptance criteria

Please make sure that the following steps are complete:

• GitHub Actions are completed and successful
• Unit Tests are updated and passing
• E2E Tests are updated and passing
• Documentation is updated if necessary (requirement for new features)
• Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

[rhdh-qodo-merge]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 RHDHBUGS-2807 - Partially compliant Compliant requirements: Fix nightly e2e orchestrator test failure "Launch template and run workflow - verify success" caused by global timeout issues (increase timeout). Fix nightly e2e orchestrator test failure "Individual workflow denied access - no workflows visible" caused by assertion failure (enhance test logic to make it reliable). Non-compliant requirements: Ensure orchestrator nightly tests pass again. Requires further human verification: Ensure orchestrator nightly tests pass again.

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🔒 No security concerns identified

⚡ Recommended focus areas for review Flaky Cleanup The helper mutates live RBAC state (deletes policies) and restoration is best-effort. If restoration fails (or the test process crashes before afterAll), the environment can be left with altered permissions and cause follow-on flakes. Consider making restore failures hard-fail (or at least bubble up), and/or adding safeguards so deletion/restoration is more transactional and resilient. async restoreGenericOrchestratorPermissions( rbacApi: RhdhRbacApi, ): Promise<void> { for (const saved of this.savedGenericPolicies) { console.log( `Restoring generic orchestrator policies to ${saved.roleName}:`, saved.policies, ); const restoreResponse = await rbacApi.createPolicies(saved.policies); if (!restoreResponse.ok()) { console.error( `Failed to restore policies to ${saved.roleName}:`, await restoreResponse.text(), ); } } } Policy Shape Risk The code reads policies via policiesResponse.json() and assumes fields like permission, policy, and effect exist and that re-creating with entityReference: role.name is sufficient. If the API includes additional required fields/metadata (or if role.name vs stripped role name mismatches expectations), policy deletion/restoration may silently behave incorrectly. Consider using the existing response-normalization utility (e.g., the Response.removeMetadataFromResponse pattern used elsewhere) and validating the payload shape before delete/restore. const policies = await policiesResponse.json(); const genericOrchestratorPolicies = policies.filter( (policy: { permission: string }) => policy.permission === "orchestrator.workflow" || policy.permission === "orchestrator.workflow.use", ); if (genericOrchestratorPolicies.length > 0) { // Save these policies for restoration later this.savedGenericPolicies.push({ roleName: roleNameForApi, policies: genericOrchestratorPolicies.map( (p: { permission: string; policy: string; effect: string }) => ({ entityReference: role.name, permission: p.permission, policy: p.policy, effect: p.effect, }), ), }); // Remove the generic policies const policiesToDelete = genericOrchestratorPolicies.map( (p: { permission: string; policy: string; effect: string }) => ({ entityReference: role.name, permission: p.permission, policy: p.policy, effect: p.effect, }), ); Hardcoded User The test user entity ref is hardcoded inline in multiple places. This increases maintenance risk and makes reuse/override harder across suites/jobs. Prefer a single shared constant (as done in orchestrator-entity-rbac.spec.ts) and pass it consistently to the helper and role membership setup. test("Remove any generic orchestrator.workflow permissions for test user", async () => { const rbacApi = await RhdhRbacApi.build(apiToken); await orchestratorRbacHelper.removeGenericOrchestratorPermissions( rbacApi, "user:default/rhdh-qe", ); }); test("Create role with greeting workflow denied permissions", async () => { const rbacApi = await RhdhRbacApi.build(apiToken); const members = ["user:default/rhdh-qe"];

📄 References No matching references available

[rhdh-qodo-merge]

PR Type

Bug fix, Tests

---

Description

• Introduced OrchestratorRbacHelper class to manage generic orchestrator.workflow permissions

• Updated orchestrator RBAC tests to remove pre-existing permissions before test execution

• Restored removed permissions after test cleanup to prevent test interference

• Increased workflow execution test timeout to 150 seconds for longer processing times

---

File Walkthrough

	Relevant files
Enhancement	orchestrator-rbac-helper.ts New orchestrator RBAC permission management helper              e2e-tests/playwright/support/api/orchestrator-rbac-helper.ts Created new helper class OrchestratorRbacHelper to manage orchestrator RBAC policies Implemented removeGenericOrchestratorPermissions() method to find and remove generic orchestrator.workflow permissions Implemented restoreGenericOrchestratorPermissions() method to restore previously saved permissions Exported SavedRolePolicy interface for policy tracking +134/-0
Bug fix	orchestrator-rbac.spec.ts Integrate permission helper into orchestrator RBAC tests  e2e-tests/playwright/e2e/plugins/orchestrator/orchestrator-rbac.spec.ts Added import for OrchestratorRbacHelper class Added new test to remove generic orchestrator.workflow permissions before test execution Updated afterAll cleanup to restore removed generic orchestrator permissions Added clarifying comments about permission override behavior +20/-0    orchestrator-entity-rbac.spec.ts Integrate permission helper and increase test timeout        e2e-tests/playwright/e2e/plugins/orchestrator/orchestrator-entity-rbac.spec.ts Added import for OrchestratorRbacHelper class Extracted test user constant testUser = "user:default/rhdh-qe" for reusability Added new setup test to remove pre-existing generic orchestrator.workflow permissions Updated afterAll cleanup to restore removed generic orchestrator permissions Increased test timeout to 150 seconds for workflow execution test +27/-2

---

[testetson22]

/test e2e-ocp-helm-nightly

[rhdh-qodo-merge]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Modifying a shared user risks test flakiness The suggestion highlights the risk of test flakiness from modifying a shared user's permissions in parallel test suites. It recommends creating a dedicated, temporary user for each test to ensure isolation and prevent race conditions. Examples: e2e-tests/playwright/e2e/plugins/orchestrator/orchestrator-entity-rbac.spec.ts [26-264] const testUser = "user:default/rhdh-qe"; test.describe.serial("Orchestrator Entity-Workflow RBAC", () => { // TODO: https://issues.redhat.com/browse/RHDHBUGS-2184 fix orchestrator tests on Operator deployment test.fixme(() => skipIfJobName(JOB_NAME_PATTERNS.OPERATOR)); test.beforeAll(async ({}, testInfo) => { testInfo.annotations.push({ type: "component", description: "orchestrator", ... (clipped 229 lines) e2e-tests/playwright/e2e/plugins/orchestrator/orchestrator-rbac.spec.ts [477-500] const orchestratorRbacHelper = new OrchestratorRbacHelper(); test.beforeAll(async ({ browser }, testInfo) => { page = (await setupBrowser(browser, testInfo)).page; uiHelper = new UIhelper(page); common = new Common(page); await common.loginAsKeycloakUser(); apiToken = await RhdhAuthApiHack.getToken(page); ... (clipped 14 lines) Solution Walkthrough: Before: // In orchestrator-entity-rbac.spec.ts const testUser = "user:default/rhdh-qe"; const orchestratorRbacHelper = new OrchestratorRbacHelper(); test("Setup: Remove any pre-existing generic orchestrator.workflow permissions", async () => { await orchestratorRbacHelper.removeGenericOrchestratorPermissions(rbacApi, testUser); }); test.afterAll(async () => { await orchestratorRbacHelper.restoreGenericOrchestratorPermissions(rbacApi); }); // In orchestrator-rbac.spec.ts const orchestratorRbacHelper = new OrchestratorRbacHelper(); test("Remove any generic orchestrator.workflow permissions for test user", async () => { await orchestratorRbacHelper.removeGenericOrchestratorPermissions(rbacApi, "user:default/rhdh-qe"); }); After: // In each test file describe("Orchestrator RBAC", () => { let testUser: string; let userApi: UserApi; // Hypothetical API to manage users test.beforeAll(async () => { // Create a unique user for this test suite testUser = `user:default/temp-user-${randomId()}`; await userApi.createUser(testUser); // ... other setup }); test("Setup: Remove generic permissions", async () => { // Helper now operates on the isolated user await orchestratorRbacHelper.removeGenericOrchestratorPermissions(rbacApi, testUser); }); test.afterAll(async () => { // ... restore permissions and clean up user await userApi.deleteUser(testUser); }); }); Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical design flaw where modifying a shared user (user:default/rhdh-qe) across parallel tests can cause race conditions, leading to flaky tests and undermining the PR's goal of improving test stability.	High
Possible issue	Add missing roleName to restore call Pass the saved.roleName to the rbacApi.createPolicies call to ensure policies are restored to the correct role. e2e-tests/playwright/support/api/orchestrator-rbac-helper.ts [118] -const restoreResponse = await rbacApi.createPolicies(saved.policies); +const restoreResponse = await rbacApi.createPolicies(saved.roleName, saved.policies); Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion identifies a critical bug where the createPolicies call is missing the roleName, which would cause the policy restoration to fail and leave the test environment in a broken state.	High
	✅ Ensure permission restoration runs on cleanup Suggestion Impact: The restoreGenericOrchestratorPermissions call was moved into a finally block to ensure it always executes; additional error handling was added around the restoration step and error messages were refined. code diff: @@ -439,13 +439,17 @@ await rbacApi.deletePolicy(roleNameForApi, policies as Policy[]); await rbacApi.deleteRole(roleNameForApi); } - - // Restore any generic orchestrator policies that were removed - await orchestratorRbacHelper.restoreGenericOrchestratorPermissions( - rbacApi, - ); } catch (error) { - console.error("Error during cleanup:", error); + console.error("Error during role cleanup:", error); + } finally { + // Always restore generic orchestrator policies, even if role cleanup fails + try { + await orchestratorRbacHelper.restoreGenericOrchestratorPermissions( + rbacApi, + ); + } catch (restoreError) { + console.error("Error restoring orchestrator policies:", restoreError); + } } Move the permission restoration logic into a finally block within the afterAll hook to guarantee it executes even if the preceding test role cleanup fails. e2e-tests/playwright/e2e/plugins/orchestrator/orchestrator-entity-rbac.spec.ts [430-450] try { // Clean up the test role const roleNameForApi = roleName.replace("role:", ""); const policiesResponse = await rbacApi.getPoliciesByRole(roleNameForApi); if (policiesResponse.ok()) { const policies = await Response.removeMetadataFromResponse(policiesResponse); await rbacApi.deletePolicy(roleNameForApi, policies as Policy[]); await rbacApi.deleteRole(roleNameForApi); } - + } catch (error) { + console.error("Error during cleanup:", error); + } finally { // Restore any generic orchestrator policies that were removed await orchestratorRbacHelper.restoreGenericOrchestratorPermissions( rbacApi, ); - } catch (error) { - console.error("Error during cleanup:", error); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This correctly points out that cleanup logic can be skipped on error, and proposes using a finally block to ensure permission restoration always runs, which is crucial for test suite stability.	Medium
	✅ Prevent state corruption on deletion failure Suggestion Impact: The commit moved saving of generic orchestrator policies to occur only after a successful delete response, and added explicit error handling when deletion fails, aligning with the goal of preventing corrupted restoration state. code diff: if (genericOrchestratorPolicies.length > 0) { - // Save these policies for restoration later - this.savedGenericPolicies.push({ - roleName: roleNameForApi, - policies: genericOrchestratorPolicies.map( - (p: { permission: string; policy: string; effect: string }) => ({ - entityReference: role.name, - permission: p.permission, - policy: p.policy, - effect: p.effect, - }), - ), - }); - - // Remove the generic policies - const policiesToDelete = genericOrchestratorPolicies.map( - (p: { permission: string; policy: string; effect: string }) => ({ + const policiesToDelete: Policy[] = genericOrchestratorPolicies.map( + (p) => ({ entityReference: role.name, permission: p.permission, policy: p.policy, @@ -91,7 +129,18 @@ roleNameForApi, policiesToDelete, ); - expect(deleteResponse.ok()).toBeTruthy(); + + if (!deleteResponse.ok()) { + throw new Error( + `Failed to remove orchestrator policies from ${role.name}: ${await deleteResponse.text()}`, + ); + } + + // Only save policies after successful deletion + this.savedGenericPolicies.push({ + roleName: roleNameForApi, + policies: policiesToDelete, + }); } Refactor the logic to only save policies for restoration after they have been successfully deleted to prevent state corruption on failure. e2e-tests/playwright/support/api/orchestrator-rbac-helper.ts [62-95] if (genericOrchestratorPolicies.length > 0) { - // Save these policies for restoration later - this.savedGenericPolicies.push({ - roleName: roleNameForApi, - policies: genericOrchestratorPolicies.map( - (p: { permission: string; policy: string; effect: string }) => ({ - entityReference: role.name, - permission: p.permission, - policy: p.policy, - effect: p.effect, - }), - ), - }); - - // Remove the generic policies - const policiesToDelete = genericOrchestratorPolicies.map( + const policiesToProcess = genericOrchestratorPolicies.map( (p: { permission: string; policy: string; effect: string }) => ({ entityReference: role.name, permission: p.permission, policy: p.policy, effect: p.effect, }), ); console.log( `Removing generic orchestrator policies from ${role.name}:`, - policiesToDelete, + policiesToProcess, ); const deleteResponse = await rbacApi.deletePolicy( roleNameForApi, - policiesToDelete, + policiesToProcess, ); expect(deleteResponse.ok()).toBeTruthy(); + + // Only save policies after successful deletion + this.savedGenericPolicies.push({ + roleName: roleNameForApi, + policies: policiesToProcess, + }); } [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential state corruption issue in the test helper, where policies could be saved for restoration but fail to be deleted, leading to inconsistent test cleanup.	Medium
General	✅ Replace in-helper expect with throw Suggestion Impact: Removed the Playwright `expect` import and replaced in-helper `expect(...).toBeTruthy()` checks with explicit `throw new Error(...)` when API responses are not ok (including the deleteResponse case highlighted in the suggestion). code diff: @@ -1,4 +1,3 @@ -import { expect } from "@playwright/test"; import RhdhRbacApi from "./rbac-api"; import { Policy } from "./rbac-api-structures"; @@ -11,6 +10,16 @@ } /** + * Represents the expected shape of a policy from the RBAC API response. + */ +interface ApiPolicy { + permission: string; + policy: string; + effect: string; + metadata?: unknown; +} + +/** * Helper class for managing orchestrator RBAC policies during tests. * * This is needed because generic orchestrator.workflow permissions override @@ -20,6 +29,41 @@ */ export class OrchestratorRbacHelper { private savedGenericPolicies: SavedRolePolicy[] = []; + + /** + * Validates that a policy object has the required fields. + */ + private isValidPolicy(policy: unknown): policy is ApiPolicy { + if (typeof policy !== "object" || policy === null) return false; + const p = policy as Record<string, unknown>; + return ( + typeof p.permission === "string" && + typeof p.policy === "string" && + typeof p.effect === "string" + ); + } + + /** + * Removes metadata from policy objects and validates their shape. + * Similar to Response.removeMetadataFromResponse pattern used elsewhere. + */ + private cleanAndValidatePolicies(policies: unknown[]): ApiPolicy[] { + const validPolicies: ApiPolicy[] = []; + for (const policy of policies) { + if (!this.isValidPolicy(policy)) { + console.warn( + `Skipping invalid policy object: ${JSON.stringify(policy)}`, + ); + continue; + } + validPolicies.push({ + permission: policy.permission, + policy: policy.policy, + effect: policy.effect, + }); + } + return validPolicies; + } /** * Removes any generic orchestrator.workflow permissions for the specified user. @@ -35,9 +79,10 @@ ): Promise<SavedRolePolicy[]> { this.savedGenericPolicies = []; - // Get all roles that the user is a member of const rolesResponse = await rbacApi.getRoles(); - expect(rolesResponse.ok()).toBeTruthy(); + if (!rolesResponse.ok()) { + throw new Error(`Failed to get roles: ${await rolesResponse.text()}`); + } const roles = await rolesResponse.json(); const userRoles = roles.filter( @@ -45,37 +90,30 @@ role.memberReferences?.includes(userEntityRef), ); - // For each role, check if it has generic orchestrator.workflow permissions for (const role of userRoles) { const roleNameForApi = role.name.replace("role:", ""); const policiesResponse = await rbacApi.getPoliciesByRole(roleNameForApi); if (!policiesResponse.ok()) continue; - const policies = await policiesResponse.json(); - const genericOrchestratorPolicies = policies.filter( - (policy: { permission: string }) => + const rawPolicies = await policiesResponse.json(); + if (!Array.isArray(rawPolicies)) { + console.warn( + `Expected array of policies for ${role.name}, got: ${typeof rawPolicies}`, + ); + continue; + } + + const validPolicies = this.cleanAndValidatePolicies(rawPolicies); + const genericOrchestratorPolicies = validPolicies.filter( + (policy) => policy.permission === "orchestrator.workflow" || policy.permission === "orchestrator.workflow.use", ); if (genericOrchestratorPolicies.length > 0) { - // Save these policies for restoration later - this.savedGenericPolicies.push({ - roleName: roleNameForApi, - policies: genericOrchestratorPolicies.map( - (p: { permission: string; policy: string; effect: string }) => ({ - entityReference: role.name, - permission: p.permission, - policy: p.policy, - effect: p.effect, - }), - ), - }); - - // Remove the generic policies - const policiesToDelete = genericOrchestratorPolicies.map( - (p: { permission: string; policy: string; effect: string }) => ({ + const policiesToDelete: Policy[] = genericOrchestratorPolicies.map( + (p) => ({ entityReference: role.name, permission: p.permission, policy: p.policy, @@ -91,7 +129,18 @@ roleNameForApi, policiesToDelete, ); - expect(deleteResponse.ok()).toBeTruthy(); + + if (!deleteResponse.ok()) { + throw new Error( + `Failed to remove orchestrator policies from ${role.name}: ${await deleteResponse.text()}`, + ); + } Replace the expect call inside the helper method with a thrown Error to provide clearer stack traces and decouple the helper from the test framework. e2e-tests/playwright/support/api/orchestrator-rbac-helper.ts [94] -expect(deleteResponse.ok()).toBeTruthy(); +if (!deleteResponse.ok()) { + throw new Error(`Failed to remove orchestrator policies from ${role.name}`); +} [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: This is a good code quality improvement that decouples the helper logic from the test framework's expect assertions, leading to clearer error messages and better maintainability.	Low
	✅ Reset helper state after restoration Suggestion Impact: The restoreGenericOrchestratorPermissions method now resets internal state by setting this.savedGenericPolicies = [] after attempting restoration, preventing reuse/accidental re-application (and additionally aggregates restoration errors and throws after reset). code diff: async restoreGenericOrchestratorPermissions( rbacApi: RhdhRbacApi, ): Promise<void> { + const errors: string[] = []; + for (const saved of this.savedGenericPolicies) { console.log( `Restoring generic orchestrator policies to ${saved.roleName}:`, @@ -117,11 +170,20 @@ ); const restoreResponse = await rbacApi.createPolicies(saved.policies); if (!restoreResponse.ok()) { - console.error( - `Failed to restore policies to ${saved.roleName}:`, - await restoreResponse.text(), + const errorText = await restoreResponse.text(); + errors.push( + `Failed to restore policies to ${saved.roleName}: ${errorText}`, ); } + } + + // Reset state after restoration attempt + this.savedGenericPolicies = []; + + if (errors.length > 0) { + throw new Error( + `Policy restoration failed. Environment may be in inconsistent state:\n${errors.join("\n")}`, + ); } After restoring policies in restoreGenericOrchestratorPermissions, clear the savedGenericPolicies array to reset the helper's state and prevent accidental re-application. e2e-tests/playwright/support/api/orchestrator-rbac-helper.ts [110-126] async restoreGenericOrchestratorPermissions( rbacApi: RhdhRbacApi, ): Promise<void> { for (const saved of this.savedGenericPolicies) { console.log( `Restoring generic orchestrator policies to ${saved.roleName}:`, saved.policies, ); const restoreResponse = await rbacApi.createPolicies(saved.policies); if (!restoreResponse.ok()) { console.error( `Failed to restore policies to ${saved.roleName}:`, await restoreResponse.text(), ); } } + this.savedGenericPolicies = []; } [Suggestion processed] Suggestion importance[1-10]: 4 __ Why: This is a good practice suggestion that improves the helper's robustness by resetting its internal state after use, preventing potential errors if the helper is reused.	Low
Update

[github-actions]

The container image build workflow finished with status: cancelled.

[testetson22]

/test e2e-ocp-helm-nightly

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4453
• quay.io/rhdh-community/rhdh:pr-4453-c1374276

[testetson22]

/test e2e-ocp-helm-nightly

[github-actions]

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[testetson22]

/retest

[openshift-ci]

@testetson22: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-ocp-helm-nightly	04ef44a	link	false	/test e2e-ocp-helm-nightly

Full PR test history. Your PR dashboard.

Details

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.