feat(cli): actionable errors for missing oauth grants - #751
Draft
maoberlehner wants to merge 15 commits into
Draft
feat(cli): actionable errors for missing oauth grants#751maoberlehner wants to merge 15 commits into
maoberlehner wants to merge 15 commits into
Conversation
Wires matchCredentialError/getCredentialContext into APIError's constructor so every handleAPIError call site benefits without being touched. Also fixes 403 falling through getErrorId() to "generic" (rendered as "Error fetching data from the API") by adding explicit forbidden/insufficient_scope error ids. The rewrite runs as the final constructor step, after the existing server-message promotion, and only replaces the last messageStack entry so the API_ACTIONS[action] context line stays visible.
…mmand getUser's local 401 message duplicated the centralized credential rewrite and, worse, suppressed it for OAuth sessions by always passing a customMessage. Gate the local message on an unknown credential context (the login --token validation path, before any session exists) so an OAuth session's 401 goes through the centralized "OAuth login is no longer valid" message instead of the masked-token text. Also collapse the message to a single line; the old template literal leaked its source indentation into the terminal output.
Without stories:write, `stories push` printed the same missing-permission message once per story, ballooning to ~1500 lines at 500 stories. A missing permission is a property of the credential, not any individual story, so every remaining item would fail identically. FailureCollector now tracks whether a recorded failure is credential-level (APIError.fatal) via hasFatal, and each of the four push loop bodies checks it to short-circuit further recording once set.
…d exit nonzero The prior fix only suppressed repeated reporting; readLocalStoriesStream, mapReferencesStream, and writeStoryStream kept running and writeStoryStream kept issuing PUT requests for every remaining story after the first credential-level failure. Add a shouldStop hook to all three stream stages plus a break in the pass-1 level loop, all keyed off FailureCollector.hasFatal, so no-longer-useful work is skipped instead of merely going unreported. Also: stories push never set process.exitCode, so a run that failed every story exited 0. Set it in the finally block (mirroring assets/push), guarded so it never overrides an exit code handleError already set. Move the mapReferencesStream onStoryError log call above its fatal guard so the audit trail comment stays true, and note why scanLocalStoryIndex's onError needs no guard (its errors are never fatal APIErrors).
The push summary headline counted attempted creations as "pushed" and folded skipped creations into "succeeded", so a run that wrote nothing (e.g. every story already existed and every update failed) still printed a fully green summary. The headline now counts updateResults.succeeded only, since a story's content is only written during the update phase; creation only reserves a placeholder id, so summing creation and update successes would double count every newly-created story. Skipped creations get their own label in the per-phase row instead of being folded into "succeeded".
…is forbidden shared_asset_folders carries no scope annotations, so an OAuth credential is 403'd regardless of consent. `push --target auto` and `pull --target with-referenced` now degrade to the space scope with a warning instead of failing outright; explicit `--target shared`/`--target all`/`--library` requests still fail with the actionable message.
resolveOAuthClient() threw one message advising --oauth and a PAT login regardless of caller. logout and the preAction refresh path surfaced that login advice while removing credentials or silently refreshing a session. Reduce the thrown error to the cause only; each caller now appends its own consequence and remedy.
The try/catch blocks added for the login and refresh remedies had zero coverage: login-flow.test.ts mocks ./client wholesale, and refresh.test.ts always sets the env-var override, so neither catch path ever ran. Add one test per caller that reaches the real placeholder-client guard and asserts on each caller's distinctive remedy text.
Also documents three stories push behavior changes from the preceding tasks: halting on the first credential-level failure, exiting 1 on any story failure, and the summary headline counting only stories whose content was actually written.
…iscovery degrade - credential-hint.ts: give PATs their own space-not-allowed message instead of reusing OAuth wording (consent screen, empty authorized-spaces list); omit the authorized-spaces parenthetical entirely when no space ids are known, for both credential kinds. - APIError now exposes the raw pre-rewrite serverError string so listLibrariesOrDegrade/buildLibraryRootResolver can degrade only on the unsupported-token-type signature instead of any 403. - Add missing coverage: customMessage suppression invariant, pat 401 context, and a different-403-still-fails regression for the library discovery degrade paths. - login-flow.ts/refresh.ts: type `client` explicitly and drop `as Error` casts in favor of instanceof narrowing. - stories/push: fix a stale "422" comment (helper returns 500), reset process.exitCode in afterAll so it can't leak into other suites, and document the exit-1-on-any-failure behavior in the push command README.
Manual QA against a real OAuth grant surfaced five issues:
- Shared-folder discovery reported "Failed to pull library folders" on the
push path. The lookup serves both pull and push, so the action is now
verb-neutral: "Failed to list library folders".
- A fatal credential failure stops a phase mid-run, but the summary still
printed the full denominator ("0/10 succeeded, 1 failed"), leaving nine
stories silently unaccounted for. Phase lines now name them explicitly.
- `stories push` exited 0 when it bailed on a precondition (no local
components, schema issues), so CI read an aborted push as a clean run.
Both now go through `handleError`, matching the sibling --space check.
- `login --oauth` opened a browser before discovering the callback port was
taken, sending the user through a consent screen whose redirect it could
never receive. `waitForCallback` becomes `startCallbackServer`, which
resolves only once bound, so a conflict fails before the tab opens.
- test/GUIDE.md claimed a port conflict hangs for the full callback timeout,
which is no longer true, and lacked the headless consent procedure that
makes the whole OAuth flow testable without a browser.
Fixes DX-486
@storyblok/angular
@storyblok/astro
@storyblok/api-client
storyblok
@storyblok/experiments
@storyblok/js
storyblok-js-client
@storyblok/lint-config
@storyblok/live-preview
@storyblok/management-api-client
@storyblok/migrations
@storyblok/nuxt
@storyblok/react
@storyblok/region-helper
@storyblok/richtext
@storyblok/schema
@storyblok/svelte
@storyblok/vue
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #703 (
feat/DX-490-cli-oauth-login) — review that one first.OAuth grants are scoped, so a logged-in user can be allowed to do some things and not others. Today the server's refusals surface raw:
Insufficient scope: stories:write is required, or a bare 403 classified as a generic error. This turns the five server signatures into one-line diagnosis-plus-remedy messages, and fixes the three bulk/summary behaviors that live testing exposed along the way.Approach
A pure matcher (
utils/error/credential-hint.ts) maps(status, serverError, credentialContext)to a message, branching on OAuth vs PAT.program.tssets the context once per run in thepreActionhook;APIError's constructor applies the rewrite, so all ~100handleAPIErrorcall sites benefit without being touched. No command→scope map, no persisted scope list — the server stays the source of truth and the CLI only translates.Context is pushed rather than imported to break the
utils/error → session → oauth/store → utilscycle.Also: 403 no longer classifies as
errorId: "generic".Beyond the message catalog
Three things live testing forced into scope:
FailureReasonGroupcollapsing is assets-only;stories pushreported per story, so 10 stories meant 10 identical errors.)assets pushworks again for OAuth users. Library discovery hitsshared_asset_folders, which is unannotated and therefore default-denied for every scoped credential — no consent can grant it. Implicit discovery (--target auto,with-referenced) now degrades with a warning; explicit--target shared|alland--librarystill fail loudly. See the backend note below.skippednow has its own label.Behavior change worth flagging
stories pushnow exits 1 when any story failed, where it always exited 0 before. This fires on pre-existing failure modes too (one bad reference, one 500), so a pipeline that tolerated partial pushes flips red. It matchesassets pushand is documented instories/push/README.md, but it's the one change here that can break someone's CI.Backend follow-up
shared_asset_folders_controllerandshared_internal_tags_controllerlackrequire_token_scopes, soenforce_pat_default_denyrejects every scoped credential regardless of granted scopes.credential.rb:23means an OAuth grant can never receive theuser_permissionbypass, so this is not fixable by consent. The degradation above is a workaround; a storyrails ticket annotating those two controllers is the real fix. Verified live with all 34 scopes granted.Verification
lint,test:types, 1378 tests, andbuildall clean with the Nx cache skipped. Live-verified against an OAuth grant before implementation; a manual QA pass against the finished code is still outstanding.