diff --git a/app/assets/javascripts/discourse/components/groups-form-interaction-fields.js.es6 b/app/assets/javascripts/discourse/components/groups-form-interaction-fields.js.es6 index 6ccc23df737..3b792fdbf3d 100644 --- a/app/assets/javascripts/discourse/components/groups-form-interaction-fields.js.es6 +++ b/app/assets/javascripts/discourse/components/groups-form-interaction-fields.js.es6 @@ -13,19 +13,25 @@ export default Ember.Component.extend({ }, { name: I18n.t( - "admin.groups.manage.interaction.visibility_levels.members" + "admin.groups.manage.interaction.visibility_levels.logged_on_users" ), value: 1 }, { - name: I18n.t("admin.groups.manage.interaction.visibility_levels.staff"), + name: I18n.t( + "admin.groups.manage.interaction.visibility_levels.members" + ), value: 2 }, + { + name: I18n.t("admin.groups.manage.interaction.visibility_levels.staff"), + value: 3 + }, { name: I18n.t( "admin.groups.manage.interaction.visibility_levels.owners" ), - value: 3 + value: 4 } ]; diff --git a/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs b/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs index 5ed87a5c027..b2dc61b09f2 100644 --- a/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs +++ b/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs @@ -9,6 +9,10 @@ content=visibilityLevelOptions castInteger=true class="groups-form-visibility-level"}} + +
+ {{i18n 'admin.groups.manage.interaction.visibility_levels.description'}} +
{{/if}} diff --git a/app/models/group.rb b/app/models/group.rb index 4d50dbe6d92..19cad44a4f9 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -89,9 +89,10 @@ class Group < ActiveRecord::Base def self.visibility_levels @visibility_levels = Enum.new( public: 0, - members: 1, - staff: 2, - owners: 3 + logged_on_users: 1, + members: 2, + staff: 3, + owners: 4 ) end @@ -112,6 +113,11 @@ class Group < ActiveRecord::Base UNION ALL + SELECT g.id FROM groups g + WHERE g.visibility_level = :logged_on_users AND :user_id IS NOT NULL + + UNION ALL + SELECT g.id FROM groups g JOIN group_users gu ON gu.group_id = g.id AND gu.user_id = :user_id @@ -324,6 +330,8 @@ class Group < ActiveRecord::Base group.update!(messageable_level: ALIAS_LEVELS[:everyone]) end + group.update!(visibility_level: Group.visibility_levels[:logged_on_users]) if group.visibility_level == Group.visibility_levels[:public] + # Remove people from groups they don't belong in. remove_subquery = case name diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 2157f174f32..b7d2452fc5c 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3185,9 +3185,11 @@ en: visibility_levels: title: "Who can see this group?" public: "Everyone" - members: "Group owners, members and admins" + logged_on_users: "Logged on users" + members: "Group owners and members" staff: "Group owners and staff" - owners: "Group owners and admins" + owners: "Group owners" + description: "Admins can see all groups." membership: automatic: Automatic trust_level: Trust Level diff --git a/db/fixtures/002_groups.rb b/db/fixtures/002_groups.rb index f3ed4808422..d9cc999bebe 100644 --- a/db/fixtures/002_groups.rb +++ b/db/fixtures/002_groups.rb @@ -5,4 +5,4 @@ if g = Group.find_by(name: 'trust_level_5', id: 15) g.destroy! end -Group.where(name: 'everyone').update_all(visibility_level: Group.visibility_levels[:owners]) +Group.where(name: 'everyone').update_all(visibility_level: Group.visibility_levels[:staff]) diff --git a/db/migrate/20190705173948_renumber_group_visibility_levels.rb b/db/migrate/20190705173948_renumber_group_visibility_levels.rb new file mode 100644 index 00000000000..fce98d6a03f --- /dev/null +++ b/db/migrate/20190705173948_renumber_group_visibility_levels.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class RenumberGroupVisibilityLevels < ActiveRecord::Migration[5.2] + def up + execute "UPDATE groups SET visibility_level = 4 WHERE visibility_level = 3" + execute "UPDATE groups SET visibility_level = 3 WHERE visibility_level = 2" + execute "UPDATE groups SET visibility_level = 2 WHERE visibility_level = 1" + end + + def down + execute "UPDATE groups SET visibility_level = 0 WHERE visibility_level = 1" + execute "UPDATE groups SET visibility_level = 1 WHERE visibility_level = 2" + execute "UPDATE groups SET visibility_level = 2 WHERE visibility_level = 3" + execute "UPDATE groups SET visibility_level = 3 WHERE visibility_level = 4" + end +end diff --git a/lib/guardian.rb b/lib/guardian.rb index 27bdff357f5..e2d4c24e613 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -194,6 +194,7 @@ class Guardian return true if group.visibility_level == Group.visibility_levels[:public] return true if is_admin? return true if is_staff? && group.visibility_level == Group.visibility_levels[:staff] + return true if authenticated? && group.visibility_level == Group.visibility_levels[:logged_on_users] return false if user.blank? membership = GroupUser.find_by(group_id: group.id, user_id: user.id) @@ -213,6 +214,7 @@ class Guardian return true if groups.all? { |g| g.visibility_level == Group.visibility_levels[:public] } return true if is_admin? return true if is_staff? && groups.all? { |g| g.visibility_level == Group.visibility_levels[:staff] } + return true if authenticated? && groups.all? { |g| g.visibility_level == Group.visibility_levels[:logged_on_users] } return false if user.blank? memberships = GroupUser.where(group: groups, user_id: user.id).pluck(:owner) diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 98eeb94cb61..adab91e7288 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -3027,7 +3027,7 @@ describe Guardian do end describe(:can_see_group) do - it 'Correctly handles owner visibile groups' do + it 'Correctly handles owner visible groups' do group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:owners]) member = Fabricate(:user) @@ -3039,13 +3039,14 @@ describe Guardian do group.reload expect(Guardian.new(admin).can_see_group?(group)).to eq(true) + expect(Guardian.new(another_user).can_see_group?(group)).to eq(false) expect(Guardian.new(moderator).can_see_group?(group)).to eq(false) expect(Guardian.new(member).can_see_group?(group)).to eq(false) expect(Guardian.new.can_see_group?(group)).to eq(false) expect(Guardian.new(owner).can_see_group?(group)).to eq(true) end - it 'Correctly handles staff visibile groups' do + it 'Correctly handles staff visible groups' do group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:staff]) member = Fabricate(:user) @@ -3056,6 +3057,7 @@ describe Guardian do group.add_owner(owner) group.reload + expect(Guardian.new(another_user).can_see_group?(group)).to eq(false) expect(Guardian.new(member).can_see_group?(group)).to eq(false) expect(Guardian.new(admin).can_see_group?(group)).to eq(true) expect(Guardian.new(moderator).can_see_group?(group)).to eq(true) @@ -3063,7 +3065,7 @@ describe Guardian do expect(Guardian.new.can_see_group?(group)).to eq(false) end - it 'Correctly handles member visibile groups' do + it 'Correctly handles member visible groups' do group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:members]) member = Fabricate(:user) @@ -3076,11 +3078,30 @@ describe Guardian do expect(Guardian.new(moderator).can_see_group?(group)).to eq(false) expect(Guardian.new.can_see_group?(group)).to eq(false) + expect(Guardian.new(another_user).can_see_group?(group)).to eq(false) expect(Guardian.new(admin).can_see_group?(group)).to eq(true) expect(Guardian.new(member).can_see_group?(group)).to eq(true) expect(Guardian.new(owner).can_see_group?(group)).to eq(true) end + it 'Correctly handles logged-on-user visible groups' do + group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:logged_on_users]) + member = Fabricate(:user) + group.add(member) + group.save! + + owner = Fabricate(:user) + group.add_owner(owner) + group.reload + + expect(Guardian.new.can_see_group?(group)).to eq(false) + expect(Guardian.new(moderator).can_see_group?(group)).to eq(true) + expect(Guardian.new(admin).can_see_group?(group)).to eq(true) + expect(Guardian.new(member).can_see_group?(group)).to eq(true) + expect(Guardian.new(owner).can_see_group?(group)).to eq(true) + expect(Guardian.new(another_user).can_see_group?(group)).to eq(true) + end + it 'Correctly handles public groups' do group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:public]) @@ -3090,7 +3111,7 @@ describe Guardian do end describe '#can_see_groups?' do - it 'correctly handles owner visibile groups' do + it 'correctly handles owner visible groups' do group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:owners]) member = Fabricate(:user) @@ -3102,6 +3123,7 @@ describe Guardian do group.reload expect(Guardian.new(admin).can_see_groups?([group])).to eq(true) + expect(Guardian.new(another_user).can_see_groups?([group])).to eq(false) expect(Guardian.new(moderator).can_see_groups?([group])).to eq(false) expect(Guardian.new(member).can_see_groups?([group])).to eq(false) expect(Guardian.new.can_see_groups?([group])).to eq(false) @@ -3126,9 +3148,10 @@ describe Guardian do expect(Guardian.new(member).can_see_groups?([group, group2])).to eq(false) expect(Guardian.new.can_see_groups?([group, group2])).to eq(false) expect(Guardian.new(owner).can_see_groups?([group, group2])).to eq(false) + expect(Guardian.new(another_user).can_see_groups?([group, group2])).to eq(false) end - it 'correctly handles staff visibile groups' do + it 'correctly handles staff visible groups' do group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:staff]) member = Fabricate(:user) @@ -3144,9 +3167,10 @@ describe Guardian do expect(Guardian.new(moderator).can_see_groups?([group])).to eq(true) expect(Guardian.new(owner).can_see_groups?([group])).to eq(true) expect(Guardian.new.can_see_groups?([group])).to eq(false) + expect(Guardian.new(another_user).can_see_groups?([group])).to eq(false) end - it 'correctly handles member visibile groups' do + it 'correctly handles member visible groups' do group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:members]) member = Fabricate(:user) @@ -3157,6 +3181,7 @@ describe Guardian do group.add_owner(owner) group.reload + expect(Guardian.new(another_user).can_see_groups?([group])).to eq(false) expect(Guardian.new(moderator).can_see_groups?([group])).to eq(false) expect(Guardian.new.can_see_groups?([group])).to eq(false) expect(Guardian.new(admin).can_see_groups?([group])).to eq(true) @@ -3164,6 +3189,25 @@ describe Guardian do expect(Guardian.new(owner).can_see_groups?([group])).to eq(true) end + it 'correctly handles logged-on-user visible groups' do + group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:logged_on_users]) + + member = Fabricate(:user) + group.add(member) + group.save! + + owner = Fabricate(:user) + group.add_owner(owner) + group.reload + + expect(Guardian.new(member).can_see_groups?([group])).to eq(true) + expect(Guardian.new(admin).can_see_groups?([group])).to eq(true) + expect(Guardian.new(moderator).can_see_groups?([group])).to eq(true) + expect(Guardian.new(owner).can_see_groups?([group])).to eq(true) + expect(Guardian.new.can_see_groups?([group])).to eq(false) + expect(Guardian.new(another_user).can_see_groups?([group])).to eq(true) + end + it 'correctly handles the case where the user is not a member of every group' do group1 = Group.new(name: 'group', visibility_level: Group.visibility_levels[:members]) group2 = Group.new(name: 'group2', visibility_level: Group.visibility_levels[:members]) @@ -3190,7 +3234,7 @@ describe Guardian do expect(Guardian.new.can_see_groups?([group])).to eq(true) end - it 'correctly handles there case where not every group is public' do + it 'correctly handles case where not every group is public' do group1 = Group.new(name: 'group', visibility_level: Group.visibility_levels[:public]) group2 = Group.new(name: 'group', visibility_level: Group.visibility_levels[:private]) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 0625cbad4d4..42cc6ee7eb9 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -243,6 +243,14 @@ describe Group do expect(g.visibility_level).to eq(Group.visibility_levels[:staff]) end + it "makes sure automatic groups are visible to logged on users" do + g = Group.refresh_automatic_group!(:moderators) + expect(g.visibility_level).to eq(Group.visibility_levels[:logged_on_users]) + + tl0 = Group.refresh_automatic_group!(:trust_level_0) + expect(tl0.visibility_level).to eq(Group.visibility_levels[:logged_on_users]) + end + it "ensures that the moderators group is messageable by all" do group = Group.find(Group::AUTO_GROUPS[:moderators]) group.update!(messageable_level: Group::ALIAS_LEVELS[:nobody]) @@ -651,6 +659,7 @@ describe Group do it 'correctly restricts group visibility' do group = Fabricate.build(:group, visibility_level: Group.visibility_levels[:owners]) + logged_on_user = Fabricate(:user) member = Fabricate(:user) group.add(member) group.save! @@ -665,6 +674,7 @@ describe Group do expect(can_view?(owner, group)).to eq(true) expect(can_view?(moderator, group)).to eq(false) expect(can_view?(member, group)).to eq(false) + expect(can_view?(logged_on_user, group)).to eq(false) expect(can_view?(nil, group)).to eq(false) group.update_columns(visibility_level: Group.visibility_levels[:staff]) @@ -673,6 +683,7 @@ describe Group do expect(can_view?(owner, group)).to eq(true) expect(can_view?(moderator, group)).to eq(true) expect(can_view?(member, group)).to eq(false) + expect(can_view?(logged_on_user, group)).to eq(false) expect(can_view?(nil, group)).to eq(false) group.update_columns(visibility_level: Group.visibility_levels[:members]) @@ -681,6 +692,7 @@ describe Group do expect(can_view?(owner, group)).to eq(true) expect(can_view?(moderator, group)).to eq(false) expect(can_view?(member, group)).to eq(true) + expect(can_view?(logged_on_user, group)).to eq(false) expect(can_view?(nil, group)).to eq(false) group.update_columns(visibility_level: Group.visibility_levels[:public]) @@ -689,7 +701,17 @@ describe Group do expect(can_view?(owner, group)).to eq(true) expect(can_view?(moderator, group)).to eq(true) expect(can_view?(member, group)).to eq(true) + expect(can_view?(logged_on_user, group)).to eq(true) expect(can_view?(nil, group)).to eq(true) + + group.update_columns(visibility_level: Group.visibility_levels[:logged_on_users]) + + expect(can_view?(admin, group)).to eq(true) + expect(can_view?(owner, group)).to eq(true) + expect(can_view?(moderator, group)).to eq(true) + expect(can_view?(member, group)).to eq(true) + expect(can_view?(logged_on_user, group)).to eq(true) + expect(can_view?(nil, group)).to eq(false) end end diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 6e6ae849b6d..e24bad0c98d 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -62,6 +62,10 @@ describe GroupsController do end context 'sortable' do + before do + sign_in(user) + end + let!(:other_group) { Fabricate(:group, name: "zzzzzz", users: [user]) } %w{ @@ -126,7 +130,7 @@ describe GroupsController do expect(group_ids).to include(group.id) expect(group_ids).to_not include(staff_group.id) expect(response_body["load_more_groups"]).to eq("/groups?page=1") - expect(response_body["total_rows_groups"]).to eq(2) + expect(response_body["total_rows_groups"]).to eq(1) expect(response_body["extras"]["type_filters"].map(&:to_sym)).to eq( described_class::TYPE_FILTERS.keys - [:my, :owner, :automatic] @@ -1371,7 +1375,8 @@ describe GroupsController do before do group.update!( name: 'GOT', - full_name: 'Daenerys Targaryen' + full_name: 'Daenerys Targaryen', + visibility_level: Group.visibility_levels[:logged_on_users] ) hidden_group diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 27ced58f4fb..2cdb8628063 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -242,6 +242,16 @@ RSpec.describe ListController do end end + describe 'group restricted to logged-on-users' do + before { group.update!(visibility_level: Group.visibility_levels[:logged_on_users]) } + + it 'should return the right response' do + get "/topics/groups/#{group.name}.json" + + expect(response.status).to eq(403) + end + end + describe 'restricted group' do before { group.update!(visibility_level: Group.visibility_levels[:staff]) } @@ -265,6 +275,16 @@ RSpec.describe ListController do expect(response.status).to eq(403) end end + + describe 'group restricted to logged-on-users' do + before { group.update!(visibility_level: Group.visibility_levels[:logged_on_users]) } + + it 'should return the right response' do + get "/topics/groups/#{group.name}.json" + + expect(response.status).to eq(200) + end + end end describe 'for a group user' do