mirror of
https://github.com/mattermost/mattermost.git
synced 2026-05-12 20:00:48 +00:00
Fix flaky TestCreatePost upload_file subtests (#36453)
* Fix flaky TestCreatePost upload_file subtests The tests removed upload_file from the global channel_user role, which races with other parallel api4 tests, and (for channel-scoped schemes) could not actually revoke upload_file because non-moderated permissions merge from the higher-scoped team channel role. Use an isolated team with its own team scheme, revoke upload_file on that scheme's channel user role, re-login after subtests that clear the client session, and align UpdatePost file-permission cases. Tests-only change. Verified with ENABLE_FULLY_PARALLEL_TESTS=true go test -run '^TestCreatePost$' -race -count=100 ./channels/api4/... Co-authored-by: Maria A Nunez <maria.nunez@mattermost.com> * Add no-scheme TestCreatePost/TestUpdatePost upload_file subtests The team-scheme isolation introduced earlier in this PR fixed the flakiness that came from mutating the shared channel_user role from parallel api4 tests, but it changed the scope under test from the global default scheme to a team scheme. The original test from PR #34538 was validating the no-scheme path, where the user's effective channel role is the built-in channel_user. Restore that coverage by adding two new subtests that isolate at the channel-member level instead of at the role level. The new helper: * creates a fresh channel inside th.BasicTeam (no team scheme, no channel scheme); * creates a unique non-scheme-managed role with channel_user's default permissions minus upload_file; * sets SchemeUser/SchemeAdmin/SchemeGuest=false and ExplicitRoles to the custom role on the user's membership via the store, so the built-in channel_user role is not merged into the effective role set; * invalidates GetAllChannelMembersForUser cache so SessionHasPermissionToChannel sees the new roles. upload_file is PermissionScopeChannel and is not granted by team_user, team_admin, system_user, etc. (only by channel_user/channel_guest), so removing it from the channel-member role is sufficient to deny the API request without touching any process-shared state. The new subtests are therefore safe under ENABLE_FULLY_PARALLEL_TESTS=true. This keeps the team-scheme deny subtests added earlier in the PR as extra coverage for the team-scheme path. Co-authored-by: Maria A Nunez <maria.nunez@mattermost.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
This commit is contained in:
Maria A Nunez
GitHub
@@ -42,6 +42,87 @@ func enableBurnOnReadFeature(th *TestHelper) {
|
||||
})
|
||||
}
|
||||
|
||||
// isolatedTeamChannelWithTeamScheme creates a fresh team (with its own team scheme) and a channel on that team.
|
||||
// Permission edits target scheme-specific role names so parallel api4 tests do not mutate built-in channel_user,
|
||||
// and upload_file is revoked on the team scheme's channel user role so MergeChannelHigherScopedPermissions does not
|
||||
// reintroduce it from the higher scope (model.Role.MergeChannelHigherScopedPermissions).
|
||||
func isolatedTeamChannelWithTeamScheme(t *testing.T, th *TestHelper) (*model.Channel, *model.Scheme) {
|
||||
t.Helper()
|
||||
require.NoError(t, th.App.SetPhase2PermissionsMigrationStatus(true))
|
||||
|
||||
team := th.CreateTeamWithClient(t, th.SystemAdminClient)
|
||||
th.LinkUserToTeam(t, th.BasicUser, team)
|
||||
|
||||
scheme := th.SetupTeamScheme(t)
|
||||
team.SchemeId = &scheme.Id
|
||||
_, appErr := th.App.UpdateTeamScheme(team)
|
||||
require.Nil(t, appErr)
|
||||
|
||||
ch := th.CreateChannelWithClientAndTeam(t, th.Client, model.ChannelTypeOpen, team.Id)
|
||||
return ch, scheme
|
||||
}
|
||||
|
||||
// isolatedNoSchemeChannelWithoutUploadFile creates a fresh channel inside th.BasicTeam (which has no team scheme)
|
||||
// and gives th.BasicUser an isolated channel-member role on it that does not grant upload_file. This exercises the
|
||||
// no-scheme permission-resolution path (i.e. the configuration the original test in PR #34538 was validating)
|
||||
// without mutating the process-shared channel_user role, so the subtest is safe under ENABLE_FULLY_PARALLEL_TESTS.
|
||||
//
|
||||
// The returned channel has neither a team scheme nor a channel scheme. The user's channel membership is configured
|
||||
// with SchemeUser/SchemeAdmin/SchemeGuest all false and ExplicitRoles set to a unique custom role that mirrors
|
||||
// channel_user's default permissions minus upload_file. Because no scheme role is injected into the effective
|
||||
// role set for that membership, the only way upload_file could be granted is via the higher-scoped team or system
|
||||
// roles, and upload_file is PermissionScopeChannel — it is not granted by team_user, team_admin, system_user, etc.
|
||||
//
|
||||
// Returns the channel and a cleanup function that removes the test role.
|
||||
func isolatedNoSchemeChannelWithoutUploadFile(t *testing.T, th *TestHelper) (*model.Channel, func()) {
|
||||
t.Helper()
|
||||
|
||||
ch := th.CreateChannelWithClientAndTeam(t, th.SystemAdminClient, model.ChannelTypeOpen, th.BasicTeam.Id)
|
||||
_, appErr := th.App.AddUserToChannel(th.Context, th.BasicUser, ch, false)
|
||||
require.Nil(t, appErr)
|
||||
|
||||
channelUserRole, appErr := th.App.GetRoleByName(th.Context, model.ChannelUserRoleId)
|
||||
require.Nil(t, appErr)
|
||||
perms := make([]string, 0, len(channelUserRole.Permissions))
|
||||
for _, p := range channelUserRole.Permissions {
|
||||
if p == model.PermissionUploadFile.Id {
|
||||
continue
|
||||
}
|
||||
perms = append(perms, p)
|
||||
}
|
||||
|
||||
roleName := "test_" + model.NewId()
|
||||
role, appErr := th.App.CreateRole(&model.Role{
|
||||
Name: roleName,
|
||||
DisplayName: roleName,
|
||||
Description: "isolated no-scheme channel-member role for TestCreatePost/TestUpdatePost",
|
||||
Permissions: perms,
|
||||
})
|
||||
require.Nil(t, appErr)
|
||||
|
||||
// Replace the membership's role flags via the store so the built-in channel_user role is not injected.
|
||||
// App.UpdateChannelMemberRoles refuses to leave a member with SchemeUser=false; the store accepts it.
|
||||
member, appErr := th.App.GetChannelMember(th.Context, ch.Id, th.BasicUser.Id)
|
||||
require.Nil(t, appErr)
|
||||
member.SchemeUser = false
|
||||
member.SchemeAdmin = false
|
||||
member.SchemeGuest = false
|
||||
member.ExplicitRoles = roleName
|
||||
_, sErr := th.App.Srv().Store().Channel().UpdateMember(th.Context, member)
|
||||
require.NoError(t, sErr)
|
||||
|
||||
// SessionHasPermissionToChannel reads through the cached GetAllChannelMembersForUser lookup.
|
||||
// Store().Channel().UpdateMember does not invalidate that cache, so do it explicitly.
|
||||
th.App.Srv().Platform().InvalidateChannelCacheForUser(th.BasicUser.Id)
|
||||
|
||||
cleanup := func() {
|
||||
if _, err := th.App.DeleteRole(role.Id); err != nil {
|
||||
t.Logf("failed to delete test role %s: %v", roleName, err)
|
||||
}
|
||||
}
|
||||
return ch, cleanup
|
||||
}
|
||||
|
||||
func TestCreatePost(t *testing.T) {
|
||||
mainHelper.Parallel(t)
|
||||
|
||||
@@ -287,19 +368,46 @@ func TestCreatePost(t *testing.T) {
|
||||
assert.Nil(t, rpost)
|
||||
})
|
||||
|
||||
t.Run("should prevent creating post with files when user lacks upload_file permission in target channel", func(t *testing.T) {
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), th.BasicChannel.Id, "test-file.txt")
|
||||
t.Run("should prevent creating post with files when user lacks upload_file permission in target channel (team scheme)", func(t *testing.T) {
|
||||
th.LoginBasic(t)
|
||||
|
||||
ch, scheme := isolatedTeamChannelWithTeamScheme(t, th)
|
||||
require.NotEmpty(t, scheme.DefaultChannelUserRole)
|
||||
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), ch.Id, "test-file.txt")
|
||||
require.NoError(t, err)
|
||||
CheckCreatedStatus(t, resp)
|
||||
fileId := fileResp.FileInfos[0].Id
|
||||
|
||||
th.RemovePermissionFromRole(t, model.PermissionUploadFile.Id, model.ChannelUserRoleId)
|
||||
th.RemovePermissionFromRole(t, model.PermissionUploadFile.Id, scheme.DefaultChannelUserRole)
|
||||
defer func() {
|
||||
th.AddPermissionToRole(t, model.PermissionUploadFile.Id, model.ChannelUserRoleId)
|
||||
th.AddPermissionToRole(t, model.PermissionUploadFile.Id, scheme.DefaultChannelUserRole)
|
||||
}()
|
||||
|
||||
post := &model.Post{
|
||||
ChannelId: th.BasicChannel.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Test post with file",
|
||||
FileIds: model.StringArray{fileId},
|
||||
}
|
||||
rpost, resp, err := client.CreatePost(context.Background(), post)
|
||||
require.Error(t, err)
|
||||
CheckForbiddenStatus(t, resp)
|
||||
assert.Nil(t, rpost)
|
||||
})
|
||||
|
||||
t.Run("should prevent creating post with files when user lacks upload_file permission in target channel (no scheme)", func(t *testing.T) {
|
||||
th.LoginBasic(t)
|
||||
|
||||
ch, cleanup := isolatedNoSchemeChannelWithoutUploadFile(t, th)
|
||||
defer cleanup()
|
||||
|
||||
fileResp, resp, err := th.SystemAdminClient.UploadFile(context.Background(), []byte("test file data"), ch.Id, "test-file.txt")
|
||||
require.NoError(t, err)
|
||||
CheckCreatedStatus(t, resp)
|
||||
fileId := fileResp.FileInfos[0].Id
|
||||
|
||||
post := &model.Post{
|
||||
ChannelId: ch.Id,
|
||||
Message: "Test post with file",
|
||||
FileIds: model.StringArray{fileId},
|
||||
}
|
||||
@@ -310,13 +418,17 @@ func TestCreatePost(t *testing.T) {
|
||||
})
|
||||
|
||||
t.Run("should allow creating post with files when user has upload_file permission", func(t *testing.T) {
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), th.BasicChannel.Id, "test-file.txt")
|
||||
th.LoginBasic(t)
|
||||
|
||||
ch, _ := isolatedTeamChannelWithTeamScheme(t, th)
|
||||
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), ch.Id, "test-file.txt")
|
||||
require.NoError(t, err)
|
||||
CheckCreatedStatus(t, resp)
|
||||
fileId := fileResp.FileInfos[0].Id
|
||||
|
||||
post := &model.Post{
|
||||
ChannelId: th.BasicChannel.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Test post with file",
|
||||
FileIds: model.StringArray{fileId},
|
||||
}
|
||||
@@ -1734,27 +1846,58 @@ func TestUpdatePost(t *testing.T) {
|
||||
CheckBadRequestStatus(t, resp)
|
||||
})
|
||||
|
||||
t.Run("should prevent updating post with files when user lacks upload_file permission in target channel", func(t *testing.T) {
|
||||
t.Run("should prevent updating post with files when user lacks upload_file permission in target channel (team scheme)", func(t *testing.T) {
|
||||
ch, scheme := isolatedTeamChannelWithTeamScheme(t, th)
|
||||
require.NotEmpty(t, scheme.DefaultChannelUserRole)
|
||||
|
||||
postWithoutFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{
|
||||
UserId: th.BasicUser.Id,
|
||||
ChannelId: channel.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Post without files",
|
||||
}, channel, model.CreatePostFlags{SetOnline: true})
|
||||
}, ch, model.CreatePostFlags{SetOnline: true})
|
||||
require.Nil(t, appErr)
|
||||
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), channel.Id, "test-file.txt")
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), ch.Id, "test-file.txt")
|
||||
require.NoError(t, err)
|
||||
CheckCreatedStatus(t, resp)
|
||||
fileId := fileResp.FileInfos[0].Id
|
||||
|
||||
th.RemovePermissionFromRole(t, model.PermissionUploadFile.Id, model.ChannelUserRoleId)
|
||||
th.RemovePermissionFromRole(t, model.PermissionUploadFile.Id, scheme.DefaultChannelUserRole)
|
||||
defer func() {
|
||||
th.AddPermissionToRole(t, model.PermissionUploadFile.Id, model.ChannelUserRoleId)
|
||||
th.AddPermissionToRole(t, model.PermissionUploadFile.Id, scheme.DefaultChannelUserRole)
|
||||
}()
|
||||
|
||||
updatePost := &model.Post{
|
||||
Id: postWithoutFiles.Id,
|
||||
ChannelId: channel.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Updated post with file",
|
||||
FileIds: model.StringArray{fileId},
|
||||
}
|
||||
updatedPost, resp, err := client.UpdatePost(context.Background(), postWithoutFiles.Id, updatePost)
|
||||
require.Error(t, err)
|
||||
CheckForbiddenStatus(t, resp)
|
||||
assert.Nil(t, updatedPost)
|
||||
})
|
||||
|
||||
t.Run("should prevent updating post with files when user lacks upload_file permission in target channel (no scheme)", func(t *testing.T) {
|
||||
ch, cleanup := isolatedNoSchemeChannelWithoutUploadFile(t, th)
|
||||
defer cleanup()
|
||||
|
||||
postWithoutFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{
|
||||
UserId: th.BasicUser.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Post without files",
|
||||
}, ch, model.CreatePostFlags{SetOnline: true})
|
||||
require.Nil(t, appErr)
|
||||
|
||||
fileResp, resp, err := th.SystemAdminClient.UploadFile(context.Background(), []byte("test file data"), ch.Id, "test-file.txt")
|
||||
require.NoError(t, err)
|
||||
CheckCreatedStatus(t, resp)
|
||||
fileId := fileResp.FileInfos[0].Id
|
||||
|
||||
updatePost := &model.Post{
|
||||
Id: postWithoutFiles.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Updated post with file",
|
||||
FileIds: model.StringArray{fileId},
|
||||
}
|
||||
@@ -1765,21 +1908,23 @@ func TestUpdatePost(t *testing.T) {
|
||||
})
|
||||
|
||||
t.Run("should allow updating post with files when user has upload_file permission", func(t *testing.T) {
|
||||
ch, _ := isolatedTeamChannelWithTeamScheme(t, th)
|
||||
|
||||
postWithoutFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{
|
||||
UserId: th.BasicUser.Id,
|
||||
ChannelId: channel.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Post without files",
|
||||
}, channel, model.CreatePostFlags{SetOnline: true})
|
||||
}, ch, model.CreatePostFlags{SetOnline: true})
|
||||
require.Nil(t, appErr)
|
||||
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), channel.Id, "test-file.txt")
|
||||
fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), ch.Id, "test-file.txt")
|
||||
require.NoError(t, err)
|
||||
CheckCreatedStatus(t, resp)
|
||||
fileId := fileResp.FileInfos[0].Id
|
||||
|
||||
updatePost := &model.Post{
|
||||
Id: postWithoutFiles.Id,
|
||||
ChannelId: channel.Id,
|
||||
ChannelId: ch.Id,
|
||||
Message: "Updated post with file",
|
||||
FileIds: model.StringArray{fileId},
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user