diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go index 45ea0d175e..c989550021 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 f7d9c3c34a..06c1b20d57 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -364,9 +364,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) + } } if oauth2Source.GroupTeamMap != "" || oauth2Source.GroupTeamMapRemoval { diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go index 8fc58f9ae1..3123e54bbb 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 3bb3d56e7b..7978c3bd04 100644 --- a/tests/integration/auth_oauth2_test.go +++ b/tests/integration/auth_oauth2_test.go @@ -13,6 +13,7 @@ import ( "time" auth_model "gitea.dev/models/auth" + "gitea.dev/models/db" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" "gitea.dev/modules/json" @@ -23,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "xorm.io/builder" ) // TestMigrateAzureADV2ToOIDC simulates a login source migration from the Azure AD V2 OAuth2 provider to the OpenID Connect provider, @@ -191,6 +193,58 @@ func TestOIDCIgnoresStaleExternalLoginLinks(t *testing.T) { }) } +// 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