fix(auth): match production's authenticate failure shape per grant - #53
Open
grayashh wants to merge 9 commits into
Open
fix(auth): match production's authenticate failure shape per grant#53grayashh wants to merge 9 commits into
grayashh wants to merge 9 commits into
Conversation
…ction magic-auth error codes
Greptile SummaryThe PR aligns authentication failures with production by introducing a shared OAuth error envelope while preserving plain errors for grants that require them.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (7): Last reviewed commit: "test(auth): cover both expired-code path..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The spec settles what probing could not. Its authenticate 400 lists the
device-flow codes (expired_token, authorization_pending, slow_down,
access_denied) as {error, error_description}, gives /sso/token nothing but
OAuth-shaped errors, and puts invalid_credentials in the plain set at 400.
That leaves the rule as an explicit allowlist rather than "RFC 6749 grants
fail OAuth-style": password is an RFC 6749 grant and still fails plain,
verified against a live environment. Stating it as a category was going to
mislead the next reader, since the category does not predict the shape.
PKCE is the one case decided by reasoning rather than evidence: the spec
does not enumerate invalid_grant for authenticate at all, even though the
live API returns it, so silence there is not evidence against. RFC 7636
§4.6 makes a failed verifier an invalid_grant, and it is the same grant on
the same endpoint already verified to fail that way.
Review follow-ups, all narrowing the gap between what the comments claim
and what the code does.
/sso/token's missing-code path still threw the plain envelope, directly
under a new comment asserting the endpoint answers OAuth-shaped throughout.
RFC 6749 §5.2 names a missing required parameter invalid_request, and it is
the one failure a client meets before it has a code to present — the worst
one to make it parse differently from the rest.
/oauth2/token had its own oauthError() building the identical
{error, error_description} body by hand, so the OAuth envelope was defined
in two places and the reusable one was reachable from everywhere except the
endpoint most obviously about OAuth. Throwing OauthApiError leaves one
definition; the m2m tests pass untouched, which is the point.
Three shapes shipped with no test: the expired refresh token's distinct
description, the device flow's expired_token (a polling client stops there
where authorization_pending tells it to keep going), and /sso/token's
unsupported_grant_type, which had no coverage before this change either.
gjtorikian
changed the title
An approved device code whose user was deleted fell through to the shared
lookup and answered with a {message, code} 404 — the one envelope this
endpoint otherwise never returns, on the grant whose whole contract is that a
polling client reads `error` to decide whether to keep going. The
refresh_token twin of this was already fixed; this mirrors it, and throws
before the delete so nothing is spent on a failure polling cannot resolve.
/sso/token reported an omitted grant_type as "not supported: undefined",
which names neither the problem nor anything the caller sent. Absent is a
malformed request — RFC 6749 §5.2 invalid_request — not a request for a grant
the endpoint declines to support.
The device-flow comment also implied slow_down and access_denied are among
the codes returned here. The spec defines them; the emulator has no
polling-interval or user-denial surface to emit either from.
And documents both error classes where error hooks are described, since a
hook that raises a failure rather than describing one now has two envelopes
to choose between.
e09d42e replaced "RFC 6749 grants fail OAuth-style" with an explicit allowlist precisely because the category did not predict the shape and was going to mislead the next reader. The allowlist then grew a third member — device_code — without the prose following it, so both the README and the comment beside the password grant still said two, one line above a table listing three. A reader checking whether their grant is OAuth-shaped counts the rows, not the sentence, but a sentence that disagrees with the table beneath it costs them the trust that makes the table worth reading.
The two remaining shapes this branch changed without pinning. Both expired-code paths moved off the plain envelope — authenticate's authorization_code from expired_code, /sso/token's from expired_code — and neither had a test, so the only evidence they render invalid_grant was that their unknown-code siblings do. They are not those siblings with a different label. /sso/token's expired branch resolves the profile behind the code first, so the authentication.sso_failed it emits carries the organization and connection the unknown-code event leaves null, and it consumes the authorization where the unknown one has nothing to consume. Asserting the org and connection is also what proves the test reached the expired branch at all rather than falling through to the unknown one. Both mint a real code and back-date it, matching how the expired refresh token and device code are already tested, so an expiry that stops being detected fails here rather than passing as a bad code.
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
2 participants
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.
Production does not use one failure shape for
POST /user_management/authenticate. Which grants fail OAuth-style is an explicit allowlist, not a category:passwordis an RFC 6749 grant and still fails with the plain{code, message}shape, verified against a live environment. The spec settles the rest (probe transcript in #51).authorization_code(unknown, expired, or badcode_verifier) andrefresh_token(unknown, expired, rotated, or user deleted) now render{error, error_description}via anOauthApiErrorsubclass. This is the class the Node SDK'sOauthExceptionexists for, and whatauthkit-nextjsmatches (error === "invalid_grant") to end a session when a refresh fails — against the emulator that path previously fell through toGenericServerException.expired_token,authorization_pending,slow_downandaccess_deniedas{error, error_description}. These already carried OAuth error codes in the plain envelope, so a client matchingerrorsaw nothing and one matchingcodeworked — the inverse of every other grant.invalid_grant, the same way an unknown code does (RFC 7636 §4.6). This is the one case decided by reasoning rather than a probe: the spec does not enumerateinvalid_grantfor authenticate at all even though the live API returns it, so its silence is not evidence against.passwordmoves 401 → 400 and its message now interpolates the email (Invalid credentials for 'x@y.test'.), matching live. The shape stays plain. Anything asserting 401 on a bad password needs updating.invalid_one_time_code/ "Invalid one-time code" andone_time_code_expired/ "One-time code for '…' has expired." (wereinvalid_code/expired_code)./sso/tokenis OAuth-shaped throughout, matching its spec definition —unsupported_grant_typefor a wrong grant,invalid_grantfor a bad or expired code, andinvalid_requestfor a missingcode(RFC 6749 §5.2), so the one failure a client hits before it has a code is not also the one it cannot parse like the rest./oauth2/tokennow throws the sameOauthApiErrorinstead of hand-building an identical body through a localoauthError()helper, leaving one definition of the OAuth envelope rather than two.authentication.*_failedevent payloads keep the spec's{code, message}error object:OauthApiErrorextendsWorkOSApiErrorreusing the same fields, sofailAuthand the error hooks need no change. Event error codes do change with the responses (invalid_code→invalid_grant)./user_management/email_verificationroute.Closes #51