You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by GitBox <gi...@apache.org> on 2020/07/16 02:45:31 UTC
[GitHub] [servicecomb-service-center] aseTo2016 commented on a change in pull request #668: [RBAC]delete, list account
aseTo2016 commented on a change in pull request #668:
URL: https://github.com/apache/servicecomb-service-center/pull/668#discussion_r455457047
##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -79,17 +77,38 @@ func (ba *TokenAuthenticator) Identify(req *http.Request) error {
log.Error("role convert failed", rbacframe.ErrConvertErr)
return rbacframe.ErrConvertErr
}
- r := dao.GetResource(context.TODO(), req.URL.Path)
- //TODO add verbs
- allow, err := rbac.Allow(context.TODO(), roleName, req.URL.Query().Get(":project"), r, "")
- if err != nil {
- log.Error("", err)
- return errors.New(errorsEx.ErrMsgRolePerm)
+ var apiPattern string
+ a := req.Context().Value(rest.CtxMatchPattern)
+ if a == nil { //handle exception
+ apiPattern = req.URL.Path
+ log.Warn("can not find api pattern")
Review comment:
建议把url打印出来，不然到时候，不知道哪个请求报这个错误
##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -79,17 +77,38 @@ func (ba *TokenAuthenticator) Identify(req *http.Request) error {
log.Error("role convert failed", rbacframe.ErrConvertErr)
return rbacframe.ErrConvertErr
}
- r := dao.GetResource(context.TODO(), req.URL.Path)
- //TODO add verbs
- allow, err := rbac.Allow(context.TODO(), roleName, req.URL.Query().Get(":project"), r, "")
- if err != nil {
- log.Error("", err)
- return errors.New(errorsEx.ErrMsgRolePerm)
+ var apiPattern string
+ a := req.Context().Value(rest.CtxMatchPattern)
+ if a == nil { //handle exception
+ apiPattern = req.URL.Path
+ log.Warn("can not find api pattern")
+ } else {
+ apiPattern = req.Context().Value(rest.CtxMatchPattern).(string)
}
- if !allow {
- return errors.New(errorsEx.ErrMsgNoPerm)
+ noPerm, err := checkPerm(roleName, apiPattern)
+ if noPerm {
+ return err
}
req2 := req.WithContext(rbacframe.NewContext(req.Context(), claims))
*req = *req2
return nil
}
+
+//this method decouple business code and perm checks
+func checkPerm(roleName, apiPattern string) (bool, error) {
Review comment:
err不为nil，返回的一定是true，感觉返回值有一个error就行，不用两个
##########
File path: server/service/rbac/dao/account_dao.go
##########
@@ -87,11 +89,31 @@ func GetAccount(ctx context.Context, name string) (*rbacframe.Account, error) {
a := &rbacframe.Account{}
err = json.Unmarshal(r.Value, a)
if err != nil {
- log.Errorf(err, "account info is invalid format")
+ log.Errorf(err, "account info format invalid")
return nil, err
}
return a, nil
}
+func ListAccount(ctx context.Context) ([]*rbacframe.Account, int64, error) {
+ key := core.GenerateAccountKey("")
+ r, n, err := kv.List(ctx, key)
+ if err != nil {
+ log.Errorf(err, "can not get account info")
+ return nil, 0, err
+ }
+ as := make([]*rbacframe.Account, 0)
Review comment:
as := make([]*rbacframe.Account, 0, n)
##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -79,17 +77,38 @@ func (ba *TokenAuthenticator) Identify(req *http.Request) error {
log.Error("role convert failed", rbacframe.ErrConvertErr)
return rbacframe.ErrConvertErr
}
- r := dao.GetResource(context.TODO(), req.URL.Path)
- //TODO add verbs
- allow, err := rbac.Allow(context.TODO(), roleName, req.URL.Query().Get(":project"), r, "")
- if err != nil {
- log.Error("", err)
- return errors.New(errorsEx.ErrMsgRolePerm)
+ var apiPattern string
+ a := req.Context().Value(rest.CtxMatchPattern)
+ if a == nil { //handle exception
+ apiPattern = req.URL.Path
+ log.Warn("can not find api pattern")
+ } else {
+ apiPattern = req.Context().Value(rest.CtxMatchPattern).(string)
Review comment:
直接apiPattern = a.(string)就可以了
##########
File path: server/service/rbac/dao/account_dao.go
##########
@@ -106,6 +128,7 @@ func DeleteAccount(ctx context.Context, name string) (bool, error) {
log.Errorf(err, "can not get account info")
return false, err
}
+ log.Info("account is deleted")
Review comment:
建议加上account的名字，不然不知道哪个account不存在
##########
File path: server/service/rbac/dao/account_dao.go
##########
@@ -87,11 +89,31 @@ func GetAccount(ctx context.Context, name string) (*rbacframe.Account, error) {
a := &rbacframe.Account{}
err = json.Unmarshal(r.Value, a)
if err != nil {
- log.Errorf(err, "account info is invalid format")
+ log.Errorf(err, "account info format invalid")
return nil, err
}
return a, nil
}
+func ListAccount(ctx context.Context) ([]*rbacframe.Account, int64, error) {
Review comment:
建议返回[]*rbacframe.Account就行，不用返回三个值，初始化as，使用as := make([]*rbacframe.Account, 0, n)，到时候外面使用cap(as)就可以了
##########
File path: server/rest/controller/v4/auth_resource.go
##########
@@ -70,11 +73,55 @@ func (r *AuthResource) CreateAccount(w http.ResponseWriter, req *http.Request) {
controller.WriteError(w, scerror.ErrConflictAccount, "")
return
}
- log.Error(errorsEx.ErrMsgCreateAccount, err)
- controller.WriteError(w, scerror.ErrInternal, errorsEx.ErrMsgCreateAccount)
+ log.Error(errorsEx.MsgOperateAccountFailed, err)
+ controller.WriteError(w, scerror.ErrInternal, errorsEx.MsgOperateAccountFailed)
return
}
}
+func (r *AuthResource) DeleteAccount(w http.ResponseWriter, req *http.Request) {
+ _, err := dao.DeleteAccount(context.TODO(), req.URL.Query().Get(":name"))
Review comment:
是否能自己删除自己，如果只有一个admin的账号，自己把自己删除了，就不能管理租户的
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org