From 2d20fda207d06b14acbfb16536ea2ff877f70107 Mon Sep 17 00:00:00 2001 From: Kevin Loftus Date: Thu, 9 Jul 2026 12:31:55 -0400 Subject: [PATCH] fix: workflow access bugs --- .../workflow/list-private-workflows.lambda.ts | 35 +++- .../pipeline/list-pipelines.lambda.ts | 69 +++++++- ...atory-workflow-access-default-migration.ts | 152 +++++++----------- .../unified-workflow-catalog-service.ts | 46 +----- .../list-private-workflows.lambda.test.ts | 32 ++-- ...-workflow-access-default-migration.test.ts | 24 +-- 6 files changed, 192 insertions(+), 166 deletions(-) diff --git a/packages/back-end/src/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.ts b/packages/back-end/src/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.ts index 1edfff1e4..c296575ae 100644 --- a/packages/back-end/src/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.ts +++ b/packages/back-end/src/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.ts @@ -23,6 +23,31 @@ const laboratoryService = new LaboratoryService(); const omicsService = new OmicsService(); const laboratoryWorkflowAccessService = new LaboratoryWorkflowAccessService(); +/** + * Retrieves every PRIVATE workflow across all pages so the workflow-access filter + * is applied to the complete set. Fetching only a single page would hide any + * granted workflow that happens to fall beyond the first page from every user, + * even though it appears in the (fully paginated) admin workflow catalog. + */ +async function listAllPrivateWorkflows(name?: string) { + const items: NonNullable>['items']> = []; + let nextToken: string | undefined; + do { + const page = await omicsService.listWorkflows({ + type: 'PRIVATE', + maxResults: 100, + startingToken: nextToken, + status: undefined, // Explicitly exclude status filter for Workflows + ...(name ? { name } : {}), + }); + if (page.items?.length) { + items.push(...page.items); + } + nextToken = page.nextToken; + } while (nextToken); + return items; +} + /** * This GET /aws-healthomics/workflow/list-private-workflows?laboratoryId={LaboratoryId} * API queries the same region's AWS HealthOmics service to retrieve a list of @@ -68,18 +93,14 @@ export const handler: Handler = async ( } const queryParameters: AwsHealthOmicsQueryParameters = getAwsHealthOmicsApiQueryParameters(event); - const response = await omicsService.listWorkflows({ - type: 'PRIVATE', - ...queryParameters, - status: undefined, // Explicitly exclude status filter for Workflows - }); + const allItems = await listAllPrivateWorkflows(queryParameters.name); const accessRows = await laboratoryWorkflowAccessService.listByLaboratoryId(laboratoryId); - const items = (response.items ?? []).filter( + const items = allItems.filter( (w) => w.id != null && isWorkflowAccessAllowed(laboratory, accessRows, 'HEALTH_OMICS', w.id), ); - return buildResponse(200, JSON.stringify({ ...response, items }), event); + return buildResponse(200, JSON.stringify({ items }), event); } catch (err: any) { console.error(err); return buildErrorResponse(err, event); diff --git a/packages/back-end/src/app/controllers/nf-tower/pipeline/list-pipelines.lambda.ts b/packages/back-end/src/app/controllers/nf-tower/pipeline/list-pipelines.lambda.ts index 68f8dc028..5dce3ae7c 100644 --- a/packages/back-end/src/app/controllers/nf-tower/pipeline/list-pipelines.lambda.ts +++ b/packages/back-end/src/app/controllers/nf-tower/pipeline/list-pipelines.lambda.ts @@ -25,6 +25,60 @@ const laboratoryService = new LaboratoryService(); const ssmService = new SsmService(); const laboratoryWorkflowAccessService = new LaboratoryWorkflowAccessService(); +/** + * Retrieves every Seqera pipeline across all pages (preserving any caller-supplied + * search / workspaceId params) so the workflow-access filter is applied to the full + * set rather than a single page. + */ +async function listAllPipelines( + seqeraApiBaseUrl: string, + apiQueryParameters: string, + accessToken: string, +): Promise { + const pipelines: NonNullable = []; + const seen = new Set(); + let firstResponse: ListPipelinesResponse | undefined; + let offset = 0; + const max = 100; + + for (;;) { + const params = new URLSearchParams(apiQueryParameters); + params.set('max', String(max)); + params.set('offset', String(offset)); + + const response: ListPipelinesResponse = await httpRequest( + `${seqeraApiBaseUrl}/pipelines?${params.toString()}`, + REST_API_METHOD.GET, + { Authorization: `Bearer ${accessToken}` }, + ); + if (!firstResponse) { + firstResponse = response; + } + + const page = response.pipelines ?? []; + if (!page.length) { + break; + } + for (const p of page) { + const id = p.pipelineId != null ? String(p.pipelineId) : undefined; + if (id != null && seen.has(id)) { + continue; + } + if (id != null) { + seen.add(id); + } + pipelines.push(p); + } + + offset += page.length; + if (page.length < max || (response.totalSize != null && offset >= response.totalSize)) { + break; + } + } + + return { ...(firstResponse ?? {}), pipelines, totalSize: pipelines.length }; +} + /** * This GET /nf-tower/pipeline/list-pipelines?laboratoryId={LaboratoryId} API * queries the NextFlow Tower GET /pipelines?workspaceId={WorkspaceId} API for a @@ -90,20 +144,21 @@ export const handler: Handler = async ( // Get Seqera API Base URL for Laboratory or default to platform-wide configured Seqera API Base URL const seqeraApiBaseUrl: string = laboratory.NextFlowTowerApiBaseUrl || process.env.SEQERA_API_BASE_URL; - // Get Query Parameters for Seqera Cloud / NextFlow Tower APIs + // Get Query Parameters for Seqera Cloud / NextFlow Tower APIs (preserves search + workspaceId) const apiQueryParameters: string = getNextFlowApiQueryParameters(event, laboratory.NextFlowTowerWorkspaceId); - const response: ListPipelinesResponse = await httpRequest( - `${seqeraApiBaseUrl}/pipelines?${apiQueryParameters}`, - REST_API_METHOD.GET, - { Authorization: `Bearer ${accessToken}` }, - ); + + // Fetch every page of pipelines before applying the workflow-access filter. + // Filtering only the first page would hide any granted pipeline beyond that + // page from every user, even though it appears in the (fully paginated) + // admin workflow catalog. + const response: ListPipelinesResponse = await listAllPipelines(seqeraApiBaseUrl, apiQueryParameters, accessToken); const accessRows = await laboratoryWorkflowAccessService.listByLaboratoryId(laboratoryId); const pipelines = (response.pipelines ?? []).filter( (p) => p.pipelineId != null && isWorkflowAccessAllowed(laboratory, accessRows, 'SEQERA', String(p.pipelineId)), ); - return buildResponse(200, JSON.stringify({ ...response, pipelines }), event); + return buildResponse(200, JSON.stringify({ ...response, pipelines, totalSize: pipelines.length }), event); } catch (err: any) { console.error(err); return buildErrorResponse(err, event); diff --git a/packages/back-end/src/app/services/easy-genomics/laboratory-workflow-access-default-migration.ts b/packages/back-end/src/app/services/easy-genomics/laboratory-workflow-access-default-migration.ts index bd2728d7a..f08e98819 100644 --- a/packages/back-end/src/app/services/easy-genomics/laboratory-workflow-access-default-migration.ts +++ b/packages/back-end/src/app/services/easy-genomics/laboratory-workflow-access-default-migration.ts @@ -1,7 +1,10 @@ -import type { UnifiedWorkflowCatalogEntry, LaboratoryWorkflowAccessPlatform } from '@easy-genomics/shared-lib/src/app/types/easy-genomics/laboratory-workflow-access'; +import type { + UnifiedWorkflowCatalogEntry, + LaboratoryWorkflowAccessPlatform, +} from '@easy-genomics/shared-lib/src/app/types/easy-genomics/laboratory-workflow-access'; import { LaboratoryWorkflowAccessService } from '@BE/services/easy-genomics/laboratory-workflow-access-service'; import { buildUnifiedWorkflowCatalogForOrganization } from '@BE/services/easy-genomics/unified-workflow-catalog-service'; -import { parseWorkflowAccessSortKey, rowIsAllow, rowIsDeny } from '@BE/utils/laboratory-workflow-access-utils'; +import { parseWorkflowAccessSortKey, rowIsDeny } from '@BE/utils/laboratory-workflow-access-utils'; const CHUNK = 20; @@ -31,116 +34,81 @@ export async function migrateWorkflowAccessOnDefaultModeChange(params: { } const accessService = new LaboratoryWorkflowAccessService(); - const catalog = await buildUnifiedWorkflowCatalogForOrganization(organizationId); - const catalogKeySet = new Set(catalog.map(catalogEntryToApiKey)); - const rows = await accessService.listByLaboratoryId(laboratoryId); if (!previousDefaultOn && nextDefaultOn) { - const allowKeys = new Set(); + // Strict -> default-on: as documented in the admin UI, turning "enable new + // workflows by default" on means workflows are allowed unless explicitly + // denied. Any rows left over from strict mode must be cleared to land on a + // clean "allowed by default" state: + // - strict-mode ALLOW rows are redundant here (default-on ignores ALLOW), and + // - DENY rows are inert under strict mode, so any that exist are stale/spurious + // (e.g. written by an earlier strict->on migration) and would otherwise hide + // every workflow from all lab users despite the toggle reading "on". + // We deliberately do NOT create DENY rows for the catalog here — doing so was + // the cause of "default on, but nobody can see any workflows". for (const row of rows) { - if (!rowIsAllow(row)) { - continue; - } const parsed = parseWorkflowAccessSortKey(row.WorkflowKey); if (!parsed) { continue; } - allowKeys.add(`${parsed.platform}::${parsed.workflowId}`); + await accessService.remove(laboratoryId, parsed.platform, parsed.workflowId); } + return; + } - const denyUpserts: UnifiedWorkflowCatalogEntry[] = []; - for (const entry of catalog) { - const k = catalogEntryToApiKey(entry); - if (!allowKeys.has(k)) { - denyUpserts.push(entry); - } - } + // default-on -> strict: preserve current visibility by ALLOW-listing every + // catalog workflow that isn't explicitly denied, then drop the DENY rows so the + // lab does not suddenly hide everything when switching into strict mode. + const catalog = await buildUnifiedWorkflowCatalogForOrganization(organizationId); - for (let i = 0; i < denyUpserts.length; i += CHUNK) { - const slice = denyUpserts.slice(i, i + CHUNK); - await Promise.all( - slice.map((entry) => { - const plat: LaboratoryWorkflowAccessPlatform = entry.platform === 'HealthOmics' ? 'HEALTH_OMICS' : 'SEQERA'; - return accessService.upsert({ - LaboratoryId: laboratoryId, - WorkflowKey: `${plat}#${entry.workflowId}`, - OrganizationId: organizationId, - WorkflowName: entry.name, - Effect: 'DENY', - }); - }), - ); - if (i + CHUNK < denyUpserts.length) { - await sleep(0); - } + const denyKeys = new Set(); + for (const row of rows) { + if (!rowIsDeny(row)) { + continue; } - - for (const row of rows) { - if (!rowIsAllow(row)) { - continue; - } - const parsed = parseWorkflowAccessSortKey(row.WorkflowKey); - if (!parsed) { - continue; - } - const k = `${parsed.platform}::${parsed.workflowId}`; - if (catalogKeySet.has(k)) { - await accessService.remove(laboratoryId, parsed.platform, parsed.workflowId); - } + const parsed = parseWorkflowAccessSortKey(row.WorkflowKey); + if (!parsed) { + continue; } - return; + denyKeys.add(`${parsed.platform}::${parsed.workflowId}`); } - if (previousDefaultOn && !nextDefaultOn) { - const denyKeys = new Set(); - for (const row of rows) { - if (!rowIsDeny(row)) { - continue; - } - const parsed = parseWorkflowAccessSortKey(row.WorkflowKey); - if (!parsed) { - continue; - } - denyKeys.add(`${parsed.platform}::${parsed.workflowId}`); + const allowUpserts: UnifiedWorkflowCatalogEntry[] = []; + for (const entry of catalog) { + const k = catalogEntryToApiKey(entry); + if (!denyKeys.has(k)) { + allowUpserts.push(entry); } + } - const allowUpserts: UnifiedWorkflowCatalogEntry[] = []; - for (const entry of catalog) { - const k = catalogEntryToApiKey(entry); - if (!denyKeys.has(k)) { - allowUpserts.push(entry); - } + for (let i = 0; i < allowUpserts.length; i += CHUNK) { + const slice = allowUpserts.slice(i, i + CHUNK); + await Promise.all( + slice.map((entry) => { + const plat: LaboratoryWorkflowAccessPlatform = entry.platform === 'HealthOmics' ? 'HEALTH_OMICS' : 'SEQERA'; + return accessService.upsert({ + LaboratoryId: laboratoryId, + WorkflowKey: `${plat}#${entry.workflowId}`, + OrganizationId: organizationId, + WorkflowName: entry.name, + Effect: 'ALLOW', + }); + }), + ); + if (i + CHUNK < allowUpserts.length) { + await sleep(0); } + } - for (let i = 0; i < allowUpserts.length; i += CHUNK) { - const slice = allowUpserts.slice(i, i + CHUNK); - await Promise.all( - slice.map((entry) => { - const plat: LaboratoryWorkflowAccessPlatform = entry.platform === 'HealthOmics' ? 'HEALTH_OMICS' : 'SEQERA'; - return accessService.upsert({ - LaboratoryId: laboratoryId, - WorkflowKey: `${plat}#${entry.workflowId}`, - OrganizationId: organizationId, - WorkflowName: entry.name, - Effect: 'ALLOW', - }); - }), - ); - if (i + CHUNK < allowUpserts.length) { - await sleep(0); - } + for (const row of rows) { + if (!rowIsDeny(row)) { + continue; } - - for (const row of rows) { - if (!rowIsDeny(row)) { - continue; - } - const parsed = parseWorkflowAccessSortKey(row.WorkflowKey); - if (!parsed) { - continue; - } - await accessService.remove(laboratoryId, parsed.platform, parsed.workflowId); + const parsed = parseWorkflowAccessSortKey(row.WorkflowKey); + if (!parsed) { + continue; } + await accessService.remove(laboratoryId, parsed.platform, parsed.workflowId); } } diff --git a/packages/back-end/src/app/services/easy-genomics/unified-workflow-catalog-service.ts b/packages/back-end/src/app/services/easy-genomics/unified-workflow-catalog-service.ts index a624e5628..edc4a0d4c 100644 --- a/packages/back-end/src/app/services/easy-genomics/unified-workflow-catalog-service.ts +++ b/packages/back-end/src/app/services/easy-genomics/unified-workflow-catalog-service.ts @@ -1,5 +1,5 @@ import type { WorkflowListItem } from '@aws-sdk/client-omics'; -import { ListSharesCommandInput, ListWorkflowsCommandInput } from '@aws-sdk/client-omics'; +import { ListWorkflowsCommandInput } from '@aws-sdk/client-omics'; import { GetParameterCommandOutput, ParameterNotFound } from '@aws-sdk/client-ssm'; import type { ListPipelinesResponse } from '@easy-genomics/shared-lib/lib/app/types/nf-tower/nextflow-tower-api'; import { Laboratory } from '@easy-genomics/shared-lib/src/app/types/easy-genomics/laboratory'; @@ -7,7 +7,6 @@ import type { UnifiedWorkflowCatalogEntry } from '@easy-genomics/shared-lib/src/ import { LaboratoryService } from '@BE/services/easy-genomics/laboratory-service'; import { OmicsService } from '@BE/services/omics-service'; import { SsmService } from '@BE/services/ssm-service'; -import { workflowIdFromOmicsShare } from '@BE/utils/laboratory-workflow-access-utils'; import { getNextFlowApiQueryParameters, httpRequest, REST_API_METHOD } from '@BE/utils/rest-api-utils'; const laboratoryService = new LaboratoryService(); @@ -32,28 +31,6 @@ async function listAllPrivateWorkflows(): Promise { return items; } -async function listAllSharedWorkflowSummaries(): Promise<{ id: string; name: string }[]> { - const out: { id: string; name: string }[] = []; - let nextToken: string | undefined; - do { - const page = await omicsService.listSharedWorkflows({ - resourceOwner: 'OTHER', - maxResults: 100, - nextToken, - }); - for (const share of page.shares ?? []) { - const id = workflowIdFromOmicsShare(share); - if (!id) { - continue; - } - const name = share.shareName ?? id; - out.push({ id, name }); - } - nextToken = page.nextToken; - } while (nextToken); - return out; -} - async function fetchSeqeraPipelinesForLab(laboratory: Laboratory): Promise { if (!laboratory.NextFlowTowerEnabled) { return []; @@ -133,10 +110,11 @@ export async function buildUnifiedWorkflowCatalogForOrganization( const anyOmics = laboratories.some((l) => l.AwsHealthOmicsEnabled); if (anyOmics) { - const [privateWf, sharedSummaries] = await Promise.all([ - listAllPrivateWorkflows(), - listAllSharedWorkflowSummaries(), - ]); + // Only PRIVATE workflows are enumerated here. Shared workflows are intentionally + // excluded: the lab-facing endpoint (list-private-workflows) only ever returns + // PRIVATE workflows and applies the access filter, so any shared workflow listed + // here would be grantable in the admin panel yet never visible to lab users. + const privateWf = await listAllPrivateWorkflows(); for (const w of privateWf) { if (!w.id) { continue; @@ -152,18 +130,6 @@ export async function buildUnifiedWorkflowCatalogForOrganization( name: w.name ?? w.id, }); } - for (const s of sharedSummaries) { - const key = `HealthOmics:${s.id}`; - if (seenKeys.has(key)) { - continue; - } - seenKeys.add(key); - workflows.push({ - platform: 'HealthOmics', - workflowId: s.id, - name: s.name, - }); - } } const seqeraLabs = laboratories.filter((l) => l.NextFlowTowerEnabled); diff --git a/packages/back-end/test/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.test.ts b/packages/back-end/test/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.test.ts index a27a1a75b..e9745f9da 100644 --- a/packages/back-end/test/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.test.ts +++ b/packages/back-end/test/app/controllers/aws-healthomics/workflow/list-private-workflows.lambda.test.ts @@ -97,23 +97,34 @@ describe('list-private-workflows.lambda', () => { EnableNewWorkflowsByDefault: false, }); - (mockOmicsService.prototype.listWorkflows as jest.Mock).mockResolvedValue({ - items: [ - { id: 'wf-allowed', name: 'Allowed' }, - { id: 'wf-blocked', name: 'Blocked' }, - ], - nextToken: 't1', - }); + // Paginated: first page carries a nextToken, second page terminates. The + // filter must be applied across every page, not just the first. + (mockOmicsService.prototype.listWorkflows as jest.Mock) + .mockResolvedValueOnce({ + items: [ + { id: 'wf-allowed', name: 'Allowed' }, + { id: 'wf-blocked', name: 'Blocked' }, + ], + nextToken: 't1', + }) + .mockResolvedValueOnce({ + items: [{ id: 'wf-allowed-2', name: 'Allowed 2' }], + nextToken: undefined, + }); (mockAccessService.prototype.listByLaboratoryId as jest.Mock).mockResolvedValue([ { LaboratoryId: LAB_ID, WorkflowKey: 'HEALTH_OMICS#wf-allowed' }, + { LaboratoryId: LAB_ID, WorkflowKey: 'HEALTH_OMICS#wf-allowed-2' }, ]); const res = await handler(createEvent({ laboratoryId: LAB_ID }), createContext(), () => {}); expect(res?.statusCode).toBe(200); const body = JSON.parse(res?.body ?? '{}'); - expect(body.items).toEqual([{ id: 'wf-allowed', name: 'Allowed' }]); - expect(body.nextToken).toBe('t1'); + expect(body.items).toEqual([ + { id: 'wf-allowed', name: 'Allowed' }, + { id: 'wf-allowed-2', name: 'Allowed 2' }, + ]); + expect(mockOmicsService.prototype.listWorkflows).toHaveBeenCalledTimes(2); }); it('when new workflows are enabled by default, omits only explicitly denied workflows', async () => { @@ -124,7 +135,8 @@ describe('list-private-workflows.lambda', () => { EnableNewWorkflowsByDefault: true, }); - (mockOmicsService.prototype.listWorkflows as jest.Mock).mockResolvedValue({ + // Single page (no nextToken) terminates pagination after one call. + (mockOmicsService.prototype.listWorkflows as jest.Mock).mockResolvedValueOnce({ items: [ { id: 'wf-ok', name: 'Ok' }, { id: 'wf-denied', name: 'Denied' }, diff --git a/packages/back-end/test/app/services/easy-genomics/laboratory-workflow-access-default-migration.test.ts b/packages/back-end/test/app/services/easy-genomics/laboratory-workflow-access-default-migration.test.ts index 99d4202ce..ebc715e9d 100644 --- a/packages/back-end/test/app/services/easy-genomics/laboratory-workflow-access-default-migration.test.ts +++ b/packages/back-end/test/app/services/easy-genomics/laboratory-workflow-access-default-migration.test.ts @@ -43,13 +43,20 @@ describe('migrateWorkflowAccessOnDefaultModeChange', () => { expect(mockListByLaboratoryId).not.toHaveBeenCalled(); }); - it('false → true: DENY catalog workflows without ALLOW; removes prior ALLOW rows', async () => { + it('false → true: never writes DENY rows; clears stale strict-mode rows so workflows are allowed by default', async () => { + // A lab that had one ALLOW row plus a stale/spurious DENY row while strict. mockListByLaboratoryId.mockResolvedValue([ { LaboratoryId: laboratoryId, OrganizationId: organizationId, WorkflowKey: 'HEALTH_OMICS#omics-a', }, + { + LaboratoryId: laboratoryId, + OrganizationId: organizationId, + WorkflowKey: 'SEQERA#pipe-b', + Effect: 'DENY', + }, ]); await migrateWorkflowAccessOnDefaultModeChange({ @@ -59,16 +66,13 @@ describe('migrateWorkflowAccessOnDefaultModeChange', () => { nextDefaultOn: true, }); - expect(mockUpsert).toHaveBeenCalledWith( - expect.objectContaining({ - LaboratoryId: laboratoryId, - WorkflowKey: 'SEQERA#pipe-b', - Effect: 'DENY', - OrganizationId: organizationId, - WorkflowName: 'Pipe B', - }), - ); + // Turning "enable new workflows by default" on must NOT deny the catalog. + expect(mockUpsert).not.toHaveBeenCalled(); + // Both the redundant ALLOW row and the stale DENY row are removed, leaving a + // clean "allowed unless explicitly denied" state. expect(mockRemove).toHaveBeenCalledWith(laboratoryId, 'HEALTH_OMICS', 'omics-a'); + expect(mockRemove).toHaveBeenCalledWith(laboratoryId, 'SEQERA', 'pipe-b'); + expect(mockRemove).toHaveBeenCalledTimes(2); }); it('true → false: ALLOW catalog workflows without DENY; removes prior DENY rows', async () => {