TheDude
/
openproject
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

employ sql based user allowed scope for acts as watchable

Browse Source
pull/4778/head
Jens Ulferts 8 years ago
parent 1ddc142fd3
commit ff3ecc9e59
10 changed files with 254 additions and 249 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 2
      app/models/board.rb
  2. 6
      app/models/user.rb
  3. 86
      app/models/user/authorization.rb
  4. 2
      app/models/wiki.rb
  5. 72
      lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb
  6. 86
      spec/models/user/authorization_spec.rb
  7. 76
      spec/models/users/allowed_scope_spec.rb
  8. 4
      spec/models/work_package/work_package_acts_as_watchable_spec.rb
  9. 163
      spec/support/shared/acts_as_watchable.rb
  10. 6
      spec_legacy/unit/watcher_spec.rb

2
app/models/board.rb
Unescape View File

@ -38,7 +38,7 @@ class Board < ActiveRecord::Base
}, dependent: :destroy
belongs_to :last_message, class_name: 'Message', foreign_key: :last_message_id
acts_as_list scope: :project_id
acts_as_watchable
acts_as_watchable permission: :view_messages
validates_presence_of :name, :description
validates_length_of :name, maximum: 30

6
app/models/user.rb
Unescape View File

@ -30,8 +30,6 @@
require 'digest/sha1'
class User < Principal
include User::Authorization
USER_FORMATS_STRUCTURE = {
firstname_lastname: [:firstname, :lastname],
firstname: [:firstname],
@ -595,6 +593,10 @@ class User < Principal
Authorization.users(action, project)
end
def self.allowed_members(action, project)
Authorization.users(action, project).where.not(members: { id: nil })
end
def allowed_to?(action, context, options = {})
authorization_service.call(action, context, options).result
end

86
app/models/user/authorization.rb
Unescape View File

@ -1,86 +0,0 @@
#-- encoding: UTF-8
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See doc/COPYRIGHT.rdoc for more details.
#++
class User
module Authorization
def self.included(base)
base.extend(ClassMethods)
base.private_class_method :eager_load_for_project_authorization
base.private_class_method :reset_associations_eager_loaded_for_project_authorization
end
module ClassMethods
def authorize_within(project, &block)
base_scope = current_scope || all
auth_scope = eager_load_for_project_authorization(base_scope, project)
returned_users = block.call(auth_scope)
unless returned_users.respond_to?(:each_with_index) && returned_users.all? { |e| e.is_a?(User) }
raise ArgumentError, 'Expect a collection of users to be returned by the block'
end
reset_associations_eager_loaded_for_project_authorization(returned_users, project)
returned_users
end
def eager_load_for_project_authorization(base_scope, project)
registered_allowance_evaluators.inject(base_scope) do |scope, evaluator|
evaluator_scope = evaluator.eager_load_for_project_authorization(project)
if evaluator_scope.nil?
scope
else
scope.merge(evaluator_scope)
end
end
end
# Prevent harmful side effects by the limited loading
# (e.g. WHERE members.project_id = 1) of associations for the project
# authorization.
def reset_associations_eager_loaded_for_project_authorization(users, project)
auth_scope = eager_load_for_project_authorization(all, project)
to_clear = reflect_on_all_associations.map(&:name) &
auth_scope.eager_load_values.map(&:keys).flatten.uniq
users.each do |user|
# For reasons I don't understand #reset does not clear
# the loaded flag on the association. Documentation says it should.
user.association_cache.except!(*to_clear)
end
users
end
end
end
end

2
app/models/wiki.rb
Unescape View File

@ -37,7 +37,7 @@ class Wiki < ActiveRecord::Base
}, class_name: 'MenuItems::WikiMenuItem', dependent: :delete_all, foreign_key: 'navigatable_id'
has_many :redirects, class_name: 'WikiRedirect', dependent: :delete_all
acts_as_watchable
acts_as_watchable permission: :view_wiki_pages
accepts_nested_attributes_for :wiki_menu_items,
allow_destroy: true,

72
lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb
Unescape View File

@ -37,21 +37,23 @@ module Redmine
module ClassMethods
# Marks an ActiveRecord::Model as watchable
# A watchable model has association with users (watchers) who wish to be informed of changes on it.
# A watchable model has association with users (watchers) who wish to
# be informed of changes on it.
#
# This also creates the routes necessary for watching/unwatching by adding the model's name to routes. This
# e.g leads to the following routes when marking issues as watchable:
# This also creates the routes necessary for watching/unwatching by
# adding the model's name to routes. This e.g leads to the following
# routes when marking issues as watchable:
# POST: issues/1/watch
# DELETE: issues/1/unwatch
# GET/POST: issues/1/watchers/new
# DELETE: issues/1/watchers/1
# Use the :route_prefix option to change the model prefix, e.g. from issues to tickets
#
# params:
# options:
# route_prefix: overrides the route calculation which would normally use the models name.
# permission: overrides the permission used to determine whether a user
# is allowed to watch
def acts_as_watchable(_options = {})
def acts_as_watchable(options = {})
return if included_modules.include?(Redmine::Acts::Watchable::InstanceMethods)
class_eval do
has_many :watchers, as: :watchable, dependent: :delete_all, validate: false
@ -61,6 +63,10 @@ module Redmine
includes(:watchers)
.where(watchers: { user_id: user_id })
}
class_attribute :acts_as_watchable_options
self.acts_as_watchable_options = options
end
send :include, Redmine::Acts::Watchable::InstanceMethods
alias_method_chain :watcher_user_ids=, :uniq_ids
@ -72,43 +78,30 @@ module Redmine
base.extend ClassMethods
end
def watching_permitted_to_all_users_message
"Let all users watch me. If this is unintended, implement :visible? on #{self.class.name}"
end
def possible_watcher?(user)
if respond_to?(:visible?)
visible?(user)
else
warn watching_permitted_to_all_users_message
end
user.allowed_to?(self.class.acts_as_watchable_permission, project)
end
# Returns all users that could potentially be watchers.
# This includes those already added as watchers.
#
# Admins are excluded at least for non public projects
# because while they have the right to be added as watchers having
# them pop up in every project would be weird.
def possible_watcher_users
# TODO: There might be addable users which are not in the current
# project. But its hard (performance wise) to find them
# correctly. So we only search for users in the project scope.
# This implementation is so wrong, it makes me sad.
watchers = project.users
watchers = if respond_to?(:visible?)
# Intentionally splitting this up into two requests as
# doing it in one will be way to slow in scenarios where
# the project's users have a lot of memberships.
possible_watcher_ids = watchers.pluck(:id)
User.where(id: possible_watcher_ids)
.authorize_within(project) do |scope|
scope.select { |user| visible?(user) }
end
end
watchers
users = User
.not_builtin
if project.is_public?
users.allowed(self.class.acts_as_watchable_permission, project)
else
users.allowed_members(self.class.acts_as_watchable_permission, project)
end
end
# Returns an array of users that are proposed as watchers
def addable_watcher_users
possible_watcher_users - watcher_users
possible_watcher_users.where.not(id: watcher_users.pluck(:id))
end
# Adds user as a watcher
@ -147,11 +140,14 @@ module Redmine
# Returns an array of watchers
def watcher_recipients
notified = watcher_users.active.where(['mail_notification != ?', 'none'])
notified.select { |user| possible_watcher?(user) }
possible_watcher_users & watcher_users.active.where.not(mail_notification: 'none')
end
module ClassMethods; end
module ClassMethods
def acts_as_watchable_permission
acts_as_watchable_options[:permission] || "view_#{name.underscore.pluralize}".to_sym
end
end
end
end
end

86
spec/models/user/authorization_spec.rb
Unescape View File

@ -1,86 +0,0 @@
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See doc/COPYRIGHT.rdoc for more details.
#++
require 'spec_helper'
describe User::Authorization, type: :model do
let(:project) { FactoryGirl.create(:project) }
let(:role) { FactoryGirl.create(:role) }
let(:user1) do
FactoryGirl.create(:user,
member_in_project: project,
member_through_role: role)
end
let(:user2) do
FactoryGirl.create(:user,
member_in_project: project,
member_through_role: role)
end
let(:other_project) { FactoryGirl.create(:project) }
let(:other_user) do
FactoryGirl.create(:user,
member_in_project: other_project,
member_through_role: role)
end
let(:users) do
[user1, user2]
end
describe '.authorize_within' do
before do
users
other_user
end
it 'takes a block that gets all the users that are members of the project' do
User.authorize_within(project) do |scope|
expect(scope).to match_array users
scope
end
end
it 'returns the users that are returned by the block' do
returned_users = User.authorize_within(project) { |_|
[users.first]
}
expect(returned_users).to match_array [users.first]
end
it 'returns users without the associations being preloaded' do
returned_users = User.authorize_within(project) { |scope| scope }
expect((returned_users.map { |u| u.association_cache.keys }).flatten).to be_empty
end
it 'throws an error unless an array of users is returned' do
expect { User.authorize_within(project) { [nil] } }.to raise_error(ArgumentError)
end
end
end

76
spec/models/users/allowed_scope_spec.rb
Unescape View File

@ -331,4 +331,80 @@ describe User, 'allowed scope' do
expect(User.allowed(action, project)).to eq []
end
end
context 'w/ only asking for members
w/o the project being public
w/o the user being member in the project
w/ the user being admin' do
before do
user.update_attribute(:admin, true)
end
it 'should return the user' do
expect(User.allowed_members(action, project)).to be_empty
end
end
context 'w/ only asking for members
w/o the project being public
w/ the user being member in the project
w/ the role having the necessary permission' do
before do
role.permissions << action
role.save!
member.save!
end
it 'should return the user' do
expect(User.allowed_members(action, project)).to match_array [user]
end
end
context 'w/ only asking for members
w/o the project being public
w/o the user being member in the project
w/ the user being admin' do
before do
user.update_attribute(:admin, true)
end
it 'should return the user' do
expect(User.allowed_members(action, project)).to be_empty
end
end
context 'w/ only asking for members
w/ the project being public
w/ the user being member in the project
w/ the role having the necessary permission' do
before do
project.update_attribute(:is_public, true)
role.permissions << action
role.save!
member.save!
end
it 'should return the user' do
expect(User.allowed_members(action, project)).to match_array [user]
end
end
context 'w/ only asking for members
w/ the project being public
w/o the user being member in the project
w/ the role having the necessary permission' do
before do
project.update_attribute(:is_public, true)
role.permissions << action
role.save!
end
it 'should return the user' do
expect(User.allowed_members(action, project)).to be_empty
end
end
end

4
spec/models/work_package/work_package_acts_as_watchable_spec.rb
Unescape View File

@ -36,10 +36,6 @@ describe WorkPackage, type: :model do
FactoryGirl.create(:work_package,
project: project)
}
let(:role) { FactoryGirl.create(:role, permissions: [:view_work_packages]) }
let(:non_member_user) { FactoryGirl.create(:user) }
let(:project_member) { FactoryGirl.create(:user, member_in_project: project, member_through_role: role) }
it_behaves_like 'acts_as_watchable included' do
let(:model_instance) { FactoryGirl.create(:work_package) }

163
spec/support/shared/acts_as_watchable.rb
Unescape View File

@ -40,7 +40,11 @@ MESSAGE
end
end
let(:watcher_role) { FactoryGirl.create(:role, permissions: [watch_permission]) }
let(:watcher_role) {
permissions = is_public_permission ? [] : [watch_permission]
FactoryGirl.create(:role, permissions: permissions)
}
let(:non_watcher_role) { FactoryGirl.create(:role, permissions: []) }
let(:non_member_user) { FactoryGirl.create(:user) }
let(:user_with_permission) do
@ -72,43 +76,68 @@ MESSAGE
Redmine::AccessControl.public_permissions.map(&:name).include?(watch_permission)
end
shared_context 'non member role has the permission to watch' do
let(:non_member_role) {
unless is_public_permission
Role.non_member.add_permission! watch_permission
end
Role.non_member
}
before do
unless is_public_permission
non_member_role.add_permission! watch_permission
end
end
end
shared_context 'anonymous role has the permission to watch' do
let(:anonymous_role) {
permissions = is_public_permission ? [] : [watch_permission]
FactoryGirl.build :anonymous_role, permissions: permissions
}
before do
anonymous_role.save!
end
end
describe '#possible_watcher_users' do
subject { model_instance.possible_watcher_users }
before do
# in case the model_instance creates users, we do not want them
# to mess with our expected users
model_instance
User.destroy_all
Role.non_member
Role.anonymous
admin.save!
anonymous_user.save!
user_with_permission.save!
user_wo_permission.save!
end
shared_context 'non member role has the permission to watch' do
let(:non_member_role) { Role.non_member }
include_context 'non member role has the permission to watch'
include_context 'anonymous role has the permission to watch'
context 'when it is a public project' do
before do
non_member_role.add_permission! watch_permission
project.update_attributes is_public: true
model_instance.reload
end
end
shared_context 'anonymous role has the permission to watch' do
let(:anonymous_role) { FactoryGirl.build :anonymous_role, permissions: [watch_permission] }
before do
anonymous_role.save!
end
end
it 'contains all allowed to view' do
expected_users = [user_with_permission,
non_member_user,
admin]
# While using share context here does not make sense for now as it
# is only used once, the intend of acts as watchable is to also
# include non members as watchers. Then the permission will be
# a variable to also test for.
include_context 'non member role has the permission to watch'
include_context 'anonymous role has the permission to watch'
expected_users << user_wo_permission if is_public_permission
context 'when it is a public project' do
it 'contains members allowed to view' do
expect(model_instance.possible_watcher_users)
.to match_array([user_with_permission])
.to match_array(expected_users)
end
end
@ -133,7 +162,9 @@ MESSAGE
subject { model_instance.watcher_recipients }
it { is_expected.to match_array([watching_user]) }
it 'has the watching user' do
is_expected.to match_array([watching_user])
end
context 'when the permission to watch has been removed' do
before do
@ -146,7 +177,9 @@ MESSAGE
model_instance.reload
end
it { is_expected.to match_array([]) }
it 'is empty' do
is_expected.to match_array([])
end
end
end
@ -158,14 +191,16 @@ MESSAGE
subject { model_instance.watched_by?(watching_user) }
it { is_expected.to be_truthy }
it 'is truthy' do
is_expected.to be_truthy
end
context 'when the permission to view work packages has been removed' do
# an existing watcher shouldn't be removed
before do
if is_public_permission
skip "Not applicable for #{model_instance.class} as #{watch_permission} " +
'is a public permission'
'is a public permission'
end
watcher_role.remove_permission! watch_permission
@ -176,4 +211,82 @@ MESSAGE
it { is_expected.to be_truthy }
end
end
describe '#addable_watcher_users' do
subject { model_instance.addable_watcher_users }
before do
# in case the model_instance creates users, we do not want them
# to mess with our expected users
model_instance
User.destroy_all
Role.non_member
Role.anonymous
admin.save!
anonymous_user.save!
user_with_permission.save!
user_wo_permission.save!
end
include_context 'non member role has the permission to watch'
include_context 'anonymous role has the permission to watch'
context 'when it is a public project' do
before do
project.update_attributes is_public: true
model_instance.reload
end
it 'contains all allowed to view' do
expected_users = [user_with_permission,
non_member_user,
admin]
expected_users << user_wo_permission if is_public_permission
is_expected
.to match_array(expected_users)
end
context 'when the user is already watching' do
before do
Watcher.create(watchable: model_instance, user: user_with_permission)
Watcher.create(watchable: model_instance, user: non_member_user)
end
it 'is no longer contained' do
expected_users = [admin]
expected_users << user_wo_permission if is_public_permission
is_expected
.to match_array(expected_users)
end
end
end
context 'when it is a private project' do
before do
project.update_attributes is_public: false
model_instance.reload
end
it 'contains members allowed to view' do
is_expected
.to match_array([user_with_permission])
end
context 'when the user is already watching' do
before do
Watcher.create(watchable: model_instance, user: user_with_permission)
end
it 'is no longer contained' do
is_expected
.to be_empty
end
end
end
end
end

6
spec_legacy/unit/watcher_spec.rb
Unescape View File

@ -79,12 +79,6 @@ describe Watcher do
assert_equal 1, @issue.watchers.count
end
it 'should addable_watcher_users' do
addable_watcher_users = @issue.addable_watcher_users
assert_kind_of Array, addable_watcher_users
assert_kind_of User, addable_watcher_users.first
end
it 'should recipients' do
@user.update_attribute :mail_notification, 'all'

Write Preview
Loading…
Cancel
Save