Merge pull request #9280 from heww/robot-acoount-improvement

fix(robot): robot account improvement

src/common/rbac/rbac.go

@@ -110,6 +110,10 @@ func (p *Policy) GetEffect() string {
 	return eft.String()
 }
 
+func (p *Policy) String() string {
+	return p.Resource.String() + ":" + p.Action.String() + ":" + p.GetEffect()
+}
+
 // Role the interface of rbac role
 type Role interface {
 	// GetRoleName returns the role identity, if empty string role's policies will be ignore

src/common/security/robot/robot.go

@@ -9,7 +9,7 @@ import (
 type robot struct {
 	username  string
 	namespace rbac.Namespace
-	policy    []*rbac.Policy
+	policies  []*rbac.Policy
 }
 
 // GetUserName get the robot name.
@@ -23,7 +23,7 @@ func (r *robot) GetPolicies() []*rbac.Policy {
 	if r.namespace.IsPublic() {
 		policies = append(policies, project.PoliciesForPublicProject(r.namespace)...)
 	}
-	policies = append(policies, r.policy...)
+	policies = append(policies, r.policies...)
 	return policies
 }
 
@@ -33,10 +33,30 @@ func (r *robot) GetRoles() []rbac.Role {
 }
 
 // NewRobot ...
-func NewRobot(username string, namespace rbac.Namespace, policy []*rbac.Policy) rbac.User {
+func NewRobot(username string, namespace rbac.Namespace, policies []*rbac.Policy) rbac.User {
 	return &robot{
 		username:  username,
 		namespace: namespace,
-		policy:    policy,
+		policies:  filterPolicies(namespace, policies),
 	}
 }
+
+func filterPolicies(namespace rbac.Namespace, policies []*rbac.Policy) []*rbac.Policy {
+	var results []*rbac.Policy
+	if len(policies) == 0 {
+		return results
+	}
+
+	mp := map[string]bool{}
+	for _, policy := range project.GetAllPolicies(namespace) {
+		mp[policy.String()] = true
+	}
+
+	for _, policy := range policies {
+		if mp[policy.String()] {
+			results = append(results, policy)
+		}
+	}
+
+	return results
+}

src/common/security/robot/robot_test.go

@@ -33,10 +33,21 @@ func TestGetPolicies(t *testing.T) {
 	robot := robot{
 		username:  "test",
 		namespace: rbac.NewProjectNamespace(1, false),
-		policy:    policies,
+		policies:  policies,
 	}
 
 	assert.Equal(t, robot.GetUserName(), "test")
 	assert.NotNil(t, robot.GetPolicies())
 	assert.Nil(t, robot.GetRoles())
 }
+
+func TestNewRobot(t *testing.T) {
+	policies := []*rbac.Policy{
+		{Resource: "/project/1/repository", Action: "pull"},
+		{Resource: "/project/library/repository", Action: "pull"},
+		{Resource: "/project/library/repository", Action: "push"},
+	}
+
+	robot := NewRobot("test", rbac.NewProjectNamespace(1, false), policies)
+	assert.Len(t, robot.GetPolicies(), 1)
+}

src/core/api/robot.go

@@ -25,6 +25,7 @@ import (
 	"github.com/goharbor/harbor/src/common/dao"
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/rbac"
+	"github.com/goharbor/harbor/src/common/rbac/project"
 	"github.com/goharbor/harbor/src/common/token"
 	"github.com/goharbor/harbor/src/core/config"
 )
@@ -107,6 +108,11 @@ func (r *RobotAPI) Post() {
 		return
 	}
 
+	if err := validateRobotReq(r.project, &robotReq); err != nil {
+		r.SendBadRequestError(err)
+		return
+	}
+
 	// Token duration in minutes
 	tokenDuration := time.Duration(config.RobotTokenDuration()) * time.Minute
 	expiresAt := time.Now().UTC().Add(tokenDuration).Unix()
@@ -254,3 +260,25 @@ func (r *RobotAPI) Delete() {
 		return
 	}
 }
+
+func validateRobotReq(p *models.Project, robotReq *models.RobotReq) error {
+	if len(robotReq.Access) == 0 {
+		return errors.New("access required")
+	}
+
+	namespace, _ := rbac.Resource(fmt.Sprintf("/project/%d", p.ProjectID)).GetNamespace()
+	policies := project.GetAllPolicies(namespace)
+
+	mp := map[string]bool{}
+	for _, policy := range policies {
+		mp[policy.String()] = true
+	}
+
+	for _, policy := range robotReq.Access {
+		if !mp[policy.String()] {
+			return fmt.Errorf("%s action of %s resource not exist in project %s", policy.Action, policy.Resource, p.Name)
+		}
+	}
+
+	return nil
+}

src/core/api/robot_test.go

@@ -16,10 +16,11 @@ package api
 
 import (
 	"fmt"
-	"github.com/goharbor/harbor/src/common/models"
-	"github.com/goharbor/harbor/src/common/rbac"
 	"net/http"
 	"testing"
+
+	"github.com/goharbor/harbor/src/common/models"
+	"github.com/goharbor/harbor/src/common/rbac"
 )
 
 var (
@@ -28,9 +29,10 @@ var (
 )
 
 func TestRobotAPIPost(t *testing.T) {
+	res := rbac.Resource("/project/1")
 
 	rbacPolicy := &rbac.Policy{
-		Resource: "/project/libray/repository",
+		Resource: res.Subresource(rbac.ResourceRepository),
 		Action:   "pull",
 	}
 	policies := []*rbac.Policy{}
@@ -83,6 +85,51 @@ func TestRobotAPIPost(t *testing.T) {
 			},
 			code: http.StatusBadRequest,
 		},
+		{
+			request: &testingRequest{
+				method: http.MethodPost,
+				url:    robotPath,
+				bodyJSON: &models.RobotReq{
+					Name:        "test",
+					Description: "resource not exist",
+					Access: []*rbac.Policy{
+						{Resource: res.Subresource("foo"), Action: rbac.ActionCreate},
+					},
+				},
+				credential: projAdmin4Robot,
+			},
+			code: http.StatusBadRequest,
+		},
+		{
+			request: &testingRequest{
+				method: http.MethodPost,
+				url:    robotPath,
+				bodyJSON: &models.RobotReq{
+					Name:        "test",
+					Description: "action not exist",
+					Access: []*rbac.Policy{
+						{Resource: res.Subresource(rbac.ResourceRepository), Action: "foo"},
+					},
+				},
+				credential: projAdmin4Robot,
+			},
+			code: http.StatusBadRequest,
+		},
+		{
+			request: &testingRequest{
+				method: http.MethodPost,
+				url:    robotPath,
+				bodyJSON: &models.RobotReq{
+					Name:        "test",
+					Description: "policy not exit",
+					Access: []*rbac.Policy{
+						{Resource: res.Subresource(rbac.ResourceMember), Action: rbac.ActionPush},
+					},
+				},
+				credential: projAdmin4Robot,
+			},
+			code: http.StatusBadRequest,
+		},
 		// 403 -- developer
 		{
 			request: &testingRequest{

tests/apitests/python/library/project.py

@@ -182,19 +182,14 @@ class Project(base.Base):
             has_pull_right = True
         access_list = []
         resource_by_project_id = "/project/"+str(project_id)+"/repository"
-        resource_by_project_name = "/project/"+project_name+"/repository"
         action_pull = "pull"
         action_push = "push"
         if has_pull_right is True:
             robotAccountAccess = swagger_client.RobotAccountAccess(resource = resource_by_project_id, action = action_pull)
             access_list.append(robotAccountAccess)
-            robotAccountAccess = swagger_client.RobotAccountAccess(resource = resource_by_project_name, action = action_pull)
-            access_list.append(robotAccountAccess)
         if has_push_right is True:
             robotAccountAccess = swagger_client.RobotAccountAccess(resource = resource_by_project_id, action = action_push)
             access_list.append(robotAccountAccess)
-            robotAccountAccess = swagger_client.RobotAccountAccess(resource = resource_by_project_name, action = action_push)
-            access_list.append(robotAccountAccess)
         robotAccountCreate = swagger_client.RobotAccountCreate(robot_name, robot_desc, access_list)
         client = self._get_client(**kwargs)
         data = []