Adds a role setting for controlling visibility of users: all or members of visible projects (#11724).

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

app/controllers/users_controller.rb

@@ -60,19 +60,17 @@ class UsersController < ApplicationController
   end
 
   def show
+    unless @user.visible?
+      render_404
+      return
+    end
+
     # show projects based on current user visibility
     @memberships = @user.memberships.where(Project.visible_condition(User.current)).to_a
 
     events = Redmine::Activity::Fetcher.new(User.current, :author => @user).events(nil, nil, :limit => 10)
     @events_by_day = events.group_by(&:event_date)
 
-    unless User.current.admin?
-      if !@user.active? || (@user != User.current  && @memberships.empty? && events.empty?)
-        render_404
-        return
-      end
-    end
-
     respond_to do |format|
       format.html { render :layout => 'base' }
       format.api

app/controllers/watchers_controller.rb

@@ -40,8 +40,9 @@ class WatchersController < ApplicationController
     else
       user_ids << params[:user_id]
     end
-    user_ids.flatten.compact.uniq.each do |user_id|
+    users = User.active.visible.where(:id => user_ids.flatten.compact.uniq)
-      Watcher.create(:watchable => @watched, :user_id => user_id)
+    users.each do |user|
+      Watcher.create(:watchable => @watched, :user => user)
     end
     respond_to do |format|
       format.html { redirect_to_referer_or {render :text => 'Watcher added.', :layout => true}}
@@ -53,7 +54,7 @@ class WatchersController < ApplicationController
   def append
     if params[:watcher].is_a?(Hash)
       user_ids = params[:watcher][:user_ids] || [params[:watcher][:user_id]]
-      @users = User.active.where(:id => user_ids).to_a
+      @users = User.active.visible.where(:id => user_ids).to_a
     end
     if @users.blank?
       render :nothing => true
@@ -61,7 +62,7 @@ class WatchersController < ApplicationController
   end
 
   def destroy
-    @watched.set_watcher(User.find(params[:user_id]), false)
+    @watched.set_watcher(User.visible.find(params[:user_id]), false)
     respond_to do |format|
       format.html { redirect_to :back }
       format.js
@@ -115,12 +116,13 @@ class WatchersController < ApplicationController
   end
 
   def users_for_new_watcher
-    users = []
+    scope = nil
     if params[:q].blank? && @project.present?
-      users = @project.users.sorted
+      scope = @project.users
     else
-      users = User.active.sorted.like(params[:q]).limit(100)
+      scope = User.all.limit(100)
     end
+    users = scope.active.visible.sorted.like(params[:q]).to_a
     if @watched
       users -= @watched.watcher_users
     end

app/models/issue_query.rb

@@ -131,17 +131,17 @@ class IssueQuery < Query
     issue_custom_fields = []
 
     if project
-      principals += project.principals.sort
+      principals += project.principals.visible
       unless project.leaf?
         subprojects = project.descendants.visible.to_a
-        principals += Principal.member_of(subprojects)
+        principals += Principal.member_of(subprojects).visible
       end
       versions = project.shared_versions.to_a
       categories = project.issue_categories.to_a
       issue_custom_fields = project.all_issue_custom_fields
     else
       if all_projects.any?
-        principals += Principal.member_of(all_projects)
+        principals += Principal.member_of(all_projects).visible
       end
       versions = Version.visible.where(:sharing => 'system').to_a
       issue_custom_fields = IssueCustomField.where(:is_for_all => true)
@@ -185,7 +185,7 @@ class IssueQuery < Query
       :type => :list_optional, :values => assigned_to_values
     ) unless assigned_to_values.empty?
 
-    group_values = Group.givable.collect {|g| [g.name, g.id.to_s] }
+    group_values = Group.givable.visible.collect {|g| [g.name, g.id.to_s] }
     add_available_filter("member_of_group",
       :type => :list_optional, :values => group_values
     ) unless group_values.empty?

app/models/principal.rb

@@ -38,6 +38,30 @@ class Principal < ActiveRecord::Base
   # Groups and active users
   scope :active, lambda { where(:status => STATUS_ACTIVE) }
 
+  scope :visible, lambda {|*args|
+    user = args.first || User.current
+
+    if user.admin?
+      all
+    else
+      view_all_active = false
+      if user.memberships.to_a.any?
+        view_all_active = user.memberships.any? {|m| m.roles.any? {|r| r.users_visibility == 'all'}}
+      else
+        view_all_active = user.builtin_role.users_visibility == 'all'
+      end
+  
+      if view_all_active
+        active
+      else
+        # self and members of visible projects
+        active.where("#{table_name}.id = ? OR #{table_name}.id IN (SELECT user_id FROM #{Member.table_name} WHERE project_id IN (?))",
+          user.id, user.visible_project_ids
+        )
+      end
+    end
+  }
+
   scope :like, lambda {|q|
     q = q.to_s
     if q.blank?
@@ -84,6 +108,10 @@ class Principal < ActiveRecord::Base
     to_s
   end
 
+  def visible?(user=User.current)
+    Principal.visible(user).where(:id => id).first == self
+  end
+
   # Return true if the principal is a member of project
   def member_of?(project)
     projects.to_a.include?(project)

app/models/role.rb

@@ -39,6 +39,11 @@ class Role < ActiveRecord::Base
     ['own', :label_issues_visibility_own]
   ]
 
+  USERS_VISIBILITY_OPTIONS = [
+    ['all', :label_users_visibility_all],
+    ['members_of_visible_projects', :label_users_visibility_members_of_visible_projects]
+  ]
+
   scope :sorted, lambda { order(:builtin, :position) }
   scope :givable, lambda { order(:position).where(:builtin => 0) }
   scope :builtin, lambda { |*args|
@@ -67,6 +72,9 @@ class Role < ActiveRecord::Base
   validates_inclusion_of :issues_visibility,
     :in => ISSUES_VISIBILITY_OPTIONS.collect(&:first),
     :if => lambda {|role| role.respond_to?(:issues_visibility)}
+  validates_inclusion_of :users_visibility,
+    :in => USERS_VISIBILITY_OPTIONS.collect(&:first),
+    :if => lambda {|role| role.respond_to?(:users_visibility)}
 
   # Copies attributes from another role, arg can be an id or a Role
   def copy_from(arg, options={})

app/models/time_entry_query.rb

@@ -40,20 +40,20 @@ class TimeEntryQuery < Query
 
     principals = []
     if project
-      principals += project.principals.sort
+      principals += project.principals.visible.sort
       unless project.leaf?
         subprojects = project.descendants.visible.to_a
         if subprojects.any?
           add_available_filter "subproject_id",
             :type => :list_subprojects,
             :values => subprojects.collect{|s| [s.name, s.id.to_s] }
-          principals += Principal.member_of(subprojects)
+          principals += Principal.member_of(subprojects).visible
         end
       end
     else
       if all_projects.any?
         # members of visible projects
-        principals += Principal.member_of(all_projects)
+        principals += Principal.member_of(all_projects).visible
         # project filter
         project_values = []
         if User.current.logged? && User.current.memberships.any?

app/models/user.rb

@@ -148,6 +148,7 @@ class User < Principal
     @notified_projects_ids = nil
     @notified_projects_ids_changed = false
     @builtin_role = nil
+    @visible_project_ids = nil
     base_reload(*args)
   end
 
@@ -528,6 +529,11 @@ class User < Principal
     @projects_by_role = hash
   end
 
+  # Returns the ids of visible projects
+  def visible_project_ids
+    @visible_project_ids ||= Project.visible(self).pluck(:id)
+  end
+
   # Returns true if user is arg or belongs to arg
   def is_or_belongs_to?(arg)
     if arg.is_a?(User)

app/views/roles/_form.html.erb

@@ -1,18 +1,22 @@
 <%= error_messages_for 'role' %>
 
-<% unless @role.anonymous? %>
 <div class="box tabular">
-<% unless @role.builtin? %>
+  <% unless @role.builtin? %>
-<p><%= f.text_field :name, :required => true %></p>
+    <p><%= f.text_field :name, :required => true %></p>
-<p><%= f.check_box :assignable %></p>
+    <p><%= f.check_box :assignable %></p>
-<% end %>
+  <% end %>
-<p><%= f.select :issues_visibility, Role::ISSUES_VISIBILITY_OPTIONS.collect {|v| [l(v.last), v.first]} %></p>
+
-<% if @role.new_record? && @roles.any? %>
+  <% unless @role.anonymous? %>
-<p><label for="copy_workflow_from"><%= l(:label_copy_workflow_from) %></label>
+    <p><%= f.select :issues_visibility, Role::ISSUES_VISIBILITY_OPTIONS.collect {|v| [l(v.last), v.first]} %></p>
-<%= 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>
+  <% end %>
-<% end %>
+
+  <p><%= f.select :users_visibility, Role::USERS_VISIBILITY_OPTIONS.collect {|v| [l(v.last), v.first]} %></p>
+
+  <% 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>
+  <% end %>
 </div>
-<% end %>
 
 <h3><%= l(:label_permissions) %></h3>
 <div class="box tabular" id="permissions">

config/locales/en.yml

@@ -338,6 +338,7 @@ en:
   field_generate_password: Generate password
   field_must_change_passwd: Must change password at next logon
   field_default_status: Default status
+  field_users_visibility: Users visibility
 
   setting_app_title: Application title
   setting_app_subtitle: Application subtitle
@@ -920,6 +921,8 @@ en:
   label_latest_compatible_version: Latest compatible version
   label_unknown_plugin: Unknown plugin
   label_add_projects: Add projects
+  label_users_visibility_all: All active users
+  label_users_visibility_members_of_visible_projects: Members of visible projects
 
   button_login: Login
   button_submit: Submit

config/locales/fr.yml

@@ -358,6 +358,7 @@ fr:
   field_generate_password: Générer un mot de passe
   field_must_change_passwd: Doit changer de mot de passe à la prochaine connexion
   field_default_status: Statut par défaut
+  field_users_visibility: Visibilité des utilisateurs
 
   setting_app_title: Titre de l'application
   setting_app_subtitle: Sous-titre de l'application
@@ -940,6 +941,8 @@ fr:
   label_latest_compatible_version: Dernière version compatible
   label_unknown_plugin: Plugin inconnu
   label_add_projects: Ajouter des projets
+  label_users_visibility_all: Tous les utilisateurs actifs
+  label_users_visibility_members_of_visible_projects: Membres des projets visibles
 
   button_login: Connexion
   button_submit: Soumettre

db/migrate/20141109112308_add_roles_users_visibility.rb

@@ -0,0 +1,9 @@
+class AddRolesUsersVisibility < ActiveRecord::Migration
+  def self.up
+    add_column :roles, :users_visibility, :string, :limit => 30, :default => 'all', :null => false
+  end
+
+  def self.down
+    remove_column :roles, :users_visibility
+  end
+end

lib/redmine/default_data/loader.rb

@@ -42,6 +42,7 @@ module Redmine
             # Roles
             manager = Role.create! :name => l(:default_role_manager),
                                    :issues_visibility => 'all',
+                                   :users_visibility => 'all',
                                    :position => 1
             manager.permissions = manager.setable_permissions.collect {|p| p.name}
             manager.save!

test/fixtures/roles.yml

@@ -4,6 +4,7 @@ roles_001:
   id: 1
   builtin: 0
   issues_visibility: all
+  users_visibility: all
   permissions: |
     --- 
     - :add_project
@@ -67,6 +68,7 @@ roles_002:
   id: 2
   builtin: 0
   issues_visibility: default
+  users_visibility: all
   permissions: |
     --- 
     - :edit_project
@@ -114,6 +116,7 @@ roles_003:
   id: 3
   builtin: 0
   issues_visibility: default
+  users_visibility: all
   permissions: |
     --- 
     - :edit_project
@@ -155,6 +158,7 @@ roles_004:
   id: 4
   builtin: 1
   issues_visibility: default
+  users_visibility: all
   permissions: |
     --- 
     - :view_issues
@@ -184,6 +188,7 @@ roles_005:
   id: 5
   builtin: 2
   issues_visibility: default
+  users_visibility: all
   permissions: |
     --- 
     - :view_issues

test/functional/users_controller_test.rb

@@ -106,12 +106,6 @@ class UsersControllerTest < ActionController::TestCase
     assert_response 404
   end
 
-  def test_show_should_not_reveal_users_with_no_visible_activity_or_project
-    @request.session[:user_id] = nil
-    get :show, :id => 9
-    assert_response 404
-  end
-
   def test_show_inactive_by_admin
     @request.session[:user_id] = 1
     get :show, :id => 5
@@ -119,6 +113,15 @@ class UsersControllerTest < ActionController::TestCase
     assert_not_nil assigns(:user)
   end
 
+  def test_show_user_who_is_not_visible_should_return_404
+    Role.anonymous.update! :users_visibility => 'members_of_visible_projects'
+    user = User.generate!
+
+    @request.session[:user_id] = nil
+    get :show, :id => user.id
+    assert_response 404
+  end
+
   def test_show_displays_memberships_based_on_project_visibility
     @request.session[:user_id] = 1
     get :show, :id => 2

test/functional/watchers_controller_test.rb

@@ -227,6 +227,21 @@ class WatchersControllerTest < ActionController::TestCase
     assert Issue.find(2).watched_by?(user)
   end
 
+  def test_autocomplete_for_user_should_return_visible_users
+    Role.update_all :users_visibility => 'members_of_visible_projects'
+
+    hidden = User.generate!(:lastname => 'autocomplete')
+    visible = User.generate!(:lastname => 'autocomplete')
+    User.add_to_project(visible, Project.find(1))
+
+    @request.session[:user_id] = 2
+    xhr :get, :autocomplete_for_user, :q => 'autocomp', :project_id => 'ecookbook'
+    assert_response :success
+
+    assert_include visible, assigns(:users)
+    assert_not_include hidden, assigns(:users)
+  end
+
   def test_append
     @request.session[:user_id] = 2
     assert_no_difference 'Watcher.count' do

test/unit/principal_test.rb

@@ -20,7 +20,7 @@
 require File.expand_path('../../test_helper', __FILE__)
 
 class PrincipalTest < ActiveSupport::TestCase
-  fixtures :users, :projects, :members, :member_roles
+  fixtures :users, :projects, :members, :member_roles, :roles
 
   def test_active_scope_should_return_groups_and_active_users
     result = Principal.active.to_a
@@ -30,6 +30,27 @@ class PrincipalTest < ActiveSupport::TestCase
     assert_nil result.detect {|p| p.is_a?(AnonymousUser)}
   end
 
+  def test_visible_scope_for_admin_should_return_all_principals
+    admin = User.generate! {|u| u.admin = true}
+    assert_equal Principal.count, Principal.visible(admin).count
+  end
+
+  def test_visible_scope_for_user_with_members_of_visible_projects_visibility_should_return_active_principals
+    Role.non_member.update! :users_visibility => 'all'
+    user = User.generate!
+
+    expected = Principal.active
+    assert_equal expected.map(&:id).sort, Principal.visible(user).pluck(:id).sort
+  end
+
+  def test_visible_scope_for_user_with_members_of_visible_projects_visibility_should_return_members_of_visible_projects_and_self
+    Role.non_member.update! :users_visibility => 'members_of_visible_projects'
+    user = User.generate!
+
+    expected = Project.visible(user).map(&:member_principals).flatten.map(&:principal).uniq << user
+    assert_equal expected.map(&:id).sort, Principal.visible(user).pluck(:id).sort
+  end
+
   def test_member_of_scope_should_return_the_union_of_all_members
     projects = Project.find([1])
     assert_equal [3, 2], Principal.member_of(projects).sort.map(&:id)