fix: prevent client grant loss and duplication in directory format exports - #1473
fix: prevent client grant loss and duplication in directory format exports#1473TheInfinity007 wants to merge 4 commits into
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
The Cause
const AUTH0_CLIENT_ID = process.env['AUTH0_E2E_CLIENT_ID'] || '';The clients and clientGrants handlers exclude the Management API client from changes via The four failures are also all YAML-path tests, while this PR only touches the directory VerificationOn unmodified
That single variable flips it — no secret or domain needed, since I also ruled out dependency drift: installing with Possible fixes1. Default the client ID in lockdown mode (suggested) The value isn't a secret — const AUTH0_CLIENT_ID = shouldUseRecordings
? 'Vp0gMRF8PtMzekil38qWoj4Fjw2VjRZE'
: process.env['AUTH0_E2E_CLIENT_ID'] || '';One line, no CI changes, no credentials. Makes 2. Enable "Pass secrets to builds from forked pull requests" in CircleCI Works, but exposes every project secret to arbitrary fork builds — probably not a trade you 3. Re-run the job with the env available, or rely on internal CI post-merge Unblocks this PR but leaves the underlying issue for the next external contributor. Happy to open a separate PR for option 1 if that's useful — I'd keep it out of this branch so |
harshithRai
left a comment
harshithRai
left a comment
There was a problem hiding this comment.
Thanks for the really thorough writeup, and nice diagnosis on both bugs!
The subject_type-in-filename fix and the stale-file pruning both make sense, and the tests are solid.
I've left a couple of inline notes.
The main one is an edge case in the excluded-client handling that looks like a data-loss path, so I'd like to get that sorted before merging.
| // 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`)); |
There was a problem hiding this comment.
There seems to be a data-loss edge case here. This seeds expectedFiles with nameFor(grant), which produces the new name format (Client-API-client.json), but a file from a prior export is on disk under the old format (Client-API.json). So for an excluded client whose grant carries a subject_type, the first export after this ships: the old file isn't in expectedFiles, the cleanup pass deletes it, and since the client is excluded nothing replaces it. The file is gone.
connections.ts avoids this by seeding from the connection name (stable, format-independent) rather than a derived filename:
for (const name of excludedConnections) {
expectedFiles.add(`${sanitize(name)}.json`);
expectedFiles.add(`${sanitize(name)}.html`);
}The new exclude test misses it because its fixture grant has no subject_type, so the old and new names match.
Let's preserve all pre-existing files for excluded clients regardless of current name (or skip pruning for excluded grants entirely), and add a test with a subject_type-bearing excluded grant that already exists under the old filename.
There was a problem hiding this comment.
Fixed. You were right that seeding from nameFor reintroduces the format dependency.
| // 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)) { |
There was a problem hiding this comment.
Small thing: this now deletes any non-.json file in the grants folder too, since expectedFiles only holds .json names. It matches the connections behavior so I'm fine with it, but this folder never pruned anything before, so it might be worth a line in the PR description so nobody's surprised.
There was a problem hiding this comment.
Good catch — fixed rather than documented. The prune now uses getFiles(grantsFolder, ['.json']), the same call parse uses, so dump only removes files it could have read back as grants.
Added a test that drops a README.md and a notes.txt into the folder alongside a stale grant file and asserts the stale .json goes while the other two stay.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
2 participants
🔧 Changes
Two related bugs in the
clientGrantsdirectory-format handler, one per commit.1. Grant filenames omit
subject_type, so grants overwrite each othersrc/context/directory/handlers/clientGrants.tsbuilt the filename from client name +API name only. But a grant's identity is
(client_id, audience, subject_type)— per theidentifierslist insrc/tools/auth0/handlers/clientGrants.ts, which also containsdedicated DELETE + CREATE handling because
subject_typeis immutable (it is instripUpdateFields).So a client holding both a
clientand ausergrant on the same audience produced asingle file: the second write silently overwrote the first. No error or warning is emitted —
the dump only validates that the generated name is non-empty. The lost grant is absent from
the export and becomes a deletion candidate on the next import, removed outright when
AUTH0_ALLOW_DELETEis enabled.The directory serializer was strictly less expressive than the data model it serializes.
subject_typeis now appended to the filename when present. Grants without the field keeptheir existing filename.
2. Stale grant files are never removed
Unlike
connections, theclientGrantsdump never pruned its folder. Two consequences:that had just been removed.
while the new name is also written.
parsethen returns the same grant twice, andcalculateChangesemits one create plus one update — the create targets a grant thatalready exists, so the import fails.
The dump now tracks written filenames and removes anything else in the folder, mirroring the
existing cleanup in
connections.ts. Filenames for excluded clients are recorded beforethose grants are filtered out, so excluding a client no longer deletes its previously dumped
file.
Name construction moved into a
nameForhelper so the cleanup pass can derive the filenamesof excluded grants. No behaviour change beyond the above.
Tenants whose grants carry
subject_typewill see files renamed on their next export(
Client-API.json→Client-API-client.json). Commit 2 makes this safe — the old file isremoved rather than left to be parsed as a duplicate — but it will show up as a rename in
users' config repos.
If you'd prefer to avoid that churn, I'm happy to switch to appending
subject_typeonlywhere a collision actually exists. That limits renames to the tenants currently losing data,
at the cost of a filename that depends on sibling grants. Happy to go either way.
📚 References
Fixes #1472
🔬 Testing
Unit tests added to
test/context/directory/clientGrants.test.js— 11 passing in that file,1423 in the full suite (up from 1420):
subject_typedump to separate filessubject_typekeeps its legacy filename (backward compatibility)exactly one asset
The first test fails on unmodified
master, confirming the bug:The pre-existing dump tests assert exact filenames and pass unmodified, since their fixtures
omit
subject_type.To reproduce manually: give one client two grants on the same audience, one with
subject_type: clientand one withsubject_type: user, thena0deploy export -c config.json -f directory -o ./outand inspect./out/grants/. Only onefile exists for that client/audience pair. The same export with
-f yamlcorrectly containsboth, since
tenant.yamlstoresclientGrantsas a list.Two known gaps left out to keep this focused — happy to file follow-ups:
dump()returns early whenclientGrants.length === 0, before the cleanup runs, sodeleting every grant prunes nothing while deleting some prunes correctly. The early
return exists to avoid the clients/resourceServers API calls, which an existing test asserts.
default_forthird-party grants have noclient_idand produce filenames beginning withthe literal string
undefined.📝 Checklist