From a2b284a0a45e5f2a2f90aca4e70b614c305f21b8 Mon Sep 17 00:00:00 2001 From: "Jason W. May" Date: Thu, 8 Jan 2015 15:35:52 -0800 Subject: [PATCH] table & model changes for group managers with permission to edit membership --- app/controllers/groups_controller.rb | 36 ++++++++++ app/models/group.rb | 7 ++ app/models/group_manager.rb | 19 ++++++ app/models/user.rb | 3 + config/routes.rb | 3 + db/migrate/20150108221703_group_managers.rb | 11 +++ lib/guardian.rb | 2 + lib/guardian/group_guardian.rb | 11 +++ spec/controllers/groups_controller_spec.rb | 74 +++++++++++++++++++++ spec/models/group_spec.rb | 23 +++++++ spec/models/user_spec.rb | 16 +++++ 11 files changed, 205 insertions(+) create mode 100644 app/models/group_manager.rb create mode 100644 db/migrate/20150108221703_group_managers.rb create mode 100644 lib/guardian/group_guardian.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index f6d7af0b5..9a51833a6 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -35,6 +35,38 @@ class GroupsController < ApplicationController } end + def add_members + guardian.ensure_can_edit!(the_group) + + added_users = [] + usernames = params.require(:usernames) + usernames.split(",").each do |username| + if user = User.find_by_username(username) + unless the_group.users.include?(user) + the_group.add(user) + added_users << user + end + end + end + + # always succeeds, even if bogus usernames were provided + render_serialized(added_users, GroupUserSerializer) + end + + def remove_member + guardian.ensure_can_edit!(the_group) + + removed_users = [] + username = params.require(:username) + if user = User.find_by_username(username) + the_group.remove(user) + removed_users << user + end + + # always succeeds, even if user was not a member + render_serialized(removed_users, GroupUserSerializer) + end + private def find_group(param_name) @@ -44,4 +76,8 @@ class GroupsController < ApplicationController group end + def the_group + @the_group ||= find_group(:group_id) + end + end diff --git a/app/models/group.rb b/app/models/group.rb index 1d9ba3721..d9985367d 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -7,6 +7,9 @@ class Group < ActiveRecord::Base has_many :categories, through: :category_groups has_many :users, through: :group_users + has_many :group_managers, dependent: :destroy + has_many :managers, through: :group_managers + after_save :destroy_deletions validate :name_format_validator @@ -277,6 +280,10 @@ class Group < ActiveRecord::Base self.group_users.where(user: user).each(&:destroy) end + def appoint_manager(user) + managers << user + end + protected def name_format_validator diff --git a/app/models/group_manager.rb b/app/models/group_manager.rb new file mode 100644 index 000000000..a30d523f5 --- /dev/null +++ b/app/models/group_manager.rb @@ -0,0 +1,19 @@ +class GroupManager < ActiveRecord::Base + belongs_to :group + belongs_to :manager, class_name: "User", foreign_key: :user_id +end + +# == Schema Information +# +# Table name: group_managers +# +# id :integer not null, primary key +# group_id :integer not null +# user_id :integer not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_group_managers_on_group_id_and_user_id (group_id,user_id) UNIQUE +# diff --git a/app/models/user.rb b/app/models/user.rb index 0f6c35b1f..f661b825f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -52,6 +52,9 @@ class User < ActiveRecord::Base has_many :groups, through: :group_users has_many :secure_categories, through: :groups, source: :categories + has_many :group_managers, dependent: :destroy + has_many :managed_groups, through: :group_managers, source: :group + has_one :user_search_data, dependent: :destroy has_one :api_key, dependent: :destroy diff --git a/config/routes.rb b/config/routes.rb index d7453e878..70d26b189 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -273,6 +273,9 @@ Discourse::Application.routes.draw do get 'members' get 'posts' get 'counts' + + put "members" => "groups#add_members" + delete "members/:username" => "groups#remove_member" end # In case people try the wrong URL diff --git a/db/migrate/20150108221703_group_managers.rb b/db/migrate/20150108221703_group_managers.rb new file mode 100644 index 000000000..9c6e356ff --- /dev/null +++ b/db/migrate/20150108221703_group_managers.rb @@ -0,0 +1,11 @@ +class GroupManagers < ActiveRecord::Migration + def change + create_table :group_managers do |t| + t.integer :group_id, null: false + t.integer :user_id, null: false + t.timestamps + end + + add_index :group_managers, [:group_id, :user_id], unique: true + end +end diff --git a/lib/guardian.rb b/lib/guardian.rb index f2e452335..d644e5485 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -4,6 +4,7 @@ require_dependency 'guardian/post_guardian' require_dependency 'guardian/topic_guardian' require_dependency 'guardian/user_guardian' require_dependency 'guardian/post_revision_guardian' +require_dependency 'guardian/group_guardian' # The guardian is responsible for confirming access to various site resources and operations class Guardian @@ -13,6 +14,7 @@ class Guardian include TopicGuardian include UserGuardian include PostRevisionGuardian + include GroupGuardian class AnonymousUser def blank?; true; end diff --git a/lib/guardian/group_guardian.rb b/lib/guardian/group_guardian.rb new file mode 100644 index 000000000..308d95b2b --- /dev/null +++ b/lib/guardian/group_guardian.rb @@ -0,0 +1,11 @@ +#mixin for all guardian methods dealing with group permissions +module GroupGuardian + + # Edit authority for groups means membership changes only. + # Automatic groups are not represented in the GROUP_USERS + # table and thus do not allow membership changes. + def can_edit_group?(group) + (group.managers.include?(user) || is_admin?) && !group.automatic + end + +end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index aca580b9c..3973449cb 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -84,4 +84,78 @@ describe GroupsController do expect(members.map{ |m| m['username'] }).to eq(usernames[3..4]) end end + + + describe "membership edit permission" do + it "refuses membership changes to unauthorized users" do + Guardian.any_instance.stubs(:can_edit?).with(group).returns(false) + + xhr :put, :add_members, group_id: group.name, usernames: "bob" + response.should be_forbidden + + xhr :delete, :remove_member, group_id: group.name, username: "bob" + response.should be_forbidden + end + + it "cannot add members to automatic groups" do + Guardian.any_instance.stubs(:is_admin?).returns(true) + auto_group = Fabricate(:group, name: "auto_group", automatic: true) + + xhr :put, :add_members, group_id: group.name, usernames: "bob" + response.should be_forbidden + end + end + + describe "membership edits" do + before do + @user1 = Fabricate(:user) + group.add(@user1) + group.reload + + Guardian.any_instance.stubs(:can_edit?).with(group).returns(true) + end + + it "can make incremental adds" do + user2 = Fabricate(:user) + xhr :put, :add_members, group_id: group.name, usernames: user2.username + + response.should be_success + group.reload + group.users.count.should eq(2) + end + + it "succeeds silently when adding non-existent users" do + xhr :put, :add_members, group_id: group.name, usernames: "nosuchperson" + + response.should be_success + group.reload + group.users.count.should eq(1) + end + + it "succeeds silently when adding duplicate users" do + xhr :put, :add_members, group_id: group.name, usernames: @user1.username + + response.should be_success + group.reload + group.users.should eq([@user1]) + end + + it "can make incremental deletes" do + xhr :delete, :remove_member, group_id: group.name, username: @user1.username + + response.should be_success + group.reload + group.users.count.should eq(0) + end + + it "succeeds silently when removing non-members" do + user2 = Fabricate(:user) + xhr :delete, :remove_member, group_id: group.name, username: user2.username + + response.should be_success + group.reload + group.users.count.should eq(1) + end + end + end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 1337be073..52a9a74f8 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -204,4 +204,27 @@ describe Group do expect(user.groups.map(&:name).sort).to eq ["trust_level_0"] end + context "group management" do + let(:group) {Fabricate(:group)} + + it "by default has no managers" do + group.managers.should be_empty + end + + it "multiple managers can be appointed" do + 2.times do |i| + u = Fabricate(:user) + group.appoint_manager(u) + end + expect(group.managers.count).to eq(2) + end + + it "manager has authority to edit membership" do + u = Fabricate(:user) + expect(Guardian.new(u).can_edit?(group)).to be_falsy + group.appoint_manager(u) + expect(Guardian.new(u).can_edit?(group)).to be_truthy + end + end + end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 74afa1583..96aa8286c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1029,6 +1029,22 @@ describe User do end end + context "group management" do + let!(:user) { Fabricate(:user) } + + it "by default has no managed groups" do + expect(user.managed_groups).to be_empty + end + + it "can manage multiple groups" do + 3.times do |i| + g = Fabricate(:group, name: "group_#{i}") + g.appoint_manager(user) + end + expect(user.managed_groups.count).to eq(3) + end + end + describe "should_be_redirected_to_top" do let!(:user) { Fabricate(:user) }