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

tianxiaoliang 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 44d5ac8  Add role manage error code (#1025)
44d5ac8 is described below

commit 44d5ac89f3ffe3b9226599ca7f9f5014ff6ee45b
Author: humingcheng <[email protected]>
AuthorDate: Sun May 30 21:05:44 2021 +0800

    Add role manage error code (#1025)
---
 datasource/etcd/role.go                  |   3 +
 datasource/mongo/role.go                 |   2 +-
 datasource/role.go                       |   1 +
 go.mod                                   |   2 +-
 go.sum                                   |   2 +
 pkg/errors/text.go                       |  18 ++--
 server/handler/auth/auth.go              |   6 +-
 server/resource/v4/auth_resource.go      |   4 +-
 server/resource/v4/role_resource.go      |  67 +++++--------
 server/resource/v4/role_resource_test.go |   2 +-
 server/service/rbac/dao/role_dao.go      |  89 ++++++++++++++---
 server/service/rbac/dao/role_dao_test.go | 162 +++++++++++++++++++++++++++++++
 server/service/rbac/decision.go          |  16 +--
 server/service/rbac/rbac.go              |   5 +-
 server/service/rbac/rbac_test.go         |  96 ++----------------
 server/service/rbac/role.go              |  40 +++++---
 16 files changed, 329 insertions(+), 186 deletions(-)

diff --git a/datasource/etcd/role.go b/datasource/etcd/role.go
index f8f2c47..1cd803c 100644
--- a/datasource/etcd/role.go
+++ b/datasource/etcd/role.go
@@ -88,6 +88,9 @@ func (ds *DataSource) GetRole(ctx context.Context, name 
string) (*rbac.Role, err
        if err != nil {
                return nil, err
        }
+       if resp.Count == 0 {
+               return nil, datasource.ErrRoleNotExist
+       }
        if resp.Count != 1 {
                return nil, client.ErrNotUnique
        }
diff --git a/datasource/mongo/role.go b/datasource/mongo/role.go
index db75c5d..7ab4ce6 100644
--- a/datasource/mongo/role.go
+++ b/datasource/mongo/role.go
@@ -70,7 +70,7 @@ func (ds *DataSource) GetRole(ctx context.Context, name 
string) (*rbac.Role, err
                return nil, err
        }
        if result.Err() != nil {
-               return nil, client.ErrNoDocuments
+               return nil, datasource.ErrRoleNotExist
        }
        var role rbac.Role
        err = result.Decode(&role)
diff --git a/datasource/role.go b/datasource/role.go
index 74ded86..076f6d6 100644
--- a/datasource/role.go
+++ b/datasource/role.go
@@ -27,6 +27,7 @@ import (
 var (
        ErrRoleDuplicated = errors.New("role is duplicated")
        ErrRoleCanNotEdit = errors.New("role can not be edited")
+       ErrRoleNotExist   = errors.New("role not exist")
 )
 
 // RoleManager contains the RBAC CRUD
diff --git a/go.mod b/go.mod
index bf99e82..fa01301 100644
--- a/go.mod
+++ b/go.mod
@@ -15,7 +15,7 @@ require (
        github.com/dgrijalva/jwt-go v3.2.0+incompatible
        github.com/elithrar/simple-scrypt v1.3.0
        github.com/ghodss/yaml v1.0.0
-       github.com/go-chassis/cari v0.4.1-0.20210528090900-ebd6bed3bd52
+       github.com/go-chassis/cari v0.4.1-0.20210530055018-eabe8a37291c
        github.com/go-chassis/foundation v0.3.1-0.20210513015331-b54416b66bcd
        github.com/go-chassis/go-archaius v1.5.1
        github.com/go-chassis/go-chassis/v2 v2.1.2-0.20210310004133-c9bc42149a18
diff --git a/go.sum b/go.sum
index 56318bd..71e5f9d 100644
--- a/go.sum
+++ b/go.sum
@@ -278,6 +278,8 @@ github.com/go-chassis/cari 
v0.4.1-0.20210528090900-ebd6bed3bd52 h1:ItPO3FBqT6yyW
 github.com/go-chassis/cari v0.4.1-0.20210528090900-ebd6bed3bd52/go.mod 
h1:av/19fqwEP4eOC8unL/z67AAbFDwXUCko6SKa4Avrd8=
 github.com/go-chassis/cari v0.4.1-0.20210528093055-1c7737622daf 
h1:g0Q9IJr+jrmPOzUM6sVNFybenM9xB3psOEgk7MXElTc=
 github.com/go-chassis/cari v0.4.1-0.20210528093055-1c7737622daf/go.mod 
h1:av/19fqwEP4eOC8unL/z67AAbFDwXUCko6SKa4Avrd8=
+github.com/go-chassis/cari v0.4.1-0.20210530055018-eabe8a37291c 
h1:F00R3MGfNXTZMToWEfjQ4k/uJ3Si5gpoQ3GyfcAdHnA=
+github.com/go-chassis/cari v0.4.1-0.20210530055018-eabe8a37291c/go.mod 
h1:av/19fqwEP4eOC8unL/z67AAbFDwXUCko6SKa4Avrd8=
 github.com/go-chassis/foundation v0.2.2-0.20201210043510-9f6d3de40234/go.mod 
h1:2PjwqpVwYEVaAldl5A58a08viH8p27pNeYaiE3ZxOBA=
 github.com/go-chassis/foundation v0.2.2/go.mod 
h1:2PjwqpVwYEVaAldl5A58a08viH8p27pNeYaiE3ZxOBA=
 github.com/go-chassis/foundation v0.3.0/go.mod 
h1:2PjwqpVwYEVaAldl5A58a08viH8p27pNeYaiE3ZxOBA=
diff --git a/pkg/errors/text.go b/pkg/errors/text.go
index 5b7e5ea..39b50e2 100644
--- a/pkg/errors/text.go
+++ b/pkg/errors/text.go
@@ -20,13 +20,13 @@ package errors
 const (
        MsgJSON = "json is invalid"
 
-       MsgOperateAccountFailed = "operate account failed"
-       MsgCantOperateRoot      = "root can not be operated"
-       MsgCantOperateRole      = "build-in role can not be operated"
-       MsgCantOperateYour      = "your account can not be operated"
-       MsgOperateRoleFailed    = "operate role failed"
-       MsgGetAccountFailed     = "get account failed"
-       MsgGetRoleFailed        = "get role failed"
-       MsgRolePerm             = "check role permissions failed"
-       MsgNoPerm               = "no permission to operate"
+       MsgOperateAccountFailed   = "operate account failed"
+       MsgCantOperateRoot        = "root can not be operated"
+       MsgCantOperateBuildInRole = "build-in role can not be operated"
+       MsgCantOperateYour        = "your account can not be operated"
+       MsgOperateRoleFailed      = "operate role failed"
+       MsgGetAccountFailed       = "get account failed"
+       MsgGetRoleFailed          = "get role failed"
+       MsgRolePerm               = "check role permissions failed"
+       MsgNoPerm                 = "no permission to operate"
 )
diff --git a/server/handler/auth/auth.go b/server/handler/auth/auth.go
index b67c465..641124b 100644
--- a/server/handler/auth/auth.go
+++ b/server/handler/auth/auth.go
@@ -18,6 +18,8 @@
 package auth
 
 import (
+       "net/http"
+
        "github.com/apache/servicecomb-service-center/pkg/chain"
        "github.com/apache/servicecomb-service-center/pkg/log"
        "github.com/apache/servicecomb-service-center/pkg/rest"
@@ -25,7 +27,7 @@ import (
        "github.com/apache/servicecomb-service-center/server/plugin/auth"
        "github.com/apache/servicecomb-service-center/server/response"
        "github.com/go-chassis/cari/discovery"
-       "net/http"
+       "github.com/go-chassis/cari/rbac"
 )
 
 const (
@@ -43,7 +45,7 @@ func (h *Handler) Handle(i *chain.Invocation) {
 
        if err := auth.Identify(r); err != nil {
                log.Errorf(err, "authenticate request failed, %s %s", r.Method, 
r.RequestURI)
-               i.Fail(discovery.NewError(discovery.ErrUnauthorized, 
err.Error()))
+               i.Fail(discovery.NewError(rbac.ErrUnauthorized, err.Error()))
                return
        }
 
diff --git a/server/resource/v4/auth_resource.go 
b/server/resource/v4/auth_resource.go
index 98a5ca4..63536f4 100644
--- a/server/resource/v4/auth_resource.go
+++ b/server/resource/v4/auth_resource.go
@@ -80,7 +80,7 @@ func (ar *AuthResource) CreateAccount(w http.ResponseWriter, 
req *http.Request)
        err = dao.CreateAccount(context.TODO(), a)
        if err != nil {
                if err == datasource.ErrAccountDuplicated {
-                       rest.WriteError(w, discovery.ErrConflictAccount, "")
+                       rest.WriteError(w, rbac.ErrAccountConflict, "")
                        return
                }
                log.Error(errorsEx.MsgOperateAccountFailed, err)
@@ -254,7 +254,7 @@ func (ar *AuthResource) Login(w http.ResponseWriter, r 
*http.Request) {
                if err == rbacsvc.ErrUnauthorized {
                        log.Error("not authorized", err)
                        rbacsvc.CountFailure(MakeBanKey(a.Name, ip))
-                       rest.WriteError(w, discovery.ErrUnauthorized, 
err.Error())
+                       rest.WriteError(w, rbac.ErrUnauthorized, err.Error())
                        return
                }
                log.Error("can not sign token", err)
diff --git a/server/resource/v4/role_resource.go 
b/server/resource/v4/role_resource.go
index 417b690..1a6b089 100644
--- a/server/resource/v4/role_resource.go
+++ b/server/resource/v4/role_resource.go
@@ -20,11 +20,9 @@ package v4
 import (
        "context"
        "encoding/json"
-       "github.com/apache/servicecomb-service-center/server/service/validator"
        "io/ioutil"
        "net/http"
 
-       "github.com/apache/servicecomb-service-center/datasource"
        errorsEx "github.com/apache/servicecomb-service-center/pkg/errors"
        "github.com/apache/servicecomb-service-center/pkg/log"
        "github.com/apache/servicecomb-service-center/pkg/rest"
@@ -42,16 +40,16 @@ type RoleResource struct {
 //URLPatterns define http pattern
 func (rr *RoleResource) URLPatterns() []rest.Route {
        return []rest.Route{
-               {Method: http.MethodGet, Path: "/v4/roles", Func: 
rr.GetRolePermission},
-               {Method: http.MethodPost, Path: "/v4/roles", Func: 
rr.CreateRolePermission},
-               {Method: http.MethodPut, Path: "/v4/roles/:roleName", Func: 
rr.UpdateRolePermission},
+               {Method: http.MethodGet, Path: "/v4/roles", Func: rr.ListRoles},
+               {Method: http.MethodPost, Path: "/v4/roles", Func: 
rr.CreateRole},
+               {Method: http.MethodPut, Path: "/v4/roles/:roleName", Func: 
rr.UpdateRole},
                {Method: http.MethodGet, Path: "/v4/roles/:roleName", Func: 
rr.GetRole},
                {Method: http.MethodDelete, Path: "/v4/roles/:roleName", Func: 
rr.DeleteRole},
        }
 }
 
-//GetRolePermission list all roles and there's permissions
-func (rr *RoleResource) GetRolePermission(w http.ResponseWriter, req 
*http.Request) {
+//ListRoles list all roles and there's permissions
+func (rr *RoleResource) ListRoles(w http.ResponseWriter, req *http.Request) {
        rs, num, err := dao.ListRole(context.TODO())
        if err != nil {
                log.Error(errorsEx.MsgGetRoleFailed, err)
@@ -77,8 +75,8 @@ func (rr *RoleResource) roleParse(body []byte) (*rbac.Role, 
error) {
        return role, nil
 }
 
-//CreateRolePermission create new role and assign permissions
-func (rr *RoleResource) CreateRolePermission(w http.ResponseWriter, req 
*http.Request) {
+//CreateRole create new role and assign permissions
+func (rr *RoleResource) CreateRole(w http.ResponseWriter, req *http.Request) {
        body, err := ioutil.ReadAll(req.Body)
        if err != nil {
                log.Error("read body err", err)
@@ -90,31 +88,19 @@ func (rr *RoleResource) CreateRolePermission(w 
http.ResponseWriter, req *http.Re
                rest.WriteError(w, discovery.ErrInvalidParams, errorsEx.MsgJSON)
                return
        }
-       err = validator.ValidateCreateRole(role)
-       if err != nil {
-               rest.WriteError(w, discovery.ErrInvalidParams, err.Error())
-               return
-       }
-       err = dao.CreateRole(context.TODO(), role)
+
+       status, err := dao.CreateRole(context.TODO(), role)
        if err != nil {
-               if err == datasource.ErrRoleDuplicated {
-                       rest.WriteError(w, ErrConflictRole, "")
-                       return
-               }
                log.Error(errorsEx.MsgOperateRoleFailed, err)
-               rest.WriteError(w, discovery.ErrInternal, 
errorsEx.MsgOperateRoleFailed)
+               rest.WriteError(w, discovery.ErrInternal, err.Error())
                return
        }
-       rest.WriteSuccess(w, req)
+       rest.WriteResponse(w, req, status, nil)
 }
 
-//UpdateRolePermission update role permissions
-func (rr *RoleResource) UpdateRolePermission(w http.ResponseWriter, req 
*http.Request) {
+//UpdateRole update role permissions
+func (rr *RoleResource) UpdateRole(w http.ResponseWriter, req *http.Request) {
        name := req.URL.Query().Get(":roleName")
-       if isBuildInRole(name) {
-               rest.WriteError(w, discovery.ErrInvalidParams, 
errorsEx.MsgCantOperateRole)
-               return
-       }
        body, err := ioutil.ReadAll(req.Body)
        if err != nil {
                log.Error("read body err", err)
@@ -126,43 +112,38 @@ func (rr *RoleResource) UpdateRolePermission(w 
http.ResponseWriter, req *http.Re
                rest.WriteError(w, discovery.ErrInvalidParams, errorsEx.MsgJSON)
                return
        }
-       err = dao.EditRole(context.TODO(), name, role)
+       status, err := dao.EditRole(context.TODO(), name, role)
        if err != nil {
                log.Error(errorsEx.MsgOperateRoleFailed, err)
                rest.WriteError(w, discovery.ErrInternal, 
errorsEx.MsgOperateRoleFailed)
                return
        }
-       rest.WriteSuccess(w, req)
+
+       rest.WriteResponse(w, req, status, nil)
 }
 
 //GetRole get the role info according to role name
 func (rr *RoleResource) GetRole(w http.ResponseWriter, r *http.Request) {
-       role, err := dao.GetRole(context.TODO(), r.URL.Query().Get(":roleName"))
+       resp, status, err := dao.GetRole(context.TODO(), 
r.URL.Query().Get(":roleName"))
        if err != nil {
                log.Error(errorsEx.MsgGetRoleFailed, err)
                rest.WriteError(w, discovery.ErrInternal, 
errorsEx.MsgGetRoleFailed)
+               return
        }
-       rest.WriteResponse(w, r, nil, role)
+
+       rest.WriteResponse(w, r, status, resp)
 }
 
 //DeleteRole delete the role info by role name
 func (rr *RoleResource) DeleteRole(w http.ResponseWriter, req *http.Request) {
        n := req.URL.Query().Get(":roleName")
-       if isBuildInRole(n) {
-               rest.WriteError(w, discovery.ErrInvalidParams, 
errorsEx.MsgCantOperateRole)
-               return
-       }
-       _, err := dao.DeleteRole(context.TODO(), n)
+
+       status, err := dao.DeleteRole(context.TODO(), n)
        if err != nil {
                log.Error(errorsEx.MsgJSON, err)
                rest.WriteError(w, discovery.ErrInternal, errorsEx.MsgJSON)
                return
        }
-       rest.WriteSuccess(w, req)
-}
-func isBuildInRole(role string) bool {
-       if role == rbac.RoleAdmin || role == rbac.RoleDeveloper {
-               return true
-       }
-       return false
+
+       rest.WriteResponse(w, req, status, nil)
 }
diff --git a/server/resource/v4/role_resource_test.go 
b/server/resource/v4/role_resource_test.go
index a4939ed..76ea993 100644
--- a/server/resource/v4/role_resource_test.go
+++ b/server/resource/v4/role_resource_test.go
@@ -122,7 +122,7 @@ func TestRoleResource_CreateOrUpdateRole(t *testing.T) {
                r3.Header.Set(restful.HeaderAuth, "Bearer "+to.TokenStr)
                w3 := httptest.NewRecorder()
                rest.GetRouter().ServeHTTP(w3, r3)
-               assert.Equal(t, http.StatusBadRequest, w3.Code)
+               assert.Equal(t, http.StatusForbidden, w3.Code)
        })
 }
 func TestRoleResource_MoreRoles(t *testing.T) {
diff --git a/server/service/rbac/dao/role_dao.go 
b/server/service/rbac/dao/role_dao.go
index 23508de..6964206 100644
--- a/server/service/rbac/dao/role_dao.go
+++ b/server/service/rbac/dao/role_dao.go
@@ -19,6 +19,10 @@ package dao
 
 import (
        "context"
+       "errors"
+       errorsEx "github.com/apache/servicecomb-service-center/pkg/errors"
+       "github.com/apache/servicecomb-service-center/server/service/validator"
+       "github.com/go-chassis/cari/discovery"
 
        "github.com/apache/servicecomb-service-center/datasource"
        "github.com/apache/servicecomb-service-center/pkg/log"
@@ -26,12 +30,35 @@ import (
        "github.com/go-chassis/cari/rbac"
 )
 
-func CreateRole(ctx context.Context, r *rbac.Role) error {
-       return datasource.Instance().CreateRole(ctx, r)
+func CreateRole(ctx context.Context, r *rbac.Role) (*discovery.Response, 
error) {
+       err := validator.ValidateCreateRole(r)
+       if err != nil {
+               log.Errorf(err, "create role [%s] failed", r.Name)
+               return discovery.CreateResponse(discovery.ErrInvalidParams, 
err.Error()), nil
+       }
+       err = datasource.Instance().CreateRole(ctx, r)
+       if err == nil {
+               log.Infof("create role [%s] success", r.Name)
+               return nil, nil
+       }
+
+       log.Errorf(err, "create role [%s] failed", r.Name)
+       if err == datasource.ErrRoleDuplicated {
+               return discovery.CreateResponse(rbac.ErrRoleConflict, 
err.Error()), nil
+       }
+
+       return nil, err
 }
 
-func GetRole(ctx context.Context, name string) (*rbac.Role, error) {
-       return datasource.Instance().GetRole(ctx, name)
+func GetRole(ctx context.Context, name string) (*rbac.Role, 
*discovery.Response, error) {
+       resp, err := datasource.Instance().GetRole(ctx, name)
+       if err == nil {
+               return resp, nil, nil
+       }
+       if err == datasource.ErrRoleNotExist {
+               return nil, discovery.CreateResponse(rbac.ErrRoleNotExist, ""), 
nil
+       }
+       return nil, nil, err
 }
 
 func ListRole(ctx context.Context) ([]*rbac.Role, int64, error) {
@@ -42,35 +69,65 @@ func RoleExist(ctx context.Context, name string) (bool, 
error) {
        return datasource.Instance().RoleExist(ctx, name)
 }
 
-func DeleteRole(ctx context.Context, name string) (bool, error) {
-       if name == "admin" || name == "developer" {
-               log.Warnf("role %s can not be delete", name)
-               return false, nil
+func DeleteRole(ctx context.Context, name string) (*discovery.Response, error) 
{
+       if isBuildInRole(name) {
+               return discovery.CreateResponse(discovery.ErrForbidden, 
errorsEx.MsgCantOperateBuildInRole), nil
+       }
+       exist, err := datasource.Instance().RoleExist(ctx, name)
+       if err != nil {
+               log.Errorf(err, "check role [%s] exist failed", name)
+               return nil, err
        }
-       return datasource.Instance().DeleteRole(ctx, name)
+       if !exist {
+               log.Errorf(err, "role [%s] not exist", name)
+               return discovery.CreateResponse(rbac.ErrRoleNotExist, ""), nil
+       }
+       succeed, err := datasource.Instance().DeleteRole(ctx, name)
+       if err != nil {
+               return nil, err
+       }
+       if !succeed {
+               return nil, errors.New("delete role failed, please retry")
+       }
+       return nil, nil
 }
 
-func EditRole(ctx context.Context, name string, a *rbac.Role) error {
+func EditRole(ctx context.Context, name string, a *rbac.Role) 
(*discovery.Response, error) {
+       if isBuildInRole(name) {
+               return discovery.CreateResponse(discovery.ErrForbidden, 
errorsEx.MsgCantOperateBuildInRole), nil
+       }
        exist, err := datasource.Instance().RoleExist(ctx, name)
        if err != nil {
                log.Errorf(err, "check role [%s] exist failed", name)
-               return err
+               return nil, err
        }
        if !exist {
-               return datasource.ErrRoleCanNotEdit
+               log.Errorf(err, "role [%s] not exist", name)
+               return discovery.CreateResponse(rbac.ErrRoleNotExist, ""), nil
        }
-       oldRole, err := GetRole(ctx, name)
+       oldRole, status, err := GetRole(ctx, name)
        if err != nil {
                log.Errorf(err, "get role [%s] failed", name)
-               return err
+               return nil, err
        }
+       if status != nil && status.GetCode() != discovery.ResponseSuccess {
+               return status, nil
+       }
+
        oldRole.Perms = a.Perms
 
        err = datasource.Instance().UpdateRole(ctx, name, oldRole)
        if err != nil {
                log.Errorf(err, "can not edit role info")
-               return err
+               return nil, err
        }
        log.Infof("role [%s] is edit", oldRole.ID)
-       return nil
+       return nil, nil
+}
+
+func isBuildInRole(role string) bool {
+       if role == rbac.RoleAdmin || role == rbac.RoleDeveloper {
+               return true
+       }
+       return false
 }
diff --git a/server/service/rbac/dao/role_dao_test.go 
b/server/service/rbac/dao/role_dao_test.go
new file mode 100644
index 0000000..443332f
--- /dev/null
+++ b/server/service/rbac/dao/role_dao_test.go
@@ -0,0 +1,162 @@
+package dao_test
+
+import (
+       "context"
+       rbacsvc 
"github.com/apache/servicecomb-service-center/server/service/rbac"
+       "github.com/apache/servicecomb-service-center/server/service/rbac/dao"
+       "github.com/go-chassis/cari/discovery"
+       "github.com/go-chassis/cari/rbac"
+       "github.com/stretchr/testify/assert"
+       "testing"
+)
+
+func exampleRole(name string) *rbac.Role {
+       return &rbac.Role{
+               Name: name,
+               Perms: []*rbac.Permission{
+                       {
+                               Resources: []*rbac.Resource{
+                                       {
+                                               Type: rbacsvc.ResourceService,
+                                       },
+                               },
+                               Verbs: []string{"*"},
+                       },
+               },
+       }
+}
+
+func TestCreateRole(t *testing.T) {
+       t.Run("create new role, should succeed", func(t *testing.T) {
+               r := exampleRole("TestCreateRole_createNewRole")
+               status, err := dao.CreateRole(context.TODO(), r)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+       })
+       t.Run("create role twice, should return: 
"+rbac.NewError(rbac.ErrRoleConflict, "").Error(), func(t *testing.T) {
+               r := exampleRole("TestCreateRole_createRoleTwice")
+               status, err := dao.CreateRole(context.TODO(), r)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+               // twice
+               status, err = dao.CreateRole(context.TODO(), r)
+               assert.Nil(t, err)
+               assert.Equal(t, rbac.ErrRoleConflict, status.GetCode())
+       })
+}
+
+func TestGetRole(t *testing.T) {
+       t.Run("get no exist role, should return: 
"+rbac.NewError(rbac.ErrRoleNotExist, "").Error(), func(t *testing.T) {
+               r, status, err := dao.GetRole(context.TODO(), 
"TestGetRole_getNoExistRole")
+               assert.Nil(t, err)
+               assert.Equal(t, rbac.ErrRoleNotExist, status.GetCode())
+               assert.Nil(t, r)
+       })
+       t.Run("get exist role, should success", func(t *testing.T) {
+               r := exampleRole("TestGetRole_getExistRole")
+               status, err := dao.CreateRole(context.TODO(), r)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+               resp, status, err := dao.GetRole(context.TODO(), r.Name)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+               assert.Equal(t, r.Name, resp.Name)
+       })
+}
+
+func TestEditRole(t *testing.T) {
+       t.Run("edit no exist role, should return: 
"+rbac.NewError(rbac.ErrRoleNotExist, "").Error(), func(t *testing.T) {
+               r := exampleRole("TestEditRole_editNoExistRole")
+               status, err := dao.EditRole(context.TODO(), r.Name, r)
+               assert.Nil(t, err)
+               assert.Equal(t, rbac.ErrRoleNotExist, status.GetCode())
+       })
+       t.Run("edit role, should success", func(t *testing.T) {
+               r := exampleRole("TestGetRole_editRole")
+               status, err := dao.CreateRole(context.TODO(), r)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+
+               // edit
+               assert.Equal(t, 1, len(r.Perms))
+               r.Perms = []*rbac.Permission{
+                       {
+                               Resources: []*rbac.Resource{
+                                       {
+                                               Type: rbacsvc.ResourceService,
+                                       },
+                               },
+                               Verbs: []string{"*"},
+                       },
+                       {
+                               Resources: []*rbac.Resource{
+                                       {
+                                               Type: rbacsvc.ResourceSchema,
+                                       },
+                               },
+                               Verbs: []string{"*"},
+                       },
+               }
+               status, err = dao.EditRole(context.TODO(), r.Name, r)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+
+               resp, status, err := dao.GetRole(context.TODO(), r.Name)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+               assert.Equal(t, 2, len(resp.Perms))
+       })
+       t.Run("edit build in role, should return: 
"+discovery.NewError(discovery.ErrForbidden, "").Error(), func(t *testing.T) {
+               for _, name := range []string{rbac.RoleDeveloper, 
rbac.RoleDeveloper} {
+                       status, err := dao.EditRole(context.TODO(), name, 
exampleRole(""))
+                       assert.Nil(t, err)
+                       assert.Equal(t, discovery.ErrForbidden, 
status.GetCode())
+               }
+       })
+}
+
+func TestDeleteRole(t *testing.T) {
+       t.Run("delete no exist role, should return: 
"+rbac.NewError(rbac.ErrRoleNotExist, "").Error(), func(t *testing.T) {
+               status, err := dao.DeleteRole(context.TODO(), 
"TestDeleteRole_deleteNoExistRole")
+               assert.Nil(t, err)
+               assert.Equal(t, rbac.ErrRoleNotExist, status.GetCode())
+       })
+       t.Run("delete role, should success", func(t *testing.T) {
+               r := exampleRole("TestDeleteRole_deleteRole")
+               status, err := dao.CreateRole(context.TODO(), r)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+
+               status, err = dao.DeleteRole(context.TODO(), r.Name)
+               assert.Nil(t, err)
+               assert.True(t, status.IsSucceed())
+
+               exist, err := dao.RoleExist(context.TODO(), r.Name)
+               assert.Nil(t, err)
+               assert.False(t, exist)
+       })
+       t.Run("delete build in role, should return: 
"+discovery.NewError(discovery.ErrForbidden, "").Error(), func(t *testing.T) {
+               for _, name := range []string{rbac.RoleDeveloper, 
rbac.RoleDeveloper} {
+                       status, err := dao.DeleteRole(context.TODO(), name)
+                       assert.Nil(t, err)
+                       assert.Equal(t, discovery.ErrForbidden, 
status.GetCode())
+               }
+       })
+}
+
+func TestListRole(t *testing.T) {
+       t.Run("list role, should success", func(t *testing.T) {
+               roles, total, err := dao.ListRole(context.TODO())
+               assert.Nil(t, err)
+               assert.True(t, total > 0)
+               assert.Equal(t, int64(len(roles)), total)
+       })
+}
+
+func TestRoleExistt(t *testing.T) {
+       t.Run("check no exist role, should success and not exist", func(t 
*testing.T) {
+               exist, err := dao.RoleExist(context.TODO(), 
"TestRoleExist_checkNoExistRole")
+               assert.Nil(t, err)
+               assert.False(t, exist)
+       })
+}
diff --git a/server/service/rbac/decision.go b/server/service/rbac/decision.go
index 545f99b..52bf54c 100644
--- a/server/service/rbac/decision.go
+++ b/server/service/rbac/decision.go
@@ -83,16 +83,18 @@ func LabelMatched(targetResourceLabel map[string]string, 
permLabel map[string]st
 
 func getPermsByRoles(ctx context.Context, roleList []string) 
([]*rbac.Permission, error) {
        var allPerms = make([]*rbac.Permission, 0)
-       for i := 0; i < len(roleList); i++ {
-               r, err := datasource.Instance().GetRole(ctx, roleList[i])
-               if err != nil {
-                       return nil, err
+       for _, name := range roleList {
+               r, err := datasource.Instance().GetRole(ctx, name)
+               if err == nil {
+                       allPerms = append(allPerms, r.Perms...)
+                       continue
                }
-               if r == nil {
-                       log.Warnf("role [%s] has no any permissions", 
roleList[i])
+               if err == datasource.ErrRoleNotExist {
+                       log.Warnf("role [%s] not exist", name)
                        continue
                }
-               allPerms = append(allPerms, r.Perms...)
+               log.Errorf(err, "get role [%s] failed", name)
+               return nil, err
        }
        return allPerms, nil
 }
diff --git a/server/service/rbac/rbac.go b/server/service/rbac/rbac.go
index 76c7258..4807a34 100644
--- a/server/service/rbac/rbac.go
+++ b/server/service/rbac/rbac.go
@@ -21,12 +21,12 @@ import (
        "context"
        "crypto/rsa"
        "errors"
+       "github.com/apache/servicecomb-service-center/datasource"
        "github.com/apache/servicecomb-service-center/server/service/validator"
        "io/ioutil"
 
        "github.com/go-chassis/cari/rbac"
 
-       "github.com/apache/servicecomb-service-center/datasource"
        "github.com/apache/servicecomb-service-center/pkg/log"
        "github.com/apache/servicecomb-service-center/server/config"
        
"github.com/apache/servicecomb-service-center/server/plugin/security/cipher"
@@ -70,8 +70,7 @@ func Init() {
        }
        readPrivateKey()
        readPublicKey()
-       initAdminRole()
-       initDevRole()
+       initBuildInRole()
        rbac.Add2WhiteAPIList(APITokenGranter)
        log.Info("rbac is enabled")
 }
diff --git a/server/service/rbac/rbac_test.go b/server/service/rbac/rbac_test.go
index 2d98516..65ae32f 100644
--- a/server/service/rbac/rbac_test.go
+++ b/server/service/rbac/rbac_test.go
@@ -21,11 +21,11 @@ import (
        "context"
        "github.com/apache/servicecomb-service-center/pkg/privacy"
        "github.com/apache/servicecomb-service-center/server/config"
-       "github.com/apache/servicecomb-service-center/server/service/rbac"
+       rbacsvc 
"github.com/apache/servicecomb-service-center/server/service/rbac"
        "github.com/apache/servicecomb-service-center/server/service/rbac/dao"
        _ "github.com/apache/servicecomb-service-center/test"
        "github.com/astaxie/beego"
-       rbacmodel "github.com/go-chassis/cari/rbac"
+       "github.com/go-chassis/cari/rbac"
        "github.com/go-chassis/go-archaius"
        "github.com/go-chassis/go-chassis/v2/security/authr"
        "github.com/go-chassis/go-chassis/v2/security/secret"
@@ -61,12 +61,12 @@ func init() {
                panic(err)
        }
 
-       archaius.Set(rbac.InitPassword, "Complicated_password1")
+       archaius.Set(rbacsvc.InitPassword, "Complicated_password1")
        dao.DeleteAccount(context.Background(), "root")
        dao.DeleteAccount(context.Background(), "a")
        dao.DeleteAccount(context.Background(), "b")
 
-       rbac.Init()
+       rbacsvc.Init()
 }
 
 func TestInitRBAC(t *testing.T) {
@@ -79,27 +79,27 @@ func TestInitRBAC(t *testing.T) {
                assert.NoError(t, err)
                claims, err := authr.Authenticate(context.Background(), token)
                assert.NoError(t, err)
-               assert.Equal(t, "root", 
claims.(map[string]interface{})[rbacmodel.ClaimsUser])
+               assert.Equal(t, "root", 
claims.(map[string]interface{})[rbac.ClaimsUser])
        })
 
        t.Run("second time init", func(t *testing.T) {
-               rbac.Init()
+               rbacsvc.Init()
        })
 
        t.Run("change pwd,admin can change any one password", func(t 
*testing.T) {
-               persisted := &rbacmodel.Account{Name: "a", Password: 
"Complicated_password1"}
+               persisted := &rbac.Account{Name: "a", Password: 
"Complicated_password1"}
                err := dao.CreateAccount(context.Background(), persisted)
                assert.NoError(t, err)
-               err = rbac.ChangePassword(context.Background(), 
[]string{rbacmodel.RoleAdmin}, "admin", &rbacmodel.Account{Name: "a", Password: 
"Complicated_password2"})
+               err = rbacsvc.ChangePassword(context.Background(), 
[]string{rbac.RoleAdmin}, "admin", &rbac.Account{Name: "a", Password: 
"Complicated_password2"})
                assert.NoError(t, err)
                a, err := dao.GetAccount(context.Background(), "a")
                assert.NoError(t, err)
                assert.True(t, privacy.SamePassword(a.Password, 
"Complicated_password2"))
        })
        t.Run("change self password", func(t *testing.T) {
-               err := dao.CreateAccount(context.Background(), 
&rbacmodel.Account{Name: "b", Password: "Complicated_password1"})
+               err := dao.CreateAccount(context.Background(), 
&rbac.Account{Name: "b", Password: "Complicated_password1"})
                assert.NoError(t, err)
-               err = rbac.ChangePassword(context.Background(), nil, "b", 
&rbacmodel.Account{Name: "b", CurrentPassword: "Complicated_password1", 
Password: "Complicated_password2"})
+               err = rbacsvc.ChangePassword(context.Background(), nil, "b", 
&rbac.Account{Name: "b", CurrentPassword: "Complicated_password1", Password: 
"Complicated_password2"})
                assert.NoError(t, err)
                a, err := dao.GetAccount(context.Background(), "b")
                assert.NoError(t, err)
@@ -111,82 +111,6 @@ func TestInitRBAC(t *testing.T) {
                assert.NoError(t, err)
                assert.Greater(t, n, int64(2))
        })
-       dao.DeleteRole(context.Background(), "tester")
-       t.Run("check is there exist role", func(t *testing.T) {
-               exist, err := dao.RoleExist(context.Background(), "admin")
-               assert.NoError(t, err)
-               assert.Equal(t, true, exist)
-
-               exist, err = dao.RoleExist(context.Background(), "developer")
-               assert.NoError(t, err)
-               assert.Equal(t, true, exist)
-
-               exist, err = dao.RoleExist(context.Background(), "tester")
-               assert.NoError(t, err)
-               assert.Equal(t, false, exist)
-       })
-
-       t.Run("delete the default role", func(t *testing.T) {
-               r, err := dao.DeleteRole(context.Background(), "admin")
-               assert.NoError(t, err)
-               assert.Equal(t, false, r)
-
-               r, err = dao.DeleteRole(context.Background(), "developer")
-               assert.NoError(t, err)
-               assert.Equal(t, false, r)
-       })
-
-       t.Run("delete the not exist role", func(t *testing.T) {
-               _, err := dao.DeleteRole(context.Background(), "tester")
-               assert.NoError(t, err)
-       })
-
-       t.Run("list exist role", func(t *testing.T) {
-               _, _, err := dao.ListRole(context.TODO())
-               assert.NoError(t, err)
-       })
-
-       tester := &rbacmodel.Role{
-               Name: "tester",
-               Perms: []*rbacmodel.Permission{
-                       {
-                               Resources: []*rbacmodel.Resource{{Type: 
"service"}, {Type: "instance"}},
-                               Verbs:     []string{"get", "create", "update"},
-                       },
-                       {
-                               Resources: []*rbacmodel.Resource{{Type: 
"rule"}},
-                               Verbs:     []string{"*"},
-                       },
-               },
-       }
-
-       t.Run("create new role success", func(t *testing.T) {
-               err := dao.CreateRole(context.Background(), tester)
-               assert.NoError(t, err)
-
-               exist, err := dao.RoleExist(context.Background(), "tester")
-               assert.NoError(t, err)
-               assert.Equal(t, true, exist)
-
-               r, err := dao.GetRole(context.Background(), "tester")
-               assert.NoError(t, err)
-               assert.NotNil(t, r)
-       })
-
-       t.Run("edit new role success", func(t *testing.T) {
-               err := dao.EditRole(context.Background(), tester.Name, tester)
-               assert.NoError(t, err)
-
-               r, err := dao.GetRole(context.Background(), "tester")
-               assert.NoError(t, err)
-               assert.NotNil(t, r)
-       })
-
-       t.Run("delete the new role", func(t *testing.T) {
-               r, err := dao.DeleteRole(context.Background(), "tester")
-               assert.NoError(t, err)
-               assert.Equal(t, true, r)
-       })
 }
 func BenchmarkAuthResource_Login(b *testing.B) {
        b.RunParallel(func(pb *testing.PB) {
diff --git a/server/service/rbac/role.go b/server/service/rbac/role.go
index dcba30d..1523579 100644
--- a/server/service/rbac/role.go
+++ b/server/service/rbac/role.go
@@ -19,7 +19,6 @@ package rbac
 
 import (
        "context"
-
        "github.com/go-chassis/cari/rbac"
 
        "github.com/apache/servicecomb-service-center/pkg/log"
@@ -28,26 +27,37 @@ import (
 
 var roleMap = map[string]*rbac.Role{}
 
-// Assign resources to admin role, admin role own all permissions
-func initAdminRole() {
-       roleMap["admin"] = &rbac.Role{
-               Name:  "admin",
+func init() {
+       // Assign resources to admin role, admin role own all permissions
+       roleMap[rbac.RoleAdmin] = &rbac.Role{
+               Name:  rbac.RoleAdmin,
                Perms: AdminPerms(),
        }
-       err := dao.CreateRole(context.Background(), roleMap["admin"])
-       if err != nil {
-               log.Errorf(err, "create admin role failed")
+       roleMap[rbac.RoleDeveloper] = &rbac.Role{
+               Name:  rbac.RoleDeveloper,
+               Perms: DevPerms(),
        }
 }
 
-// Assign resources to developer role
-func initDevRole() {
-       roleMap["developer"] = &rbac.Role{
-               Name:  "developer",
-               Perms: DevPerms(),
+func initBuildInRole() {
+       for _, r := range roleMap {
+               createBuildInRole(r)
        }
-       err := dao.CreateRole(context.Background(), roleMap["developer"])
+}
+
+func createBuildInRole(r *rbac.Role) {
+       status, err := dao.CreateRole(context.Background(), r)
        if err != nil {
-               log.Errorf(err, "create developer role failed")
+               log.Fatalf(err, "create role [%s] failed", r.Name)
+               return
+       }
+       if status.IsSucceed() {
+               log.Infof("create role [%s] success", r.Name)
+               return
+       }
+       if status.Code == rbac.ErrRoleConflict {
+               log.Infof("role [%s] already exists", r.Name)
+               return
        }
+       log.Fatalf(nil, "create role [%s] failed: %s", r.Name, 
status.GetMessage())
 }

Reply via email to