diff --git a/api/harbor/swagger.yaml b/api/harbor/swagger.yaml index 7538305a9..159e8171b 100644 --- a/api/harbor/swagger.yaml +++ b/api/harbor/swagger.yaml @@ -938,12 +938,12 @@ paths: format: int required: true description: Registered user ID - - name: has_admin_role + - name: sysadmin_flag in: body description: Toggle a user to admin or not. required: true schema: - $ref: '#/definitions/HasAdminRole' + $ref: '#/definitions/SysAdminFlag' tags: - Products responses: @@ -4976,8 +4976,11 @@ definitions: role_id: type: integer format: int - has_admin_role: + sysadmin_flag: type: boolean + admin_role_in_auth: + type: boolean + description: indicate the admin privilege is grant by authenticator (LDAP), is always false unless it is the current login user reset_uuid: type: string Salt: @@ -5276,12 +5279,12 @@ definitions: insecure: type: boolean description: Whether or not the certificate will be verified when Harbor tries to access the server. - HasAdminRole: + SysAdminFlag: type: object properties: - has_admin_role: + sysadmin_flag: type: boolean - description: '1-has admin, 0-not.' + description: 'true-admin, false-not admin.' UserProfile: type: object properties: diff --git a/src/common/dao/dao_test.go b/src/common/dao/dao_test.go index b0f216059..7ca2622cc 100644 --- a/src/common/dao/dao_test.go +++ b/src/common/dao/dao_test.go @@ -626,12 +626,41 @@ func TestGetRoleByID(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) + isAdmin, err := isAdminRole(currentUser.UserID) if err != nil { t.Errorf("Error in IsAdminRole: %v, user id: %d", err, currentUser.UserID) } @@ -642,7 +671,7 @@ func TestToggleAdminRole(t *testing.T) { if err != nil { t.Errorf("Error in toggle ToggleUserAdmin role: %v, user: %+v", err, currentUser) } - isAdmin, err = IsAdminRole(currentUser.UserID) + isAdmin, err = isAdminRole(currentUser.UserID) if err != nil { t.Errorf("Error in IsAdminRole: %v, user id: %d", err, currentUser.UserID) } diff --git a/src/common/dao/register.go b/src/common/dao/register.go index fa6a8ac91..ef2e0b2c0 100644 --- a/src/common/dao/register.go +++ b/src/common/dao/register.go @@ -33,7 +33,7 @@ func Register(user models.User) (int64, error) { values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) RETURNING user_id` var userID int64 err := o.Raw(sql, user.Username, utils.Encrypt(user.Password, salt, utils.SHA256), utils.SHA256, user.Realname, user.Email, - user.Comment, salt, user.HasAdminRole, now, now).QueryRow(&userID) + user.Comment, salt, user.SysAdminFlag, now, now).QueryRow(&userID) if err != nil { return 0, err } diff --git a/src/common/dao/role.go b/src/common/dao/role.go index 9427af855..a55f78099 100644 --- a/src/common/dao/role.go +++ b/src/common/dao/role.go @@ -15,8 +15,6 @@ package dao import ( - "fmt" - "github.com/astaxie/beego/orm" "github.com/goharbor/harbor/src/common/models" ) @@ -44,35 +42,6 @@ func GetUserProjectRoles(userID int, projectID int64, entityType string) ([]mode return roleList, nil } -// 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.HasAdminRole, nil -} - // GetRoleByID ... func GetRoleByID(id int) (*models.Role, error) { o := GetOrmer() diff --git a/src/common/dao/user.go b/src/common/dao/user.go index 6eba2203b..639a7d036 100644 --- a/src/common/dao/user.go +++ b/src/common/dao/user.go @@ -262,7 +262,7 @@ func OnBoardUser(u *models.User) error { return err } u.Email = existing.Email - u.HasAdminRole = existing.HasAdminRole + u.SysAdminFlag = existing.SysAdminFlag u.Realname = existing.Realname u.UserID = existing.UserID } diff --git a/src/common/models/user.go b/src/common/models/user.go index c47ad03b8..400d53ee8 100644 --- a/src/common/models/user.go +++ b/src/common/models/user.go @@ -34,15 +34,16 @@ type User struct { Rolename string `orm:"-" json:"role_name"` // if this field is named as "RoleID", beego orm can not map role_id // to it. - Role int `orm:"-" json:"role_id"` - // RoleList []Role `json:"role_list"` - HasAdminRole bool `orm:"column(sysadmin_flag)" json:"has_admin_role"` - ResetUUID string `orm:"column(reset_uuid)" json:"reset_uuid"` - Salt string `orm:"column(salt)" json:"-"` - CreationTime time.Time `orm:"column(creation_time);auto_now_add" json:"creation_time"` - UpdateTime time.Time `orm:"column(update_time);auto_now" json:"update_time"` - GroupIDs []int `orm:"-" json:"-"` - OIDCUserMeta *OIDCUser `orm:"-" json:"oidc_user_meta,omitempty"` + Role int `orm:"-" json:"role_id"` + SysAdminFlag bool `orm:"column(sysadmin_flag)" json:"sysadmin_flag"` + // AdminRoleInAuth to store the admin privilege granted by external authentication provider + AdminRoleInAuth bool `orm:"-" json:"admin_role_in_auth"` + ResetUUID string `orm:"column(reset_uuid)" json:"reset_uuid"` + Salt string `orm:"column(salt)" json:"-"` + CreationTime time.Time `orm:"column(creation_time);auto_now_add" json:"creation_time"` + UpdateTime time.Time `orm:"column(update_time);auto_now" json:"update_time"` + GroupIDs []int `orm:"-" json:"-"` + OIDCUserMeta *OIDCUser `orm:"-" json:"oidc_user_meta,omitempty"` } // UserQuery ... diff --git a/src/common/security/local/context.go b/src/common/security/local/context.go index a80130da8..7ebbda506 100644 --- a/src/common/security/local/context.go +++ b/src/common/security/local/context.go @@ -52,13 +52,18 @@ func (s *SecurityContext) GetUsername() string { return s.user.Username } +// User get the current user +func (s *SecurityContext) User() *models.User { + return s.user +} + // IsSysAdmin returns whether the authenticated user is system admin // It returns false if the user has not been authenticated func (s *SecurityContext) IsSysAdmin() bool { if !s.IsAuthenticated() { return false } - return s.user.HasAdminRole + return s.user.SysAdminFlag || s.user.AdminRoleInAuth } // IsSolutionUser ... diff --git a/src/common/security/local/context_test.go b/src/common/security/local/context_test.go index 45cd14450..538c22163 100644 --- a/src/common/security/local/context_test.go +++ b/src/common/security/local/context_test.go @@ -162,7 +162,7 @@ func TestIsSysAdmin(t *testing.T) { // authenticated, admin ctx = NewSecurityContext(&models.User{ Username: "test", - HasAdminRole: true, + SysAdminFlag: true, }, nil) assert.True(t, ctx.IsSysAdmin()) } @@ -197,7 +197,7 @@ func TestHasPullPerm(t *testing.T) { // private project, authenticated, system admin ctx = NewSecurityContext(&models.User{ Username: "admin", - HasAdminRole: true, + SysAdminFlag: true, }, pm) assert.True(t, ctx.Can(rbac.ActionPull, resource)) } @@ -220,7 +220,7 @@ func TestHasPushPerm(t *testing.T) { // authenticated, system admin ctx = NewSecurityContext(&models.User{ Username: "admin", - HasAdminRole: true, + SysAdminFlag: true, }, pm) assert.True(t, ctx.Can(rbac.ActionPush, resource)) } @@ -239,7 +239,7 @@ func TestHasPushPullPerm(t *testing.T) { // authenticated, system admin ctx = NewSecurityContext(&models.User{ Username: "admin", - HasAdminRole: true, + SysAdminFlag: true, }, pm) assert.True(t, ctx.Can(rbac.ActionPush, resource) && ctx.Can(rbac.ActionPull, resource)) } diff --git a/src/core/api/harborapi_test.go b/src/core/api/harborapi_test.go index ffc891cee..7a0d4a4f0 100644 --- a/src/core/api/harborapi_test.go +++ b/src/core/api/harborapi_test.go @@ -978,7 +978,7 @@ func (a testapi) UsersToggleAdminRole(userID int, authInfo usrInfo, hasAdminRole path := "/api/users/" + fmt.Sprintf("%d", userID) + "/sysadmin" _sling = _sling.Path(path) type QueryParams struct { - HasAdminRole bool `json:"has_admin_role,omitempty"` + HasAdminRole bool `json:"sysadmin_flag,omitempty"` } _sling = _sling.BodyJSON(&QueryParams{HasAdminRole: hasAdminRole}) diff --git a/src/core/api/user.go b/src/core/api/user.go index 518b19d4e..93c11f92d 100644 --- a/src/core/api/user.go +++ b/src/core/api/user.go @@ -27,6 +27,7 @@ import ( "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/rbac/project" + "github.com/goharbor/harbor/src/common/security/local" "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/config" @@ -151,7 +152,11 @@ func (ua *UserAPI) Get() { } u.Password = "" if ua.userID == ua.currentUserID { - u.HasAdminRole = ua.SecurityCtx.IsSysAdmin() + sc := ua.SecurityCtx + switch lsc := sc.(type) { + case *local.SecurityContext: + u.AdminRoleInAuth = lsc.User().AdminRoleInAuth + } } if ua.AuthMode == common.OIDCAuth { o, err := ua.getOIDCUserInfo() @@ -336,7 +341,7 @@ func (ua *UserAPI) Post() { return } - if !ua.IsAdmin && user.HasAdminRole { + if !ua.IsAdmin && user.SysAdminFlag { msg := "Non-admin cannot create an admin user." log.Errorf(msg) ua.SendForbiddenError(errors.New(msg)) @@ -461,7 +466,7 @@ func (ua *UserAPI) ToggleUserAdminRole() { ua.SendBadRequestError(err) return } - if err := dao.ToggleUserAdminRole(userQuery.UserID, userQuery.HasAdminRole); err != nil { + if err := dao.ToggleUserAdminRole(userQuery.UserID, userQuery.SysAdminFlag); err != nil { log.Errorf("Error occurred in ToggleUserAdminRole: %v", err) ua.SendInternalServerError(errors.New("internal error")) return diff --git a/src/core/auth/ldap/ldap.go b/src/core/auth/ldap/ldap.go index 2f07ab6ae..4b24462b5 100644 --- a/src/core/auth/ldap/ldap.go +++ b/src/core/auth/ldap/ldap.go @@ -65,7 +65,6 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { log.Warningf("ldap search fail: %v", err) return nil, err } - if len(ldapUsers) == 0 { log.Warningf("Not found an entry.") return nil, auth.NewErrAuth("Not found an entry") @@ -75,16 +74,24 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { } log.Debugf("Found ldap user %+v", ldapUsers[0]) - u := models.User{} - u.Username = ldapUsers[0].Username - u.Email = strings.TrimSpace(ldapUsers[0].Email) - u.Realname = ldapUsers[0].Realname dn := ldapUsers[0].DN if err = ldapSession.Bind(dn, m.Password); err != nil { - log.Warningf("Failed to bind user, username: %s, dn: %s, error: %v", u.Username, dn, err) + log.Warningf("Failed to bind user, username: %s, dn: %s, error: %v", p, dn, err) return nil, auth.NewErrAuth(err.Error()) } + u := models.User{} + u.Username = ldapUsers[0].Username + u.Realname = ldapUsers[0].Realname + u.Email = strings.TrimSpace(ldapUsers[0].Email) + + l.syncUserInfoFromDB(&u) + l.attachLDAPGroup(ldapUsers, &u) + + return &u, nil +} + +func (l *Auth) attachLDAPGroup(ldapUsers []models.LdapUser, u *models.User) { // Retrieve ldap related info in login to avoid too many traffic with LDAP server. // Get group admin dn groupCfg, err := config.LDAPGroupConf() @@ -99,7 +106,7 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { groupDN = utils.TrimLower(groupDN) // Attach LDAP group admin if len(groupAdminDN) > 0 && groupAdminDN == groupDN { - u.HasAdminRole = true + u.AdminRoleInAuth = true } } @@ -111,8 +118,19 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { if err != nil { log.Warningf("Failed to fetch ldap group configuration:%v", err) } +} - return &u, nil +func (l *Auth) syncUserInfoFromDB(u *models.User) { + // Retrieve SysAdminFlag from DB so that it transfer to session + dbUser, err := dao.GetUser(models.User{Username: u.Username}) + if err != nil { + log.Errorf("failed to sync user info from DB error %v", err) + return + } + if dbUser == nil { + return + } + u.SysAdminFlag = dbUser.SysAdminFlag } // OnBoardUser will check if a user exists in user table, if not insert the user and @@ -233,8 +251,6 @@ func (l *Auth) PostAuthenticate(u *models.User) error { return nil } u.UserID = dbUser.UserID - // If user has admin role already, do not overwrite by user info in DB. - u.HasAdminRole = u.HasAdminRole || dbUser.HasAdminRole if dbUser.Email != u.Email { Re := regexp.MustCompile(`^[a-z0-9._%+\-]+@[a-z0-9.\-]+\.[a-z]{2,4}$`) diff --git a/src/core/auth/ldap/ldap_test.go b/src/core/auth/ldap/ldap_test.go index fb086a88b..6b4242166 100644 --- a/src/core/auth/ldap/ldap_test.go +++ b/src/core/auth/ldap/ldap_test.go @@ -163,7 +163,7 @@ func TestAuthenticateWithAdmin(t *testing.T) { if user.Username != "mike" { t.Errorf("unexpected ldap user authenticate fail: %s = %s", "user.Username", user.Username) } - if !user.HasAdminRole { + if !user.AdminRoleInAuth { t.Errorf("ldap user mike should have admin role!") } } @@ -179,7 +179,7 @@ func TestAuthenticateWithoutAdmin(t *testing.T) { if user.Username != "user001" { t.Errorf("unexpected ldap user authenticate fail: %s = %s", "user.Username", user.Username) } - if user.HasAdminRole { + if user.AdminRoleInAuth { t.Errorf("ldap user user001 should not have admin role!") } } diff --git a/src/core/auth/uaa/uaa.go b/src/core/auth/uaa/uaa.go index 8ca250fc1..aedcdbf9e 100644 --- a/src/core/auth/uaa/uaa.go +++ b/src/core/auth/uaa/uaa.go @@ -92,7 +92,7 @@ func (u *Auth) PostAuthenticate(user *models.User) error { return u.OnBoardUser(user) } user.UserID = dbUser.UserID - user.HasAdminRole = dbUser.HasAdminRole + user.SysAdminFlag = dbUser.SysAdminFlag fillEmailRealName(user) if err2 := dao.ChangeUserProfile(*user, "Email", "Realname"); err2 != nil { log.Warningf("Failed to update user profile, user: %s, error: %v", user.Username, err2) diff --git a/src/core/filter/security_test.go b/src/core/filter/security_test.go index 5c23dd7ec..9de2ec698 100644 --- a/src/core/filter/security_test.go +++ b/src/core/filter/security_test.go @@ -328,7 +328,7 @@ func TestSessionReqCtxModifier(t *testing.T) { Username: "admin", UserID: 1, Email: "admin@example.com", - HasAdminRole: true, + SysAdminFlag: true, } req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/api/projects/", nil) diff --git a/src/testing/apitests/apilib/user.go b/src/testing/apitests/apilib/user.go index 62a93aae0..224b8de90 100644 --- a/src/testing/apitests/apilib/user.go +++ b/src/testing/apitests/apilib/user.go @@ -43,7 +43,7 @@ type User struct { RoleId int32 `json:"role_id,omitempty"` - HasAdminRole bool `json:"has_admin_role,omitempty"` + HasAdminRole bool `json:"sysadmin_flag,omitempty"` ResetUuid string `json:"reset_uuid,omitempty"` diff --git a/tests/apitests/python/library/user.py b/tests/apitests/python/library/user.py index 7b7a9181b..e577b4780 100644 --- a/tests/apitests/python/library/user.py +++ b/tests/apitests/python/library/user.py @@ -79,8 +79,8 @@ class User(base.Base): def update_user_role_as_sysadmin(self, user_id, IsAdmin, **kwargs): client = self._get_client(**kwargs) - has_admin_role = swagger_client.HasAdminRole(IsAdmin) - print "has_admin_role:", has_admin_role - _, status_code, _ = client.users_user_id_sysadmin_put_with_http_info(user_id, has_admin_role) + sysadmin_flag = swagger_client.SysAdminFlag(IsAdmin) + print "sysadmin_flag:", sysadmin_flag + _, status_code, _ = client.users_user_id_sysadmin_put_with_http_info(user_id, sysadmin_flag) base._assert_status_code(200, status_code) return user_id diff --git a/tests/apitests/python/test_ldap_admin_role.py b/tests/apitests/python/test_ldap_admin_role.py index 681bea40b..869b3ac59 100644 --- a/tests/apitests/python/test_ldap_admin_role.py +++ b/tests/apitests/python/test_ldap_admin_role.py @@ -56,6 +56,12 @@ class TestLdapAdminRole(unittest.TestCase): projects = self.mike_product_api.projects_get(name="test_private") self.assertTrue(projects.count>1) self.project_id = projects[0].project_id + + # check the mike is not admin in Database + user_list = self.product_api.users_get(username="mike") + pprint(user_list[0]) + self.assertFalse(user_list[0].sysadmin_flag) + pass diff --git a/tests/robot-cases/Group3-Upgrade/prepare.py b/tests/robot-cases/Group3-Upgrade/prepare.py index 852e1cb4d..98fed911d 100644 --- a/tests/robot-cases/Group3-Upgrade/prepare.py +++ b/tests/robot-cases/Group3-Upgrade/prepare.py @@ -29,9 +29,9 @@ class HarborAPI: r = request(url+"users?username="+user+"", 'get') userid = str(r.json()[0]['user_id']) if args.version == "1.6": - body=dict(body={"has_admin_role": True}) + body=dict(body={"sysadmin_flag": True}) else: - body=dict(body={"has_admin_role": 1}) + body=dict(body={"sysadmin_flag": 1}) request(url+"users/"+userid+"/sysadmin", 'put', **body) def add_member(self, project, user, role): diff --git a/tests/robot-cases/Group3-Upgrade/prepare_v17.py b/tests/robot-cases/Group3-Upgrade/prepare_v17.py index b694b994b..429f9817a 100644 --- a/tests/robot-cases/Group3-Upgrade/prepare_v17.py +++ b/tests/robot-cases/Group3-Upgrade/prepare_v17.py @@ -28,9 +28,9 @@ class HarborAPI: r = request(url+"users?username="+user+"", 'get') userid = str(r.json()[0]['user_id']) if args.version == "1.6": - body=dict(body={"has_admin_role": True}) + body=dict(body={"sysadmin_flag": True}) else: - body=dict(body={"has_admin_role": 1}) + body=dict(body={"sysadmin_flag": 1}) request(url+"users/"+userid+"/sysadmin", 'put', **body) def add_member(self, project, user, role):