feat(auth): Doorkeeper route parity with the legacy Ruby auth service (PPT-2536) - #2
Merged
Merged
Conversation
…serinfo
Adds the remaining Doorkeeper discovery surface for drop-in parity
(PPT-2536): /auth/.well-known/{openid-configuration,oauth-authorization-server},
the RFC 8414 root alias, webfinger at both mounts (400 without resource,
matching Rails ParameterMissing), and the POST verb on userinfo (OIDC
Core 5.3). The provider document now advertises the /auth/oauth/*
endpoint family plus jwks_uri and introspection_endpoint.
Note: at the legacy service's locked gem versions these endpoints 500
(doorkeeper-openid_connect 1.10.1 issuer-block arity regression); this
implements the documented intent.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Publishes the RS256 verification key as a JWKS document at the legacy Doorkeeper-openid_connect mount point (PPT-2536), derived from the same key JWT_SECRET configures for signing. RFC 7638 thumbprint kid. RSA n/e are extracted by walking the public-key PEM's DER directly (both SPKI and PKCS#1 armors) since the openssl_ext bindings in our dependency tree don't expose RSA_get0_key. Golden-fixture spec pins the kid and modulus for the dev keypair. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Serves POST /auth/oauth/introspect (RFC 7662) and GET /auth/oauth/token/info for drop-in parity (PPT-2536), each at the short /auth/* path and the legacy /auth/oauth/* mount. Both source their fields (client_id, resource owner, scope, timestamps) from the persisted Model::OAuthToken record looked up by jti, not from the JWT claims -- the access-token JWT's aud is the authority domain and carries no client id. Introspection authenticates the caller (client credentials via Basic or params, or a bearer token) and only reveals a token's state to the application that owns it. Also updates the discovery assertions in oauth_spec to the /auth/oauth/* endpoint paths advertised since the well-known parity work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Completes the authorization-endpoint parity (PPT-2536): POST /authorize (the consent submit, which issued a grant directly since the legacy service never rendered a consent screen), DELETE /authorize (deny, which redirects back with error=access_denied), and GET /authorize/native (the out-of-band code display page). All at both the short and /oauth/ mounts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…outes
Adds the Doorkeeper /auth/oauth/applications CRUD (index/new/edit/show/
create/update/destroy) and /auth/oauth/authorized_applications
(index/destroy) for drop-in parity (PPT-2536).
Applications are admin-gated on a signed-in sys_admin (non-admins get
404, matching the legacy service's intent) and reproduce the gem's JSON
quirks: 204-empty index, full record incl. plaintext secret, {errors:[]}
on 422. Authorized-applications are session-scoped to the resource owner
and list/revoke by the tokens they hold.
In the platform these records are managed via rest-api's /oauth_apps +
Backoffice; these routes exist for wire parity with the Ruby service.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The legacy service appended a root catch-all (/*any -> errors#not_found) returning 404 for any unmatched path and verb. action-controller's default no-route behaviour already matches; this locks it in (PPT-2536). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… docs Adds the POST /auth/signup route, returning 403. Ruby only completed a signup with a valid short-lived social cookie, which auth.cr never issues (OAuth users are auto-created in the provider callback), so 403 is the only state reachable in either service. The user-creation path stays intentionally dropped; this gives route-table parity (PPT-2536). Also allowlists the additive short-path /auth/* aliases in route_diff.sh (route parity now reports OK) and updates parity_matrix.md to match the implemented behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Completes the auth_migration folder (PPT-2536): the Rails route dump, the auth.cr baseline route table, and a README indexing the parity artefacts alongside the already-committed route_diff.sh + parity_matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review found that a bearer-authenticated introspection caller could introspect ANY token: authenticate_introspection_caller returned an empty client id for bearer callers, which short-circuited the cross-client guard and disclosed active/revocation state, client_id, scope and exp for tokens belonging to other applications and users. The caller is now identified by its own application (via the bearer's persisted token record), and the same-application restriction applies to bearer callers too: a bearer caller may only introspect its own app's tokens and never the token it authenticated with (both -> 401 invalid_token), matching Doorkeeper's default policy. A request with no credentials now returns 400 invalid_request (was 401), and 401 responses carry the WWW-Authenticate + Cache-Control headers Doorkeeper emitted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- deny authorize (DELETE /authorize) now validates redirect_uri is registered for the client before redirecting, closing a potential open redirect - applications create/update accept JSON request bodies (nested or flat), not only form-encoded, matching Doorkeeper - new applications default to confidential (Doorkeeper's default; the model default is false), so a new app's uid is not exposed in the public authorized_applications shape - token/info floors expires_in at 0 for a non-expiring token - fix a Style/VerboseBlock ameba failure in discovery_spec that would have failed the crystal-style CI job - test hygiene: clear OAuthToken between suites; add coverage for bearer-caller cross-client isolation, self-introspection, the no-auth 400, WWW-Authenticate headers, deny open-redirect rejection, JSON-body create, authorized-apps revoked/expired exclusion, and signup 403 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
I also got Claude to break down what it's confident about and what might have issues: What I'm genuinely confident about (low risk):
What Steve may well push back on (these aren't bugs — they're judgment/scope calls he's entitled to have opinions on):
|
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.
What & why
Makes the Crystal
auth.crservice a drop-in replacement for the legacy Ruby/Rails Doorkeeper auth at the route level (PPT-2536). Building on the wire-parity work in #1, this adds the remaining Doorkeeper/OIDC endpoints so that every route the Rails service exposed is served here with matching behaviour../auth_migration/route_diff.shreports route parity: OK — every Rails route is matched (the additive short-path/auth/*aliases are allowlisted).Routes added
Discovery / OIDC
/.well-known/oauth-authorization-server+ the/auth/.well-known/{openid-configuration,oauth-authorization-server}mount variants, andwebfingerat both rootsGET /auth/oauth/discovery/keys— JWKS derived from the signing key (RFC 7638 thumbprintkid)POST /auth/oauth/userinfo(the OIDC §5.3 verb; the discovery doc now advertises the/auth/oauth/*mounts +jwks_uri+introspection_endpoint)Token endpoints
POST /auth/oauth/introspect(RFC 7662) — caller-authenticated (client credentials or bearer), Doorkeeper's cross-client visibility policyGET /auth/oauth/token/infoAuthorization endpoint
POST(consent submit) andDELETE(deny) on/auth/oauth/authorize, plus the native OOB code pageApplication management
/auth/oauth/applicationsCRUD (admin-gated) and/auth/oauth/authorized_applicationsMisc
POST /auth/signup(403 stub for route parity — see note), catch-all 404 parityParity notes / accepted divergences
Every deliberate difference from the Ruby service is documented in
auth_migration/parity_matrix.md, per route. Highlights:userinforeturns a superset of Ruby's claims (Ruby effectively returned onlysubbecauseprofile/email/phonewere never registered as issuable scopes).doorkeeper-openid_connectissuer-block arity regression) — this implements the intended behaviour.POST /auth/signupis served but always returns 403: auth.cr never issues the short-livedsocialcookie the Ruby signup required (OAuth users are auto-created in the callback), so 403 is the only reachable state in either service. The user-creation path stays intentionally dropped — resurrecting it is a separate product decision.{"errors":[…]}on 422) but serves JSON rather than the gem's HTML admin views — in the platform these are managed via rest-api's/oauth_apps+ Backoffice.Testing
./test)crystal tool format --checkcleanAn adversarial multi-lens self-review before opening this PR caught and fixed a token-introspection cross-client visibility bug (a bearer caller could introspect another application's token) and hardened several parity gaps; see the last two commits.
The
auth_migration/folderContains the parity artefacts: the Rails + auth.cr route dumps,
parity_matrix.md, androute_diff.sh(a mechanical parity check that can run in CI). Happy to relocate these if a maintainer would prefer them elsewhere.Open questions for reviewer
A few deliberate choices where I'd value @stakach's call — flagging them so they read as intentional rather than oversights:
auth_migration/folder placement. It ships in the service repo (route dumps,parity_matrix.md,route_diff.sh— the last is a mechanical parity check that could run in CI). Happy to move it to a docs/investigation location or drop it from the service repo if you'd prefer.POST /auth/signup. The route is served but always returns 403 — auth.cr never issues thesocialcookie the Ruby signup required, so 403 is the only reachable state. This is purely for route-table parity; the user-creation path stays dropped per the README. If you'd rather not carry a stub route at all, I'll remove it and record signup as an intentional route-level omission instead./auth/oauth/applications(+authorized_applications) is implemented for faithful parity, but it's superseded in the platform by rest-api's/oauth_apps+ Backoffice. If parity here isn't worth the surface, I can drop it (and the placeholdernew/editHTML forms) and document it as a deliberate divergence.confidential-defaults-true fix lives in the controller rather than changing theoauth_applicationsmodel default (which isfalse); and the authorize endpoint's redirectable errors return a JSON 400 rather than a 302 back toredirect_uri?error=…(pre-existing GET-handler behaviour, now shared by the new POST alias). Both are documented inparity_matrix.md— say the word if either should change here.🤖 Generated with Claude Code