refactor(server): extract the interactive auth flow into an authflow package - #4915
Merged
Conversation
nabokihms
force-pushed
the
authflow-nextstep
branch
6 times, most recently
from
July 17, 2026 12:17
948a585 to
07b6547
Compare
…package Move the browser-facing authorization flow (/auth, connector and password login, session SSO, MFA, approval and RP-initiated logout) out of the server god-object into a dedicated server/authflow package, decomposed into focused sub-packages rather than one large handler: - server/authflow/session: the session cookie, SSO lookup and auth-session CRUD (session.Manager). - server/authflow/mfa: the authenticator chain and the TOTP/WebAuthn endpoints (mfa.Manager, mfa.Provider and the TOTP/WebAuthn providers). - server/authflow/web: shared browser infrastructure — HTML error rendering and issuer-relative URL building (web.UI), embedded by the components that render. - authflow.Handler: HTTP orchestration only; it holds the components and delegates. Their internals are unexported, so the Handler cannot reach past each component's API. - server/log: the request-scoped logging attributes carried through the context (request ID, client IP), in a neutral low-level package the server, the auth flow and the CLI logger all reference. Two data-oriented abstractions keep the flow honest: - nextAuthStep centralises the post-login decision (MFA chain -> consent -> issue) as behaviour-free data, so the login, session-login and approval handlers share one decision instead of each re-deriving it. - responseTypeHandler issues the authorization response with one self-selecting handler per response_type (code/token/id_token). The Handler takes a narrow Config instead of reaching into the Server and mounts its own routes. session.Config, mfa.Provider and PKCEConfig are owned by the packages; the server and cmd reference them directly. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
nabokihms
force-pushed
the
authflow-nextstep
branch
from
July 17, 2026 12:20
07b6547 to
c7a7cce
Compare
The test files in the server package all exercise the server (through newTestServer or a Server built directly); prefix them server_ so the package's test surface is grouped under one name. Pure file renames. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Rename mfa.go into manager.go (Manager, Provider, chain and step logic), totp.go (TOTP provider and endpoint) and webauthn.go, with matching totp_test.go and webauthn_test.go, instead of a mismatched manager_test.go. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
…kage Move parseAuthorizationRequest, the request validation (redirect_uri, connector_id, id_token_hint) and the request-error types (displayed vs redirected) out of the Handler into a dedicated authflow/authreq package (authreq.Parser). PKCEConfig moves with it — the parser owns it — and the server and cmd reference authreq.PKCEConfig directly. The Handler holds a *authreq.Parser and delegates parsing. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Move sendCodeResponse and the per-response_type issue handlers (code, access token, id_token) out of approval.go into an authflow/authcode package (authcode.Issuer). The consent screen (handleApproval) and the consent check stay as Handler orchestration; the Handler holds an *authcode.Issuer and delegates issuing the response to it. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
The authreq and authcode packages moved code around without clarifying the flow, so fold them back into the authflow package as files — request.go (parse/validate the /auth request) and response.go (issue the code/token response) — the way fosite keeps request handling and response writing as files on one provider rather than separate packages. The real fix is the control flow. The next-hop decision (next MFA factor, consent screen, or issue) was scattered across finalizeLogin, finishSessionLogin and the approval handler, each building the URL itself with a different return shape ((url, skip, err) vs (url, bool)). Introduce advance (nextstep.go): the single place that reads nextAuthStep and dispatches — redirect to MFA or consent, or issue the code — in the spirit of zitadel's NextSteps. finalizeLogin now just finalizes the identity and returns the auth request; connector login, callback and session login each finalize their step and call advance. prompt=none's "no interaction" rule lives in advance too, so it is enforced in one place. Conventions: package docs moved to doc.go in every package, matching the rest of the tree; the test harness file is handler_test.go (not helpers_test.go), and the session tests are sessionlogin_test.go, mirroring their source. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
authStep is now a real interface with a run method: each step redirects to the next MFA factor or the consent screen, or issues the response, and advance just runs whatever nextAuthStep returns — no empty marker methods, no kind switch. Consolidate the sessions-enabled check on h.sessions.Enabled() (config-driven) and drop featureflags from authflow; cmd already sets the session config iff the feature flag is on, so the test servers do the same, keeping one source of truth. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
The package holds per-request context values (request ID, client IP), not a logger, and its client IP feeds both the CLI log attributes and the session audit record. reqctx names what it is; it stays a low-level package because the auth flow reads it and cannot import server. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
…t prompt=none check Drop the positional New constructors on session.Manager, mfa.Manager and web.UI and export their fields, so the Handler builds each with a named-field struct literal — clear which value goes into which field instead of a positional call. Pull the prompt=none gate in advance into blockedByPromptNone. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Give mfa.Manager a Mount that registers its own /mfa routes and make the HTTP endpoint methods unexported — the auth-flow Handler just calls h.mfa.Mount(m) instead of wiring six routes into another component's handlers. Remove two comments left dangling after their functions moved. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
MFA is orthogonal to sessions: it verifies a factor and marks the auth request, so it should not ride on the sessions guard. Mount it as its own router.Handler in the server and gate the routes on having at least one authenticator configured. This also fixes MFA being unreachable (redirect to /mfa 404s) when MFA is set up but sessions are disabled. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
The authorization-response writer (mint the auth code, sign implicit/hybrid tokens, redirect back to the client) was a set of private methods on the browser Handler, yet issuance is a shared flow step: any front-channel flow that completes login funnels through it. Move it into server/authflow/issue, built from the already-shared lower components (storage, tokens.Issuer, the session manager, browser rendering) so it carries no browser-login code. This is a peer to the mfa component; consent will follow and depend on it. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Consent is a shared flow step: the browser login flow and, via the /auth redirect, the device flow both pass through it. It was private to the browser Handler (approval.go). Move it into server/authflow/consent, owning the /approval endpoint, the consent screen, consent persistence and the skip decision (Satisfied). It hands off to the issue component once consent is granted, and checks the mfa chain directly for the mid-flow MFA case, so it never reaches back into the login orchestration. The Handler shrinks toward the login flow plus the step-sequencing spine. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
RP-Initiated Logout is a terminal flow: it ends a session rather than advancing the login/consent/issue sequence, and shares nothing with the spine. Move it into server/authflow/logout, mounting its endpoints only when sessions are enabled (the guard moves onto the component, like mfa). It keeps a small local id_token_hint verifier so it depends on no login-flow code. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
The shared flow steps (mfa, consent, issue, logout) and their shared infrastructure (session, web) are peer domains to grants, device and tokens, not subordinate to the browser-login package. Move them from server/authflow/* up to server/*, matching dex's flat domain layout. authflow keeps only the login flow and the step-sequencing spine, and imports the components as siblings. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
The package name web collided with the existing github.com/dexidp/dex/web (embedded frontend assets). Rename it to render, which also better describes its job: HTML error rendering and issuer-relative URL building for the flow. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
The shared components (session, mfa, consent, issue, logout) are peer domains, so the server builds them and mounts each directly in the handler loop instead of reaching through authFlow.MFA()/Consent()/Logout() accessors. authflow now receives the ones its spine drives via Config and no longer constructs or exposes them; the dead issuer/skipApproval fields and the accessors are gone. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Following zitadel's login layout (one handler file per mechanism: password_handler, external_provider_handler, ...), split the 584-line login.go into focused files: login.go (entry + connector dispatch), password.go (the password-credential mechanism), callback.go (the redirect-return mechanism for OAuth2/SAML), and finalize.go (the post-authentication step shared by all mechanisms). No behaviour change. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Readable multi-line struct literals instead of one crammed line each. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
…ct pipeline The post-login decision was a spine (advance/nextAuthStep) on the login handler that referenced mfa, consent and issue, and consent in turn referenced mfa and issue. That mutual referencing is what forced the flow components to be shared variables. Dissolve it into a zitadel-style step chain where each step hands off to the next by an HMAC-protected redirect and holds no reference to any other step: login -> /mfa/start -> /approval -> /issue -> client - render gains BuildMFAURL/BuildIssueURL and RedirectAuthError; each hop's URL carries an HMAC bound to (auth request id, step), so a browser can only reach a step it was routed to. issue and the MFA gate verify it (TestFlowStepsRequireHMAC). - mfa grows a /mfa/start gate (always mounted) that decides factor-or-skip; issue grows the /issue endpoint; consent redirects to /issue and drops its mfa and issue references. prompt=none is enforced per step. - With the references gone, authflow no longer holds mfa/consent/issue, and the flow handlers are constructed inline in the server's mount list, sharing only the render and session infrastructure. Server-level tests follow the chain with a followFlow helper; the end-to-end outcomes are unchanged. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
…issue Replace the linear step chain (login -> /mfa/start -> /approval -> /issue) with a hub: a /continue dispatcher on the login handler that every step returns to. On each entry it inspects the auth request and decides the next step in one place — MFA factor, consent, or issuing the response inline — mirroring hydra's authorize endpoint and zitadel's finalize step. - Remove the /issue endpoint (issuance is inline at the dispatcher, as in hydra/zitadel) and the /mfa/start gate (the dispatcher owns the chain decision). prompt=none is enforced once, in the dispatcher. - mfa and consent no longer reference each other or issue: a factor and a consent POST just redirect back to /continue (HMAC-protected). The dispatcher is the only referrer of the steps — a legitimate hub, not a tangle. - Fewer redirects on the common path (login -> /continue -> client). - Drop the now-dead approvalURL plumbing and unused returnURL param in mfa. TestFlowStepsRequireHMAC covers the dispatcher's step-skip protection. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
A dedicated /continue endpoint was redundant: the convergence point of the flow is the authorization endpoint itself. hydra re-enters /oauth2/auth with a login/consent verifier, and zitadel finalizes on the authorize request — neither has a separate continue endpoint. Make /auth the dispatcher: a request carrying an auth-request id (req) is a step reconverging on the flow and is dispatched; otherwise /auth starts a new request as before. Steps redirect back to /auth?req=&hmac=. Renames continue.go to dispatch.go. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
mfa, consent and logout each expose Mount, so they are handlers; name their type Handler, not Manager, matching the other domain handlers. session.Manager and render.UI keep their names — they have no Mount. Renames mfa/manager.go to mfa/handler.go. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Once /issue was dropped, the issue package had a single consumer — the flow dispatcher — and was no longer a mounted handler. It is the issuance half of the authorize endpoint, so move it back into authflow as response.go (Handler methods writeResponse/issueCode/issueAccessToken/issueIDToken); the dispatcher calls it inline. authflow.Config takes Issuer again instead of the issue.Writer. render stays its own package: it has six consumers (mfa, consent, logout, authflow, server) and no single home. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
…line The dispatcher coupled the login handler to mfa and consent (it called ChainForClient/Satisfied to decide the next step), which forced them to be shared variables. Distribute the decision to the steps themselves so no handler references another: /auth (login) -> /mfa/start -> /approval -> /auth?hmac=issue -> client - The MFA gate decides its own chain and hands off to consent; consent decides its own Satisfied and hands off to issuance; each redirects by HMAC-protected URL (built by render, the shared URL builder). prompt=none is enforced per step. - Issuance is the authorize endpoint's own job: /auth with an issue HMAC calls writeResponse. No separate issue/dispatcher endpoint. - With no cross-references left, every handler — including tokenEndpoint, discovery, userinfo, introspection, device, home (via newHomeHandler) — is constructed inline in the mount list. Only ui and sessions remain as shared infrastructure vars. Each handler is now self-sufficient and fits in one package. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
home.Handler needed a post-construction step to copy CookieName/ CookieEncryptionKey out of the session config only when sessions were enabled, which kept it out of the inline mount list. Give it the *session.Config directly (nil when sessions are off) and let it derive what it needs, so it too is constructed inline. Drops newHomeHandler. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
…lers render is shared by mfa, consent, logout and authflow (RenderError, AbsPath, AbsURL, RedirectAuthError), so it stays a separate package — moving it into authflow would make the step handlers depend on the login package. But its three Build*URL helpers were each used by exactly one handler and encoded flow knowledge (which path, which HMAC label), not generic rendering: BuildMFAURL -> authflow (buildMFAURL: login hands off to the MFA gate) BuildApprovalURL-> mfa (buildApprovalURL: MFA hands off to consent) BuildIssueURL -> consent (buildIssueURL: consent hands off to issuance) Each handler now builds the URL of its own next hop; render keeps only generic browser helpers. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Model the flow on hydra's authorize strategy (verified against ory/hydra consent/strategy_default.go): /auth is the dispatcher, every step returns to it with an HMAC verifier, and it alone decides the next step. - /auth?req routes by verifier: after login/MFA the browser carries the "continue" verifier; after consent the "approved" verifier. The dispatcher queries mfa.ChainForClient and consent.Satisfied itself and redirects to a factor or the approval screen only when the step is actually needed — otherwise it skips straight through, no bounce. prompt=none is enforced here. - The MFA gate (/mfa/start) is gone: /auth checks MFA directly and sends the user to a factor; the factor returns to /auth. The approval screen is now a dumb view that POSTs back to /auth with the "approved" verifier. - State stays in the AuthRequest (MFAValidated) and UserIdentity.Consents plus the single-use "approved" HMAC — no new storage field. ForceApprovalPrompt no longer loops: "approved" resolves consent for the request. - The dispatcher holds mfa and consent (injected via Config), like hydra's strategy holds its managers; the steps hold no reference back. Device is unchanged: its browser leg is a normal AuthRequest through /auth. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
render.UI was a shared embedded type for what the rest of the server does locally: device and home each have their own renderError, and URLs are built with path.Join on the issuer path. It was also carrying a now-dead RedirectAuthError (the dispatcher owns prompt=none). Give each flow handler its own renderError plus absPath/absURL (matching device/home) and delete the render package. The shared 'ui' var in the server is gone; handlers take IssuerURL/Templates/Logger directly. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
authflow and grants were the last handlers built through a Config plus a
NewHandler/NewEndpoint constructor while the rest of the mount list is plain
&Type{...} literals. Export their fields and drop the Config indirection so all
handlers are wired the same way, with the field names visible at the call site.
grants builds its grant map in Mount now instead of in the constructor. Rename
grants.Endpoint to grants.Handler for consistency with the other domains and
move the discovery handler to the top of the mount list.
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Pull the X-Remote-* stripping on /callback out of the inline closure into a
named helper and clarify why only the bare callback strips: it serves OAuth/OIDC
redirects where those headers never belong, while /callback/{connector} is the
authproxy connector's own route and must pass them through.
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
The /auth dispatcher held *mfa.Handler and *consent.Handler to query its step decisions, the last handler-to-handler reference in the flow. Remove it so mfa and consent mount inline like the rest. The dispatcher now decides from persisted state and config alone: it reads the client's requested MFA chain (client.MFAChain, else the server default) to gate MFA, and calls consent.Satisfied as a package function. MFA owns the rest — a new /mfa entry resolves the effective, provider-filtered chain and picks the factor. Because the dispatcher gates on the unfiltered chain, the entry records MFA as satisfied when nothing applies to the connector, so control is not routed back. sessions stays shared infrastructure; only the peer-handler refs are gone. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors Dex’s interactive, browser-facing OAuth/OIDC authorization flow by extracting it from the server “god object” into a dedicated server/authflow package with clearer boundaries, while keeping the public server.Config/cmd/dex surface largely stable.
Changes:
- Introduces
server/authflow.Handlerto own/auth, connector/password login, callback handling, dispatching, and response issuance. - Extracts related flow steps into focused packages (
server/session,server/mfa,server/consent,server/logout) and introducesserver/reqctxfor shared request-scoped context keys. - Moves/splits tests to align with the new package boundaries and adds/adjusts integration coverage.
Reviewed changes
Copilot reviewed 57 out of 69 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| server/session/doc.go | New package doc for sessions |
| server/server_test.go | Test server config updates (sessions) |
| server/server_oauth2_test.go | New OAuth/signer/token tests |
| server/server_login_test.go | Flow tests updated to use ServeHTTP |
| server/server_introspection_test.go | New introspection endpoint tests |
| server/server_home_test.go | Session config type update in tests |
| server/server_grant_tokenexchange_test.go | New token-exchange grant tests |
| server/server_grant_refresh_test.go | Refresh grant tests updated for session pkg |
| server/server_grant_password_test.go | New password grant tests |
| server/server_grant_devicecode_test.go | New device-code grant tests |
| server/server_grant_clientcredentials_test.go | New client-credentials grant tests |
| server/server_grant_authcode_test.go | New auth-code redemption tests |
| server/server_flow_test.go | New dispatcher/HMAC flow tests |
| server/server_errors_test.go | Removes renderError safe-message test (moved) |
| server/server_discovery_test.go | New discovery document tests |
| server/server_device_callback_test.go | New device callback tests (safe errors) |
| server/server_device_authorize_test.go | New device authorize/code tests |
| server/server_authorize_test.go | Authorization tests updated to new routing/sessions |
| server/server_approval_test.go | Approval tests updated for new HMAC verifiers/flow |
| server/server_api_cache_test.go | New connector cache invalidation test |
| server/reqctx/reqctx.go | New request context key helpers |
| server/reqctx/doc.go | New reqctx package docs |
| server/mfa/webauthn_test.go | WebAuthn tests moved into mfa pkg |
| server/mfa/totp.go | TOTP MFA implementation moved into mfa pkg |
| server/mfa/totp_test.go | TOTP tests moved into mfa pkg |
| server/mfa/handler.go | New MFA handler (routing + chain resolution) |
| server/mfa/doc.go | New mfa package docs |
| server/mfa_webauthn_test.go | Removed (tests moved to server/mfa) |
| server/mfa_test.go | Removed (tests moved to server/mfa, authflow) |
| server/logout/doc.go | New logout package docs |
| server/home/home.go | Home handler updated to use session.Config |
| server/helpers_test.go | Adds flow-following helper for tests |
| server/grants/doc.go | Updates terminology (Endpoint → Handler) |
| server/consent/doc.go | New consent package docs |
| server/consent/consent.go | New consent handler + skip logic |
| server/consent/consent_test.go | Consent unit tests moved into consent pkg |
| server/authflow/urls.go | Shared flow URL builders (HMAC-protected) |
| server/authflow/sessionlogin.go | Session/SSO login path in authflow |
| server/authflow/response.go | Response-type handlers + issuance logic |
| server/authflow/render.go | Shared error rendering + URL helpers |
| server/authflow/password.go | Password login handler moved into authflow |
| server/authflow/mfa_test.go | Authflow-level MFA integration/unit tests |
| server/authflow/login.go | Connector login entry moved into authflow |
| server/authflow/login_test.go | Authflow login tests (blocked account) |
| server/authflow/handler.go | Authflow handler definition + route mounting |
| server/authflow/handler_test.go | Authflow test harness / assembly |
| server/authflow/finalize.go | finalizeLogin moved into authflow |
| server/authflow/errors.go | Safe user-facing error messages moved into authflow |
| server/authflow/errors_test.go | Safe-message tests moved into authflow |
| server/authflow/doc.go | New authflow package docs |
| server/authflow/dispatch.go | /auth dispatcher and next-step routing |
| server/authflow/callback.go | Connector callback handler moved into authflow |
| server/authflow/authorize.go | /auth entrypoint logic moved into authflow |
| server/approval.go | Removed (consent + issuance moved/refactored) |
| cmd/dex/serve.go | Wires authflow/session/mfa in CLI assembly |
| cmd/dex/logger.go | Switches to reqctx keys for structured logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tests The package split left several doc comments on the wrong declarations: the authflow package doc still described advance/nextstep.go and a web sub-package; mfaRequestContext, TestNewWebAuthnProvider and trySessionLoginWithSession carried comments for other symbols; and totp.go/totp_test.go ended with dangling comments for a moved function and test. Rewrite or drop each to match the code. The two ID-token issuer tests built tokens.NewIssuer with the nil s.storage field; use the in-memory store already created for the signer so the issuer has real storage. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 69 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
server/authflow/authorize.go:151
- For prompt=none with no existing session, the redirect uses oauth2.LoginRequired but the error_description currently mentions id_token_hint mismatch, which is misleading (no hint is involved here). Use a description that reflects the actual condition (user not authenticated / cannot authenticate silently).
server/authflow/callback.go:57 - A connector mismatch here is caused by a crafted/invalid callback URL, not a server failure. Returning 500 makes it look like an internal outage; return a 4xx status (e.g., 400) instead.
} else if connID != "" && connID != authReq.ConnectorID {
h.Logger.ErrorContext(r.Context(), "connector mismatch: callback triggered for different connector than authentication start", "authentication_start_connector_id", authReq.ConnectorID, "connector_id", connID)
h.renderError(r, w, http.StatusInternalServerError, "Requested resource does not exist.")
return
authflow/mfa_test.go tested functions that live in the mfa package (CompleteStep, the /mfa entry, the WebAuthn handlers), so it had no matching source file in authflow. Move those tests to server/mfa, split across handler_test.go and webauthn_test.go, with a small router harness that mounts only the MFA handler. Rename authflow/login_test.go to finalize_test.go since its one test covers finalizeLogin (finalize.go). Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
A connector path segment that fails to URL-unescape, and a connector that does not match the one the auth request started with, are both malformed client requests, not server faults. Return 400 instead of 500 in the password-login and callback handlers so a crafted request no longer looks like an internal outage, and fix the unescape log line that mislabeled the failure as "failed to get connector". Also drop a dangling doc comment left at the end of sessionlogin.go after updateSessionTokenIssuedAt moved to the session package. Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 68 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
server/server_login_test.go:375
- The else-branch assertion uses require.Error(t, storage.ErrNotFound, err), which treats storage.ErrNotFound as the error under test and never checks the actual err returned by GetOfflineSessions. This can make the test pass even when GetOfflineSessions fails for a different reason. Use require.ErrorIs(t, err, storage.ErrNotFound) instead.
This issue also appears on line 509 of the same file.
server/server_login_test.go:510
- The else-branch assertion uses require.Error(t, storage.ErrNotFound, err), which treats storage.ErrNotFound as the error under test and never checks the actual err returned by GetOfflineSessions. This can make the test pass even when GetOfflineSessions fails for a different reason. Use require.ErrorIs(t, err, storage.ErrNotFound) instead.
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.
Overview
Extracts dex's interactive, browser-facing authorization flow out of the
servergod-object into a dedicatedserver/authflowpackage, and splits the flow's shared steps into their own sibling packages. The flow is reorganised around a single/authdispatcher modelled on ory/hydra.What this PR does / why we need it
Continues the decomposition of the
serverpackage (after the/tokengrants split in #4910 and the gRPC API extraction in #4911). The interactive flow —/auth, connector and password login, session SSO, MFA (TOTP and WebAuthn), consent/approval and RP-initiated logout — was the largest remaining chunk living directly onServer.New
server/authflowpackage. It owns/auth,/auth/{connector}[/login]and/callback[/{connector}], and mounts its own routes viarouter.Mux, the same pattern thegrantsendpoint uses. It holds no reference toServer: its dependencies are plain exported fields on theHandler, wired as a struct literal inserver.gonext to every other mounted handler./authis the flow dispatcher (hydra-style). After login and after every step, the browser re-enters/authcarrying an HMAC verifier; the dispatcher reads the persistedAuthRequestand decides the next step: an MFA factor, the consent screen, or issuing the response. Steps never route to one another, each redirects back to/auth. This replaces the scattered if-cascades where each handler re-derived the next hop.Flow steps as self-sufficient sibling packages.
server/session(cookie, SSO, auth-session CRUD),server/mfa(the authenticator chain, TOTP/WebAuthn and the/mfaentry),server/consent(the approval screen) andserver/logout(RP-initiated logout) each own and mount their own routes. No handler holds a Go reference to another; the dispatcher decides MFA and consent from persisted state and config, not by calling into those handlers.Request-scoped context keys moved to
server/reqctx.RequestKeyRemoteIPandRequestKeyRequestIDare read by both the server middleware and the CLI logger, so they now live in a small package importable fromcmd(unlikeserver/internal).The runtime YAML config is unchanged. The Go-level aliases the flow used to expose on
server(server.PKCEConfig,server.SessionConfig,server.MFAProvider,server.NewTOTPProvider/NewWebAuthnProvider) are dropped;cmdnow imports these from their real packages (authflow.PKCEConfig,session.Config,mfa.Provider).No behaviour change intended, the split is structural. Two small fixes came out of it:
finalizeLoginnow keeps the in-memoryAuthRequestclaims in sync with what it persisted (the consent check reads them), and the password-login and callback handlers return 400 instead of 500 for client-caused connector errors (a path segment that fails to unescape, or a connector that does not match the one the request started with).Tests were moved to match the new boundaries: each package's unit tests live with it (named after the source file they cover), while the end-to-end HTTP tests stay in
server(prefixedserver_) and drive the flow through the mounted routes.Special notes for your reviewer
authflow/dispatch.go(the central decision and the HMAC verifiers),authflow/handler.go(the route andMountboundary), and the/mfaentry inserver/mfa/handler.go— it records MFA as satisfied when no factor applies to the connector, so the dispatcher does not loop back to it.&Type{...}literal inserver.gowith no per-handlerConfigor constructor.grants.Endpointwas renamed togrants.Handlerfor consistency.