diff --git a/api/src/processings/router.ts b/api/src/processings/router.ts index ccc0f4d..98b4705 100644 --- a/api/src/processings/router.ts +++ b/api/src/processings/router.ts @@ -35,11 +35,14 @@ const ajv = ajvFormats(new Ajv({ strict: false })) /** * Ensure the plugin tarball is in the API's local cache (downloads on miss). * Registry enforces that `processing.owner` has access to the artefact; a 403 - * here surfaces directly to the caller. + * here surfaces directly to the caller as a user-facing French message. * * The plugin's processing config schema is read from the conventional - * `processing-config-schema.json` shipped alongside the plugin (same convention - * as v5). Returns `undefined` when the plugin doesn't ship one. + * `processing-config-schema.json` shipped alongside the plugin (v6 convention), + * falling back to `plugin.json#processingConfigSchema` for legacy v5 plugins + * whose tarballs were packed verbatim by the registry migration and never had + * the schema extracted into a standalone file. Returns `undefined` when + * neither source carries one. */ async function ensurePluginAndReadSchema (processing: Pick) { const account = { @@ -50,17 +53,39 @@ async function ensurePluginAndReadSchema (processing: Pick | undefined if (await fs.pathExists(schemaPath)) { processingConfigSchema = await fs.readJson(schemaPath) + } else { + const pluginJsonPath = path.join(ensured.path, 'plugin.json') + if (await fs.pathExists(pluginJsonPath)) { + const pluginJson = await fs.readJson(pluginJsonPath) + if (pluginJson?.processingConfigSchema) processingConfigSchema = pluginJson.processingConfigSchema + } } return { ensured, processingConfigSchema } } @@ -101,9 +126,8 @@ const sendProcessingEvent = ( * - validate the config against the plugin's processingConfigSchema (read * from the cached package.json#registry block, downloaded on cache miss) * - * Errors from registry (404 unknown artefact, 403 owner has no access) bubble - * up as the corresponding HTTP errors and replace the explicit access check - * that used to live in the create/update endpoints. + * Skipped entirely when the processing has no config. Registry errors are + * translated to user-facing French messages inside ensurePluginAndReadSchema. */ async function validateFullProcessing (processing: any): Promise { (await import('#types/processing/index.ts')).returnValid(processing) @@ -118,7 +142,12 @@ async function validateFullProcessing (processing: any): Promise { } const prepareProcessing = async (processing: Processing) => { - // Get the plugin file and execute the prepare function if it exists. + // `prepare` operates on the user-supplied config (e.g. exchanging OAuth + // refresh tokens for ciphered secrets). With no config there's nothing to + // prepare — and skipping the registry fetch lets a processing be created + // even when the owner has no access to the plugin yet. The same broken + // state is what the UI banner already shows for deleted plugins. + if (!processing.config) return const { ensured } = await ensurePluginAndReadSchema(processing) const plugin = await importPluginModule<{ prepare?: PrepareFunction }>(ensured.path, { cacheBust: true }) if (!(plugin.prepare && typeof plugin.prepare === 'function')) return @@ -336,9 +365,10 @@ router.post('', async (req, res) => { date: new Date().toISOString() } - // Plugin access check is delegated to registry: validateFullProcessing - // calls ensureArtefact with owner=body.owner, and registry returns 403 if - // the artefact is private and the owner has no privateAccess entry. + // Plugin access is only checked when actually needed: validateFullProcessing + // and prepareProcessing both short-circuit when `body.config` is absent. A + // processing created without config lands in the same "plugin not loaded" + // state that existed for deleted/revoked plugins — see ui pluginBroken. const processing = await validateFullProcessing(body) Object.assign(processing, await prepareProcessing(processing)) await mongo.processings.insertOne(processing) @@ -425,7 +455,11 @@ router.get('/:id/plugin-config-schema', async (req, res, next) => { if (!processing) return res.status(404).send() if (!['admin', 'exec', 'read'].includes(permissions.getUserResourceProfile(processing.owner, processing.permissions, sessionState) ?? '')) return res.status(403).send() const { processingConfigSchema } = await ensurePluginAndReadSchema(processing) - if (!processingConfigSchema) return res.status(404).send() + if (!processingConfigSchema) { + return res.status(404).type('text/plain').send( + `Plugin "${processing.plugin}" ships no processing config schema (no processing-config-schema.json in tarball and no processingConfigSchema field in plugin.json).` + ) + } res.status(200).json(processingConfigSchema) } catch (err) { next(err) } }) diff --git a/tests/features/processings/broken-plugin.api.spec.ts b/tests/features/processings/broken-plugin.api.spec.ts index 8d2c287..b795702 100644 --- a/tests/features/processings/broken-plugin.api.spec.ts +++ b/tests/features/processings/broken-plugin.api.spec.ts @@ -38,21 +38,30 @@ test.describe('processing with unavailable plugin', () => { expect(res.data.plugin).toBe('@test-never-existed-1') }) - test('PATCH /processings/:id surfaces the registry error', async () => { + test('PATCH /processings/:id with no config field succeeds even when the plugin is gone', async () => { + const superadmin = await axiosAuth('test_superadmin@test.com') + const id = await createBrokenProcessing(superadmin) + + // Renaming/changing scheduling on a broken-plugin processing must work — + // ensurePluginAndReadSchema is only invoked when there's a config to + // validate or prepare. Lets users fix metadata, switch owner, or delete. + const res = await superadmin.patch(`/api/v1/processings/${id}`, { title: 'New title' }) + expect(res.status).toBe(200) + expect(res.data.title).toBe('New title') + }) + + test('PATCH /processings/:id with a config field surfaces a 404 with the plugin id', async () => { const superadmin = await axiosAuth('test_superadmin@test.com') const id = await createBrokenProcessing(superadmin) const res = await superadmin.patch( `/api/v1/processings/${id}`, - { title: 'New title' }, + { config: { message: 'hello' } }, { validateStatus: () => true } ) - // Registry returns 404 for an unknown artefact. In the current - // implementation ensurePluginAndReadSchema does NOT forward the registry - // status — the unhandled AxiosRequestError propagates and Express returns - // 500. We include 403/404 in the set so the test also passes if the - // forwarding behaviour is improved in a future refactor. - expect([403, 404, 500]).toContain(res.status) + expect(res.status).toBe(404) + expect(typeof res.data === 'string' ? res.data : JSON.stringify(res.data)) + .toContain('@test-never-existed-1') }) test('DELETE /processings/:id returns 204', async () => { diff --git a/tests/features/processings/plugin-access.api.spec.ts b/tests/features/processings/plugin-access.api.spec.ts new file mode 100644 index 0000000..de414d0 --- /dev/null +++ b/tests/features/processings/plugin-access.api.spec.ts @@ -0,0 +1,128 @@ +import { test, expect } from '@playwright/test' +import { axiosAuth, clean } from '../../support/axios.ts' +import { publishFixturePlugin } from '../../support/registry.ts' + +/** + * A superadmin can see every artefact in the registry, so the create-form + * picker lets them pick a plugin no matter who they set as owner. When the + * picked plugin is private and the owner is not on `privateAccess`, the + * registry's /:id/download route returns 403 (no longer 404, since the + * caller already knows the id). The processings API translates that into + * a French user-facing message and routes the response status correctly. + * + * The owner used here is `test_user1` because the default fixture grants only + * include `test_superadmin` + the two orgs — a plain user account is the + * cleanest "has a grant but no privateAccess for THIS plugin" subject. + */ +test.describe('processing creation with restricted plugin access', () => { + test.beforeEach(clean) + test.afterAll(clean) + + test('superadmin can create a processing without config even if the owner has no plugin access (broken state)', async () => { + const superadmin = await axiosAuth('test_superadmin@test.com') + const fixture = await publishFixturePlugin({ + name: '@data-fair/processing-hello-world', + version: '1.2.2', + isPublic: false, + privateAccess: [{ type: 'organization', id: 'test_org1', name: 'Test Org 1' }], + // Grant test_user1 the global access-grant so the only thing missing + // is the privateAccess entry — that's the scenario we want to test. + grants: [ + { type: 'user', id: 'test_superadmin' }, + { type: 'user', id: 'test_user1' }, + { type: 'organization', id: 'test_org1' } + ] + }) + + const res = await superadmin.post('/api/v1/processings', { + title: 'Owner has no access', + plugin: fixture.pluginId, + owner: { type: 'user', id: 'test_user1', name: 'Test User 1' } + }) + expect(res.status).toBe(200) + expect(res.data._id).toBeTruthy() + expect(res.data.owner.id).toBe('test_user1') + + // The schema fetch is the first call that hits the registry on behalf of + // the owner. Pre-rebuild registry image hides the artefact behind 404; + // post-rebuild it returns 403 (since the caller already knows the id and + // we want to distinguish "no access" from "doesn't exist"). Either way + // the processings API translates to a French message containing the + // plugin id, NOT the previous opaque 500. + const schema = await superadmin.get( + `/api/v1/processings/${res.data._id}/plugin-config-schema`, + { validateStatus: () => true } + ) + expect([403, 404]).toContain(schema.status) + const body = typeof schema.data === 'string' ? schema.data : JSON.stringify(schema.data) + expect(body).toContain(fixture.pluginId) + // Post-rebuild only: the message names the owner. Skip if the registry + // still returns 404 — that's just "Le plugin … est introuvable". + if (schema.status === 403) expect(body).toContain('Test User 1') + }) + + test('PATCH with a config field on a processing whose owner has no access fails with 403 + message', async () => { + const superadmin = await axiosAuth('test_superadmin@test.com') + const fixture = await publishFixturePlugin({ + name: '@data-fair/processing-hello-world', + version: '1.2.2', + isPublic: false, + privateAccess: [{ type: 'organization', id: 'test_org1', name: 'Test Org 1' }], + grants: [ + { type: 'user', id: 'test_superadmin' }, + { type: 'user', id: 'test_user1' }, + { type: 'organization', id: 'test_org1' } + ] + }) + + const created = (await superadmin.post('/api/v1/processings', { + title: 'Owner has no access', + plugin: fixture.pluginId, + owner: { type: 'user', id: 'test_user1', name: 'Test User 1' } + })).data + + const patch = await superadmin.patch( + `/api/v1/processings/${created._id}`, + { + config: { + datasetMode: 'create', + dataset: { id: 'test_x', title: 'X' }, + overwrite: false, + message: 'x' + } + }, + { validateStatus: () => true } + ) + // 403 once the registry change deploys; 404 in the meantime (registry + // currently hides the artefact behind 404 from the owner's view). + expect([403, 404]).toContain(patch.status) + const body = typeof patch.data === 'string' ? patch.data : JSON.stringify(patch.data) + expect(body).toContain(fixture.pluginId) + if (patch.status === 403) expect(body).toContain('Test User 1') + }) + + test('owner with privateAccess can have a processing created with config', async () => { + const superadmin = await axiosAuth('test_superadmin@test.com') + const fixture = await publishFixturePlugin({ + name: '@data-fair/processing-hello-world', + version: '1.2.2', + isPublic: false, + privateAccess: [{ type: 'organization', id: 'test_org1', name: 'Test Org 1' }] + }) + + const res = await superadmin.post('/api/v1/processings', { + title: 'Authorized owner with config', + plugin: fixture.pluginId, + owner: { type: 'organization', id: 'test_org1', name: 'Test Org 1' }, + config: { + datasetMode: 'create', + dataset: { id: 'test_y', title: 'Y' }, + overwrite: false, + message: 'y' + } + }) + expect(res.status).toBe(200) + expect(res.data._id).toBeTruthy() + expect(res.data.config?.message).toBe('y') + }) +}) diff --git a/ui/src/pages/processings/[id]/index.vue b/ui/src/pages/processings/[id]/index.vue index dcbc470..6fdb299 100644 --- a/ui/src/pages/processings/[id]/index.vue +++ b/ui/src/pages/processings/[id]/index.vue @@ -168,8 +168,17 @@ const configSchemaFetch = useFetch>( computed(() => processing.value?.plugin ? `${$apiPath}/processings/${processingId}/plugin-config-schema` : null), + // We handle notifications ourselves below — useFetch's built-in toast + // would also fire on the legitimate "plugin ships no schema" 404. { notifError: false } ) +const { sendUiNotif } = useUiNotif() +watch(() => configSchemaFetch.error.value, (err) => { + if (!err) return + const status = err.statusCode ?? err.status + if (status === 404) return // legitimate "plugin ships no schema" + sendUiNotif({ msg: t('configSchemaFetchError'), error: err }) +}) const configSchema = computed | null>(() => { const err = configSchemaFetch.error.value // The endpoint returns 404 to mean "this plugin doesn't ship a schema"; we @@ -326,6 +335,7 @@ const timezoneLabel = (timeZone: string) => { updateError: Error while updating the processing pluginUnavailableTitle: Plugin unavailable pluginUnavailableBody: This processing's plugin has been removed or its access revoked. You can no longer edit or run this processing, but you can still view its run history and delete it. + configSchemaFetchError: Failed to load the plugin's configuration schema fr: frequency: @@ -340,6 +350,7 @@ const timezoneLabel = (timeZone: string) => { updateError: Erreur lors de la mise à jour du traitement pluginUnavailableTitle: Plugin indisponible pluginUnavailableBody: Le plugin de ce traitement a été supprimé ou son accès retiré. Vous ne pouvez plus modifier ni exécuter ce traitement, mais vous pouvez consulter son historique et le supprimer. + configSchemaFetchError: Échec du chargement du schéma de configuration du plugin