diff --git a/server/channels/app/authorization.go b/server/channels/app/authorization.go index 8ed4cc072d..4e11559e91 100644 --- a/server/channels/app/authorization.go +++ b/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 { diff --git a/server/channels/app/authorization_test.go b/server/channels/app/authorization_test.go index 711988665f..414ac2d660 100644 --- a/server/channels/app/authorization_test.go +++ b/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() diff --git a/server/channels/store/storetest/channel_store.go b/server/channels/store/storetest/channel_store.go index 956d767313..1e8f61c355 100644 --- a/server/channels/store/storetest/channel_store.go +++ b/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,