Separate create & update user in mysql (#18785) (#18786)

Signed-off-by: kejiang <ke.jiang@zilliz.com>

Signed-off-by: kejiang <ke.jiang@zilliz.com>
Co-authored-by: kejiang <ke.jiang@zilliz.com>

internal/metastore/catalog.go

@@ -33,6 +33,7 @@ type RootCoordCatalog interface {
 
 	GetCredential(ctx context.Context, username string) (*model.Credential, error)
 	CreateCredential(ctx context.Context, credential *model.Credential) error
+	AlterCredential(ctx context.Context, credential *model.Credential) error
 	DropCredential(ctx context.Context, username string) error
 	ListCredentials(ctx context.Context) ([]string, error)
 

internal/metastore/db/dao/user.go

@@ -63,3 +63,13 @@ func (s *userDb) MarkDeletedByUsername(tenantID string, username string) error {
 
 	return nil
 }
+
+func (s *userDb) UpdatePassword(tenantID string, username string, encryptedPassword string) error {
+	err := s.db.Model(&dbmodel.User{}).Where("tenant_id = ? AND username = ?", tenantID, username).Update("encrypted_password", encryptedPassword).Error
+	if err != nil {
+		log.Error("update password by username failed", zap.String("tenant", tenantID), zap.String("username", username), zap.Error(err))
+		return err
+	}
+
+	return nil
+}

internal/metastore/db/dao/user_test.go

@@ -177,3 +177,35 @@ func TestUser_MarkDeletedByUsername_Error(t *testing.T) {
 	err := userTestDb.MarkDeletedByUsername(tenantID, username)
 	assert.Error(t, err)
 }
+
+func TestUser_UpdatePassword(t *testing.T) {
+	username := "test_username_1"
+	encryptedPassword := "test_encrypted_password_1"
+
+	// expectation
+	mock.ExpectBegin()
+	mock.ExpectExec("UPDATE `credential_users` SET `encrypted_password`=?,`updated_at`=? WHERE tenant_id = ? AND username = ?").
+		WithArgs(encryptedPassword, AnyTime{}, tenantID, username).
+		WillReturnResult(sqlmock.NewResult(1, 1))
+	mock.ExpectCommit()
+
+	// actual
+	err := userTestDb.UpdatePassword(tenantID, username, encryptedPassword)
+	assert.Nil(t, err)
+}
+
+func TestUser_UpdatePassword_Error(t *testing.T) {
+	username := "test_username_1"
+	encryptedPassword := "test_encrypted_password_1"
+
+	// expectation
+	mock.ExpectBegin()
+	mock.ExpectExec("UPDATE `credential_users` SET `encrypted_password`=?,`updated_at`=? WHERE tenant_id = ? AND username = ?").
+		WithArgs(encryptedPassword, AnyTime{}, tenantID, username).
+		WillReturnError(errors.New("test error"))
+	mock.ExpectRollback()
+
+	// actual
+	err := userTestDb.UpdatePassword(tenantID, username, encryptedPassword)
+	assert.Error(t, err)
+}

internal/metastore/db/dbmodel/mocks/IUserDb.go

@@ -86,6 +86,20 @@ func (_m *IUserDb) MarkDeletedByUsername(tenantID string, username string) error
 	return r0
 }
 
+// UpdatePassword provides a mock function with given fields: tenantID, username, encryptedPassword
+func (_m *IUserDb) UpdatePassword(tenantID string, username string, encryptedPassword string) error {
+	ret := _m.Called(tenantID, username, encryptedPassword)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string, string, string) error); ok {
+		r0 = rf(tenantID, username, encryptedPassword)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
 type mockConstructorTestingTNewIUserDb interface {
 	mock.TestingT
 	Cleanup(func())

internal/metastore/db/dbmodel/user.go

@@ -27,6 +27,7 @@ type IUserDb interface {
 	ListUser(tenantID string) ([]*User, error)
 	Insert(in *User) error
 	MarkDeletedByUsername(tenantID string, username string) error
+	UpdatePassword(tenantID string, username string, encryptedPassword string) error
 }
 
 // model <---> db

internal/metastore/db/rootcoord/table_catalog.go

@@ -753,6 +753,17 @@ func (tc *Catalog) CreateCredential(ctx context.Context, credential *model.Crede
 	return nil
 }
 
+func (tc *Catalog) AlterCredential(ctx context.Context, credential *model.Credential) error {
+	tenantID := contextutil.TenantID(ctx)
+
+	err := tc.metaDomain.UserDb(ctx).UpdatePassword(tenantID, credential.Username, credential.EncryptedPassword)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (tc *Catalog) DropCredential(ctx context.Context, username string) error {
 	tenantID := contextutil.TenantID(ctx)
 

internal/metastore/db/rootcoord/table_catalog_test.go

@@ -1340,6 +1340,35 @@ func TestTableCatalog_CreateCredential_InsertUserError(t *testing.T) {
 	require.Error(t, gotErr)
 }
 
+func TestTableCatalog_AlterCredential(t *testing.T) {
+	in := &model.Credential{
+		Username:          username,
+		EncryptedPassword: password,
+	}
+
+	// expectation
+	userDbMock.On("UpdatePassword", tenantID, username, password).Return(nil).Once()
+
+	// actual
+	gotErr := mockCatalog.AlterCredential(ctx, in)
+	require.NoError(t, gotErr)
+}
+
+func TestTableCatalog_AlterCredential_Error(t *testing.T) {
+	in := &model.Credential{
+		Username:          username,
+		EncryptedPassword: password,
+	}
+
+	// expectation
+	errTest := errors.New("test error")
+	userDbMock.On("UpdatePassword", tenantID, username, password).Return(errTest).Once()
+
+	// actual
+	gotErr := mockCatalog.AlterCredential(ctx, in)
+	require.Error(t, gotErr)
+}
+
 func TestTableCatalog_DropCredential(t *testing.T) {
 	// expectation
 	userDbMock.On("MarkDeletedByUsername", tenantID, username).Return(nil).Once()

internal/metastore/kv/rootcoord/kv_catalog.go

@@ -344,6 +344,10 @@ func (kc *Catalog) CreateCredential(ctx context.Context, credential *model.Crede
 	return nil
 }
 
+func (kc *Catalog) AlterCredential(ctx context.Context, credential *model.Credential) error {
+	return kc.CreateCredential(ctx, credential)
+}
+
 func (kc *Catalog) listPartitionsAfter210(ctx context.Context, collectionID typeutil.UniqueID, ts typeutil.Timestamp) ([]*model.Partition, error) {
 	prefix := buildPartitionPrefix(collectionID)
 	_, values, err := kc.Snapshot.LoadWithPrefix(prefix, ts)

internal/rootcoord/meta_table.go

@@ -1315,20 +1315,21 @@ func (mt *MetaTable) AddCredential(credInfo *internalpb.CredentialInfo) error {
 	return mt.catalog.CreateCredential(mt.ctx, credential)
 }
 
-// UpdateCredential update credential
-func (mt *MetaTable) UpdateCredential(credInfo *internalpb.CredentialInfo) error {
-	mt.permissionLock.Lock()
-	defer mt.permissionLock.Unlock()
+// AlterCredential update credential
+func (mt *MetaTable) AlterCredential(credInfo *internalpb.CredentialInfo) error {
+	if credInfo.Username == "" {
+		return fmt.Errorf("username is empty")
+	}
 
 	credential := &model.Credential{
 		Username:          credInfo.Username,
 		EncryptedPassword: credInfo.EncryptedPassword,
 	}
-	return mt.catalog.CreateCredential(mt.ctx, credential)
+	return mt.catalog.AlterCredential(mt.ctx, credential)
 }
 
 // GetCredential get credential by username
-func (mt *MetaTable) getCredential(username string) (*internalpb.CredentialInfo, error) {
+func (mt *MetaTable) GetCredential(username string) (*internalpb.CredentialInfo, error) {
 	credential, err := mt.catalog.GetCredential(mt.ctx, username)
 	return model.MarshalCredentialModel(credential), err
 }

internal/rootcoord/meta_table_test.go

@@ -1067,7 +1067,17 @@ func TestMetaTable(t *testing.T) {
 			return fmt.Errorf("save error")
 		}
 		err = mt.AddCredential(&internalpb.CredentialInfo{Username: "x", EncryptedPassword: "a\xc5z"})
-		assert.NotNil(t, err)
+		assert.Error(t, err)
+	})
+
+	wg.Add(1)
+	t.Run("alter credential failed", func(t *testing.T) {
+		defer wg.Done()
+		mockTxnKV.save = func(key, value string) error {
+			return fmt.Errorf("save error")
+		}
+		err = mt.AlterCredential(&internalpb.CredentialInfo{Username: "", EncryptedPassword: "az"})
+		assert.Error(t, err)
 	})
 
 	wg.Add(1)

internal/rootcoord/root_coord.go

@@ -1280,7 +1280,7 @@ func (c *Core) Init() error {
 }
 
 func (c *Core) initData() error {
-	credInfo, _ := c.MetaTable.getCredential(util.UserRoot)
+	credInfo, _ := c.MetaTable.GetCredential(util.UserRoot)
 	if credInfo == nil {
 		log.Debug("RootCoord init user root")
 		encryptedRootPassword, _ := crypto.PasswordEncrypt(util.DefaultRootPassword)
@@ -2848,7 +2848,7 @@ func (c *Core) GetCredential(ctx context.Context, in *rootcoordpb.GetCredentialR
 	log.Debug("GetCredential", zap.String("role", typeutil.RootCoordRole),
 		zap.String("username", in.Username))
 
-	credInfo, err := c.MetaTable.getCredential(in.Username)
+	credInfo, err := c.MetaTable.GetCredential(in.Username)
 	if err != nil {
 		log.Error("GetCredential query credential failed", zap.String("role", typeutil.RootCoordRole),
 			zap.String("username", in.Username), zap.Error(err))
@@ -2877,7 +2877,7 @@ func (c *Core) UpdateCredential(ctx context.Context, credInfo *internalpb.Creden
 	log.Debug("UpdateCredential", zap.String("role", typeutil.RootCoordRole),
 		zap.String("username", credInfo.Username))
 	// update data on storage
-	err := c.MetaTable.UpdateCredential(credInfo)
+	err := c.MetaTable.AlterCredential(credInfo)
 	if err != nil {
 		log.Error("UpdateCredential save credential failed", zap.String("role", typeutil.RootCoordRole),
 			zap.String("username", credInfo.Username), zap.Error(err))