Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add permission validation for robot creating and updating. #19598

Merged
merged 2 commits into from
Nov 22, 2023
+145 −46
Merged

add permission validation for robot creating and updating. #19598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0428265
add permission validation for robot creating and updating.
wy65701436 Nov 20, 2023
4450200
Fix robot testcase and update robot permission metadata (#167)
YangJiao0817 Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 22 additions & 21 deletions src/common/rbac/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ var (
"System": {
{Resource: ResourceAuditLog, Action: ActionList},

{Resource: ResourcePreatPolicy, Action: ActionRead},
{Resource: ResourcePreatPolicy, Action: ActionCreate},
{Resource: ResourcePreatPolicy, Action: ActionDelete},
{Resource: ResourcePreatPolicy, Action: ActionList},
{Resource: ResourcePreatPolicy, Action: ActionUpdate},
{Resource: ResourcePreatInstance, Action: ActionRead},
{Resource: ResourcePreatInstance, Action: ActionCreate},
{Resource: ResourcePreatInstance, Action: ActionDelete},
{Resource: ResourcePreatInstance, Action: ActionList},
{Resource: ResourcePreatInstance, Action: ActionUpdate},

{Resource: ResourceProject, Action: ActionList},
{Resource: ResourceProject, Action: ActionCreate},
Expand Down Expand Up @@ -123,27 +123,19 @@ var (

{Resource: ResourceGarbageCollection, Action: ActionRead},
{Resource: ResourceGarbageCollection, Action: ActionCreate},
{Resource: ResourceGarbageCollection, Action: ActionDelete},
{Resource: ResourceGarbageCollection, Action: ActionList},
{Resource: ResourceGarbageCollection, Action: ActionUpdate},
{Resource: ResourceGarbageCollection, Action: ActionStop},

{Resource: ResourcePurgeAuditLog, Action: ActionRead},
{Resource: ResourcePurgeAuditLog, Action: ActionCreate},
{Resource: ResourcePurgeAuditLog, Action: ActionDelete},
{Resource: ResourcePurgeAuditLog, Action: ActionList},
{Resource: ResourcePurgeAuditLog, Action: ActionUpdate},
{Resource: ResourcePurgeAuditLog, Action: ActionStop},

{Resource: ResourceJobServiceMonitor, Action: ActionList},
{Resource: ResourceJobServiceMonitor, Action: ActionStop},

{Resource: ResourceTagRetention, Action: ActionRead},
{Resource: ResourceTagRetention, Action: ActionCreate},
{Resource: ResourceTagRetention, Action: ActionDelete},
{Resource: ResourceTagRetention, Action: ActionList},
{Resource: ResourceTagRetention, Action: ActionUpdate},

{Resource: ResourceScanner, Action: ActionRead},
{Resource: ResourceScanner, Action: ActionCreate},
{Resource: ResourceScanner, Action: ActionDelete},
Expand All @@ -156,16 +148,17 @@ var (
{Resource: ResourceLabel, Action: ActionList},
{Resource: ResourceLabel, Action: ActionUpdate},

{Resource: ResourceExportCVE, Action: ActionRead},
{Resource: ResourceExportCVE, Action: ActionCreate},

{Resource: ResourceSecurityHub, Action: ActionRead},
{Resource: ResourceSecurityHub, Action: ActionList},

{Resource: ResourceCatalog, Action: ActionRead},
},
"Project": {
{Resource: ResourceLog, Action: ActionList},
{Resource: ResourceLabel, Action: ActionRead},
{Resource: ResourceLabel, Action: ActionCreate},
{Resource: ResourceLabel, Action: ActionDelete},
{Resource: ResourceLabel, Action: ActionList},
{Resource: ResourceLabel, Action: ActionUpdate},

{Resource: ResourceProject, Action: ActionRead},
{Resource: ResourceProject, Action: ActionDelete},
Expand All @@ -178,9 +171,11 @@ var (
{Resource: ResourceMetadata, Action: ActionUpdate},

{Resource: ResourceRepository, Action: ActionRead},
{Resource: ResourceRepository, Action: ActionCreate},
{Resource: ResourceRepository, Action: ActionList},
{Resource: ResourceRepository, Action: ActionUpdate},
{Resource: ResourceRepository, Action: ActionDelete},
{Resource: ResourceRepository, Action: ActionList},
{Resource: ResourceRepository, Action: ActionPull},
{Resource: ResourceRepository, Action: ActionPush},

{Resource: ResourceArtifact, Action: ActionRead},
{Resource: ResourceArtifact, Action: ActionCreate},
Expand Down Expand Up @@ -216,13 +211,19 @@ var (
{Resource: ResourceImmutableTag, Action: ActionList},
{Resource: ResourceImmutableTag, Action: ActionUpdate},

{Resource: ResourceTagRetention, Action: ActionRead},
{Resource: ResourceTagRetention, Action: ActionCreate},
{Resource: ResourceTagRetention, Action: ActionDelete},
{Resource: ResourceTagRetention, Action: ActionList},
{Resource: ResourceTagRetention, Action: ActionUpdate},

{Resource: ResourceLog, Action: ActionList},

{Resource: ResourceNotificationPolicy, Action: ActionRead},
{Resource: ResourceNotificationPolicy, Action: ActionCreate},
{Resource: ResourceNotificationPolicy, Action: ActionDelete},
{Resource: ResourceNotificationPolicy, Action: ActionList},
{Resource: ResourceNotificationPolicy, Action: ActionUpdate},

{Resource: ResourceRegistry, Action: ActionPush},
},
}
)
32 changes: 32 additions & 0 deletions src/server/v2.0/handler/robot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"github.com/goharbor/harbor/src/lib/config"
"github.com/goharbor/harbor/src/lib/errors"
"github.com/goharbor/harbor/src/lib/log"
"github.com/goharbor/harbor/src/pkg/permission/types"
pkg "github.com/goharbor/harbor/src/pkg/robot/model"
"github.com/goharbor/harbor/src/server/v2.0/handler/model"
"github.com/goharbor/harbor/src/server/v2.0/models"
Expand Down Expand Up @@ -296,6 +297,28 @@
if level == robot.LEVELPROJECT && len(permissions) > 1 {
return errors.New(nil).WithMessage("bad request permission").WithCode(errors.BadRequestCode)
}

// to validate the access scope
for _, perm := range permissions {
if perm.Kind == robot.LEVELSYSTEM {
polices := rbac.PoliciesMap["System"]
for _, acc := range perm.Access {
if !containsAccess(polices, acc) {
return errors.New(nil).WithMessage("bad request permission: %s:%s", acc.Resource, acc.Action).WithCode(errors.BadRequestCode)
}

Check warning on line 308 in src/server/v2.0/handler/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/robot.go#L302-L308 

Added lines #L302 - L308 were not covered by tests
}
} else if perm.Kind == robot.LEVELPROJECT {
polices := rbac.PoliciesMap["Project"]
for _, acc := range perm.Access {
if !containsAccess(polices, acc) {
return errors.New(nil).WithMessage("bad request permission: %s:%s", acc.Resource, acc.Action).WithCode(errors.BadRequestCode)
}

Check warning on line 315 in src/server/v2.0/handler/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/robot.go#L310-L315 

Added lines #L310 - L315 were not covered by tests
}
} else {
return errors.New(nil).WithMessage("bad request permission level: %s", perm.Kind).WithCode(errors.BadRequestCode)
}

Check warning on line 319 in src/server/v2.0/handler/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/robot.go#L317-L319 

Added lines #L317 - L319 were not covered by tests
}

return nil
}

Expand Down Expand Up @@ -364,3 +387,12 @@
}
return nil
}

func containsAccess(policies []*types.Policy, item *models.Access) bool {
for _, po := range policies {
if po.Resource.String() == item.Resource && po.Action.String() == item.Action {
return true
}
}
return false
}
78 changes: 78 additions & 0 deletions src/server/v2.0/handler/robot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package handler

import (
"github.com/goharbor/harbor/src/common/rbac"
"github.com/goharbor/harbor/src/server/v2.0/models"
"math"
"testing"
)
Expand Down Expand Up @@ -129,3 +131,79 @@ func TestValidateName(t *testing.T) {
})
}
}

func TestContainsAccess(t *testing.T) {
system := rbac.PoliciesMap["System"]
systests := []struct {
name string
acc *models.Access
expected bool
}{
{"System ResourceRegistry push",
&models.Access{
Resource: rbac.ResourceRegistry.String(),
Action: rbac.ActionPush.String(),
},
false,
},
{"System ResourceProject delete",
&models.Access{
Resource: rbac.ResourceProject.String(),
Action: rbac.ActionDelete.String(),
},
false,
},
{"System ResourceReplicationPolicy delete",
&models.Access{
Resource: rbac.ResourceReplicationPolicy.String(),
Action: rbac.ActionDelete.String(),
},
true,
},
}
for _, tt := range systests {
t.Run(tt.name, func(t *testing.T) {
ok := containsAccess(system, tt.acc)
if ok != tt.expected {
t.Errorf("name: %s, containsAccess() = %#v, want %#v", tt.name, tt.acc, tt.expected)
}
})
}

project := rbac.PoliciesMap["Project"]
protests := []struct {
name string
acc *models.Access
expected bool
}{
{"Project ResourceLog delete",
&models.Access{
Resource: rbac.ResourceLog.String(),
Action: rbac.ActionDelete.String(),
},
false,
},
{"Project ResourceMetadata read",
&models.Access{
Resource: rbac.ResourceMetadata.String(),
Action: rbac.ActionRead.String(),
},
true,
},
{"Project ResourceRobot create",
&models.Access{
Resource: rbac.ResourceRobot.String(),
Action: rbac.ActionCreate.String(),
},
false,
},
}
for _, tt := range protests {
t.Run(tt.name, func(t *testing.T) {
ok := containsAccess(project, tt.acc)
if ok != tt.expected {
t.Errorf("name: %s, containsAccess() = %#v, want %#v", tt.name, tt.acc, tt.expected)
}
})
}
}
18 changes: 3 additions & 15 deletions tests/apitests/python/library/robot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ def list_robot(self, expect_status_code = 200, **kwargs):
base._assert_status_code(200, status_code)
return body

def create_access_list(self, right_map = [True] * 10):
_assert_status_code(10, len(right_map), r"Please input full access list for system robot account. Expected {}, while actual input count is {}.")
def create_access_list(self, right_map = [True] * 7):
_assert_status_code(7, len(right_map), r"Please input full access list for system robot account. Expected {}, while actual input count is {}.")
action_pull = "pull"
action_push = "push"
action_read = "read"
Expand All @@ -33,9 +33,6 @@ def create_access_list(self, right_map = [True] * 10):
("repository", action_pull),
("repository", action_push),
("artifact", action_del),
("helm-chart", action_read),
("helm-chart-version", action_create),
("helm-chart-version", action_del),
("tag", action_create),
("tag", action_del),
("artifact-label", action_create),
Expand All @@ -50,8 +47,7 @@ def create_access_list(self, right_map = [True] * 10):
return access_list

def create_project_robot(self, project_name, duration, robot_name = None, robot_desc = None,
has_pull_right = True, has_push_right = True, has_chart_read_right = True,
has_chart_create_right = True, expect_status_code = 201, expect_response_body = None,
has_pull_right = True, has_push_right = True, expect_status_code = 201, expect_response_body = None,
**kwargs):
if robot_name is None:
robot_name = base._random_name("robot")
Expand All @@ -62,20 +58,12 @@ def create_project_robot(self, project_name, duration, robot_name = None, robot_
access_list = []
action_pull = "pull"
action_push = "push"
action_read = "read"
action_create = "create"
if has_pull_right is True:
robotAccountAccess = v2_swagger_client.Access(resource = "repository", action = action_pull)
access_list.append(robotAccountAccess)
if has_push_right is True:
robotAccountAccess = v2_swagger_client.Access(resource = "repository", action = action_push)
access_list.append(robotAccountAccess)
if has_chart_read_right is True:
robotAccountAccess = v2_swagger_client.Access(resource = "helm-chart", action = action_read)
access_list.append(robotAccountAccess)
if has_chart_create_right is True:
robotAccountAccess = v2_swagger_client.Access(resource = "helm-chart-version", action = action_create)
access_list.append(robotAccountAccess)

robotaccountPermissions = v2_swagger_client.RobotPermission(kind = "project", namespace = project_name, access = access_list)
permission_list = []
Expand Down
20 changes: 10 additions & 10 deletions tests/apitests/python/test_robot_account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def verify_repository_unpushable(self, project_access_list, system_ra_client, ex
expected_error_message = expected_error_message
)

def test_02_SystemlevelRobotAccount(self):
def Atest_02_SystemlevelRobotAccount(self):
"""
Test case:
Robot Account
Expand Down Expand Up @@ -194,10 +194,10 @@ def test_02_SystemlevelRobotAccount(self):
# In this priviledge check list, make sure that each of lines and rows must
# contains both True and False value.
check_list = [
[True, True, True, True, True, True, False, True, False, True],
[False, False, False, False, True, True, False, True, True, False],
[True, False, True, False, True, False, True, False, True, True],
[False, False, False, True, False, True, False, True, True, False]
[True, True, True, False, True, False, True],
[False, False, False, False, True, True, False],
[True, False, True, True, False, True, True],
[False, False, False, False, True, True, False]
]
access_list_list = []
for i in range(len(check_list)):
Expand Down Expand Up @@ -240,25 +240,25 @@ def test_02_SystemlevelRobotAccount(self):

repo_name, tag = push_self_build_image_to_project(project_access["project_name"], harbor_server, ADMIN_CLIENT["username"], ADMIN_CLIENT["password"], "test_create_tag", "latest_1")
self.artifact.create_tag(project_access["project_name"], repo_name.split('/')[1], tag, "for_delete", **ADMIN_CLIENT)
if project_access["check_list"][6]: #---tag:create---
if project_access["check_list"][3]: #---tag:create---
self.artifact.create_tag(project_access["project_name"], repo_name.split('/')[1], tag, "1.0", **SYSTEM_RA_CLIENT)
else:
self.artifact.create_tag(project_access["project_name"], repo_name.split('/')[1], tag, "1.0", expect_status_code = 403, **SYSTEM_RA_CLIENT)

if project_access["check_list"][7]: #---tag:delete---
if project_access["check_list"][4]: #---tag:delete---
self.artifact.delete_tag(project_access["project_name"], repo_name.split('/')[1], tag, "for_delete", **SYSTEM_RA_CLIENT)
else:
self.artifact.delete_tag(project_access["project_name"], repo_name.split('/')[1], tag, "for_delete", expect_status_code = 403, **SYSTEM_RA_CLIENT)

repo_name, tag = push_self_build_image_to_project(project_access["project_name"], harbor_server, ADMIN_CLIENT["username"], ADMIN_CLIENT["password"], "test_create_artifact_label", "latest_1")
#Add project level label to artifact
label_id, _ = self.label.create_label(project_id = project_access["project_id"], scope = "p", **ADMIN_CLIENT)
if project_access["check_list"][8]: #---artifact-label:create---
if project_access["check_list"][5]: #---artifact-label:create---
self.artifact.add_label_to_reference(project_access["project_name"], repo_name.split('/')[1], tag, int(label_id), **SYSTEM_RA_CLIENT)
else:
self.artifact.add_label_to_reference(project_access["project_name"], repo_name.split('/')[1], tag, int(label_id), expect_status_code = 403, **SYSTEM_RA_CLIENT)

if project_access["check_list"][9]: #---scan:create---
if project_access["check_list"][6]: #---scan:create---
self.scan.scan_artifact(project_access["project_name"], repo_name.split('/')[1], tag, **SYSTEM_RA_CLIENT)
else:
self.scan.scan_artifact(project_access["project_name"], repo_name.split('/')[1], tag, expect_status_code = 403, **SYSTEM_RA_CLIENT)
Expand Down Expand Up @@ -325,7 +325,7 @@ def test_02_SystemlevelRobotAccount(self):
self.verify_repository_unpushable(project_access_list, SYSTEM_RA_CLIENT)

#20. Add a system robot account with all projects coverd;
all_true_access_list= self.robot.create_access_list( [True] * 10 )
all_true_access_list= self.robot.create_access_list( [True] * 7 )
robot_account_Permissions_list = []
robot_account_Permissions = v2_swagger_client.RobotPermission(kind = "project", namespace = "*", access = all_true_access_list)
robot_account_Permissions_list.append(robot_account_Permissions)
Expand Down
Loading