Merge pull request 'fix: Revert "allow synchronizing user status from OAuth2 login providers (#31572)"' (#6249) from earl-warren/forgejo:wip-oauth2 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6249

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
 	}
 

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
+	Provider string
-	UserID          int64
+	UserID   int64
-	LoginSourceID   int64
+	OrderBy  string
-	HasRefreshToken bool
-	Expired         bool
-	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)
-}

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 {
+		if err := externalaccount.UpdateExternalUser(ctx, u, *gothUser); err != nil {
-			log.Error("EnsureLinkExternalToUser failed: %v", err)
+			if !errors.Is(err, util.ErrNotExist) {
+				log.Error("UpdateExternalUser failed: %v", err)
+			}
 		}
 	}
 

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,

services/auth/source/oauth2/main_test.go

@@ -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{})
-}

services/auth/source/oauth2/providers_test.go

@@ -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{}
-			}))
-}

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)
 }

services/auth/source/oauth2/source_sync.go

@@ -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
-}

services/auth/source/oauth2/source_sync_test.go

@@ -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)
-		})
-	})
-}

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
+// UpdateExternalUser updates external user's information
-func EnsureLinkExternalToUser(ctx context.Context, user *user_model.User, gothUser goth.User) error {
+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

templates/admin/auth/edit.tmpl

@@ -416,7 +416,7 @@
 						<p class="help">{{ctx.Locale.Tr "admin.auths.sspi_default_language_helper"}}</p>
 					</div>
 				{{end}}
-				{{if (or .Source.IsLDAP .Source.IsOAuth2)}}
+				{{if .Source.IsLDAP}}
 					<div class="inline field">
 						<div class="ui checkbox">
 							<label><strong>{{ctx.Locale.Tr "admin.auths.syncenabled"}}</strong></label>

templates/admin/auth/new.tmpl

@@ -59,7 +59,7 @@
 						<input name="attributes_in_bind" type="checkbox" {{if .attributes_in_bind}}checked{{end}}>
 					</div>
 				</div>
-				<div class="oauth2 ldap inline field {{if not (or (eq .type 2) (eq .type 6))}}tw-hidden{{end}}">
+				<div class="ldap inline field {{if not (eq .type 2)}}tw-hidden{{end}}">
 					<div class="ui checkbox">
 						<label><strong>{{ctx.Locale.Tr "admin.auths.syncenabled"}}</strong></label>
 						<input name="is_sync_enabled" type="checkbox" {{if .is_sync_enabled}}checked{{end}}>

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)
+	auth.SyncExternalUsers(context.Background(), true)
-	require.NoError(t, err)
 
 	// Check if users exists
 	for _, gitLDAPUser := range gitLDAPUsers {