From 5bd1cfdbf2a180bbccc21f921a5a9547145d43f4 Mon Sep 17 00:00:00 2001 From: He Weiwei Date: Thu, 14 Nov 2019 07:00:44 +0000 Subject: [PATCH] fix(robot,project-member): check owner of member, robot when update, delete Signed-off-by: He Weiwei --- src/common/api/base.go | 5 +++ src/core/api/harborapi_test.go | 37 ++++++++++++++----- src/core/api/project_test.go | 6 ++-- src/core/api/projectmember.go | 58 +++++++++++++++--------------- src/core/api/projectmember_test.go | 41 +++++++++++++++++++++ src/core/api/robot.go | 37 ++++++------------- src/core/api/robot_test.go | 50 ++++++++++++++++++++++++++ 7 files changed, 166 insertions(+), 68 deletions(-) diff --git a/src/common/api/base.go b/src/common/api/base.go index 928c37e08..9049538a2 100644 --- a/src/common/api/base.go +++ b/src/common/api/base.go @@ -48,6 +48,11 @@ func (b *BaseAPI) GetInt64FromPath(key string) (int64, error) { return strconv.ParseInt(value, 10, 64) } +// ParamExistsInPath returns true when param exists in the path +func (b *BaseAPI) ParamExistsInPath(key string) bool { + return b.GetStringFromPath(key) != "" +} + // Render returns nil as it won't render template func (b *BaseAPI) Render() error { return nil diff --git a/src/core/api/harborapi_test.go b/src/core/api/harborapi_test.go index 824afa6eb..f80625a33 100644 --- a/src/core/api/harborapi_test.go +++ b/src/core/api/harborapi_test.go @@ -26,6 +26,7 @@ import ( "path/filepath" "runtime" "strconv" + "strings" "github.com/astaxie/beego" "github.com/dghubble/sling" @@ -247,19 +248,25 @@ func init() { defer mockServer.Close() } -func request(_sling *sling.Sling, acceptHeader string, authInfo ...usrInfo) (int, []byte, error) { +func request0(_sling *sling.Sling, acceptHeader string, authInfo ...usrInfo) (int, http.Header, []byte, error) { _sling = _sling.Set("Accept", acceptHeader) req, err := _sling.Request() if err != nil { - return 400, nil, err + return 400, nil, nil, err } if len(authInfo) > 0 { req.SetBasicAuth(authInfo[0].Name, authInfo[0].Passwd) } w := httptest.NewRecorder() beego.BeeApp.Handlers.ServeHTTP(w, req) + body, err := ioutil.ReadAll(w.Body) - return w.Code, body, err + return w.Code, w.Header(), body, err +} + +func request(_sling *sling.Sling, acceptHeader string, authInfo ...usrInfo) (int, []byte, error) { + code, _, body, err := request0(_sling, acceptHeader, authInfo...) + return code, body, err } // Search for projects and repositories @@ -543,23 +550,35 @@ func (a testapi) GetProjectMembersByProID(prjUsr usrInfo, projectID string) (int } // Add project role member accompany with projectID -// func (a testapi) AddProjectMember(prjUsr usrInfo, projectID string, roles apilib.RoleParam) (int, error) { -func (a testapi) AddProjectMember(prjUsr usrInfo, projectID string, member *models.MemberReq) (int, error) { +// func (a testapi) AddProjectMember(prjUsr usrInfo, projectID string, roles apilib.RoleParam) (int, int, error) { +func (a testapi) AddProjectMember(prjUsr usrInfo, projectID string, member *models.MemberReq) (int, int, error) { _sling := sling.New().Post(a.basePath) path := "/api/projects/" + projectID + "/members/" _sling = _sling.Path(path) _sling = _sling.BodyJSON(member) - httpStatusCode, _, err := request(_sling, jsonAcceptHeader, prjUsr) - return httpStatusCode, err + httpStatusCode, header, _, err := request0(_sling, jsonAcceptHeader, prjUsr) + var memberID int + location := header.Get("Location") + if location != "" { + parts := strings.Split(location, "/") + if len(parts) > 0 { + i, err := strconv.Atoi(parts[len(parts)-1]) + if err == nil { + memberID = i + } + } + } + + return httpStatusCode, memberID, err } // Delete project role member accompany with projectID -func (a testapi) DeleteProjectMember(authInfo usrInfo, projectID string, userID string) (int, error) { +func (a testapi) DeleteProjectMember(authInfo usrInfo, projectID string, memberID string) (int, error) { _sling := sling.New().Delete(a.basePath) - path := "/api/projects/" + projectID + "/members/" + userID + path := "/api/projects/" + projectID + "/members/" + memberID _sling = _sling.Path(path) httpStatusCode, _, err := request(_sling, jsonAcceptHeader, authInfo) return httpStatusCode, err diff --git a/src/core/api/project_test.go b/src/core/api/project_test.go index 406b439f8..86ec25922 100644 --- a/src/core/api/project_test.go +++ b/src/core/api/project_test.go @@ -214,7 +214,8 @@ func TestListProjects(t *testing.T) { }, } projectID := strconv.Itoa(addPID) - httpStatusCode, err = apiTest.AddProjectMember(*admin, projectID, member) + var memberID int + httpStatusCode, memberID, err = apiTest.AddProjectMember(*admin, projectID, member) if err != nil { t.Error("Error whihle add project role member", err.Error()) t.Log(err) @@ -234,8 +235,7 @@ func TestListProjects(t *testing.T) { assert.Equal("true", result[0].Metadata[models.ProMetaPublic], "Public is wrong") assert.Equal(int32(2), result[0].CurrentUserRoleId, "User project role is wrong") } - id := strconv.Itoa(CommonGetUserID()) - httpStatusCode, err = apiTest.DeleteProjectMember(*admin, projectID, id) + httpStatusCode, err = apiTest.DeleteProjectMember(*admin, projectID, strconv.Itoa(memberID)) if err != nil { t.Error("Error whihle add project role member", err.Error()) t.Log(err) diff --git a/src/core/api/projectmember.go b/src/core/api/projectmember.go index 54e0707cd..538cb98b2 100644 --- a/src/core/api/projectmember.go +++ b/src/core/api/projectmember.go @@ -27,7 +27,6 @@ import ( "github.com/goharbor/harbor/src/common/dao/project" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/rbac" - "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/auth" "github.com/goharbor/harbor/src/core/config" ) @@ -35,11 +34,10 @@ import ( // ProjectMemberAPI handles request to /api/projects/{}/members/{} type ProjectMemberAPI struct { BaseController - id int - entityID int - entityType string - project *models.Project - groupType int + id int + project *models.Project + member *models.Member + groupType int } // ErrDuplicateProjectMember ... @@ -67,26 +65,38 @@ func (pma *ProjectMemberAPI) Prepare() { pma.SendBadRequestError(errors.New(text)) return } - project, err := pma.ProjectMgr.Get(pid) + pro, err := pma.ProjectMgr.Get(pid) if err != nil { pma.ParseAndHandleError(fmt.Sprintf("failed to get project %d", pid), err) return } - if project == nil { + if pro == nil { pma.SendNotFoundError(fmt.Errorf("project %d not found", pid)) return } - pma.project = project + pma.project = pro - pmid, err := pma.GetInt64FromPath(":pmid") - if err != nil { - log.Warningf("Failed to get pmid from path, error %v", err) + if pma.ParamExistsInPath(":pmid") { + pmid, err := pma.GetInt64FromPath(":pmid") + if err != nil || pmid <= 0 { + pma.SendBadRequestError(fmt.Errorf("The project member id is invalid, pmid:%s", pma.GetStringFromPath(":pmid"))) + return + } + pma.id = int(pmid) + + members, err := project.GetProjectMember(models.Member{ProjectID: pid, ID: pma.id}) + if err != nil { + pma.SendInternalServerError(err) + return + } + if len(members) == 0 { + pma.SendNotFoundError(fmt.Errorf("project member %d not found in project %d", pmid, pid)) + return + } + + pma.member = members[0] } - if pmid <= 0 && (pma.Ctx.Input.IsPut() || pma.Ctx.Input.IsDelete()) { - pma.SendBadRequestError(fmt.Errorf("The project member id is invalid, pmid:%s", pma.GetStringFromPath(":pmid"))) - return - } - pma.id = int(pmid) + authMode, err := config.AuthMode() if err != nil { pma.SendInternalServerError(fmt.Errorf("failed to get authentication mode")) @@ -123,22 +133,10 @@ func (pma *ProjectMemberAPI) Get() { } } else { - // return a specific member - queryMember.ID = pma.id - memberList, err := project.GetProjectMember(queryMember) - if err != nil { - pma.SendInternalServerError(fmt.Errorf("Failed to query database for member list, error: %v", err)) - return - } - if len(memberList) == 0 { - pma.SendNotFoundError(fmt.Errorf("The project member does not exist, pmid:%v", pma.id)) - return - } - if !pma.requireAccess(rbac.ActionRead) { return } - pma.Data["json"] = memberList[0] + pma.Data["json"] = pma.member } pma.ServeJSON() } diff --git a/src/core/api/projectmember_test.go b/src/core/api/projectmember_test.go index 306fe138f..b296c6318 100644 --- a/src/core/api/projectmember_test.go +++ b/src/core/api/projectmember_test.go @@ -268,6 +268,23 @@ func TestProjectMemberAPI_PutAndDelete(t *testing.T) { if err != nil { t.Errorf("Error occurred when add project member: %v", err) } + + projectID, err := dao.AddProject(models.Project{Name: "memberputanddelete", OwnerID: 1}) + if err != nil { + t.Errorf("Error occurred when add project: %v", err) + } + defer dao.DeleteProject(projectID) + + memberID, err := project.AddProjectMember(models.Member{ + ProjectID: projectID, + Role: 1, + EntityID: int(userID), + EntityType: "u", + }) + if err != nil { + t.Errorf("Error occurred when add project member: %v", err) + } + URL := fmt.Sprintf("/api/projects/1/members/%v", ID) badURL := fmt.Sprintf("/api/projects/1/members/%v", 0) cases := []*codeCheckingCase{ @@ -330,6 +347,18 @@ func TestProjectMemberAPI_PutAndDelete(t *testing.T) { }, code: http.StatusBadRequest, }, + // 404 + { + request: &testingRequest{ + method: http.MethodPut, + url: fmt.Sprintf("/api/projects/1/members/%v", memberID), + bodyJSON: &models.Member{ + Role: 2, + }, + credential: admin, + }, + code: http.StatusNotFound, + }, // 200 { request: &testingRequest{ @@ -339,6 +368,18 @@ func TestProjectMemberAPI_PutAndDelete(t *testing.T) { }, code: http.StatusOK, }, + // 404 + { + request: &testingRequest{ + method: http.MethodDelete, + url: fmt.Sprintf("/api/projects/1/members/%v", memberID), + bodyJSON: &models.Member{ + Role: 2, + }, + credential: admin, + }, + code: http.StatusNotFound, + }, } runCodeCheckingCases(t, cases...) diff --git a/src/core/api/robot.go b/src/core/api/robot.go index ded3a8d58..2ba1fd148 100644 --- a/src/core/api/robot.go +++ b/src/core/api/robot.go @@ -16,6 +16,9 @@ package api import ( "fmt" + "net/http" + "strconv" + "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/rbac" @@ -24,8 +27,6 @@ import ( "github.com/goharbor/harbor/src/pkg/robot" "github.com/goharbor/harbor/src/pkg/robot/model" "github.com/pkg/errors" - "net/http" - "strconv" ) // RobotAPI ... @@ -40,7 +41,6 @@ type RobotAPI struct { func (r *RobotAPI) Prepare() { r.BaseController.Prepare() - method := r.Ctx.Request.Method if !r.SecurityCtx.IsAuthenticated() { r.SendUnAuthorizedError(errors.New("UnAuthorized")) @@ -70,10 +70,10 @@ func (r *RobotAPI) Prepare() { r.project = project r.ctr = robot.RobotCtr - if method == http.MethodPut || method == http.MethodDelete { + if r.ParamExistsInPath(":id") { id, err := r.GetInt64FromPath(":id") if err != nil || id <= 0 { - r.SendBadRequestError(errors.New("invalid robot ID")) + r.SendBadRequestError(fmt.Errorf("invalid robot ID %s", r.GetStringFromPath(":id"))) return } robot, err := r.ctr.GetRobotAccount(id) @@ -87,6 +87,11 @@ func (r *RobotAPI) Prepare() { return } + if robot.ProjectID != pid { + r.SendNotFoundError(fmt.Errorf("robot %d not found in project %d", id, pid)) + return + } + r.robot = robot } } @@ -173,27 +178,7 @@ func (r *RobotAPI) Get() { return } - id, err := r.GetInt64FromPath(":id") - if err != nil || id <= 0 { - r.SendBadRequestError(fmt.Errorf("invalid robot ID: %s", r.GetStringFromPath(":id"))) - return - } - - robot, err := r.ctr.GetRobotAccount(id) - if err != nil { - r.SendInternalServerError(errors.Wrap(err, "robot API: get robot")) - return - } - if robot == nil { - r.SendNotFoundError(fmt.Errorf("robot API: robot %d not found", id)) - return - } - if !robot.Visible { - r.SendForbiddenError(fmt.Errorf("robot API: robot %d is invisible", id)) - return - } - - r.Data["json"] = robot + r.Data["json"] = r.robot r.ServeJSON() } diff --git a/src/core/api/robot_test.go b/src/core/api/robot_test.go index c6644ca13..05861ea2d 100644 --- a/src/core/api/robot_test.go +++ b/src/core/api/robot_test.go @@ -19,6 +19,8 @@ import ( "net/http" "testing" + "github.com/goharbor/harbor/src/common/dao" + "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/pkg/robot/model" ) @@ -163,6 +165,12 @@ func TestRobotAPIPost(t *testing.T) { } func TestRobotAPIGet(t *testing.T) { + projectID, err := dao.AddProject(models.Project{Name: "robotget", OwnerID: 1}) + if err != nil { + t.Errorf("Error occurred when add project: %v", err) + } + defer dao.DeleteProject(projectID) + cases := []*codeCheckingCase{ // 400 { @@ -183,6 +191,16 @@ func TestRobotAPIGet(t *testing.T) { code: http.StatusNotFound, }, + // 404 robot 1 not belong to the project + { + request: &testingRequest{ + method: http.MethodGet, + url: fmt.Sprintf("/api/projects/%d/robots/1", projectID), + credential: sysAdmin, + }, + code: http.StatusNotFound, + }, + // 200 { request: &testingRequest{ @@ -251,6 +269,12 @@ func TestRobotAPIList(t *testing.T) { } func TestRobotAPIPut(t *testing.T) { + projectID, err := dao.AddProject(models.Project{Name: "robotput", OwnerID: 1}) + if err != nil { + t.Errorf("Error occurred when add project: %v", err) + } + defer dao.DeleteProject(projectID) + cases := []*codeCheckingCase{ // 401 { @@ -281,6 +305,16 @@ func TestRobotAPIPut(t *testing.T) { code: http.StatusNotFound, }, + // 404 robot 1 not belong to the project + { + request: &testingRequest{ + method: http.MethodPut, + url: fmt.Sprintf("/api/projects/%d/robots/1", projectID), + credential: sysAdmin, + }, + code: http.StatusNotFound, + }, + // 403 non-member user { request: &testingRequest{ @@ -319,6 +353,12 @@ func TestRobotAPIPut(t *testing.T) { } func TestRobotAPIDelete(t *testing.T) { + projectID, err := dao.AddProject(models.Project{Name: "robotdelete", OwnerID: 1}) + if err != nil { + t.Errorf("Error occurred when add project: %v", err) + } + defer dao.DeleteProject(projectID) + cases := []*codeCheckingCase{ // 401 { @@ -349,6 +389,16 @@ func TestRobotAPIDelete(t *testing.T) { code: http.StatusNotFound, }, + // 404 robot 1 not belong to the project + { + request: &testingRequest{ + method: http.MethodDelete, + url: fmt.Sprintf("/api/projects/%d/robots/1", projectID), + credential: sysAdmin, + }, + code: http.StatusNotFound, + }, + // 403 non-member user { request: &testingRequest{