Replace comma in username to avoid casbin issue

Check username when creating user by API Replace comma with underscore in username for OnboardUser Fixes #19356 Signed-off-by: stonezdj <[email protected]>
goharbor · Nov 2, 2023 · 88ed1b3 · 88ed1b3
1 parent f75a2f9
commit 88ed1b3
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 13 deletions.
diff --git a/src/common/const.go b/src/common/const.go
@@ -228,4 +228,6 @@ const (
 	ExecutionStatusRefreshIntervalSeconds = "execution_status_refresh_interval_seconds"
 	// QuotaUpdateProvider is the provider for updating quota, currently support Redis and DB
 	QuotaUpdateProvider = "quota_update_provider"
+	// IllegalCharsInUsername is the illegal chars in username
+	IllegalCharsInUsername = `,"~#%$`
 )
diff --git a/src/common/utils/utils.go b/src/common/utils/utils.go
@@ -247,16 +247,6 @@ func IsIllegalLength(s string, min int, max int) bool {
 	return (len(s) < min || len(s) > max)
 }
 
-// IsContainIllegalChar ...
-func IsContainIllegalChar(s string, illegalChar []string) bool {
-	for _, c := range illegalChar {
-		if strings.Contains(s, c) {
-			return true
-		}
-	}
-	return false
-}
-
 // ParseJSONInt ...
 func ParseJSONInt(value interface{}) (int, bool) {
 	switch v := value.(type) {

diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go
@@ -247,8 +247,8 @@ func (oc *OIDCController) Onboard() {
 		oc.SendBadRequestError(errors.New("username with illegal length"))
 		return
 	}
-	if utils.IsContainIllegalChar(username, []string{",", "~", "#", "$", "%"}) {
-		oc.SendBadRequestError(errors.New("username contains illegal characters"))
+	if strings.ContainsAny(username, common.IllegalCharsInUsername) {
+		oc.SendBadRequestError(errors.Errorf("username %v contains illegal characters: %v", username, common.IllegalCharsInUsername))
 		return
 	}
 

diff --git a/src/pkg/user/manager.go b/src/pkg/user/manager.go
@@ -165,6 +165,10 @@ func (m *manager) SetSysAdminFlag(ctx context.Context, id int, admin bool) error
 
 func (m *manager) Create(ctx context.Context, user *commonmodels.User) (int, error) {
 	injectPasswd(user, user.Password)
+	// replace comma in username with underscore to avoid #19356
+	// if the username conflict with existing username,
+	// it will return error and have to update to a new username manually with sql
+	user.Username = strings.Replace(user.Username, ",", "_", -1)
 	return m.dao.Create(ctx, user)
 }
 

diff --git a/src/pkg/user/manager_test.go b/src/pkg/user/manager_test.go
@@ -143,3 +143,27 @@ func TestInjectPasswd(t *testing.T) {
 	assert.Equal(t, "sha256", u.PasswordVersion)
 	assert.Equal(t, utils.Encrypt(p, u.Salt, "sha256"), u.Password)
 }
+
+func (m *mgrTestSuite) TestCreate() {
+	m.dao.On("Create", mock.Anything, testifymock.Anything).Return(3, nil)
+	u := &models.User{
+		Username: "test",
+		Email:    "[email protected]",
+		Realname: "test",
+	}
+	id, err := m.mgr.Create(context.Background(), u)
+	m.Nil(err)
+	m.Equal(3, id)
+	m.Equal(u.Username, "test")
+
+	u2 := &models.User{
+		Username: "test,test",
+		Email:    "[email protected]",
+		Realname: "test",
+	}
+
+	id, err = m.mgr.Create(context.Background(), u2)
+	m.Nil(err)
+	m.Equal(3, id)
+	m.Equal(u2.Username, "test_test", "username should be sanitized")
+}
diff --git a/src/server/v2.0/handler/user.go b/src/server/v2.0/handler/user.go
@@ -495,10 +495,18 @@ func validateUserProfile(user *commonmodels.User) error {
 		return errors.BadRequestError(nil).WithMessage("realname with illegal length")
 	}
 
-	if utils.IsContainIllegalChar(user.Realname, []string{",", "~", "#", "$", "%"}) {
+	if strings.ContainsAny(user.Realname, common.IllegalCharsInUsername) {
 		return errors.BadRequestError(nil).WithMessage("realname contains illegal characters")
 	}
 
+	if utils.IsIllegalLength(user.Username, 1, 255) {
+		return errors.BadRequestError(nil).WithMessage("usernamae with illegal length")
+	}
+
+	if strings.ContainsAny(user.Username, common.IllegalCharsInUsername) {
+		return errors.BadRequestError(nil).WithMessage("username contains illegal characters")
+	}
+
 	if utils.IsIllegalLength(user.Comment, -1, 30) {
 		return errors.BadRequestError(nil).WithMessage("comment with illegal length")
 	}

diff --git a/src/server/v2.0/handler/user_test.go b/src/server/v2.0/handler/user_test.go
@@ -2,6 +2,7 @@ package handler
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -113,3 +114,30 @@ func (uts *UserTestSuite) TestGetRandomSecret() {
 func TestUserTestSuite(t *testing.T) {
 	suite.Run(t, &UserTestSuite{})
 }
+
+func Test_validateUserProfile(t *testing.T) {
+	tooLongUsername := "mike012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789mike012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789mike012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+	type args struct {
+		user *commonmodels.User
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr assert.ErrorAssertionFunc
+	}{
+		{"normal_test", args{&commonmodels.User{Username: "mike", Realname: "mike", Email: "[email protected]"}}, assert.NoError},
+		{"illegall_username_,", args{&commonmodels.User{Username: "mike,mike", Realname: "mike", Email: "[email protected]"}}, assert.Error},
+		{"illegall_username_$", args{&commonmodels.User{Username: "mike$mike", Realname: "mike", Email: "[email protected]"}}, assert.Error},
+		{"illegall_username_%", args{&commonmodels.User{Username: "mike%mike", Realname: "mike", Email: "[email protected]"}}, assert.Error},
+		{"illegall_username_#", args{&commonmodels.User{Username: "mike#mike", Realname: "mike", Email: "[email protected]"}}, assert.Error},
+		{"illegall_realname", args{&commonmodels.User{Username: "mike", Realname: "mike,mike", Email: "[email protected]"}}, assert.Error},
+		{"username_too_long", args{&commonmodels.User{Username: tooLongUsername, Realname: "mike", Email: "[email protected]"}}, assert.Error},
+		{"invalid_email", args{&commonmodels.User{Username: "mike", Realname: "mike", Email: "mike#example.com"}}, assert.Error},
+		{"invalid_comment", args{&commonmodels.User{Username: "mike", Realname: "mike", Email: "[email protected]", Comment: tooLongUsername}}, assert.Error},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.wantErr(t, validateUserProfile(tt.args.user), fmt.Sprintf("validateUserProfile(%v)", tt.args.user))
+		})
+	}
+}