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

@@ -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)
 	}
 

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)
 

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 {
 

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())
+}

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.

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)

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")

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{}

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{})