IntenseWebs/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2026-08-26 21:27:40 -05:00
Code Issues Packages Projects Releases Wiki Activity

Strip remote_id field from user patch API requests (#36008)

Browse Source 
* Reapply "Strip remote_id field from user patch API requests (#35910)" (#35996)

This reverts commit d1ca297721.

* Fix SetUserRemoteID to use test's own database in parallel mode

Replace testlib.SetUserRemoteID (which used mainHelper's shared
database) with a squirrel query against GetInternalMasterDB(), which
resolves to the correct per-test pooled database under parallel
execution.
This commit is contained in:
JG Heithcock
2026-04-10 09:40:08 -07:00
committed by GitHub
parent 008373ad50
commit f83d32e42c
11 changed files with 155 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/channels/api4/apitestlib.go
+20
View File
@@ -21,6 +21,7 @@ import (
"time"
"github.com/gorilla/websocket"
sq "github.com/mattermost/squirrel"
s3 "github.com/minio/minio-go/v7"
"github.com/minio/minio-go/v7/pkg/credentials"
"github.com/stretchr/testify/require"
@@ -1409,6 +1410,25 @@ func (th *TestHelper) SetupScheme(tb testing.TB, scope string) *model.Scheme {
return scheme
}
func (th *TestHelper) SetUserRemoteID(tb testing.TB, userID, remoteID string) *model.User {
tb.Helper()
query, args, err := sq.StatementBuilder.PlaceholderFormat(sq.Dollar).
Update("Users").
Set("RemoteId", remoteID).
Where(sq.Eq{"Id": userID}).
ToSql()
require.NoError(tb, err)
_, err = th.App.Srv().Store().GetInternalMasterDB().Exec(query, args...)
require.NoError(tb, err)
th.App.InvalidateCacheForUser(userID)
user, appErr := th.App.GetUser(userID)
require.Nil(tb, appErr)
return user
}
func (th *TestHelper) Parallel(t *testing.T) {
mainHelper.Parallel(t)
}
server/channels/api4/shared_channel_test.go
+4 -14
View File
@@ -221,10 +221,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) {
}()
localUser := th.BasicUser
remoteUser := th.CreateUser(t)
remoteUser.RemoteId = model.NewPointer(model.NewId())
remoteUser, appErr := th.App.UpdateUser(th.Context, remoteUser, false)
require.Nil(t, appErr)
remoteUser := th.SetUserRemoteID(t, th.CreateUser(t).Id, model.NewId())
dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id)
require.Error(t, err)
@@ -243,10 +240,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) {
}()
localUser := th.BasicUser
remoteUser := th.CreateUser(t)
remoteUser.RemoteId = model.NewPointer(model.NewId())
remoteUser, appErr := th.App.UpdateUser(th.Context, remoteUser, false)
require.Nil(t, appErr)
remoteUser := th.SetUserRemoteID(t, th.CreateUser(t).Id, model.NewId())
dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id)
require.NoError(t, err)
@@ -278,9 +272,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) {
rc, appErr := th.App.AddRemoteCluster(rc)
require.Nil(t, appErr)
remoteUser.RemoteId = model.NewPointer(rc.RemoteId)
remoteUser, appErr = th.App.UpdateUser(th.Context, remoteUser, false)
require.Nil(t, appErr)
th.SetUserRemoteID(t, remoteUser.Id, rc.RemoteId)
dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id)
require.NoError(t, err)
@@ -312,9 +304,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) {
rc, appErr := th.App.AddRemoteCluster(rc)
require.Nil(t, appErr)
remoteUser.RemoteId = model.NewPointer(rc.RemoteId)
remoteUser, appErr = th.App.UpdateUser(th.Context, remoteUser, false)
require.Nil(t, appErr)
th.SetUserRemoteID(t, remoteUser.Id, rc.RemoteId)
dm, _, err := client.CreateDirectChannel(context.Background(), remoteUser.Id, localUser.Id)
require.NoError(t, err)
server/channels/api4/user.go
+2
View File
@@ -1450,6 +1450,8 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) {
return
}
patch.RemoteId = nil
auditRec := c.MakeAuditRecord(model.AuditEventPatchUser, model.AuditStatusFail)
model.AddEventParameterAuditableToAuditRec(auditRec, "user_patch", &patch)
defer c.LogAuditRec(auditRec)
server/channels/api4/user_test.go
+98
View File
@@ -2316,6 +2316,50 @@ func TestUpdateUser(t *testing.T) {
})
}
func TestUpdateUserRemoteIdIgnored(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t)
t.Run("remote_id in update body is ignored for regular user", func(t *testing.T) {
user := th.CreateUser(t)
_, _, err := th.Client.Login(context.Background(), user.Email, user.Password)
require.NoError(t, err)
user.RemoteId = model.NewPointer("attacker-remote-id")
user.Nickname = "updated-nickname"
ruser, _, err := th.Client.UpdateUser(context.Background(), user)
require.NoError(t, err)
require.Equal(t, "updated-nickname", ruser.Nickname)
require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty")
dbUser, appErr := th.App.GetUser(user.Id)
require.Nil(t, appErr)
require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted")
})
t.Run("remote_id in update body is ignored for system admin", func(t *testing.T) {
user := th.CreateUser(t)
user.RemoteId = model.NewPointer("admin-remote-id")
user.Nickname = "admin-updated"
ruser, _, err := th.SystemAdminClient.UpdateUser(context.Background(), user)
require.NoError(t, err)
require.Equal(t, "admin-updated", ruser.Nickname)
require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty even for admin")
})
t.Run("existing remote_id is preserved when updating other fields", func(t *testing.T) {
remoteId := model.NewId()
user := th.SetUserRemoteID(t, th.CreateUser(t).Id, remoteId)
user.Nickname = "updated-nickname"
ruser, _, err := th.SystemAdminClient.UpdateUser(context.Background(), user)
require.NoError(t, err)
require.Equal(t, "updated-nickname", ruser.Nickname)
require.Equal(t, remoteId, model.SafeDereference(ruser.RemoteId), "existing remote_id should be preserved")
})
}
func TestUpdateAdminUser(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
@@ -2474,6 +2518,60 @@ func TestPatchUser(t *testing.T) {
require.NoError(t, err)
}
func TestPatchUserRemoteIdIgnored(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
t.Run("remote_id in patch is ignored for regular user", func(t *testing.T) {
user := th.CreateUser(t)
_, _, err := th.Client.Login(context.Background(), user.Email, user.Password)
require.NoError(t, err)
patch := &model.UserPatch{
RemoteId: model.NewPointer("attacker-remote-id"),
Nickname: model.NewPointer("new-nickname"),
}
ruser, _, err := th.Client.PatchUser(context.Background(), user.Id, patch)
require.NoError(t, err)
require.Equal(t, "new-nickname", ruser.Nickname)
require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty")
dbUser, appErr := th.App.GetUser(user.Id)
require.Nil(t, appErr)
require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted")
})
t.Run("remote_id in patch is ignored for system admin", func(t *testing.T) {
user := th.CreateUser(t)
patch := &model.UserPatch{
RemoteId: model.NewPointer("admin-remote-id"),
Nickname: model.NewPointer("admin-patched"),
}
ruser, _, err := th.SystemAdminClient.PatchUser(context.Background(), user.Id, patch)
require.NoError(t, err)
require.Equal(t, "admin-patched", ruser.Nickname)
require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty even for admin")
dbUser, appErr := th.App.GetUser(user.Id)
require.Nil(t, appErr)
require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted even for admin")
})
t.Run("existing remote_id is preserved when patching other fields", func(t *testing.T) {
remoteId := model.NewId()
user := th.SetUserRemoteID(t, th.CreateUser(t).Id, remoteId)
patch := &model.UserPatch{
Nickname: model.NewPointer("updated-nickname"),
}
ruser, _, err := th.SystemAdminClient.PatchUser(context.Background(), user.Id, patch)
require.NoError(t, err)
require.Equal(t, "updated-nickname", ruser.Nickname)
require.Equal(t, remoteId, model.SafeDereference(ruser.RemoteId), "existing remote_id should be preserved")
})
}
func TestPatchBotUser(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
server/channels/app/channel_test.go
+2 -6
View File
@@ -599,9 +599,7 @@ func TestGetOrCreateDirectChannel(t *testing.T) {
})
t.Run("Cannot create with a remote user", func(t *testing.T) {
user2.RemoteId = model.NewPointer(model.NewId())
_, appErr := th.App.UpdateUser(th.Context, user2, false)
require.Nil(t, appErr)
th.SetUserRemoteID(t, user2.Id, model.NewId())
dm, appErr := th.App.GetOrCreateDirectChannel(th.Context, user1.Id, user2.Id)
require.Nil(t, dm)
@@ -622,9 +620,7 @@ func TestCreateGroupChannel(t *testing.T) {
groupUserIds = append(groupUserIds, th.BasicUser.Id)
t.Run("Should not allow to create a group with a remote user", func(t *testing.T) {
user2.RemoteId = model.NewPointer(model.NewId())
_, appErr := th.App.UpdateUser(th.Context, user2, false)
require.Nil(t, appErr)
th.SetUserRemoteID(t, user2.Id, model.NewId())
dm, appErr := th.App.CreateGroupChannel(th.Context, groupUserIds, th.BasicUser.Id)
require.NotNil(t, appErr)
server/channels/app/helper_test.go
+20
View File
@@ -15,6 +15,7 @@ import (
"testing"
"time"
sq "github.com/mattermost/squirrel"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
@@ -814,6 +815,25 @@ func decodeJSON[T any](tb testing.TB, o any, result *T) *T {
return result
}
func (th *TestHelper) SetUserRemoteID(tb testing.TB, userID, remoteID string) *model.User {
tb.Helper()
query, args, err := sq.StatementBuilder.PlaceholderFormat(sq.Dollar).
Update("Users").
Set("RemoteId", remoteID).
Where(sq.Eq{"Id": userID}).
ToSql()
require.NoError(tb, err)
_, err = th.App.Srv().Store().GetInternalMasterDB().Exec(query, args...)
require.NoError(tb, err)
th.App.InvalidateCacheForUser(userID)
user, appErr := th.App.GetUser(userID)
require.Nil(tb, appErr)
return user
}
func (th *TestHelper) Parallel(t *testing.T) {
mainHelper.Parallel(t)
}
server/channels/app/shared_channel_global_user_sync_self_referential_test.go
+1 -1
View File
@@ -227,7 +227,7 @@ func TestSharedChannelGlobalUserSyncSelfReferential(t *testing.T) {
// Create remote user (should NOT be synced)
remoteUser := th.CreateUser(t)
remoteUser.RemoteId = &selfCluster.RemoteId
remoteUser = th.SetUserRemoteID(t, remoteUser.Id, selfCluster.RemoteId)
remoteUser.UpdateAt = baseTime + 600
_, err = ss.User().Update(th.Context, remoteUser, true)
require.NoError(t, err)
server/channels/app/shared_channel_membership_sync_self_referential_test.go
+1 -3
View File
@@ -984,9 +984,7 @@ func TestSharedChannelMembershipSyncSelfReferential(t *testing.T) {
// Create a remote user belonging to cluster-2
userFromCluster2 := th.CreateUser(t)
userFromCluster2.RemoteId = &clusters[1].RemoteId
userFromCluster2, appErr = th.App.UpdateUser(th.Context, userFromCluster2, false)
require.Nil(t, appErr)
userFromCluster2 = th.SetUserRemoteID(t, userFromCluster2.Id, clusters[1].RemoteId)
_, _, appErr = th.App.AddUserToTeam(th.Context, team.Id, userFromCluster2.Id, th.BasicUser.Id)
require.Nil(t, appErr)
server/channels/app/shared_channel_test.go
+5 -4
View File
@@ -598,12 +598,13 @@ func TestTransformMentionsOnReceive(t *testing.T) {
// Helper to create test users
createUser := func(username string, remoteId *string) *model.User {
user := th.CreateUser(t)
user.Username = username
if remoteId != nil {
user.RemoteId = remoteId
user = th.SetUserRemoteID(t, user.Id, *remoteId)
}
user, updateErr := th.App.UpdateUser(th.Context, user, false)
require.Nil(t, updateErr)
user.Username = username
var appErr *model.AppError
user, appErr = th.App.UpdateUser(th.Context, user, false)
require.Nil(t, appErr)
th.LinkUserToTeam(t, user, th.BasicTeam)
th.AddUserToChannel(t, user, sharedChannel)
return user
server/channels/app/user_test.go
+1 -4
View File
@@ -2602,10 +2602,7 @@ func createTestRemoteCluster(t *testing.T, th *TestHelper, ss store.Store, name,
func createRemoteUser(t *testing.T, th *TestHelper, remoteCluster *model.RemoteCluster) *model.User {
user := th.CreateUser(t)
user.RemoteId = &remoteCluster.RemoteId
updatedUser, appErr := th.App.UpdateUser(th.Context, user, false)
require.Nil(t, appErr)
return updatedUser
return th.SetUserRemoteID(t, user.Id, remoteCluster.RemoteId)
}
func ensureRemoteClusterConnected(t *testing.T, ss store.Store, cluster *model.RemoteCluster, connected bool) {
server/channels/store/sqlstore/user_store.go
+1
View File
@@ -287,6 +287,7 @@ func (us SqlUserStore) Update(rctx request.CTX, user *model.User, trustedUpdateD
user.MfaActive = oldUser.MfaActive
user.MfaUsedTimestamps = oldUser.MfaUsedTimestamps
user.LastLogin = oldUser.LastLogin
user.RemoteId = oldUser.RemoteId
if !trustedUpdateData {
user.Roles = oldUser.Roles