Commit 6313a552 authored by stonezdj's avatar stonezdj
Browse files

Fix admin permission not revoked when removed from LDAP admin group



Seperate the HasAdminRole(In DB) with the privileges from external auth, and use user.HasAdminPrivilege to check
Signed-off-by: default avatarstonezdj <stonezdj@gmail.com>
parent efa8ff16
......@@ -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:
......
......@@ -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)
}
......
......@@ -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
}
......
......@@ -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()
......
......@@ -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
}
......
......@@ -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 ...
......
......@@ -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 ...
......
......@@ -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))
}
......
......@@ -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})
......
......@@ -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
......
......@@ -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}$`)
......
......@@ -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!")
}
}
......
......@@ -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)
......
......@@ -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)
......
......@@ -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"`
......
......@@ -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
......@@ -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
......
......@@ -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):
......
......@@ -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):
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment