Merge pull request #13685 from wy65701436/robot-name-conflict

fix robot name conflicate issue

src/controller/robot/controller.go

@@ -76,7 +76,7 @@ func (d *controller) Count(ctx context.Context, query *q.Query) (int64, error) {
 
 // Create ...
 func (d *controller) Create(ctx context.Context, r *Robot) (int64, string, error) {
-	if err := d.setProjectID(ctx, r); err != nil {
+	if err := d.setProject(ctx, r); err != nil {
 		return 0, "", err
 	}
 
@@ -95,8 +95,13 @@ func (d *controller) Create(ctx context.Context, r *Robot) (int64, string, error
 	salt := utils.GenerateRandomString()
 	secret := utils.Encrypt(pwd, salt, utils.SHA256)
 
+	name := r.Name
+	// for the project level robot, set the name pattern as projectname+robotname, and + is a illegal character.
+	if r.Level == LEVELPROJECT {
+		name = fmt.Sprintf("%s+%s", r.ProjectName, r.Name)
+	}
 	robotID, err := d.robotMgr.Create(ctx, &model.Robot{
-		Name:        r.Name,
+		Name:        name,
 		Description: r.Description,
 		ProjectID:   r.ProjectID,
 		ExpiresAt:   expiresAt,
@@ -208,10 +213,7 @@ func (d *controller) populate(ctx context.Context, r *model.Robot, option *Optio
 	robot.Name = fmt.Sprintf("%s%s", config.RobotPrefix(), r.Name)
 	robot.setLevel()
 	robot.setEditable()
-	if option == nil {
-		return robot, nil
-	}
-	if option.WithPermission {
+	if option != nil && option.WithPermission {
 		if err := d.populatePermissions(ctx, robot); err != nil {
 			return nil, err
 		}
@@ -273,24 +275,19 @@ func (d *controller) populatePermissions(ctx context.Context, r *Robot) error {
 	return nil
 }
 
-func (d *controller) setProjectID(ctx context.Context, r *Robot) error {
+// set the project info if it's a project level robot
+func (d *controller) setProject(ctx context.Context, r *Robot) error {
 	if r == nil {
 		return nil
 	}
-	var projectID int64
-	switch r.Level {
-	case LEVELSYSTEM:
-		projectID = 0
-	case LEVELPROJECT:
+	if r.Level == LEVELPROJECT {
 		pro, err := d.proMgr.Get(ctx, r.Permissions[0].Namespace)
 		if err != nil {
 			return err
 		}
-		projectID = pro.ProjectID
-	default:
-		return errors.New(nil).WithMessage("unknown robot account level").WithCode(errors.BadRequestCode)
+		r.ProjectName = pro.Name
+		r.ProjectID = pro.ProjectID
 	}
-	r.ProjectID = projectID
 	return nil
 }
 

src/controller/robot/controller_test.go

@@ -34,7 +34,7 @@ func (suite *ControllerTestSuite) TestGet() {
 	ctx := context.TODO()
 	projectMgr.On("Get", mock.Anything, mock.Anything).Return(&models.Project{ProjectID: 1, Name: "library"}, nil)
 	robotMgr.On("Get", mock.Anything, mock.Anything).Return(&model.Robot{
-		Name:        "test",
+		Name:        "library+test",
 		Description: "test get method",
 		ProjectID:   1,
 		Secret:      utils.RandStringBytes(10),

src/server/middleware/security/robot2.go

@@ -10,10 +10,11 @@ import (
 	"github.com/goharbor/harbor/src/lib/log"
 	"github.com/goharbor/harbor/src/lib/q"
 	"github.com/goharbor/harbor/src/pkg/permission/types"
+	"strings"
+	"time"
 
 	"github.com/goharbor/harbor/src/pkg/robot2/model"
 	"net/http"
-	"strings"
 )
 
 type robot2 struct{}
@@ -27,10 +28,10 @@ func (r *robot2) Generate(req *http.Request) security.Context {
 	if !strings.HasPrefix(name, config.RobotPrefix()) {
 		return nil
 	}
-
-	// TODO use the naming pattern to avoid the permission boundary crossing.
+	name = strings.TrimPrefix(name, config.RobotPrefix())
+	// The robot name can be used as the unique identifier to locate robot as it contains the project name.
 	robots, err := robot_ctl.Ctl.List(req.Context(), q.New(q.KeyWords{
-		"name": strings.TrimPrefix(name, config.RobotPrefix()),
+		"name": name,
 	}), &robot_ctl.Option{
 		WithPermission: true,
 	})
@@ -51,7 +52,11 @@ func (r *robot2) Generate(req *http.Request) security.Context {
 		log.Errorf("failed to authenticate disabled robot account: %s", name)
 		return nil
 	}
-	// add the expiration check
+	now := time.Now().Unix()
+	if robot.ExpiresAt != -1 && robot.ExpiresAt <= now {
+		log.Errorf("the robot account is expired: %s", name)
+		return nil
+	}
 
 	var accesses []*types.Policy
 	for _, p := range robot.Permissions {
@@ -65,7 +70,7 @@ func (r *robot2) Generate(req *http.Request) security.Context {
 	}
 
 	modelRobot := &model.Robot{
-		Name: strings.TrimPrefix(name, config.RobotPrefix()),
+		Name: name,
 	}
 	log.Infof("a robot2 security context generated for request %s %s", req.Method, req.URL.Path)
 	return robotCtx.NewSecurityContext(modelRobot, robot.Level == robot_ctl.LEVELSYSTEM, accesses)

src/server/middleware/security/robot2_test.go

@@ -2,24 +2,16 @@ package security
 
 import (
 	"github.com/goharbor/harbor/src/common"
-	"github.com/goharbor/harbor/src/common/utils/test"
 	core_cfg "github.com/goharbor/harbor/src/core/config"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"net/http"
-	"os"
 	"testing"
 )
 
 func TestRobot2(t *testing.T) {
-	secretKeyPath := "/tmp/secretkey"
-	_, err := test.GenerateKey(secretKeyPath)
-	assert.Nil(t, err)
-	defer os.Remove(secretKeyPath)
-	os.Setenv("KEY_PATH", secretKeyPath)
-
 	conf := map[string]interface{}{
-		common.RobotPrefix: "robot@",
+		common.RobotNamePrefix: "robot@",
 	}
 	core_cfg.InitWithSettings(conf)
 

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

@@ -32,6 +32,10 @@ type robotAPI struct {
 }
 
 func (rAPI *robotAPI) CreateRobot(ctx context.Context, params operation.CreateRobotParams) middleware.Responder {
+	if err := validateName(params.Robot.Name); err != nil {
+		return rAPI.SendError(ctx, err)
+	}
+
 	if err := rAPI.validate(params.Robot.Duration, params.Robot.Level, params.Robot.Permissions); err != nil {
 		return rAPI.SendError(ctx, err)
 	}
@@ -306,3 +310,13 @@ func isValidSec(sec string) bool {
 	}
 	return false
 }
+
+// validateName validates the robot name, especially '+' cannot be a valid character
+func validateName(name string) error {
+	robotNameReg := `^[a-z0-9]+(?:[._-][a-z0-9]+)*$`
+	legal := regexp.MustCompile(robotNameReg).MatchString(name)
+	if !legal {
+		return errors.BadRequestError(nil).WithMessage("robot name is not in lower case or contains illegal characters")
+	}
+	return nil
+}