fix: make it run (PPT-2536) - #1
Merged
Merged
Conversation
The authorization_code grant relied on Authly's default
grant_strategy.user_id (nil), so issued tokens carried a random sub.
As a result the ClaimsProvider could not resolve the user and omitted
the aud claim and the legacy u{n,e,p,r} metadata block.
Recover the user_id captured in the authorization code (matching the
legacy Ruby Doorkeeper::JWT token, whose sub is the user id) so sub,
aud, and u are populated for downstream PlaceOS services.
Adds spec/controllers/oauth_token_claims_spec.cr covering the code flow.
Authly builds the scope claim as a space-separated string, but the legacy Ruby service emitted scope: Array(opts[:scopes]) and downstream PlaceOS services decode it into UserJWT#scope : Array(Scope). A string scope makes UserJWT.decode raise. Split the scope string into an array in the ClaimsProvider so issued tokens are decodable by consumers.
Authly adds a cid (client id) claim that the legacy Ruby Doorkeeper token never carried. Delete it in the ClaimsProvider so the emitted claim set matches the legacy token shape. The persisted OAuthToken record still records the client id via the token-store metadata, which is populated independently of the JWT payload.
The legacy Ruby service mounts its Doorkeeper OAuth server endpoints under /auth/oauth/* (token, authorize, revoke, userinfo), and that path family is the documented public contract for server-to-server and native app clients (auth/README.md, auth/docs/sample_auth.md). auth.cr serves them at /auth/*. Add /auth/oauth/* route aliases on the existing actions so clients that hardcode the Doorkeeper paths keep working after the swap. The OIDC discovery document continues to advertise the /auth/* paths for discovery-driven clients.
The legacy Ruby service accepts browser login forms posted as application/x-www-form-urlencoded, but action-controller only registers a JSON body parser by default, so POST /auth/signin returned 415 for form posts. SigninBody already defined .from_form; wire it up by registering a form-urlencoded body parser on the Application base controller so form logins are accepted (202 + Set-Cookie), matching the legacy behaviour.
The legacy Ruby logout redirects via redirect_continue(continue || "/"), issuing a 302 to the continue target when safe, to "/" when no continue is given, and to the authority logout_url only as the cross-domain fallback. auth.cr issued a 303 and defaulted to the authority logout_url even with no continue. Align the target selection and use 302 so logout behaves as a drop-in for existing clients.
Authly's AccessToken#expires_in derives from an ACCESS_TTL constant captured at class-load time, before configure! applies the 2-hour access TTL, so the token response reported the authly default of 3600s while the JWT exp claim was a full 7200s. Report the live configured TTL so the RFC 6749 expires_in matches both the JWT exp and the legacy 2-hour Ruby service.
chillfox
changed the title
chillfox
changed the title
External IdP app registrations pin the exact redirect_uri the auth service sends. The legacy Ruby service (generic_oauth#callback_url) sends .../auth/oauth2/callback?id=<strat>, and its RewriteRedirectResponse middleware rewrites that to the path form .../auth/oauth2/callback/<strat> for *.b2clogin.com hosts (Azure B2C won't round-trip a query string on redirect_uri). auth.cr was sending a bare .../callback with no id, which every external IdP would reject as an unregistered redirect_uri, and the strat id never round-tripped. - callback_uri now carries ?id=<strat>, byte-identical to the legacy URL, used for both the authorize redirect and the token exchange. - Add rewrite_b2clogin_redirect to convert the authorize redirect's encoded redirect_uri to the path form for *.b2clogin.com hosts, mirroring RewriteRedirectResponse. The inbound path form was already handled by the callback_alias route (RewriteCallbackRequest equivalent). This makes external OAuth2 SSO a true drop-in: no IdP re-registration.
The legacy Ruby service registered SAML as the OmniAuth strategy 'adfs' (generic_adfs). That name is baked into external IdP registrations, the /auth/adfs kickoff+callback paths, the login-event payload, and the UserAuthLookup id (auth-<authority>-adfs-<uid>). auth.cr used 'saml', which would 404 the /auth/adfs callback and \u2014 worse \u2014 fail to resolve every existing SAML-linked account (recreating users instead). It also advertised its own computed callback as the SAML ACS URL rather than the DB-configured assertion_consumer_service_url the IdP has registered, and never sent the strat's name_identifier_format. - Register the SAML factory under 'adfs' (primary) with 'saml' kept as an internal alias; the recorded provider name is always 'adfs'. - Advertise strat.assertion_consumer_service_url (DB) as the ACS URL. - Pass strat.name_identifier_format through to the AuthnRequest. Makes SAML SSO a true drop-in: no IdP re-registration, no orphaned UserAuthLookup rows. Adds SAMLRequest-decoding specs asserting the ACS URL + issuer come from the DB and that /auth/adfs is served.
Adds end-to-end drop-in coverage for the common external OAuth2 IdPs a tenant configures as generic_oauth strats: - Google: authorize request + registered redirect_uri (?id= form) and a full round-trip mapping the Google claims (sub/given_name/family_name). - Azure AD (Entra): authorize request + redirect_uri and a round-trip mapping the Azure claims (oid/given_name/family_name). - Azure B2C: asserts the *.b2clogin.com redirect_uri path-form rewrite and completes the round-trip via the path-form callback. Each lands a UserAuthLookup under the oauth2 provider, matching the legacy service. Locks the redirect_uri/b2clogin behaviour so external IdP registrations keep working across the swap.
Adds negative/edge-case coverage for the external provider flows, which surfaced a drop-in gap: a failed code->token exchange raised an uncaught OAuth2::Error (500) and a non-JSON userinfo body raised JSON::ParseException (400). The legacy OmniAuth service bounced every provider round-trip failure to /auth/failure (302), so mirror that: rescue OAuth2::Error, JSON::ParseException, and IO::Error around engine.user and redirect to the failure page. New tests (spec/controllers/oauth_provider_flows_spec.cr): - token endpoint error -> 302 /auth/failure - userinfo endpoint error -> 302 /auth/failure - B2C state mismatch on the path-form callback -> 401 - B2C emails[] array claim mapped via emails[0] index syntax
chillfox
marked this pull request as ready for review
July 16, 2026 03:23
Member
|
LGTM very exciting |
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.
No description provided.