mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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 <details> <summary>Investigation notes</summary> ### 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. </details> --- *This PR was created on behalf of @ericpaulsen by the Coder Agents AI assistant.*
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user