From b337f51e7e414f70cd7c9159c2b6aa1fbaa4fa92 Mon Sep 17 00:00:00 2001 From: "stonezdj(Daojun Zhang)" Date: Thu, 2 Nov 2023 11:50:50 +0800 Subject: [PATCH] Replace comma in username to avoid casbin issue (#19505) Check username when creating user by API Replace comma with underscore in username for OnboardUser Fixes #19356 Signed-off-by: stonezdj --- src/common/const.go | 2 ++ src/common/utils/utils.go | 10 ---------- src/core/controllers/oidc.go | 4 ++-- src/pkg/user/manager.go | 4 ++++ src/pkg/user/manager_test.go | 24 ++++++++++++++++++++++++ src/server/v2.0/handler/user.go | 10 +++++++++- src/server/v2.0/handler/user_test.go | 28 ++++++++++++++++++++++++++++ 7 files changed, 69 insertions(+), 13 deletions(-) diff --git a/src/common/const.go b/src/common/const.go index d94edf1af..2daf68299 100644 --- 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 index 22875014c..ab07c3ed3 100644 --- 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 index 3d95ca560..998786784 100644 --- 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 index 641ab0118..f6420f4fa 100644 --- 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 index a125e582a..32714be5f 100644 --- 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: "test@example.com", + 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: "test@example.com", + 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 index e87f6f4c8..9523960b5 100644 --- 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 index 65cfc2475..8afe465cc 100644 --- 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: "mike@example.com"}}, assert.NoError}, + {"illegall_username_,", args{&commonmodels.User{Username: "mike,mike", Realname: "mike", Email: "mike@example.com"}}, assert.Error}, + {"illegall_username_$", args{&commonmodels.User{Username: "mike$mike", Realname: "mike", Email: "mike@example.com"}}, assert.Error}, + {"illegall_username_%", args{&commonmodels.User{Username: "mike%mike", Realname: "mike", Email: "mike@example.com"}}, assert.Error}, + {"illegall_username_#", args{&commonmodels.User{Username: "mike#mike", Realname: "mike", Email: "mike@example.com"}}, assert.Error}, + {"illegall_realname", args{&commonmodels.User{Username: "mike", Realname: "mike,mike", Email: "mike@example.com"}}, assert.Error}, + {"username_too_long", args{&commonmodels.User{Username: tooLongUsername, Realname: "mike", Email: "mike@example.com"}}, 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: "mike@example.com", 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)) + }) + } +}