Merge pull request #9862 from stonezdj/191112_subresource

Refactor immutable tag rule

src/core/api/immutabletagrule.go

@@ -40,13 +40,22 @@ func (itr *ImmutableTagRuleAPI) Prepare() {
 		return
 	}
 	itr.projectID = pid
-
+	itr.ctr = immutabletag.ImmuCtr
 	ruleID, err := itr.GetInt64FromPath(":id")
 	if err == nil || ruleID > 0 {
 		itr.ID = ruleID
-	}
+		itRule, err := itr.ctr.GetImmutableRule(itr.ID)
+		if err != nil {
+			itr.SendInternalServerError(err)
+			return
+		}
+		if itRule == nil || itRule.ProjectID != itr.projectID {
+			err := fmt.Errorf("immutable tag rule %v not found", itr.ID)
+			itr.SendNotFoundError(err)
+			return
+		}
 
-	itr.ctr = immutabletag.ImmuCtr
+	}
 
 	if strings.EqualFold(itr.Ctx.Request.Method, "get") {
 		if !itr.requireAccess(rbac.ActionList) {

src/core/api/immutabletagrule_test.go

@@ -228,6 +228,7 @@ func TestImmutableTagRuleAPI_Put(t *testing.T) {
 	defer mgr.DeleteImmutableRule(id)
 
 	url := fmt.Sprintf("/api/projects/1/immutabletagrules/%d", id)
+	url2 := fmt.Sprintf("/api/projects/3/immutabletagrules/%d", id)
 	cases := []*codeCheckingCase{
 		// 401
 		{
@@ -268,6 +269,16 @@ func TestImmutableTagRuleAPI_Put(t *testing.T) {
 			},
 			code: http.StatusForbidden,
 		},
+		// 404
+		{
+			request: &testingRequest{
+				method:     http.MethodPut,
+				url:        url2,
+				credential: projAdmin,
+				bodyJSON:   metadata2,
+			},
+			code: http.StatusNotFound,
+		},
 	}
 	runCodeCheckingCases(t, cases...)
 }
@@ -302,6 +313,7 @@ func TestImmutableTagRuleAPI_Delete(t *testing.T) {
 	defer mgr.DeleteImmutableRule(id)
 
 	url := fmt.Sprintf("/api/projects/1/immutabletagrules/%d", id)
+	wrongURL := fmt.Sprintf("/api/projects/3/immutabletagrules/%d", id)
 
 	cases := []*codeCheckingCase{
 		// 401
@@ -321,6 +333,15 @@ func TestImmutableTagRuleAPI_Delete(t *testing.T) {
 			},
 			code: http.StatusForbidden,
 		},
+		// 404
+		{
+			request: &testingRequest{
+				method:     http.MethodDelete,
+				url:        wrongURL,
+				credential: projAdmin,
+			},
+			code: http.StatusNotFound,
+		},
 		// 200
 		{
 			request: &testingRequest{
@@ -330,6 +351,15 @@ func TestImmutableTagRuleAPI_Delete(t *testing.T) {
 			},
 			code: http.StatusOK,
 		},
+		// 404
+		{
+			request: &testingRequest{
+				method:     http.MethodDelete,
+				url:        url,
+				credential: projAdmin,
+			},
+			code: http.StatusNotFound,
+		},
 	}
 	runCodeCheckingCases(t, cases...)
 }

src/pkg/immutabletag/controller.go

@@ -1,6 +1,8 @@
 package immutabletag
 
 import (
+	"fmt"
+
 	"github.com/goharbor/harbor/src/pkg/immutabletag/model"
 )
 
@@ -54,6 +56,9 @@ func (r *DefaultAPIController) UpdateImmutableRule(pid int64, m *model.Metadata)
 	if err != nil {
 		return err
 	}
+	if m0 == nil {
+		return fmt.Errorf("the immutable tag rule is not found id:%v", m.ID)
+	}
 	if m0.Disabled != m.Disabled {
 		_, err := r.manager.EnableImmutableRule(m.ID, m.Disabled)
 		return err

src/pkg/immutabletag/controller_test.go

@@ -1,6 +1,8 @@
 package immutabletag
 
 import (
+	"testing"
+
 	"github.com/goharbor/harbor/src/common/dao"
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/utils/test"
@@ -8,7 +10,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
-	"testing"
 )
 
 type ControllerTestSuite struct {
@@ -88,7 +89,7 @@ func (s *ControllerTestSuite) TestImmutableRule() {
 		},
 		Disabled: false,
 	}
-	err = s.ctr.UpdateImmutableRule(1, update)
+	err = s.ctr.UpdateImmutableRule(projectID, update)
 	s.require.Nil(err)
 
 	getRule, err := s.ctr.GetImmutableRule(s.ruleID)
@@ -119,7 +120,7 @@ func (s *ControllerTestSuite) TestImmutableRule() {
 		},
 		Disabled: true,
 	}
-	err = s.ctr.UpdateImmutableRule(1, update2)
+	err = s.ctr.UpdateImmutableRule(projectID, update2)
 	s.require.Nil(err)
 	getRule, err = s.ctr.GetImmutableRule(s.ruleID)
 	s.require.Nil(err)
@@ -150,7 +151,7 @@ func (s *ControllerTestSuite) TestImmutableRule() {
 	s.ruleID, err = s.ctr.CreateImmutableRule(rule2)
 	s.require.Nil(err)
 
-	rules, err := s.ctr.ListImmutableRules(1)
+	rules, err := s.ctr.ListImmutableRules(projectID)
 	s.require.Nil(err)
 	s.require.Equal(2, len(rules))
 

src/pkg/immutabletag/manager.go

@@ -2,10 +2,11 @@ package immutabletag
 
 import (
 	"encoding/json"
+	"sort"
+
 	"github.com/goharbor/harbor/src/pkg/immutabletag/dao"
 	dao_model "github.com/goharbor/harbor/src/pkg/immutabletag/dao/model"
 	"github.com/goharbor/harbor/src/pkg/immutabletag/model"
-	"sort"
 )
 
 var (
@@ -62,6 +63,9 @@ func (drm *defaultRuleManager) GetImmutableRule(id int64) (*model.Metadata, erro
 		return nil, err
 	}
 	rule := &model.Metadata{}
+	if daoRule == nil {
+		return nil, nil
+	}
 	if err = json.Unmarshal([]byte(daoRule.TagFilter), rule); err != nil {
 		return nil, err
 	}

src/pkg/immutabletag/manager_test.go

@@ -111,7 +111,7 @@ func (m *managerTestingSuite) TestQueryImmutableRuleByProjectID() {
 			ID:        1,
 			ProjectID: 1,
 			Disabled:  false,
-			TagFilter: "{\"id\":1, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
+			TagFilter: "{\"id\":1, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
 				"\"template\":\"immutable_template\"," +
 				"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
 				"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@@ -120,7 +120,7 @@ func (m *managerTestingSuite) TestQueryImmutableRuleByProjectID() {
 			ID:        2,
 			ProjectID: 1,
 			Disabled:  false,
-			TagFilter: "{\"id\":2, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
+			TagFilter: "{\"id\":2, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
 				"\"template\":\"immutable_template\"," +
 				"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
 				"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@@ -138,7 +138,7 @@ func (m *managerTestingSuite) TestQueryEnabledImmutableRuleByProjectID() {
 			ID:        1,
 			ProjectID: 1,
 			Disabled:  true,
-			TagFilter: "{\"id\":1, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
+			TagFilter: "{\"id\":1, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
 				"\"template\":\"immutable_template\"," +
 				"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
 				"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@@ -147,7 +147,7 @@ func (m *managerTestingSuite) TestQueryEnabledImmutableRuleByProjectID() {
 			ID:        2,
 			ProjectID: 1,
 			Disabled:  true,
-			TagFilter: "{\"id\":2, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
+			TagFilter: "{\"id\":2, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
 				"\"template\":\"immutable_template\"," +
 				"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
 				"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@@ -164,7 +164,7 @@ func (m *managerTestingSuite) TestGetImmutableRule() {
 		ID:        1,
 		ProjectID: 1,
 		Disabled:  true,
-		TagFilter: "{\"id\":1, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
+		TagFilter: "{\"id\":1, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
 			"\"template\":\"immutable_template\"," +
 			"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
 			"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",