update code per review comments

Signed-off-by: wang yan <wangyan@vmware.com>

src/common/security/robot/robot.go

@@ -63,13 +63,7 @@ func getAllPolicies(namespace rbac.Namespace) map[string]bool {
 	for _, policy := range project.GetAllPolicies(namespace) {
 		mp[policy.String()] = true
 	}
-
+	scannerPull := &rbac.Policy{Resource: rbac.ResourceRepository, Action: rbac.ActionScannerPull}
-	scannerPull := &rbac.Policy{Resource: rbac.ResourceRepository, Action: rbac.ActionCreate}
+	mp[scannerPull.String()] = true
-	robotExternalPolicies := []*rbac.Policy{}
-	robotExternalPolicies = append(robotExternalPolicies, scannerPull)
-	for _, policy := range robotExternalPolicies {
-		mp[policy.String()] = true
-	}
-
 	return mp
 }

src/core/api/robot.go

@@ -108,7 +108,6 @@ func (r *RobotAPI) Post() {
 		return
 	}
 	robotReq.Visible = true
-	robotReq.ByPassPolicyCheck = false
 	robotReq.ProjectID = r.project.ProjectID
 
 	if err := validateRobotReq(r.project, &robotReq); err != nil {

src/core/middlewares/contenttrust/handler.go

@@ -49,14 +49,10 @@ func (cth contentTrustHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque
 		cth.next.ServeHTTP(rw, req)
 		return
 	}
-	// Token bypass policy check
+	if bypass, ok := util.BypassPolicyCheckFromContext(req.Context()); ok && bypass {
-	if bypassPC := req.Context().Value(util.ByPassPolicyCheckCtxKey); bypassPC != nil {
-		bypassPolicyCheck, ok := bypassPC.(bool)
-		if ok && bypassPolicyCheck {
 		cth.next.ServeHTTP(rw, req)
 		return
 	}
-	}
 	if !util.GetPolicyChecker().ContentTrustEnabled(img.ProjectName) {
 		cth.next.ServeHTTP(rw, req)
 		return

src/core/middlewares/regtoken/handler.go

@@ -1,7 +1,6 @@
 package regtoken
 
 import (
-	"context"
 	"github.com/docker/distribution/registry/auth"
 	"github.com/goharbor/harbor/src/common/rbac"
 	"github.com/goharbor/harbor/src/common/utils/log"
@@ -28,13 +27,7 @@ func New(next http.Handler) http.Handler {
 
 // ServeHTTP ...
 func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
-
+	img, _ := util.ImageInfoFromContext(req.Context())
-	imgRaw := req.Context().Value(util.ImageInfoCtxKey)
-	if imgRaw == nil {
-		r.next.ServeHTTP(rw, req)
-		return
-	}
-	img, _ := req.Context().Value(util.ImageInfoCtxKey).(util.ImageInfo)
 	if img.Digest == "" {
 		r.next.ServeHTTP(rw, req)
 		return
@@ -63,13 +56,10 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		Action: rbac.ActionScannerPull.String(),
 	})
 
-	accessSet := regTK.Claims.(*registry.Claim).GetAccessSet()
+	accessSet := regTK.Claims.(*registry.Claim).GetAccess()
 	for _, access := range accessItems {
 		if accessSet.Contains(access) {
-			ctx := context.WithValue(req.Context(), util.ByPassPolicyCheckCtxKey, true)
+			*req = *(req.WithContext(util.NewBypassPolicyCheckContext(req.Context(), true)))
-			req = req.WithContext(ctx)
-			r.next.ServeHTTP(rw, req)
-			return
 		}
 	}
 	r.next.ServeHTTP(rw, req)

src/core/middlewares/url/handler.go

@@ -37,7 +37,7 @@ func New(next http.Handler) http.Handler {
 
 // ServeHTTP ...
 func (uh urlHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
-	log.Infof("in url handler, path: %s", req.URL.Path)
+	log.Debugf("in url handler, path: %s", req.URL.Path)
 	flag, repository, reference := util.MatchPullManifest(req)
 	if flag {
 		components := strings.SplitN(repository, "/", 2)

src/core/middlewares/util/util.go

@@ -445,6 +445,17 @@ func ManifestInfoFromContext(ctx context.Context) (*ManifestInfo, bool) {
 	return info, ok
 }
 
+// NewBypassPolicyCheckContext returns context with policy check info
+func NewBypassPolicyCheckContext(ctx context.Context, bypass bool) context.Context {
+	return context.WithValue(ctx, ByPassPolicyCheckCtxKey, bypass)
+}
+
+// BypassPolicyCheckFromContext returns whether to bypass policy check
+func BypassPolicyCheckFromContext(ctx context.Context) (bool, bool) {
+	info, ok := ctx.Value(ByPassPolicyCheckCtxKey).(bool)
+	return info, ok
+}
+
 // NewBlobInfoContext returns context with blob info
 func NewBlobInfoContext(ctx context.Context, info *BlobInfo) context.Context {
 	return context.WithValue(ctx, blobInfoKey, info)

src/core/middlewares/vulnerable/handler.go

@@ -52,14 +52,10 @@ func (vh vulnerableHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request)
 		return
 	}
 
-	// Token bypass policy check
+	if bypass, ok := util.BypassPolicyCheckFromContext(req.Context()); ok && bypass {
-	if bypassPC := req.Context().Value(util.ByPassPolicyCheckCtxKey); bypassPC != nil {
-		bypassPolicyCheck, ok := bypassPC.(bool)
-		if ok && bypassPolicyCheck {
 		vh.next.ServeHTTP(rw, req)
 		return
 	}
-	}
 
 	// Is vulnerable policy set?
 	projectVulnerableEnabled, projectVulnerableSeverity, wl := util.GetPolicyChecker().VulnerablePolicy(img.ProjectName)

src/core/service/token/creator.go

@@ -154,7 +154,7 @@ type repositoryFilter struct {
 
 func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManager,
 	a *token.ResourceActions) error {
-	// clear action list to assign to new access element after perm check.
+	// clear action list to assign to new acess element after perm check.
 	img, err := rep.parser.parse(a.Name)
 	if err != nil {
 		return err
@@ -177,12 +177,11 @@ func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManage
 		permission = "RWM"
 	} else if ctx.Can(rbac.ActionPush, resource) {
 		permission = "RW"
+	} else if ctx.Can(rbac.ActionScannerPull, resource) {
+		permission = "RS"
 	} else if ctx.Can(rbac.ActionPull, resource) {
 		permission = "R"
 	}
-	if ctx.Can(rbac.ActionScannerPull, resource) {
-		permission = fmt.Sprintf("%s%s", permission, "S")
-	}
 
 	a.Actions = permToActions(permission)
 	return nil

src/pkg/robot/controller.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"github.com/dgrijalva/jwt-go"
 	"github.com/goharbor/harbor/src/common"
-	"github.com/goharbor/harbor/src/common/rbac"
 	"github.com/goharbor/harbor/src/common/utils/log"
 	"github.com/goharbor/harbor/src/core/config"
 	"github.com/goharbor/harbor/src/pkg/q"
@@ -72,12 +71,6 @@ func (d *DefaultAPIController) CreateRobotAccount(robotReq *model.RobotCreate) (
 		ExpiresAt:   expiresAt,
 		Visible:     robotReq.Visible,
 	}
-	if robotReq.ByPassPolicyCheck {
-		robotReq.Access = append(robotReq.Access, &rbac.Policy{
-			Resource: rbac.NewProjectNamespace(robotReq.ProjectID).Resource(rbac.ResourceRepository),
-			Action:   rbac.ActionScannerPull,
-		})
-	}
 	id, err := d.manager.CreateRobotAccount(robot)
 	if err != nil {
 		return nil, err

src/pkg/robot/model/robot.go

@@ -50,7 +50,6 @@ type RobotCreate struct {
 	Description string         `json:"description"`
 	Disabled    bool           `json:"disabled"`
 	Visible     bool           `json:"-"`
-	ByPassPolicyCheck bool           `json:"-"`
 	Access      []*rbac.Policy `json:"access"`
 }
 

src/pkg/token/claims/registry/accesses.go

@@ -0,0 +1,49 @@
+package registry
+
+import (
+	"github.com/docker/distribution/registry/auth"
+)
+
+// Accesses ...
+type Accesses map[auth.Resource]actions
+
+// Contains ...
+func (s Accesses) Contains(access auth.Access) bool {
+	actionSet, ok := s[access.Resource]
+	if ok {
+		return actionSet.contains(access.Action)
+	}
+
+	return false
+}
+
+type actions struct {
+	stringSet
+}
+
+func newActions(set ...string) actions {
+	return actions{newStringSet(set...)}
+}
+
+func (s actions) contains(action string) bool {
+	return s.stringSet.contains(action)
+}
+
+type stringSet map[string]struct{}
+
+func newStringSet(keys ...string) stringSet {
+	ss := make(stringSet, len(keys))
+	ss.add(keys...)
+	return ss
+}
+
+func (ss stringSet) add(keys ...string) {
+	for _, key := range keys {
+		ss[key] = struct{}{}
+	}
+}
+
+func (ss stringSet) contains(key string) bool {
+	_, ok := ss[key]
+	return ok
+}

src/pkg/token/claims/registry/accessset.go

@@ -1,18 +0,0 @@
-package registry
-
-import (
-	"github.com/docker/distribution/registry/auth"
-)
-
-// AccessSet ...
-type AccessSet map[auth.Resource]actionSet
-
-// Contains ...
-func (s AccessSet) Contains(access auth.Access) bool {
-	actionSet, ok := s[access.Resource]
-	if ok {
-		return actionSet.contains(access.Action)
-	}
-
-	return false
-}

src/pkg/token/claims/registry/actionset.go

@@ -1,14 +0,0 @@
-package registry
-
-// actionSet is a special type of stringSet.
-type actionSet struct {
-	stringSet
-}
-
-func newActionSet(actions ...string) actionSet {
-	return actionSet{newStringSet(actions...)}
-}
-
-func (s actionSet) contains(action string) bool {
-	return s.stringSet.contains(action)
-}

src/pkg/token/claims/registry/registry.go

@@ -17,22 +17,22 @@ func (rc *Claim) Valid() error {
 	return rc.StandardClaims.Valid()
 }
 
-// GetAccessSet ...
+// GetAccess ...
-func (rc *Claim) GetAccessSet() AccessSet {
+func (rc *Claim) GetAccess() Accesses {
-	accessSet := make(AccessSet, len(rc.Access))
+	accesses := make(Accesses, len(rc.Access))
 	for _, resourceActions := range rc.Access {
 		resource := auth.Resource{
 			Type: resourceActions.Type,
 			Name: resourceActions.Name,
 		}
-		set, exists := accessSet[resource]
+		set, exists := accesses[resource]
 		if !exists {
-			set = newActionSet()
+			set = newActions()
-			accessSet[resource] = set
+			accesses[resource] = set
 		}
 		for _, action := range resourceActions.Actions {
 			set.add(action)
 		}
 	}
-	return accessSet
+	return accesses
 }

src/pkg/token/claims/registry/registry_test.go

@@ -48,7 +48,7 @@ func TestGetAccessSet(t *testing.T) {
 		},
 		Action: rbac.ActionScannerPull.String(),
 	}
-	set := rClaims.GetAccessSet()
+	set := rClaims.GetAccess()
 	assert.True(t, set.Contains(auth1))
 	assert.False(t, set.Contains(auth2))
 }

src/pkg/token/claims/registry/stringset.go

@@ -1,21 +0,0 @@
-package registry
-
-// StringSet is a useful type for looking up strings.
-type stringSet map[string]struct{}
-
-func newStringSet(keys ...string) stringSet {
-	ss := make(stringSet, len(keys))
-	ss.add(keys...)
-	return ss
-}
-
-func (ss stringSet) add(keys ...string) {
-	for _, key := range keys {
-		ss[key] = struct{}{}
-	}
-}
-
-func (ss stringSet) contains(key string) bool {
-	_, ok := ss[key]
-	return ok
-}