From a3c298e9fdb3e31e72ca75a7745fa556debac5cd Mon Sep 17 00:00:00 2001 From: stonezdj Date: Wed, 13 Nov 2019 19:09:28 +0800 Subject: [PATCH] Refactor immutable tag rule Change implementation Fix some nil pointer issue Signed-off-by: stonezdj --- src/core/api/immutabletagrule.go | 15 ++++++++++--- src/core/api/immutabletagrule_test.go | 30 +++++++++++++++++++++++++ src/pkg/immutabletag/controller.go | 5 +++++ src/pkg/immutabletag/controller_test.go | 9 ++++---- src/pkg/immutabletag/manager.go | 6 ++++- src/pkg/immutabletag/manager_test.go | 10 ++++----- 6 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/core/api/immutabletagrule.go b/src/core/api/immutabletagrule.go index 91a3b901e..13ed88b12 100644 --- a/src/core/api/immutabletagrule.go +++ b/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) { diff --git a/src/core/api/immutabletagrule_test.go b/src/core/api/immutabletagrule_test.go index 8a65e9e1a..ee31f9c26 100644 --- a/src/core/api/immutabletagrule_test.go +++ b/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...) } diff --git a/src/pkg/immutabletag/controller.go b/src/pkg/immutabletag/controller.go index 01ea828a1..95c18d403 100644 --- a/src/pkg/immutabletag/controller.go +++ b/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 diff --git a/src/pkg/immutabletag/controller_test.go b/src/pkg/immutabletag/controller_test.go index 624c88685..2c57b08a3 100644 --- a/src/pkg/immutabletag/controller_test.go +++ b/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)) diff --git a/src/pkg/immutabletag/manager.go b/src/pkg/immutabletag/manager.go index a3d6423ed..b78911c9f 100644 --- a/src/pkg/immutabletag/manager.go +++ b/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 } diff --git a/src/pkg/immutabletag/manager_test.go b/src/pkg/immutabletag/manager_test.go index cf14fc960..2079c9273 100644 --- a/src/pkg/immutabletag/manager_test.go +++ b/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\":\"**\"}]}}",