[servicecomb-service-center] branch master updated: change password force only when admin role change other users (#1034)

[li...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

littlecui pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/servicecomb-service-center.git


The following commit(s) were added to refs/heads/master by this push:
     new 4a5b220  change password force only when admin role change other users (#1034)
4a5b220 is described below

commit 4a5b22048f3289bcb7f37b286913ac24564a8b3f
Author: humingcheng <hu...@163.com>
AuthorDate: Wed Jun 2 14:10:50 2021 +0800

    change password force only when admin role change other users (#1034)
---
 server/service/rbac/password.go  | 25 +++++++++++++++----------
 server/service/rbac/rbac_test.go | 31 +++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/server/service/rbac/password.go b/server/service/rbac/password.go
index 61d540e..bbe72db 100644
--- a/server/service/rbac/password.go
+++ b/server/service/rbac/password.go
@@ -42,22 +42,23 @@ func ChangePassword(ctx context.Context, a *rbac.Account) error {
 		return discovery.NewError(discovery.ErrInternal, err.Error())
 	}
 
-	// authority has been checked before
-	// admin role can change any user password without supply current password
+	// change self password, need to check password mismatch
+	if changer.Name == a.Name {
+		return changePassword(ctx, a.Name, a.CurrentPassword, a.Password)
+	}
+
+	// change other user's password, only admin role can do this and no need
+	// supply current password
 	for _, r := range changer.Roles {
 		if r == rbac.RoleAdmin {
 			return changePasswordForcibly(ctx, a.Name, a.Password)
 		}
 	}
-	// change self password, or user with account authority(not admin role)
-	// change other one's password
-	// need to check password mismatch
-	if a.CurrentPassword == "" {
-		log.Error("current pwd is empty", nil)
-		return discovery.NewError(discovery.ErrInvalidParams, ErrEmptyCurrentPassword.Error())
-	}
-	return changePassword(ctx, a.Name, a.CurrentPassword, a.Password)
+
+	// other cases, change password is forbidden
+	return discovery.NewError(discovery.ErrForbidden, ErrNoPermChangeAccount.Error())
 }
+
 func changePasswordForcibly(ctx context.Context, name, pwd string) error {
 	old, err := GetAccount(ctx, name)
 	if err != nil {
@@ -67,6 +68,10 @@ func changePasswordForcibly(ctx context.Context, name, pwd string) error {
 	return doChangePassword(ctx, old, pwd)
 }
 func changePassword(ctx context.Context, name, currentPassword, pwd string) error {
+	if currentPassword == "" {
+		log.Error("current pwd is empty", nil)
+		return discovery.NewError(discovery.ErrInvalidParams, ErrEmptyCurrentPassword.Error())
+	}
 	ip := util.GetIPFromContext(ctx)
 	if IsBanned(MakeBanKey(name, ip)) {
 		log.Warnf("ip [%s] is banned, account: %s", ip, name)
diff --git a/server/service/rbac/rbac_test.go b/server/service/rbac/rbac_test.go
index 195b4c3..4deca73 100644
--- a/server/service/rbac/rbac_test.go
+++ b/server/service/rbac/rbac_test.go
@@ -24,6 +24,8 @@ import (
 	rbacsvc "github.com/apache/servicecomb-service-center/server/service/rbac"
 	_ "github.com/apache/servicecomb-service-center/test"
 	"github.com/astaxie/beego"
+	"github.com/go-chassis/cari/discovery"
+	"github.com/go-chassis/cari/pkg/errsvc"
 	"github.com/go-chassis/cari/rbac"
 	"github.com/go-chassis/go-archaius"
 	"github.com/go-chassis/go-chassis/v2/security/authr"
@@ -94,6 +96,24 @@ func TestInitRBAC(t *testing.T) {
 		assert.NoError(t, err)
 		assert.True(t, privacy.SamePassword(a.Password, "Complicated_password2"))
 	})
+	t.Run("admin change self, must provide current pwd", func(t *testing.T) {
+		name := "admin_change_self"
+		a := newAccount(name)
+		a.Roles = []string{rbac.RoleAdmin}
+		err := rbacsvc.CreateAccount(context.TODO(), a)
+		assert.Nil(t, err)
+
+		claims := map[string]interface{}{
+			rbac.ClaimsUser:  name,
+			rbac.ClaimsRoles: []interface{}{rbac.RoleAdmin},
+		}
+		ctx := context.WithValue(context.Background(), rbacsvc.CtxRequestClaims, claims)
+		err = rbacsvc.ChangePassword(ctx, &rbac.Account{Name: a.Name, CurrentPassword: "", Password: testPwd1})
+		assert.True(t, errsvc.IsErrEqualCode(err, discovery.ErrInvalidParams))
+
+		err = rbacsvc.ChangePassword(ctx, &rbac.Account{Name: a.Name, CurrentPassword: testPwd0, Password: testPwd1})
+		assert.Nil(t, err)
+	})
 	t.Run("change self password", func(t *testing.T) {
 		a := newAccount("change_self_pwd")
 		err := rbacsvc.CreateAccount(context.Background(), a)
@@ -109,7 +129,18 @@ func TestInitRBAC(t *testing.T) {
 		assert.NoError(t, err)
 		assert.True(t, privacy.SamePassword(resp.Password, testPwd1))
 	})
+	t.Run("no admin account change other user password, should return: "+discovery.NewError(discovery.ErrForbidden, "").Error(), func(t *testing.T) {
+		a := newAccount("test")
+		claims := map[string]interface{}{
+			rbac.ClaimsUser:  "change_other_user_password",
+			rbac.ClaimsRoles: []interface{}{rbac.RoleDeveloper},
+		}
+		ctx := context.WithValue(context.Background(), rbacsvc.CtxRequestClaims, claims)
+		err := rbacsvc.ChangePassword(ctx, &rbac.Account{Name: a.Name, CurrentPassword: testPwd0, Password: testPwd1})
+		assert.True(t, errsvc.IsErrEqualCode(err, discovery.ErrForbidden))
+	})
 }
+
 func BenchmarkAuthResource_Login(b *testing.B) {
 	b.RunParallel(func(pb *testing.PB) {
 		for pb.Next() {