feat(oauth2): add an error hook so server_error causes are observable - #65
Merged
Conversation
An RFC 6749 §5.2 response carries a code, a description and a URI — never the cause. A server_error therefore reached the client as an opaque 500 and was dropped everywhere else, leaving an operator with nothing to diagnose. RFC 7009 §2.2 made it worse on /revoke: a failed revocation still answers 200 OK, so the failure was unobservable too. ServerConfig.OnError (type ErrorHook) is called with the normalized *Error envelope — cause intact — at the point the server decides on its answer: every RFC 6749 §5.2 body, every /authorize redirect, the pre-redirect refusals answered with a bare 400, and the best-effort revocations /revoke swallows. grant.Config.OnError does the same for the family revocation a grant swallows during BCP §8.10.3 reuse detection. The hook is purely observational — no response, status code or redirect changes. Refs #63.
Coverage Report for CI Build 32661203771Coverage increased (+0.07%) to 91.755%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
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.
Closes #63.
Problem
An RFC 6749 §5.2 response carries a code, a description and a URI — never
the cause. The grants and the endpoints attach the cause carefully
(
oauth2.ErrServerError.WithCause(err), 14 sites), butwriteOAuthErrorread everything except
Cause, andServerConfigexposed no logger and nohook. An operator facing a 500 on
/tokenhad no way, from anywhere, tolearn why.
RFC 7009 §2.2 made it worse on
/revoke: revocation answers200 OKevenwhen the revocation itself failed, and the failure was dropped
(
_ = s.cfg.Storage.RevokeAccessToken(...)). A revocation that fails isexactly the event an operator needs to see.
What this adds
A new public type
oauth2.ErrorHook:wired at two levels:
ServerConfig.OnError— called with the normalized*oauth2.Errorenvelope, cause intact, at the point the server decides on its answer:
/token,/revoke,/introspect, metadata) —writeOAuthErrorbecame a*Servermethodtaking the request context;
/authorize— same forredirectAuthorizeError;/authorizeanswers with a bare 400(unknown client, unregistered
redirect_uri,ParseForm), which left notrace at all — the
LoadClienterror in particular was swallowedentirely;
/revokeswallows to honour RFC 7009 §2.2.grant.Config.OnError— the same hook for the family revocation agrant swallows during BCP §8.10.3 reuse detection
(
grant/refresh_token.go:63). The returned error staysoauth2.ErrRefreshTokenReused, soerrors.Iskeeps working — the failuretravels through the hook, not through the returned error.
Usage:
Wire behaviour
Unchanged. Same codes, same statuses, same redirects, same bodies. The hook
is purely observational and optional — a nil
OnErroris a no-op, so thisis backward compatible for every existing
ServerConfig/grant.Configliteral.
Design notes
strictly more informative (the cause is reachable via
errors.Unwrap/errors.As) and lets the application filter on the OAuth2 code withoauth2.IsCodeinstead of guessing.server_error— as the issuerequested. Filtering by code is documented and shown in the example,
otherwise normal 4xx traffic floods the logs.
implementation slows the request down.
/revokestay quiet — an unknown token is not anincident, only a failed revocation is.
Files
oauth2/hook.go(new),server.go,token_endpoint.go,authorize_endpoint.go,revoke_endpoint.go,introspect_endpoint.go,metadata_endpoint.go,doc.gooauth2/grant/grant.go,oauth2/grant/refresh_token.gooauth2/hook_test.go,oauth2/grant/hook_test.go— 10 testscovering the server_error cause, the notified 4xx, the optional hook, the
/authorizepre-redirect and redirect paths, failed access-token andfamily revocations, and the silence on an unknown token / a successful
revocation.
CHANGELOG.md,docs/observability.md(new "OAuth2 error hook"section),
examples/oauth2/main.go(demo wiring).Checks
make test— 26 packages pass with-race;oauth2coverage 90.9% ->91.0%,
oauth2/grant91.2%.make lint— 0 issues (anestiftriggered bybestEffortRevokewasresolved by extracting
revokeAccess/revokeFamily).make build— OK.