Ability to limit member management to certain roles (#19707).

git-svn-id: http://svn.redmine.org/redmine/trunk@14293 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/controllers/members_controller.rb

@@ -53,14 +53,12 @@ class MembersController < ApplicationController
   def create
     members = []
     if params[:membership]
-      if params[:membership][:user_ids]
-        attrs = params[:membership].dup
-        user_ids = attrs.delete(:user_ids)
-        user_ids.each do |user_id|
-          members << Member.new(:role_ids => params[:membership][:role_ids], :user_id => user_id)
-        end
-      else
-        members << Member.new(:role_ids => params[:membership][:role_ids], :user_id => params[:membership][:user_id])
+      user_ids = Array.wrap(params[:membership][:user_id] || params[:membership][:user_ids])
+      user_ids << nil if user_ids.empty?
+      user_ids.each do |user_id|
+        member = Member.new(:project => @project, :user_id => user_id)
+        member.set_editable_role_ids(params[:membership][:role_ids])
+        members << member
       end
       @project.members << members
     end
@@ -84,7 +82,7 @@ class MembersController < ApplicationController
 
   def update
     if params[:membership]
-      @member.role_ids = params[:membership][:role_ids]
+      @member.set_editable_role_ids(params[:membership][:role_ids])
     end
     saved = @member.save
     respond_to do |format|
@@ -101,7 +99,7 @@ class MembersController < ApplicationController
   end
 
   def destroy
-    if request.delete? && @member.deletable?
+    if @member.deletable?
       @member.destroy
     end
     respond_to do |format|

app/models/member.rb

@@ -29,6 +29,12 @@ class Member < ActiveRecord::Base
 
   before_destroy :set_issue_category_nil
 
+  alias :base_reload :reload
+  def reload(*args)
+    @managed_roles = nil
+    base_reload(*args)
+  end
+
   def role
   end
 
@@ -70,22 +76,52 @@ class Member < ActiveRecord::Base
     end
   end
 
-  def deletable?
-    member_roles.detect {|mr| mr.inherited_from}.nil?
+	# Set member role ids ignoring any change to roles that
+	# user is not allowed to manage
+  def set_editable_role_ids(ids, user=User.current)
+    ids = (ids || []).collect(&:to_i) - [0]
+    editable_role_ids = user.managed_roles(project).map(&:id)
+    untouched_role_ids = self.role_ids - editable_role_ids
+    touched_role_ids = ids & editable_role_ids
+    self.role_ids = untouched_role_ids + touched_role_ids
   end
 
-  def destroy
-    if member_roles.reload.present?
-      # destroying the last role will destroy another instance
-      # of the same Member record, #super would then trigger callbacks twice
-      member_roles.destroy_all
-      @destroyed = true
-      freeze
+	# Returns true if one of the member roles is inherited
+  def any_inherited_role?
+    member_roles.any? {|mr| mr.inherited_from}
+  end
+
+	# Returns true if the member has the role and if it's inherited
+  def has_inherited_role?(role)
+    member_roles.any? {|mr| mr.role_id == role.id && mr.inherited_from.present?}
+  end
+
+	# Returns true if the member's role is editable by user
+  def role_editable?(role, user=User.current)
+    if has_inherited_role?(role)
+      false
     else
-      super
+      user.managed_roles(project).include?(role)
     end
   end
 
+	# Returns true if the member is deletable by user
+  def deletable?(user=User.current)
+    if any_inherited_role?
+      false
+    else
+      roles & user.managed_roles(project) == roles
+    end
+  end
+
+	# Destroys the member
+  def destroy
+    member_roles.reload.each(&:destroy_without_member_removal)
+    super
+  end
+
+	# Returns true if the member is user or is a group
+	# that includes user
   def include?(user)
     if principal.is_a?(Group)
       !user.nil? && user.groups.include?(principal)
@@ -102,6 +138,27 @@ class Member < ActiveRecord::Base
     end
   end
 
+  # Returns the roles that the member is allowed to manage
+  # in the project the member belongs to
+  def managed_roles
+    @managed_roles ||= begin
+      if principal.try(:admin?)
+        Role.givable.to_a
+      else
+        members_management_roles = roles.select do |role|
+          role.has_permission?(:manage_members)
+        end
+        if members_management_roles.empty?
+          []
+        elsif members_management_roles.any?(&:all_roles_managed?)
+          Role.givable.to_a
+        else
+          members_management_roles.map(&:managed_roles).reduce(&:|)
+        end
+      end
+    end
+  end
+
   # Creates memberships for principal with the attributes
   # * project_ids : one or more project ids
   # * role_ids : ids of the roles to give to each membership

app/models/member_role.rb

@@ -36,10 +36,17 @@ class MemberRole < ActiveRecord::Base
     !inherited_from.nil?
   end
 
+  # Destroys the MemberRole without destroying its Member if it doesn't have
+  # any other roles
+  def destroy_without_member_removal
+    @member_removal = false
+    destroy
+  end
+
   private
 
   def remove_member_if_empty
-    if member.roles.empty?
+    if @member_removal != false && member.roles.empty?
       member.destroy
     end
   end

app/models/role.rb

@@ -64,6 +64,10 @@ class Role < ActiveRecord::Base
   end
   has_and_belongs_to_many :custom_fields, :join_table => "#{table_name_prefix}custom_fields_roles#{table_name_suffix}", :foreign_key => "role_id"
 
+  has_and_belongs_to_many :managed_roles, :class_name => 'Role',
+    :join_table => "#{table_name_prefix}roles_managed_roles#{table_name_suffix}",
+    :association_foreign_key => "managed_role_id"
+
   has_many :member_roles, :dependent => :destroy
   has_many :members, :through => :member_roles
   acts_as_list

app/models/user.rb

@@ -566,6 +566,15 @@ class User < Principal
     @visible_project_ids ||= Project.visible(self).pluck(:id)
   end
 
+  # Returns the roles that the user is allowed to manage for the given project
+  def managed_roles(project)
+    if admin?
+      Role.givable.to_a
+    else
+      membership(project).try(:managed_roles) || []
+    end
+  end
+
   # Returns true if user is arg or belongs to arg
   def is_or_belongs_to?(arg)
     if arg.is_a?(User)

app/views/members/_new_form.html.erb

@@ -9,7 +9,7 @@
 <fieldset class="box">
   <legend><%= l(:label_role_plural) %> <%= toggle_checkboxes_link('.roles-selection input') %></legend>
   <div class="roles-selection">
-    <% Role.givable.all.each do |role| %>
+    <% User.current.managed_roles(@project).each do |role| %>
       <label><%= check_box_tag 'membership[role_ids][]', role.id, false, :id => nil %> <%= role %></label>
     <% end %>
   </div>

app/views/projects/settings/_members.html.erb

@@ -32,9 +32,7 @@
             <%= check_box_tag('membership[role_ids][]',
                               role.id, member.roles.include?(role),
                               :id => nil,
-                              :disabled => member.member_roles.detect {
-                                 |mr| mr.role_id == role.id && !mr.inherited_from.nil?
-                                 } ) %> <%= role %>
+                              :disabled => !member.role_editable?(role)) %> <%= role %>
           </label><br />
           <% end %>
         </p>

app/views/roles/_form.html.erb

@@ -16,6 +16,28 @@
 
   <p><%= f.select :users_visibility, Role::USERS_VISIBILITY_OPTIONS.collect {|v| [l(v.last), v.first]} %></p>
 
+  <% unless @role.builtin? %>
+    <p id="manage_members_options">
+      <label>Gestion des membres</label>
+      <label class="block">
+        <%= radio_button_tag 'role[all_roles_managed]', 1, @role.all_roles_managed?, :id => 'role_all_roles_managed_on',
+              :data => {:disables => '.role_managed_role input'} %>
+        tous les rôles
+      </label>
+      <label class="block">
+        <%= radio_button_tag 'role[all_roles_managed]', 0, !@role.all_roles_managed?, :id => 'role_all_roles_managed_off',
+              :data => {:enables => '.role_managed_role input'} %>
+        ces rôles uniquement:
+      </label>
+      <% Role.givable.sorted.each do |role| %>
+        <label class="block role_managed_role" style="padding-left:2em;">
+          <%= check_box_tag 'role[managed_role_ids][]', role.id, @role.managed_roles.include?(role), :id => nil %>
+          <%= role.name %>
+        </label>
+      <% end %>
+      <%= hidden_field_tag 'role[managed_role_ids][]', '' %>
+  <% end %>
+
   <% if @role.new_record? && @roles.any? %>
     <p><label for="copy_workflow_from"><%= l(:label_copy_workflow_from) %></label>
     <%= select_tag(:copy_workflow_from, content_tag("option") + options_from_collection_for_select(@roles, :id, :name, params[:copy_workflow_from] || @copy_from.try(:id))) %></p>
@@ -29,7 +51,8 @@
     <fieldset><legend><%= mod.blank? ? l(:label_project) : l_or_humanize(mod, :prefix => 'project_module_') %></legend>
     <% perms_by_module[mod].each do |permission| %>
         <label class="floating">
-        <%= check_box_tag 'role[permissions][]', permission.name, (@role.permissions.include? permission.name), :id => nil %>
+        <%= check_box_tag 'role[permissions][]', permission.name, (@role.permissions.include? permission.name),
+              :id => "role_permissions_#{permission.name}" %>
         <%= l_or_humanize(permission.name, :prefix => 'permission_') %>
         </label>
     <% end %>
@@ -38,3 +61,11 @@
 <br /><%= check_all_links 'permissions' %>
 <%= hidden_field_tag 'role[permissions][]', '' %>
 </div>
+
+<%= javascript_tag do %>
+$(document).ready(function(){
+  $("#role_permissions_manage_members").change(function(){
+    $("#manage_members_options").toggle($(this).is(":checked"));
+  }).change();
+});
+<% end %>

db/migrate/20150528084820_add_roles_all_roles_managed.rb

@@ -0,0 +1,5 @@
+class AddRolesAllRolesManaged < ActiveRecord::Migration
+  def change
+    add_column :roles, :all_roles_managed, :boolean, :default => true, :null => false
+  end
+end

db/migrate/20150528092912_create_roles_managed_roles.rb

@@ -0,0 +1,8 @@
+class CreateRolesManagedRoles < ActiveRecord::Migration
+  def change
+    create_table :roles_managed_roles, :id => false do |t|
+      t.integer :role_id, :null => false
+      t.integer :managed_role_id, :null => false
+    end
+  end
+end

db/migrate/20150528093249_add_unique_index_on_roles_managed_roles.rb

@@ -0,0 +1,5 @@
+class AddUniqueIndexOnRolesManagedRoles < ActiveRecord::Migration
+  def change
+    add_index :roles_managed_roles, [:role_id, :managed_role_id], :unique => true
+  end
+end

test/functional/members_controller_test.rb

@@ -30,6 +30,20 @@ class MembersControllerTest < ActionController::TestCase
     assert_response :success
   end
 
+  def test_new_should_propose_managed_roles_only
+    role = Role.find(1)
+    role.update! :all_roles_managed => false
+    role.managed_roles = Role.where(:id => [2, 3]).to_a
+
+    get :new, :project_id => 1
+    assert_response :success
+    assert_select 'div.roles-selection' do
+      assert_select 'label', :text => 'Manager', :count => 0
+      assert_select 'label', :text => 'Developer'
+      assert_select 'label', :text => 'Reporter'
+    end
+  end
+
   def test_xhr_new
     xhr :get, :new, :project_id => 1
     assert_response :success
@@ -52,6 +66,29 @@ class MembersControllerTest < ActionController::TestCase
     assert User.find(7).member_of?(Project.find(1))
   end
 
+  def test_create_should_ignore_unmanaged_roles
+    role = Role.find(1)
+    role.update! :all_roles_managed => false
+    role.managed_roles = Role.where(:id => [2, 3]).to_a
+
+    assert_difference 'Member.count' do
+      post :create, :project_id => 1, :membership => {:role_ids => [1, 2], :user_id => 7}
+    end
+    member = Member.order(:id => :desc).first
+    assert_equal [2], member.role_ids
+  end
+
+  def test_create_should_be_allowed_for_admin_without_role
+    User.find(1).members.delete_all
+    @request.session[:user_id] = 1
+
+    assert_difference 'Member.count' do
+      post :create, :project_id => 1, :membership => {:role_ids => [1, 2], :user_id => 7}
+    end
+    member = Member.order(:id => :desc).first
+    assert_equal [1, 2], member.role_ids
+  end
+
   def test_xhr_create
     assert_difference 'Member.count', 3 do
       xhr :post, :create, :project_id => 1, :membership => {:role_ids => [1], :user_ids => [7, 8, 9]}
@@ -75,14 +112,34 @@ class MembersControllerTest < ActionController::TestCase
     assert_match /alert/, response.body, "Alert message not sent"
   end
 
-  def test_edit
+  def test_update
     assert_no_difference 'Member.count' do
       put :update, :id => 2, :membership => {:role_ids => [1], :user_id => 3}
     end
     assert_redirected_to '/projects/ecookbook/settings/members'
   end
 
-  def test_xhr_edit
+  def test_update_should_not_add_unmanaged_roles
+    role = Role.find(1)
+    role.update! :all_roles_managed => false
+    role.managed_roles = Role.where(:id => [2, 3]).to_a
+    member = Member.create!(:user => User.find(9), :role_ids => [3], :project_id => 1)
+
+    put :update, :id => member.id, :membership => {:role_ids => [1, 2, 3]}
+    assert_equal [2, 3], member.reload.role_ids.sort
+  end
+
+  def test_update_should_not_remove_unmanaged_roles
+    role = Role.find(1)
+    role.update! :all_roles_managed => false
+    role.managed_roles = Role.where(:id => [2, 3]).to_a
+    member = Member.create!(:user => User.find(9), :role_ids => [1, 3], :project_id => 1)
+
+    put :update, :id => member.id, :membership => {:role_ids => [2]}
+    assert_equal [1, 2], member.reload.role_ids.sort
+  end
+
+  def test_xhr_update
     assert_no_difference 'Member.count' do
       xhr :put, :update, :id => 2, :membership => {:role_ids => [1], :user_id => 3}
       assert_response :success
@@ -103,6 +160,28 @@ class MembersControllerTest < ActionController::TestCase
     assert !User.find(3).member_of?(Project.find(1))
   end
 
+  def test_destroy_should_fail_with_unmanaged_roles
+    role = Role.find(1)
+    role.update! :all_roles_managed => false
+    role.managed_roles = Role.where(:id => [2, 3]).to_a
+    member = Member.create!(:user => User.find(9), :role_ids => [1, 3], :project_id => 1)
+
+    assert_no_difference 'Member.count' do
+      delete :destroy, :id => member.id
+    end
+  end
+
+  def test_destroy_should_succeed_with_managed_roles_only
+    role = Role.find(1)
+    role.update! :all_roles_managed => false
+    role.managed_roles = Role.where(:id => [2, 3]).to_a
+    member = Member.create!(:user => User.find(9), :role_ids => [3], :project_id => 1)
+
+    assert_difference 'Member.count', -1 do
+      delete :destroy, :id => member.id
+    end
+  end
+
   def test_xhr_destroy
     assert_difference 'Member.count', -1 do
       xhr :delete, :destroy, :id => 2

test/unit/member_test.rb

@@ -159,4 +159,35 @@ class MemberTest < ActiveSupport::TestCase
     assert_equal -1, a <=> b
     assert_equal 1,  b <=> a
   end
+
+  def test_managed_roles_should_return_all_roles_for_role_with_all_roles_managed
+    member = Member.new
+    member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => true)
+    assert_equal Role.givable.all, member.managed_roles
+  end
+
+  def test_managed_roles_should_return_all_roles_for_admins
+    member = Member.new(:user => User.find(1))
+    member.roles << Role.generate!
+    assert_equal Role.givable.all, member.managed_roles
+  end
+
+  def test_managed_roles_should_return_limited_roles_for_role_without_all_roles_managed
+    member = Member.new
+    member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => false, :managed_role_ids => [2, 3])
+    assert_equal [2, 3], member.managed_roles.map(&:id).sort
+  end
+
+  def test_managed_roles_should_cumulated_managed_roles
+    member = Member.new
+    member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => false, :managed_role_ids => [3])
+    member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => false, :managed_role_ids => [2])
+    assert_equal [2, 3], member.managed_roles.map(&:id).sort
+  end
+
+  def test_managed_roles_should_return_no_roles_for_role_without_permission
+    member = Member.new
+    member.roles << Role.generate!(:all_roles_managed => true)
+    assert_equal [], member.managed_roles
+  end
 end