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 2021/05/26 13:37:02 UTC

[GitHub] [servicecomb-service-center] humingcheng opened a new pull request #1014: add rbac logic

humingcheng opened a new pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014


   Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SCB) filed for the change (usually before you start working on it).  Trivial changes like typos do not require a JIRA issue.  Your pull request should address just this issue, without pulling in other changes.
    - [ ] Each commit in the pull request should have a meaningful subject line and body.
    - [ ] Format the pull request title like `[SCB-XXX] Fixes bug in ApproximateQuantiles`, where you replace `SCB-XXX` with the appropriate JIRA issue.
    - [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
    - [ ] Run `go build` `go test` `go fmt` `go vet` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
    - [ ] Never comment source code, delete it.
    - [ ] UT should has "context, subject, expected result" result as test case name, when you call t.Run().
   ---
   


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



[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639759668



##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -48,86 +49,113 @@ type TokenAuthenticator struct {
 }
 
 func (ba *TokenAuthenticator) Identify(req *http.Request) error {
-	if !rbacsvc.Enabled() {
+	if !rbac.Enabled() {
 		return nil
 	}
 	pattern, ok := req.Context().Value(rest.CtxMatchPattern).(string)
-	if ok && !mustAuth(pattern) {
-		return nil
-	}
-	v := req.Header.Get(restful.HeaderAuth)
-	if v == "" {
-		return rbac.ErrNoHeader
+	if !ok {
+		pattern = req.URL.Path
+		log.Warn("can not find api pattern")
 	}
-	s := strings.Split(v, " ")
-	if len(s) != 2 {
-		return rbac.ErrInvalidHeader
+
+	if !mustAuth(pattern) {
+		return nil
 	}
-	to := s[1]
 
-	claims, err := authr.Authenticate(req.Context(), to)
+	claims, err := ba.VerifyToken(req)
 	if err != nil {
-		log.Errorf(err, "authenticate request failed, %s %s", req.Method, req.RequestURI)
+		log.Errorf(err, "verify request token failed, %s %s", req.Method, req.RequestURI)
 		return err
 	}
+
 	m, ok := claims.(map[string]interface{})
 	if !ok {
-		log.Error("claims convert failed", rbac.ErrConvertErr)
-		return rbac.ErrConvertErr
+		log.Error("claims convert failed", rbacModel.ErrConvertErr)
+		return rbacModel.ErrConvertErr
 	}
-
-	roleList, err := rbac.GetRolesList(m)
+	account, err := rbacModel.GetAccount(m)
 	if err != nil {
-		log.Error("get role list failed", err)
+		log.Error("get account  failed", err)
 		return err
 	}
+	util.SetRequestContext(req, authHandler.CtxRequestClaims, m)
+	// user can change self password
+	if pattern == rbac.APIAccountPassword && account.Name == req.URL.Query().Get(":name") {
+		return nil
+	}
 
-	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 = a.(string)
+	if len(account.Roles) == 0 {
+		log.Error("no role found in token", nil)
+		return errors.New(errorsEx.MsgNoPerm)
 	}
 
 	project := req.URL.Query().Get(":project")
-	verbs := rbacsvc.MethodToVerbs[req.Method]
-	err = checkPerm(roleList, project, apiPattern, verbs)
+	allow, matchedLabels, err := checkPerm(account.Roles, project, req, pattern, req.Method)
 	if err != nil {
 		return err
 	}
-	req2 := req.WithContext(rbac.NewContext(req.Context(), m))
-	*req = *req2
+	if !allow {
+		return errors.New(errorsEx.MsgNoPerm)
+	}
+
+	util.SetRequestContext(req, authHandler.CtxResourceLabels, matchedLabels)
 	return nil
 }
 
-//this method decouple business code and perm checks
-func checkPerm(roleList []string, project, apiPattern, verbs string) error {
-	resource := rbac.GetResource(apiPattern)
-	if resource == "" {
-		//fast fail, no need to access role storage
-		return errors.New(errorsEx.MsgNoPerm)
+func filterRoles(roleList []string) (hasAdmin bool, normalRoles []string) {
+	for _, r := range roleList {
+		if r == rbacModel.RoleAdmin {
+			hasAdmin = true
+			return
+		}
+		normalRoles = append(normalRoles, r)
 	}
-	//TODO add verbs,project
-	allow, err := rbacsvc.Allow(context.TODO(), roleList, project, resource, verbs)
-	if err != nil {
-		log.Error("", err)
-		return errors.New(errorsEx.MsgRolePerm)
+	return
+}
+
+func (ba *TokenAuthenticator) VerifyToken(req *http.Request) (interface{}, error) {
+	v := req.Header.Get(restful.HeaderAuth)
+	if v == "" {
+		return nil, rbacModel.ErrNoHeader
 	}
-	if !allow {
-		return errors.New(errorsEx.MsgNoPerm)
+	s := strings.Split(v, " ")
+	if len(s) != 2 {
+		return nil, rbacModel.ErrInvalidHeader
 	}
-	return nil
+	to := s[1]
+
+	return authr.Authenticate(req.Context(), to)
+}
+
+//this method decouple business code and perm checks
+func checkPerm(roleList []string, project string, req *http.Request, apiPattern, method string) (bool, []map[string]string, error) {
+	hasAdmin, normalRoles := filterRoles(roleList)
+	if hasAdmin {
+		return true, nil, nil
+	}
+	//todo fast check for dev role
+	targetResource, ok := req.Context().Value(authHandler.CtxResourceScopes).(*auth.ResourceScope)
+	if !ok || targetResource == nil {
+		return false, nil, errors.New("no valid resouce scope")
+	}
+	//TODO add project
+	return rbac.Allow(context.TODO(), project, normalRoles, targetResource)
 }
 
 func mustAuth(pattern string) bool {
 	if util.IsVersionOrHealthPattern(pattern) {
 		return false
 	}
-	return rbac.MustAuth(pattern)
+	return rbacModel.MustAuth(pattern)
 }
 
 func (ba *TokenAuthenticator) ResourceScopes(r *http.Request) *auth.ResourceScope {
 	return FromRequest(r)
 }
+func AccountFromContext(ctx context.Context) (*rbacModel.Account, error) {
+	m, ok := ctx.Value(authHandler.CtxRequestClaims).(map[string]interface{})

Review comment:
       claims的存储早已经标准化了,怎么又使用了一种全新的方式




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



[GitHub] [servicecomb-service-center] humingcheng commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
humingcheng commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639778125



##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -25,13 +25,14 @@ import (
 	"strings"
 
 	"github.com/apache/servicecomb-service-center/pkg/plugin"
-	"github.com/go-chassis/cari/rbac"
+	rbacModel "github.com/go-chassis/cari/rbac"

Review comment:
       done




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



[GitHub] [servicecomb-service-center] humingcheng commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
humingcheng commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639775896



##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -48,86 +49,113 @@ type TokenAuthenticator struct {
 }
 
 func (ba *TokenAuthenticator) Identify(req *http.Request) error {
-	if !rbacsvc.Enabled() {
+	if !rbac.Enabled() {
 		return nil
 	}
 	pattern, ok := req.Context().Value(rest.CtxMatchPattern).(string)
-	if ok && !mustAuth(pattern) {
-		return nil
-	}
-	v := req.Header.Get(restful.HeaderAuth)
-	if v == "" {
-		return rbac.ErrNoHeader
+	if !ok {
+		pattern = req.URL.Path
+		log.Warn("can not find api pattern")
 	}
-	s := strings.Split(v, " ")
-	if len(s) != 2 {
-		return rbac.ErrInvalidHeader
+
+	if !mustAuth(pattern) {
+		return nil
 	}
-	to := s[1]
 
-	claims, err := authr.Authenticate(req.Context(), to)
+	claims, err := ba.VerifyToken(req)
 	if err != nil {
-		log.Errorf(err, "authenticate request failed, %s %s", req.Method, req.RequestURI)
+		log.Errorf(err, "verify request token failed, %s %s", req.Method, req.RequestURI)
 		return err
 	}
+
 	m, ok := claims.(map[string]interface{})
 	if !ok {
-		log.Error("claims convert failed", rbac.ErrConvertErr)
-		return rbac.ErrConvertErr
+		log.Error("claims convert failed", rbacModel.ErrConvertErr)
+		return rbacModel.ErrConvertErr
 	}
-
-	roleList, err := rbac.GetRolesList(m)
+	account, err := rbacModel.GetAccount(m)
 	if err != nil {
-		log.Error("get role list failed", err)
+		log.Error("get account  failed", err)
 		return err
 	}
+	util.SetRequestContext(req, authHandler.CtxRequestClaims, m)
+	// user can change self password
+	if pattern == rbac.APIAccountPassword && account.Name == req.URL.Query().Get(":name") {
+		return nil
+	}
 
-	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 = a.(string)
+	if len(account.Roles) == 0 {
+		log.Error("no role found in token", nil)
+		return errors.New(errorsEx.MsgNoPerm)
 	}
 
 	project := req.URL.Query().Get(":project")
-	verbs := rbacsvc.MethodToVerbs[req.Method]
-	err = checkPerm(roleList, project, apiPattern, verbs)
+	allow, matchedLabels, err := checkPerm(account.Roles, project, req, pattern, req.Method)
 	if err != nil {
 		return err
 	}
-	req2 := req.WithContext(rbac.NewContext(req.Context(), m))
-	*req = *req2
+	if !allow {
+		return errors.New(errorsEx.MsgNoPerm)
+	}
+
+	util.SetRequestContext(req, authHandler.CtxResourceLabels, matchedLabels)
 	return nil
 }
 
-//this method decouple business code and perm checks
-func checkPerm(roleList []string, project, apiPattern, verbs string) error {
-	resource := rbac.GetResource(apiPattern)
-	if resource == "" {
-		//fast fail, no need to access role storage
-		return errors.New(errorsEx.MsgNoPerm)
+func filterRoles(roleList []string) (hasAdmin bool, normalRoles []string) {
+	for _, r := range roleList {
+		if r == rbacModel.RoleAdmin {
+			hasAdmin = true
+			return
+		}
+		normalRoles = append(normalRoles, r)
 	}
-	//TODO add verbs,project
-	allow, err := rbacsvc.Allow(context.TODO(), roleList, project, resource, verbs)
-	if err != nil {
-		log.Error("", err)
-		return errors.New(errorsEx.MsgRolePerm)
+	return
+}
+
+func (ba *TokenAuthenticator) VerifyToken(req *http.Request) (interface{}, error) {
+	v := req.Header.Get(restful.HeaderAuth)
+	if v == "" {
+		return nil, rbacModel.ErrNoHeader
 	}
-	if !allow {
-		return errors.New(errorsEx.MsgNoPerm)
+	s := strings.Split(v, " ")
+	if len(s) != 2 {
+		return nil, rbacModel.ErrInvalidHeader
 	}
-	return nil
+	to := s[1]
+
+	return authr.Authenticate(req.Context(), to)
+}
+
+//this method decouple business code and perm checks
+func checkPerm(roleList []string, project string, req *http.Request, apiPattern, method string) (bool, []map[string]string, error) {
+	hasAdmin, normalRoles := filterRoles(roleList)
+	if hasAdmin {
+		return true, nil, nil
+	}
+	//todo fast check for dev role
+	targetResource, ok := req.Context().Value(authHandler.CtxResourceScopes).(*auth.ResourceScope)
+	if !ok || targetResource == nil {
+		return false, nil, errors.New("no valid resouce scope")
+	}
+	//TODO add project
+	return rbac.Allow(context.TODO(), project, normalRoles, targetResource)
 }
 
 func mustAuth(pattern string) bool {
 	if util.IsVersionOrHealthPattern(pattern) {
 		return false
 	}
-	return rbac.MustAuth(pattern)
+	return rbacModel.MustAuth(pattern)
 }
 
 func (ba *TokenAuthenticator) ResourceScopes(r *http.Request) *auth.ResourceScope {
 	return FromRequest(r)
 }
+func AccountFromContext(ctx context.Context) (*rbacModel.Account, error) {
+	m, ok := ctx.Value(authHandler.CtxRequestClaims).(map[string]interface{})

Review comment:
       原来的语句,会导致req中的context指针被修改,sc里面其他地方就获取不到对应的context内容,所以暂时改成sc的方式。




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



[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639756135



##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -48,86 +49,113 @@ type TokenAuthenticator struct {
 }
 
 func (ba *TokenAuthenticator) Identify(req *http.Request) error {
-	if !rbacsvc.Enabled() {
+	if !rbac.Enabled() {
 		return nil
 	}
 	pattern, ok := req.Context().Value(rest.CtxMatchPattern).(string)
-	if ok && !mustAuth(pattern) {
-		return nil
-	}
-	v := req.Header.Get(restful.HeaderAuth)
-	if v == "" {
-		return rbac.ErrNoHeader
+	if !ok {
+		pattern = req.URL.Path
+		log.Warn("can not find api pattern")
 	}
-	s := strings.Split(v, " ")
-	if len(s) != 2 {
-		return rbac.ErrInvalidHeader
+
+	if !mustAuth(pattern) {
+		return nil
 	}
-	to := s[1]
 
-	claims, err := authr.Authenticate(req.Context(), to)
+	claims, err := ba.VerifyToken(req)
 	if err != nil {
-		log.Errorf(err, "authenticate request failed, %s %s", req.Method, req.RequestURI)
+		log.Errorf(err, "verify request token failed, %s %s", req.Method, req.RequestURI)
 		return err
 	}
+
 	m, ok := claims.(map[string]interface{})
 	if !ok {
-		log.Error("claims convert failed", rbac.ErrConvertErr)
-		return rbac.ErrConvertErr
+		log.Error("claims convert failed", rbacModel.ErrConvertErr)
+		return rbacModel.ErrConvertErr
 	}
-
-	roleList, err := rbac.GetRolesList(m)
+	account, err := rbacModel.GetAccount(m)
 	if err != nil {
-		log.Error("get role list failed", err)
+		log.Error("get account  failed", err)
 		return err
 	}
+	util.SetRequestContext(req, authHandler.CtxRequestClaims, m)
+	// user can change self password
+	if pattern == rbac.APIAccountPassword && account.Name == req.URL.Query().Get(":name") {

Review comment:
       if语句太长,这句话抽func




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



[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639753918



##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -25,13 +25,14 @@ import (
 	"strings"
 
 	"github.com/apache/servicecomb-service-center/pkg/plugin"
-	"github.com/go-chassis/cari/rbac"
+	rbacModel "github.com/go-chassis/cari/rbac"

Review comment:
       rbac包并不是提供模型的包,所以保持原状

##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -48,86 +49,113 @@ type TokenAuthenticator struct {
 }
 
 func (ba *TokenAuthenticator) Identify(req *http.Request) error {
-	if !rbacsvc.Enabled() {
+	if !rbac.Enabled() {

Review comment:
       这个事sc的rbacsvc,保持原状




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



[GitHub] [servicecomb-service-center] humingcheng commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
humingcheng commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639777940



##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -48,86 +49,113 @@ type TokenAuthenticator struct {
 }
 
 func (ba *TokenAuthenticator) Identify(req *http.Request) error {
-	if !rbacsvc.Enabled() {
+	if !rbac.Enabled() {
 		return nil
 	}
 	pattern, ok := req.Context().Value(rest.CtxMatchPattern).(string)
-	if ok && !mustAuth(pattern) {
-		return nil
-	}
-	v := req.Header.Get(restful.HeaderAuth)
-	if v == "" {
-		return rbac.ErrNoHeader
+	if !ok {
+		pattern = req.URL.Path
+		log.Warn("can not find api pattern")
 	}
-	s := strings.Split(v, " ")
-	if len(s) != 2 {
-		return rbac.ErrInvalidHeader
+
+	if !mustAuth(pattern) {
+		return nil
 	}
-	to := s[1]
 
-	claims, err := authr.Authenticate(req.Context(), to)
+	claims, err := ba.VerifyToken(req)
 	if err != nil {
-		log.Errorf(err, "authenticate request failed, %s %s", req.Method, req.RequestURI)
+		log.Errorf(err, "verify request token failed, %s %s", req.Method, req.RequestURI)
 		return err
 	}
+
 	m, ok := claims.(map[string]interface{})
 	if !ok {
-		log.Error("claims convert failed", rbac.ErrConvertErr)
-		return rbac.ErrConvertErr
+		log.Error("claims convert failed", rbacModel.ErrConvertErr)
+		return rbacModel.ErrConvertErr
 	}
-
-	roleList, err := rbac.GetRolesList(m)
+	account, err := rbacModel.GetAccount(m)
 	if err != nil {
-		log.Error("get role list failed", err)
+		log.Error("get account  failed", err)
 		return err
 	}
+	util.SetRequestContext(req, authHandler.CtxRequestClaims, m)
+	// user can change self password
+	if pattern == rbac.APIAccountPassword && account.Name == req.URL.Query().Get(":name") {
+		return nil
+	}
 
-	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 = a.(string)
+	if len(account.Roles) == 0 {
+		log.Error("no role found in token", nil)
+		return errors.New(errorsEx.MsgNoPerm)
 	}
 
 	project := req.URL.Query().Get(":project")
-	verbs := rbacsvc.MethodToVerbs[req.Method]
-	err = checkPerm(roleList, project, apiPattern, verbs)
+	allow, matchedLabels, err := checkPerm(account.Roles, project, req, pattern, req.Method)
 	if err != nil {
 		return err
 	}
-	req2 := req.WithContext(rbac.NewContext(req.Context(), m))
-	*req = *req2
+	if !allow {
+		return errors.New(errorsEx.MsgNoPerm)
+	}
+
+	util.SetRequestContext(req, authHandler.CtxResourceLabels, matchedLabels)
 	return nil
 }
 
-//this method decouple business code and perm checks
-func checkPerm(roleList []string, project, apiPattern, verbs string) error {
-	resource := rbac.GetResource(apiPattern)
-	if resource == "" {
-		//fast fail, no need to access role storage
-		return errors.New(errorsEx.MsgNoPerm)
+func filterRoles(roleList []string) (hasAdmin bool, normalRoles []string) {
+	for _, r := range roleList {
+		if r == rbacModel.RoleAdmin {
+			hasAdmin = true
+			return
+		}
+		normalRoles = append(normalRoles, r)
 	}
-	//TODO add verbs,project
-	allow, err := rbacsvc.Allow(context.TODO(), roleList, project, resource, verbs)
-	if err != nil {
-		log.Error("", err)
-		return errors.New(errorsEx.MsgRolePerm)
+	return
+}
+
+func (ba *TokenAuthenticator) VerifyToken(req *http.Request) (interface{}, error) {
+	v := req.Header.Get(restful.HeaderAuth)
+	if v == "" {
+		return nil, rbacModel.ErrNoHeader
 	}
-	if !allow {
-		return errors.New(errorsEx.MsgNoPerm)
+	s := strings.Split(v, " ")
+	if len(s) != 2 {
+		return nil, rbacModel.ErrInvalidHeader
 	}
-	return nil
+	to := s[1]
+
+	return authr.Authenticate(req.Context(), to)
+}
+
+//this method decouple business code and perm checks
+func checkPerm(roleList []string, project string, req *http.Request, apiPattern, method string) (bool, []map[string]string, error) {
+	hasAdmin, normalRoles := filterRoles(roleList)
+	if hasAdmin {
+		return true, nil, nil
+	}
+	//todo fast check for dev role
+	targetResource, ok := req.Context().Value(authHandler.CtxResourceScopes).(*auth.ResourceScope)
+	if !ok || targetResource == nil {
+		return false, nil, errors.New("no valid resouce scope")
+	}
+	//TODO add project
+	return rbac.Allow(context.TODO(), project, normalRoles, targetResource)
 }
 
 func mustAuth(pattern string) bool {
 	if util.IsVersionOrHealthPattern(pattern) {
 		return false
 	}
-	return rbac.MustAuth(pattern)
+	return rbacModel.MustAuth(pattern)
 }
 
 func (ba *TokenAuthenticator) ResourceScopes(r *http.Request) *auth.ResourceScope {
 	return FromRequest(r)
 }
+func AccountFromContext(ctx context.Context) (*rbacModel.Account, error) {
+	m, ok := ctx.Value(authHandler.CtxRequestClaims).(map[string]interface{})

Review comment:
       跟sc的context读写不兼容。




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



[GitHub] [servicecomb-service-center] humingcheng commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
humingcheng commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639778267



##########
File path: server/service/rbac/decision_test.go
##########
@@ -18,55 +18,175 @@
 package rbac_test
 
 import (
-	"context"
-	"io/ioutil"
 	"testing"
 
-	"github.com/go-chassis/go-archaius"
-	"github.com/go-chassis/go-chassis/v2/security/secret"
+	rbacModel "github.com/go-chassis/cari/rbac"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/apache/servicecomb-service-center/server/service/rbac"
-	"github.com/apache/servicecomb-service-center/server/service/rbac/dao"
 )
 
-func TestAllow(t *testing.T) {
-	err := archaius.Init(archaius.WithMemorySource(), archaius.WithENVSource())
-	assert.NoError(t, err)
+func TestGetLabel(t *testing.T) {
+	perms := []*rbacModel.Permission{
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"e": "f"},
+				},
+			},
+			Verbs: []string{"get"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"c": "d"},
+				},
+			},
+			Verbs: []string{"*"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type: rbac.ResourceService,
+				},
+			},
+			Verbs: []string{"delete"},
+		},
+	}
+	t.Run("resource and verb matched, should allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+	})
 
-	pri, pub, err := secret.GenRSAKeyPair(4096)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, should allow and combine their labels", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "get")
+		assert.True(t, allow)
+		assert.Equal(t, 2, len(labelList))
+	})
 
-	b, err := secret.RSAPrivate2Bytes(pri)
-	assert.NoError(t, err)
-	ioutil.WriteFile("./private.key", b, 0600)
-	b, err = secret.RSAPublicKey2Bytes(pub)
-	err = ioutil.WriteFile("./rbac.pub", b, 0600)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, one of them has no label, should allow and no label", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "delete")
+		assert.True(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("resource not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceRole, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("Verb not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceAccount, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	archaius.Set(rbac.InitPassword, "Complicated_password1")
+func TestGetLabelFromSinglePerm(t *testing.T) {
+	t.Run("resource and verb match, should allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+		assert.Equal(t, "b", labelList[0]["a"])
+	})
 
-	dao.DeleteAccount(context.Background(), "root")
-	dao.DeleteAccount(context.Background(), "a")
-	dao.DeleteAccount(context.Background(), "b")
+	t.Run("resource not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceService, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
 
-	rbac.Init()
-	a, err := dao.GetAccount(context.Background(), "root")
-	assert.NoError(t, err)
-	assert.Equal(t, "root", a.Name)
+	t.Run("verb not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"get"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	t.Run("admin can operate any resource", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"admin"}, "default", "account", "create")
-		assert.True(t, ok)
-		ok, _ = rbac.Allow(context.TODO(), []string{"admin"}, "default", "service", "create")
-		assert.True(t, ok)
+func TestLabelMatched(t *testing.T) {
+	targetResourceLabel := map[string]string{
+		"a": "b",
+		"c": "d",
+	}
+	t.Run("value not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can not operate account", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "account", "create")
-		assert.False(t, ok)
+	t.Run("key not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"h": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can operate service", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "service", "create")
-		assert.True(t, ok)
+	t.Run("target resource label matches no permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
+	t.Run("target resource label matches part permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",
+			"a": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
+	})
+	t.Run("target resource label matches  permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "b",
+		}
+		assert.True(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
+	})
+}
+func TestFilterLabel(t *testing.T) {
+	targetResourceLabel := []map[string]string{
+		{"a": "b", "c": "d"},

Review comment:
       done




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



[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639755197



##########
File path: server/service/rbac/decision_test.go
##########
@@ -18,55 +18,175 @@
 package rbac_test
 
 import (
-	"context"
-	"io/ioutil"
 	"testing"
 
-	"github.com/go-chassis/go-archaius"
-	"github.com/go-chassis/go-chassis/v2/security/secret"
+	rbacModel "github.com/go-chassis/cari/rbac"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/apache/servicecomb-service-center/server/service/rbac"
-	"github.com/apache/servicecomb-service-center/server/service/rbac/dao"
 )
 
-func TestAllow(t *testing.T) {
-	err := archaius.Init(archaius.WithMemorySource(), archaius.WithENVSource())
-	assert.NoError(t, err)
+func TestGetLabel(t *testing.T) {
+	perms := []*rbacModel.Permission{
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"e": "f"},
+				},
+			},
+			Verbs: []string{"get"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"c": "d"},
+				},
+			},
+			Verbs: []string{"*"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type: rbac.ResourceService,
+				},
+			},
+			Verbs: []string{"delete"},
+		},
+	}
+	t.Run("resource and verb matched, should allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+	})
 
-	pri, pub, err := secret.GenRSAKeyPair(4096)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, should allow and combine their labels", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "get")
+		assert.True(t, allow)
+		assert.Equal(t, 2, len(labelList))
+	})
 
-	b, err := secret.RSAPrivate2Bytes(pri)
-	assert.NoError(t, err)
-	ioutil.WriteFile("./private.key", b, 0600)
-	b, err = secret.RSAPublicKey2Bytes(pub)
-	err = ioutil.WriteFile("./rbac.pub", b, 0600)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, one of them has no label, should allow and no label", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "delete")
+		assert.True(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("resource not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceRole, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("Verb not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceAccount, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	archaius.Set(rbac.InitPassword, "Complicated_password1")
+func TestGetLabelFromSinglePerm(t *testing.T) {
+	t.Run("resource and verb match, should allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+		assert.Equal(t, "b", labelList[0]["a"])
+	})
 
-	dao.DeleteAccount(context.Background(), "root")
-	dao.DeleteAccount(context.Background(), "a")
-	dao.DeleteAccount(context.Background(), "b")
+	t.Run("resource not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceService, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
 
-	rbac.Init()
-	a, err := dao.GetAccount(context.Background(), "root")
-	assert.NoError(t, err)
-	assert.Equal(t, "root", a.Name)
+	t.Run("verb not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"get"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	t.Run("admin can operate any resource", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"admin"}, "default", "account", "create")
-		assert.True(t, ok)
-		ok, _ = rbac.Allow(context.TODO(), []string{"admin"}, "default", "service", "create")
-		assert.True(t, ok)
+func TestLabelMatched(t *testing.T) {
+	targetResourceLabel := map[string]string{
+		"a": "b",
+		"c": "d",
+	}
+	t.Run("value not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can not operate account", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "account", "create")
-		assert.False(t, ok)
+	t.Run("key not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"h": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can operate service", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "service", "create")
-		assert.True(t, ok)
+	t.Run("target resource label matches no permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
+	t.Run("target resource label matches part permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",
+			"a": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
+	})
+	t.Run("target resource label matches  permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "b",
+		}
+		assert.True(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
+	})
+}
+func TestFilterLabel(t *testing.T) {
+	targetResourceLabel := []map[string]string{
+		{"a": "b", "c": "d"},

Review comment:
       这个真的不太好理解




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



[GitHub] [servicecomb-service-center] tianxiaoliang merged pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
tianxiaoliang merged pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014


   


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



[GitHub] [servicecomb-service-center] humingcheng commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
humingcheng commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639798475



##########
File path: server/plugin/auth/buildin/buildin.go
##########
@@ -48,86 +49,113 @@ type TokenAuthenticator struct {
 }
 
 func (ba *TokenAuthenticator) Identify(req *http.Request) error {
-	if !rbacsvc.Enabled() {
+	if !rbac.Enabled() {
 		return nil
 	}
 	pattern, ok := req.Context().Value(rest.CtxMatchPattern).(string)
-	if ok && !mustAuth(pattern) {
-		return nil
-	}
-	v := req.Header.Get(restful.HeaderAuth)
-	if v == "" {
-		return rbac.ErrNoHeader
+	if !ok {
+		pattern = req.URL.Path
+		log.Warn("can not find api pattern")
 	}
-	s := strings.Split(v, " ")
-	if len(s) != 2 {
-		return rbac.ErrInvalidHeader
+
+	if !mustAuth(pattern) {
+		return nil
 	}
-	to := s[1]
 
-	claims, err := authr.Authenticate(req.Context(), to)
+	claims, err := ba.VerifyToken(req)
 	if err != nil {
-		log.Errorf(err, "authenticate request failed, %s %s", req.Method, req.RequestURI)
+		log.Errorf(err, "verify request token failed, %s %s", req.Method, req.RequestURI)
 		return err
 	}
+
 	m, ok := claims.(map[string]interface{})
 	if !ok {
-		log.Error("claims convert failed", rbac.ErrConvertErr)
-		return rbac.ErrConvertErr
+		log.Error("claims convert failed", rbacModel.ErrConvertErr)
+		return rbacModel.ErrConvertErr
 	}
-
-	roleList, err := rbac.GetRolesList(m)
+	account, err := rbacModel.GetAccount(m)
 	if err != nil {
-		log.Error("get role list failed", err)
+		log.Error("get account  failed", err)
 		return err
 	}
+	util.SetRequestContext(req, authHandler.CtxRequestClaims, m)
+	// user can change self password
+	if pattern == rbac.APIAccountPassword && account.Name == req.URL.Query().Get(":name") {

Review comment:
       done




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



[GitHub] [servicecomb-service-center] humingcheng commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
humingcheng commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639813394



##########
File path: server/service/rbac/decision_test.go
##########
@@ -18,55 +18,175 @@
 package rbac_test
 
 import (
-	"context"
-	"io/ioutil"
 	"testing"
 
-	"github.com/go-chassis/go-archaius"
-	"github.com/go-chassis/go-chassis/v2/security/secret"
+	rbacModel "github.com/go-chassis/cari/rbac"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/apache/servicecomb-service-center/server/service/rbac"
-	"github.com/apache/servicecomb-service-center/server/service/rbac/dao"
 )
 
-func TestAllow(t *testing.T) {
-	err := archaius.Init(archaius.WithMemorySource(), archaius.WithENVSource())
-	assert.NoError(t, err)
+func TestGetLabel(t *testing.T) {
+	perms := []*rbacModel.Permission{
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"e": "f"},
+				},
+			},
+			Verbs: []string{"get"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"c": "d"},
+				},
+			},
+			Verbs: []string{"*"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type: rbac.ResourceService,
+				},
+			},
+			Verbs: []string{"delete"},
+		},
+	}
+	t.Run("resource and verb matched, should allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+	})
 
-	pri, pub, err := secret.GenRSAKeyPair(4096)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, should allow and combine their labels", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "get")
+		assert.True(t, allow)
+		assert.Equal(t, 2, len(labelList))
+	})
 
-	b, err := secret.RSAPrivate2Bytes(pri)
-	assert.NoError(t, err)
-	ioutil.WriteFile("./private.key", b, 0600)
-	b, err = secret.RSAPublicKey2Bytes(pub)
-	err = ioutil.WriteFile("./rbac.pub", b, 0600)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, one of them has no label, should allow and no label", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "delete")
+		assert.True(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("resource not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceRole, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("Verb not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceAccount, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	archaius.Set(rbac.InitPassword, "Complicated_password1")
+func TestGetLabelFromSinglePerm(t *testing.T) {
+	t.Run("resource and verb match, should allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+		assert.Equal(t, "b", labelList[0]["a"])
+	})
 
-	dao.DeleteAccount(context.Background(), "root")
-	dao.DeleteAccount(context.Background(), "a")
-	dao.DeleteAccount(context.Background(), "b")
+	t.Run("resource not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceService, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
 
-	rbac.Init()
-	a, err := dao.GetAccount(context.Background(), "root")
-	assert.NoError(t, err)
-	assert.Equal(t, "root", a.Name)
+	t.Run("verb not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"get"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	t.Run("admin can operate any resource", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"admin"}, "default", "account", "create")
-		assert.True(t, ok)
-		ok, _ = rbac.Allow(context.TODO(), []string{"admin"}, "default", "service", "create")
-		assert.True(t, ok)
+func TestLabelMatched(t *testing.T) {
+	targetResourceLabel := map[string]string{
+		"a": "b",
+		"c": "d",
+	}
+	t.Run("value not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can not operate account", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "account", "create")
-		assert.False(t, ok)
+	t.Run("key not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"h": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can operate service", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "service", "create")
-		assert.True(t, ok)
+	t.Run("target resource label matches no permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
+	t.Run("target resource label matches part permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",
+			"a": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
+	})
+	t.Run("target resource label matches  permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "b",
+		}
+		assert.True(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
+	})
+}
+func TestFilterLabel(t *testing.T) {
+	targetResourceLabel := []map[string]string{
+		{"a": "b", "c": "d"},

Review comment:
       改成了environment/serviceName等业务相关的内容

##########
File path: server/service/rbac/decision_test.go
##########
@@ -18,55 +18,175 @@
 package rbac_test
 
 import (
-	"context"
-	"io/ioutil"
 	"testing"
 
-	"github.com/go-chassis/go-archaius"
-	"github.com/go-chassis/go-chassis/v2/security/secret"
+	rbacModel "github.com/go-chassis/cari/rbac"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/apache/servicecomb-service-center/server/service/rbac"
-	"github.com/apache/servicecomb-service-center/server/service/rbac/dao"
 )
 
-func TestAllow(t *testing.T) {
-	err := archaius.Init(archaius.WithMemorySource(), archaius.WithENVSource())
-	assert.NoError(t, err)
+func TestGetLabel(t *testing.T) {
+	perms := []*rbacModel.Permission{
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"e": "f"},
+				},
+			},
+			Verbs: []string{"get"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"c": "d"},
+				},
+			},
+			Verbs: []string{"*"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type: rbac.ResourceService,
+				},
+			},
+			Verbs: []string{"delete"},
+		},
+	}
+	t.Run("resource and verb matched, should allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+	})
 
-	pri, pub, err := secret.GenRSAKeyPair(4096)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, should allow and combine their labels", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "get")
+		assert.True(t, allow)
+		assert.Equal(t, 2, len(labelList))
+	})
 
-	b, err := secret.RSAPrivate2Bytes(pri)
-	assert.NoError(t, err)
-	ioutil.WriteFile("./private.key", b, 0600)
-	b, err = secret.RSAPublicKey2Bytes(pub)
-	err = ioutil.WriteFile("./rbac.pub", b, 0600)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, one of them has no label, should allow and no label", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "delete")
+		assert.True(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("resource not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceRole, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("Verb not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceAccount, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	archaius.Set(rbac.InitPassword, "Complicated_password1")
+func TestGetLabelFromSinglePerm(t *testing.T) {
+	t.Run("resource and verb match, should allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+		assert.Equal(t, "b", labelList[0]["a"])
+	})
 
-	dao.DeleteAccount(context.Background(), "root")
-	dao.DeleteAccount(context.Background(), "a")
-	dao.DeleteAccount(context.Background(), "b")
+	t.Run("resource not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceService, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
 
-	rbac.Init()
-	a, err := dao.GetAccount(context.Background(), "root")
-	assert.NoError(t, err)
-	assert.Equal(t, "root", a.Name)
+	t.Run("verb not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"get"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	t.Run("admin can operate any resource", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"admin"}, "default", "account", "create")
-		assert.True(t, ok)
-		ok, _ = rbac.Allow(context.TODO(), []string{"admin"}, "default", "service", "create")
-		assert.True(t, ok)
+func TestLabelMatched(t *testing.T) {
+	targetResourceLabel := map[string]string{
+		"a": "b",
+		"c": "d",
+	}
+	t.Run("value not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can not operate account", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "account", "create")
-		assert.False(t, ok)
+	t.Run("key not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"h": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can operate service", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "service", "create")
-		assert.True(t, ok)
+	t.Run("target resource label matches no permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",

Review comment:
       改成了environment/serviceName等业务相关的内容




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



[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1014: add rbac logic

Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1014:
URL: https://github.com/apache/servicecomb-service-center/pull/1014#discussion_r639760861



##########
File path: server/service/rbac/decision_test.go
##########
@@ -18,55 +18,175 @@
 package rbac_test
 
 import (
-	"context"
-	"io/ioutil"
 	"testing"
 
-	"github.com/go-chassis/go-archaius"
-	"github.com/go-chassis/go-chassis/v2/security/secret"
+	rbacModel "github.com/go-chassis/cari/rbac"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/apache/servicecomb-service-center/server/service/rbac"
-	"github.com/apache/servicecomb-service-center/server/service/rbac/dao"
 )
 
-func TestAllow(t *testing.T) {
-	err := archaius.Init(archaius.WithMemorySource(), archaius.WithENVSource())
-	assert.NoError(t, err)
+func TestGetLabel(t *testing.T) {
+	perms := []*rbacModel.Permission{
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"e": "f"},
+				},
+			},
+			Verbs: []string{"get"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceService,
+					Labels: map[string]string{"c": "d"},
+				},
+			},
+			Verbs: []string{"*"},
+		},
+		&rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type: rbac.ResourceService,
+				},
+			},
+			Verbs: []string{"delete"},
+		},
+	}
+	t.Run("resource and verb matched, should allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+	})
 
-	pri, pub, err := secret.GenRSAKeyPair(4096)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, should allow and combine their labels", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "get")
+		assert.True(t, allow)
+		assert.Equal(t, 2, len(labelList))
+	})
 
-	b, err := secret.RSAPrivate2Bytes(pri)
-	assert.NoError(t, err)
-	ioutil.WriteFile("./private.key", b, 0600)
-	b, err = secret.RSAPublicKey2Bytes(pub)
-	err = ioutil.WriteFile("./rbac.pub", b, 0600)
-	assert.NoError(t, err)
+	t.Run("nums of resource matched, one of them has no label, should allow and no label", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceService, "delete")
+		assert.True(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("resource not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceRole, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+	t.Run("Verb not matched, should not allow", func(t *testing.T) {
+		allow, labelList := rbac.GetLabel(perms, rbac.ResourceAccount, "delete")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	archaius.Set(rbac.InitPassword, "Complicated_password1")
+func TestGetLabelFromSinglePerm(t *testing.T) {
+	t.Run("resource and verb match, should allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "b"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.True(t, allow)
+		assert.Equal(t, 1, len(labelList))
+		assert.Equal(t, "b", labelList[0]["a"])
+	})
 
-	dao.DeleteAccount(context.Background(), "root")
-	dao.DeleteAccount(context.Background(), "a")
-	dao.DeleteAccount(context.Background(), "b")
+	t.Run("resource not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"*"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceService, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
 
-	rbac.Init()
-	a, err := dao.GetAccount(context.Background(), "root")
-	assert.NoError(t, err)
-	assert.Equal(t, "root", a.Name)
+	t.Run("verb not match, should no allow", func(t *testing.T) {
+		perms := &rbacModel.Permission{
+			Resources: []*rbacModel.Resource{
+				{
+					Type:   rbac.ResourceAccount,
+					Labels: map[string]string{"a": "a"},
+				},
+			},
+			Verbs: []string{"get"},
+		}
+		allow, labelList := rbac.GetLabelFromSinglePerm(perms, rbac.ResourceAccount, "create")
+		assert.False(t, allow)
+		assert.Equal(t, 0, len(labelList))
+	})
+}
 
-	t.Run("admin can operate any resource", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"admin"}, "default", "account", "create")
-		assert.True(t, ok)
-		ok, _ = rbac.Allow(context.TODO(), []string{"admin"}, "default", "service", "create")
-		assert.True(t, ok)
+func TestLabelMatched(t *testing.T) {
+	targetResourceLabel := map[string]string{
+		"a": "b",
+		"c": "d",
+	}
+	t.Run("value not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"a": "h",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can not operate account", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "account", "create")
-		assert.False(t, ok)
+	t.Run("key not match, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"h": "b",
+		}
+		assert.False(t, rbac.LabelMatched(targetResourceLabel, permResourceLabel))
 	})
-	t.Run("developer can operate service", func(t *testing.T) {
-		ok, _ := rbac.Allow(context.TODO(), []string{"developer"}, "default", "service", "create")
-		assert.True(t, ok)
+	t.Run("target resource label matches no permission resource label, should not match", func(t *testing.T) {
+		permResourceLabel := map[string]string{
+			"g": "h",

Review comment:
       这些字母真的没法读




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