From 7f8f9b878fa64ce09096a745fe2e71ea64516b0c Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 9 Dec 2024 06:58:58 +0100 Subject: [PATCH] fix: Revert "allow synchronizing user status from OAuth2 login providers (#31572)" This commit has a fundamental flaw, in order to syncronize if external users are still active the commit checks if the refresh token is accepted by the OAuth provider, if that is not the case it sees that as the user is disabled and sets the is active field to `false` to signal that. Because it might be possible (this commit makes this a highly likelyhood) that the OAuth provider still recognizes this user the commit introduces code to allow users to re-active themselves via the oauth flow if they were disabled because of this. However this code makes no distinction in why the user was disabled and always re-actives the user. Thus the reactivation via the OAuth flow allows users to bypass the manually activation setting (`[service].REGISTER_MANUAL_CONFIRM`) or if the admin for other reasons disabled the user. This reverts commit 21fdd28f084e7f1aef309c9ebd7599ffa6986453. --- models/auth/source.go | 2 +- models/user/external_login_user.go | 41 +------ routers/web/auth/auth.go | 6 +- routers/web/auth/oauth.go | 64 +++++----- services/auth/source/oauth2/main_test.go | 14 --- services/auth/source/oauth2/providers_test.go | 62 ---------- services/auth/source/oauth2/source.go | 2 +- services/auth/source/oauth2/source_sync.go | 114 ------------------ .../auth/source/oauth2/source_sync_test.go | 101 ---------------- services/externalaccount/user.go | 6 +- templates/admin/auth/edit.tmpl | 2 +- templates/admin/auth/new.tmpl | 2 +- tests/integration/auth_ldap_test.go | 3 +- 13 files changed, 48 insertions(+), 371 deletions(-) delete mode 100644 services/auth/source/oauth2/main_test.go delete mode 100644 services/auth/source/oauth2/providers_test.go delete mode 100644 services/auth/source/oauth2/source_sync.go delete mode 100644 services/auth/source/oauth2/source_sync_test.go diff --git a/models/auth/source.go b/models/auth/source.go index 8f7c2a89db..d03d4975dc 100644 --- a/models/auth/source.go +++ b/models/auth/source.go @@ -216,7 +216,7 @@ func CreateSource(ctx context.Context, source *Source) error { return ErrSourceAlreadyExist{source.Name} } // Synchronization is only available with LDAP for now - if !source.IsLDAP() && !source.IsOAuth2() { + if !source.IsLDAP() { source.IsSyncEnabled = false } diff --git a/models/user/external_login_user.go b/models/user/external_login_user.go index 0e764efb9f..965b7a5ed1 100644 --- a/models/user/external_login_user.go +++ b/models/user/external_login_user.go @@ -160,34 +160,12 @@ func UpdateExternalUserByExternalID(ctx context.Context, external *ExternalLogin return err } -// EnsureLinkExternalToUser link the external user to the user -func EnsureLinkExternalToUser(ctx context.Context, external *ExternalLoginUser) error { - has, err := db.Exist[ExternalLoginUser](ctx, builder.Eq{ - "external_id": external.ExternalID, - "login_source_id": external.LoginSourceID, - }) - if err != nil { - return err - } - - if has { - _, err = db.GetEngine(ctx).Where("external_id=? AND login_source_id=?", external.ExternalID, external.LoginSourceID).AllCols().Update(external) - return err - } - - _, err = db.GetEngine(ctx).Insert(external) - return err -} - // FindExternalUserOptions represents an options to find external users type FindExternalUserOptions struct { db.ListOptions - Provider string - UserID int64 - LoginSourceID int64 - HasRefreshToken bool - Expired bool - OrderBy string + Provider string + UserID int64 + OrderBy string } func (opts FindExternalUserOptions) ToConds() builder.Cond { @@ -198,22 +176,9 @@ func (opts FindExternalUserOptions) ToConds() builder.Cond { if opts.UserID > 0 { cond = cond.And(builder.Eq{"user_id": opts.UserID}) } - if opts.Expired { - cond = cond.And(builder.Lt{"expires_at": time.Now()}) - } - if opts.HasRefreshToken { - cond = cond.And(builder.Neq{"refresh_token": ""}) - } - if opts.LoginSourceID != 0 { - cond = cond.And(builder.Eq{"login_source_id": opts.LoginSourceID}) - } return cond } func (opts FindExternalUserOptions) ToOrders() string { return opts.OrderBy } - -func IterateExternalLogin(ctx context.Context, opts FindExternalUserOptions, f func(ctx context.Context, u *ExternalLoginUser) error) error { - return db.Iterate(ctx, opts.ToConds(), f) -} diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 71d7b8ca11..ccab47a9a2 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -599,8 +599,10 @@ func handleUserCreated(ctx *context.Context, u *user_model.User, gothUser *goth. notify_service.NewUserSignUp(ctx, u) // update external user information if gothUser != nil { - if err := externalaccount.EnsureLinkExternalToUser(ctx, u, *gothUser); err != nil { - log.Error("EnsureLinkExternalToUser failed: %v", err) + if err := externalaccount.UpdateExternalUser(ctx, u, *gothUser); err != nil { + if !errors.Is(err, util.ErrNotExist) { + log.Error("UpdateExternalUser failed: %v", err) + } } } diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index e329729dcd..fbdd47479a 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -1205,39 +1205,9 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model groups := getClaimedGroups(oauth2Source, &gothUser) - opts := &user_service.UpdateOptions{} - - // Reactivate user if they are deactivated - if !u.IsActive { - opts.IsActive = optional.Some(true) - } - - // Update GroupClaims - opts.IsAdmin, opts.IsRestricted = getUserAdminAndRestrictedFromGroupClaims(oauth2Source, &gothUser) - - if oauth2Source.GroupTeamMap != "" || oauth2Source.GroupTeamMapRemoval { - if err := source_service.SyncGroupsToTeams(ctx, u, groups, groupTeamMapping, oauth2Source.GroupTeamMapRemoval); err != nil { - ctx.ServerError("SyncGroupsToTeams", err) - return - } - } - - if err := externalaccount.EnsureLinkExternalToUser(ctx, u, gothUser); err != nil { - ctx.ServerError("EnsureLinkExternalToUser", err) - return - } - // If this user is enrolled in 2FA and this source doesn't override it, // we can't sign the user in just yet. Instead, redirect them to the 2FA authentication page. if !needs2FA { - // Register last login - opts.SetLastLogin = true - - if err := user_service.UpdateUser(ctx, u, opts); err != nil { - ctx.ServerError("UpdateUser", err) - return - } - if err := updateSession(ctx, nil, map[string]any{ "uid": u.ID, }); err != nil { @@ -1248,6 +1218,29 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model // Clear whatever CSRF cookie has right now, force to generate a new one ctx.Csrf.DeleteCookie(ctx) + opts := &user_service.UpdateOptions{ + SetLastLogin: true, + } + opts.IsAdmin, opts.IsRestricted = getUserAdminAndRestrictedFromGroupClaims(oauth2Source, &gothUser) + if err := user_service.UpdateUser(ctx, u, opts); err != nil { + ctx.ServerError("UpdateUser", err) + return + } + + if oauth2Source.GroupTeamMap != "" || oauth2Source.GroupTeamMapRemoval { + if err := source_service.SyncGroupsToTeams(ctx, u, groups, groupTeamMapping, oauth2Source.GroupTeamMapRemoval); err != nil { + ctx.ServerError("SyncGroupsToTeams", err) + return + } + } + + // update external user information + if err := externalaccount.UpdateExternalUser(ctx, u, gothUser); err != nil { + if !errors.Is(err, util.ErrNotExist) { + log.Error("UpdateExternalUser failed: %v", err) + } + } + if err := resetLocale(ctx, u); err != nil { ctx.ServerError("resetLocale", err) return @@ -1263,13 +1256,22 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model return } - if opts.IsActive.Has() || opts.IsAdmin.Has() || opts.IsRestricted.Has() { + opts := &user_service.UpdateOptions{} + opts.IsAdmin, opts.IsRestricted = getUserAdminAndRestrictedFromGroupClaims(oauth2Source, &gothUser) + if opts.IsAdmin.Has() || opts.IsRestricted.Has() { if err := user_service.UpdateUser(ctx, u, opts); err != nil { ctx.ServerError("UpdateUser", err) return } } + if oauth2Source.GroupTeamMap != "" || oauth2Source.GroupTeamMapRemoval { + if err := source_service.SyncGroupsToTeams(ctx, u, groups, groupTeamMapping, oauth2Source.GroupTeamMapRemoval); err != nil { + ctx.ServerError("SyncGroupsToTeams", err) + return + } + } + if err := updateSession(ctx, nil, map[string]any{ // User needs to use 2FA, save data and redirect to 2FA page. "twofaUid": u.ID, diff --git a/services/auth/source/oauth2/main_test.go b/services/auth/source/oauth2/main_test.go deleted file mode 100644 index 57c74fd3e7..0000000000 --- a/services/auth/source/oauth2/main_test.go +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package oauth2 - -import ( - "testing" - - "code.gitea.io/gitea/models/unittest" -) - -func TestMain(m *testing.M) { - unittest.MainTest(m, &unittest.TestOptions{}) -} diff --git a/services/auth/source/oauth2/providers_test.go b/services/auth/source/oauth2/providers_test.go deleted file mode 100644 index 353816c71e..0000000000 --- a/services/auth/source/oauth2/providers_test.go +++ /dev/null @@ -1,62 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package oauth2 - -import ( - "time" - - "github.com/markbates/goth" - "golang.org/x/oauth2" -) - -type fakeProvider struct{} - -func (p *fakeProvider) Name() string { - return "fake" -} - -func (p *fakeProvider) SetName(name string) {} - -func (p *fakeProvider) BeginAuth(state string) (goth.Session, error) { - return nil, nil -} - -func (p *fakeProvider) UnmarshalSession(string) (goth.Session, error) { - return nil, nil -} - -func (p *fakeProvider) FetchUser(goth.Session) (goth.User, error) { - return goth.User{}, nil -} - -func (p *fakeProvider) Debug(bool) { -} - -func (p *fakeProvider) RefreshToken(refreshToken string) (*oauth2.Token, error) { - switch refreshToken { - case "expired": - return nil, &oauth2.RetrieveError{ - ErrorCode: "invalid_grant", - } - default: - return &oauth2.Token{ - AccessToken: "token", - TokenType: "Bearer", - RefreshToken: "refresh", - Expiry: time.Now().Add(time.Hour), - }, nil - } -} - -func (p *fakeProvider) RefreshTokenAvailable() bool { - return true -} - -func init() { - RegisterGothProvider( - NewSimpleProvider("fake", "Fake", []string{"account"}, - func(clientKey, secret, callbackURL string, scopes ...string) goth.Provider { - return &fakeProvider{} - })) -} diff --git a/services/auth/source/oauth2/source.go b/services/auth/source/oauth2/source.go index 3454c9ad55..675005e55a 100644 --- a/services/auth/source/oauth2/source.go +++ b/services/auth/source/oauth2/source.go @@ -36,7 +36,7 @@ func (source *Source) FromDB(bs []byte) error { return json.UnmarshalHandleDoubleEncode(bs, &source) } -// ToDB exports an OAuth2Config to a serialized format. +// ToDB exports an SMTPConfig to a serialized format. func (source *Source) ToDB() ([]byte, error) { return json.Marshal(source) } diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go deleted file mode 100644 index 5e30313c8f..0000000000 --- a/services/auth/source/oauth2/source_sync.go +++ /dev/null @@ -1,114 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package oauth2 - -import ( - "context" - "time" - - "code.gitea.io/gitea/models/auth" - "code.gitea.io/gitea/models/db" - user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" - - "github.com/markbates/goth" - "golang.org/x/oauth2" -) - -// Sync causes this OAuth2 source to synchronize its users with the db. -func (source *Source) Sync(ctx context.Context, updateExisting bool) error { - log.Trace("Doing: SyncExternalUsers[%s] %d", source.authSource.Name, source.authSource.ID) - - if !updateExisting { - log.Info("SyncExternalUsers[%s] not running since updateExisting is false", source.authSource.Name) - return nil - } - - provider, err := createProvider(source.authSource.Name, source) - if err != nil { - return err - } - - if !provider.RefreshTokenAvailable() { - log.Trace("SyncExternalUsers[%s] provider doesn't support refresh tokens, can't synchronize", source.authSource.Name) - return nil - } - - opts := user_model.FindExternalUserOptions{ - HasRefreshToken: true, - Expired: true, - LoginSourceID: source.authSource.ID, - } - - return user_model.IterateExternalLogin(ctx, opts, func(ctx context.Context, u *user_model.ExternalLoginUser) error { - return source.refresh(ctx, provider, u) - }) -} - -func (source *Source) refresh(ctx context.Context, provider goth.Provider, u *user_model.ExternalLoginUser) error { - log.Trace("Syncing login_source_id=%d external_id=%s expiration=%s", u.LoginSourceID, u.ExternalID, u.ExpiresAt) - - shouldDisable := false - - token, err := provider.RefreshToken(u.RefreshToken) - if err != nil { - if err, ok := err.(*oauth2.RetrieveError); ok && err.ErrorCode == "invalid_grant" { - // this signals that the token is not valid and the user should be disabled - shouldDisable = true - } else { - return err - } - } - - user := &user_model.User{ - LoginName: u.ExternalID, - LoginType: auth.OAuth2, - LoginSource: u.LoginSourceID, - } - - hasUser, err := user_model.GetUser(ctx, user) - if err != nil { - return err - } - - // If the grant is no longer valid, disable the user and - // delete local tokens. If the OAuth2 provider still - // recognizes them as a valid user, they will be able to login - // via their provider and reactivate their account. - if shouldDisable { - log.Info("SyncExternalUsers[%s] disabling user %d", source.authSource.Name, user.ID) - - return db.WithTx(ctx, func(ctx context.Context) error { - if hasUser { - user.IsActive = false - err := user_model.UpdateUserCols(ctx, user, "is_active") - if err != nil { - return err - } - } - - // Delete stored tokens, since they are invalid. This - // also provents us from checking this in subsequent runs. - u.AccessToken = "" - u.RefreshToken = "" - u.ExpiresAt = time.Time{} - - return user_model.UpdateExternalUserByExternalID(ctx, u) - }) - } - - // Otherwise, update the tokens - u.AccessToken = token.AccessToken - u.ExpiresAt = token.Expiry - - // Some providers only update access tokens provide a new - // refresh token, so avoid updating it if it's empty - if token.RefreshToken != "" { - u.RefreshToken = token.RefreshToken - } - - err = user_model.UpdateExternalUserByExternalID(ctx, u) - - return err -} diff --git a/services/auth/source/oauth2/source_sync_test.go b/services/auth/source/oauth2/source_sync_test.go deleted file mode 100644 index 746df82055..0000000000 --- a/services/auth/source/oauth2/source_sync_test.go +++ /dev/null @@ -1,101 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package oauth2 - -import ( - "context" - "testing" - - "code.gitea.io/gitea/models/auth" - "code.gitea.io/gitea/models/unittest" - user_model "code.gitea.io/gitea/models/user" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestSource(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) - - source := &Source{ - Provider: "fake", - authSource: &auth.Source{ - ID: 12, - Type: auth.OAuth2, - Name: "fake", - IsActive: true, - IsSyncEnabled: true, - }, - } - - user := &user_model.User{ - LoginName: "external", - LoginType: auth.OAuth2, - LoginSource: source.authSource.ID, - Name: "test", - Email: "external@example.com", - } - - err := user_model.CreateUser(context.Background(), user, &user_model.CreateUserOverwriteOptions{}) - require.NoError(t, err) - - e := &user_model.ExternalLoginUser{ - ExternalID: "external", - UserID: user.ID, - LoginSourceID: user.LoginSource, - RefreshToken: "valid", - } - err = user_model.LinkExternalToUser(context.Background(), user, e) - require.NoError(t, err) - - provider, err := createProvider(source.authSource.Name, source) - require.NoError(t, err) - - t.Run("refresh", func(t *testing.T) { - t.Run("valid", func(t *testing.T) { - err := source.refresh(context.Background(), provider, e) - require.NoError(t, err) - - e := &user_model.ExternalLoginUser{ - ExternalID: e.ExternalID, - LoginSourceID: e.LoginSourceID, - } - - ok, err := user_model.GetExternalLogin(context.Background(), e) - require.NoError(t, err) - assert.True(t, ok) - assert.Equal(t, "refresh", e.RefreshToken) - assert.Equal(t, "token", e.AccessToken) - - u, err := user_model.GetUserByID(context.Background(), user.ID) - require.NoError(t, err) - assert.True(t, u.IsActive) - }) - - t.Run("expired", func(t *testing.T) { - err := source.refresh(context.Background(), provider, &user_model.ExternalLoginUser{ - ExternalID: "external", - UserID: user.ID, - LoginSourceID: user.LoginSource, - RefreshToken: "expired", - }) - require.NoError(t, err) - - e := &user_model.ExternalLoginUser{ - ExternalID: e.ExternalID, - LoginSourceID: e.LoginSourceID, - } - - ok, err := user_model.GetExternalLogin(context.Background(), e) - require.NoError(t, err) - assert.True(t, ok) - assert.Equal(t, "", e.RefreshToken) - assert.Equal(t, "", e.AccessToken) - - u, err := user_model.GetUserByID(context.Background(), user.ID) - require.NoError(t, err) - assert.False(t, u.IsActive) - }) - }) -} diff --git a/services/externalaccount/user.go b/services/externalaccount/user.go index b53e33654a..3cfd8c81f9 100644 --- a/services/externalaccount/user.go +++ b/services/externalaccount/user.go @@ -71,14 +71,14 @@ func LinkAccountToUser(ctx context.Context, user *user_model.User, gothUser goth return nil } -// EnsureLinkExternalToUser link the gothUser to the user -func EnsureLinkExternalToUser(ctx context.Context, user *user_model.User, gothUser goth.User) error { +// UpdateExternalUser updates external user's information +func UpdateExternalUser(ctx context.Context, user *user_model.User, gothUser goth.User) error { externalLoginUser, err := toExternalLoginUser(ctx, user, gothUser) if err != nil { return err } - return user_model.EnsureLinkExternalToUser(ctx, externalLoginUser) + return user_model.UpdateExternalUserByExternalID(ctx, externalLoginUser) } // UpdateMigrationsByType updates all migrated repositories' posterid from gitServiceType to replace originalAuthorID to posterID diff --git a/templates/admin/auth/edit.tmpl b/templates/admin/auth/edit.tmpl index a8b2049f92..8a8bd61148 100644 --- a/templates/admin/auth/edit.tmpl +++ b/templates/admin/auth/edit.tmpl @@ -416,7 +416,7 @@

{{ctx.Locale.Tr "admin.auths.sspi_default_language_helper"}}

{{end}} - {{if (or .Source.IsLDAP .Source.IsOAuth2)}} + {{if .Source.IsLDAP}}
diff --git a/templates/admin/auth/new.tmpl b/templates/admin/auth/new.tmpl index 47fa82825c..c70bd3ba43 100644 --- a/templates/admin/auth/new.tmpl +++ b/templates/admin/auth/new.tmpl @@ -59,7 +59,7 @@
-
+
diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index 04c8a4b614..9bcb532d75 100644 --- a/tests/integration/auth_ldap_test.go +++ b/tests/integration/auth_ldap_test.go @@ -245,8 +245,7 @@ func TestLDAPUserSync(t *testing.T) { } defer tests.PrepareTestEnv(t)() addAuthSourceLDAP(t, "", "", "", "") - err := auth.SyncExternalUsers(context.Background(), true) - require.NoError(t, err) + auth.SyncExternalUsers(context.Background(), true) // Check if users exists for _, gitLDAPUser := range gitLDAPUsers {