From 821e38573ceaa62ffa067b4e173fad50f0f20f05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Dachary?= Date: Thu, 20 Jul 2023 22:29:05 +0200 Subject: [PATCH] [F3] user.Put: idempotency --- services/f3/driver/user.go | 119 ++++++++++++++++++++++++++++------- tests/integration/f3_test.go | 70 +++++++++++++++++++-- 2 files changed, 162 insertions(+), 27 deletions(-) diff --git a/services/f3/driver/user.go b/services/f3/driver/user.go index bef7906ed2..cc3ee54970 100644 --- a/services/f3/driver/user.go +++ b/services/f3/driver/user.go @@ -5,6 +5,7 @@ package driver import ( "context" "fmt" + "strings" auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" @@ -48,7 +49,29 @@ func (o *User) IsNil() bool { } func (o *User) Equals(other *User) bool { - return (o.ID == other.ID) + if o.ID != other.ID { + return false + } + // + // Only compare user data if both are managed by F3 otherwise + // they are equal if they have the same ID. Here is an example: + // + // * mirror from F3 to Forgejo => user jane created and assigned + // ID 213 & IsF3() + // * mirror from F3 to Forgejo => user jane username in F3 is updated + // the username for user ID 213 in Forgejo is also updated + // * user jane sign in with OAuth from the same source as the + // F3 mirror. They are promoted to IsIndividual() + // * mirror from F3 to Forgejo => user jane username in F3 is updated + // the username for user ID 213 in Forgejo is **NOT** updated, it + // no longer is managed by F3 + // + if !o.IsF3() || !other.IsF3() { + return true + } + return (o.Name == other.Name && + o.FullName == other.FullName && + o.Email == other.Email) } func (o *User) ToFormatInterface() format.Interface { @@ -68,6 +91,7 @@ func (o *User) ToFormat() *format.User { func (o *User) FromFormat(user *format.User) { *o = User{ User: user_model.User{ + Type: user_model.UserTypeF3, ID: user.Index.GetID(), Name: user.UserName, FullName: user.Name, @@ -122,12 +146,14 @@ func (o *UserProvider) FromFormat(ctx context.Context, p *format.User) *User { } func (o *UserProvider) GetObjects(ctx context.Context, page int) []*User { - users, _, err := user_model.SearchUsers(&user_model.SearchUserOptions{ - Actor: o.g.GetDoer(), - Type: user_model.UserTypeIndividual, - ListOptions: db.ListOptions{Page: page, PageSize: o.g.perPage}, - }) - if err != nil { + sess := db.GetEngine(ctx).In("type", user_model.UserTypeIndividual, user_model.UserTypeF3) + if page != 0 { + sess = db.SetSessionPagination(sess, &db.ListOptions{Page: page, PageSize: o.g.perPage}) + } + sess = sess.Select("`user`.*") + users := make([]*user_model.User, 0, o.g.perPage) + + if err := sess.Find(&users); err != nil { panic(fmt.Errorf("error while listing users: %v", err)) } return f3_util.ConvertMap[*user_model.User, *User](users, UserConverter) @@ -136,15 +162,29 @@ func (o *UserProvider) GetObjects(ctx context.Context, page int) []*User { func (o *UserProvider) ProcessObject(ctx context.Context, user *User) { } +func GetUserByName(ctx context.Context, name string) (*user_model.User, error) { + if len(name) == 0 { + return nil, user_model.ErrUserNotExist{Name: name} + } + u := &user_model.User{Name: name} + has, err := db.GetEngine(ctx).In("type", user_model.UserTypeIndividual, user_model.UserTypeF3).Get(u) + if err != nil { + return nil, err + } else if !has { + return nil, user_model.ErrUserNotExist{Name: name} + } + return u, nil +} + func (o *UserProvider) Get(ctx context.Context, exemplar *User) *User { - o.g.GetLogger().Debug("%+v", exemplar) + o.g.GetLogger().Debug("%+v", *exemplar) var user *user_model.User var err error if exemplar.GetID() > 0 { user, err = user_model.GetUserByID(ctx, exemplar.GetID()) - o.g.GetLogger().Debug("GetUserByID: %+v %v", user, err) + o.g.GetLogger().Debug("%+v %v", user, err) } else if exemplar.Name != "" { - user, err = user_model.GetUserByName(ctx, exemplar.Name) + user, err = GetUserByName(ctx, exemplar.Name) } else { panic("GetID() == 0 and UserName == \"\"") } @@ -152,26 +192,61 @@ func (o *UserProvider) Get(ctx context.Context, exemplar *User) *User { if user_model.IsErrUserNotExist(err) { return &User{} } - panic(fmt.Errorf("user %v %w", exemplar, err)) + panic(fmt.Errorf("user %+v %w", *exemplar, err)) } return UserConverter(user) } func (o *UserProvider) Put(ctx context.Context, user *User) *User { - overwriteDefault := &user_model.CreateUserOverwriteOptions{ - IsActive: util.OptionalBoolTrue, + o.g.GetLogger().Trace("begin %+v", *user) + u := &user_model.User{ + ID: user.GetID(), + Type: user_model.UserTypeF3, } - u := user_model.User{ - Name: user.Name, - FullName: user.FullName, - Email: user.Email, - Passwd: user.Passwd, + // + // Get the user, if any + // + var has bool + var err error + if u.ID > 0 { + has, err = db.GetEngine(ctx).Get(u) + if err != nil { + panic(err) + } } - err := user_model.CreateUser(&u, overwriteDefault) - if err != nil { - panic(err) + // + // Set user information + // + u.Name = user.Name + u.LowerName = strings.ToLower(u.Name) + u.FullName = user.FullName + u.Email = user.Email + if !has { + // + // The user does not exist, create it + // + o.g.GetLogger().Trace("creating %+v", *u) + u.ID = 0 + u.Passwd = user.Passwd + overwriteDefault := &user_model.CreateUserOverwriteOptions{ + IsActive: util.OptionalBoolTrue, + } + err := user_model.CreateUser(u, overwriteDefault) + if err != nil { + panic(err) + } + } else { + // + // The user already exists, update it + // + o.g.GetLogger().Trace("updating %+v", *u) + if err := user_model.UpdateUserCols(ctx, u, "name", "lower_name", "email", "full_name"); err != nil { + panic(err) + } } - return o.Get(ctx, UserConverter(&u)) + r := o.Get(ctx, UserConverter(u)) + o.g.GetLogger().Trace("finish %+v", r.User) + return r } func (o *UserProvider) Delete(ctx context.Context, user *User) *User { diff --git a/tests/integration/f3_test.go b/tests/integration/f3_test.go index d7e10af36b..1673eaa4c4 100644 --- a/tests/integration/f3_test.go +++ b/tests/integration/f3_test.go @@ -30,7 +30,7 @@ import ( f3_util "lab.forgefriends.org/friendlyforgeformat/gof3/util" ) -func TestF3(t *testing.T) { +func TestF3Mirror(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { AllowLocalNetworks := setting.Migrations.AllowLocalNetworks setting.F3.Enabled = true @@ -53,6 +53,7 @@ func TestF3(t *testing.T) { Directory: tmpDir, }, Features: gof3.AllFeatures, + Logger: util.ToF3Logger(nil), }, Remap: true, }) @@ -75,7 +76,7 @@ func TestF3(t *testing.T) { fixture.NewCommentReaction() // - // Step 2: mirror the fixture into Forgejo + // Step 2: mirror F3 into Forgejo // doer, err := user_model.GetAdminUser(context.Background()) assert.NoError(t, err) @@ -181,7 +182,7 @@ func TestMaybePromoteF3User(t *testing.T) { assert.Equal(t, userAfterSignIn.Email, gitlabEmail) } -func TestF3UserMapping(t *testing.T) { +func TestF3UserMappingExisting(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { AllowLocalNetworks := setting.Migrations.AllowLocalNetworks setting.F3.Enabled = true @@ -194,7 +195,7 @@ func TestF3UserMapping(t *testing.T) { setting.AppVer = AppVer }() - log.Debug("Step 1: create a fixture") + log.Debug("Step 1: create a fixture in F3") fixtureNewF3Forge := func(t f3_tests.TestingT, user *format.User, tmpDir string) *f3_forges.ForgeRoot { root := f3_forges.NewForgeRoot(&f3_f3.Options{ Options: gof3.Options{ @@ -213,7 +214,7 @@ func TestF3UserMapping(t *testing.T) { fixture.NewUser(userID) // fixture.NewProject() - log.Debug("Step 2: mirror the fixture into Forgejo") + log.Debug("Step 2: mirror F3 into Forgejo") // // OAuth2 authentication source GitLab // @@ -268,3 +269,62 @@ func TestF3UserMapping(t *testing.T) { assert.Contains(t, files, fmt.Sprintf("/user/%d", gitlabUser.ID)) }) } + +func TestF3UserMappingNew(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + AllowLocalNetworks := setting.Migrations.AllowLocalNetworks + setting.F3.Enabled = true + setting.Migrations.AllowLocalNetworks = true + AppVer := setting.AppVer + // Gitea SDK (go-sdk) need to parse the AppVer from server response, so we must set it to a valid version string. + setting.AppVer = "1.16.0" + defer func() { + setting.Migrations.AllowLocalNetworks = AllowLocalNetworks + setting.AppVer = AppVer + }() + + log.Debug("Step 1: create a fixture in F3") + fixtureNewF3Forge := func(t f3_tests.TestingT, user *format.User, tmpDir string) *f3_forges.ForgeRoot { + root := f3_forges.NewForgeRoot(&f3_f3.Options{ + Options: gof3.Options{ + Configuration: gof3.Configuration{ + Directory: tmpDir, + }, + Features: gof3.AllFeatures, + Logger: util.ToF3Logger(nil), + }, + Remap: true, + }) + return root + } + fixture := f3_forges.NewFixture(t, f3_forges.FixtureForgeFactory{Fun: fixtureNewF3Forge, AdminRequired: false}) + userID := int64(5432) + fixture.NewUser(userID) + + log.Debug("Step 2: mirror F3 into Forgejo") + doer, err := user_model.GetAdminUser(context.Background()) + assert.NoError(t, err) + forgejoLocalDestination := util.ForgejoForgeRoot(gof3.AllFeatures, doer, 0) + options := f3_common.NewMirrorOptionsRecurse() + forgejoLocalDestination.Forge.Mirror(context.Background(), fixture.Forge, options) + + log.Debug("Step 3: change the Name of the user in F3 and mirror to Forgejo") + otherusername := "otheruser" + fixture.UserFormat.UserName = otherusername + fixture.Forge.Users.Upsert(context.Background(), fixture.UserFormat) + forgejoLocalDestination.Forge.Mirror(context.Background(), fixture.Forge, options) + + log.Debug("Step 4: mirror Forgejo into F3 using the changed name") + f3 := util.F3ForgeRoot(gof3.AllFeatures, t.TempDir()) + forgejoLocalOrigin := util.ForgejoForgeRoot(gof3.AllFeatures, doer, 0) + forgejoLocalOriginUser := forgejoLocalOrigin.Forge.Users.GetFromFormat(context.Background(), &format.User{UserName: otherusername}) + options = f3_common.NewMirrorOptionsRecurse(forgejoLocalOriginUser) + f3.Forge.Mirror(context.Background(), forgejoLocalOrigin.Forge, options) + + // + // verify the fixture and F3 are equivalent + // + files := f3_util.Command(context.Background(), "find", f3.GetDirectory()) + assert.Contains(t, files, fmt.Sprintf("/user/%d", forgejoLocalOriginUser.GetID())) + }) +}