fix(auth): match production's authenticate failure shape per grant - #53
Merged
Merged
Conversation
…ction magic-auth error codes
Greptile SummaryThe PR aligns authentication failures with production’s grant-specific response contracts.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
|
![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.
The third instance of a hole already closed twice on this branch. Deleting
a user cascades to its sessions, memberships, factors, identities, password
resets, email verifications and magic auths — but not to its authorization
codes, so a code outlives the user it was minted for.
Redeeming one fell through to the shared lookup and answered 404
{"message":"User not found","code":"not_found"}: the spec shapes an
authenticate 404 as a bare {message}, so there is no `error` for a client
matching invalid_grant and no `code` worth reading either. It is also the
grant an AuthKit callback actually takes, and the path authkit-nextjs uses
to decide a session is over.
Guarded before the delete, like the device code's twin of this, so a
failure no retry can fix does not also cost the caller their code. Routed
through failAuth rather than a bare throw, because every other
authorization_code failure emits authentication.oauth_failed and this one
was silently skipping it.
The spec settles what the last pass had to reason about. Its authenticate
400 lists `invalid_request` among the {error, error_description} variants
and nowhere among the {code, message} ones, so a missing or unrecognized
parameter is OAuth-shaped on every grant — including the grants whose
credential failures are plain. The envelope is a property of the failure,
not only of the grant, and the eleven throws here now say so.
This is the argument 1ab8ec2 made for /sso/token's missing-code path,
applied to the endpoint it was skipped on. It was sharpest on PKCE, where a
wrong code_verifier answered {error, error_description} and a missing one
answered {code, message} two lines away: the same grant, the same request,
two envelopes depending on which way the client got it wrong.
The unrecognized-grant_type branch keeps `invalid_request` rather than
moving to `unsupported_grant_type`, which appears exactly once in the whole
spec, under /sso/token, and never in authenticate's 400. The asymmetry
reads as real rather than an omission: authenticate's body is a oneOf
discriminated on grant_type, so an unknown value fails body validation
instead of reaching a handler that could decline it. What changes is the
message, which claimed "Unsupported grant_type" under a code that says
malformed request.
Also corrects the PKCE comment. e09d42e recorded that the spec does not
enumerate invalid_grant for authenticate at all, which is why that case
was decided by RFC 7636 alone; it does enumerate it, as an
{error, error_description} variant, so the case is spec-backed like its
siblings.
1ab8ec2 asserted that /sso/token answers OAuth-shaped throughout, and the test beside it went further: "the endpoint has no plain-shaped response for a caller to have to parse." Twenty lines below the comment, a stored authorization pointing at a missing profile throws a plain 500. The code is right — that is emulator state gone wrong, not a request anyone can fix by sending something else, and RFC 6749 §5.2's code list covers client errors only, so there is nothing to render it as. The claim is what was wrong. Both now say every failure a caller can *cause*, and the branch itself explains why it is the one that isn't.
Collaborator
|
thanks! |
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