From 2a17f1ccd79b480f95731a020dad0b7b023ebe39 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 21 Jul 2017 15:12:24 +0900 Subject: [PATCH] FIX: Group owners should be able to invite users to their groups. https://meta.discourse.org/t/group-owner-cannot-send-an-invite-to-a-group/60617/12 --- .../javascripts/admin/models/web-hook.js.es6 | 3 +- .../components/group-selector.js.es6 | 26 +++-- .../components/search-advanced-options.js.es6 | 2 +- .../discourse/controllers/invite.js.es6 | 24 ++--- .../javascripts/discourse/models/group.js.es6 | 2 +- app/controllers/admin/groups_controller.rb | 15 --- app/controllers/groups_controller.rb | 19 +++- app/controllers/invites_controller.rb | 20 +++- app/controllers/topics_controller.rb | 10 +- app/models/group.rb | 19 ++-- .../basic_group_user_serializer.rb | 6 +- config/routes.rb | 4 + lib/guardian.rb | 6 +- spec/components/guardian_spec.rb | 25 ++++- .../admin/groups_controller_spec.rb | 40 -------- spec/controllers/invites_controller_spec.rb | 18 +++- spec/integration/groups_spec.rb | 95 ++++++++++++++++++- .../basic_group_user_serializer_spec.rb | 36 +++++++ 18 files changed, 256 insertions(+), 114 deletions(-) create mode 100644 spec/serializers/basic_group_user_serializer_spec.rb diff --git a/app/assets/javascripts/admin/models/web-hook.js.es6 b/app/assets/javascripts/admin/models/web-hook.js.es6 index 325d9310452..27d72878b08 100644 --- a/app/assets/javascripts/admin/models/web-hook.js.es6 +++ b/app/assets/javascripts/admin/models/web-hook.js.es6 @@ -37,7 +37,7 @@ export default RestModel.extend({ }, groupFinder(term) { - return Group.findAll({search: term, ignore_automatic: false}); + return Group.findAll({ term: term, ignore_automatic: false }); }, @computed('wildcard_web_hook', 'web_hook_event_types.[]') @@ -82,4 +82,3 @@ export default RestModel.extend({ return this.createProperties(); } }); - diff --git a/app/assets/javascripts/discourse/components/group-selector.js.es6 b/app/assets/javascripts/discourse/components/group-selector.js.es6 index 4beb5a3a932..563911b7252 100644 --- a/app/assets/javascripts/discourse/components/group-selector.js.es6 +++ b/app/assets/javascripts/discourse/components/group-selector.js.es6 @@ -15,31 +15,27 @@ export default Ember.Component.extend({ @on('didInsertElement') _initializeAutocomplete(opts) { - var self = this; - var selectedGroups; - var groupNames = this.get('groupNames'); + let selectedGroups; + let groupNames = this.get('groupNames'); - self.$('input').autocomplete({ + this.$('input').autocomplete({ allowAny: false, items: _.isArray(groupNames) ? groupNames : (Ember.isEmpty(groupNames)) ? [] : [groupNames], single: this.get('single'), updateData: (opts && opts.updateData) ? opts.updateData : false, - onChangeItems: function(items){ + onChangeItems: items => { selectedGroups = items; - self.set("groupNames", items.join(",")); + this.set("groupNames", items.join(",")); }, - transformComplete: function(g) { + transformComplete: g => { return g.name; }, - dataSource: function(term) { - return self.get("groupFinder")(term).then(function(groups){ + dataSource: term => { + return this.get("groupFinder")(term).then(groups => { + if(!selectedGroups) return groups; - if(!selectedGroups){ - return groups; - } - - return groups.filter(function(group){ - return !selectedGroups.any(function(s){return s === group.name;}); + return groups.filter(group => { + return !selectedGroups.any(s => s === group.name); }); }); }, diff --git a/app/assets/javascripts/discourse/components/search-advanced-options.js.es6 b/app/assets/javascripts/discourse/components/search-advanced-options.js.es6 index e0fea77cd55..daea5cdbe5c 100644 --- a/app/assets/javascripts/discourse/components/search-advanced-options.js.es6 +++ b/app/assets/javascripts/discourse/components/search-advanced-options.js.es6 @@ -527,7 +527,7 @@ export default Em.Component.extend({ }, groupFinder(term) { - return Group.findAll({search: term, ignore_automatic: false}); + return Group.findAll({ term: term, ignore_automatic: false }); }, badgeFinder(term) { diff --git a/app/assets/javascripts/discourse/controllers/invite.js.es6 b/app/assets/javascripts/discourse/controllers/invite.js.es6 index 90020f449df..1846cf93604 100644 --- a/app/assets/javascripts/discourse/controllers/invite.js.es6 +++ b/app/assets/javascripts/discourse/controllers/invite.js.es6 @@ -26,12 +26,7 @@ export default Ember.Controller.extend(ModalFunctionality, { } }, - @computed - isAdmin() { - return Discourse.User.currentProp("admin"); - }, - - @computed('isAdmin', 'emailOrUsername', 'invitingToTopic', 'isPrivateTopic', 'model.groupNames', 'model.saving', 'model.details.can_invite_to') + @computed('model.admin', 'emailOrUsername', 'invitingToTopic', 'isPrivateTopic', 'model.groupNames', 'model.saving', 'model.details.can_invite_to') disabled(isAdmin, emailOrUsername, invitingToTopic, isPrivateTopic, groupNames, saving, can_invite_to) { if (saving) return true; if (Ember.isEmpty(emailOrUsername)) return true; @@ -48,7 +43,7 @@ export default Ember.Controller.extend(ModalFunctionality, { return false; }, - @computed('isAdmin', 'emailOrUsername', 'model.saving', 'isPrivateTopic', 'model.groupNames', 'hasCustomMessage') + @computed('model.admin', 'emailOrUsername', 'model.saving', 'isPrivateTopic', 'model.groupNames', 'hasCustomMessage') disabledCopyLink(isAdmin, emailOrUsername, saving, isPrivateTopic, groupNames, hasCustomMessage) { if (hasCustomMessage) return true; if (saving) return true; @@ -98,10 +93,15 @@ export default Ember.Controller.extend(ModalFunctionality, { // Allow Existing Members? (username autocomplete) allowExistingMembers: Ember.computed.alias('invitingToTopic'), + @computed("model.admin", "model.group_users") + isGroupOwnerOrAdmin(isAdmin, groupUsers) { + return isAdmin || groupUsers.some(groupUser => groupUser.owner); + }, + // Show Groups? (add invited user to private group) - @computed('isAdmin', 'emailOrUsername', 'isPrivateTopic', 'isMessage', 'invitingToTopic', 'canInviteViaEmail') - showGroups(isAdmin, emailOrUsername, isPrivateTopic, isMessage, invitingToTopic, canInviteViaEmail) { - return isAdmin && + @computed('isGroupOwnerOrAdmin', 'emailOrUsername', 'isPrivateTopic', 'isMessage', 'invitingToTopic', 'canInviteViaEmail') + showGroups(isGroupOwnerOrAdmin, emailOrUsername, isPrivateTopic, isMessage, invitingToTopic, canInviteViaEmail) { + return isGroupOwnerOrAdmin && canInviteViaEmail && !isMessage && (emailValid(emailOrUsername) || isPrivateTopic || !invitingToTopic); @@ -113,7 +113,7 @@ export default Ember.Controller.extend(ModalFunctionality, { }, // Instructional text for the modal. - @computed('isMessage', 'invitingToTopic', 'emailOrUsername', 'isPrivateTopic', 'isAdmin', 'canInviteViaEmail') + @computed('isMessage', 'invitingToTopic', 'emailOrUsername', 'isPrivateTopic', 'model.admin', 'canInviteViaEmail') inviteInstructions(isMessage, invitingToTopic, emailOrUsername, isPrivateTopic, isAdmin, canInviteViaEmail) { if (!canInviteViaEmail) { // can't invite via email, only existing users @@ -150,7 +150,7 @@ export default Ember.Controller.extend(ModalFunctionality, { }, groupFinder(term) { - return Group.findAll({search: term, ignore_automatic: true}); + return Group.findAll({ term: term, ignore_automatic: true }); }, @computed('isMessage', 'emailOrUsername', 'invitingExistingUserToTopic') diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 67c70e510b5..c156613ef56 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/app/assets/javascripts/discourse/models/group.js.es6 @@ -216,7 +216,7 @@ const Group = RestModel.extend({ Group.reopenClass({ findAll(opts) { - return ajax("/admin/groups.json", { data: opts }).then(function (groups){ + return ajax("/groups/search.json", { data: opts }).then(groups => { return groups.map(g => Group.create(g)); }); }, diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 3c4182ac6da..2a1f9a36e02 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -1,19 +1,4 @@ class Admin::GroupsController < Admin::AdminController - - def index - groups = Group.order(:name).where("groups.id <> ?", Group::AUTO_GROUPS[:everyone]) - - if search = params[:search].to_s - groups = groups.where("name ILIKE ?", "%#{search}%") - end - - if params[:ignore_automatic].to_s == "true" - groups = groups.where(automatic: false) - end - - render_serialized(groups, BasicGroupSerializer) - end - def show render nothing: true end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 4d447b0d3bb..46a69b6be4f 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -6,7 +6,8 @@ class GroupsController < ApplicationController :update, :messages, :histories, - :request_membership + :request_membership, + :search ] skip_before_filter :preload_json, :check_xhr, only: [:posts_feed, :mentions_feed] @@ -296,6 +297,22 @@ class GroupsController < ApplicationController ) end + def search + groups = Group.visible_groups(current_user) + .where("groups.id <> ?", Group::AUTO_GROUPS[:everyone]) + .order(:name) + + if term = params[:term].to_s + groups = groups.where("name ILIKE :term OR full_name ILIKE :term", term: "%#{term}%") + end + + if params[:ignore_automatic].to_s == "true" + groups = groups.where(automatic: false) + end + + render_serialized(groups, BasicGroupSerializer) + end + private def group_params diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 4da902a8e40..42ab4ca712e 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -61,9 +61,13 @@ class InvitesController < ApplicationController def create params.require(:email) - group_ids = Group.lookup_group_ids(params) + groups = Group.lookup_groups( + group_ids: params[:group_ids], + group_names: params[:group_names] + ) - guardian.ensure_can_invite_to_forum!(group_ids) + guardian.ensure_can_invite_to_forum!(groups) + group_ids = groups.map(&:id) invite_exists = Invite.where(email: params[:email], invited_by_id: current_user.id).first if invite_exists && !guardian.can_send_multiple_invites?(current_user) @@ -71,7 +75,7 @@ class InvitesController < ApplicationController end begin - if Invite.invite_by_email(params[:email], current_user, _topic=nil, group_ids, params[:custom_message]) + if Invite.invite_by_email(params[:email], current_user, nil, group_ids, params[:custom_message]) render json: success_json else render json: failed_json, status: 422 @@ -83,9 +87,15 @@ class InvitesController < ApplicationController def create_invite_link params.require(:email) - group_ids = Group.lookup_group_ids(params) + + groups = Group.lookup_groups( + group_ids: params[:group_ids], + group_names: params[:group_names] + ) + + guardian.ensure_can_invite_to_forum!(groups) topic = Topic.find_by(id: params[:topic_id]) - guardian.ensure_can_invite_to_forum!(group_ids) + group_ids = groups.map(&:id) invite_exists = Invite.where(email: params[:email], invited_by_id: current_user.id).first if invite_exists && !guardian.can_send_multiple_invites?(current_user) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index c5930606039..e35ca7391c4 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -484,8 +484,14 @@ class TopicsController < ApplicationController topic = Topic.find_by(id: params[:topic_id]) - group_ids = Group.lookup_group_ids(params) - guardian.ensure_can_invite_to!(topic,group_ids) + + groups = Group.lookup_groups( + group_ids: params[:group_ids], + group_names: params[:group_names] + ) + + guardian.ensure_can_invite_to!(topic, groups) + group_ids = groups.map(&:id) begin if topic.invite(current_user, username_or_email, group_ids, params[:custom_message]) diff --git a/app/models/group.rb b/app/models/group.rb index 5a9720c7f58..bd014dea135 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -338,22 +338,19 @@ class Group < ActiveRecord::Base end end - def self.lookup_group_ids(opts) - if group_ids = opts[:group_ids] - group_ids = group_ids.split(",").map(&:to_i) - group_ids = Group.where(id: group_ids).pluck(:id) + def self.lookup_groups(group_ids: [], group_names: []) + if group_ids.present? + group_ids = group_ids.split(",") + group_ids.map!(&:to_i) + groups = Group.where(id: group_ids) if group_ids.present? end - group_ids ||= [] - - if group_names = opts[:group_names] + if group_names.present? group_names = group_names.split(",") - if group_names.present? - group_ids += Group.where(name: group_names).pluck(:id) - end + groups = (groups || Group).where(name: group_names) if group_names.present? end - group_ids + groups || [] end def self.desired_trust_level_groups(trust_level) diff --git a/app/serializers/basic_group_user_serializer.rb b/app/serializers/basic_group_user_serializer.rb index fa2015b8c4f..3b402724709 100644 --- a/app/serializers/basic_group_user_serializer.rb +++ b/app/serializers/basic_group_user_serializer.rb @@ -1,3 +1,7 @@ class BasicGroupUserSerializer < ApplicationSerializer - attributes :group_id, :user_id, :notification_level + attributes :group_id, :user_id, :notification_level, :owner + + def include_owner? + object.user_id == scope&.user&.id + end end diff --git a/config/routes.rb b/config/routes.rb index 037a1d3b0eb..a66dadb4895 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -440,6 +440,10 @@ Discourse::Application.routes.draw do get 'mentionable' get 'logs' => 'groups#histories' + collection do + get "search" => "groups#search" + end + member do put "members" => "groups#add_members" delete "members" => "groups#remove_member" diff --git a/lib/guardian.rb b/lib/guardian.rb index fb092563aff..48c9ea4077e 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -242,16 +242,16 @@ class Guardian (!SiteSetting.must_approve_users? && @user.has_trust_level?(TrustLevel[2])) || is_staff? ) && - (groups.blank? || is_admin?) + (groups.blank? || is_admin? || groups.all? { |g| can_edit_group?(g) }) end - def can_invite_to?(object, group_ids=nil) + def can_invite_to?(object, groups=nil) return false unless authenticated? return true if is_admin? return false unless SiteSetting.enable_private_messages? return false if (SiteSetting.max_invites_per_day.to_i == 0 && !is_staff?) return false unless can_see?(object) - return false if group_ids.present? + return false if groups.present? if object.is_a?(Topic) && object.category if object.category.groups.any? diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 8261d7c0b04..dc7680ea2cf 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -304,7 +304,7 @@ describe Guardian do end it 'returns true when the site requires approving users and is mod' do - SiteSetting.expects(:must_approve_users?).returns(true) + SiteSetting.must_approve_users = true expect(Guardian.new(moderator).can_invite_to_forum?).to be_truthy end @@ -328,6 +328,29 @@ describe Guardian do expect(Guardian.new(moderator).can_invite_to_forum?).to be_falsey end + context 'with groups' do + let(:group) { Fabricate(:group) } + let(:another_group) { Fabricate(:group) } + let(:groups) { [group, another_group] } + + before do + user.update!(trust_level: TrustLevel[2]) + group.add_owner(user) + end + + it 'returns false when user is not allowed to edit a group' do + expect(Guardian.new(user).can_invite_to_forum?(groups)).to eq(false) + + expect(Guardian.new(Fabricate(:admin)).can_invite_to_forum?(groups)) + .to eq(true) + end + + it 'returns true when user is allowed to edit groups' do + another_group.add_owner(user) + + expect(Guardian.new(user).can_invite_to_forum?(groups)).to eq(true) + end + end end describe 'can_invite_to?' do diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 27c95cb78f7..5e6b51581ff 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -12,46 +12,6 @@ describe Admin::GroupsController do expect(Admin::GroupsController < Admin::AdminController).to eq(true) end - context ".index" do - - it "produces valid json for groups" do - group = Fabricate.build(:group, name: "test") - group.add(@admin) - group.save - - xhr :get, :index - expect(response.status).to eq(200) - json = ::JSON.parse(response.body) - expect(json.select { |r| r["id"] == Group::AUTO_GROUPS[:everyone] }).to be_empty - expect(json.select { |r| r["id"] == group.id }).to eq([{ - "id"=>group.id, - "name"=>group.name, - "user_count"=>1, - "automatic"=>false, - "alias_level"=>0, - "visibility_level"=>0, - "automatic_membership_email_domains"=>nil, - "automatic_membership_retroactive"=>false, - "title"=>nil, - "primary_group"=>false, - "grant_trust_level"=>nil, - "incoming_email"=>nil, - "has_messages"=>false, - "flair_url"=>nil, - "flair_bg_color"=>nil, - "flair_color"=>nil, - "bio_raw"=>nil, - "bio_cooked"=>nil, - "public"=>false, - "allow_membership_requests"=>false, - "full_name"=>group.full_name, - "default_notification_level"=>3 - }]) - - end - - end - context ".bulk" do it "can assign users to a group by email or username" do group = Fabricate(:group, name: "test", primary_group: true, title: 'WAT', grant_trust_level: 3) diff --git a/spec/controllers/invites_controller_spec.rb b/spec/controllers/invites_controller_spec.rb index 786eec79690..ffd69f985cf 100644 --- a/spec/controllers/invites_controller_spec.rb +++ b/spec/controllers/invites_controller_spec.rb @@ -52,11 +52,11 @@ describe InvitesController do end - context '.create' do + context '#create' do it 'requires you to be logged in' do - expect { + expect do post :create, email: 'jake@adventuretime.ooo' - }.to raise_error(Discourse::NotLoggedIn) + end.to raise_error(Discourse::NotLoggedIn) end context 'while logged in' do @@ -86,6 +86,18 @@ describe InvitesController do expect(Invite.find_by(email: email).invited_groups.count).to eq(1) end + it 'allows group owners to invite to groups' do + group = Fabricate(:group) + user = log_in + user.update!(trust_level: TrustLevel[2]) + group.add_owner(user) + + post :create, email: email, group_names: group.name + + expect(response).to be_success + expect(Invite.find_by(email: email).invited_groups.count).to eq(1) + end + it "allows admin to send multiple invites to same email" do user = log_in(:admin) invite = Invite.invite_by_email("invite@example.com", user) diff --git a/spec/integration/groups_spec.rb b/spec/integration/groups_spec.rb index c2755944528..c380368de5f 100644 --- a/spec/integration/groups_spec.rb +++ b/spec/integration/groups_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' describe "Groups" do let(:user) { Fabricate(:user) } - let!(:group) { Fabricate(:group, users: [user]) } + let(:group) { Fabricate(:group, users: [user]) } describe 'viewing groups' do let!(:staff_group) do @@ -19,6 +19,7 @@ describe "Groups" do end it 'should return the right response' do + group get "/groups.json" expect(response).to be_success @@ -578,4 +579,96 @@ describe "Groups" do expect(topic.allowed_groups).to eq([]) end end + + describe 'search for groups' do + let(:hidden_group) do + Fabricate(:group, + visibility_level: Group.visibility_levels[:owners], + name: 'KingOfTheNorth' + ) + end + + before do + group.update!( + name: 'GOT', + full_name: 'Daenerys Targaryen' + ) + + hidden_group + end + + context 'as an anon user' do + it "returns the right response" do + expect { xhr :get, '/groups/search' }.to raise_error(Discourse::NotLoggedIn) + end + end + + context 'as a normal user' do + it "returns the right response" do + sign_in(user) + + xhr :get, '/groups/search' + + expect(response).to be_success + groups = JSON.parse(response.body) + + expected_ids = Group::AUTO_GROUPS.map { |name, id| id } + expected_ids.delete(Group::AUTO_GROUPS[:everyone]) + expected_ids << group.id + + expect(groups.map { |group| group["id"] }).to contain_exactly(*expected_ids) + + ['GO', 'nerys'].each do |term| + xhr :get, "/groups/search?term=#{term}" + + expect(response).to be_success + groups = JSON.parse(response.body) + + expect(groups.length).to eq(1) + expect(groups.first['id']).to eq(group.id) + end + + xhr :get, "/groups/search?term=KingOfTheNorth" + + expect(response).to be_success + groups = JSON.parse(response.body) + + expect(groups).to eq([]) + end + end + + context 'as a group owner' do + before do + hidden_group.add_owner(user) + end + + it "returns the right response" do + sign_in(user) + + xhr :get, "/groups/search?term=north" + + expect(response).to be_success + groups = JSON.parse(response.body) + + expect(groups.length).to eq(1) + expect(groups.first['id']).to eq(hidden_group.id) + end + end + + context 'as an admin' do + it "returns the right response" do + sign_in(Fabricate(:admin)) + + xhr :get, '/groups/search?ignore_automatic=true' + + expect(response).to be_success + groups = JSON.parse(response.body) + + expect(groups.length).to eq(2) + + expect(groups.map { |group| group['id'] }) + .to contain_exactly(group.id, hidden_group.id) + end + end + end end diff --git a/spec/serializers/basic_group_user_serializer_spec.rb b/spec/serializers/basic_group_user_serializer_spec.rb new file mode 100644 index 00000000000..065981604e3 --- /dev/null +++ b/spec/serializers/basic_group_user_serializer_spec.rb @@ -0,0 +1,36 @@ +require 'rails_helper' + +describe BasicGroupUserSerializer do + let(:group) { Fabricate(:group) } + let(:user) { Fabricate(:user) } + + before do + group.add(user) + end + + describe '#owner' do + describe 'when scoped to the user' do + it 'should be false' do + json = described_class.new( + GroupUser.last, + scope: Guardian.new(user), + root: false + ).as_json + + expect(json[:owner]).to eq(false) + end + end + + describe 'when not scoped to the user' do + it 'should be nil' do + json = described_class.new( + GroupUser.last, + scope: Guardian.new, + root: false + ).as_json + + expect(json[:owner]).to eq(nil) + end + end + end +end