PPT-2536: auth.cr drop-in fixes (public clients, verified cookie, PKCE, refresh identity, session secret, OAuth provider params)#3
Merged
Conversation
When COOKIE_SESSION_SECRET is unset the service generated an ephemeral per-boot key, so sessions did not survive a restart and did not match across replicas — the PlaceOS platform does not provide that env var (the Ruby auth used SECRET_KEY_BASE), so a real deployment logs every user out on each redeploy and fails intermittently behind >1 replica. Fall back to a stable key derived (SHA-256, namespaced) from PLACE_SERVER_SECRET, which the platform always provides and which is identical across restarts and replicas — so sessions persist with no new deployment config. Ephemeral generation remains only when neither secret is set (a bare dev box). Verified in a local platform stack: a session issued before an auth container restart is still accepted afterwards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… a secret The token endpoint declared `client_secret` as a required parameter, so a public client (SPA / native app, `confidential: false`) that authenticates with PKCE — Backoffice, for example — was rejected with a 422 "missing required parameter 'client_secret'" before any OAuth logic ran. Doorkeeper made `client_secret` optional for public clients and skipped client-secret validation for them. This restores that: - `token` now takes `client_secret : String? = nil`. - `AuthlyAdapter::Client#authorized?` returns true for public clients (they authenticate via PKCE); confidential clients still require a constant-time secret match. - `allowed_grant_type?` denies `client_credentials` to public clients, so the secret bypass can't be used to mint owner-scoped tokens from a (public) client_id. - Discovery advertises `token_endpoint_auth_methods_supported: [..., "none"]`. PKCE is left optional (matching Doorkeeper); enforcement can be enabled later via `Authly.config.enforce_pkce`. Specs updated: fixtures that authenticate with a real secret now model a confidential client, and a new "public client" block covers the no-secret code exchange + refresh and the client_credentials denial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nginx gates every SPA asset behind a `verified` cookie: its access_by_lua_block recomputes HMAC-SHA256(SECRET_KEY_BASE, <data>) and redirects to /auth/login whenever the cookie is missing or its signature is wrong. auth.cr never issued it (deferred to "Phase 6"), so after login every asset request bounced back to /auth/login — an infinite loop; the SPA (Backoffice) never loaded. The legacy Ruby service set this cookie at the tail of `new_session` and cleared it in `remove_session`. This restores byte-for-byte parity: - `SECRET_KEY_BASE` constant, read from the same env var nginx renders into its lua (verified: both containers already share the value). - `configure_asset_access` sets `verified = "<16 hex>.<hmac>"` at path /, Secure, HttpOnly, SameSite=None, 19y expiry; `clear_asset_access` expires it. Wired into `new_session` / `remove_session`, plus the inline-api-key continue branch and `/auth/authority` (valid token), matching the Ruby call sites. Spec: asserts the cookie is issued on signin with a signature that recomputes correctly, cleared on logout, and has the right attributes. `Spec.signin!` and one assertion updated to select the session cookie by name now that responses carry two Set-Cookie headers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ts-client / Backoffice — like most OAuth SDKs — send the S256 `code_challenge` as base64url (`-`/`_`). authly validates it against Crystal's `Digest::SHA256.base64digest`, which is *standard* base64 (`+`/`/`, padded), so every real browser PKCE handshake was rejected with `unauthorized_client` (visible only once the client_secret barrier was removed). Normalize the challenge from base64url to standard base64 (and pad to a multiple of 4) before handing it to authly, for S256 only — `plain` challenges are the verifier verbatim and legitimately contain `-`/`_`. auth.cr now accepts both base64url and standard-base64 clients. Underlying cause is in place-labs/authly (S256#valid? should use base64url per RFC 7636); this normalization is the drop-in fix and also insulates auth.cr from that behaviour. Spec drives the full authorize->token flow with a ts-client-style base64url challenge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
authly's refresh grant structurally loses the resource owner: `RefreshToken`
never overrides `user_id`, and the refresh token it mints carries
`sub => client_id` with no user id — so a refreshed access token gets a
random `sub`. Downstream that also strips the `u{n,e,p,r}` block and the
`aud` claim our ClaimsProvider attaches (it does `User.find?(sub)` and
returns early when the sub isn't a real user), leaving an unidentified,
domain-less token that fails `ensure_matching_domain`.
This is the same upstream gap the existing `AuthorizationCode#user_id`
patch already compensates for on the code path; here we extend the identical
mechanism to refresh (no authly shard change):
- reopen `Authly::AccessToken#initialize` to re-mint the refresh token with
the resource-owner id embedded (`@sub` is already the real user id at that
point for user grants; client_credentials is left untouched);
- reopen `Authly::RefreshToken#user_id` to recover it on redeem — a direct
analog of the AuthorizationCode patch.
Identity chains across refresh-of-refresh because each new refresh token
re-embeds the recovered id. Spec verifies sub + u{} + aud survive one and
two refreshes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two GenericOAuth2 drop-in regressions vs the legacy Ruby generic_oauth,
both fixed entirely in auth.cr (no multi_auth shard edit):
(A) authorize_params dropped. The OAuthAuthentication.authorize_params
column (e.g. Google access_type=offline/prompt=consent for refresh
tokens, Azure domain_hint) never reached the authorize request —
multi_auth has no concept of the column. ProviderCallbacks#initiate
already owns and rewrites the authorize URL, so it now merges the
strat's authorize_params into it before redirecting (safe &-append
leaves the embedded redirect_uri untouched for the b2c rewrite).
(B) info_mappings comma-fallback lost. Ruby treated each mapping value as
a comma-separated fallback list (email => "email,mail,userPrincipalName",
first present wins — how Azure/Entra surfaces email); multi_auth does a
single literal lookup so comma-lists resolve to nil. Reopen
MultiAuth::Provider::GenericOAuth2#get_value_from_json from auth.cr
(same technique as authly_adapter.cr) to try each comma-separated key.
It's the single choke point for every mapped field, so it also fixes
the getter-only uid/name that can't be overridden after construction.
Specs added to the provider-flows suite cover both, incl. a comma-fallback
case that exercises the getter-only uid field.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…E_SERVER_SECRET) Investigation of the dev + k8s deployments showed auth does NOT receive PLACE_SERVER_SECRET in prod (it's wired to api/core/fe-loader for settings encryption, not auth). Keying the session-cookie secret off it would fall back to an ephemeral per-boot key in prod, so sessions would still not survive a restart — the exact bug this fix targets. auth already receives SECRET_KEY_BASE in both deployments (docker-compose `.env.secret_key` and k8s `auth.secrets.SECRET_KEY_BASE`), it's the value nginx shares for the asset-access cookie, and it's what Rails/Doorkeeper derived its session key from — so it's the correct, faithful choice. Derivation stays SHA-256-namespaced so it doesn't collide with the raw-HMAC use of the same secret. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on agnostic) The trailing comment made crystal 1.16.3 insert a blank line before ensure that crystal:latest strips, so the crystal-style (latest) and Ameba (older) CI checks disagreed. Moving the comment to its own line formats identically under both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Lands the PPT-2536 drop-in fixes that make auth.cr a working replacement for the legacy Ruby auth, validated end-to-end (Backoffice logs in + refreshes tokens through the real browser flow against the local stack).
Fixes (each a distinct commit)
client_secret, 422-ing every Backoffice (public/SPA) token exchange. Public clients now authenticate via PKCE;client_credentialsstays confidential-only.verifiedasset-access cookie — auth.cr never issued it, so nginx bounced every SPA asset to/auth/login(infinite loop). Restored, keyed on the sharedSECRET_KEY_BASE.sub; the refreshed token lost the user (and theu{}/audclaims). Recovered via the existing monkey-patch pattern.PLACE_SERVER_SECRETin k8s).access_type=offline/prompt=consentand AzureuserPrincipalNamemappings, entirely in auth.cr (no multi_auth edit).Testing
🤖 Generated with Claude Code