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) }