From b6033aee03025b105c31eca47f64126342e97082 Mon Sep 17 00:00:00 2001 From: Eric Paulsen Date: Thu, 11 Jun 2026 14:39:21 +0100 Subject: [PATCH] fix(coderd): reject suspended user during OIDC and GitHub OAuth login (#24996) Previously, a suspended user authenticating via OIDC or GitHub OAuth was silently issued a session cookie and redirected to the dashboard. The very next API call (`/api/v2/users/me`) failed with `401` from the suspended-user check in `httpmw.ExtractAPIKey`, the SPA treated the 401 as "signed out", and bounced the user back to `/login` with no indication of why. The password login path does not have this bug because `loginRequest` rejects suspended users *before* creating an API key. The shared `oauthLogin` handler in `coderd/userauth.go` only special-cased the `dormant` status. Add a parallel check for `suspended` that returns an `idpsync.HTTPError` with `RenderStaticPage: true`, so the OIDC and GitHub callback handlers render an explanatory error page. The GitHub device flow already clears `RenderStaticPage` for `idpsync.HTTPError` responses, so it returns the same fields as JSON. Returning from inside `db.InTx` rolls the transaction back, so no link insert/update or IDP sync side-effects are persisted for a rejected suspended user. Closing https://github.com/coder/coder/issues/24614
Investigation notes ### Trace through the bug on `main` 1. `userOIDC` callback in `coderd/userauth.go` enters `oauthLogin`. 2. Inside the `db.InTx` closure, only `user.Status == database.UserStatusDormant` is special-cased (auto-activates). A `suspended` user falls through and the transaction commits as-is. 3. `oauthLogin` then calls `api.createAPIKey(...)` and the session cookie is set. 4. The handler issues `http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)` to the post-login URL. 5. The SPA loads and calls `GET /api/v2/users/me`. `httpmw.ExtractAPIKey` returns `401 "User is not active (status = \"suspended\"). Contact an admin to reactivate your account."` (`coderd/httpmw/apikey.go:685`). 6. `site/src/contexts/auth/RequireAuth.tsx` treats any `401` from `/users/me` as "signed out" and redirects to `/login` without surfacing the message body. Verified by reverting the fix and re-running the new test: the OIDC callback returns `307` (the bug) instead of the expected `403`. ### Why this placement The new check is placed alongside the existing `Dormant` branch: - It runs after the new-user creation block, so first-login signup is unaffected (new users are always created `active`). - Returning an `*idpsync.HTTPError` from inside `db.InTx` rolls the transaction back, so no `user_links` insert/update or IDP sync is persisted. - `idpsync.HTTPError` with `RenderStaticPage: true` is already the convention used by the OIDC and GitHub callbacks for "Email not verified" and "Signups disabled" via `idpsync.IsHTTPError(err) -> httpErr.Write(rw, r)`. - `oauthLogin` is shared between OIDC and GitHub OAuth, so a single change fixes both flows. The GitHub device-flow branch in `userOAuth2Github` already clears `RenderStaticPage` for `idpsync.HTTPError` and returns JSON, so device clients get the same `403` with `Msg`/`Detail` fields. ### Test `TestUserOIDC/OIDCSuspended` mirrors the existing `OIDCDormancy` test: - Pre-seed a `database.User` with `LoginType: LoginTypeOIDC` and `Status: UserStatusSuspended`. - Drive the OIDC callback via `oidctest.FakeIDP.AttemptLogin`. - Assert HTTP `403`, response body contains `"suspended"`, and the user's DB status is unchanged. ### Out of scope The issue mentions allowing admins to customize the suspension message as an extra step. Not included; that would be a separate feature.
--- *This PR was created on behalf of @ericpaulsen by the Coder Agents AI assistant.* --- coderd/userauth.go | 18 ++++++++++++++++ coderd/userauth_test.go | 47 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index c8f329f5cf..bdcaad7397 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1853,6 +1853,24 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } + // Reject the login if the linked user is suspended. Suspending only + // applies to existing users, so this check is intentionally placed + // after the new-user creation branch above. Returning an HTTPError + // rolls back the transaction so no link/sync side effects are + // persisted, and the caller renders a static error page describing + // what happened. + if user.Status == database.UserStatusSuspended { + return &idpsync.HTTPError{ + Code: http.StatusForbidden, + Msg: "Account suspended", + Detail: fmt.Sprintf( + "Your account %q has been suspended. Contact your Coder administrator to reactivate your account.", + user.Username, + ), + RenderStaticPage: true, + } + } + // Activate dormant user on sign-in if user.Status == database.UserStatusDormant { // This is necessary because transactions can be retried, and we diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index e73a2e9354..9c656b9c7b 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -2001,6 +2001,53 @@ func TestUserOIDC(t *testing.T) { "linked_id must not be modified when the login is blocked") }) + t.Run("OIDCSuspended", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + fake := oidctest.NewFakeIDP(t, + oidctest.WithRefresh(func(_ string) error { + return xerrors.New("refreshing token should never occur") + }), + oidctest.WithServing(), + ) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }) + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{ + OIDCConfig: cfg, + Logger: &logger, + }) + + // Pre-existing OIDC user that has been suspended by an admin. + user := dbgen.User(t, db, database.User{ + LoginType: database.LoginTypeOIDC, + Status: database.UserStatusSuspended, + }) + + _, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{ + "email": user.Email, + "sub": uuid.NewString(), + }) + // The OIDC handler should reject the login with an explanatory + // 403 instead of silently issuing a session and letting the SPA + // bounce the user back to /login with no message. + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Contains(t, string(body), "suspended", "error page should explain why login was rejected") + + // The user's status must remain suspended; nothing in the OAuth + // transaction should have been committed. + //nolint:gocritic // System read for verification. + dbUser, err := db.GetUserByID(dbauthz.AsSystemRestricted(ctx), user.ID) + require.NoError(t, err) + require.Equal(t, database.UserStatusSuspended, dbUser.Status) + }) + t.Run("OIDCConvert", func(t *testing.T) { t.Parallel()