From f562dfeefa76781d63eb85b048eb9085bfac13cb Mon Sep 17 00:00:00 2001 From: Norman Abramovitz Date: Sat, 4 Jul 2026 18:31:16 -0700 Subject: [PATCH 1/2] fix(permissions): let the CF checker disown non-CF permission configs The strict-null pass in #5446 replaced CfUserPermissionsChecker's implicit "this config is not mine" (returning undefined from getBaseSimpleCheck's missing default case) with an explicit of(false) denial. That made the CF checker claim every stratos-level check (internal, internal-scope, api-key) alongside StratosUserPermissionsChecker, so findChecker() saw two claimants and denied the permission app-wide ("Permissions: Found too many ..."). Every stratos permission was denied for every user: the Register Endpoint button never rendered for admins (#5570), the /api-keys route guard bounced admins to home (#5571), and password-change / profile-edit / endpoint-admin gating were silently off. Restore the contract explicitly: getSimpleCheck returns undefined for non-CF permission types, and getComplexCheck returns null (not a truthy empty array) when it handles none of the config groups. The new spec pins the checker contract and the two-checkers-registered integration paths that #5446's gate missed. Fixes #5570 Fixes #5571 --- .../cf-user-permissions-checkers.spec.ts | 116 ++++++++++++++++++ .../cf-user-permissions-checkers.ts | 33 ++++- 2 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.spec.ts diff --git a/src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.spec.ts b/src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.spec.ts new file mode 100644 index 0000000000..526bd849e3 --- /dev/null +++ b/src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.spec.ts @@ -0,0 +1,116 @@ +import { HttpClient } from '@angular/common/http'; +import { provideZonelessChangeDetection } from '@angular/core'; +import { TestBed } from '@angular/core/testing'; +import { + CUSTOM_USER_PERMISSION_CHECKERS, + CurrentUserPermissionsService, + CurrentUserRolesSignalService, + PermissionConfig, + PermissionTypes, + StratosCurrentUserPermissions, +} from '@stratosui/core'; +import { EndpointsDataService, SessionData } from '@stratosui/store'; +import { firstValueFrom, of } from 'rxjs'; +import { describe, expect, it, beforeEach } from 'vitest'; + +import { CfCurrentUserRolesSignalService } from './cf-current-user-roles-signal.service'; +import { CfUserPermissionsChecker } from './cf-user-permissions-checkers'; + +/** + * Checker contract (#5570 / #5571): a checker signals "this config is not + * mine" by returning undefined (simple) / null (complex / fallback). Returning + * a denial observable instead makes CurrentUserPermissionsService.findChecker + * see two claimants for every Stratos-level check ("Found too many ... + * Permission Denied"), which denied every internal / internal-scope / api-key + * permission app-wide (Register Endpoint button, /api-keys route guard, ...). + * + * Stratos permission types are spelled as literals here because the enum is + * core-internal; the values are pinned by stratosPermissionConfigs. + */ +const STRATOS_INTERNAL = 'internal' as PermissionTypes; +const STRATOS_INTERNAL_SCOPE = 'internal-scope' as PermissionTypes; +const STRATOS_API_KEY = 'api-key' as PermissionTypes; + +describe('CfUserPermissionsChecker contract', () => { + let checker: CfUserPermissionsChecker; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [ + provideZonelessChangeDetection(), + { provide: CfCurrentUserRolesSignalService, useValue: {} }, + { provide: HttpClient, useValue: {} }, + CfUserPermissionsChecker, + ], + }); + checker = TestBed.inject(CfUserPermissionsChecker); + }); + + it('returns undefined from getSimpleCheck for non-CF permission types', () => { + expect(checker.getSimpleCheck(new PermissionConfig(STRATOS_INTERNAL, 'isAdmin'))).toBeUndefined(); + expect(checker.getSimpleCheck(new PermissionConfig(STRATOS_INTERNAL_SCOPE, 'password.write'))).toBeUndefined(); + expect(checker.getSimpleCheck(new PermissionConfig(STRATOS_API_KEY, ''))).toBeUndefined(); + }); + + it('returns null from getComplexCheck when it handles none of the config groups', () => { + const stratosOnlyGroup = [ + new PermissionConfig(STRATOS_INTERNAL_SCOPE, 'password.write'), + new PermissionConfig(STRATOS_INTERNAL_SCOPE, 'scim.write'), + ]; + expect(checker.getComplexCheck(stratosOnlyGroup)).toBeNull(); + }); + + it('returns null from getPermissionConfig for stratos-owned keys', () => { + expect(checker.getPermissionConfig(StratosCurrentUserPermissions.API_KEYS)).toBeFalsy(); + }); +}); + +describe('CurrentUserPermissionsService with both checkers registered', () => { + function setup(sessionData: Partial | null, isAdmin: boolean) { + TestBed.configureTestingModule({ + providers: [ + provideZonelessChangeDetection(), + { provide: CfCurrentUserRolesSignalService, useValue: {} }, + { provide: HttpClient, useValue: {} }, + { provide: EndpointsDataService, useValue: {} }, + { + provide: CurrentUserRolesSignalService, + useValue: { + stratosRole$: (role: string) => of(role === 'isAdmin' && isAdmin), + stratosHasScope$: () => of(false), + sessionData$: () => of(sessionData as SessionData), + }, + }, + CfUserPermissionsChecker, + { + provide: CUSTOM_USER_PERMISSION_CHECKERS, + useFactory: (cfChecker: CfUserPermissionsChecker) => [cfChecker], + deps: [CfUserPermissionsChecker], + }, + CurrentUserPermissionsService, + ], + }); + return TestBed.inject(CurrentUserPermissionsService); + } + + it('resolves stratos-admin permission for an admin (#5570 Register Endpoint)', async () => { + const service = setup(null, true); + await expect( + firstValueFrom(service.can(StratosCurrentUserPermissions.EDIT_ADMIN_ENDPOINT)), + ).resolves.toBe(true); + }); + + it('resolves api-keys permission for an admin when APIKeysEnabled=admin_only (#5571 route guard)', async () => { + const service = setup({ config: { APIKeysEnabled: 'admin_only' } } as Partial, true); + await expect( + firstValueFrom(service.can(StratosCurrentUserPermissions.API_KEYS)), + ).resolves.toBe(true); + }); + + it('still denies stratos-admin permission for a non-admin', async () => { + const service = setup(null, false); + await expect( + firstValueFrom(service.can(StratosCurrentUserPermissions.EDIT_ADMIN_ENDPOINT)), + ).resolves.toBe(false); + }); +}); diff --git a/src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.ts b/src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.ts index 1fda525a1f..be9c679754 100644 --- a/src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.ts +++ b/src/frontend/packages/cloud-foundry/src/user-permissions/cf-user-permissions-checkers.ts @@ -162,15 +162,28 @@ export class CfUserPermissionsChecker extends BaseCurrentUserPermissionsChecker /** * @param permissionConfig Single permission to be checked */ - public getSimpleCheck(permissionConfig: PermissionConfig, endpointGuid?: string, orgOrSpaceGuid?: string, spaceGuid?: string) { + public getSimpleCheck( + permissionConfig: PermissionConfig, + endpointGuid?: string, + orgOrSpaceGuid?: string, + spaceGuid?: string + ): Observable | undefined { const check$ = this.getBaseSimpleCheck(permissionConfig, endpointGuid, orgOrSpaceGuid, spaceGuid); + if (!check$) { + return undefined; + } if (permissionConfig.type === CfPermissionTypes.ORGANIZATION || permissionConfig.type === CfPermissionTypes.SPACE) { return this.applyAdminCheck(check$, endpointGuid); } return check$; } - private getBaseSimpleCheck(permissionConfig: PermissionConfig, endpointGuid?: string, orgOrSpaceGuid?: string, spaceGuid?: string) { + private getBaseSimpleCheck( + permissionConfig: PermissionConfig, + endpointGuid?: string, + orgOrSpaceGuid?: string, + spaceGuid?: string + ): Observable | undefined { switch (permissionConfig.type) { case (CfPermissionTypes.FEATURE_FLAG): return this.getFeatureFlagCheck(permissionConfig, endpointGuid); @@ -181,8 +194,12 @@ export class CfUserPermissionsChecker extends BaseCurrentUserPermissionsChecker case (CfPermissionTypes.ENDPOINT_SCOPE): return this.getEndpointScopesCheck(permissionConfig.permission as CfScopeStrings, endpointGuid); default: - // Unknown permission type — deny rather than emit an undefined stream. - return of(false); + // Not a CF permission type: returning undefined tells + // CurrentUserPermissionsService this checker does not own the config. + // Returning a denial stream here instead makes findChecker() count two + // claimants for every stratos-level check and deny it app-wide (#5570, + // #5571). + return undefined; } } @@ -382,9 +399,9 @@ export class CfUserPermissionsChecker extends BaseCurrentUserPermissionsChecker endpointGuid?: string, orgOrSpaceGuid?: string, spaceGuid?: string - ): IPermissionCheckCombiner[] { + ): IPermissionCheckCombiner[] | null { const groupedChecks = this.groupConfigs(permissionConfigs); - return Object.keys(groupedChecks) + const combiners = Object.keys(groupedChecks) .map((permission: PermissionTypes) => { const configGroup = groupedChecks[permission]; const checkCombiner = this.getBaseCheckFromConfig(configGroup, permission, endpointGuid, orgOrSpaceGuid, spaceGuid); @@ -394,6 +411,10 @@ export class CfUserPermissionsChecker extends BaseCurrentUserPermissionsChecker return checkCombiner; }) .filter((checkCombiner): checkCombiner is IPermissionCheckCombiner => !!checkCombiner); + // No group handled → this checker does not own the config; an empty array + // is truthy and would double-claim the check in findChecker() (see + // getBaseSimpleCheck's default case). + return combiners.length ? combiners : null; } From 515e752c1b4f7a40d4f4cca76917f3960cb9049e Mon Sep 17 00:00:00 2001 From: Norman Abramovitz Date: Sat, 4 Jul 2026 18:31:26 -0700 Subject: [PATCH 2/2] test(e2e): teach the list harness signal-list markup The api-keys CRUD tests were unreachable while the route guard bounced admins off /api-keys (#5571); with the guard fixed they run again and hit three stale legacy selectors in the shared list harness: - ListComponent's default root only matched the legacy app-list; migrated pages render app-signal-list. Match either (a page has exactly one). - Header parsing read sortable th text verbatim, which includes the material-icons sort ligature ("Description sort"), so findRow() never matched a column. Strip icon elements before reading header text. - Row-action menus: signal-list renders data-test="row-actions" / "row-actions-menu" instead of the app-table-cell-actions markup; accept both in openRowActionMenuByRow and the api-keys delete step. api-keys.spec.ts now passes 10/10 including the delete flow, which had always self-skipped ("key row not found"). --- e2e/components/list.component.ts | 23 +++++++++++++++++------ e2e/tests/core/api-keys.spec.ts | 3 ++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/e2e/components/list.component.ts b/e2e/components/list.component.ts index d8ff63567c..0283267e3f 100644 --- a/e2e/components/list.component.ts +++ b/e2e/components/list.component.ts @@ -57,9 +57,15 @@ export class ListTableComponent { } async getTableData(): Promise { - // Read headers from app-table-cell inside each header cell to avoid sort icon text + // Read headers from app-table-cell inside each header cell to avoid sort icon text. + // Signal-list headers put the sort icon (a material-icons ligature, e.g. "sort") + // inside the th text, so strip icon elements before reading. const headerCells = await this.table.locator('.app-table__header-cell app-table-cell, th').all(); - const headers = await Promise.all(headerCells.map(h => h.textContent().then(t => (t || '').trim()))); + const headers = await Promise.all(headerCells.map(h => h.evaluate(el => { + const clone = el.cloneNode(true) as HTMLElement; + clone.querySelectorAll('.material-icons, mat-icon').forEach(icon => icon.remove()); + return (clone.textContent || '').trim(); + }))); const rows = await this.getRows().all(); @@ -123,12 +129,15 @@ export class ListTableComponent { async openRowActionMenuByRow(row: Locator): Promise { // Target the overflow menu button (more_vert btn-icon), not inline action buttons - const actionButton = row.locator('app-table-cell-actions button.btn-icon, button[aria-label="Actions"]').first(); + const actionButton = row.locator( + 'app-table-cell-actions button.btn-icon, button[aria-label="Actions"], button[data-test="row-actions"]', + ).first(); await actionButton.click(); - // Wait for either Angular Material menu or the custom table-cell-actions dropdown + // Wait for the Angular Material menu, the custom table-cell-actions + // dropdown, or the signal-list row-actions menu const matMenu = this.page.locator('.mat-menu-content, .mat-mdc-menu-content'); - const customMenu = row.locator('.table-cell-actions-menu--open'); + const customMenu = row.locator('.table-cell-actions-menu--open, [data-test="row-actions-menu"]'); await Promise.any([ matMenu.waitFor({ state: 'visible', timeout: 5000 }), customMenu.waitFor({ state: 'visible', timeout: 5000 }), @@ -422,7 +431,9 @@ export class ListComponent { public locator: Locator; constructor(private page: Page, locator?: Locator) { - this.locator = locator || page.locator('app-list').first(); + // Pages are migrating from the legacy app-list to app-signal-list; a page + // renders exactly one of the two, so match either. + this.locator = locator || page.locator('app-list, app-signal-list').first(); this.table = new ListTableComponent(page, this.locator); this.cards = new ListCardComponent(page, this.locator); this.header = new ListHeaderComponent(page, this.locator); diff --git a/e2e/tests/core/api-keys.spec.ts b/e2e/tests/core/api-keys.spec.ts index 262646ecb8..603205857f 100644 --- a/e2e/tests/core/api-keys.spec.ts +++ b/e2e/tests/core/api-keys.spec.ts @@ -162,7 +162,8 @@ test.describe('API Keys', () => { await apiKeysPage.list.table.openRowActionMenuByIndex(rowIndex); // Click Delete within the opened menu row only (not across all rows) - await sharedPage.locator('.table-cell-actions-menu--open button').filter({ hasText: 'Delete' }).click(); + await sharedPage.locator('.table-cell-actions-menu--open button, [data-test="row-actions-menu"] button') + .filter({ hasText: 'Delete' }).click(); await ConfirmDialogComponent.expectDialogAndConfirm(sharedPage, 'Delete', 'Delete Key');