This is an automated email from the ASF dual-hosted git repository.
humingcheng pushed a commit to branch dev
in repository https://gitbox.apache.org/repos/asf/servicecomb-service-center.git
The following commit(s) were added to refs/heads/dev by this push:
new 7df8f70f support admin reset self's password without current password
(#1506)
7df8f70f is described below
commit 7df8f70ff076fce5381bb31d0bc390252d1fdcb6
Author: Wanghb1 <[email protected]>
AuthorDate: Tue Jun 24 10:11:07 2025 +0800
support admin reset self's password without current password (#1506)
fix the flaw that admin can change other admins' password
---
go.mod | 2 +-
go.sum | 6 ++--
server/service/rbac/account_service_test.go | 45 +++++++++++------------
server/service/rbac/password.go | 34 +++++++++++-------
server/service/rbac/rbac_test.go | 56 +++++++++++++++++++----------
5 files changed, 87 insertions(+), 56 deletions(-)
diff --git a/go.mod b/go.mod
index b74e81f7..a9d45cad 100644
--- a/go.mod
+++ b/go.mod
@@ -21,7 +21,7 @@ require (
github.com/cloudflare/gokey v0.1.2
github.com/deckarep/golang-set v1.8.0
github.com/elithrar/simple-scrypt v1.3.0
- github.com/go-chassis/cari v0.9.1-0.20250206085436-b02623c62de2
+ github.com/go-chassis/cari v0.9.1-0.20250623123416-20ac4b73bdff
github.com/go-chassis/etcdadpt v0.5.3-0.20240328092602-984e34b756fe
github.com/go-chassis/foundation v0.4.0
github.com/go-chassis/go-archaius v1.5.6
diff --git a/go.sum b/go.sum
index 807c82d2..bc1622bd 100644
--- a/go.sum
+++ b/go.sum
@@ -99,7 +99,7 @@ github.com/armon/go-metrics v0.3.10
h1:FR+drcQStOe+32sYyJYyZ7FIdgoGGBnwLl+flodp8
github.com/armon/go-metrics v0.3.10/go.mod
h1:4O98XIr/9W0sxpJ8UaYkvjk10Iff7SnFrb4QAOwNTFc=
github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod
h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=
github.com/armon/go-radix v1.0.0
h1:F4z6KzEeeQIMeLFa97iZU6vupzoecKdU5TX24SNppXI=
-github.com/armon/go-radix v1.0.0/go.mod
h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=
+github.com/armon/go-radix v1.0.0/go.mod
h1:TsTFsXBVHVK4HQ+UrFSsQEhBXZGCDqoY+cr+sUq5ZmA=
github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a
h1:idn718Q4B6AGu/h5Sxe66HYVdqdGu2l9Iebqhi/AEoA=
github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod
h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
github.com/aws/aws-sdk-go v1.34.28
h1:sscPpn/Ns3i0F4HPEWAVcwdIRaZZCuL7llJ2/60yPIk=
@@ -235,8 +235,8 @@ github.com/go-chassis/cari v0.4.0/go.mod
h1:av/19fqwEP4eOC8unL/z67AAbFDwXUCko6SK
github.com/go-chassis/cari v0.5.0/go.mod
h1:av/19fqwEP4eOC8unL/z67AAbFDwXUCko6SKa4Avrd8=
github.com/go-chassis/cari v0.5.1-0.20210823023004-74041d1363c4/go.mod
h1:av/19fqwEP4eOC8unL/z67AAbFDwXUCko6SKa4Avrd8=
github.com/go-chassis/cari v0.6.0/go.mod
h1:mSDRCOQXGmlD69A6NG0hsv0UP1xbVPtL6HCGI6X1tqs=
-github.com/go-chassis/cari v0.9.1-0.20250206085436-b02623c62de2
h1:XCav5UOU6Vu9G45EIaaAuhqb4zWAGFP3B/fzxo9TBwQ=
-github.com/go-chassis/cari v0.9.1-0.20250206085436-b02623c62de2/go.mod
h1:ibqLyh+Q+1n9PlldW3glD9G+2s/yeSyVMCCkQWKRwuE=
+github.com/go-chassis/cari v0.9.1-0.20250623123416-20ac4b73bdff
h1:UOYq7xuNK460huHGivaD6maJvALuMmv+seVvQ81wRmA=
+github.com/go-chassis/cari v0.9.1-0.20250623123416-20ac4b73bdff/go.mod
h1:ibqLyh+Q+1n9PlldW3glD9G+2s/yeSyVMCCkQWKRwuE=
github.com/go-chassis/etcdadpt v0.5.3-0.20240328092602-984e34b756fe
h1:peLHEt3wzab6nKVcmcu0qkj1+ZXK6D1ymtiyyMBv/XA=
github.com/go-chassis/etcdadpt v0.5.3-0.20240328092602-984e34b756fe/go.mod
h1:HV8OZ1Npu+lttD+pJA5nUxWZR3/SBFetTh7w8nYFkUA=
github.com/go-chassis/foundation v0.2.2-0.20201210043510-9f6d3de40234/go.mod
h1:2PjwqpVwYEVaAldl5A58a08viH8p27pNeYaiE3ZxOBA=
diff --git a/server/service/rbac/account_service_test.go
b/server/service/rbac/account_service_test.go
index 1b902375..7d9f3bc5 100644
--- a/server/service/rbac/account_service_test.go
+++ b/server/service/rbac/account_service_test.go
@@ -22,10 +22,11 @@ import (
"context"
"testing"
- rbacsvc
"github.com/apache/servicecomb-service-center/server/service/rbac"
"github.com/go-chassis/cari/discovery"
"github.com/go-chassis/cari/pkg/errsvc"
+ rbacsvc
"github.com/apache/servicecomb-service-center/server/service/rbac"
+
_ "github.com/apache/servicecomb-service-center/test"
beego "github.com/beego/beego/v2/server/web"
@@ -42,7 +43,7 @@ const (
testPwd1 = "Ab@11111"
)
-func newAccount(name string) *rbac.Account {
+func newAdminAccount(name string) *rbac.Account {
return &rbac.Account{
Name: name,
Password: testPwd0,
@@ -54,24 +55,24 @@ func newAccount(name string) *rbac.Account {
func TestCreateAccount(t *testing.T) {
ctx := context.TODO()
t.Run("create account, should succeed", func(t *testing.T) {
- a := newAccount("TestCreateAccount_create_account")
+ a := newAdminAccount("TestCreateAccount_create_account")
err := rbacsvc.CreateAccount(ctx, a)
assert.Nil(t, err)
})
t.Run("create account twice, should return:
"+rbac.NewError(rbac.ErrAccountConflict, "").Error(), func(t *testing.T) {
name := "TestCreateAccount_create_account_twice"
- a := newAccount(name)
+ a := newAdminAccount(name)
err := rbacsvc.CreateAccount(ctx, a)
assert.Nil(t, err)
- a = newAccount(name)
+ a = newAdminAccount(name)
err = rbacsvc.CreateAccount(ctx, a)
assert.NotNil(t, err)
svcErr := err.(*errsvc.Error)
assert.Equal(t, rbac.ErrAccountConflict, svcErr.Code)
})
t.Run("account has invalid role, should return:
"+rbac.NewError(rbac.ErrAccountHasInvalidRole, "").Error(), func(t *testing.T) {
- a := newAccount("TestCreateAccount_account_has_invalid_role")
+ a :=
newAdminAccount("TestCreateAccount_account_has_invalid_role")
a.Roles = append(a.Roles, "invalid_role")
err := rbacsvc.CreateAccount(ctx, a)
assert.NotNil(t, err)
@@ -80,7 +81,7 @@ func TestCreateAccount(t *testing.T) {
})
t.Run("account has id, should succeed", func(t *testing.T) {
accountName := "TestCreateAccount_account_has_id"
- a := newAccount(accountName)
+ a := newAdminAccount(accountName)
a.ID = "specifyID"
err := rbacsvc.CreateAccount(ctx, a)
assert.NoError(t, err)
@@ -95,7 +96,7 @@ func TestCreateAccount(t *testing.T) {
func TestDeleteAccount(t *testing.T) {
t.Run("delete account, should succeed", func(t *testing.T) {
- a := newAccount("TestDeleteAccount_delete_account")
+ a := newAdminAccount("TestDeleteAccount_delete_account")
err := rbacsvc.CreateAccount(context.TODO(), a)
assert.Nil(t, err)
@@ -121,7 +122,7 @@ func TestDeleteAccount(t *testing.T) {
assert.Equal(t, rbac.ErrForbidOperateBuildInAccount,
svcErr.Code)
})
t.Run("delete self, should return:
"+rbac.NewError(rbac.ErrForbidOperateSelfAccount, "").Error(), func(t
*testing.T) {
- a := newAccount("TestDeleteAccount_delete_self")
+ a := newAdminAccount("TestDeleteAccount_delete_self")
err := rbacsvc.CreateAccount(context.TODO(), a)
assert.Nil(t, err)
claims := map[string]interface{}{
@@ -138,11 +139,11 @@ func TestDeleteAccount(t *testing.T) {
func TestUpdateAccount(t *testing.T) {
t.Run("update account, should succeed", func(t *testing.T) {
name := "TestUpdateAccount_update_account"
- a := newAccount(name)
+ a := newAdminAccount(name)
err := rbacsvc.CreateAccount(context.TODO(), a)
assert.Nil(t, err)
- a = newAccount(name)
+ a = newAdminAccount(name)
a.Roles = []string{rbac.RoleAdmin, rbac.RoleDeveloper}
err = rbacsvc.UpdateAccount(context.TODO(), a.Name, a)
assert.Nil(t, err)
@@ -152,14 +153,14 @@ func TestUpdateAccount(t *testing.T) {
})
t.Run("update no exist account, should return:
"+rbac.NewError(rbac.ErrAccountNotExist, "").Error(), func(t *testing.T) {
name := "TestUpdateAccount_update_no_exist_account"
- a := newAccount(name)
+ a := newAdminAccount(name)
err := rbacsvc.UpdateAccount(context.TODO(), a.Name, a)
assert.NotNil(t, err)
svcErr := err.(*errsvc.Error)
assert.Equal(t, rbac.ErrAccountNotExist, svcErr.Code)
})
t.Run("update root, should return:
"+discovery.NewError(rbac.ErrForbidOperateBuildInAccount, "").Error(), func(t
*testing.T) {
- a := newAccount("root")
+ a := newAdminAccount("root")
err := rbacsvc.UpdateAccount(context.TODO(), a.Name, a)
assert.NotNil(t, err)
svcErr := err.(*errsvc.Error)
@@ -167,7 +168,7 @@ func TestUpdateAccount(t *testing.T) {
})
t.Run("account has invalid role, should return:
"+rbac.NewError(rbac.ErrAccountHasInvalidRole, "").Error(), func(t *testing.T) {
name := "TestUpdateAccount_account_has_invalid_role"
- a := newAccount(name)
+ a := newAdminAccount(name)
err := rbacsvc.CreateAccount(context.TODO(), a)
assert.Nil(t, err)
@@ -179,11 +180,11 @@ func TestUpdateAccount(t *testing.T) {
})
t.Run("roles status empty both, should return:
"+discovery.NewError(discovery.ErrInvalidParams, "").Error(), func(t
*testing.T) {
name := "TestUpdateAccount_roles_status_empty_both"
- a := newAccount(name)
+ a := newAdminAccount(name)
err := rbacsvc.CreateAccount(context.TODO(), a)
assert.Nil(t, err)
- a = newAccount(name)
+ a = newAdminAccount(name)
a.Roles = nil
a.Status = ""
err = rbacsvc.UpdateAccount(context.TODO(), a.Name, a)
@@ -193,11 +194,11 @@ func TestUpdateAccount(t *testing.T) {
})
t.Run("update self, should return:
"+rbac.NewError(rbac.ErrForbidOperateSelfAccount, "").Error(), func(t
*testing.T) {
name := "TestDeleteAccount_update_self"
- a := newAccount(name)
+ a := newAdminAccount(name)
err := rbacsvc.CreateAccount(context.TODO(), a)
assert.Nil(t, err)
- a = newAccount(name)
+ a = newAdminAccount(name)
claims := map[string]interface{}{
rbac.ClaimsUser: a.Name,
}
@@ -211,7 +212,7 @@ func TestUpdateAccount(t *testing.T) {
func TestEditAccount(t *testing.T) {
t.Run("edit no exist account, should return:
"+rbac.NewError(rbac.ErrAccountNotExist, "").Error(), func(t *testing.T) {
- a := newAccount("TestEditAccount_edit_no_exist_account")
+ a := newAdminAccount("TestEditAccount_edit_no_exist_account")
err := rbacsvc.UpdateAccount(context.TODO(), a.Name, a)
assert.NotNil(t, err)
svcErr := err.(*errsvc.Error)
@@ -254,10 +255,10 @@ func TestBatchCreateAccounts(t *testing.T) {
assert.Equal(t, discovery.ErrInvalidParams, svcErr.Code)
})
t.Run("batch create accounts, should succeed", func(t *testing.T) {
- a1 := newAccount("TestBatchCreateAccounts_account_1")
- a2 := newAccount("TestBatchCreateAccounts_account_no_pwd")
+ a1 := newAdminAccount("TestBatchCreateAccounts_account_1")
+ a2 := newAdminAccount("TestBatchCreateAccounts_account_no_pwd")
a2.Password = ""
- a3 := newAccount("TestBatchCreateAccounts_account_invalid_pwd")
+ a3 :=
newAdminAccount("TestBatchCreateAccounts_account_invalid_pwd")
a3.Password = "1"
defer func() {
diff --git a/server/service/rbac/password.go b/server/service/rbac/password.go
index 9631bab9..54373cde 100644
--- a/server/service/rbac/password.go
+++ b/server/service/rbac/password.go
@@ -21,12 +21,13 @@ import (
"context"
"fmt"
+ "github.com/go-chassis/cari/discovery"
+ "github.com/go-chassis/cari/rbac"
+
"github.com/apache/servicecomb-service-center/pkg/log"
"github.com/apache/servicecomb-service-center/pkg/privacy"
"github.com/apache/servicecomb-service-center/pkg/util"
"github.com/apache/servicecomb-service-center/server/service/validator"
- "github.com/go-chassis/cari/discovery"
- "github.com/go-chassis/cari/rbac"
)
func ChangePassword(ctx context.Context, a *rbac.Account) error {
@@ -40,20 +41,29 @@ func ChangePassword(ctx context.Context, a *rbac.Account)
error {
return discovery.NewError(discovery.ErrInternal, err.Error())
}
- // change self password, need to check password mismatch
- if changer.Name == a.Name {
- return changePassword(ctx, a.Name, a.CurrentPassword,
a.Password)
+ // non-admin user can only change self
+ if !changer.HasAdminRole() {
+ if changer.Name == a.Name {
+ return changePassword(ctx, a.Name, a.CurrentPassword,
a.Password)
+ }
+ return discovery.NewError(discovery.ErrForbidden,
ErrNoPermChangeAccount.Error())
}
- // 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)
- }
+ // admin user can reset self or non-admin without password
+ if a.Name == changer.Name {
+ return changePasswordForcibly(ctx, a.Name, a.Password)
+ }
+
+ curAccount, err := GetAccount(ctx, a.Name)
+ if err != nil {
+ return discovery.NewError(discovery.ErrInternal, err.Error())
+ }
+
+ if !curAccount.HasAdminRole() {
+ return changePasswordForcibly(ctx, a.Name, a.Password)
}
- // other cases, change password is forbidden
+ // cannot change other admins' password
return discovery.NewError(discovery.ErrForbidden,
ErrNoPermChangeAccount.Error())
}
diff --git a/server/service/rbac/rbac_test.go b/server/service/rbac/rbac_test.go
index b424eaeb..3755d09b 100644
--- a/server/service/rbac/rbac_test.go
+++ b/server/service/rbac/rbac_test.go
@@ -24,9 +24,6 @@ import (
_ "github.com/apache/servicecomb-service-center/test"
- "github.com/apache/servicecomb-service-center/pkg/privacy"
- "github.com/apache/servicecomb-service-center/server/config"
- rbacsvc
"github.com/apache/servicecomb-service-center/server/service/rbac"
beego "github.com/beego/beego/v2/server/web"
"github.com/go-chassis/cari/discovery"
"github.com/go-chassis/cari/pkg/errsvc"
@@ -35,6 +32,10 @@ import (
"github.com/go-chassis/go-chassis/v2/security/authr"
"github.com/go-chassis/go-chassis/v2/security/secret"
"github.com/stretchr/testify/assert"
+
+ "github.com/apache/servicecomb-service-center/pkg/privacy"
+ "github.com/apache/servicecomb-service-center/server/config"
+ rbacsvc
"github.com/apache/servicecomb-service-center/server/service/rbac"
)
func init() {
@@ -89,15 +90,16 @@ func TestInitRBAC(t *testing.T) {
assert.NotNil(t, key)
})
- t.Run("change pwd,admin can change any one password", func(t
*testing.T) {
- accountName := "admin_change_other_pwd"
- persisted := newAccount(accountName)
+ t.Run("admin can change non-admin password", func(t *testing.T) {
+ accountName := "admin_change_non_admin_pwd"
+ persisted := newAdminAccount(accountName)
+ persisted.Roles = []string{rbac.RoleDeveloper}
err := rbacsvc.CreateAccount(ctx, persisted)
assert.NoError(t, err)
defer rbacsvc.DeleteAccount(ctx, accountName)
claims := map[string]interface{}{
- rbac.ClaimsUser: "test",
+ rbac.ClaimsUser: "test_admin",
rbac.ClaimsRoles: []interface{}{rbac.RoleAdmin},
}
ctx := context.WithValue(ctx, rbacsvc.CtxRequestClaims, claims)
@@ -107,17 +109,33 @@ 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)
{
- accountName := "admin_change_self"
- a := newAccount(accountName)
- a.Roles = []string{rbac.RoleAdmin}
+ t.Run("admin change other admin, should return:
"+discovery.NewError(discovery.ErrForbidden, "").Error(), func(t *testing.T) {
+ accountName := "test"
+ a := newAdminAccount(accountName)
+ err := rbacsvc.CreateAccount(ctx, a)
+ assert.NoError(t, err)
+ defer rbacsvc.DeleteAccount(ctx, accountName)
+
+ claims := map[string]interface{}{
+ rbac.ClaimsUser: "change_other_user_password",
+ rbac.ClaimsRoles: []interface{}{rbac.RoleAdmin},
+ }
+ ctx := context.WithValue(ctx, rbacsvc.CtxRequestClaims, claims)
+ err = rbacsvc.ChangePassword(ctx, &rbac.Account{Name: a.Name,
CurrentPassword: testPwd0, Password: testPwd1})
+ assert.True(t, errsvc.IsErrEqualCode(err,
discovery.ErrForbidden))
+ })
+
+ t.Run("non-admin change self, must provide current pwd", func(t
*testing.T) {
+ accountName := "non_admin_change_self"
+ a := newAdminAccount(accountName)
+ a.Roles = []string{rbac.RoleDeveloper}
err := rbacsvc.CreateAccount(context.TODO(), a)
assert.Nil(t, err)
defer rbacsvc.DeleteAccount(ctx, accountName)
claims := map[string]interface{}{
rbac.ClaimsUser: accountName,
- rbac.ClaimsRoles: []interface{}{rbac.RoleAdmin},
+ rbac.ClaimsRoles: []interface{}{rbac.RoleDeveloper},
}
ctx := context.WithValue(ctx, rbacsvc.CtxRequestClaims, claims)
err = rbacsvc.ChangePassword(ctx, &rbac.Account{Name: a.Name,
CurrentPassword: "", Password: testPwd1})
@@ -126,27 +144,29 @@ func TestInitRBAC(t *testing.T) {
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) {
- accountName := "change_self_pwd"
- a := newAccount(accountName)
+
+ t.Run("admin change self password may not provide current pwd", func(t
*testing.T) {
+ accountName := "admin_change_self"
+ a := newAdminAccount(accountName)
err := rbacsvc.CreateAccount(ctx, a)
assert.NoError(t, err)
defer rbacsvc.DeleteAccount(ctx, accountName)
claims := map[string]interface{}{
rbac.ClaimsUser: accountName,
- rbac.ClaimsRoles: []interface{}{rbac.RoleDeveloper},
+ rbac.ClaimsRoles: []interface{}{rbac.RoleAdmin},
}
ctx := context.WithValue(ctx, rbacsvc.CtxRequestClaims, claims)
- err = rbacsvc.ChangePassword(ctx, &rbac.Account{Name: a.Name,
CurrentPassword: testPwd0, Password: testPwd1})
+ err = rbacsvc.ChangePassword(ctx, &rbac.Account{Name: a.Name,
Password: testPwd1})
assert.NoError(t, err)
resp, err := rbacsvc.GetAccount(ctx, a.Name)
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) {
accountName := "test"
- a := newAccount(accountName)
+ a := newAdminAccount(accountName)
defer rbacsvc.DeleteAccount(ctx, accountName)
claims := map[string]interface{}{
