summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJean-Philippe Lang <jp_lang@yahoo.fr>2015-05-31 07:16:23 +0000
committerJean-Philippe Lang <jp_lang@yahoo.fr>2015-05-31 07:16:23 +0000
commited9f00178c65cc4b37ad2ab56cc89b1c79c4fb8b (patch)
tree3420c99029f21a21c236c4ff8c0c532dd548401e
parent48d40a8c8884aefc2287f2030c439578057c9516 (diff)
downloadredmine-ed9f00178c65cc4b37ad2ab56cc89b1c79c4fb8b.tar.gz
redmine-ed9f00178c65cc4b37ad2ab56cc89b1c79c4fb8b.zip
Ability to limit member management to certain roles (#19707).
git-svn-id: http://svn.redmine.org/redmine/trunk@14293 e93f8b46-1217-0410-a6f0-8f06a7374b81
-rw-r--r--app/controllers/members_controller.rb18
-rw-r--r--app/models/member.rb77
-rw-r--r--app/models/member_role.rb9
-rw-r--r--app/models/role.rb4
-rw-r--r--app/models/user.rb9
-rw-r--r--app/views/members/_new_form.html.erb2
-rw-r--r--app/views/projects/settings/_members.html.erb4
-rw-r--r--app/views/roles/_form.html.erb33
-rw-r--r--db/migrate/20150528084820_add_roles_all_roles_managed.rb5
-rw-r--r--db/migrate/20150528092912_create_roles_managed_roles.rb8
-rw-r--r--db/migrate/20150528093249_add_unique_index_on_roles_managed_roles.rb5
-rw-r--r--test/functional/members_controller_test.rb83
-rw-r--r--test/unit/member_test.rb31
13 files changed, 260 insertions, 28 deletions
diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index da580fd83..5e5bee822 100644
--- a/app/controllers/members_controller.rb
+++ b/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|
diff --git a/app/models/member.rb b/app/models/member.rb
index 72a92db53..037a5a057 100644
--- a/app/models/member.rb
+++ b/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
diff --git a/app/models/member_role.rb b/app/models/member_role.rb
index afa5d2e6a..526daf9cf 100644
--- a/app/models/member_role.rb
+++ b/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
diff --git a/app/models/role.rb b/app/models/role.rb
index 43f32b726..42dddaf47 100644
--- a/app/models/role.rb
+++ b/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
diff --git a/app/models/user.rb b/app/models/user.rb
index 70610159f..9ff706178 100644
--- a/app/models/user.rb
+++ b/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)
diff --git a/app/views/members/_new_form.html.erb b/app/views/members/_new_form.html.erb
index 9140da7c4..3fe9187d1 100644
--- a/app/views/members/_new_form.html.erb
+++ b/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>
diff --git a/app/views/projects/settings/_members.html.erb b/app/views/projects/settings/_members.html.erb
index 63a4bfba0..c9a91e3b1 100644
--- a/app/views/projects/settings/_members.html.erb
+++ b/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>
diff --git a/app/views/roles/_form.html.erb b/app/views/roles/_form.html.erb
index 03cbe58d2..8feab5d92 100644
--- a/app/views/roles/_form.html.erb
+++ b/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 %>
diff --git a/db/migrate/20150528084820_add_roles_all_roles_managed.rb b/db/migrate/20150528084820_add_roles_all_roles_managed.rb
new file mode 100644
index 000000000..c4e2b9b17
--- /dev/null
+++ b/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
diff --git a/db/migrate/20150528092912_create_roles_managed_roles.rb b/db/migrate/20150528092912_create_roles_managed_roles.rb
new file mode 100644
index 000000000..94e5ee2d8
--- /dev/null
+++ b/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
diff --git a/db/migrate/20150528093249_add_unique_index_on_roles_managed_roles.rb b/db/migrate/20150528093249_add_unique_index_on_roles_managed_roles.rb
new file mode 100644
index 000000000..74dc64b38
--- /dev/null
+++ b/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
diff --git a/test/functional/members_controller_test.rb b/test/functional/members_controller_test.rb
index c1f435086..5bad28745 100644
--- a/test/functional/members_controller_test.rb
+++ b/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
diff --git a/test/unit/member_test.rb b/test/unit/member_test.rb
index 2ed65543b..221cae4f8 100644
--- a/test/unit/member_test.rb
+++ b/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