Merge pull request #10643 from ywk253100/200204_auth

Add permission check for artifact related APIs

api/v2.0/swagger.yaml

@@ -15,8 +15,6 @@ consumes:
 securityDefinitions:
   basicAuth:
     type: basic
-security:
-  - basicAuth: []
 paths:
   /projects/{project_name}/repositories/{repository_name}/artifacts:
     get:

src/common/rbac/const.go

@@ -53,7 +53,7 @@ const (
 	ResourceTagRetention               = Resource("tag-retention")
 	ResourceImmutableTag               = Resource("immutable-tag")
 	ResourceRepositoryLabel            = Resource("repository-label")
-	ResourceRepositoryTag              = Resource("repository-tag")
+	ResourceRepositoryTag              = Resource("repository-tag") // TODO remove
 	ResourceRepositoryTagLabel         = Resource("repository-tag-label")
 	ResourceRepositoryTagManifest      = Resource("repository-tag-manifest")
 	ResourceRepositoryTagScanJob       = Resource("repository-tag-scan-job")      // TODO: remove
@@ -62,5 +62,7 @@ const (
 	ResourceNotificationPolicy         = Resource("notification-policy")
 	ResourceScan                       = Resource("scan")
 	ResourceScanner                    = Resource("scanner")
+	ResourceArtifact                   = Resource("artifact")
+	ResourceTag                        = Resource("tag")
 	ResourceSelf                       = Resource("") // subresource for self
 )

src/common/rbac/project/util.go

@@ -48,6 +48,9 @@ var (
 
 		{Resource: rbac.ResourceScan, Action: rbac.ActionRead},
 		{Resource: rbac.ResourceScanner, Action: rbac.ActionRead},
+
+		{Resource: rbac.ResourceArtifact, Action: rbac.ActionRead},
+		{Resource: rbac.ResourceArtifact, Action: rbac.ActionList},
 	}
 
 	// all policies for the projects

src/common/rbac/project/visitor_role.go

@@ -127,6 +127,13 @@ var (
 
 			{Resource: rbac.ResourceScanner, Action: rbac.ActionRead},
 			{Resource: rbac.ResourceScanner, Action: rbac.ActionCreate},
+
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionDelete},
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionList},
+
+			{Resource: rbac.ResourceTag, Action: rbac.ActionCreate},
+			{Resource: rbac.ResourceTag, Action: rbac.ActionDelete},
 		},
 
 		"master": {
@@ -216,6 +223,13 @@ var (
 			{Resource: rbac.ResourceScan, Action: rbac.ActionRead},
 
 			{Resource: rbac.ResourceScanner, Action: rbac.ActionRead},
+
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionDelete},
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionList},
+
+			{Resource: rbac.ResourceTag, Action: rbac.ActionCreate},
+			{Resource: rbac.ResourceTag, Action: rbac.ActionDelete},
 		},
 
 		"developer": {
@@ -272,6 +286,11 @@ var (
 			{Resource: rbac.ResourceScan, Action: rbac.ActionRead},
 
 			{Resource: rbac.ResourceScanner, Action: rbac.ActionRead},
+
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionList},
+
+			{Resource: rbac.ResourceTag, Action: rbac.ActionCreate},
 		},
 
 		"guest": {
@@ -316,6 +335,9 @@ var (
 			{Resource: rbac.ResourceScan, Action: rbac.ActionRead},
 
 			{Resource: rbac.ResourceScanner, Action: rbac.ActionRead},
+
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionList},
 		},
 
 		"limitedGuest": {
@@ -344,6 +366,9 @@ var (
 			{Resource: rbac.ResourceScan, Action: rbac.ActionRead},
 
 			{Resource: rbac.ResourceScanner, Action: rbac.ActionRead},
+
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceArtifact, Action: rbac.ActionList},
 		},
 	}
 )

src/server/v2.0/handler/artifact.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"github.com/go-openapi/runtime/middleware"
 	"github.com/goharbor/harbor/src/api/artifact"
+	"github.com/goharbor/harbor/src/common/rbac"
 	ierror "github.com/goharbor/harbor/src/internal/error"
 	"github.com/goharbor/harbor/src/pkg/project"
 	"github.com/goharbor/harbor/src/pkg/q"
@@ -43,9 +44,10 @@ type artifactAPI struct {
 	repoMgr repository.Manager
 }
 
-// TODO do auth in a separate middleware
-
 func (a *artifactAPI) ListArtifacts(ctx context.Context, params operation.ListArtifactsParams) middleware.Responder {
+	if err := a.RequireProjectAccess(ctx, params.ProjectName, rbac.ActionList, rbac.ResourceArtifact); err != nil {
+		return a.SendError(ctx, err)
+	}
 	// set query
 	query := &q.Query{
 		Keywords: map[string]interface{}{},
@@ -85,6 +87,9 @@ func (a *artifactAPI) ListArtifacts(ctx context.Context, params operation.ListAr
 }
 
 func (a *artifactAPI) GetArtifact(ctx context.Context, params operation.GetArtifactParams) middleware.Responder {
+	if err := a.RequireProjectAccess(ctx, params.ProjectName, rbac.ActionRead, rbac.ResourceArtifact); err != nil {
+		return a.SendError(ctx, err)
+	}
 	// set option
 	option := option(params.WithTag, params.WithImmutableStatus,
 		params.WithLabel, params.WithScanOverview, params.WithSignature)
@@ -98,6 +103,9 @@ func (a *artifactAPI) GetArtifact(ctx context.Context, params operation.GetArtif
 }
 
 func (a *artifactAPI) DeleteArtifact(ctx context.Context, params operation.DeleteArtifactParams) middleware.Responder {
+	if err := a.RequireProjectAccess(ctx, params.ProjectName, rbac.ActionDelete, rbac.ResourceArtifact); err != nil {
+		return a.SendError(ctx, err)
+	}
 	artifact, err := a.artCtl.GetByReference(ctx, fmt.Sprintf("%s/%s", params.ProjectName, params.RepositoryName), params.Reference, nil)
 	if err != nil {
 		return a.SendError(ctx, err)
@@ -109,6 +117,9 @@ func (a *artifactAPI) DeleteArtifact(ctx context.Context, params operation.Delet
 }
 
 func (a *artifactAPI) CreateTag(ctx context.Context, params operation.CreateTagParams) middleware.Responder {
+	if err := a.RequireProjectAccess(ctx, params.ProjectName, rbac.ActionCreate, rbac.ResourceTag); err != nil {
+		return a.SendError(ctx, err)
+	}
 	art, err := a.artCtl.GetByReference(ctx, fmt.Sprintf("%s/%s", params.ProjectName, params.RepositoryName),
 		params.Reference, &artifact.Option{
 			WithTag: true,
@@ -129,6 +140,9 @@ func (a *artifactAPI) CreateTag(ctx context.Context, params operation.CreateTagP
 }
 
 func (a *artifactAPI) DeleteTag(ctx context.Context, params operation.DeleteTagParams) middleware.Responder {
+	if err := a.RequireProjectAccess(ctx, params.ProjectName, rbac.ActionDelete, rbac.ResourceTag); err != nil {
+		return a.SendError(ctx, err)
+	}
 	artifact, err := a.artCtl.GetByReference(ctx, fmt.Sprintf("%s/%s", params.ProjectName, params.RepositoryName),
 		params.Reference, &artifact.Option{
 			WithTag: true,

src/server/v2.0/handler/base.go

@@ -18,6 +18,8 @@ package handler
 
 import (
 	"context"
+	"errors"
+	ierror "github.com/goharbor/harbor/src/internal/error"
 
 	"github.com/go-openapi/runtime/middleware"
 	"github.com/goharbor/harbor/src/common/rbac"
@@ -45,7 +47,7 @@ func (*BaseAPI) SendError(ctx context.Context, err error) middleware.Responder {
 func (*BaseAPI) HasPermission(ctx context.Context, action rbac.Action, resource rbac.Resource) bool {
 	s, ok := security.FromContext(ctx)
 	if !ok {
-		log.Warningf("security not found in the contxt")
+		log.Warningf("security not found in the context")
 		return false
 	}
 
@@ -77,3 +79,19 @@ func (b *BaseAPI) HasProjectPermission(ctx context.Context, projectIDOrName inte
 	resource := rbac.NewProjectNamespace(projectID).Resource(subresource...)
 	return b.HasPermission(ctx, action, resource)
 }
+
+// RequireProjectAccess checks the permission against the resources according to the context
+// An error will be returned if it doesn't meet the requirement
+func (b *BaseAPI) RequireProjectAccess(ctx context.Context, projectIDOrName interface{}, action rbac.Action, subresource ...rbac.Resource) error {
+	if b.HasProjectPermission(ctx, projectIDOrName, action, subresource...) {
+		return nil
+	}
+	secCtx, ok := security.FromContext(ctx)
+	if !ok {
+		return ierror.UnauthorizedError(errors.New("security context not found"))
+	}
+	if !secCtx.IsAuthenticated() {
+		return ierror.UnauthorizedError(nil)
+	}
+	return ierror.ForbiddenError(nil)
+}