Skip to content

Commit

Permalink
fix
Browse files Browse the repository at this point in the history 
Signed-off-by: wang yan <wangyan@vmware.com>
  • Loading branch information
@wy65701436
wy65701436 committed Sep 25, 2024
1 parent 740985f commit 9656d3e
Show file tree
Hide file tree
Showing 2 changed files with 207 additions and 27 deletions.
47 changes: 22 additions & 25 deletions src/server/v2.0/handler/robot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,40 +67,39 @@ func (rAPI *robotAPI) CreateRobot(ctx context.Context, params operation.CreateRo
return rAPI.SendError(ctx, err)
}

var creatorRef int64
switch s := sc.(type) {
case *local.SecurityContext:
creatorRef = int64(s.User().UserID)
case *robotSc.SecurityContext:
creatorRef = s.User().ID
default:
return rAPI.SendError(ctx, errors.New(nil).WithMessage("invalid security context"))
}

r := &robot.Robot{
Robot: pkg.Robot{
Name: params.Robot.Name,
Description: params.Robot.Description,
Duration: params.Robot.Duration,
Visible: true,
CreatorRef: creatorRef,
CreatorType: sc.Name(),
},
Level: params.Robot.Level,
ProjectNameOrID: params.Robot.Permissions[0].Namespace,
// a robot account cannot assign permissions to another robot account that exceed its own.
}

if err := robot.SetProject(ctx, r); err != nil {
if err := rAPI.requireAccess(ctx, r, rbac.ActionCreate); err != nil {
return rAPI.SendError(ctx, err)
}

if err := rAPI.requireAccess(ctx, r, rbac.ActionCreate); err != nil {
var creatorRef int64
switch s := sc.(type) {
case *local.SecurityContext:
creatorRef = int64(s.User().UserID)
case *robotSc.SecurityContext:
creatorRef = s.User().ID
default:
return rAPI.SendError(ctx, errors.New(nil).WithMessage("invalid security context"))
}
r.CreatorType = sc.Name()
r.CreatorRef = creatorRef

if err := robot.SetProject(ctx, r); err != nil {
return rAPI.SendError(ctx, err)
}

if _, ok := sc.(*robotSc.SecurityContext); ok {
robots, err := rAPI.robotCtl.List(ctx, q.New(q.KeyWords{
creatorRobots, err := rAPI.robotCtl.List(ctx, q.New(q.KeyWords{
"name": strings.TrimPrefix(sc.GetUsername(), config.RobotPrefix(ctx)),
"project_id": r.ProjectID,
}), &robot.Option{
Expand All @@ -109,12 +108,12 @@ func (rAPI *robotAPI) CreateRobot(ctx context.Context, params operation.CreateRo
if err != nil {
return rAPI.SendError(ctx, err)
}
if len(robots) == 0 {
if len(creatorRobots) == 0 {
return rAPI.SendError(ctx, errors.DeniedError(nil))
}

if !validPermissionScope(params.Robot.Permissions, robots[0].Permissions) {
return rAPI.SendError(ctx, errors.New(nil).WithMessage("the permission scope is valid, it's must be less and equals the the creator robot").WithCode(errors.DENIED))
if !isValidPermissionScope(params.Robot.Permissions, creatorRobots[0].Permissions) {
return rAPI.SendError(ctx, errors.New(nil).WithMessage("permission scope is invalid. It must be equal to or more restrictive than the creator robot's permissions: %s", creatorRobots[0].Name).WithCode(errors.DENIED))
}
}

Expand Down Expand Up @@ -453,8 +452,8 @@ func (rAPI *robotAPI) updateV2Robot(ctx context.Context, params operation.Update
}
}

if !validPermissionScope(params.Robot.Permissions, creatorRobot.Permissions) {
return errors.New(nil).WithMessage("the permission scope is valid, it's must be less and equals the the creator robot").WithCode(errors.DENIED)
if !isValidPermissionScope(params.Robot.Permissions, creatorRobot.Permissions) {
return errors.New(nil).WithMessage("permission scope is invalid. It must be equal to or more restrictive than the creator robot's permissions: %s", creatorRobot.Name).WithCode(errors.DENIED)
}
}

Expand Down Expand Up @@ -493,9 +492,8 @@ func containsAccess(policies []*types.Policy, item *models.Access) bool {
return false
}

// validPermissionScope checks if slice A is a subset of slice B
func validPermissionScope(creating []*models.RobotPermission, creator []*robot.Permission) bool {
// Convert creator into a map for easier lookup
// isValidPermissionScope checks if permission slice A is a subset of permission slice B
func isValidPermissionScope(creating []*models.RobotPermission, creator []*robot.Permission) bool {
creatorMap := make(map[string]*robot.Permission)
for _, creatorPerm := range creator {
key := fmt.Sprintf("%s:%s", creatorPerm.Kind, creatorPerm.Namespace)
Expand All @@ -517,7 +515,6 @@ func validPermissionScope(creating []*models.RobotPermission, creator []*robot.P
return true
}

// Check if each permission in A exists in B with less than or equal access
for _, pCreating := range creating {
key := fmt.Sprintf("%s:%s", pCreating.Kind, pCreating.Namespace)
creatingPerm, found := creatorMap[key]
Expand Down
187 changes: 185 additions & 2 deletions src/server/v2.0/handler/robot_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package handler

import (
"github.com/goharbor/harbor/src/common/rbac"
"github.com/goharbor/harbor/src/server/v2.0/models"
"math"
"testing"

"github.com/stretchr/testify/assert"

"github.com/goharbor/harbor/src/common/rbac"
"github.com/goharbor/harbor/src/controller/robot"
"github.com/goharbor/harbor/src/pkg/permission/types"
"github.com/goharbor/harbor/src/server/v2.0/models"
)

func TestValidLevel(t *testing.T) {
Expand Down Expand Up @@ -207,3 +212,181 @@ func TestContainsAccess(t *testing.T) {
})
}
}

func TestValidPermissionScope(t *testing.T) {
tests := []struct {
name string
creatingPerms []*models.RobotPermission
creatorPerms []*robot.Permission
expected bool
}{
{
name: "Project - subset",
creatingPerms: []*models.RobotPermission{
{
Kind: "project",
Namespace: "testSubset",
Access: []*models.Access{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
creatorPerms: []*robot.Permission{
{
Kind: "project",
Namespace: "testSubset",
Access: []*types.Policy{
{Resource: "repository", Action: "pull", Effect: "allow"},
{Resource: "repository", Action: "push", Effect: "allow"},
},
},
},
expected: true,
},
{
name: "Project - not Subset",
creatingPerms: []*models.RobotPermission{
{
Kind: "project",
Namespace: "testNotSubset",
Access: []*models.Access{
{Resource: "repository", Action: "push", Effect: "allow"},
},
},
},
creatorPerms: []*robot.Permission{
{
Kind: "project",
Namespace: "testNotSubset",
Access: []*types.Policy{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
expected: false,
},
{
name: "Project - equal",
creatingPerms: []*models.RobotPermission{
{
Kind: "project",
Namespace: "library",
Access: []*models.Access{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
creatorPerms: []*robot.Permission{
{
Kind: "project",
Namespace: "library",
Access: []*types.Policy{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
expected: true,
},
{
name: "Project - different",
creatingPerms: []*models.RobotPermission{
{
Kind: "project",
Namespace: "library",
Access: []*models.Access{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
creatorPerms: []*robot.Permission{
{
Kind: "project",
Namespace: "other",
Access: []*types.Policy{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
expected: false,
},
{
name: "Project - empty creator",
creatingPerms: []*models.RobotPermission{
{
Kind: "project",
Namespace: "library",
Access: []*models.Access{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
creatorPerms: []*robot.Permission{},
expected: false,
},
{
name: "Project - empty creating",
creatingPerms: []*models.RobotPermission{},
creatorPerms: []*robot.Permission{
{
Kind: "project",
Namespace: "library",
Access: []*types.Policy{
{Resource: "repository", Action: "pull", Effect: "allow"},
},
},
},
expected: true,
},
{
name: "System - subset",
creatingPerms: []*models.RobotPermission{
{
Kind: "system",
Namespace: "admin",
Access: []*models.Access{
{Resource: "user", Action: "create", Effect: "allow"},
},
},
},
creatorPerms: []*robot.Permission{
{
Kind: "system",
Namespace: "admin",
Access: []*types.Policy{
{Resource: "user", Action: "create", Effect: "allow"},
{Resource: "user", Action: "delete", Effect: "allow"},
},
},
},
expected: true,
},
{
name: "System - not subset",
creatingPerms: []*models.RobotPermission{
{
Kind: "system",
Namespace: "admin",
Access: []*models.Access{
{Resource: "user", Action: "delete", Effect: "allow"},
},
},
},
creatorPerms: []*robot.Permission{
{
Kind: "system",
Namespace: "admin",
Access: []*types.Policy{
{Resource: "user", Action: "create", Effect: "allow"},
},
},
},
expected: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := validPermissionScope(tt.creatingPerms, tt.creatorPerms)
assert.Equal(t, tt.expected, result)
})
}
}

0 comments on commit 9656d3e

Please sign in to comment.