Merge pull request #3084 from jmay/group-managers

table & model changes for group managers with permission to edit members

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

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

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
+#

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
 

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

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

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

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

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

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

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