From 9f8c743d7357df3730b7f66c64191172b4aec12f Mon Sep 17 00:00:00 2001 From: Ram Chandra <46575254+TheInfinity007@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:59:30 +0530 Subject: [PATCH 1/4] fix(directory): include subject_type in client grant filenames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A client grant's identity is (client_id, audience, subject_type) — see the `identifiers` list in src/tools/auth0/handlers/clientGrants.ts, which also contains dedicated DELETE + CREATE handling because subject_type is immutable. The directory dump encoded only client name and API name, so grants differing solely by subject type (e.g. `client` vs `user` on the same client and audience) resolved to the same filename and silently overwrote each other. One of the two grants was lost from the export with no error or warning, and would then be treated as a deletion candidate on the next import. Append subject_type to the filename when present. Grants without the field keep their existing filename, so exports from tenants that do not use subject_type are unchanged. Co-Authored-By: Claude Opus 5 --- .../directory/handlers/clientGrants.ts | 8 +- test/context/directory/clientGrants.test.js | 100 ++++++++++++++++++ 2 files changed, 106 insertions(+), 2 deletions(-) diff --git a/src/context/directory/handlers/clientGrants.ts b/src/context/directory/handlers/clientGrants.ts index 961b811b0..70d103c9a 100644 --- a/src/context/directory/handlers/clientGrants.ts +++ b/src/context/directory/handlers/clientGrants.ts @@ -115,8 +115,12 @@ async function dump(context: DirectoryContext): Promise { ? keywordReplace(grant.audience, context.mappings) : grant.audience; - // Construct the name using non-marker names - const name = sanitize(`${clientNameNonMarker}-${apiName(apiAudienceNonMarker)}`); + // Construct the name using non-marker names. `subject_type` is part of a grant's identity + // (see `identifiers` in src/tools/auth0/handlers/clientGrants.ts), so it must be included: + // without it, grants differing only by subject type (e.g. `client` vs `user` on the same + // client and audience) resolve to the same filename and silently overwrite each other. + const baseName = `${clientNameNonMarker}-${apiName(apiAudienceNonMarker)}`; + const name = sanitize(grant.subject_type ? `${baseName}-${grant.subject_type}` : baseName); // Ensure the name is not empty or invalid if (!name || name.trim().length === 0) { diff --git a/test/context/directory/clientGrants.test.js b/test/context/directory/clientGrants.test.js index c08ace580..6631cf0cd 100644 --- a/test/context/directory/clientGrants.test.js +++ b/test/context/directory/clientGrants.test.js @@ -195,6 +195,106 @@ describe('#directory context clientGrants', () => { ).to.deep.equal(context.assets.clientGrants[2]); }); + it('should dump grants differing only by subject_type to separate files', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpSubjectType'); + cleanThenMkdir(dir); + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [{ client_id: 'client-id-1', name: 'Primary M2M' }]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { + id: 'resource-server-1', + name: 'Payments Service', + identifier: 'https://payments.travel0.com/api', + }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { + audience: 'https://payments.travel0.com/api', + client_id: 'client-id-1', + scope: ['read:card'], + subject_type: 'client', + }, + { + audience: 'https://payments.travel0.com/api', + client_id: 'client-id-1', + scope: ['update:card'], + subject_type: 'user', + }, + ]; + + await handler.dump(context); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + + const files = getFiles(clientGrantsFolder, ['.json']); + + // Both grants must survive the dump; previously the second overwrote the first. + expect(files).to.have.length(2); + expect(files).to.have.members([ + path.join(clientGrantsFolder, 'Primary M2M-Payments Service-client.json'), + path.join(clientGrantsFolder, 'Primary M2M-Payments Service-user.json'), + ]); + + expect( + loadJSON(path.join(clientGrantsFolder, 'Primary M2M-Payments Service-client.json')) + ).to.deep.equal(context.assets.clientGrants[0]); + expect( + loadJSON(path.join(clientGrantsFolder, 'Primary M2M-Payments Service-user.json')) + ).to.deep.equal(context.assets.clientGrants[1]); + }); + + it('should keep the legacy filename when subject_type is absent', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpNoSubjectType'); + cleanThenMkdir(dir); + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [{ client_id: 'client-id-1', name: 'Primary M2M' }]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { + id: 'resource-server-1', + name: 'Payments Service', + identifier: 'https://payments.travel0.com/api', + }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { + audience: 'https://payments.travel0.com/api', + client_id: 'client-id-1', + scope: ['read:card'], + }, + ]; + + await handler.dump(context); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + + const files = getFiles(clientGrantsFolder, ['.json']); + + expect(files).to.have.length(1); + expect(files[0]).to.equal(path.join(clientGrantsFolder, 'Primary M2M-Payments Service.json')); + }); + it('should not dump grants for excluded clients', async () => { const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpExclude'); cleanThenMkdir(dir); From 8c6bc9be289566368f7775b3b432c3467b08efca Mon Sep 17 00:00:00 2001 From: Ram Chandra <46575254+TheInfinity007@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:06:27 +0530 Subject: [PATCH 2/4] fix(directory): remove stale client grant files on dump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clientGrants dump never pruned files, unlike connections. Two consequences: 1. A grant deleted from the tenant kept its file, so the next import recreated the grant that had just been removed. 2. A grant whose filename changes is left behind under its old name while the new name is also written. `parse` then returns the same grant twice, and `calculateChanges` emits one create plus one update — the create targets a grant that already exists and the import fails. The second case is reachable from the preceding commit, which changes filenames for grants that carry a subject_type. Track written filenames and remove anything else in the folder, mirroring the existing cleanup in the connections handler. Filenames for excluded clients are recorded before those grants are filtered out, so excluding a client no longer deletes its previously dumped file. Name construction moves into a `nameFor` helper so the cleanup pass can derive the filenames of excluded grants. Co-Authored-By: Claude Opus 5 --- .../directory/handlers/clientGrants.ts | 87 ++++++---- test/context/directory/clientGrants.test.js | 161 ++++++++++++++++++ 2 files changed, 218 insertions(+), 30 deletions(-) diff --git a/src/context/directory/handlers/clientGrants.ts b/src/context/directory/handlers/clientGrants.ts index 70d103c9a..174a2cd20 100644 --- a/src/context/directory/handlers/clientGrants.ts +++ b/src/context/directory/handlers/clientGrants.ts @@ -66,26 +66,22 @@ async function dump(context: DirectoryContext): Promise { include_totals: true, }); - // Filter out grants for excluded clients - if (excludedClientsByNames.length) { - const excludedClientIds = new Set( - allClients - .filter((c) => c.name !== undefined && excludedClientsByNames.includes(c.name)) - .map((c) => c.client_id) - ); - clientGrants = clientGrants.filter( - (grant: ClientGrant) => !excludedClientIds.has(grant.client_id) + // Convert audience to the API name for readability + const apiName = (grantAudience: string | undefined) => { + if (!grantAudience) return grantAudience; + + const associatedAPI = allResourceServers.find( + (resourceServer) => resourceServer.identifier === grantAudience ); - } - // Convert client_id to the client name for readability - clientGrants.forEach((grant: ClientGrant) => { - const dumpGrant = { ...grant }; + if (associatedAPI === undefined) return grantAudience; // Use the audience if the API is not found - if (context.assets.clientsOrig) { - dumpGrant.client_id = convertClientIdToName(dumpGrant.client_id, context.assets.clientsOrig); - } + return associatedAPI.name; // Use the name of the API + }; + // Derive the filename for a grant. Shared by the cleanup pass below, which needs the names of + // excluded grants before they are filtered out. + const nameFor = (grant: ClientGrant) => { const clientName = (() => { const associatedClient = allClients.find((client) => client.client_id === grant.client_id); @@ -94,19 +90,6 @@ async function dump(context: DirectoryContext): Promise { return associatedClient.name; })(); - // Convert audience to the API name for readability - const apiName = (grantAudience: string | undefined) => { - if (!grantAudience) return grantAudience; - - const associatedAPI = allResourceServers.find( - (resourceServer) => resourceServer.identifier === grantAudience - ); - - if (associatedAPI === undefined) return grantAudience; // Use the audience if the API is not found - - return associatedAPI.name; // Use the name of the API - }; - // Replace keyword markers if necessary const clientNameNonMarker = doesHaveKeywordMarker(clientName, context.mappings) ? keywordReplace(clientName, context.mappings) @@ -120,7 +103,40 @@ async function dump(context: DirectoryContext): Promise { // without it, grants differing only by subject type (e.g. `client` vs `user` on the same // client and audience) resolve to the same filename and silently overwrite each other. const baseName = `${clientNameNonMarker}-${apiName(apiAudienceNonMarker)}`; - const name = sanitize(grant.subject_type ? `${baseName}-${grant.subject_type}` : baseName); + + return sanitize(grant.subject_type ? `${baseName}-${grant.subject_type}` : baseName); + }; + + // Track files that should remain after the dump (written + excluded). + const expectedFiles = new Set(); + + // Filter out grants for excluded clients + if (excludedClientsByNames.length) { + const excludedClientIds = new Set( + allClients + .filter((c) => c.name !== undefined && excludedClientsByNames.includes(c.name)) + .map((c) => c.client_id) + ); + // Excluded grants are never written, so record their filenames up front to stop the cleanup + // pass below from removing files for clients the user deliberately excluded. + clientGrants + .filter((grant: ClientGrant) => excludedClientIds.has(grant.client_id)) + .forEach((grant: ClientGrant) => expectedFiles.add(`${nameFor(grant)}.json`)); + + clientGrants = clientGrants.filter( + (grant: ClientGrant) => !excludedClientIds.has(grant.client_id) + ); + } + + // Convert client_id to the client name for readability + clientGrants.forEach((grant: ClientGrant) => { + const dumpGrant = { ...grant }; + + if (context.assets.clientsOrig) { + dumpGrant.client_id = convertClientIdToName(dumpGrant.client_id, context.assets.clientsOrig); + } + + const name = nameFor(grant); // Ensure the name is not empty or invalid if (!name || name.trim().length === 0) { @@ -129,7 +145,18 @@ async function dump(context: DirectoryContext): Promise { const grantFile = path.join(grantsFolder, `${name}.json`); dumpJSON(grantFile, dumpGrant); + expectedFiles.add(`${name}.json`); }); + + // Remove files that belong to grants no longer present (and not excluded). Without this, a grant + // whose filename changes is left behind under its old name and parsed back as a duplicate on the + // next import, and grants deleted from the tenant are silently recreated. + for (const existing of fs.readdirSync(grantsFolder)) { + const fullPath = path.join(grantsFolder, existing); + if (fs.statSync(fullPath).isFile() && !expectedFiles.has(existing)) { + fs.removeSync(fullPath); + } + } } const clientGrantsHandler: DirectoryHandler = { diff --git a/test/context/directory/clientGrants.test.js b/test/context/directory/clientGrants.test.js index 6631cf0cd..8105ebc0c 100644 --- a/test/context/directory/clientGrants.test.js +++ b/test/context/directory/clientGrants.test.js @@ -336,6 +336,167 @@ describe('#directory context clientGrants', () => { expect(files[0]).to.equal(path.join(clientGrantsFolder, 'IncludedClient-Some API.json')); }); + it('should remove files for grants no longer present', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPrune'); + cleanThenMkdir(dir); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + fs.ensureDirSync(clientGrantsFolder); + + // A grant that no longer exists on the tenant, left over from an earlier dump. + fs.writeFileSync( + path.join(clientGrantsFolder, 'Primary M2M-Removed Service.json'), + JSON.stringify({ + audience: 'https://removed.travel0.com/api', + client_id: 'client-id-1', + scope: [], + }) + ); + + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [{ client_id: 'client-id-1', name: 'Primary M2M' }]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { + id: 'resource-server-1', + name: 'Payments Service', + identifier: 'https://payments.travel0.com/api', + }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { + audience: 'https://payments.travel0.com/api', + client_id: 'client-id-1', + scope: ['read:card'], + }, + ]; + + await handler.dump(context); + + const files = getFiles(clientGrantsFolder, ['.json']); + expect(files).to.have.length(1); + expect(files[0]).to.equal(path.join(clientGrantsFolder, 'Primary M2M-Payments Service.json')); + }); + + it('should remove a stale file when a grant filename changes', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPruneRename'); + cleanThenMkdir(dir); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + fs.ensureDirSync(clientGrantsFolder); + + const grant = { + audience: 'https://payments.travel0.com/api', + client_id: 'client-id-1', + scope: ['read:card'], + subject_type: 'client', + }; + + // Filename produced before subject_type was included in the name. + fs.writeFileSync( + path.join(clientGrantsFolder, 'Primary M2M-Payments Service.json'), + JSON.stringify(grant) + ); + + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [{ client_id: 'client-id-1', name: 'Primary M2M' }]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { + id: 'resource-server-1', + name: 'Payments Service', + identifier: 'https://payments.travel0.com/api', + }, + ]), + }, + } + ); + + context.assets.clientGrants = [grant]; + + await handler.dump(context); + + // The old name must not survive alongside the new one, otherwise the grant is parsed + // back twice and the import attempts to create a grant that already exists. + const files = getFiles(clientGrantsFolder, ['.json']); + expect(files).to.have.length(1); + expect(files[0]).to.equal( + path.join(clientGrantsFolder, 'Primary M2M-Payments Service-client.json') + ); + + const parseContext = new Context({ AUTH0_INPUT_FILE: dir }, mockMgmtClient()); + await parseContext.loadAssetsFromLocal(); + expect(parseContext.assets.clientGrants).to.have.length(1); + }); + + it('should preserve files for excluded clients when removing stale files', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPruneExclude'); + cleanThenMkdir(dir); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + fs.ensureDirSync(clientGrantsFolder); + + // Dumped before the client was excluded; must survive the cleanup pass. + fs.writeFileSync( + path.join(clientGrantsFolder, 'ExcludedClient-Some API.json'), + JSON.stringify({ + audience: 'https://some.api.com', + client_id: 'client-id-2', + scope: ['write:data'], + }) + ); + + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [ + { client_id: 'client-id-1', name: 'IncludedClient' }, + { client_id: 'client-id-2', name: 'ExcludedClient' }, + ]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { + id: 'resource-server-1', + name: 'Some API', + identifier: 'https://some.api.com', + }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { audience: 'https://some.api.com', client_id: 'client-id-1', scope: ['read:data'] }, + { audience: 'https://some.api.com', client_id: 'client-id-2', scope: ['write:data'] }, + ]; + context.assets.exclude = { clients: ['ExcludedClient'] }; + + await handler.dump(context); + + const files = getFiles(clientGrantsFolder, ['.json']).map((f) => path.basename(f)); + expect(files).to.have.members(['IncludedClient-Some API.json', 'ExcludedClient-Some API.json']); + }); + it('should not fetch clients and resource servers if no client grants defined', async () => { const dir = path.join(testDataDir, 'directory', 'clientGrantsDump'); cleanThenMkdir(dir); From 0f5987c172fb7954422615060f944a5003f8ac29 Mon Sep 17 00:00:00 2001 From: Ram Chandra <46575254+TheInfinity007@users.noreply.github.com> Date: Thu, 27 Aug 2026 22:24:31 +0530 Subject: [PATCH 3/4] fix(directory): limit client grant cleanup to JSON files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cleanup pass added in the previous commit walked every entry returned by `readdirSync`, but `expectedFiles` only ever holds `${name}.json`. Any other file in the grants folder — a README, notes — was therefore deleted on dump. Connections needs the wider sweep because it writes `.html` email bodies and records them in `expectedFiles`. Grants only ever write `.json`, so scanning everything deletes user files without preventing any duplicate or recreated grant: a non-JSON file is never read back by `parse` and so is not stale state. Use `getFiles(grantsFolder, ['.json'])`, the same call `parse` uses, making the invariant explicit: dump removes exactly what parse would have read back. Reported in review on the preceding commit. Co-Authored-By: Claude Opus 5 --- .../directory/handlers/clientGrants.ts | 12 ++-- test/context/directory/clientGrants.test.js | 59 +++++++++++++++++++ 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/context/directory/handlers/clientGrants.ts b/src/context/directory/handlers/clientGrants.ts index 174a2cd20..45407363c 100644 --- a/src/context/directory/handlers/clientGrants.ts +++ b/src/context/directory/handlers/clientGrants.ts @@ -151,12 +151,12 @@ async function dump(context: DirectoryContext): Promise { // Remove files that belong to grants no longer present (and not excluded). Without this, a grant // whose filename changes is left behind under its old name and parsed back as a duplicate on the // next import, and grants deleted from the tenant are silently recreated. - for (const existing of fs.readdirSync(grantsFolder)) { - const fullPath = path.join(grantsFolder, existing); - if (fs.statSync(fullPath).isFile() && !expectedFiles.has(existing)) { - fs.removeSync(fullPath); - } - } + // + // Restricted to the `.json` files `parse` reads: anything else in the folder (a README, notes) + // can never come back as a grant, so it is not stale state and must not be deleted. + getFiles(grantsFolder, ['.json']) + .filter((file) => !expectedFiles.has(path.basename(file))) + .forEach((file) => fs.removeSync(file)); } const clientGrantsHandler: DirectoryHandler = { diff --git a/test/context/directory/clientGrants.test.js b/test/context/directory/clientGrants.test.js index 8105ebc0c..ecd07545e 100644 --- a/test/context/directory/clientGrants.test.js +++ b/test/context/directory/clientGrants.test.js @@ -445,6 +445,65 @@ describe('#directory context clientGrants', () => { expect(parseContext.assets.clientGrants).to.have.length(1); }); + it('should not remove non-JSON files when removing stale files', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPruneNonJson'); + cleanThenMkdir(dir); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + fs.ensureDirSync(clientGrantsFolder); + + // Files `parse` never reads back as grants. They are not stale state, so the cleanup pass + // must leave them alone. + fs.writeFileSync(path.join(clientGrantsFolder, 'README.md'), '# Grants'); + fs.writeFileSync(path.join(clientGrantsFolder, 'notes.txt'), 'why these grants exist'); + + // A genuinely stale grant file, to prove cleanup still runs. + fs.writeFileSync( + path.join(clientGrantsFolder, 'Primary M2M-Removed Service.json'), + JSON.stringify({ + audience: 'https://removed.travel0.com/api', + client_id: 'client-id-1', + scope: [], + }) + ); + + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [{ client_id: 'client-id-1', name: 'Primary M2M' }]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { + id: 'resource-server-1', + name: 'Payments Service', + identifier: 'https://payments.travel0.com/api', + }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { + audience: 'https://payments.travel0.com/api', + client_id: 'client-id-1', + scope: ['read:card'], + }, + ]; + + await handler.dump(context); + + expect(fs.readdirSync(clientGrantsFolder).sort()).to.deep.equal([ + 'Primary M2M-Payments Service.json', + 'README.md', + 'notes.txt', + ]); + }); + it('should preserve files for excluded clients when removing stale files', async () => { const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPruneExclude'); cleanThenMkdir(dir); From 721f1216e28254e1c3a9a98da3406d8bd8cbba3f Mon Sep 17 00:00:00 2001 From: Ram Chandra <46575254+TheInfinity007@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:11:55 +0530 Subject: [PATCH 4/4] fix(directory): preserve excluded client grant files by contents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cleanup pass seeded `expectedFiles` for excluded clients with `nameFor(grant)`, which produces the current name format. A file from an earlier export is on disk under the old format, so for an excluded client whose grant carries a subject_type the first export after the naming change deletes the old file — and because the client is excluded, nothing writes it back. The file is gone. `connections.ts` avoids this by seeding from the connection name, but that works only because a connection's filename *is* its sanitized name. A grant's filename is derived from the client name, the API name, the grant's subject_type and the naming format itself, so it cannot be reconstructed from the exclude list. Decide preservation from the file's contents instead: read each candidate and keep it when its `client_id` identifies an excluded client, whatever the file is called. This also covers a renamed resource server and a grant that has since been deleted from the tenant, neither of which the name-derived seeding survived. - Match both forms the field can take: the client name when `clientsOrig` was available at dump time, the raw client_id otherwise. Names come from the exclude list so that excluding a client absent from the tenant still protects its file. - Read via `loadJSON` with the context mappings, as `parse` does, so keyword markers resolve before comparison. - Keep unreadable files rather than deleting them, and never let one bad file fail the export — this is the first time `dump` reads these files at all. - Log each removal, matching `dumpJSON` logging each write. The `nameFor` seeding is dropped rather than kept as a fallback: it protects nothing the contents check misses, since a grant absent from the export has no name to derive. Known gap: a client renamed on the tenant while excluded still loses its file. The file records the old name and the exclude list must carry the current one, so nothing links them. The existing exclude test passed on a name coincidence — its fixture grant had no subject_type, so the old and new names matched. It now carries one. Co-Authored-By: Claude Opus 5 --- .../directory/handlers/clientGrants.ts | 62 ++++++-- test/context/directory/clientGrants.test.js | 149 +++++++++++++++++- 2 files changed, 193 insertions(+), 18 deletions(-) diff --git a/src/context/directory/handlers/clientGrants.ts b/src/context/directory/handlers/clientGrants.ts index 45407363c..7dd55ea4b 100644 --- a/src/context/directory/handlers/clientGrants.ts +++ b/src/context/directory/handlers/clientGrants.ts @@ -2,6 +2,7 @@ import path from 'path'; import fs from 'fs-extra'; import { constants, keywordReplace } from '../../../tools'; +import log from '../../../logger'; import { getFiles, existsMustBeDir, @@ -79,8 +80,7 @@ async function dump(context: DirectoryContext): Promise { return associatedAPI.name; // Use the name of the API }; - // Derive the filename for a grant. Shared by the cleanup pass below, which needs the names of - // excluded grants before they are filtered out. + // Derive the filename for a grant. const nameFor = (grant: ClientGrant) => { const clientName = (() => { const associatedClient = allClients.find((client) => client.client_id === grant.client_id); @@ -107,22 +107,49 @@ async function dump(context: DirectoryContext): Promise { return sanitize(grant.subject_type ? `${baseName}-${grant.subject_type}` : baseName); }; - // Track files that should remain after the dump (written + excluded). + const excludedClients = allClients.filter( + (c) => c.name !== undefined && excludedClientsByNames.includes(c.name) + ); + + // Values that can stand for an excluded client in the `client_id` field of a dumped file: the + // client name when `clientsOrig` was available at dump time (see `convertClientIdToName` below), + // the raw client_id otherwise. Names come from the exclude list rather than from `allClients` so + // that excluding a client absent from the tenant still protects its file. + const excludedClientIdentities = new Set([ + ...excludedClientsByNames, + ...excludedClients.map((c) => c.client_id).filter((id): id is string => !!id), + ]); + + // Whether a file this dump did not write must nonetheless survive the cleanup pass. Its name + // cannot answer that: the name is derived from the client name, the API name, the grant's + // subject_type and the current naming format, so a file written by an earlier version — or + // before its API was renamed — no longer matches the name `nameFor` produces today. Read the + // file instead, because the client identity recorded inside it does not drift. + const mustPreserve = (file: string): boolean => { + if (excludedClientIdentities.size === 0) return false; + + let grant; + try { + grant = loadJSON(file, { + mappings: context.mappings, + disableKeywordReplacement: context.disableKeywordReplacement, + }); + } catch (err) { + // Deleting a file it cannot read is not the export's call to make, and one bad file must not + // fail the whole export. Keep it and let `parse` report the problem on the next import. + log.warn(`Keeping ${file}, it could not be read while cleaning up client grants: ${err}`); + return true; + } + + return excludedClientIdentities.has(grant?.client_id); + }; + + // Track files written by this dump; everything else in the folder is a cleanup candidate. const expectedFiles = new Set(); // Filter out grants for excluded clients if (excludedClientsByNames.length) { - const excludedClientIds = new Set( - allClients - .filter((c) => c.name !== undefined && excludedClientsByNames.includes(c.name)) - .map((c) => c.client_id) - ); - // Excluded grants are never written, so record their filenames up front to stop the cleanup - // pass below from removing files for clients the user deliberately excluded. - clientGrants - .filter((grant: ClientGrant) => excludedClientIds.has(grant.client_id)) - .forEach((grant: ClientGrant) => expectedFiles.add(`${nameFor(grant)}.json`)); - + const excludedClientIds = new Set(excludedClients.map((c) => c.client_id)); clientGrants = clientGrants.filter( (grant: ClientGrant) => !excludedClientIds.has(grant.client_id) ); @@ -155,8 +182,11 @@ async function dump(context: DirectoryContext): Promise { // Restricted to the `.json` files `parse` reads: anything else in the folder (a README, notes) // can never come back as a grant, so it is not stale state and must not be deleted. getFiles(grantsFolder, ['.json']) - .filter((file) => !expectedFiles.has(path.basename(file))) - .forEach((file) => fs.removeSync(file)); + .filter((file) => !expectedFiles.has(path.basename(file)) && !mustPreserve(file)) + .forEach((file) => { + log.info(`Removing ${file}`); + fs.removeSync(file); + }); } const clientGrantsHandler: DirectoryHandler = { diff --git a/test/context/directory/clientGrants.test.js b/test/context/directory/clientGrants.test.js index ecd07545e..8da97da3b 100644 --- a/test/context/directory/clientGrants.test.js +++ b/test/context/directory/clientGrants.test.js @@ -510,13 +510,17 @@ describe('#directory context clientGrants', () => { const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); fs.ensureDirSync(clientGrantsFolder); - // Dumped before the client was excluded; must survive the cleanup pass. + // Dumped by an older version, before subject_type was part of the filename, and before the + // client was excluded. The name this dump would derive for the grant is + // `ExcludedClient-Some API-client.json`, so the file must be preserved on its contents rather + // than on a name match. fs.writeFileSync( path.join(clientGrantsFolder, 'ExcludedClient-Some API.json'), JSON.stringify({ audience: 'https://some.api.com', client_id: 'client-id-2', scope: ['write:data'], + subject_type: 'client', }) ); @@ -546,7 +550,12 @@ describe('#directory context clientGrants', () => { context.assets.clientGrants = [ { audience: 'https://some.api.com', client_id: 'client-id-1', scope: ['read:data'] }, - { audience: 'https://some.api.com', client_id: 'client-id-2', scope: ['write:data'] }, + { + audience: 'https://some.api.com', + client_id: 'client-id-2', + scope: ['write:data'], + subject_type: 'client', + }, ]; context.assets.exclude = { clients: ['ExcludedClient'] }; @@ -556,6 +565,142 @@ describe('#directory context clientGrants', () => { expect(files).to.have.members(['IncludedClient-Some API.json', 'ExcludedClient-Some API.json']); }); + it('should preserve an excluded client file recorded by client name', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPruneExcludeByName'); + cleanThenMkdir(dir); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + fs.ensureDirSync(clientGrantsFolder); + + // Dumps run with `clientsOrig` available store the client name in `client_id`, so the + // preservation check has to recognise that form too. + fs.writeFileSync( + path.join(clientGrantsFolder, 'Renamed API grant.json'), + JSON.stringify({ + audience: 'https://some.api.com', + client_id: 'ExcludedClient', + scope: ['write:data'], + }) + ); + + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [ + { client_id: 'client-id-1', name: 'IncludedClient' }, + { client_id: 'client-id-2', name: 'ExcludedClient' }, + ]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { id: 'resource-server-1', name: 'Some API', identifier: 'https://some.api.com' }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { audience: 'https://some.api.com', client_id: 'client-id-1', scope: ['read:data'] }, + ]; + context.assets.exclude = { clients: ['ExcludedClient'] }; + + await handler.dump(context); + + const files = getFiles(clientGrantsFolder, ['.json']).map((f) => path.basename(f)); + expect(files).to.have.members(['IncludedClient-Some API.json', 'Renamed API grant.json']); + }); + + it('should preserve an excluded client file for a grant no longer on the tenant', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPruneExcludeGone'); + cleanThenMkdir(dir); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + fs.ensureDirSync(clientGrantsFolder); + + // The excluded client's grant is absent from the export, so no filename can be derived for it. + // The file is still the user's own config for a client they excluded, and must survive. + fs.writeFileSync( + path.join(clientGrantsFolder, 'ExcludedClient-Some API.json'), + JSON.stringify({ + audience: 'https://some.api.com', + client_id: 'client-id-2', + scope: ['write:data'], + }) + ); + + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [ + { client_id: 'client-id-1', name: 'IncludedClient' }, + { client_id: 'client-id-2', name: 'ExcludedClient' }, + ]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { id: 'resource-server-1', name: 'Some API', identifier: 'https://some.api.com' }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { audience: 'https://some.api.com', client_id: 'client-id-1', scope: ['read:data'] }, + ]; + context.assets.exclude = { clients: ['ExcludedClient'] }; + + await handler.dump(context); + + const files = getFiles(clientGrantsFolder, ['.json']).map((f) => path.basename(f)); + expect(files).to.have.members(['IncludedClient-Some API.json', 'ExcludedClient-Some API.json']); + }); + + it('should keep an unreadable file instead of failing the dump', async () => { + const dir = path.join(testDataDir, 'directory', 'clientGrantsDumpPruneMalformed'); + cleanThenMkdir(dir); + const clientGrantsFolder = path.join(dir, constants.CLIENTS_GRANTS_DIRECTORY); + fs.ensureDirSync(clientGrantsFolder); + + // Reading files to identify excluded grants must not turn one bad file into a failed export. + fs.writeFileSync(path.join(clientGrantsFolder, 'broken.json'), '{ not json'); + + const context = new Context( + { AUTH0_INPUT_FILE: dir }, + { + ...mockMgmtClient(), + clients: { + list: (params) => + mockPagedData(params, 'clients', [ + { client_id: 'client-id-1', name: 'IncludedClient' }, + { client_id: 'client-id-2', name: 'ExcludedClient' }, + ]), + }, + resourceServers: { + list: (params) => + mockPagedData(params, 'resource_servers', [ + { id: 'resource-server-1', name: 'Some API', identifier: 'https://some.api.com' }, + ]), + }, + } + ); + + context.assets.clientGrants = [ + { audience: 'https://some.api.com', client_id: 'client-id-1', scope: ['read:data'] }, + ]; + context.assets.exclude = { clients: ['ExcludedClient'] }; + + await handler.dump(context); + + const files = getFiles(clientGrantsFolder, ['.json']).map((f) => path.basename(f)); + expect(files).to.have.members(['IncludedClient-Some API.json', 'broken.json']); + }); + it('should not fetch clients and resource servers if no client grants defined', async () => { const dir = path.join(testDataDir, 'directory', 'clientGrantsDump'); cleanThenMkdir(dir);