From 2bde4fa5d268415fdce9442826d63443ad81d47f Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 21 Jun 2026 14:23:24 +0200 Subject: [PATCH] fix(auth): do not auto-reactivate disabled users on OAuth2 callback (#38009) (#38183) Backport #38009 The OAuth2 sign-in callback unconditionally set IsActive=true on the local user row whenever the IdP authenticated them, silently undoing an administrator's "Disable Account" action and granting the user a fresh session in the same response. Treat the local IsActive flag as an authoritative admin override: inactive users get a session and are routed through the existing activate / prohibit-login pages by verifyAuthWithOptions, matching the local-credentials sign-in path. Adds an integration regression test that disables a linked local user and asserts the row stays IsActive=false after a full OIDC callback. Co-authored-by: wxiaoguang --- models/unittest/unit_tests.go | 2 +- routers/web/auth/oauth.go | 16 ++- services/auth/source/oauth2/source_sync.go | 4 +- tests/integration/auth_oauth2_test.go | 113 +++++++++++++++++++++ 4 files changed, 130 insertions(+), 5 deletions(-) diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go index c49b26fea4..a5852deea0 100644 --- a/models/unittest/unit_tests.go +++ b/models/unittest/unit_tests.go @@ -47,7 +47,7 @@ func OrderBy(orderBy string) any { } func whereOrderConditions(e db.Engine, conditions []any) db.Engine { - orderBy := "id" // query must have the "ORDER BY", otherwise the result is not deterministic + orderBy := "id" // query must have the "ORDER BY", otherwise the result is not deterministic. FIXME: some tables do not have "id" column for _, condition := range conditions { switch cond := condition.(type) { case *testCond: diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index b8d1d6ac94..a03afd5470 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -368,9 +368,21 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m opts := &user_service.UpdateOptions{} - // Reactivate user if they are deactivated + // HINT: OAUTH-AUTO-SYNC-USER-ACTIVATION: see services/auth/source/oauth2/source_sync.go + // Reactivate user only if they were disabled by the OAuth2 auto sync cron (invalid_grant), + // which clears AccessToken/RefreshToken/ExpiresAt on the ExternalLoginUser row + // An admin-disabled user has no such signature, so we leave IsActive alone + // and let verifyAuthWithOptions route them through the prohibit-login / activate page. if !u.IsActive { - opts.IsActive = optional.Some(true) + extLogin, hasExt, err := user_model.GetExternalLogin(ctx, authSource.ID, gothUser.UserID) + if err != nil { + ctx.ServerError("GetExternalLogin", err) + return + } + isDisabledByAutoSync := hasExt && extLogin.RefreshToken == "" + if isDisabledByAutoSync { + opts.IsActive = optional.Some(true) + } } // Update GroupClaims diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go index 445e281a06..4ad15b49dd 100644 --- a/services/auth/source/oauth2/source_sync.go +++ b/services/auth/source/oauth2/source_sync.go @@ -88,8 +88,8 @@ func (source *Source) refresh(ctx context.Context, provider goth.Provider, u *us } } - // Delete stored tokens, since they are invalid. This - // also provents us from checking this in subsequent runs. + // HINT: OAUTH-AUTO-SYNC-USER-ACTIVATION + // Delete stored tokens, since they are invalid. This also prevents us from checking this in subsequent runs. u.AccessToken = "" u.RefreshToken = "" u.ExpiresAt = time.Time{} diff --git a/tests/integration/auth_oauth2_test.go b/tests/integration/auth_oauth2_test.go index 3294e6d103..00a66332a4 100644 --- a/tests/integration/auth_oauth2_test.go +++ b/tests/integration/auth_oauth2_test.go @@ -13,6 +13,7 @@ import ( "time" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/json" @@ -23,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "xorm.io/builder" ) func TestOIDCIgnoresStaleExternalLoginLinks(t *testing.T) { @@ -135,6 +137,117 @@ func newFakeOIDCServerWithProfile(t *testing.T, sub, oid, email, name string) *h return srv } +// TestOAuth2CallbackReactivationGating exercises the gate in handleOAuth2SignIn: +// an inactive user can only be reactivated when who was disabled by auto-sync +func TestOAuth2CallbackReactivationGating(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)() + defer test.MockVariableValue(&setting.OAuth2Client.Username, setting.OAuth2UsernameUserid)() + + srv := newFakeOIDCServer(t, FakeOIDCConfig{Sub: "test-sub", Email: "test@example.com", Name: "Test User"}) + addOAuth2Source(t, "test-oauth-source", oauth2.Source{ + Provider: "openidConnect", + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + OpenIDConnectAutoDiscoveryURL: srv.URL + "/.well-known/openid-configuration", + }) + authSource, err := auth_model.GetActiveOAuth2SourceByAuthName(t.Context(), "test-oauth-source") + require.NoError(t, err) + + u := &user_model.User{Name: "test-user", Email: "test@example.com"} + require.NoError(t, user_model.CreateUser(t.Context(), u, &user_model.Meta{})) + + extLink := &user_model.ExternalLoginUser{ + UserID: u.ID, + LoginSourceID: authSource.ID, + Provider: authSource.Name, + ExternalID: "test-sub", + } + require.NoError(t, user_model.LinkExternalToUser(t.Context(), u, extLink)) + + prepareUserExternalLink := func(t *testing.T, refreshToken string) { + err := user_model.UpdateUserCols(t.Context(), &user_model.User{ID: u.ID, IsActive: false}, "is_active") + require.NoError(t, err) + _, err = db.GetEngine(t.Context()).Where(builder.Eq{"user_id": u.ID}).Cols("refresh_token"). + Update(&user_model.ExternalLoginUser{RefreshToken: refreshToken}) + require.NoError(t, err) + require.False(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: u.ID}).IsActive) + } + + t.Run("admin-disabled user is not reactivated", func(t *testing.T) { + prepareUserExternalLink(t, "non-empty-refresh-token") + doOIDCSignIn(t, authSource.Name) + after := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: u.ID}) + assert.False(t, after.IsActive, "OAuth callback must not re-enable an administrator-disabled account") + }) + + t.Run("auto-sync-disabled user is reactivated", func(t *testing.T) { + prepareUserExternalLink(t, "" /* empty refresh token */) + doOIDCSignIn(t, authSource.Name) + after := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: u.ID}) + assert.True(t, after.IsActive, "OAuth callback must reactivate a sync-disabled account on successful login") + }) +} + +// FakeOIDCConfig holds configuration for the fake OIDC server used in tests. +type FakeOIDCConfig struct { + Sub string + OID string + Email string + Name string + Groups []string +} + +// newFakeOIDCServer starts a httptest.Server that implements the minimum OIDC endpoints needed to complete a sign-in flow +func newFakeOIDCServer(t *testing.T, cfg FakeOIDCConfig) *httptest.Server { + t.Helper() + + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/.well-known/openid-configuration": // discovery document + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": srv.URL, + "authorization_endpoint": srv.URL + "/authorize", + "token_endpoint": srv.URL + "/token", + "userinfo_endpoint": srv.URL + "/userinfo", + }) + case "/token": // returns an ID token with both "sub" and "oid" claims so tests can verify which one ends up as ExternalID + claims := map[string]any{ + "iss": srv.URL, + "aud": "test-client-id", + "exp": time.Now().Add(time.Hour).Unix(), + "sub": cfg.Sub, + "oid": cfg.OID, + } + payload, _ := json.Marshal(claims) + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"none"}`)) + + // build a JWT-shaped string whose payload encodes claims. + // goth's decodeJWT only base64-decodes the payload without verifying the signature, so no real signing infrastructure is needed. + idToken := header + "." + base64.RawURLEncoding.EncodeToString(payload) + ".fakesig" + + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "fake-access-token", + "token_type": "Bearer", + "id_token": idToken, + }) + case "/userinfo": + // sub MUST match the id_token sub; goth rejects mismatches. + _ = json.NewEncoder(w).Encode(map[string]any{ + "sub": cfg.Sub, + "email": cfg.Email, + "name": cfg.Name, + }) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + return srv +} + // doOIDCSignIn runs a mock OIDC sign-in flow for the given auth source. func doOIDCSignIn(t *testing.T, sourceName string) { t.Helper()