From aa0d01b3d9f5ae5634eda73e1becd75cc4668f3e Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Mon, 11 Apr 2011 17:53:15 +0000 Subject: [PATCH] Adds an issues visibility level on roles (#7412). It can be set so that users only see their own issues (created or assigned). git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@5416 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/application_controller.rb | 8 +++- app/controllers/issues_controller.rb | 8 +++- app/models/issue.rb | 22 ++++++++- app/models/project.rb | 7 +++ app/models/role.rb | 10 +++- app/models/user.rb | 23 +++++---- app/views/roles/_form.rhtml | 5 +- config/locales/en.yml | 3 ++ config/locales/fr.yml | 3 ++ ...10408103312_add_roles_issues_visibility.rb | 9 ++++ test/fixtures/roles.yml | 5 ++ test/unit/issue_test.rb | 48 +++++++++++++++++-- 12 files changed, 132 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20110408103312_add_roles_issues_visibility.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 803eb5f2e..dbdeb9dfb 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,5 +1,5 @@ -# redMine - project management software -# Copyright (C) 2006-2007 Jean-Philippe Lang +# Redmine - project management software +# Copyright (C) 2006-2011 Jean-Philippe Lang # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License @@ -221,6 +221,10 @@ class ApplicationController < ActionController::Base def find_issues @issues = Issue.find_all_by_id(params[:id] || params[:ids]) raise ActiveRecord::RecordNotFound if @issues.empty? + if @issues.detect {|issue| !issue.visible?} + deny_access + return + end @projects = @issues.collect(&:project).compact.uniq @project = @projects.first if @projects.size == 1 rescue ActiveRecord::RecordNotFound diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index fdce296c4..4acd728ba 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -1,5 +1,5 @@ # Redmine - project management software -# Copyright (C) 2006-2008 Jean-Philippe Lang +# Copyright (C) 2006-2011 Jean-Philippe Lang # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License @@ -251,7 +251,13 @@ class IssuesController < ApplicationController private def find_issue + # Issue.visible.find(...) can not be used to redirect user to the login form + # if the issue actually exists but requires authentication @issue = Issue.find(params[:id], :include => [:project, :tracker, :status, :author, :priority, :category]) + unless @issue.visible? + deny_access + return + end @project = @issue.project rescue ActiveRecord::RecordNotFound render_404 diff --git a/app/models/issue.rb b/app/models/issue.rb index aca68b1ae..cbfad6a57 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -88,12 +88,30 @@ class Issue < ActiveRecord::Base # Returns a SQL conditions string used to find all issues visible by the specified user def self.visible_condition(user, options={}) - Project.allowed_to_condition(user, :view_issues, options) + Project.allowed_to_condition(user, :view_issues, options) do |role, user| + case role.issues_visibility + when 'default' + nil + when 'own' + "(#{table_name}.author_id = #{user.id} OR #{table_name}.assigned_to_id = #{user.id})" + else + '1=0' + end + end end # Returns true if usr or current user is allowed to view the issue def visible?(usr=nil) - (usr || User.current).allowed_to?(:view_issues, self.project) + (usr || User.current).allowed_to?(:view_issues, self.project) do |role, user| + case role.issues_visibility + when 'default' + true + when 'own' + self.author == user || self.assigned_to == user + else + false + end + end end def after_initialize diff --git a/app/models/project.rb b/app/models/project.rb index 4724b7465..6d1d8eae6 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -174,6 +174,13 @@ class Project < ActiveRecord::Base if statement_by_role.empty? "1=0" else + if block_given? + statement_by_role.each do |role, statement| + if s = yield(role, user) + statement_by_role[role] = "(#{statement} AND (#{s}))" + end + end + end "((#{base_statement}) AND (#{statement_by_role.values.join(' OR ')}))" end end diff --git a/app/models/role.rb b/app/models/role.rb index e0b18434e..38edd360d 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -19,6 +19,11 @@ class Role < ActiveRecord::Base # Built-in roles BUILTIN_NON_MEMBER = 1 BUILTIN_ANONYMOUS = 2 + + ISSUES_VISIBILITY_OPTIONS = [ + ['default', :label_issues_visibility_all], + ['own', :label_issues_visibility_own] + ] named_scope :givable, { :conditions => "builtin = 0", :order => 'position' } named_scope :builtin, lambda { |*args| @@ -43,7 +48,10 @@ class Role < ActiveRecord::Base validates_presence_of :name validates_uniqueness_of :name validates_length_of :name, :maximum => 30 - + validates_inclusion_of :issues_visibility, + :in => ISSUES_VISIBILITY_OPTIONS.collect(&:first), + :if => lambda {|role| role.respond_to?(:issues_visibility)} + def permissions read_attribute(:permissions) || [] end diff --git a/app/models/user.rb b/app/models/user.rb index 1018c33e0..c06a907fe 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -394,10 +394,10 @@ class User < Principal # * a permission Symbol (eg. :edit_project) # Context can be: # * a project : returns true if user is allowed to do the specified action on this project - # * a group of projects : returns true if user is allowed on every project + # * an array of projects : returns true if user is allowed on every project # * nil with options[:global] set : check if user has at least one role allowed for this action, # or falls back to Non Member / Anonymous permissions depending if the user is logged - def allowed_to?(action, context, options={}) + def allowed_to?(action, context, options={}, &block) if context && context.is_a?(Project) # No action allowed on archived projects return false unless context.active? @@ -408,12 +408,15 @@ class User < Principal roles = roles_for_project(context) return false unless roles - roles.detect {|role| (context.is_public? || role.member?) && role.allowed_to?(action)} - + roles.detect {|role| + (context.is_public? || role.member?) && + role.allowed_to?(action) && + (block_given? ? yield(role, self) : true) + } elsif context && context.is_a?(Array) # Authorize if user is authorized on every element of the array context.map do |project| - allowed_to?(action,project,options) + allowed_to?(action, project, options, &block) end.inject do |memo,allowed| memo && allowed end @@ -423,7 +426,11 @@ class User < Principal # authorize if user has at least one role that has this permission roles = memberships.collect {|m| m.roles}.flatten.uniq - roles.detect {|r| r.allowed_to?(action)} || (self.logged? ? Role.non_member.allowed_to?(action) : Role.anonymous.allowed_to?(action)) + roles << (self.logged? ? Role.non_member : Role.anonymous) + roles.detect {|role| + role.allowed_to?(action) && + (block_given? ? yield(role, self) : true) + } else false end @@ -431,8 +438,8 @@ class User < Principal # Is the user allowed to do the specified action on any project? # See allowed_to? for the actions and valid options. - def allowed_to_globally?(action, options) - allowed_to?(action, nil, options.reverse_merge(:global => true)) + def allowed_to_globally?(action, options, &block) + allowed_to?(action, nil, options.reverse_merge(:global => true), &block) end safe_attributes 'login', diff --git a/app/views/roles/_form.rhtml b/app/views/roles/_form.rhtml index a167907e8..232a72582 100644 --- a/app/views/roles/_form.rhtml +++ b/app/views/roles/_form.rhtml @@ -1,15 +1,16 @@ <%= error_messages_for 'role' %> -<% unless @role.builtin? %>
+<% unless @role.builtin? %>

<%= f.text_field :name, :required => true %>

<%= f.check_box :assignable %>

+<% end %> +

<%= f.select :issues_visibility, Role::ISSUES_VISIBILITY_OPTIONS.collect {|v| [l(v.last), v.first]} %>

<% if @role.new_record? && @roles.any? %>

<%= select_tag(:copy_workflow_from, content_tag("option") + options_from_collection_for_select(@roles, :id, :name)) %>

<% end %>
-<% end %>

<%= l(:label_permissions) %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index 29129c498..307984a41 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -304,6 +304,7 @@ en: field_text: Text field field_visible: Visible field_warn_on_leaving_unsaved: "Warn me when leaving a page with unsaved text" + field_issues_visibility: Issues visibility setting_app_title: Application title setting_app_subtitle: Application subtitle @@ -804,6 +805,8 @@ en: label_user_search: "Search for user:" label_additional_workflow_transitions_for_author: Additional transitions allowed when the user is the author label_additional_workflow_transitions_for_assignee: Additional transitions allowed when the user is the assignee + label_issues_visibility_all: All issues + label_issues_visibility_own: Issues created by or assigned to the user button_login: Login button_submit: Submit diff --git a/config/locales/fr.yml b/config/locales/fr.yml index f12832ef3..0b777a72a 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -308,6 +308,7 @@ fr: field_parent_issue: Tâche parente field_visible: Visible field_warn_on_leaving_unsaved: "M'avertir lorsque je quitte une page contenant du texte non sauvegardé" + field_issues_visibility: Visibilité des demandes setting_app_title: Titre de l'application setting_app_subtitle: Sous-titre de l'application @@ -791,6 +792,8 @@ fr: label_user_search: "Rechercher un utilisateur :" label_additional_workflow_transitions_for_author: Autorisations supplémentaires lorsque l'utilisateur a créé la demande label_additional_workflow_transitions_for_assignee: Autorisations supplémentaires lorsque la demande est assignée à l'utilisateur + label_issues_visibility_all: Toutes les demandes + label_issues_visibility_own: Demandes créées par ou assignées à l'utilisateur button_login: Connexion button_submit: Soumettre diff --git a/db/migrate/20110408103312_add_roles_issues_visibility.rb b/db/migrate/20110408103312_add_roles_issues_visibility.rb new file mode 100644 index 000000000..1e6b29a64 --- /dev/null +++ b/db/migrate/20110408103312_add_roles_issues_visibility.rb @@ -0,0 +1,9 @@ +class AddRolesIssuesVisibility < ActiveRecord::Migration + def self.up + add_column :roles, :issues_visibility, :string, :limit => 30, :default => 'default', :null => false + end + + def self.down + remove_column :roles, :issues_visibility + end +end diff --git a/test/fixtures/roles.yml b/test/fixtures/roles.yml index 979cc3211..7491f2f07 100644 --- a/test/fixtures/roles.yml +++ b/test/fixtures/roles.yml @@ -3,6 +3,7 @@ roles_001: name: Manager id: 1 builtin: 0 + issues_visibility: default permissions: | --- - :add_project @@ -58,6 +59,7 @@ roles_002: name: Developer id: 2 builtin: 0 + issues_visibility: default permissions: | --- - :edit_project @@ -102,6 +104,7 @@ roles_003: name: Reporter id: 3 builtin: 0 + issues_visibility: default permissions: | --- - :edit_project @@ -140,6 +143,7 @@ roles_004: name: Non member id: 4 builtin: 1 + issues_visibility: default permissions: | --- - :view_issues @@ -170,6 +174,7 @@ roles_005: name: Anonymous id: 5 builtin: 2 + issues_visibility: default permissions: | --- - :view_issues diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb index d12eb2205..104933089 100644 --- a/test/unit/issue_test.rb +++ b/test/unit/issue_test.rb @@ -65,35 +65,76 @@ class IssueTest < ActiveSupport::TestCase assert_equal 'PostgreSQL', issue.custom_value_for(field).value end + def assert_visibility_match(user, issues) + assert_equal issues.collect(&:id).sort, Issue.all.select {|issue| issue.visible?(user)}.collect(&:id).sort + end + def test_visible_scope_for_anonymous # Anonymous user should see issues of public projects only issues = Issue.visible(User.anonymous).all assert issues.any? assert_nil issues.detect {|issue| !issue.project.is_public?} + assert_visibility_match User.anonymous, issues + end + + def test_visible_scope_for_anonymous_with_own_issues_visibility + Role.anonymous.update_attribute :issues_visibility, 'own' + Issue.create!(:project_id => 1, :tracker_id => 1, :author_id => User.anonymous.id, :subject => 'Issue by anonymous') + + issues = Issue.visible(User.anonymous).all + assert issues.any? + assert_nil issues.detect {|issue| issue.author != User.anonymous} + assert_visibility_match User.anonymous, issues + end + + def test_visible_scope_for_anonymous_without_view_issues_permissions # Anonymous user should not see issues without permission Role.anonymous.remove_permission!(:view_issues) issues = Issue.visible(User.anonymous).all assert issues.empty? + assert_visibility_match User.anonymous, issues end - def test_visible_scope_for_user + def test_visible_scope_for_non_member user = User.find(9) assert user.projects.empty? # Non member user should see issues of public projects only issues = Issue.visible(user).all assert issues.any? assert_nil issues.detect {|issue| !issue.project.is_public?} + assert_visibility_match user, issues + end + + def test_visible_scope_for_non_member_with_own_issues_visibility + Role.non_member.update_attribute :issues_visibility, 'own' + Issue.create!(:project_id => 1, :tracker_id => 1, :author_id => 9, :subject => 'Issue by non member') + user = User.find(9) + + issues = Issue.visible(user).all + assert issues.any? + assert_nil issues.detect {|issue| issue.author != user} + assert_visibility_match user, issues + end + + def test_visible_scope_for_non_member_without_view_issues_permissions # Non member user should not see issues without permission Role.non_member.remove_permission!(:view_issues) - user.reload + user = User.find(9) + assert user.projects.empty? issues = Issue.visible(user).all assert issues.empty? + assert_visibility_match user, issues + end + + def test_visible_scope_for_member + user = User.find(9) # User should see issues of projects for which he has view_issues permissions only + Role.non_member.remove_permission!(:view_issues) Member.create!(:principal => user, :project_id => 2, :role_ids => [1]) - user.reload issues = Issue.visible(user).all assert issues.any? assert_nil issues.detect {|issue| issue.project_id != 2} + assert_visibility_match user, issues end def test_visible_scope_for_admin @@ -104,6 +145,7 @@ class IssueTest < ActiveSupport::TestCase assert issues.any? # Admin should see issues on private projects that he does not belong to assert issues.detect {|issue| !issue.project.is_public?} + assert_visibility_match user, issues end def test_visible_scope_with_project -- 2.39.5