MM-55042 Fixes permissions checks (#25253)

* fix permissions check on SessionHasPermissionToTeams and SessionHasPermissionToChannels

* add tests, make updates

* remove commented code

* update to handle session permissions first

* Update authorization.go

Remove unnecessary check

---------

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

server/channels/app/authorization.go

@@ -68,31 +68,22 @@ func (a *App) SessionHasPermissionToTeams(c request.CTX, session model.Session,
 			return false
 		}
 	}
-	if session.IsUnrestricted() {
+
+	// Check session permission, if it allows access, no need to check teams.
+	if a.SessionHasPermissionTo(session, permission) {
 		return true
 	}
-
-	// Getting the list of unique roles from all teams.
-	var roles []string
-	uniqueRoles := make(map[string]bool)
 	for _, teamID := range teamIDs {
 		tm := session.GetTeamByTeamId(teamID)
 		if tm != nil {
-			for _, role := range tm.GetRoles() {
-				uniqueRoles[role] = true
+			// If a team member has permission, then no need to check further.
+			if a.RolesGrantPermission(tm.GetRoles(), permission.Id) {
+				continue
 			}
 		}
+		return false
 	}
-
-	for role := range uniqueRoles {
-		roles = append(roles, role)
-	}
-
-	if a.RolesGrantPermission(roles, permission.Id) {
-		return true
-	}
-
-	return a.RolesGrantPermission(session.GetUserRoles(), permission.Id)
+	return true
 }
 
 func (a *App) SessionHasPermissionToChannel(c request.CTX, session model.Session, channelID string, permission *model.Permission) bool {
@@ -139,55 +130,34 @@ func (a *App) SessionHasPermissionToChannels(c request.CTX, session model.Sessio
 		}
 	}
 
-	if session.IsUnrestricted() {
+	// if System Roles (ie. Admin, TeamAdmin) allow permissions
+	// if so, no reason to check team
+	if a.SessionHasPermissionTo(session, permission) {
+		// make sure all channels exist, otherwise return false.
+		for _, channelID := range channelIDs {
+			_, appErr := a.GetChannel(c, channelID)
+			if appErr != nil && appErr.StatusCode == http.StatusNotFound {
+				return false
+			}
+		}
 		return true
 	}
 
 	ids, err := a.Srv().Store().Channel().GetAllChannelMembersForUser(session.UserId, true, true)
-
 	var channelRoles []string
-	uniqueRoles := make(map[string]bool)
-	if err == nil {
-		for _, channelID := range channelIDs {
+	for _, channelID := range channelIDs {
+		if err == nil {
+			// If a channel member has permission, then no need to check further.
 			if roles, ok := ids[channelID]; ok {
-				for _, role := range strings.Fields(roles) {
-					uniqueRoles[role] = true
+				channelRoles = strings.Fields(roles)
+				if a.RolesGrantPermission(channelRoles, permission.Id) {
+					continue
 				}
 			}
 		}
-	}
-
-	for role := range uniqueRoles {
-		channelRoles = append(channelRoles, role)
-	}
-
-	if a.RolesGrantPermission(channelRoles, permission.Id) {
-		return true
-	}
-
-	channels, appErr := a.GetChannels(c, channelIDs)
-	if appErr != nil && appErr.StatusCode == http.StatusNotFound {
 		return false
 	}
-
-	// Get TeamIDs from channels
-	uniqueTeamIDs := make(map[string]bool)
-	for _, ch := range channels {
-		if ch.TeamId != "" {
-			uniqueTeamIDs[ch.TeamId] = true
-		}
-	}
-
-	var teamIDs []string
-	for teamID := range uniqueTeamIDs {
-		teamIDs = append(teamIDs, teamID)
-	}
-
-	if appErr == nil && len(teamIDs) > 0 {
-		return a.SessionHasPermissionToTeams(c, session, teamIDs, permission)
-	}
-
-	return a.SessionHasPermissionTo(session, permission)
+	return true
 }
 
 func (a *App) SessionHasPermissionToGroup(session model.Session, groupID string, permission *model.Permission) bool {

server/channels/app/authorization_test.go

@@ -70,6 +70,64 @@ func TestHasPermissionToTeam(t *testing.T) {
 	assert.True(t, th.App.HasPermissionToTeam(th.Context, th.SystemAdminUser.Id, th.BasicTeam.Id, model.PermissionListTeamChannels))
 }
 
+func TestSessionHasPermissionToTeams(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	// Adding another team with more channels (public and private)
+	myTeam := th.CreateTeam()
+
+	bothTeams := []string{th.BasicTeam.Id, myTeam.Id}
+	t.Run("session with team members can access teams", func(t *testing.T) {
+		session := model.Session{
+			UserId: th.BasicUser.Id,
+			TeamMembers: []*model.TeamMember{
+				{
+					UserId: th.BasicUser.Id,
+					TeamId: th.BasicTeam.Id,
+					Roles:  model.TeamUserRoleId,
+				},
+				{
+					UserId: th.BasicUser.Id,
+					TeamId: myTeam.Id,
+					Roles:  model.TeamUserRoleId,
+				},
+			},
+		}
+		assert.True(t, th.App.SessionHasPermissionToTeams(th.Context, session, bothTeams, model.PermissionJoinPublicChannels))
+	})
+
+	t.Run("session with one team members cannot access teams", func(t *testing.T) {
+		session := model.Session{
+			UserId: th.BasicUser.Id,
+			TeamMembers: []*model.TeamMember{
+				{
+					UserId: th.BasicUser.Id,
+					TeamId: th.BasicTeam.Id,
+					Roles:  model.TeamUserRoleId,
+				},
+			},
+		}
+		assert.False(t, th.App.SessionHasPermissionToTeams(th.Context, session, bothTeams, model.PermissionJoinPublicChannels))
+	})
+
+	t.Run("session role  cannot access teams", func(t *testing.T) {
+		session := model.Session{
+			UserId: th.BasicUser.Id,
+			Roles:  model.SystemUserRoleId,
+		}
+		assert.False(t, th.App.SessionHasPermissionToTeams(th.Context, session, bothTeams, model.PermissionJoinPublicChannels))
+	})
+
+	t.Run("session admin role can access teams", func(t *testing.T) {
+		session := model.Session{
+			UserId: th.BasicUser.Id,
+			Roles:  model.SystemAdminRoleId,
+		}
+		assert.True(t, th.App.SessionHasPermissionToTeams(th.Context, session, bothTeams, model.PermissionJoinPublicChannels))
+	})
+}
+
 func TestSessionHasPermissionToChannel(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()
@@ -114,6 +172,71 @@ func TestSessionHasPermissionToChannel(t *testing.T) {
 	})
 }
 
+func TestSessionHasPermissionToChannels(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	ch1 := th.CreateChannel(th.Context, th.BasicTeam)
+	ch2 := th.CreatePrivateChannel(th.Context, th.BasicTeam)
+	th.App.AddUserToChannel(th.Context, th.BasicUser, ch1, false)
+	th.App.AddUserToChannel(th.Context, th.BasicUser, ch2, false)
+
+	allChannels := []string{th.BasicChannel.Id, ch1.Id, ch2.Id}
+
+	t.Run("basic user can access basic channels", func(t *testing.T) {
+		session := model.Session{
+			UserId: th.BasicUser.Id,
+		}
+
+		assert.True(t, th.App.SessionHasPermissionToChannels(th.Context, session, allChannels, model.PermissionReadChannel))
+	})
+
+	t.Run("basic user removed from channel cannot access", func(t *testing.T) {
+		session := model.Session{
+			UserId: th.BasicUser.Id,
+		}
+
+		th.App.removeUserFromChannel(th.Context, th.BasicUser.Id, th.SystemAdminUser.Id, ch1)
+		assert.False(t, th.App.SessionHasPermissionToChannels(th.Context, session, allChannels, model.PermissionReadChannel))
+	})
+
+	t.Run("System Admins can access basic channels", func(t *testing.T) {
+		session := model.Session{
+			UserId: th.SystemAdminUser.Id,
+			Roles:  model.SystemAdminRoleId,
+		}
+		assert.True(t, th.App.SessionHasPermissionToChannels(th.Context, session, allChannels, model.PermissionManagePrivateChannelMembers))
+	})
+
+	t.Run("does not panic if fetching channel causes an error", func(t *testing.T) {
+		// Regression test for MM-29812
+		// Mock the channel store so getting the channel returns with an error, as per the bug report.
+		mockStore := mocks.Store{}
+		mockChannelStore := mocks.ChannelStore{}
+		mockChannelStore.On("Get", mock.Anything, mock.Anything).Return(nil, fmt.Errorf("arbitrary error"))
+		mockChannelStore.On("GetAllChannelMembersForUser", mock.Anything, mock.Anything, mock.Anything).Return(th.App.Srv().Store().Channel().GetAllChannelMembersForUser(th.BasicUser.Id, false, false))
+		mockChannelStore.On("ClearCaches").Return()
+		mockStore.On("Channel").Return(&mockChannelStore)
+		mockStore.On("FileInfo").Return(th.App.Srv().Store().FileInfo())
+		mockStore.On("License").Return(th.App.Srv().Store().License())
+		mockStore.On("Post").Return(th.App.Srv().Store().Post())
+		mockStore.On("Role").Return(th.App.Srv().Store().Role())
+		mockStore.On("System").Return(th.App.Srv().Store().System())
+		mockStore.On("Team").Return(th.App.Srv().Store().Team())
+		mockStore.On("User").Return(th.App.Srv().Store().User())
+		mockStore.On("Webhook").Return(th.App.Srv().Store().Webhook())
+		mockStore.On("Close").Return(nil)
+		th.App.Srv().SetStore(&mockStore)
+
+		// If there's an error returned from the GetChannel call the code should continue to cascade and since there
+		// are no session level permissions in this test case, the permission should be denied.
+		session := model.Session{
+			UserId: th.BasicUser.Id,
+		}
+		assert.False(t, th.App.SessionHasPermissionToChannels(th.Context, session, allChannels, model.PermissionReadChannel))
+	})
+}
+
 func TestHasPermissionToUser(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()

server/channels/store/storetest/channel_store.go

@@ -3533,6 +3533,10 @@ func testChannelStoreGetChannels(t *testing.T, rctx request.CTX, ss store.Store)
 	_, ok = ids4[o1.Id]
 	require.True(t, ok, "missing channel")
 
+	ids5, err := ss.Channel().GetAllChannelMembersForUser(model.NewId(), true, true)
+	require.NoError(t, err)
+	require.True(t, len(ids5) == 0)
+
 	// Sleeping to guarantee that the
 	// UpdateAt is different.
 	// The proper way would be to set UpdateAt during channel creation itself,