fix per comments

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

src/common/rbac/const.go

@@ -29,7 +29,7 @@ const (
 	ActionList   = Action("list")
 
 	ActionOperate     = Action("operate")
-	ActionScannerPull = Action("scannerpull") // for robot account created by scanner to pull image, bypass the policy check
+	ActionScannerPull = Action("scanner-pull") // for robot account created by scanner to pull image, bypass the policy check
 )
 
 // const resource variables

src/common/security/robot/robot.go

@@ -47,11 +47,7 @@ func filterPolicies(namespace rbac.Namespace, policies []*rbac.Policy) []*rbac.P
 		return results
 	}
 
-	mp := map[string]bool{}
-	for _, policy := range project.GetAllPolicies(namespace) {
-		mp[policy.String()] = true
-	}
-
+	mp := getAllPolicies(namespace)
 	for _, policy := range policies {
 		if mp[policy.String()] {
 			results = append(results, policy)
@@ -60,3 +56,20 @@ func filterPolicies(namespace rbac.Namespace, policies []*rbac.Policy) []*rbac.P
 
 	return results
 }
+
+// getAllPolicies gets all of supported policies supported in project and external policies supported for robot account
+func getAllPolicies(namespace rbac.Namespace) map[string]bool {
+	mp := map[string]bool{}
+	for _, policy := range project.GetAllPolicies(namespace) {
+		mp[policy.String()] = true
+	}
+
+	scannerPull := &rbac.Policy{Resource: rbac.ResourceRepository, Action: rbac.ActionCreate}
+	robotExternalPolicies := []*rbac.Policy{}
+	robotExternalPolicies = append(robotExternalPolicies, scannerPull)
+	for _, policy := range robotExternalPolicies {
+		mp[policy.String()] = true
+	}
+
+	return mp
+}

src/common/security/robot/robot_test.go

@@ -44,10 +44,11 @@ func TestGetPolicies(t *testing.T) {
 func TestNewRobot(t *testing.T) {
 	policies := []*rbac.Policy{
 		{Resource: "/project/1/repository", Action: "pull"},
+		{Resource: "/project/1/repository", Action: "scanner-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)
+	assert.Len(t, robot.GetPolicies(), 2)
 }

src/core/middlewares/regtoken/handler.go

@@ -5,7 +5,6 @@ import (
 	"github.com/docker/distribution/registry/auth"
 	"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/core/middlewares/util"
 	pkg_token "github.com/goharbor/harbor/src/pkg/token"
 	"github.com/goharbor/harbor/src/pkg/token/claims/registry"
@@ -31,7 +30,7 @@ func New(next http.Handler) http.Handler {
 func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 
 	imgRaw := req.Context().Value(util.ImageInfoCtxKey)
-	if imgRaw == nil || !config.WithClair() {
+	if imgRaw == nil {
 		r.next.ServeHTTP(rw, req)
 		return
 	}
@@ -50,7 +49,7 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	opt := pkg_token.DefaultTokenOptions()
 	regTK, err := pkg_token.Parse(opt, rawToken, &registry.Claim{})
 	if err != nil {
-		log.Debugf("failed to decode reg token: %v, the error is skipped and round the request to native registry.", err)
+		log.Errorf("failed to decode reg token: %v, the error is skipped and round the request to native registry.", err)
 		r.next.ServeHTTP(rw, req)
 		return
 	}
@@ -58,7 +57,7 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	accessItems := []auth.Access{}
 	accessItems = append(accessItems, auth.Access{
 		Resource: auth.Resource{
-			Type: "repository",
+			Type: rbac.ResourceRepository.String(),
 			Name: img.Repository,
 		},
 		Action: rbac.ActionScannerPull.String(),

src/core/middlewares/regtoken/handler_test.go

@@ -0,0 +1,55 @@
+package regtoken
+
+import (
+	"fmt"
+	"github.com/goharbor/harbor/src/core/middlewares/util"
+	"github.com/stretchr/testify/suite"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+)
+
+type HandlerSuite struct {
+	suite.Suite
+}
+
+func doPullManifestRequest(projectName, name, tag string, next ...http.HandlerFunc) int {
+	repository := fmt.Sprintf("%s/%s", projectName, name)
+
+	url := fmt.Sprintf("/v2/%s/manifests/%s", repository, tag)
+	req, _ := http.NewRequest("GET", url, nil)
+
+	token := "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6IkNWUTc6REM3NTpHVEROOkxTTUs6VUFJTjpIUUVWOlZVSDQ6Q0lRRDpRV01COlM0Qzc6U0c0STpGRUhYIn0.eyJpc3MiOiJoYXJib3ItdG9rZW4taXNzdWVyIiwic3ViIjoicm9ib3QkZGVtbzExIiwiYXVkIjoiaGFyYm9yLXJlZ2lzdHJ5IiwiZXhwIjoxNTcxNzYzOTI2LCJuYmYiOjE1NzE3NjM4NjYsImlhdCI6MTU3MTc2Mzg2NiwianRpIjoiTnRaZWx4Z01KTUU1MXlEMCIsImFjY2VzcyI6W3sidHlwZSI6InJlcG9zaXRvcnkiLCJuYW1lIjoibGlicmFyeS9oZWxsby13b3JsZCIsImFjdGlvbnMiOlsicHVzaCIsIioiLCJwdWxsIiwic2Nhbm5lcnB1bGwiXX1dfQ.GlWuvtoxmChnpvbWaG5901Z9-g63DrzyNUREWlDbR5gnNeuOKjLNyE4QpogAQKx2yYtcGxbqNL3VfJkExJ_gMS0Qw8e10utGOawwqD4oqf_J06eKq4HzpZJengZfcjMA4g2RoeOlqdVdwimB_PdX9vkBO1od0wX0Cc2v0p2w5TkibcThKRoeLeVs2oRewkKLuVHNSM8wwRIlAvpWJuNnvRCFlHRkLcZM_KpGXqT7H-PZETTisWCi1pMxeYEwIsDFLlTKdV8LaiDeDmH-RaLOsuyAySYEW9Ynk5K3P_dUl2c_SYQXloPyi0MvXxSn6EWE4eHF2oQDM_SvIzR9sOVB8TtjMjKKMQ4yr_mqgMcfEpnInJATExBR56wmxNdLESncHl8rUYCe2jCjQFuR9NGQA1tGdjI4NoBN-OVD0dBs9rm_mkb2tgD-3gEhyzAw6hg0uzDsF7bj5Aq8scoi42UurhX2bZM89s4-TWBp4DWuBG0HDiwpOiBvB3RMm6MpQxsqrl0hQm_WH18L6QCknAW2e3d_6DJWJ0eBzISrhDr7LkqJKl1J8pv4zqoh_EUVeLyzTmjEULm-VbnpVF4wW5yTLF3S6F7Ox4vwWtVfi1XQNVOcJDB3VPUsRgiTTuCW-ZGcBLw-OdIcwaJ3T_QZkEjUw1f6i1JcGa0Mpgl83aLiSdQ 0xc0003c77c0 map[alg:RS256 kid:CVQ7:DC75:GTDN:LSMK:UAIN:HQEV:VUH4:CIQD:QWMB:S4C7:SG4I:FEHX typ:JWT] 0xc000496000 GlWuvtoxmChnpvbWaG5901Z9-g63DrzyNUREWlDbR5gnNeuOKjLNyE4QpogAQKx2yYtcGxbqNL3VfJkExJ_gMS0Qw8e10utGOawwqD4oqf_J06eKq4HzpZJengZfcjMA4g2RoeOlqdVdwimB_PdX9vkBO1od0wX0Cc2v0p2w5TkibcThKRoeLeVs2oRewkKLuVHNSM8wwRIlAvpWJuNnvRCFlHRkLcZM_KpGXqT7H-PZETTisWCi1pMxeYEwIsDFLlTKdV8LaiDeDmH-RaLOsuyAySYEW9Ynk5K3P_dUl2c_SYQXloPyi0MvXxSn6EWE4eHF2oQDM_SvIzR9sOVB8TtjMjKKMQ4yr_mqgMcfEpnInJATExBR56wmxNdLESncHl8rUYCe2jCjQFuR9NGQA1tGdjI4NoBN-OVD0dBs9rm_mkb2tgD-3gEhyzAw6hg0uzDsF7bj5Aq8scoi42UurhX2bZM89s4-TWBp4DWuBG0HDiwpOiBvB3RMm6MpQxsqrl0hQm_WH18L6QCknAW2e3d_6DJWJ0eBzISrhDr7LkqJKl1J8pv4zqoh_EUVeLyzTmjEULm-VbnpVF4wW5yTLF3S6F7Ox4vwWtVfi1XQNVOcJDB3VPUsRgiTTuCW-ZGcBLw-OdIcwaJ3T_QZkEjUw1f6i1JcGa0Mpgl83aLiSdQ"
+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
+	rr := httptest.NewRecorder()
+
+	var n http.HandlerFunc
+	if len(next) > 0 {
+		n = next[0]
+	} else {
+		n = func(w http.ResponseWriter, req *http.Request) {
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}
+
+	h := New(http.HandlerFunc(n))
+	h.ServeHTTP(util.NewCustomResponseWriter(rr), req)
+
+	return rr.Code
+}
+
+func (suite *HandlerSuite) TestPullManifest() {
+	code1 := doPullManifestRequest("library", "photon", "release-1.10")
+	suite.Equal(http.StatusNotFound, code1)
+}
+
+func TestMain(m *testing.M) {
+	if result := m.Run(); result != 0 {
+		os.Exit(result)
+	}
+}
+
+func TestRunHandlerSuite(t *testing.T) {
+	suite.Run(t, new(HandlerSuite))
+}

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.Debugf("in url handler, path: %s", req.URL.Path)
+	log.Infof("in url handler, path: %s", req.URL.Path)
 	flag, repository, reference := util.MatchPullManifest(req)
 	if flag {
 		components := strings.SplitN(repository, "/", 2)

src/core/service/token/authutils.go

@@ -134,7 +134,7 @@ func permToActions(p string) []string {
 		res = append(res, "pull")
 	}
 	if strings.Contains(p, "S") {
-		res = append(res, "scannerpull")
+		res = append(res, "scanner-pull")
 	}
 	return res
 }

src/core/service/token/creator.go

@@ -28,7 +28,6 @@ import (
 	"github.com/goharbor/harbor/src/core/config"
 	"github.com/goharbor/harbor/src/core/filter"
 	"github.com/goharbor/harbor/src/core/promgr"
-	"net/http/httputil"
 )
 
 var creatorMap map[string]Creator
@@ -182,7 +181,7 @@ func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManage
 		permission = "R"
 	}
 	if ctx.Can(rbac.ActionScannerPull, resource) {
-		permission = "S"
+		permission = fmt.Sprintf("%s%s", permission, "S")
 	}
 
 	a.Actions = permToActions(permission)
@@ -204,7 +203,6 @@ func (g generalCreator) Create(r *http.Request) (*models.Token, error) {
 	var err error
 	scopes := parseScopes(r.URL)
 	log.Debugf("scopes: %v", scopes)
-	httputil.DumpRequest(r, true)
 
 	ctx, err := filter.GetSecurityContext(r)
 	if err != nil {

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

@@ -14,11 +14,7 @@ type Claim struct {
 
 // Valid valid the standard claims
 func (rc *Claim) Valid() error {
-	stdErr := rc.StandardClaims.Valid()
-	if stdErr != nil {
-		return stdErr
-	}
-	return nil
+	return rc.StandardClaims.Valid()
 }
 
 // GetAccessSet ...

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

@@ -0,0 +1,54 @@
+package registry
+
+import (
+	"github.com/docker/distribution/registry/auth"
+	"github.com/docker/distribution/registry/auth/token"
+	"github.com/goharbor/harbor/src/common/rbac"
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func TestValid(t *testing.T) {
+	access := &token.ResourceActions{
+		Type:    "type",
+		Name:    "repository",
+		Actions: []string{"pull", "push"},
+	}
+	accesses := []*token.ResourceActions{}
+	accesses = append(accesses, access)
+	rClaims := &Claim{
+		Access: accesses,
+	}
+	assert.Nil(t, rClaims.Valid())
+}
+
+func TestGetAccessSet(t *testing.T) {
+	access := &token.ResourceActions{
+		Type:    "repository",
+		Name:    "hello-world",
+		Actions: []string{"pull", "push", "scanner-pull"},
+	}
+	accesses := []*token.ResourceActions{}
+	accesses = append(accesses, access)
+	rClaims := &Claim{
+		Access: accesses,
+	}
+
+	auth1 := auth.Access{
+		Resource: auth.Resource{
+			Type: "repository",
+			Name: "hello-world",
+		},
+		Action: rbac.ActionScannerPull.String(),
+	}
+	auth2 := auth.Access{
+		Resource: auth.Resource{
+			Type: "repository",
+			Name: "busubox",
+		},
+		Action: rbac.ActionScannerPull.String(),
+	}
+	set := rClaims.GetAccessSet()
+	assert.True(t, set.Contains(auth1))
+	assert.False(t, set.Contains(auth2))
+}