From 8c155e0c5002cefcd8667d44b3f6bba468e43306 Mon Sep 17 00:00:00 2001 From: wang yan Date: Fri, 11 Oct 2019 01:25:17 +0800 Subject: [PATCH 1/5] fix quota migration still execute on launch even data sync success This commit is to fix the issue for the following scenario: 1, user success migrate harbor to v1.9.0 from a previous version 2, add a project, push images into the project. 3, delete images and then to delete the project. 4, re-launch harbor. After that, it still execute the quota migration as the condition doesn't consider the deleted projects usage. And in this case, the harbor core crashes with a duplicate sql err, and unable to launch. [Workaroud] Clean table of project_blob with: TRUNCATE TABLE project_blob, and re-launch harbor, wait for quota sync success. Signed-off-by: wang yan --- src/core/main.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/core/main.go b/src/core/main.go index ceaece635..bf485275f 100755 --- a/src/core/main.go +++ b/src/core/main.go @@ -85,17 +85,22 @@ func updateInitPassword(userID int, password string) error { // Quota migration func quotaSync() error { - usages, err := dao.ListQuotaUsages() - if err != nil { - log.Errorf("list quota usage error, %v", err) - return err - } projects, err := dao.GetProjects(nil) if err != nil { log.Errorf("list project error, %v", err) return err } + var pids []string + for _, project := range projects { + pids = append(pids, strconv.FormatInt(project.ProjectID, 10)) + } + usages, err := dao.ListQuotaUsages(&models.QuotaUsageQuery{Reference: "project", ReferenceIDs: pids}) + if err != nil { + log.Errorf("list quota usage error, %v", err) + return err + } + // The condition handles these two cases: // 1, len(project) > 1 && len(usages) == 1. existing projects without usage, as we do always has 'library' usage in DB. // 2, migration fails at the phase of inserting usage into DB, and parts of them are inserted successfully. From 96a271f388048e361bd8e96326a0c9ab8635da99 Mon Sep 17 00:00:00 2001 From: "Danfeng Liu (c)" Date: Tue, 24 Sep 2019 18:35:19 +0800 Subject: [PATCH 2/5] In nightly migrate pipeline, after migration, should check all the data which were populated, for now, project meta data were populated, but were not been verified, so I add these verification in this PR. Signed-off-by: Danfeng Liu (c) --- tests/resources/Harbor-Pages/Project.robot | 8 +++- .../Harbor-Pages/Project_Elements.robot | 4 ++ tests/resources/Harbor-Pages/Verify.robot | 44 +++++++++++++++---- tests/robot-cases/Group3-Upgrade/data.json | 20 +++++---- tests/robot-cases/Group3-Upgrade/prepare.py | 5 ++- .../robot-cases/Group3-Upgrade/prepare_v17.py | 33 +++++++++----- 6 files changed, 81 insertions(+), 33 deletions(-) diff --git a/tests/resources/Harbor-Pages/Project.robot b/tests/resources/Harbor-Pages/Project.robot index 94ce7493e..5d1e8f36b 100644 --- a/tests/resources/Harbor-Pages/Project.robot +++ b/tests/resources/Harbor-Pages/Project.robot @@ -61,6 +61,10 @@ Switch To Replication Retry Element Click xpath=${project_replication_xpath} Sleep 1 +Switch To Project Configuration + Retry Element Click ${project_config_tabsheet} + Sleep 1 + Navigate To Projects Retry Element Click xpath=${projects_xpath} Sleep 2 @@ -82,7 +86,7 @@ Search Private Projects Make Project Private [Arguments] ${projectname} Go Into Project ${project name} - Retry Element Click ${project_config_tabsheet} + Switch To Project Configuration Retry Checkbox Should Be Selected ${project_config_public_checkbox} Retry Double Keywords When Error Retry Element Click ${project_config_public_checkbox_label} Retry Checkbox Should Not Be Selected ${project_config_public_checkbox} Retry Element Click //button[contains(.,'SAVE')] @@ -91,7 +95,7 @@ Make Project Private Make Project Public [Arguments] ${projectname} Go Into Project ${project name} - Retry Element Click ${project_config_tabsheet} + Switch To Project Configuration Retry Checkbox Should Not Be Selected ${project_config_public_checkbox} Retry Double Keywords When Error Retry Element Click ${project_config_public_checkbox_label} Retry Checkbox Should Be Selected ${project_config_public_checkbox} Retry Element Click //button[contains(.,'SAVE')] diff --git a/tests/resources/Harbor-Pages/Project_Elements.robot b/tests/resources/Harbor-Pages/Project_Elements.robot index a2200f66c..5fa922fb2 100644 --- a/tests/resources/Harbor-Pages/Project_Elements.robot +++ b/tests/resources/Harbor-Pages/Project_Elements.robot @@ -50,6 +50,10 @@ ${tag_images_btn} xpath=//hbr-repository//button[contains(.,'Images')] ${project_member_action_xpath} xpath=//*[@id='member-action'] ${project_member_set_role_xpath} xpath=//clr-dropdown-menu//label[contains(.,'SET ROLE')] ${project_config_public_checkbox} xpath=//input[@name='public'] +${project_config_content_trust_checkbox} xpath=//input[@name='content-trust'] +${project_config_scan_images_on_push_checkbox} xpath=//input[@name='scan-image-on-push'] +${project_config_prevent_vulnerable_images_from_running_checkbox} xpath=//input[@name='prevent-vulenrability-image-input'] +${project_config_severity_select} xpath=//select[@id='severity'] ${project_config_public_checkbox_label} xpath=//*[@id="clr-wrapper-public"]/div/clr-checkbox-wrapper/label ${project_config_prevent_vulenrability_checkbox_label} xpath=//*[@id='prevent-vulenrability-image']//clr-checkbox-wrapper//label ${project_config_system_wl_radio_input} xpath=//clr-radio-wrapper//label[contains(.,'System whitelist')] diff --git a/tests/resources/Harbor-Pages/Verify.robot b/tests/resources/Harbor-Pages/Verify.robot index 9337058be..00616e84a 100644 --- a/tests/resources/Harbor-Pages/Verify.robot +++ b/tests/resources/Harbor-Pages/Verify.robot @@ -27,8 +27,7 @@ Verify Project Sign In Harbor ${HARBOR_URL} ${HARBOR_ADMIN} ${HARBOR_PASSWORD} :FOR ${project} IN @{project} \ Page Should Contain ${project} - #TO_DO: - #Verify project metadata. + Verify Project Metadata ${json} Close Browser Verify Image Tag @@ -40,11 +39,38 @@ Verify Image Tag \ @{out_has_image}= Get Value From Json ${json} $.projects[?(@.name=${project})].has_image \ ${has_image} Set Variable If @{out_has_image}[0] == ${true} ${true} ${false} \ Go Into Project ${project} has_image=${has_image} - \ @{repo}= Get Value From Json ${json} $.projects[?(@name=${project})]..repo..name - \ Loop Image Repo @{repo} + \ @{repo}= Get Value From Json ${json} $.projects[?(@.name=${project})]..repo..name + \ Run Keyword If ${has_image} == ${true} Loop Image Repo @{repo} \ Navigate To Projects Close Browser +Verify Project Metadata + [Arguments] ${json} + @{project}= Get Value From Json ${json} $.projects.[*].name + Init Chrome Driver + Sign In Harbor ${HARBOR_URL} ${HARBOR_ADMIN} ${HARBOR_PASSWORD} + :FOR ${project} IN @{project} + \ @{out_has_image}= Get Value From Json ${json} $.projects[?(@.name=${project})].has_image + \ ${has_image} Set Variable If @{out_has_image}[0] == ${true} ${true} ${false} + \ Go Into Project ${project} has_image=${has_image} + \ Switch To Project Configuration + \ Verify Checkbox ${json} $.projects[?(@.name=${project})].configuration.public ${project_config_public_checkbox} + \ Verify Checkbox ${json} $.projects[?(@.name=${project})].configuration.enable_content_trust ${project_config_content_trust_checkbox} + \ Verify Checkbox ${json} $.projects[?(@.name=${project})].configuration.automatically_scan_images_on_push ${project_config_scan_images_on_push_checkbox} + \ Verify Checkbox ${json} $.projects[?(@.name=${project})].configuration.prevent_vulnerable_images_from_running ${project_config_prevent_vulnerable_images_from_running_checkbox} + \ ${ret} Get Selected List Value ${project_config_severity_select} + \ @{severity}= Get Value From Json ${json} $.projects[?(@.name=${project})].configuration.prevent_vlunerable_images_from_running_severity + \ Should Contain ${ret} @{severity}[0] + \ Navigate To Projects + Close Browser + +Verify Checkbox + [Arguments] ${json} ${key} ${checkbox} + @{out}= Get Value From Json ${json} ${key} + Run Keyword If '@{out}[0]'=='true' Checkbox Should Be Selected ${checkbox} + ... ELSE Checkbox Should Not Be Selected ${checkbox} + + Loop Image Repo [Arguments] @{repo} :For ${repo} In @{repo} @@ -60,7 +86,7 @@ Verify Member Exist \ ${has_image} Set Variable If @{out_has_image}[0] == ${true} ${true} ${false} \ Go Into Project ${project} has_image=${has_image} \ Switch To Member - \ @{members}= Get Value From Json ${json} $.projects[?(@name=${project})].member..name + \ @{members}= Get Value From Json ${json} $.projects[?(@.name=${project})].member..name \ Loop Member @{members} \ Navigate To Projects Close Browser @@ -76,10 +102,10 @@ Verify User System Admin Role Init Chrome Driver :FOR ${user} IN @{user} \ Sign In Harbor ${HARBOR_URL} ${user} ${HARBOR_PASSWORD} - \ Page Should Contain Administration + \ Page Should Contain Administration \ Logout Harbor Close Browser - + Verify System Label [Arguments] ${json} @{label}= Get Value From Json ${json} $..syslabel..name @@ -106,7 +132,7 @@ Verify Project Label \ \ Page Should Contain ${projectlabel} \ Navigate To Projects Close Browser - + Verify Endpoint [Arguments] ${json} @{endpoint}= Get Value From Json ${json} $.endpoint..name @@ -135,7 +161,7 @@ Verify Project Setting \ ${contenttrust}= Get Value From Json ${json} $.projects[?(@.name=${project})]..enable_content_trust \ ${preventrunning}= Get Value From Json ${json} $.projects[?(@.name=${project})]..prevent_vulnerable_images_from_running \ ${scanonpush}= Get Value From Json ${json} $.projects[?(@.name=${project})]..automatically_scan_images_on_push - \ Init Chrome Driver + \ Init Chrome Driver \ Sign In Harbor ${HARBOR_URL} ${HARBOR_ADMIN} ${HARBOR_PASSWORD} \ @{out_has_image}= Get Value From Json ${json} $.projects[?(@.name=${project})].has_image \ ${has_image} Set Variable If @{out_has_image}[0] == ${true} ${true} ${false} diff --git a/tests/robot-cases/Group3-Upgrade/data.json b/tests/robot-cases/Group3-Upgrade/data.json index 2bdb73941..07317ee3a 100644 --- a/tests/robot-cases/Group3-Upgrade/data.json +++ b/tests/robot-cases/Group3-Upgrade/data.json @@ -99,12 +99,12 @@ { "name":"busybox", "tag":"latest", - "signed":"False" + "signed":"false" }, { "name":"alpine", "tag":"latest", - "signed":"True" + "signed":"true" } ], "member":[ @@ -144,10 +144,11 @@ } ], "configuration":{ + "public":"true", "enable_content_trust":"true", "automatically_scan_images_on_push":"true", "prevent_vulnerable_images_from_running":"true", - "prevent_vlunerable_images_from_running_severity":"High" + "prevent_vlunerable_images_from_running_severity":"high" } }, { @@ -159,12 +160,12 @@ { "name":"busybox", "tag":"latest", - "signed":"False" + "signed":"false" }, { "name":"alpine", "tag":"latest", - "signed":"True" + "signed":"true" } ], "member":[ @@ -204,10 +205,11 @@ } ], "configuration":{ - "enable_content_trust":"True", - "automatically_scan_images_on_push":"True", - "prevent_vulnerable_images_from_running":"True", - "prevent_vlunerable_images_from_running_severity":"High" + "public":"false", + "enable_content_trust":"false", + "automatically_scan_images_on_push":"false", + "prevent_vulnerable_images_from_running":"true", + "prevent_vlunerable_images_from_running_severity":"medium" } } ] diff --git a/tests/robot-cases/Group3-Upgrade/prepare.py b/tests/robot-cases/Group3-Upgrade/prepare.py index bcb9407f1..852e1cb4d 100644 --- a/tests/robot-cases/Group3-Upgrade/prepare.py +++ b/tests/robot-cases/Group3-Upgrade/prepare.py @@ -68,13 +68,13 @@ class HarborAPI: body=dict(body=payload) request(url+"replication/policies", 'post', **body) - def update_project_setting(self, project, contenttrust, preventrunning, preventseverity, scanonpush): + def update_project_setting(self, project, public, contenttrust, preventrunning, preventseverity, scanonpush): r = request(url+"projects?name="+project+"", 'get') projectid = str(r.json()[0]['project_id']) payload = { "project_name": ""+project+"", "metadata": { - "public": "True", + "public": public, "enable_content_trust": contenttrust, "prevent_vulnerable_images_from_running": preventrunning, "prevent_vulnerable_images_from_running_severity": preventseverity, @@ -188,6 +188,7 @@ def do_data_creation(): replicationrule["rulename"]) for project in data["projects"]: harborAPI.update_project_setting(project["name"], + project["configuration"]["public"], project["configuration"]["enable_content_trust"], project["configuration"]["prevent_vulnerable_images_from_running"], project["configuration"]["prevent_vlunerable_images_from_running_severity"], diff --git a/tests/robot-cases/Group3-Upgrade/prepare_v17.py b/tests/robot-cases/Group3-Upgrade/prepare_v17.py index 8589f20d4..19b9fa2a6 100644 --- a/tests/robot-cases/Group3-Upgrade/prepare_v17.py +++ b/tests/robot-cases/Group3-Upgrade/prepare_v17.py @@ -58,19 +58,29 @@ class HarborAPI: body=dict(body=payload) request(url+"policies/replication", 'post', **body) - def update_project_setting(self, project, contenttrust, preventrunning, preventseverity, scanonpush): + def update_project_setting(self, project, public, contenttrust, preventrunning, preventseverity, scanonpush): r = request(url+"projects?name="+project+"", 'get') projectid = str(r.json()[0]['project_id']) - payload = { - "project_name": ""+project+"", - "metadata": { - "public": "True", - "enable_content_trust": contenttrust, - "prevent_vulnerable_images_from_running": preventrunning, - "prevent_vulnerable_images_from_running_severity": preventseverity, - "automatically_scan_images_on_push": scanonpush + if args.version == "1.6": + payload = { + "metadata": { + "public": public, + "enable_content_trust": contenttrust, + "prevent_vulnerable_images_from_running": preventrunning, + "prevent_vulnerable_images_from_running_severity": preventseverity, + "automatically_scan_images_on_push": scanonpush + } + } + else: + payload = { + "metadata": { + "public": public, + "enable_content_trust": contenttrust, + "prevent_vul": preventrunning, + "severity": preventseverity, + "auto_scan": scanonpush + } } - } body=dict(body=payload) request(url+"projects/"+projectid+"", 'put', **body) @@ -178,9 +188,10 @@ def do_data_creation(): replicationrule["rulename"]) for project in data["projects"]: harborAPI.update_project_setting(project["name"], + project["configuration"]["public"], project["configuration"]["enable_content_trust"], project["configuration"]["prevent_vulnerable_images_from_running"], - project["configuration"]["prevent_vlunerable_images_from_running_severity"], + project["configuration"]["prevent_vlunerable_images_from_running_severity"], project["configuration"]["automatically_scan_images_on_push"]) harborAPI.update_systemsetting(data["configuration"]["emailsetting"]["emailfrom"], data["configuration"]["emailsetting"]["emailserver"], From 53a13e165db9bbacc8eb669d022f4338e940809b Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Thu, 10 Oct 2019 19:46:30 +0800 Subject: [PATCH 3/5] API for user to set the CLI secret This commit replace the API to generate CLI secret with a new API to update the secret Signed-off-by: Daniel Jiang --- docs/swagger.yaml | 24 +++++++++++++---------- src/core/api/user.go | 41 ++++++++++++++++++++++++++++----------- src/core/api/user_test.go | 16 ++++++++++++--- src/core/router.go | 2 +- 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 191fe4775..e2a83c8ab 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -959,13 +959,13 @@ paths: description: User ID does not exist. '500': description: Unexpected internal errors. - '/users/{user_id}/gen_cli_secret': - post: - summary: Generate new CLI secret for a user. + '/users/{user_id}/cli_secret': + put: + summary: Set CLI secret for a user. description: | This endpoint let user generate a new CLI secret for himself. This API only works when auth mode is set to 'OIDC'. Once this API returns with successful status, the old secret will be invalid, as there will be only one CLI secret - for a user. The new secret will be returned in the response. + for a user. parameters: - name: user_id in: path @@ -973,19 +973,23 @@ paths: format: int required: true description: User ID - tags: - - Products - responses: - '200': - description: The secret is successfully generated. + - name: input_secret + in: body + description: JSON object that includes the new secret + required: true schema: type: object properties: secret: type: string description: The new secret + tags: + - Products + responses: + '200': + description: The secret is successfully updated '400': - description: Invalid user ID. Or user is not onboarded via OIDC authentication. + description: Invalid user ID. Or user is not onboarded via OIDC authentication. Or the secret does not meet the standard. '401': description: User need to log in first. '403': diff --git a/src/core/api/user.go b/src/core/api/user.go index 952c6fc34..3372bdee8 100644 --- a/src/core/api/user.go +++ b/src/core/api/user.go @@ -52,7 +52,7 @@ type userSearch struct { Username string `json:"username"` } -type secretResp struct { +type secretReq struct { Secret string `json:"secret"` } @@ -405,8 +405,8 @@ func (ua *UserAPI) ChangePassword() { return } - if len(req.NewPassword) == 0 { - ua.SendBadRequestError(errors.New("empty new_password")) + if err := validateSecret(req.NewPassword); err != nil { + ua.SendBadRequestError(err) return } @@ -512,8 +512,8 @@ func (ua *UserAPI) ListUserPermissions() { return } -// GenCLISecret generates a new CLI secret and replace the old one -func (ua *UserAPI) GenCLISecret() { +// SetCLISecret handles request PUT /api/users/:id/cli_secret to update the CLI secret of the user +func (ua *UserAPI) SetCLISecret() { if ua.AuthMode != common.OIDCAuth { ua.SendPreconditionFailedError(errors.New("the auth mode has to be oidc auth")) return @@ -534,8 +534,17 @@ func (ua *UserAPI) GenCLISecret() { return } - sec := utils.GenerateRandomString() - encSec, err := utils.ReversibleEncrypt(sec, ua.secretKey) + s := &secretReq{} + if err := ua.DecodeJSONReq(s); err != nil { + ua.SendBadRequestError(err) + return + } + if err := validateSecret(s.Secret); err != nil { + ua.SendBadRequestError(err) + return + } + + encSec, err := utils.ReversibleEncrypt(s.Secret, ua.secretKey) if err != nil { log.Errorf("Failed to encrypt secret, error: %v", err) ua.SendInternalServerError(errors.New("failed to encrypt secret")) @@ -548,8 +557,6 @@ func (ua *UserAPI) GenCLISecret() { ua.SendInternalServerError(errors.New("failed to update secret in DB")) return } - ua.Data["json"] = secretResp{sec} - ua.ServeJSON() } func (ua *UserAPI) getOIDCUserInfo() (*models.OIDCUser, error) { @@ -588,12 +595,24 @@ func validate(user models.User) error { if utils.IsContainIllegalChar(user.Username, []string{",", "~", "#", "$", "%"}) { return fmt.Errorf("username contains illegal characters") } - if utils.IsIllegalLength(user.Password, 8, 20) { - return fmt.Errorf("password with illegal length") + + if err := validateSecret(user.Password); err != nil { + return err } + return commonValidate(user) } +func validateSecret(in string) error { + hasLower := regexp.MustCompile(`[a-z]`) + hasUpper := regexp.MustCompile(`[A-Z]`) + hasNumber := regexp.MustCompile(`[0-9]`) + if len(in) >= 8 && hasLower.MatchString(in) && hasUpper.MatchString(in) && hasNumber.MatchString(in) { + return nil + } + return errors.New("the password or secret must longer than 8 chars with at least 1 uppercase letter, 1 lowercase letter and 1 number") +} + // commonValidate validates email, realname, comment information when user register or change their profile func commonValidate(user models.User) error { diff --git a/src/core/api/user_test.go b/src/core/api/user_test.go index 88f35dd0d..530af0500 100644 --- a/src/core/api/user_test.go +++ b/src/core/api/user_test.go @@ -380,8 +380,8 @@ func buildChangeUserPasswordURL(id int) string { func TestUsersUpdatePassword(t *testing.T) { fmt.Println("Testing Update User Password") - oldPassword := "old_password" - newPassword := "new_password" + oldPassword := "old_Passw0rd" + newPassword := "new_Passw0rd" user01 := models.User{ Username: "user01_for_testing_change_password", @@ -515,7 +515,7 @@ func TestUsersUpdatePassword(t *testing.T) { method: http.MethodPut, url: buildChangeUserPasswordURL(user01.UserID), bodyJSON: &passwordReq{ - NewPassword: "another_new_password", + NewPassword: "another_new_Passw0rd", }, credential: admin, }, @@ -642,3 +642,13 @@ func TestUsersCurrentPermissions(t *testing.T) { assert.Nil(err) assert.Equal(int(403), httpStatusCode, "httpStatusCode should be 403") } + +func TestValidateSecret(t *testing.T) { + assert.NotNil(t, validateSecret("")) + assert.NotNil(t, validateSecret("12345678")) + assert.NotNil(t, validateSecret("passw0rd")) + assert.NotNil(t, validateSecret("PASSW0RD")) + assert.NotNil(t, validateSecret("Sh0rt")) + assert.Nil(t, validateSecret("Passw0rd")) + assert.Nil(t, validateSecret("Thisis1Valid_password")) +} diff --git a/src/core/router.go b/src/core/router.go index b7ee2736d..fa33ae6a6 100755 --- a/src/core/router.go +++ b/src/core/router.go @@ -52,7 +52,7 @@ func initRouters() { beego.Router("/api/users/:id([0-9]+)/password", &api.UserAPI{}, "put:ChangePassword") beego.Router("/api/users/:id/permissions", &api.UserAPI{}, "get:ListUserPermissions") beego.Router("/api/users/:id/sysadmin", &api.UserAPI{}, "put:ToggleUserAdminRole") - beego.Router("/api/users/:id/gen_cli_secret", &api.UserAPI{}, "post:GenCLISecret") + beego.Router("/api/users/:id/cli_secret", &api.UserAPI{}, "put:SetCLISecret") beego.Router("/api/usergroups/?:ugid([0-9]+)", &api.UserGroupAPI{}) beego.Router("/api/ldap/ping", &api.LdapAPI{}, "post:Ping") beego.Router("/api/ldap/users/search", &api.LdapAPI{}, "get:Search") From 67dfc5b35c30d71fc9cfb2727ce3cf2106cc0233 Mon Sep 17 00:00:00 2001 From: Stuart Clements Date: Fri, 11 Oct 2019 09:16:22 +0200 Subject: [PATCH 4/5] Fixed typo --- docs/user_guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user_guide.md b/docs/user_guide.md index 5832ab13b..42d08e7db 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -906,7 +906,7 @@ For example, you have following tags, listed according to their push time, and a You configure a retention policy to retain the two latest tags that match `harbor-*`, so that `harbor-rc` and `harbor-latest` are deleted. However, since all tags refer to the same SHA digest, this policy would also delete the tags `harbor-1.8` and `harbor-release`, so all tags are retained. -### Combining Rules on a Respository +### Combining Rules on a Repository You can define up to 15 rules per project. You can apply multiple rules to a repository or set of repositories. When you apply multiple rules to a repository, they are applied with `OR` logic rather than with `AND` logic. In this way, there is no prioritization of application of the rules on a given repository. Rules run concurrently in the background, and the resulting sets from each rule are combined at the end of the run. From 6f6f113f0fdc88b53b05b492716833916fa690c4 Mon Sep 17 00:00:00 2001 From: wang yan Date: Thu, 10 Oct 2019 19:15:02 +0800 Subject: [PATCH 5/5] refactor robot api 1, add API controller for robot account, make it callable internally 2, add Manager to handler dao releate operation Signed-off-by: wang yan --- src/common/dao/robot.go | 106 ------------ src/common/dao/robot_test.go | 159 ------------------ src/common/models/base.go | 1 - src/common/security/robot/context.go | 5 +- src/common/security/robot/context_test.go | 13 +- src/core/api/immutabletagrule_test.go | 5 +- src/core/api/robot.go | 110 ++++-------- src/core/api/robot_test.go | 20 +-- src/core/filter/security.go | 4 +- src/pkg/robot/controller.go | 115 +++++++++++++ src/pkg/robot/controller_test.go | 103 ++++++++++++ src/pkg/robot/dao/robot.go | 121 +++++++++++++ src/pkg/robot/dao/robot_test.go | 140 +++++++++++++++ src/pkg/robot/manager.go | 71 ++++++++ src/pkg/robot/manager_test.go | 142 ++++++++++++++++ .../models => pkg/robot/model}/robot.go | 45 +++-- 16 files changed, 773 insertions(+), 387 deletions(-) delete mode 100644 src/common/dao/robot.go delete mode 100644 src/common/dao/robot_test.go create mode 100644 src/pkg/robot/controller.go create mode 100644 src/pkg/robot/controller_test.go create mode 100644 src/pkg/robot/dao/robot.go create mode 100644 src/pkg/robot/dao/robot_test.go create mode 100644 src/pkg/robot/manager.go create mode 100644 src/pkg/robot/manager_test.go rename src/{common/models => pkg/robot/model}/robot.go (70%) diff --git a/src/common/dao/robot.go b/src/common/dao/robot.go deleted file mode 100644 index 0d8b5c7f1..000000000 --- a/src/common/dao/robot.go +++ /dev/null @@ -1,106 +0,0 @@ -// Copyright Project Harbor Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package dao - -import ( - "github.com/astaxie/beego/orm" - "github.com/goharbor/harbor/src/common/models" - "strings" - "time" -) - -// AddRobot ... -func AddRobot(robot *models.Robot) (int64, error) { - now := time.Now() - robot.CreationTime = now - robot.UpdateTime = now - id, err := GetOrmer().Insert(robot) - if err != nil { - if strings.Contains(err.Error(), "duplicate key value violates unique constraint") { - return 0, ErrDupRows - } - return 0, err - } - return id, nil -} - -// GetRobotByID ... -func GetRobotByID(id int64) (*models.Robot, error) { - robot := &models.Robot{ - ID: id, - } - if err := GetOrmer().Read(robot); err != nil { - if err == orm.ErrNoRows { - return nil, nil - } - return nil, err - } - - return robot, nil -} - -// ListRobots list robots according to the query conditions -func ListRobots(query *models.RobotQuery) ([]*models.Robot, error) { - qs := getRobotQuerySetter(query).OrderBy("Name") - if query != nil { - if query.Size > 0 { - qs = qs.Limit(query.Size) - if query.Page > 0 { - qs = qs.Offset((query.Page - 1) * query.Size) - } - } - } - robots := []*models.Robot{} - _, err := qs.All(&robots) - return robots, err -} - -func getRobotQuerySetter(query *models.RobotQuery) orm.QuerySeter { - qs := GetOrmer().QueryTable(&models.Robot{}) - - if query == nil { - return qs - } - - if len(query.Name) > 0 { - if query.FuzzyMatchName { - qs = qs.Filter("Name__icontains", query.Name) - } else { - qs = qs.Filter("Name", query.Name) - } - } - if query.ProjectID != 0 { - qs = qs.Filter("ProjectID", query.ProjectID) - } - return qs -} - -// CountRobot ... -func CountRobot(query *models.RobotQuery) (int64, error) { - return getRobotQuerySetter(query).Count() -} - -// UpdateRobot ... -func UpdateRobot(robot *models.Robot) error { - robot.UpdateTime = time.Now() - _, err := GetOrmer().Update(robot) - return err -} - -// DeleteRobot ... -func DeleteRobot(id int64) error { - _, err := GetOrmer().QueryTable(&models.Robot{}).Filter("ID", id).Delete() - return err -} diff --git a/src/common/dao/robot_test.go b/src/common/dao/robot_test.go deleted file mode 100644 index 0ffbcf081..000000000 --- a/src/common/dao/robot_test.go +++ /dev/null @@ -1,159 +0,0 @@ -// Copyright Project Harbor Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package dao - -import ( - "testing" - - "github.com/goharbor/harbor/src/common/models" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestAddRobot(t *testing.T) { - robotName := "test1" - robot := &models.Robot{ - Name: robotName, - Description: "test1 description", - ProjectID: 1, - } - - // add - id, err := AddRobot(robot) - require.Nil(t, err) - robot.ID = id - - require.Nil(t, err) - assert.NotNil(t, id) - -} - -func TestGetRobot(t *testing.T) { - robotName := "test2" - robot := &models.Robot{ - Name: robotName, - Description: "test2 description", - ProjectID: 1, - } - - // add - id, err := AddRobot(robot) - require.Nil(t, err) - robot.ID = id - - robot, err = GetRobotByID(id) - require.Nil(t, err) - assert.Equal(t, robotName, robot.Name) - -} - -func TestListRobots(t *testing.T) { - robotName := "test3" - robot := &models.Robot{ - Name: robotName, - Description: "test3 description", - ProjectID: 1, - } - - _, err := AddRobot(robot) - require.Nil(t, err) - - robots, err := ListRobots(&models.RobotQuery{ - ProjectID: 1, - }) - require.Nil(t, err) - assert.Equal(t, 3, len(robots)) - -} - -func TestDisableRobot(t *testing.T) { - robotName := "test4" - robot := &models.Robot{ - Name: robotName, - Description: "test4 description", - ProjectID: 1, - } - - // add - id, err := AddRobot(robot) - require.Nil(t, err) - - // Disable - robot.Disabled = true - err = UpdateRobot(robot) - require.Nil(t, err) - - // Get - robot, err = GetRobotByID(id) - require.Nil(t, err) - assert.Equal(t, true, robot.Disabled) - -} - -func TestEnableRobot(t *testing.T) { - robotName := "test5" - robot := &models.Robot{ - Name: robotName, - Description: "test5 description", - Disabled: true, - ProjectID: 1, - } - - // add - id, err := AddRobot(robot) - require.Nil(t, err) - - // Disable - robot.Disabled = false - err = UpdateRobot(robot) - require.Nil(t, err) - - // Get - robot, err = GetRobotByID(id) - require.Nil(t, err) - assert.Equal(t, false, robot.Disabled) - -} - -func TestDeleteRobot(t *testing.T) { - robotName := "test6" - robot := &models.Robot{ - Name: robotName, - Description: "test6 description", - ProjectID: 1, - } - - // add - id, err := AddRobot(robot) - require.Nil(t, err) - - // Disable - err = DeleteRobot(id) - require.Nil(t, err) - - // Get - robot, err = GetRobotByID(id) - assert.Nil(t, robot) - -} - -func TestListAllRobot(t *testing.T) { - - robots, err := ListRobots(nil) - require.Nil(t, err) - assert.Equal(t, 5, len(robots)) - -} diff --git a/src/common/models/base.go b/src/common/models/base.go index de04d0285..c08b0f37c 100644 --- a/src/common/models/base.go +++ b/src/common/models/base.go @@ -35,7 +35,6 @@ func init() { new(UserGroup), new(AdminJob), new(JobLog), - new(Robot), new(OIDCUser), new(NotificationPolicy), new(NotificationJob), diff --git a/src/common/security/robot/context.go b/src/common/security/robot/context.go index 8fc622fe0..43ebf5921 100644 --- a/src/common/security/robot/context.go +++ b/src/common/security/robot/context.go @@ -18,17 +18,18 @@ import ( "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/core/promgr" + "github.com/goharbor/harbor/src/pkg/robot/model" ) // SecurityContext implements security.Context interface based on database type SecurityContext struct { - robot *models.Robot + robot *model.Robot pm promgr.ProjectManager policy []*rbac.Policy } // NewSecurityContext ... -func NewSecurityContext(robot *models.Robot, pm promgr.ProjectManager, policy []*rbac.Policy) *SecurityContext { +func NewSecurityContext(robot *model.Robot, pm promgr.ProjectManager, policy []*rbac.Policy) *SecurityContext { return &SecurityContext{ robot: robot, pm: pm, diff --git a/src/common/security/robot/context_test.go b/src/common/security/robot/context_test.go index 36a8a5316..a16cd40fe 100644 --- a/src/common/security/robot/context_test.go +++ b/src/common/security/robot/context_test.go @@ -26,6 +26,7 @@ import ( "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/promgr" "github.com/goharbor/harbor/src/core/promgr/pmsdriver/local" + "github.com/goharbor/harbor/src/pkg/robot/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -96,7 +97,7 @@ func TestIsAuthenticated(t *testing.T) { assert.False(t, ctx.IsAuthenticated()) // authenticated - ctx = NewSecurityContext(&models.Robot{ + ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, }, nil, nil) @@ -109,7 +110,7 @@ func TestGetUsername(t *testing.T) { assert.Equal(t, "", ctx.GetUsername()) // authenticated - ctx = NewSecurityContext(&models.Robot{ + ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, }, nil, nil) @@ -122,7 +123,7 @@ func TestIsSysAdmin(t *testing.T) { assert.False(t, ctx.IsSysAdmin()) // authenticated, non admin - ctx = NewSecurityContext(&models.Robot{ + ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, }, nil, nil) @@ -141,7 +142,7 @@ func TestHasPullPerm(t *testing.T) { Action: rbac.ActionPull, }, } - robot := &models.Robot{ + robot := &model.Robot{ Name: "test_robot_1", Description: "desc", } @@ -158,7 +159,7 @@ func TestHasPushPerm(t *testing.T) { Action: rbac.ActionPush, }, } - robot := &models.Robot{ + robot := &model.Robot{ Name: "test_robot_2", Description: "desc", } @@ -179,7 +180,7 @@ func TestHasPushPullPerm(t *testing.T) { Action: rbac.ActionPull, }, } - robot := &models.Robot{ + robot := &model.Robot{ Name: "test_robot_3", Description: "desc", } diff --git a/src/core/api/immutabletagrule_test.go b/src/core/api/immutabletagrule_test.go index 0877b442b..8a65e9e1a 100644 --- a/src/core/api/immutabletagrule_test.go +++ b/src/core/api/immutabletagrule_test.go @@ -97,9 +97,10 @@ func TestImmutableTagRuleAPI_List(t *testing.T) { func TestImmutableTagRuleAPI_Post(t *testing.T) { // body := `{ - // "id":0, - // "projectID":1, + // "projectID":1, // "priority":0, + // "template": "immutable_template", + // "action": "immutable", // "disabled":false, // "action":"immutable", // "template":"immutable_template", diff --git a/src/core/api/robot.go b/src/core/api/robot.go index 2d4c91012..5c42629a8 100644 --- a/src/core/api/robot.go +++ b/src/core/api/robot.go @@ -15,30 +15,29 @@ package api import ( - "errors" "fmt" - "net/http" - "strconv" - "time" - - "github.com/goharbor/harbor/src/common" "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/common/rbac/project" - "github.com/goharbor/harbor/src/common/token" - "github.com/goharbor/harbor/src/core/config" + "github.com/goharbor/harbor/src/pkg/robot" + "github.com/goharbor/harbor/src/pkg/robot/model" + "github.com/pkg/errors" + "net/http" + "strconv" ) // RobotAPI ... type RobotAPI struct { BaseController project *models.Project - robot *models.Robot + ctr robot.Controller + robot *model.Robot } // Prepare ... func (r *RobotAPI) Prepare() { + r.BaseController.Prepare() method := r.Ctx.Request.Method @@ -68,6 +67,7 @@ func (r *RobotAPI) Prepare() { return } r.project = project + r.ctr = robot.RobotCtr if method == http.MethodPut || method == http.MethodDelete { id, err := r.GetInt64FromPath(":id") @@ -75,8 +75,7 @@ func (r *RobotAPI) Prepare() { r.SendBadRequestError(errors.New("invalid robot ID")) return } - - robot, err := dao.GetRobotByID(id) + robot, err := r.ctr.GetRobotAccount(id) if err != nil { r.SendInternalServerError(fmt.Errorf("failed to get robot %d: %v", id, err)) return @@ -101,7 +100,7 @@ func (r *RobotAPI) Post() { return } - var robotReq models.RobotReq + var robotReq model.RobotCreate isValid, err := r.DecodeJSONReqAndValidate(&robotReq) if !isValid { r.SendBadRequestError(err) @@ -113,59 +112,25 @@ func (r *RobotAPI) Post() { return } - // Token duration in minutes - tokenDuration := time.Duration(config.RobotTokenDuration()) * time.Minute - expiresAt := time.Now().UTC().Add(tokenDuration).Unix() - createdName := common.RobotPrefix + robotReq.Name - - // first to add a robot account, and get its id. - robot := models.Robot{ - Name: createdName, - Description: robotReq.Description, - ProjectID: r.project.ProjectID, - ExpiresAt: expiresAt, - } - id, err := dao.AddRobot(&robot) + robot, err := r.ctr.CreateRobotAccount(&robotReq) if err != nil { if err == dao.ErrDupRows { r.SendConflictError(errors.New("conflict robot account")) return } - r.SendInternalServerError(fmt.Errorf("failed to create robot account: %v", err)) + r.SendInternalServerError(errors.Wrap(err, "robot API: post")) return } - // generate the token, and return it with response data. - // token is not stored in the database. - jwtToken, err := token.New(id, r.project.ProjectID, expiresAt, robotReq.Access) - if err != nil { - r.SendInternalServerError(fmt.Errorf("failed to valid parameters to generate token for robot account, %v", err)) - err := dao.DeleteRobot(id) - if err != nil { - r.SendInternalServerError(fmt.Errorf("failed to delete the robot account: %d, %v", id, err)) - } - return - } - - rawTk, err := jwtToken.Raw() - if err != nil { - r.SendInternalServerError(fmt.Errorf("failed to sign token for robot account, %v", err)) - err := dao.DeleteRobot(id) - if err != nil { - r.SendInternalServerError(fmt.Errorf("failed to delete the robot account: %d, %v", id, err)) - } - return - } - - robotRep := models.RobotRep{ - Name: robot.Name, - Token: rawTk, - } - w := r.Ctx.ResponseWriter w.Header().Set("Content-Type", "application/json") - r.Redirect(http.StatusCreated, strconv.FormatInt(id, 10)) + robotRep := model.RobotRep{ + Name: robot.Name, + Token: robot.Token, + } + + r.Redirect(http.StatusCreated, strconv.FormatInt(robot.ID, 10)) r.Data["json"] = robotRep r.ServeJSON() } @@ -176,28 +141,19 @@ func (r *RobotAPI) List() { return } - query := models.RobotQuery{ - ProjectID: r.project.ProjectID, - } - - count, err := dao.CountRobot(&query) + robots, err := r.ctr.ListRobotAccount(r.project.ProjectID) if err != nil { - r.SendInternalServerError(fmt.Errorf("failed to list robots on project: %d, %v", r.project.ProjectID, err)) + r.SendInternalServerError(errors.Wrap(err, "robot API: list")) return } - query.Page, query.Size, err = r.GetPaginationParams() + count := len(robots) + page, size, err := r.GetPaginationParams() if err != nil { r.SendBadRequestError(err) return } - robots, err := dao.ListRobots(&query) - if err != nil { - r.SendInternalServerError(fmt.Errorf("failed to get robots %v", err)) - return - } - - r.SetPaginationHeader(count, query.Page, query.Size) + r.SetPaginationHeader(int64(count), page, size) r.Data["json"] = robots r.ServeJSON() } @@ -214,13 +170,13 @@ func (r *RobotAPI) Get() { return } - robot, err := dao.GetRobotByID(id) + robot, err := r.ctr.GetRobotAccount(id) if err != nil { - r.SendInternalServerError(fmt.Errorf("failed to get robot %d: %v", id, err)) + r.SendInternalServerError(errors.Wrap(err, "robot API: get robot")) return } if robot == nil { - r.SendNotFoundError(fmt.Errorf("robot %d not found", id)) + r.SendNotFoundError(fmt.Errorf("robot API: robot %d not found", id)) return } @@ -234,7 +190,7 @@ func (r *RobotAPI) Put() { return } - var robotReq models.RobotReq + var robotReq model.RobotCreate if err := r.DecodeJSONReq(&robotReq); err != nil { r.SendBadRequestError(err) return @@ -242,8 +198,8 @@ func (r *RobotAPI) Put() { r.robot.Disabled = robotReq.Disabled - if err := dao.UpdateRobot(r.robot); err != nil { - r.SendInternalServerError(fmt.Errorf("failed to update robot %d: %v", r.robot.ID, err)) + if err := r.ctr.UpdateRobotAccount(r.robot); err != nil { + r.SendInternalServerError(errors.Wrap(err, "robot API: update")) return } @@ -255,13 +211,13 @@ func (r *RobotAPI) Delete() { return } - if err := dao.DeleteRobot(r.robot.ID); err != nil { - r.SendInternalServerError(fmt.Errorf("failed to delete robot %d: %v", r.robot.ID, err)) + if err := r.ctr.DeleteRobotAccount(r.robot.ID); err != nil { + r.SendInternalServerError(errors.Wrap(err, "robot API: delete")) return } } -func validateRobotReq(p *models.Project, robotReq *models.RobotReq) error { +func validateRobotReq(p *models.Project, robotReq *model.RobotCreate) error { if len(robotReq.Access) == 0 { return errors.New("access required") } diff --git a/src/core/api/robot_test.go b/src/core/api/robot_test.go index 6316e2602..c6644ca13 100644 --- a/src/core/api/robot_test.go +++ b/src/core/api/robot_test.go @@ -19,8 +19,8 @@ import ( "net/http" "testing" - "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/rbac" + "github.com/goharbor/harbor/src/pkg/robot/model" ) var ( @@ -53,7 +53,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{}, + bodyJSON: &model.RobotCreate{}, credential: nonSysAdmin, }, code: http.StatusForbidden, @@ -63,7 +63,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{ + bodyJSON: &model.RobotCreate{ Name: "test", Description: "test desc", Access: policies, @@ -77,7 +77,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{ + bodyJSON: &model.RobotCreate{ Name: "testIllgel#", Description: "test desc", }, @@ -89,7 +89,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{ + bodyJSON: &model.RobotCreate{ Name: "test", Description: "resource not exist", Access: []*rbac.Policy{ @@ -104,7 +104,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{ + bodyJSON: &model.RobotCreate{ Name: "test", Description: "action not exist", Access: []*rbac.Policy{ @@ -119,7 +119,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{ + bodyJSON: &model.RobotCreate{ Name: "test", Description: "policy not exit", Access: []*rbac.Policy{ @@ -135,7 +135,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{ + bodyJSON: &model.RobotCreate{ Name: "test2", Description: "test2 desc", }, @@ -149,7 +149,7 @@ func TestRobotAPIPost(t *testing.T) { request: &testingRequest{ method: http.MethodPost, url: robotPath, - bodyJSON: &models.RobotReq{ + bodyJSON: &model.RobotCreate{ Name: "test", Description: "test desc", Access: policies, @@ -306,7 +306,7 @@ func TestRobotAPIPut(t *testing.T) { request: &testingRequest{ method: http.MethodPut, url: fmt.Sprintf("%s/%d", robotPath, 1), - bodyJSON: &models.Robot{ + bodyJSON: &model.Robot{ Disabled: true, }, credential: projAdmin4Robot, diff --git a/src/core/filter/security.go b/src/core/filter/security.go index 785fbba72..b56429252 100644 --- a/src/core/filter/security.go +++ b/src/core/filter/security.go @@ -43,6 +43,7 @@ import ( "strings" "github.com/goharbor/harbor/src/pkg/authproxy" + "github.com/goharbor/harbor/src/pkg/robot" ) // ContextValueKey for content value @@ -194,7 +195,8 @@ func (r *robotAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { return false } // Do authn for robot account, as Harbor only stores the token ID, just validate the ID and disable. - robot, err := dao.GetRobotByID(htk.Claims.(*token.RobotClaims).TokenID) + ctr := robot.RobotCtr + robot, err := ctr.GetRobotAccount(htk.Claims.(*token.RobotClaims).TokenID) if err != nil { log.Errorf("failed to get robot %s: %v", robotName, err) return false diff --git a/src/pkg/robot/controller.go b/src/pkg/robot/controller.go new file mode 100644 index 000000000..7f793fdbf --- /dev/null +++ b/src/pkg/robot/controller.go @@ -0,0 +1,115 @@ +package robot + +import ( + "fmt" + "github.com/goharbor/harbor/src/common" + "github.com/goharbor/harbor/src/common/token" + "github.com/goharbor/harbor/src/common/utils/log" + "github.com/goharbor/harbor/src/core/config" + "github.com/goharbor/harbor/src/pkg/robot/model" + "github.com/pkg/errors" + "time" +) + +var ( + // RobotCtr is a global variable for the default robot account controller implementation + RobotCtr = NewController(NewDefaultRobotAccountManager()) +) + +// Controller to handle the requests related with robot account +type Controller interface { + // GetRobotAccount ... + GetRobotAccount(id int64) (*model.Robot, error) + + // CreateRobotAccount ... + CreateRobotAccount(robotReq *model.RobotCreate) (*model.Robot, error) + + // DeleteRobotAccount ... + DeleteRobotAccount(id int64) error + + // UpdateRobotAccount ... + UpdateRobotAccount(r *model.Robot) error + + // ListRobotAccount ... + ListRobotAccount(pid int64) ([]*model.Robot, error) +} + +// DefaultAPIController ... +type DefaultAPIController struct { + manager Manager +} + +// NewController ... +func NewController(robotMgr Manager) Controller { + return &DefaultAPIController{ + manager: robotMgr, + } +} + +// GetRobotAccount ... +func (d *DefaultAPIController) GetRobotAccount(id int64) (*model.Robot, error) { + return d.manager.GetRobotAccount(id) +} + +// CreateRobotAccount ... +func (d *DefaultAPIController) CreateRobotAccount(robotReq *model.RobotCreate) (*model.Robot, error) { + + var deferDel error + // Token duration in minutes + tokenDuration := time.Duration(config.RobotTokenDuration()) * time.Minute + expiresAt := time.Now().UTC().Add(tokenDuration).Unix() + createdName := common.RobotPrefix + robotReq.Name + + // first to add a robot account, and get its id. + robot := &model.Robot{ + Name: createdName, + Description: robotReq.Description, + ProjectID: robotReq.ProjectID, + ExpiresAt: expiresAt, + } + id, err := d.manager.CreateRobotAccount(robot) + if err != nil { + return nil, err + } + + // generate the token, and return it with response data. + // token is not stored in the database. + jwtToken, err := token.New(id, robotReq.ProjectID, expiresAt, robotReq.Access) + if err != nil { + deferDel = err + return nil, fmt.Errorf("failed to valid parameters to generate token for robot account, %v", err) + } + + rawTk, err := jwtToken.Raw() + if err != nil { + deferDel = err + return nil, fmt.Errorf("failed to sign token for robot account, %v", err) + } + + defer func(deferDel error) { + if deferDel != nil { + if err := d.manager.DeleteRobotAccount(id); err != nil { + log.Error(errors.Wrap(err, fmt.Sprintf("failed to delete the robot account: %d", id))) + } + } + }(deferDel) + + robot.Token = rawTk + robot.ID = id + return robot, nil +} + +// DeleteRobotAccount ... +func (d *DefaultAPIController) DeleteRobotAccount(id int64) error { + return d.manager.DeleteRobotAccount(id) +} + +// UpdateRobotAccount ... +func (d *DefaultAPIController) UpdateRobotAccount(r *model.Robot) error { + return d.manager.UpdateRobotAccount(r) +} + +// ListRobotAccount ... +func (d *DefaultAPIController) ListRobotAccount(pid int64) ([]*model.Robot, error) { + return d.manager.ListRobotAccount(pid) +} diff --git a/src/pkg/robot/controller_test.go b/src/pkg/robot/controller_test.go new file mode 100644 index 000000000..6557942bd --- /dev/null +++ b/src/pkg/robot/controller_test.go @@ -0,0 +1,103 @@ +package robot + +import ( + "github.com/goharbor/harbor/src/common" + "github.com/goharbor/harbor/src/common/rbac" + "github.com/goharbor/harbor/src/common/utils/test" + core_cfg "github.com/goharbor/harbor/src/core/config" + "github.com/goharbor/harbor/src/pkg/robot/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" + "testing" +) + +type ControllerTestSuite struct { + suite.Suite + ctr Controller + t *testing.T + assert *assert.Assertions + require *require.Assertions + + robotID int64 +} + +// SetupSuite ... +func (s *ControllerTestSuite) SetupSuite() { + test.InitDatabaseFromEnv() + conf := map[string]interface{}{ + common.RobotTokenDuration: "30", + } + core_cfg.InitWithSettings(conf) + s.t = s.T() + s.assert = assert.New(s.t) + s.require = require.New(s.t) + s.ctr = RobotCtr +} + +func (s *ControllerTestSuite) TestRobotAccount() { + + res := rbac.Resource("/project/1") + + rbacPolicy := &rbac.Policy{ + Resource: res.Subresource(rbac.ResourceRepository), + Action: "pull", + } + policies := []*rbac.Policy{} + policies = append(policies, rbacPolicy) + + robot1 := &model.RobotCreate{ + Name: "robot1", + Description: "TestCreateRobotAccount", + ProjectID: int64(1), + Access: policies, + } + + robot, err := s.ctr.CreateRobotAccount(robot1) + s.require.Nil(err) + s.require.Equal(robot.ProjectID, int64(1)) + s.require.Equal(robot.Description, "TestCreateRobotAccount") + s.require.NotEmpty(robot.Token) + s.require.Equal(robot.Name, common.RobotPrefix+"robot1") + + robotGet, err := s.ctr.GetRobotAccount(robot.ID) + s.require.Nil(err) + s.require.Equal(robotGet.ProjectID, int64(1)) + s.require.Equal(robotGet.Description, "TestCreateRobotAccount") + + robot.Disabled = true + err = s.ctr.UpdateRobotAccount(robot) + s.require.Nil(err) + s.require.Equal(robot.Disabled, true) + + robot2 := &model.RobotCreate{ + Name: "robot2", + Description: "TestCreateRobotAccount", + ProjectID: int64(1), + Access: policies, + } + r2, _ := s.ctr.CreateRobotAccount(robot2) + s.robotID = r2.ID + + robots, err := s.ctr.ListRobotAccount(int64(1)) + s.require.Nil(err) + s.require.Equal(len(robots), 2) + s.require.Equal(robots[1].Name, common.RobotPrefix+"robot2") + + err = s.ctr.DeleteRobotAccount(robot.ID) + s.require.Nil(err) + + robots, err = s.ctr.ListRobotAccount(int64(1)) + s.require.Equal(len(robots), 1) +} + +// TearDownSuite clears env for test suite +func (s *ControllerTestSuite) TearDownSuite() { + err := s.ctr.DeleteRobotAccount(s.robotID) + require.NoError(s.T(), err, "delete robot") +} + +// TestController ... +func TestController(t *testing.T) { + suite.Run(t, new(ControllerTestSuite)) +} diff --git a/src/pkg/robot/dao/robot.go b/src/pkg/robot/dao/robot.go new file mode 100644 index 000000000..fbfe1509d --- /dev/null +++ b/src/pkg/robot/dao/robot.go @@ -0,0 +1,121 @@ +package dao + +import ( + "fmt" + "github.com/astaxie/beego/orm" + "github.com/goharbor/harbor/src/common/dao" + "github.com/goharbor/harbor/src/pkg/q" + "github.com/goharbor/harbor/src/pkg/robot/model" + "strings" + "time" +) + +// RobotAccountDao defines the interface to access the ImmutableRule data model +type RobotAccountDao interface { + // CreateRobotAccount ... + CreateRobotAccount(robot *model.Robot) (int64, error) + + // UpdateRobotAccount ... + UpdateRobotAccount(robot *model.Robot) error + + // GetRobotAccount ... + GetRobotAccount(id int64) (*model.Robot, error) + + // ListRobotAccounts ... + ListRobotAccounts(query *q.Query) ([]*model.Robot, error) + + // DeleteRobotAccount ... + DeleteRobotAccount(id int64) error +} + +// New creates a default implementation for RobotAccountDao +func New() RobotAccountDao { + return &robotAccountDao{} +} + +type robotAccountDao struct{} + +// CreateRobotAccount ... +func (r *robotAccountDao) CreateRobotAccount(robot *model.Robot) (int64, error) { + now := time.Now() + robot.CreationTime = now + robot.UpdateTime = now + id, err := dao.GetOrmer().Insert(robot) + if err != nil { + if strings.Contains(err.Error(), "duplicate key value violates unique constraint") { + return 0, dao.ErrDupRows + } + return 0, err + } + return id, nil +} + +// GetRobotAccount ... +func (r *robotAccountDao) GetRobotAccount(id int64) (*model.Robot, error) { + robot := &model.Robot{ + ID: id, + } + if err := dao.GetOrmer().Read(robot); err != nil { + if err == orm.ErrNoRows { + return nil, nil + } + return nil, err + } + + return robot, nil +} + +// ListRobotAccounts ... +func (r *robotAccountDao) ListRobotAccounts(query *q.Query) ([]*model.Robot, error) { + o := dao.GetOrmer() + qt := o.QueryTable(new(model.Robot)) + + if query != nil { + if len(query.Keywords) > 0 { + for k, v := range query.Keywords { + qt = qt.Filter(fmt.Sprintf("%s__icontains", k), v) + } + } + + if query.PageNumber > 0 && query.PageSize > 0 { + qt = qt.Limit(query.PageSize, (query.PageNumber-1)*query.PageSize) + } + } + + robots := make([]*model.Robot, 0) + _, err := qt.All(&robots) + return robots, err +} + +// UpdateRobotAccount ... +func (r *robotAccountDao) UpdateRobotAccount(robot *model.Robot) error { + robot.UpdateTime = time.Now() + _, err := dao.GetOrmer().Update(robot) + return err +} + +// DeleteRobotAccount ... +func (r *robotAccountDao) DeleteRobotAccount(id int64) error { + _, err := dao.GetOrmer().QueryTable(&model.Robot{}).Filter("ID", id).Delete() + return err +} + +func getRobotQuerySetter(query *model.RobotQuery) orm.QuerySeter { + qs := dao.GetOrmer().QueryTable(&model.Robot{}) + + if query == nil { + return qs + } + + if len(query.Name) > 0 { + if query.FuzzyMatchName { + qs = qs.Filter("Name__icontains", query.Name) + } else { + qs = qs.Filter("Name", query.Name) + } + } + if query.ProjectID != 0 { + qs = qs.Filter("ProjectID", query.ProjectID) + } + return qs +} diff --git a/src/pkg/robot/dao/robot_test.go b/src/pkg/robot/dao/robot_test.go new file mode 100644 index 000000000..b55d722d4 --- /dev/null +++ b/src/pkg/robot/dao/robot_test.go @@ -0,0 +1,140 @@ +package dao + +import ( + "github.com/goharbor/harbor/src/common/dao" + "github.com/goharbor/harbor/src/pkg/q" + "github.com/goharbor/harbor/src/pkg/robot/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" + "testing" +) + +type robotAccountDaoTestSuite struct { + suite.Suite + require *require.Assertions + assert *assert.Assertions + dao RobotAccountDao + id1 int64 + id2 int64 + id3 int64 + id4 int64 +} + +func (t *robotAccountDaoTestSuite) SetupSuite() { + t.require = require.New(t.T()) + t.assert = assert.New(t.T()) + dao.PrepareTestForPostgresSQL() + t.dao = New() +} + +func (t *robotAccountDaoTestSuite) TestCreateRobotAccount() { + robotName := "test1" + robot := &model.Robot{ + Name: robotName, + Description: "test1 description", + ProjectID: 1, + } + id, err := t.dao.CreateRobotAccount(robot) + t.require.Nil(err) + t.id1 = id + t.require.Nil(err) + t.require.NotNil(id) +} + +func (t *robotAccountDaoTestSuite) TestGetRobotAccount() { + robotName := "test2" + robot := &model.Robot{ + Name: robotName, + Description: "test2 description", + ProjectID: 1, + } + + // add + id, err := t.dao.CreateRobotAccount(robot) + t.require.Nil(err) + t.id2 = id + + robot, err = t.dao.GetRobotAccount(id) + t.require.Nil(err) + t.require.Equal(robotName, robot.Name) +} + +func (t *robotAccountDaoTestSuite) TestListRobotAccounts() { + robotName := "test3" + robot := &model.Robot{ + Name: robotName, + Description: "test3 description", + ProjectID: 1, + } + + id, err := t.dao.CreateRobotAccount(robot) + t.require.Nil(err) + t.id3 = id + + keywords := make(map[string]interface{}) + keywords["ProjectID"] = 1 + robots, err := t.dao.ListRobotAccounts(&q.Query{ + Keywords: keywords, + }) + t.require.Nil(err) + t.require.Equal(3, len(robots)) +} + +func (t *robotAccountDaoTestSuite) TestUpdateRobotAccount() { + robotName := "test4" + robot := &model.Robot{ + Name: robotName, + Description: "test4 description", + ProjectID: 1, + } + // add + id, err := t.dao.CreateRobotAccount(robot) + t.require.Nil(err) + t.id4 = id + // Disable + robot.Disabled = true + err = t.dao.UpdateRobotAccount(robot) + t.require.Nil(err) + // Get + robot, err = t.dao.GetRobotAccount(id) + t.require.Nil(err) + t.require.Equal(true, robot.Disabled) +} + +func (t *robotAccountDaoTestSuite) TestDeleteRobotAccount() { + robotName := "test5" + robot := &model.Robot{ + Name: robotName, + Description: "test5 description", + ProjectID: 1, + } + // add + id, err := t.dao.CreateRobotAccount(robot) + t.require.Nil(err) + // Disable + err = t.dao.DeleteRobotAccount(id) + t.require.Nil(err) + // Get + robot, err = t.dao.GetRobotAccount(id) + t.require.Nil(err) +} + +// TearDownSuite clears env for test suite +func (t *robotAccountDaoTestSuite) TearDownSuite() { + err := t.dao.DeleteRobotAccount(t.id1) + require.NoError(t.T(), err, "delete robot 1") + + err = t.dao.DeleteRobotAccount(t.id2) + require.NoError(t.T(), err, "delete robot 2") + + err = t.dao.DeleteRobotAccount(t.id3) + require.NoError(t.T(), err, "delete robot 3") + + err = t.dao.DeleteRobotAccount(t.id4) + require.NoError(t.T(), err, "delete robot 4") +} + +func TestRobotAccountDaoTestSuite(t *testing.T) { + suite.Run(t, &robotAccountDaoTestSuite{}) +} diff --git a/src/pkg/robot/manager.go b/src/pkg/robot/manager.go new file mode 100644 index 000000000..57a86b7e1 --- /dev/null +++ b/src/pkg/robot/manager.go @@ -0,0 +1,71 @@ +package robot + +import ( + "github.com/goharbor/harbor/src/pkg/q" + "github.com/goharbor/harbor/src/pkg/robot/dao" + "github.com/goharbor/harbor/src/pkg/robot/model" +) + +var ( + // Mgr is a global variable for the default robot account manager implementation + Mgr = NewDefaultRobotAccountManager() +) + +// Manager ... +type Manager interface { + // GetRobotAccount ... + GetRobotAccount(id int64) (*model.Robot, error) + + // CreateRobotAccount ... + CreateRobotAccount(m *model.Robot) (int64, error) + + // DeleteRobotAccount ... + DeleteRobotAccount(id int64) error + + // UpdateRobotAccount ... + UpdateRobotAccount(m *model.Robot) error + + // ListRobotAccount ... + ListRobotAccount(pid int64) ([]*model.Robot, error) +} + +type defaultRobotManager struct { + dao dao.RobotAccountDao +} + +// NewDefaultRobotAccountManager return a new instance of defaultRobotManager +func NewDefaultRobotAccountManager() Manager { + return &defaultRobotManager{ + dao: dao.New(), + } +} + +// GetRobotAccount ... +func (drm *defaultRobotManager) GetRobotAccount(id int64) (*model.Robot, error) { + return drm.dao.GetRobotAccount(id) +} + +// CreateRobotAccount ... +func (drm *defaultRobotManager) CreateRobotAccount(r *model.Robot) (int64, error) { + return drm.dao.CreateRobotAccount(r) +} + +// DeleteRobotAccount ... +func (drm *defaultRobotManager) DeleteRobotAccount(id int64) error { + return drm.dao.DeleteRobotAccount(id) +} + +// UpdateRobotAccount ... +func (drm *defaultRobotManager) UpdateRobotAccount(r *model.Robot) error { + return drm.dao.UpdateRobotAccount(r) +} + +// ListRobotAccount ... +func (drm *defaultRobotManager) ListRobotAccount(pid int64) ([]*model.Robot, error) { + keywords := make(map[string]interface{}) + keywords["ProjectID"] = pid + query := q.Query{ + Keywords: keywords, + } + return drm.dao.ListRobotAccounts(&query) +} diff --git a/src/pkg/robot/manager_test.go b/src/pkg/robot/manager_test.go new file mode 100644 index 000000000..fad225969 --- /dev/null +++ b/src/pkg/robot/manager_test.go @@ -0,0 +1,142 @@ +package robot + +import ( + "github.com/goharbor/harbor/src/pkg/q" + "github.com/goharbor/harbor/src/pkg/robot/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" + "os" + "testing" +) + +type mockRobotDao struct { + mock.Mock +} + +func (m *mockRobotDao) CreateRobotAccount(r *model.Robot) (int64, error) { + args := m.Called(r) + return int64(args.Int(0)), args.Error(1) +} + +func (m *mockRobotDao) UpdateRobotAccount(r *model.Robot) error { + args := m.Called(r) + return args.Error(1) +} + +func (m *mockRobotDao) DeleteRobotAccount(id int64) error { + args := m.Called(id) + return args.Error(1) +} + +func (m *mockRobotDao) GetRobotAccount(id int64) (*model.Robot, error) { + args := m.Called(id) + var r *model.Robot + if args.Get(0) != nil { + r = args.Get(0).(*model.Robot) + } + return r, args.Error(1) +} + +func (m *mockRobotDao) ListRobotAccounts(query *q.Query) ([]*model.Robot, error) { + args := m.Called() + var rs []*model.Robot + if args.Get(0) != nil { + rs = args.Get(0).([]*model.Robot) + } + return rs, args.Error(1) +} + +type managerTestingSuite struct { + suite.Suite + t *testing.T + assert *assert.Assertions + require *require.Assertions + mockRobotDao *mockRobotDao +} + +func (m *managerTestingSuite) SetupSuite() { + m.t = m.T() + m.assert = assert.New(m.t) + m.require = require.New(m.t) + + err := os.Setenv("RUN_MODE", "TEST") + m.require.Nil(err) +} + +func (m *managerTestingSuite) TearDownSuite() { + err := os.Unsetenv("RUN_MODE") + m.require.Nil(err) +} + +func (m *managerTestingSuite) SetupTest() { + m.mockRobotDao = &mockRobotDao{} + Mgr = &defaultRobotManager{ + dao: m.mockRobotDao, + } +} + +func TestManagerTestingSuite(t *testing.T) { + suite.Run(t, &managerTestingSuite{}) +} + +func (m *managerTestingSuite) TestCreateRobotAccount() { + m.mockRobotDao.On("CreateRobotAccount", mock.Anything).Return(1, nil) + id, err := Mgr.CreateRobotAccount(&model.Robot{}) + m.mockRobotDao.AssertCalled(m.t, "CreateRobotAccount", mock.Anything) + m.require.Nil(err) + m.assert.Equal(int64(1), id) +} + +func (m *managerTestingSuite) TestUpdateRobotAccount() { + m.mockRobotDao.On("UpdateRobotAccount", mock.Anything).Return(1, nil) + err := Mgr.UpdateRobotAccount(&model.Robot{}) + m.mockRobotDao.AssertCalled(m.t, "UpdateRobotAccount", mock.Anything) + m.require.Nil(err) +} + +func (m *managerTestingSuite) TestDeleteRobotAccount() { + m.mockRobotDao.On("DeleteRobotAccount", mock.Anything).Return(1, nil) + err := Mgr.DeleteRobotAccount(int64(1)) + m.mockRobotDao.AssertCalled(m.t, "DeleteRobotAccount", mock.Anything) + m.require.Nil(err) +} + +func (m *managerTestingSuite) TestGetRobotAccount() { + m.mockRobotDao.On("GetRobotAccount", mock.Anything).Return(&model.Robot{ + ID: 1, + ProjectID: 1, + Disabled: true, + ExpiresAt: 150000, + }, nil) + ir, err := Mgr.GetRobotAccount(1) + m.mockRobotDao.AssertCalled(m.t, "GetRobotAccount", mock.Anything) + m.require.Nil(err) + m.require.NotNil(ir) + m.assert.Equal(int64(1), ir.ID) +} + +func (m *managerTestingSuite) ListRobotAccount() { + m.mockRobotDao.On("ListRobotAccount", mock.Anything).Return([]model.Robot{ + { + ID: 1, + ProjectID: 1, + Disabled: false, + ExpiresAt: 12345, + }, + { + ID: 2, + ProjectID: 1, + Disabled: false, + ExpiresAt: 54321, + }}, nil) + + rs, err := Mgr.ListRobotAccount(int64(1)) + m.mockRobotDao.AssertCalled(m.t, "ListRobotAccount", mock.Anything) + m.require.Nil(err) + m.assert.Equal(len(rs), 2) + m.assert.Equal(rs[0].Disabled, false) + m.assert.Equal(rs[1].ExpiresAt, 54321) + +} diff --git a/src/common/models/robot.go b/src/pkg/robot/model/robot.go similarity index 70% rename from src/common/models/robot.go rename to src/pkg/robot/model/robot.go index 2e64ca8d2..a83a6f13a 100644 --- a/src/common/models/robot.go +++ b/src/pkg/robot/model/robot.go @@ -1,20 +1,7 @@ -// Copyright Project Harbor Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package models +package model import ( + "github.com/astaxie/beego/orm" "github.com/astaxie/beego/validation" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/utils" @@ -24,10 +11,15 @@ import ( // RobotTable is the name of table in DB that holds the robot object const RobotTable = "robot" +func init() { + orm.RegisterModel(&Robot{}) +} + // Robot holds the details of a robot. type Robot struct { ID int64 `orm:"pk;auto;column(id)" json:"id"` Name string `orm:"column(name)" json:"name"` + Token string `orm:"-" json:"token"` Description string `orm:"column(description)" json:"description"` ProjectID int64 `orm:"column(project_id)" json:"project_id"` ExpiresAt int64 `orm:"column(expiresat)" json:"expires_at"` @@ -36,6 +28,11 @@ type Robot struct { UpdateTime time.Time `orm:"column(update_time);auto_now" json:"update_time"` } +// TableName ... +func (r *Robot) TableName() string { + return RobotTable +} + // RobotQuery ... type RobotQuery struct { Name string @@ -45,16 +42,23 @@ type RobotQuery struct { Pagination } -// RobotReq ... -type RobotReq struct { +// RobotCreate ... +type RobotCreate struct { Name string `json:"name"` + ProjectID int64 `json:"pid"` Description string `json:"description"` Disabled bool `json:"disabled"` Access []*rbac.Policy `json:"access"` } +// Pagination ... +type Pagination struct { + Page int64 + Size int64 +} + // Valid ... -func (rq *RobotReq) Valid(v *validation.Validation) { +func (rq *RobotCreate) Valid(v *validation.Validation) { if utils.IsIllegalLength(rq.Name, 1, 255) { v.SetError("name", "robot name with illegal length") } @@ -68,8 +72,3 @@ type RobotRep struct { Name string `json:"name"` Token string `json:"token"` } - -// TableName ... -func (r *Robot) TableName() string { - return RobotTable -}