[MM-45272] Fix MM-45272 (#24701)

* Fix MM-45272

* Properly handle permalinks

* Fix

* Fix tests

* Handle only not found case for team member

* Fix lint

* Use proper config value

* Separate permission in several statements

* Add tests

* Fix lint

* Revert changes on utils

* Address feedback and more fixes

* Address feedback

* Fix test

* Fix test and related bug

* Fix and reorder test

* Address feedback

* Address feedback

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/api4/post_test.go

@@ -3795,7 +3795,7 @@ func TestPostGetInfo(t *testing.T) {
 	dmPost, _, err := client.CreatePost(context.Background(), &model.Post{ChannelId: dmChannel.Id})
 	require.NoError(t, err)
 
-	openTeam, _, err := sysadminClient.CreateTeam(context.Background(), &model.Team{Type: model.TeamOpen, Name: "open-team", DisplayName: "Open Team"})
+	openTeam, _, err := sysadminClient.CreateTeam(context.Background(), &model.Team{Type: model.TeamOpen, Name: "open-team", DisplayName: "Open Team", AllowOpenInvite: true})
 	require.NoError(t, err)
 	openTeamOpenChannel, _, err := sysadminClient.CreateChannel(context.Background(), &model.Channel{TeamId: openTeam.Id, Type: model.ChannelTypeOpen, Name: "open-team-open-channel", DisplayName: "Open Team - Open Channel"})
 	require.NoError(t, err)
@@ -3803,7 +3803,7 @@ func TestPostGetInfo(t *testing.T) {
 	require.NoError(t, err)
 
 	// Alt team is a team without the sysadmin in it.
-	altOpenTeam, _, err := client.CreateTeam(context.Background(), &model.Team{Type: model.TeamOpen, Name: "alt-open-team", DisplayName: "Alt Open Team"})
+	altOpenTeam, _, err := client.CreateTeam(context.Background(), &model.Team{Type: model.TeamOpen, Name: "alt-open-team", DisplayName: "Alt Open Team", AllowOpenInvite: true})
 	require.NoError(t, err)
 	altOpenTeamOpenChannel, _, err := client.CreateChannel(context.Background(), &model.Channel{TeamId: altOpenTeam.Id, Type: model.ChannelTypeOpen, Name: "alt-open-team-open-channel", DisplayName: "Open Team - Open Channel"})
 	require.NoError(t, err)
@@ -4008,8 +4008,12 @@ func TestPostGetInfo(t *testing.T) {
 			require.Equal(t, tc.channel.DisplayName, info.ChannelDisplayName)
 			require.Equal(t, tc.hasJoinedChannel, info.HasJoinedChannel)
 			if tc.team != nil {
+				teamType := "I"
+				if tc.team.AllowOpenInvite {
+					teamType = "O"
+				}
 				require.Equal(t, tc.team.Id, info.TeamId)
-				require.Equal(t, tc.team.Type, info.TeamType)
+				require.Equal(t, teamType, info.TeamType)
 				require.Equal(t, tc.team.DisplayName, info.TeamDisplayName)
 				require.Equal(t, tc.hasJoinedTeam, info.HasJoinedTeam)
 			}

server/channels/app/authorization.go

@@ -380,5 +380,13 @@ func (a *App) HasPermissionToReadChannel(c request.CTX, userID string, channel *
 	if !*a.Config().TeamSettings.ExperimentalViewArchivedChannels && channel.DeleteAt != 0 {
 		return false
 	}
-	return a.HasPermissionToChannel(c, userID, channel.Id, model.PermissionReadChannelContent) || (channel.Type == model.ChannelTypeOpen && a.HasPermissionToTeam(c, userID, channel.TeamId, model.PermissionReadPublicChannel))
+	if a.HasPermissionToChannel(c, userID, channel.Id, model.PermissionReadChannelContent) {
+		return true
+	}
+
+	if channel.Type == model.ChannelTypeOpen && !*a.Config().ComplianceSettings.Enable {
+		return a.HasPermissionToTeam(c, userID, channel.TeamId, model.PermissionReadPublicChannel)
+	}
+
+	return false
 }

server/channels/app/authorization_test.go

@@ -553,3 +553,117 @@ func TestSessionHasPermissionToGroup(t *testing.T) {
 		}
 	}
 }
+
+func TestHasPermissionToReadChannel(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	ttcc := []struct {
+		name                    string
+		configViewArchived      bool
+		configComplianceEnabled bool
+		channelDeleted          bool
+		canReadChannel          bool
+		channelIsOpen           bool
+		canReadPublicChannel    bool
+		expected                bool
+	}{
+		{
+			name:                    "Cannot read archived channels if the config doesn't allow it",
+			configViewArchived:      false,
+			configComplianceEnabled: true,
+			channelDeleted:          true,
+			canReadChannel:          true,
+			channelIsOpen:           true,
+			canReadPublicChannel:    true,
+			expected:                false,
+		},
+		{
+			name:                    "Can read if it has permissions to read",
+			configViewArchived:      false,
+			configComplianceEnabled: true,
+			channelDeleted:          false,
+			canReadChannel:          true,
+			channelIsOpen:           false,
+			canReadPublicChannel:    true,
+			expected:                true,
+		},
+		{
+			name:                    "Cannot read private channels if it has no permission",
+			configViewArchived:      false,
+			configComplianceEnabled: false,
+			channelDeleted:          false,
+			canReadChannel:          false,
+			channelIsOpen:           false,
+			canReadPublicChannel:    true,
+			expected:                false,
+		},
+		{
+			name:                    "Cannot read open channels if compliance is enabled",
+			configViewArchived:      false,
+			configComplianceEnabled: true,
+			channelDeleted:          false,
+			canReadChannel:          false,
+			channelIsOpen:           true,
+			canReadPublicChannel:    true,
+			expected:                false,
+		},
+		{
+			name:                    "Cannot read open channels if it has no team permissions",
+			configViewArchived:      false,
+			configComplianceEnabled: false,
+			channelDeleted:          false,
+			canReadChannel:          false,
+			channelIsOpen:           true,
+			canReadPublicChannel:    false,
+			expected:                false,
+		},
+		{
+			name:                    "Can read open channels if it has team permissions and compliance is not enabled",
+			configViewArchived:      false,
+			configComplianceEnabled: false,
+			channelDeleted:          false,
+			canReadChannel:          false,
+			channelIsOpen:           true,
+			canReadPublicChannel:    true,
+			expected:                true,
+		},
+	}
+
+	for _, tc := range ttcc {
+		t.Run(tc.name, func(t *testing.T) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
+				configViewArchived := tc.configViewArchived
+				configComplianceEnabled := tc.configComplianceEnabled
+				cfg.TeamSettings.ExperimentalViewArchivedChannels = &configViewArchived
+				cfg.ComplianceSettings.Enable = &configComplianceEnabled
+			})
+
+			team := th.CreateTeam()
+			if tc.canReadPublicChannel {
+				th.LinkUserToTeam(th.BasicUser2, team)
+			}
+
+			var channel *model.Channel
+			if tc.channelIsOpen {
+				channel = th.CreateChannel(th.Context, team)
+			} else {
+				channel = th.CreatePrivateChannel(th.Context, team)
+			}
+			if tc.canReadChannel {
+				_, err := th.App.AddUserToChannel(th.Context, th.BasicUser2, channel, false)
+				require.Nil(t, err)
+			}
+
+			if tc.channelDeleted {
+				err := th.App.DeleteChannel(th.Context, channel, th.SystemAdminUser.Id)
+				require.Nil(t, err)
+				channel, err = th.App.GetChannel(th.Context, channel.Id)
+				require.Nil(t, err)
+			}
+
+			result := th.App.HasPermissionToReadChannel(th.Context, th.BasicUser2.Id, channel)
+			require.Equal(t, tc.expected, result)
+		})
+	}
+}

server/channels/app/post.go

@@ -799,10 +799,13 @@ func (a *App) publishWebsocketEventForPermalinkPost(c request.CTX, post *model.P
 		return false, err
 	}
 
+	originalEmbeds := post.Metadata.Embeds
+	originalProps := post.GetProps()
 	permalinkPreviewedPost := post.GetPreviewPost()
 	for _, userID := range userIDs {
 		if permalinkPreviewedPost != nil {
-			post.Metadata.Embeds[0].Data = permalinkPreviewedPost
+			post.Metadata.Embeds = originalEmbeds
+			post.SetProps(originalProps)
 		}
 
 		postForUser := a.sanitizePostMetadataForUserAndChannel(c, post, permalinkPreviewedPost, permalinkPreviewedChannel, userID)
@@ -822,6 +825,12 @@ func (a *App) publishWebsocketEventForPermalinkPost(c request.CTX, post *model.P
 		a.Publish(messageCopy)
 	}
 
+	// Restore the metadata that may have been removed in the sanitization
+	if permalinkPreviewedPost != nil {
+		post.Metadata.Embeds = originalEmbeds
+		post.SetProps(originalProps)
+	}
+
 	return true, nil
 }
 
@@ -2033,7 +2042,7 @@ func (a *App) GetPostIfAuthorized(c request.CTX, postID string, session *model.S
 	}
 
 	if !a.SessionHasPermissionToChannel(c, *session, channel.Id, model.PermissionReadChannelContent) {
-		if channel.Type == model.ChannelTypeOpen {
+		if channel.Type == model.ChannelTypeOpen && !*a.Config().ComplianceSettings.Enable {
 			if !a.SessionHasPermissionToTeam(*session, channel.TeamId, model.PermissionReadPublicChannel) {
 				return nil, a.MakePermissionError(session, []*model.Permission{model.PermissionReadPublicChannel})
 			}
@@ -2233,10 +2242,23 @@ func (a *App) GetPostInfo(c request.CTX, postID string) (*model.PostInfo, *model
 			return nil, appErr
 		}
 
-		if team.Type == model.TeamOpen {
-			hasPermissionToAccessTeam = a.HasPermissionToTeam(c, userID, team.Id, model.PermissionJoinPublicTeams)
-		} else if team.Type == model.TeamInvite {
-			hasPermissionToAccessTeam = a.HasPermissionToTeam(c, userID, team.Id, model.PermissionJoinPrivateTeams)
+		teamMember, appErr := a.GetTeamMember(c, channel.TeamId, userID)
+		if appErr != nil && appErr.StatusCode != http.StatusNotFound {
+			return nil, appErr
+		}
+
+		if appErr == nil {
+			if teamMember.DeleteAt == 0 {
+				hasPermissionToAccessTeam = true
+			}
+		}
+
+		if !hasPermissionToAccessTeam {
+			if team.AllowOpenInvite {
+				hasPermissionToAccessTeam = a.HasPermissionToTeam(c, userID, team.Id, model.PermissionJoinPublicTeams)
+			} else {
+				hasPermissionToAccessTeam = a.HasPermissionToTeam(c, userID, team.Id, model.PermissionJoinPrivateTeams)
+			}
 		}
 	} else {
 		// This happens in case of DMs and GMs.
@@ -2269,12 +2291,16 @@ func (a *App) GetPostInfo(c request.CTX, postID string) (*model.PostInfo, *model
 		HasJoinedChannel:   channelMemberErr == nil,
 	}
 	if team != nil {
-		_, teamMemberErr := a.GetTeamMember(c, team.Id, userID)
+		teamMember, teamMemberErr := a.GetTeamMember(c, team.Id, userID)
 
+		teamType := model.TeamInvite
+		if team.AllowOpenInvite {
+			teamType = model.TeamOpen
+		}
 		info.TeamId = team.Id
-		info.TeamType = team.Type
+		info.TeamType = teamType
 		info.TeamDisplayName = team.DisplayName
-		info.HasJoinedTeam = teamMemberErr == nil
+		info.HasJoinedTeam = teamMemberErr == nil && teamMember.DeleteAt == 0
 	}
 	return &info, nil
 }

server/channels/app/post_metadata.go

@@ -196,7 +196,19 @@ func (a *App) sanitizePostMetadataForUserAndChannel(c request.CTX, post *model.P
 	}
 
 	if previewedChannel != nil && !a.HasPermissionToReadChannel(c, userID, previewedChannel) {
-		post.Metadata.Embeds[0].Data = nil
+		// Remove all permalink embeds and only keep non-permalink embeds.
+		// We always have only one permalink embed even if the post
+		// contains multiple permalinks.
+		var newEmbeds []*model.PostEmbed
+		for _, embed := range post.Metadata.Embeds {
+			if embed.Type != model.PostEmbedPermalink {
+				newEmbeds = append(newEmbeds, embed)
+			}
+		}
+
+		post.Metadata.Embeds = newEmbeds
+
+		post.DelProp(model.PostPropsPreviewedPost)
 	}
 
 	return post
@@ -229,6 +241,8 @@ func (a *App) SanitizePostMetadataForUser(c request.CTX, post *model.Post, userI
 		}
 
 		post.Metadata.Embeds = newEmbeds
+
+		post.DelProp(model.PostPropsPreviewedPost)
 	}
 
 	return post, nil

server/channels/app/post_metadata_test.go

@@ -2820,7 +2820,7 @@ func TestSanitizePostMetadataForUserAndChannel(t *testing.T) {
 		require.Nil(t, appErr)
 
 		actual = th.App.sanitizePostMetadataForUserAndChannel(th.Context, post, previewedPost, directChannel, guest.Id)
-		assert.Nil(t, actual.Metadata.Embeds[0].Data)
+		assert.Len(t, actual.Metadata.Embeds, 0)
 	})
 
 	t.Run("should not preview for archived channels", func(t *testing.T) {
@@ -2882,7 +2882,7 @@ func TestSanitizePostMetadataForUserAndChannel(t *testing.T) {
 		})
 
 		actual = th.App.sanitizePostMetadataForUserAndChannel(th.Context, post, previewedPost, publicChannel, th.BasicUser.Id)
-		assert.Nil(t, actual.Metadata.Embeds[0].Data)
+		assert.Len(t, actual.Metadata.Embeds, 0)
 	})
 }
 

server/channels/app/post_test.go

@@ -1001,51 +1001,49 @@ func TestCreatePost(t *testing.T) {
 		directChannel, err := th.App.createDirectChannel(th.Context, user1.Id, user2.Id)
 		require.Nil(t, err)
 
-		referencedPost := &model.Post{
-			ChannelId: th.BasicChannel.Id,
-			Message:   "hello world",
-			UserId:    th.BasicUser.Id,
-		}
-
 		th.Context.Session().UserId = th.BasicUser.Id
 
-		referencedPost, err = th.App.CreatePost(th.Context, referencedPost, th.BasicChannel, false, false)
-		require.Nil(t, err)
-
-		permalink := fmt.Sprintf("%s/%s/pl/%s", *th.App.Config().ServiceSettings.SiteURL, th.BasicTeam.Name, referencedPost.Id)
-
 		testCases := []struct {
 			Description string
 			Channel     *model.Channel
 			Author      string
-			Assert      func(t assert.TestingT, object any, msgAndArgs ...any) bool
+			Length      int
 		}{
 			{
 				Description: "removes metadata from post for members who cannot read channel",
 				Channel:     directChannel,
 				Author:      user1.Id,
-				Assert:      assert.Nil,
+				Length:      0,
 			},
 			{
 				Description: "does not remove metadata from post for members who can read channel",
 				Channel:     th.BasicChannel,
 				Author:      th.BasicUser.Id,
-				Assert:      assert.NotNil,
+				Length:      1,
 			},
 		}
 
 		for _, testCase := range testCases {
 			t.Run(testCase.Description, func(t *testing.T) {
-				previewPost := &model.Post{
+				referencedPost := &model.Post{
 					ChannelId: testCase.Channel.Id,
-					Message:   permalink,
+					Message:   "hello world",
 					UserId:    testCase.Author,
 				}
-
-				previewPost, err = th.App.CreatePost(th.Context, previewPost, testCase.Channel, false, false)
+				referencedPost, err = th.App.CreatePost(th.Context, referencedPost, testCase.Channel, false, false)
 				require.Nil(t, err)
 
-				testCase.Assert(t, previewPost.Metadata.Embeds[0].Data)
+				permalink := fmt.Sprintf("%s/%s/pl/%s", *th.App.Config().ServiceSettings.SiteURL, th.BasicTeam.Name, referencedPost.Id)
+				previewPost := &model.Post{
+					ChannelId: th.BasicChannel.Id,
+					Message:   permalink,
+					UserId:    th.BasicUser.Id,
+				}
+
+				previewPost, err = th.App.CreatePost(th.Context, previewPost, th.BasicChannel, false, false)
+				require.Nil(t, err)
+
+				require.Len(t, previewPost.Metadata.Embeds, testCase.Length)
 			})
 		}
 	})
@@ -1451,73 +1449,6 @@ func TestUpdatePost(t *testing.T) {
 		require.Nil(t, err)
 		assert.Equal(t, testPost.GetProps(), model.StringInterface{"previewed_post": referencedPost.Id})
 	})
-
-	t.Run("sanitizes post metadata appropriately", func(t *testing.T) {
-		th := Setup(t).InitBasic()
-		defer th.TearDown()
-
-		th.App.UpdateConfig(func(cfg *model.Config) {
-			*cfg.ServiceSettings.SiteURL = "http://mymattermost.com"
-		})
-
-		th.AddUserToChannel(th.BasicUser, th.BasicChannel)
-
-		user1 := th.CreateUser()
-		user2 := th.CreateUser()
-		directChannel, err := th.App.createDirectChannel(th.Context, user1.Id, user2.Id)
-		require.Nil(t, err)
-
-		referencedPost := &model.Post{
-			ChannelId: th.BasicChannel.Id,
-			Message:   "hello world",
-			UserId:    th.BasicUser.Id,
-		}
-
-		th.Context.Session().UserId = th.BasicUser.Id
-
-		referencedPost, err = th.App.CreatePost(th.Context, referencedPost, th.BasicChannel, false, false)
-		require.Nil(t, err)
-
-		permalink := fmt.Sprintf("%s/%s/pl/%s", *th.App.Config().ServiceSettings.SiteURL, th.BasicTeam.Name, referencedPost.Id)
-
-		testCases := []struct {
-			Description string
-			Channel     *model.Channel
-			Author      string
-			Assert      func(t assert.TestingT, object any, msgAndArgs ...any) bool
-		}{
-			{
-				Description: "removes metadata from post for members who cannot read channel",
-				Channel:     directChannel,
-				Author:      user1.Id,
-				Assert:      assert.Nil,
-			},
-			{
-				Description: "does not remove metadata from post for members who can read channel",
-				Channel:     th.BasicChannel,
-				Author:      th.BasicUser.Id,
-				Assert:      assert.NotNil,
-			},
-		}
-
-		for _, testCase := range testCases {
-			t.Run(testCase.Description, func(t *testing.T) {
-				previewPost := &model.Post{
-					ChannelId: testCase.Channel.Id,
-					UserId:    testCase.Author,
-				}
-
-				previewPost, err = th.App.CreatePost(th.Context, previewPost, testCase.Channel, false, false)
-				require.Nil(t, err)
-
-				previewPost.Message = permalink
-				previewPost, err = th.App.UpdatePost(th.Context, previewPost, false)
-				require.Nil(t, err)
-
-				testCase.Assert(t, previewPost.Metadata.Embeds[0].Data)
-			})
-		}
-	})
 }
 
 func TestSearchPostsForUser(t *testing.T) {
@@ -3043,26 +2974,64 @@ func TestGetPostIfAuthorized(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()
 
-	privateChannel := th.CreatePrivateChannel(th.Context, th.BasicTeam)
-	post, err := th.App.CreatePost(th.Context, &model.Post{UserId: th.BasicUser.Id, ChannelId: privateChannel.Id, Message: "Hello"}, privateChannel, false, false)
-	require.Nil(t, err)
-	require.NotNil(t, post)
+	t.Run("Private channel", func(t *testing.T) {
+		privateChannel := th.CreatePrivateChannel(th.Context, th.BasicTeam)
+		post, err := th.App.CreatePost(th.Context, &model.Post{UserId: th.BasicUser.Id, ChannelId: privateChannel.Id, Message: "Hello"}, privateChannel, false, false)
+		require.Nil(t, err)
+		require.NotNil(t, post)
 
-	session1, err := th.App.CreateSession(th.Context, &model.Session{UserId: th.BasicUser.Id, Props: model.StringMap{}})
-	require.Nil(t, err)
-	require.NotNil(t, session1)
+		session1, err := th.App.CreateSession(th.Context, &model.Session{UserId: th.BasicUser.Id, Props: model.StringMap{}})
+		require.Nil(t, err)
+		require.NotNil(t, session1)
 
-	session2, err := th.App.CreateSession(th.Context, &model.Session{UserId: th.BasicUser2.Id, Props: model.StringMap{}})
-	require.Nil(t, err)
-	require.NotNil(t, session2)
+		session2, err := th.App.CreateSession(th.Context, &model.Session{UserId: th.BasicUser2.Id, Props: model.StringMap{}})
+		require.Nil(t, err)
+		require.NotNil(t, session2)
 
-	// User is not authorized to get post
-	_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session2, false)
-	require.NotNil(t, err)
+		// User is not authorized to get post
+		_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session2, false)
+		require.NotNil(t, err)
 
-	// User is authorized to get post
-	_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session1, false)
-	require.Nil(t, err)
+		// User is authorized to get post
+		_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session1, false)
+		require.Nil(t, err)
+	})
+
+	t.Run("Public channel", func(t *testing.T) {
+		publicChannel := th.CreateChannel(th.Context, th.BasicTeam)
+		post, err := th.App.CreatePost(th.Context, &model.Post{UserId: th.BasicUser.Id, ChannelId: publicChannel.Id, Message: "Hello"}, publicChannel, false, false)
+		require.Nil(t, err)
+		require.NotNil(t, post)
+
+		session1, err := th.App.CreateSession(th.Context, &model.Session{UserId: th.BasicUser.Id, Props: model.StringMap{}})
+		require.Nil(t, err)
+		require.NotNil(t, session1)
+
+		session2, err := th.App.CreateSession(th.Context, &model.Session{UserId: th.BasicUser2.Id, Props: model.StringMap{}})
+		require.Nil(t, err)
+		require.NotNil(t, session2)
+
+		// User is authorized to get post
+		_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session2, false)
+		require.Nil(t, err)
+
+		// User is authorized to get post
+		_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session1, false)
+		require.Nil(t, err)
+
+		th.App.UpdateConfig(func(c *model.Config) {
+			b := true
+			c.ComplianceSettings.Enable = &b
+		})
+
+		// User is not authorized to get post
+		_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session2, false)
+		require.NotNil(t, err)
+
+		// User is authorized to get post
+		_, err = th.App.GetPostIfAuthorized(th.Context, post.Id, session1, false)
+		require.Nil(t, err)
+	})
 }
 
 func TestShouldNotRefollowOnOthersReply(t *testing.T) {