From 01858e3d7136943b271466b73b7f403fd4822101 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Thu, 29 Apr 2021 17:37:06 +0800 Subject: [PATCH] Clean up user related funcs in common/dao This commit remove some funcs from package `common/dao/user` that can be covered by the manager in `pkg/user`. Ideally all funcs should be replaced but the dependency relationships are tricky for some of them I'll push other commit to clean them up. Signed-off-by: Daniel Jiang --- src/common/dao/config_test.go | 73 -------------- src/common/dao/dao_test.go | 80 --------------- src/common/dao/oidc_user_test.go | 5 - src/common/dao/user.go | 102 -------------------- src/common/dao/user_test.go | 52 ---------- src/common/models/user.go | 8 -- src/controller/quota/driver/project/util.go | 12 ++- src/core/api/api_test.go | 8 +- src/core/api/dataprepare_test.go | 94 ------------------ src/core/api/harborapi_test.go | 2 + 10 files changed, 14 insertions(+), 422 deletions(-) delete mode 100644 src/common/dao/config_test.go delete mode 100644 src/core/api/dataprepare_test.go diff --git a/src/common/dao/config_test.go b/src/common/dao/config_test.go deleted file mode 100644 index fea70c07c..000000000 --- a/src/common/dao/config_test.go +++ /dev/null @@ -1,73 +0,0 @@ -// Copyright Project Harbor Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package dao - -import ( - "github.com/goharbor/harbor/src/lib/orm" - "testing" - - "github.com/goharbor/harbor/src/common/models" -) - -func TestAuthModeCanBeModified(t *testing.T) { - ctx := orm.Context() - c, err := GetOrmer().QueryTable(&models.User{}).Count() - if err != nil { - t.Fatalf("failed to count users: %v", err) - } - - if c == 2 { - flag, err := AuthModeCanBeModified(ctx) - if err != nil { - t.Fatalf("failed to determine whether auth mode can be modified: %v", err) - } - if !flag { - t.Errorf("unexpected result: %t != %t", flag, true) - } - - user := models.User{ - Username: "user_for_config_test", - Email: "user_for_config_test@vmware.com", - Password: "P@ssword", - Realname: "user_for_config_test", - } - id, err := Register(user) - if err != nil { - t.Fatalf("failed to register user: %v", err) - } - defer func(id int64) { - if err := CleanUser(id); err != nil { - t.Fatalf("failed to delete user %d: %v", id, err) - } - }(id) - - flag, err = AuthModeCanBeModified(ctx) - if err != nil { - t.Fatalf("failed to determine whether auth mode can be modified: %v", err) - } - if flag { - t.Errorf("unexpected result: %t != %t", flag, false) - } - - } else { - flag, err := AuthModeCanBeModified(ctx) - if err != nil { - t.Fatalf("failed to determine whether auth mode can be modified: %v", err) - } - if flag { - t.Errorf("unexpected result: %t != %t", flag, false) - } - } -} diff --git a/src/common/dao/dao_test.go b/src/common/dao/dao_test.go index 938f8ddc7..b36f4fb3f 100644 --- a/src/common/dao/dao_test.go +++ b/src/common/dao/dao_test.go @@ -27,7 +27,6 @@ import ( "github.com/goharbor/harbor/src/lib/log" libOrm "github.com/goharbor/harbor/src/lib/orm" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) var testCtx context.Context @@ -101,15 +100,6 @@ const username string = "Tester01" const password string = "Abc12345" const projectName string = "test_project" const repositoryName string = "test_repository" -const repoTag string = "test1.1" -const repoTag2 string = "test1.2" -const SysAdmin int = 1 -const projectAdmin int = 2 -const developer int = 3 -const guest int = 4 - -const publicityOn = 1 -const publicityOff = 0 func TestMain(m *testing.M) { databases := []string{"postgresql"} @@ -281,22 +271,6 @@ func TestGetUser(t *testing.T) { assert.NotNil(t, err) } -func TestListUsers(t *testing.T) { - users, err := ListUsers(nil) - require.Nil(t, err) - assert.Greater(t, len(users), 0) - users2, err := ListUsers(&models.UserQuery{Username: username}) - require.Nil(t, err) - assert.Equal(t, 1, len(users2)) - assert.Equal(t, username, users2[0].Username) - users3, err := ListUsers(&models.UserQuery{Username: username, Pagination: &models.Pagination{Page: 2, Size: 1}}) - require.Nil(t, err) - assert.Equal(t, 0, len(users3)) - users4, err := ListUsers(&models.UserQuery{Username: "__"}) - require.Nil(t, err) - assert.Equal(t, 0, len(users4)) -} - func TestResetUserPassword(t *testing.T) { uuid := utils.GenerateRandomString() @@ -425,60 +399,6 @@ func TestGetProjects(t *testing.T) { } } -// isAdminRole returns whether the user is admin. -func isAdminRole(userIDOrUsername interface{}) (bool, error) { - u := models.User{} - - switch v := userIDOrUsername.(type) { - case int: - u.UserID = v - case string: - u.Username = v - default: - return false, fmt.Errorf("invalid parameter, only int and string are supported: %v", userIDOrUsername) - } - - if u.UserID == NonExistUserID && len(u.Username) == 0 { - return false, nil - } - - user, err := GetUser(u) - if err != nil { - return false, err - } - - if user == nil { - return false, nil - } - - return user.SysAdminFlag, nil -} - -func TestToggleAdminRole(t *testing.T) { - err := ToggleUserAdminRole(currentUser.UserID, true) - if err != nil { - t.Errorf("Error in toggle ToggleUserAdmin role: %v, user: %+v", err, currentUser) - } - isAdmin, err := isAdminRole(currentUser.UserID) - if err != nil { - t.Errorf("Error in IsAdminRole: %v, user id: %d", err, currentUser.UserID) - } - if !isAdmin { - t.Errorf("User is not admin after toggled, user id: %d", currentUser.UserID) - } - err = ToggleUserAdminRole(currentUser.UserID, false) - if err != nil { - t.Errorf("Error in toggle ToggleUserAdmin role: %v, user: %+v", err, currentUser) - } - isAdmin, err = isAdminRole(currentUser.UserID) - if err != nil { - t.Errorf("Error in IsAdminRole: %v, user id: %d", err, currentUser.UserID) - } - if isAdmin { - t.Errorf("User is still admin after toggled, user id: %d", currentUser.UserID) - } -} - func TestChangeUserProfile(t *testing.T) { user := models.User{UserID: currentUser.UserID, Email: username + "@163.com", Realname: "test", Comment: "Unit Test"} err := ChangeUserProfile(user) diff --git a/src/common/dao/oidc_user_test.go b/src/common/dao/oidc_user_test.go index 711d1143c..c538f5f12 100644 --- a/src/common/dao/oidc_user_test.go +++ b/src/common/dao/oidc_user_test.go @@ -60,11 +60,6 @@ func TestOIDCUserMetaDaoMethods(t *testing.T) { require.NotNil(t, err) assert.Equal(t, "unable to onboard as empty oidc user", err.Error()) - // test get by userID - oidcUser2, err := GetOIDCUserByUserID(user111.UserID) - require.Nil(t, err) - assert.Equal(t, "QWE123123RT1", oidcUser2.SubIss) - // test get by sub and iss userGetBySubIss, err := GetUserBySubIss("QWE123", "123RT1") require.Nil(t, err) diff --git a/src/common/dao/user.go b/src/common/dao/user.go index 83ecd30ca..cdd09b019 100644 --- a/src/common/dao/user.go +++ b/src/common/dao/user.go @@ -15,14 +15,10 @@ package dao import ( - "context" "errors" "fmt" - libOrm "github.com/goharbor/harbor/src/lib/orm" "time" - "github.com/astaxie/beego/orm" - "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/lib/log" @@ -98,68 +94,6 @@ func LoginByDb(auth models.AuthModel) (*models.User, error) { return &user, nil } -// GetTotalOfUsers ... -func GetTotalOfUsers(query *models.UserQuery) (int64, error) { - return userQueryConditions(query).Count() -} - -// ListUsers lists all users according to different conditions. -func ListUsers(query *models.UserQuery) ([]models.User, error) { - qs := userQueryConditions(query) - if query != nil && query.Pagination != nil { - offset := (query.Pagination.Page - 1) * query.Pagination.Size - qs = qs.Offset(offset).Limit(query.Pagination.Size) - } - users := []models.User{} - _, err := qs.OrderBy("username").All(&users) - return users, err -} - -func userQueryConditions(query *models.UserQuery) orm.QuerySeter { - qs := GetOrmer().QueryTable(&models.User{}).Filter("deleted", 0) - - if query == nil { - // Exclude admin account, see https://github.com/goharbor/harbor/issues/2527 - return qs.Filter("user_id__gt", 1) - } - - if len(query.UserIDs) > 0 { - qs = qs.Filter("user_id__in", query.UserIDs) - } else { - // Exclude admin account when not filter by UserIDs, see https://github.com/goharbor/harbor/issues/2527 - qs = qs.Filter("user_id__gt", 1) - } - - if len(query.Username) > 0 { - qs = qs.Filter("username__contains", libOrm.Escape(query.Username)) - } - - if len(query.Email) > 0 { - qs = qs.Filter("email__contains", libOrm.Escape(query.Email)) - } - - return qs -} - -// ToggleUserAdminRole gives a user admin role. -func ToggleUserAdminRole(userID int, hasAdmin bool) error { - o := GetOrmer() - queryParams := make([]interface{}, 1) - sql := `update harbor_user set sysadmin_flag = ? where user_id = ?` - queryParams = append(queryParams, hasAdmin) - queryParams = append(queryParams, userID) - r, err := o.Raw(sql, queryParams).Exec() - if err != nil { - return err - } - - if _, err := r.RowsAffected(); err != nil { - return err - } - - return nil -} - // ChangeUserPassword ... func ChangeUserPassword(u models.User) error { u.UpdateTime = time.Now() @@ -204,26 +138,6 @@ func UpdateUserResetUUID(u models.User) error { return err } -// DeleteUser ... -func DeleteUser(userID int) error { - o := GetOrmer() - - user, err := GetUser(models.User{ - UserID: userID, - }) - if err != nil { - return err - } - - name := fmt.Sprintf("%s#%d", user.Username, user.UserID) - email := fmt.Sprintf("%s#%d", user.Email, user.UserID) - - _, err = o.Raw(`update harbor_user - set deleted = true, username = ?, email = ? - where user_id = ?`, name, email, userID).Exec() - return err -} - // ChangeUserProfile - Update user in local db, // cols to specify the columns need to update, // Email, and RealName, Comment are updated by default. @@ -294,19 +208,3 @@ func CleanUser(id int64) error { func matchPassword(u *models.User, password string) bool { return utils.Encrypt(password, u.Salt, u.PasswordVersion) == u.Password } - -// AuthModeCanBeModified determines whether auth mode can be -// modified or not. Auth mode can modified when there is only admin -// user in database. -func AuthModeCanBeModified(ctx context.Context) (bool, error) { - o, err := libOrm.FromContext(ctx) - if err != nil { - return false, err - } - c, err := o.QueryTable(&models.User{}).Count() - if err != nil { - return false, err - } - // admin and anonymous - return c == 2, nil -} diff --git a/src/common/dao/user_test.go b/src/common/dao/user_test.go index 2b3029c17..58a579c78 100644 --- a/src/common/dao/user_test.go +++ b/src/common/dao/user_test.go @@ -15,64 +15,12 @@ package dao import ( - "fmt" "testing" "github.com/goharbor/harbor/src/common/models" "github.com/stretchr/testify/assert" ) -func TestDeleteUser(t *testing.T) { - username := "user_for_test" - email := "user_for_test@vmware.com" - password := "P@ssword" - realname := "user_for_test" - - u := models.User{ - Username: username, - Email: email, - Password: password, - Realname: realname, - } - id, err := Register(u) - if err != nil { - t.Fatalf("failed to register user: %v", err) - } - defer func(id int64) { - if err := CleanUser(id); err != nil { - t.Fatalf("failed to delete user %d: %v", id, err) - } - }(id) - - err = DeleteUser(int(id)) - if err != nil { - t.Fatalf("Error occurred in DeleteUser: %v", err) - } - - user := &models.User{} - sql := "select * from harbor_user where user_id = ?" - if err = GetOrmer().Raw(sql, id). - QueryRow(user); err != nil { - t.Fatalf("failed to query user: %v", err) - } - - if user.Deleted != true { - t.Error("user is not deleted") - } - - expected := fmt.Sprintf("%s#%d", u.Username, id) - if user.Username != expected { - t.Errorf("unexpected username: %s != %s", user.Username, - expected) - } - - expected = fmt.Sprintf("%s#%d", u.Email, id) - if user.Email != expected { - t.Errorf("unexpected email: %s != %s", user.Email, - expected) - } -} - func TestOnBoardUser(t *testing.T) { assert := assert.New(t) u := &models.User{ diff --git a/src/common/models/user.go b/src/common/models/user.go index 6ef5daf3b..9d9bf2e75 100644 --- a/src/common/models/user.go +++ b/src/common/models/user.go @@ -46,14 +46,6 @@ type User struct { OIDCUserMeta *OIDCUser `orm:"-" json:"oidc_user_meta,omitempty"` } -// UserQuery ... -type UserQuery struct { - UserIDs []int - Username string - Email string - Pagination *Pagination -} - // TableName ... func (u *User) TableName() string { return UserTable diff --git a/src/controller/quota/driver/project/util.go b/src/controller/quota/driver/project/util.go index 1341cbaf3..55883a69b 100644 --- a/src/controller/quota/driver/project/util.go +++ b/src/controller/quota/driver/project/util.go @@ -18,11 +18,11 @@ import ( "context" "strconv" - "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/q" "github.com/goharbor/harbor/src/pkg/project" + "github.com/goharbor/harbor/src/pkg/user" "github.com/graph-gophers/dataloader" ) @@ -49,22 +49,24 @@ func getProjectsBatchFn(ctx context.Context, keys dataloader.Keys) []*dataloader return handleError(err) } - var ownerIDs []int + var ownerIDs []interface{} var projectsMap = make(map[int64]*models.Project, len(projectIDs)) for _, project := range projects { ownerIDs = append(ownerIDs, project.OwnerID) projectsMap[project.ProjectID] = project } - // TODO: change to use user manager - owners, err := dao.ListUsers(&models.UserQuery{UserIDs: ownerIDs}) + owners, err := user.Mgr.List(ctx, q.New(q.KeyWords{ + "UserID": q.NewOrList(ownerIDs), + })) + if err != nil { return handleError(err) } var ownersMap = make(map[int]*models.User, len(owners)) for i, owner := range owners { - ownersMap[owner.UserID] = &owners[i] + ownersMap[owner.UserID] = owners[i] } var results []*dataloader.Result diff --git a/src/core/api/api_test.go b/src/core/api/api_test.go index 1fd1d5026..8046d3075 100644 --- a/src/core/api/api_test.go +++ b/src/core/api/api_test.go @@ -16,7 +16,6 @@ package api import ( "encoding/json" "fmt" - "github.com/goharbor/harbor/src/lib/orm" "io/ioutil" "net/http" "net/http/httptest" @@ -28,6 +27,8 @@ import ( "github.com/goharbor/harbor/src/chartserver" "github.com/goharbor/harbor/src/common" + "github.com/goharbor/harbor/src/lib/orm" + "github.com/goharbor/harbor/src/pkg/user" "github.com/dghubble/sling" "github.com/goharbor/harbor/src/common/dao" @@ -338,16 +339,17 @@ func prepare() error { } func clean() { + ctx := orm.Context() pmids := []int{projAdminPMID, projDeveloperPMID, projGuestPMID} for _, id := range pmids { - if err := member.Mgr.Delete(orm.Context(), 1, id); err != nil { + if err := member.Mgr.Delete(ctx, 1, id); err != nil { fmt.Printf("failed to clean up member %d from project library: %v", id, err) } } userids := []int64{nonSysAdminID, projAdminID, projDeveloperID, projGuestID} for _, id := range userids { - if err := dao.DeleteUser(int(id)); err != nil { + if err := user.Mgr.Delete(ctx, int(id)); err != nil { fmt.Printf("failed to clean up user %d: %v \n", id, err) } } diff --git a/src/core/api/dataprepare_test.go b/src/core/api/dataprepare_test.go deleted file mode 100644 index 7d640454f..000000000 --- a/src/core/api/dataprepare_test.go +++ /dev/null @@ -1,94 +0,0 @@ -// Copyright 2018 Project Harbor Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package api - -import ( - "github.com/goharbor/harbor/src/common/dao" - "github.com/goharbor/harbor/src/common/models" -) - -const ( - // Prepare Test info - TestUserName = "testUser0001" - TestUserPwd = "testUser0001" - TestUserEmail = "testUser0001@mydomain.com" - TestProName = "testProject0001" - TestRegistryName = "testRegistry0001" - TestRepoName = "testRepo0001" -) - -func CommonAddUser() { - - commonUser := models.User{ - Username: TestUserName, - Password: TestUserPwd, - Email: TestUserEmail, - } - - _, _ = dao.Register(commonUser) - -} - -func CommonGetUserID() int { - queryUser := &models.User{ - Username: TestUserName, - } - commonUser, _ := dao.GetUser(*queryUser) - return commonUser.UserID -} - -func CommonDelUser() { - queryUser := &models.User{ - Username: TestUserName, - } - commonUser, _ := dao.GetUser(*queryUser) - _ = dao.DeleteUser(commonUser.UserID) - -} - -func CommonAddProject() { - - queryUser := &models.User{ - Username: "admin", - } - adminUser, _ := dao.GetUser(*queryUser) - commonProject := &models.Project{ - Name: TestProName, - OwnerID: adminUser.UserID, - } - - _, _ = dao.AddProject(*commonProject) - -} - -func CommonDelProject() { - commonProject, _ := dao.GetProjectByName(TestProName) - - _ = dao.DeleteProject(commonProject.ProjectID) -} - -func CommonAddRepository() { - commonRepository := &models.RepoRecord{ - RepositoryID: 31, - Name: TestRepoName, - ProjectID: 1, - PullCount: 1, - } - _ = dao.AddRepository(*commonRepository) -} - -func CommonDelRepository() { - _ = dao.DeleteRepository(TestRepoName) -} diff --git a/src/core/api/harborapi_test.go b/src/core/api/harborapi_test.go index 074a3bb19..9653f4279 100644 --- a/src/core/api/harborapi_test.go +++ b/src/core/api/harborapi_test.go @@ -47,6 +47,8 @@ import ( ) const ( + TestUserName = "testUser0001" + TestUserPwd = "testUser0001" jsonAcceptHeader = "application/json" testAcceptHeader = "text/plain" adminName = "admin"