From 224f75b9a6565369cb0ed1b134432c814b55071b Mon Sep 17 00:00:00 2001 From: Tan Jiang Date: Thu, 14 Dec 2017 17:21:29 +0800 Subject: [PATCH] Refactor /users API, add more restircation in password reset Simplified the code when checking if a user is modiable in different auth modes. Also add restriction in password, such that when the auth mode is not DB auth, only the super user can choose to reset his password. --- src/common/dao/dao_test.go | 62 ++++++++++----------- src/common/dao/user.go | 5 ++ src/ui/api/user.go | 60 ++++++++------------- src/ui/api/user_test.go | 51 ++++++++++++++++++ src/ui/auth/authenticator.go | 9 ---- src/ui/config/config.go | 7 ++- src/ui/controllers/base.go | 37 +++++++++++-- src/ui/controllers/controllers_test.go | 74 ++++++++++++++++++++++---- src/ui/router.go | 2 +- 9 files changed, 212 insertions(+), 95 deletions(-) diff --git a/src/common/dao/dao_test.go b/src/common/dao/dao_test.go index bb0c2ce20..7f03b73c1 100644 --- a/src/common/dao/dao_test.go +++ b/src/common/dao/dao_test.go @@ -20,8 +20,8 @@ import ( "time" "github.com/astaxie/beego/orm" - "github.com/vmware/harbor/src/common" "github.com/stretchr/testify/assert" + "github.com/vmware/harbor/src/common" "github.com/vmware/harbor/src/common/models" "github.com/vmware/harbor/src/common/utils" "github.com/vmware/harbor/src/common/utils/log" @@ -305,6 +305,7 @@ var currentUser *models.User func TestGetUser(t *testing.T) { queryUser := models.User{ Username: username, + Email: "tester01@vmware.com", } var err error currentUser, err = GetUser(queryUser) @@ -1632,36 +1633,35 @@ func TestGetScanJobsByStatus(t *testing.T) { assert.Equal(sj1.Repository, r2[0].Repository) } - -func TestSaveConfigEntries(t *testing.T) { - configEntries :=[]models.ConfigEntry{ +func TestSaveConfigEntries(t *testing.T) { + configEntries := []models.ConfigEntry{ { - Key:"teststringkey", - Value:"192.168.111.211", + Key: "teststringkey", + Value: "192.168.111.211", }, { - Key:"testboolkey", - Value:"true", + Key: "testboolkey", + Value: "true", }, { - Key:"testnumberkey", - Value:"5", + Key: "testnumberkey", + Value: "5", }, { - Key:common.CfgDriverDB, - Value:"db", + Key: common.CfgDriverDB, + Value: "db", }, } err := SaveConfigEntries(configEntries) if err != nil { t.Fatalf("failed to save configuration to database %v", err) } - readEntries, err:=GetConfigEntries() - if err !=nil { + readEntries, err := GetConfigEntries() + if err != nil { t.Fatalf("Failed to get configuration from database %v", err) } - findItem:=0 - for _,entry:= range readEntries{ + findItem := 0 + for _, entry := range readEntries { switch entry.Key { case "teststringkey": if "192.168.111.211" == entry.Value { @@ -1678,38 +1678,38 @@ func TestSaveConfigEntries(t *testing.T) { default: } } - if findItem !=3 { + if findItem != 3 { t.Fatalf("Should update 3 configuration but only update %d", findItem) } - configEntries =[]models.ConfigEntry{ + configEntries = []models.ConfigEntry{ { - Key:"teststringkey", - Value:"192.168.111.215", + Key: "teststringkey", + Value: "192.168.111.215", }, { - Key:"testboolkey", - Value:"false", + Key: "testboolkey", + Value: "false", }, { - Key:"testnumberkey", - Value:"7", + Key: "testnumberkey", + Value: "7", }, { - Key:common.CfgDriverDB, - Value:"db", + Key: common.CfgDriverDB, + Value: "db", }, } err = SaveConfigEntries(configEntries) if err != nil { t.Fatalf("failed to save configuration to database %v", err) } - readEntries, err=GetConfigEntries() - if err !=nil { + readEntries, err = GetConfigEntries() + if err != nil { t.Fatalf("Failed to get configuration from database %v", err) } - findItem=0 - for _,entry:= range readEntries{ + findItem = 0 + for _, entry := range readEntries { switch entry.Key { case "teststringkey": if "192.168.111.215" == entry.Value { @@ -1726,7 +1726,7 @@ func TestSaveConfigEntries(t *testing.T) { default: } } - if findItem !=3 { + if findItem != 3 { t.Fatalf("Should update 3 configuration but only update %d", findItem) } diff --git a/src/common/dao/user.go b/src/common/dao/user.go index 0f11706b3..17ed5e37f 100644 --- a/src/common/dao/user.go +++ b/src/common/dao/user.go @@ -52,6 +52,11 @@ func GetUser(query models.User) (*models.User, error) { queryParam = append(queryParam, query.ResetUUID) } + if query.Email != "" { + sql += ` and email = ? ` + queryParam = append(queryParam, query.Email) + } + var u []models.User n, err := o.Raw(sql, queryParam).QueryRows(&u) diff --git a/src/ui/api/user.go b/src/ui/api/user.go index d96b02dbd..b43de5a4e 100644 --- a/src/ui/api/user.go +++ b/src/ui/api/user.go @@ -21,6 +21,7 @@ import ( "strconv" "strings" + "github.com/vmware/harbor/src/common" "github.com/vmware/harbor/src/common/dao" "github.com/vmware/harbor/src/common/models" "github.com/vmware/harbor/src/common/utils/log" @@ -160,16 +161,9 @@ func (ua *UserAPI) List() { // Put ... func (ua *UserAPI) Put() { - ldapAdminUser := (ua.AuthMode == "ldap_auth" && ua.userID == 1 && ua.userID == ua.currentUserID) - - if !(ua.AuthMode == "db_auth" || ldapAdminUser) { - ua.CustomAbort(http.StatusForbidden, "") - } - if !ua.IsAdmin { - if ua.userID != ua.currentUserID { - log.Warning("Guests can only change their own account.") - ua.CustomAbort(http.StatusForbidden, "Guests can only change their own account.") - } + if !ua.modifiable() { + ua.RenderError(http.StatusForbidden, fmt.Sprintf("User with ID %d cannot be modified", ua.userID)) + return } user := models.User{UserID: ua.userID} ua.DecodeJSONReq(&user) @@ -210,7 +204,7 @@ func (ua *UserAPI) Put() { // Post ... func (ua *UserAPI) Post() { - if !(ua.AuthMode == "db_auth") { + if !(ua.AuthMode == common.DBAuth) { ua.CustomAbort(http.StatusForbidden, "") } @@ -258,22 +252,8 @@ func (ua *UserAPI) Post() { // Delete ... func (ua *UserAPI) Delete() { - if !ua.IsAdmin { - log.Warningf("current user, id: %d does not have admin role, can not remove user", ua.currentUserID) - ua.RenderError(http.StatusForbidden, "User does not have admin role") - return - } - - if ua.AuthMode == "ldap_auth" { - ua.CustomAbort(http.StatusForbidden, "user can not be deleted in LDAP authentication mode") - } - - if ua.currentUserID == ua.userID { - ua.CustomAbort(http.StatusForbidden, "can not delete yourself") - } - - if ua.userID == 1 { - ua.HandleForbidden(ua.SecurityCtx.GetUsername()) + if !ua.IsAdmin || ua.AuthMode != common.DBAuth || ua.userID == 1 || ua.currentUserID == ua.userID { + ua.RenderError(http.StatusForbidden, fmt.Sprintf("User with ID: %d cannot be removed, auth mode: %s, current user ID: %d", ua.userID, ua.AuthMode, ua.currentUserID)) return } @@ -288,17 +268,9 @@ func (ua *UserAPI) Delete() { // ChangePassword handles PUT to /api/users/{}/password func (ua *UserAPI) ChangePassword() { - ldapAdminUser := (ua.AuthMode == "ldap_auth" && ua.userID == 1 && ua.userID == ua.currentUserID) - - if !(ua.AuthMode == "db_auth" || ldapAdminUser) { - ua.CustomAbort(http.StatusForbidden, "") - } - - if !ua.IsAdmin { - if ua.userID != ua.currentUserID { - log.Error("Guests can only change their own account.") - ua.CustomAbort(http.StatusForbidden, "Guests can only change their own account.") - } + if !ua.modifiable() { + ua.RenderError(http.StatusForbidden, fmt.Sprintf("User with ID: %d is not modifiable", ua.userID)) + return } var req passwordReq @@ -345,6 +317,18 @@ func (ua *UserAPI) ToggleUserAdminRole() { } } +// modifiable returns whether the modify is allowed based on current auth mode and context +func (ua *UserAPI) modifiable() bool { + if ua.AuthMode == common.DBAuth { + //When the auth mode is local DB, admin can modify anyone, non-admin can modify himself. + return ua.IsAdmin || ua.userID == ua.currentUserID + } + //When the auth mode is external IDM backend, only the super user can modify himself, + //because he's the only one whose information is stored in local DB. + return ua.userID == 1 && ua.userID == ua.currentUserID + +} + // validate only validate when user register func validate(user models.User) error { diff --git a/src/ui/api/user_test.go b/src/ui/api/user_test.go index 2c74a177f..089e786b2 100644 --- a/src/ui/api/user_test.go +++ b/src/ui/api/user_test.go @@ -16,8 +16,11 @@ package api import ( "fmt" "github.com/stretchr/testify/assert" + "github.com/vmware/harbor/src/common/api" "github.com/vmware/harbor/tests/apitests/apilib" "testing" + + "github.com/astaxie/beego" ) var testUser0002ID, testUser0003ID int @@ -430,3 +433,51 @@ func TestUsersDelete(t *testing.T) { assert.Equal(200, code, "Delete test user status should be 200") } } + +func TestModifiable(t *testing.T) { + assert := assert.New(t) + base := BaseController{ + api.BaseAPI{ + beego.Controller{}, + }, + nil, + nil, + } + + ua1 := &UserAPI{ + base, + 3, + 4, + false, + false, + "db_auth", + } + assert.False(ua1.modifiable()) + ua2 := &UserAPI{ + base, + 3, + 4, + false, + true, + "db_auth", + } + assert.True(ua2.modifiable()) + ua3 := &UserAPI{ + base, + 3, + 4, + false, + true, + "ldap_auth", + } + assert.False(ua3.modifiable()) + ua4 := &UserAPI{ + base, + 1, + 1, + false, + true, + "ldap_auth", + } + assert.True(ua4.modifiable()) +} diff --git a/src/ui/auth/authenticator.go b/src/ui/auth/authenticator.go index 6124e3aa1..d58fa4ac7 100644 --- a/src/ui/auth/authenticator.go +++ b/src/ui/auth/authenticator.go @@ -26,15 +26,6 @@ import ( // 1.5 seconds const frozenTime time.Duration = 1500 * time.Millisecond -const ( - // DBAuth is the database auth mode. - DBAuth = "db_auth" - // LDAPAuth is the ldap auth mode. - LDAPAuth = "ldap_auth" - // UAAAuth is the uaa auth mode. - UAAAuth = "uaa_auth" -) - var lock = NewUserLock(frozenTime) // AuthenticateHelper provides interface for user management in different auth modes. diff --git a/src/ui/config/config.go b/src/ui/config/config.go index 1609e82d3..1fdb34c13 100644 --- a/src/ui/config/config.go +++ b/src/ui/config/config.go @@ -61,12 +61,17 @@ var ( func Init() error { //init key provider initKeyProvider() - adminServerURL := os.Getenv("ADMINSERVER_URL") if len(adminServerURL) == 0 { adminServerURL = "http://adminserver" } + return InitByURL(adminServerURL) + +} + +// InitByURL Init configurations with given url +func InitByURL(adminServerURL string) error { log.Infof("initializing client for adminserver %s ...", adminServerURL) authorizer := auth.NewSecretAuthorizer(secretCookieName, UISecret()) AdminserverClient = client.NewClient(adminServerURL, authorizer) diff --git a/src/ui/controllers/base.go b/src/ui/controllers/base.go index bac64b5a4..9e67d24af 100644 --- a/src/ui/controllers/base.go +++ b/src/ui/controllers/base.go @@ -25,6 +25,7 @@ import ( "github.com/astaxie/beego" "github.com/beego/i18n" + "github.com/vmware/harbor/src/common" "github.com/vmware/harbor/src/common/dao" "github.com/vmware/harbor/src/common/models" "github.com/vmware/harbor/src/common/utils" @@ -101,8 +102,8 @@ func (cc *CommonController) UserExists() { cc.ServeJSON() } -// SendEmail verifies the Email address and contact SMTP server to send reset password Email. -func (cc *CommonController) SendEmail() { +// SendResetEmail verifies the Email address and contact SMTP server to send reset password Email. +func (cc *CommonController) SendResetEmail() { email := cc.GetString("email") @@ -117,16 +118,21 @@ func (cc *CommonController) SendEmail() { } queryUser := models.User{Email: email} - exist, err := dao.UserExists(queryUser, "email") + u, err := dao.GetUser(queryUser) if err != nil { - log.Errorf("Error occurred in UserExists: %v", err) + log.Errorf("Error occurred in GetUser: %v", err) cc.CustomAbort(http.StatusInternalServerError, "Internal error.") } - if !exist { + if u == nil { log.Debugf("email %s not found", email) cc.CustomAbort(http.StatusNotFound, "email_does_not_exist") } + if !isUserResetable(u) { + log.Errorf("Resetting password for user with ID: %d is not allowed", u.UserID) + cc.CustomAbort(http.StatusForbidden, http.StatusText(http.StatusForbidden)) + } + uuid := utils.GenerateRandomString() user := models.User{ResetUUID: uuid, Email: email} if err = dao.UpdateUserResetUUID(user); err != nil { @@ -192,6 +198,7 @@ func (cc *CommonController) ResetPassword() { queryUser := models.User{ResetUUID: resetUUID} user, err := dao.GetUser(queryUser) + if err != nil { log.Errorf("Error occurred in GetUser: %v", err) cc.CustomAbort(http.StatusInternalServerError, "Internal error.") @@ -201,6 +208,11 @@ func (cc *CommonController) ResetPassword() { cc.CustomAbort(http.StatusBadRequest, "User does not exist") } + if !isUserResetable(user) { + log.Errorf("Resetting password for user with ID: %d is not allowed", user.UserID) + cc.CustomAbort(http.StatusForbidden, http.StatusText(http.StatusForbidden)) + } + password := cc.GetString("password") if password != "" { @@ -215,6 +227,21 @@ func (cc *CommonController) ResetPassword() { } } +func isUserResetable(u *models.User) bool { + if u == nil { + return false + } + mode, err := config.AuthMode() + if err != nil { + log.Errorf("Failed to get the auth mode, error: %v", err) + return false + } + if mode == common.DBAuth { + return true + } + return u.UserID == 1 +} + func init() { //conf/app.conf -> os.Getenv("config_path") configPath := os.Getenv("CONFIG_PATH") diff --git a/src/ui/controllers/controllers_test.go b/src/ui/controllers/controllers_test.go index c5231560f..f8bb7ec7f 100644 --- a/src/ui/controllers/controllers_test.go +++ b/src/ui/controllers/controllers_test.go @@ -22,11 +22,14 @@ import ( "testing" "fmt" + "os" "strings" "github.com/astaxie/beego" "github.com/stretchr/testify/assert" - "github.com/vmware/harbor/src/common/utils/log" + "github.com/vmware/harbor/src/common" + "github.com/vmware/harbor/src/common/models" + "github.com/vmware/harbor/src/common/utils/test" "github.com/vmware/harbor/src/ui/config" "github.com/vmware/harbor/src/ui/proxy" ) @@ -44,14 +47,6 @@ import ( //var admin *usrInfo func init() { - if err := config.Init(); err != nil { - log.Fatalf("failed to initialize configurations: %v", err) - } - - if err := proxy.Init(); err != nil { - log.Fatalf("Failed to initialize the proxy: %v", err) - } - _, file, _, _ := runtime.Caller(1) apppath, _ := filepath.Abs(filepath.Dir(filepath.Join(file, ".."+string(filepath.Separator)))) beego.BConfig.WebConfig.Session.SessionOn = true @@ -64,15 +59,74 @@ func init() { beego.Router("/log_out", &CommonController{}, "get:LogOut") beego.Router("/reset", &CommonController{}, "post:ResetPassword") beego.Router("/userExists", &CommonController{}, "post:UserExists") - beego.Router("/sendEmail", &CommonController{}, "get:SendEmail") + beego.Router("/sendEmail", &CommonController{}, "get:SendResetEmail") beego.Router("/registryproxy/*", &RegistryProxy{}, "*:Handle") +} +func TestMain(m *testing.M) { + + rc := m.Run() + if rc != 0 { + os.Exit(rc) + } //Init user Info //admin = &usrInfo{adminName, adminPwd} } +// TestUserResettable +func TestUserResettable(t *testing.T) { + assert := assert.New(t) + DBAuthConfig := map[string]interface{}{ + common.AUTHMode: common.DBAuth, + common.CfgExpiration: 5, + common.TokenExpiration: 30, + } + + LDAPAuthConfig := map[string]interface{}{ + common.AUTHMode: common.LDAPAuth, + common.CfgExpiration: 5, + common.TokenExpiration: 30, + } + DBAuthAdminsvr, err := test.NewAdminserver(DBAuthConfig) + if err != nil { + panic(err) + } + LDAPAuthAdminsvr, err := test.NewAdminserver(LDAPAuthConfig) + if err != nil { + panic(err) + } + defer DBAuthAdminsvr.Close() + defer LDAPAuthAdminsvr.Close() + if err := config.InitByURL(LDAPAuthAdminsvr.URL); err != nil { + panic(err) + } + u1 := &models.User{ + UserID: 3, + Username: "daniel", + Email: "daniel@test.com", + } + u2 := &models.User{ + UserID: 1, + Username: "jack", + Email: "jack@test.com", + } + assert.False(isUserResetable(u1)) + assert.True(isUserResetable(u2)) + if err := config.InitByURL(DBAuthAdminsvr.URL); err != nil { + panic(err) + } + assert.True(isUserResetable(u1)) +} + // TestMain is a sample to run an endpoint test func TestAll(t *testing.T) { + if err := config.Init(); err != nil { + panic(err) + } + if err := proxy.Init(); err != nil { + panic(err) + } + assert := assert.New(t) // v := url.Values{} diff --git a/src/ui/router.go b/src/ui/router.go index 2c2078e9d..105226f65 100644 --- a/src/ui/router.go +++ b/src/ui/router.go @@ -65,7 +65,7 @@ func initRouters() { beego.Router("/log_out", &controllers.CommonController{}, "get:LogOut") beego.Router("/reset", &controllers.CommonController{}, "post:ResetPassword") beego.Router("/userExists", &controllers.CommonController{}, "post:UserExists") - beego.Router("/sendEmail", &controllers.CommonController{}, "get:SendEmail") + beego.Router("/sendEmail", &controllers.CommonController{}, "get:SendResetEmail") //API: beego.Router("/api/projects/:pid([0-9]+)/members/?:mid", &api.ProjectMemberAPI{})