Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add verification that robot account duration is not 0 #19829

Merged
merged 1 commit into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions api/v2.0/swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4719,7 +4719,7 @@ paths:
summary: Get job log by job id
description: Get job log by job id, it is only used by administrator
produces:
- text/plain
- text/plain
tags:
- jobservice
parameters:
Expand Down Expand Up @@ -6071,7 +6071,7 @@ paths:
description: Specify whether the dangerous Artifact are included inside summary information
type: boolean
required: false
default: false
default: false
responses:
'200':
description: Success
Expand All @@ -6090,15 +6090,15 @@ paths:
get:
summary: Get the vulnerability list.
description: |
Get the vulnerability list. use q to pass the query condition,
Get the vulnerability list. use q to pass the query condition,
supported conditions:
cve_id(exact match)
cvss_score_v3(range condition)
severity(exact match)
repository_name(exact match)
project_id(exact match)
repository_name(exact match)
project_id(exact match)
package(exact match)
tag(exact match)
tag(exact match)
digest(exact match)
tags:
- securityhub
Expand Down Expand Up @@ -7656,8 +7656,9 @@ definitions:
description: The level of the robot, project or system
duration:
type: integer
x-nullable: true
format: int64
description: The duration of the robot in days
description: The duration of the robot in days, duration must be either -1(Never) or a positive integer
editable:
type: boolean
x-omitempty: false
Expand Down Expand Up @@ -7704,7 +7705,7 @@ definitions:
duration:
type: integer
format: int64
description: The duration of the robot in days
description: The duration of the robot in days, duration must be either -1(Never) or a positive integer
permissions:
type: array
items:
Expand Down Expand Up @@ -7994,7 +7995,7 @@ definitions:
type: string
description: |
The schedule type. The valid values are 'Hourly', 'Daily', 'Weekly', 'Custom', 'Manual', 'None' and 'Schedule'.
'Manual' means to trigger it right away, 'Schedule' means to trigger it by a specified cron schedule and
'Manual' means to trigger it right away, 'Schedule' means to trigger it by a specified cron schedule and
'None' means to cancel the schedule.
enum:
- Hourly
Expand Down Expand Up @@ -9813,12 +9814,12 @@ definitions:
type: object
description: the dangerous CVE information
properties:
cve_id:
cve_id:
type: string
description: the cve id
severity:
type: string
description: the severity of the CVE
description: the severity of the CVE
cvss_score_v3:
type: number
format: float64
Expand All @@ -9828,22 +9829,22 @@ definitions:
description: the description of the CVE
package:
type: string
description: the package of the CVE
description: the package of the CVE
version:
type: string
description: the version of the package
DangerousArtifact:
type: object
description: the dangerous artifact information
properties:
project_id:
project_id:
type: integer
format: int64
description: the project id of the artifact
repository_name:
type: string
description: the repository name of the artifact
digest:
digest:
type: string
description: the digest of the artifact
critical_cnt:
Expand Down Expand Up @@ -9903,6 +9904,6 @@ definitions:
description: The description of the vulnerability
links:
type: array
items:
items:
type: string
description: Links of the vulnerability
4 changes: 0 additions & 4 deletions src/controller/robot/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,6 @@ func (d *controller) Create(ctx context.Context, r *Robot) (int64, string, error
var expiresAt int64
if r.Duration == -1 {
expiresAt = -1
} else if r.Duration == 0 {
// system default robot duration
r.Duration = int64(config.RobotTokenDuration(ctx))
expiresAt = time.Now().AddDate(0, 0, config.RobotTokenDuration(ctx)).Unix()
} else {
durationStr := strconv.FormatInt(r.Duration, 10)
duration, err := strconv.Atoi(durationStr)
Expand Down
1 change: 1 addition & 0 deletions src/controller/scan/base_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,7 @@ func (bc *basicController) makeRobotAccount(ctx context.Context, projectID int64
Name: fmt.Sprintf("%s-%s-%s", scannerPrefix, registration.Name, UUID),
Description: "for scan",
ProjectID: projectID,
Duration: -1,
},
Level: robot.LEVELPROJECT,
Permissions: []*robot.Permission{
Expand Down
4 changes: 3 additions & 1 deletion src/controller/scan/base_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ func (suite *ControllerTestSuite) SetupSuite() {
Name: rname,
Description: "for scan",
ProjectID: suite.artifact.ProjectID,
Duration: -1,
},
Level: robot.LEVELPROJECT,
Permissions: []*robot.Permission{
Expand Down Expand Up @@ -229,6 +230,7 @@ func (suite *ControllerTestSuite) SetupSuite() {
Secret: "robot-account",
Description: "for scan",
ProjectID: suite.artifact.ProjectID,
Duration: -1,
},
Level: "project",
}, nil)
Expand Down Expand Up @@ -336,7 +338,7 @@ func (suite *ControllerTestSuite) TestScanControllerScan() {
mock.OnAnything(suite.execMgr, "Create").Return(int64(1), nil).Once()
mock.OnAnything(suite.taskMgr, "Create").Return(int64(1), nil).Once()

ctx := orm.NewContext(nil, &ormtesting.FakeOrmer{})
ctx := orm.NewContext(context.TODO(), &ormtesting.FakeOrmer{})

suite.Require().NoError(suite.c.Scan(ctx, suite.artifact))
}
Expand Down
2 changes: 1 addition & 1 deletion src/server/v2.0/handler/model/robot.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
Name: r.Name,
Description: r.Description,
ExpiresAt: r.ExpiresAt,
Duration: r.Duration,
Duration: &r.Duration,

Check warning on line 47 in src/server/v2.0/handler/model/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/model/robot.go#L47

Added line #L47 was not covered by tests
Level: r.Level,
Disable: r.Disabled,
Editable: r.Editable,
Expand Down
21 changes: 10 additions & 11 deletions src/server/v2.0/handler/robot.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
"github.com/goharbor/harbor/src/common/utils"
"github.com/goharbor/harbor/src/controller/robot"
"github.com/goharbor/harbor/src/lib"
"github.com/goharbor/harbor/src/lib/config"
"github.com/goharbor/harbor/src/lib/errors"
"github.com/goharbor/harbor/src/lib/log"
"github.com/goharbor/harbor/src/pkg/permission/types"
Expand Down Expand Up @@ -276,7 +275,7 @@
// more validation
func (rAPI *robotAPI) validate(d int64, level string, permissions []*models.RobotPermission) error {
if !isValidDuration(d) {
return errors.New(nil).WithMessage("bad request error duration input: %d", d).WithCode(errors.BadRequestCode)
return errors.New(nil).WithMessage("bad request error duration input: %d, duration must be either -1(Never) or a positive integer", d).WithCode(errors.BadRequestCode)

Check warning on line 278 in src/server/v2.0/handler/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/robot.go#L278

Added line #L278 was not covered by tests
}

if !isValidLevel(level) {
Expand Down Expand Up @@ -323,7 +322,10 @@
}

func (rAPI *robotAPI) updateV2Robot(ctx context.Context, params operation.UpdateRobotParams, r *robot.Robot) error {
if err := rAPI.validate(params.Robot.Duration, params.Robot.Level, params.Robot.Permissions); err != nil {
if params.Robot.Duration == nil {
params.Robot.Duration = &r.Duration
}
if err := rAPI.validate(*params.Robot.Duration, params.Robot.Level, params.Robot.Permissions); err != nil {

Check warning on line 328 in src/server/v2.0/handler/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/robot.go#L325-L328

Added lines #L325 - L328 were not covered by tests
return err
}
if r.Level != robot.LEVELSYSTEM {
Expand All @@ -342,15 +344,12 @@
return errors.BadRequestError(nil).WithMessage("cannot update the level or name of robot")
}

if r.Duration != params.Robot.Duration {
r.Duration = params.Robot.Duration
if params.Robot.Duration == -1 {
if r.Duration != *params.Robot.Duration {
r.Duration = *params.Robot.Duration
if *params.Robot.Duration == -1 {

Check warning on line 349 in src/server/v2.0/handler/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/robot.go#L347-L349

Added lines #L347 - L349 were not covered by tests
r.ExpiresAt = -1
} else if params.Robot.Duration == 0 {
r.Duration = int64(config.RobotTokenDuration(ctx))
r.ExpiresAt = r.CreationTime.AddDate(0, 0, config.RobotTokenDuration(ctx)).Unix()
} else {
r.ExpiresAt = r.CreationTime.AddDate(0, 0, int(params.Robot.Duration)).Unix()
r.ExpiresAt = r.CreationTime.AddDate(0, 0, int(*params.Robot.Duration)).Unix()

Check warning on line 352 in src/server/v2.0/handler/robot.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/robot.go#L352

Added line #L352 was not covered by tests
}
}

Expand All @@ -375,7 +374,7 @@
}

func isValidDuration(d int64) bool {
return d >= int64(-1) && d < math.MaxInt32
return d >= int64(-1) && d != 0 && d < math.MaxInt32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d == -1 || (d > 0 && d <= math.MaxInt32)

is more readable?

}

// validateName validates the robot name, especially '+' cannot be a valid character
Expand Down
2 changes: 1 addition & 1 deletion src/server/v2.0/handler/robot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestValidDuration(t *testing.T) {
}{
{"duration 0",
0,
true,
false,
},
{"duration 1",
1,
Expand Down
Loading