From 164daa48f838e260ba5d92fb775a9e1c76dd913c Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 26 Dec 2022 11:32:56 +0100 Subject: [PATCH] Fix archive project through API The `Projects::UpdateService` used by the API endpoint was duplicating the checks done in `ArchiveContract` and `UnarchiveContract`. Now the relevant contract is called and used instead. As the duplication has been removed, the class `Projects::Archiver` is not needed anymore and its methods have been inlined. Also, as project is not active when being archived, all permission checks would fail because no action is allowed on archived projects. So the condition was relaxed a little to also allow permission check on projects being archived. --- .../concerns/requires_admin_guard.rb | 8 +-- app/contracts/projects/archive_contract.rb | 21 +++++--- app/contracts/projects/archiver.rb | 50 ------------------- app/contracts/projects/base_contract.rb | 12 ++--- app/contracts/projects/unarchive_contract.rb | 9 ++-- app/models/project.rb | 4 ++ app/models/users/project_role_cache.rb | 12 ++++- .../authorization/user_allowed_service.rb | 2 +- app/services/projects/update_service.rb | 24 +++++---- spec/models/project_spec.rb | 48 +++++++++++++++--- .../api/v3/projects/update_resource_spec.rb | 18 +++---- .../user_allowed_service_spec.rb | 13 +++++ 12 files changed, 113 insertions(+), 108 deletions(-) delete mode 100644 app/contracts/projects/archiver.rb diff --git a/app/contracts/concerns/requires_admin_guard.rb b/app/contracts/concerns/requires_admin_guard.rb index b528807c35..568193a0bc 100644 --- a/app/contracts/concerns/requires_admin_guard.rb +++ b/app/contracts/concerns/requires_admin_guard.rb @@ -30,15 +30,11 @@ module RequiresAdminGuard extend ActiveSupport::Concern included do - validate { validate_admin_only(user, errors) } + validate :validate_admin_only end - module_function - # Adds an error if user is archived or not an admin. - # - # Can be used from outside like +RequiresAdminGuard.validate_admin_only(user, errors)+ - def validate_admin_only(user, errors) + def validate_admin_only unless user.admin? && user.active? errors.add :base, :error_unauthorized end diff --git a/app/contracts/projects/archive_contract.rb b/app/contracts/projects/archive_contract.rb index a0a3300267..4efb22ba8e 100644 --- a/app/contracts/projects/archive_contract.rb +++ b/app/contracts/projects/archive_contract.rb @@ -27,15 +27,24 @@ #++ module Projects - class ArchiveContract < ModelContract - include Projects::Archiver - + class ArchiveContract < ::BaseContract validate :validate_no_foreign_wp_references validate :validate_has_archive_project_permission - validate :validate_has_archive_project_permission protected + # Check that there is no wp of a non descendant project that is assigned + # to one of the project or descendant versions + def validate_no_foreign_wp_references + version_ids = model.rolled_up_versions.select(:id) + + exists = WorkPackage + .where.not(project_id: model.self_and_descendants.select(:id)) + .exists?(version_id: version_ids) + + errors.add :base, :foreign_wps_reference_version if exists + end + def validate_has_archive_project_permission validate_can_archive_project validate_can_archive_subprojects @@ -57,9 +66,5 @@ module Projects errors.add :base, :archive_permission_missing_on_subprojects end end - - def validate_model? - false - end end end diff --git a/app/contracts/projects/archiver.rb b/app/contracts/projects/archiver.rb deleted file mode 100644 index 1b3b8e954d..0000000000 --- a/app/contracts/projects/archiver.rb +++ /dev/null @@ -1,50 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2023 the OpenProject GmbH -# -# 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 COPYRIGHT and LICENSE files for more details. -#++ - -module Projects - module Archiver - # Check that there is no wp of a non descendant project that is assigned - # to one of the project or descendant versions - def validate_no_foreign_wp_references - version_ids = model.rolled_up_versions.select(:id) - - exists = WorkPackage - .where.not(project_id: model.self_and_descendants.select(:id)) - .where(version_id: version_ids) - .exists? - - errors.add :base, :foreign_wps_reference_version if exists - end - - def validate_all_ancestors_active - if model.ancestors.any?(&:archived?) - errors.add :base, :archived_ancestor - end - end - end -end diff --git a/app/contracts/projects/base_contract.rb b/app/contracts/projects/base_contract.rb index c5fe9015e6..a51c8b7bbe 100644 --- a/app/contracts/projects/base_contract.rb +++ b/app/contracts/projects/base_contract.rb @@ -30,7 +30,6 @@ module Projects class BaseContract < ::ModelContract include AssignableValuesContract include AssignableCustomFieldValues - include Projects::Archiver attribute :name attribute :identifier @@ -131,14 +130,11 @@ module Projects def validate_changing_active return unless model.active_changed? - RequiresAdminGuard.validate_admin_only(user, errors) + contract_klass = model.being_archived? ? ArchiveContract : UnarchiveContract + contract = contract_klass.new(model, user) + contract.validate - if model.active? - # switched to active -> unarchiving - validate_all_ancestors_active - else - validate_no_foreign_wp_references - end + errors.merge!(contract.errors) end end end diff --git a/app/contracts/projects/unarchive_contract.rb b/app/contracts/projects/unarchive_contract.rb index d0f6b57848..e2144abea9 100644 --- a/app/contracts/projects/unarchive_contract.rb +++ b/app/contracts/projects/unarchive_contract.rb @@ -27,16 +27,17 @@ #++ module Projects - class UnarchiveContract < ModelContract + class UnarchiveContract < ::BaseContract include RequiresAdminGuard - include Projects::Archiver validate :validate_all_ancestors_active protected - def validate_model? - false + def validate_all_ancestors_active + if model.ancestors.any?(&:archived?) + errors.add :base, :archived_ancestor + end end end end diff --git a/app/models/project.rb b/app/models/project.rb index 8b9f027c97..b09223f034 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -162,6 +162,10 @@ class Project < ApplicationRecord !active? end + def being_archived? + (active == false) && (active_was == true) + end + def copy_allowed? User.current.allowed_to?(:copy_projects, self) end diff --git a/app/models/users/project_role_cache.rb b/app/models/users/project_role_cache.rb index 2ed6f6a421..8301a61c9f 100644 --- a/app/models/users/project_role_cache.rb +++ b/app/models/users/project_role_cache.rb @@ -40,8 +40,9 @@ class Users::ProjectRoleCache private def roles(project) - # No role on archived projects - return [] unless !project || project&.active? + # Project is nil if checking global role + # No roles on archived projects, unless the active state is being changed + return [] if project && archived?(project) # Return all roles if user is admin return all_givable_roles if user.admin? @@ -56,4 +57,11 @@ class Users::ProjectRoleCache def all_givable_roles @all_givable_roles ||= Role.givable.to_a end + + def archived?(project) + # project for which activity is being changed is still considered active + return false if project.being_archived? + + project.archived? + end end diff --git a/app/services/authorization/user_allowed_service.rb b/app/services/authorization/user_allowed_service.rb index 6a174c6b05..fa140bdb09 100644 --- a/app/services/authorization/user_allowed_service.rb +++ b/app/services/authorization/user_allowed_service.rb @@ -79,7 +79,7 @@ class Authorization::UserAllowedService end # No action allowed on archived projects - return false unless project.active? + return false unless project.active? || project.being_archived? # No action allowed on disabled modules return false unless project.allows_to?(action) # Inactive users are never authorized diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 4a756227ff..098f195b46 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -90,17 +90,19 @@ module Projects def handle_archiving return unless model.saved_change_to_active? - if model.active? - # was unarchived - Projects::UnarchiveService - .new(user:, model:) - .call - else - # as archived - Projects::ArchiveService - .new(user:, model:) - .call - end + service_class = + if model.active? + # was unarchived + Projects::UnarchiveService + else + # was archived + Projects::ArchiveService + end + + # EmptyContract is used because archive/unarchive conditions have + # already been checked in Projects::UpdateContract + service = service_class.new(user:, model:, contract_class: EmptyContract) + service.call end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 91affacda1..a36e293fb7 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -55,18 +55,54 @@ describe Project do end describe '#archived?' do - context 'if archived' do - it 'is true' do - expect(project).not_to be_archived + subject { project.archived? } + + context 'if active is true' do + let(:active) { true } + + it { is_expected.to be false } + end + + context 'if active is false' do + let(:active) { false } + + it { is_expected.to be true } + end + end + + describe '#being_archived?' do + subject { project.being_archived? } + + context 'if active is true' do + let(:active) { true } + + it { is_expected.to be false } + end + + context 'if active was true and changes to false (marking as archived)' do + let(:active) { true } + + before do + project.active = false end + + it { is_expected.to be true } end - context 'if not archived' do + context 'if active is false' do let(:active) { false } - it 'is false' do - expect(project).to be_archived + it { is_expected.to be false } + end + + context 'if active was false and changes to true (marking as active)' do + let(:active) { false } + + before do + project.active = true end + + it { is_expected.to be false } end end diff --git a/spec/requests/api/v3/projects/update_resource_spec.rb b/spec/requests/api/v3/projects/update_resource_spec.rb index a4781e23a0..bd1865d670 100644 --- a/spec/requests/api/v3/projects/update_resource_spec.rb +++ b/spec/requests/api/v3/projects/update_resource_spec.rb @@ -268,6 +268,12 @@ describe 'API v3 Project resource update', type: :request, content_type: :json d end context 'when deactivating (archiving) the project' do + let(:body) do + { + active: false + } + end + context 'for an admin' do let(:current_user) do create(:admin) @@ -281,12 +287,6 @@ describe 'API v3 Project resource update', type: :request, content_type: :json d create(:project) end - let(:body) do - { - active: false - } - end - it 'responds with 200 OK' do expect(last_response.status) .to be(200) @@ -304,12 +304,6 @@ describe 'API v3 Project resource update', type: :request, content_type: :json d end context 'for a non admin' do - let(:body) do - { - active: false - } - end - it 'responds with 403' do expect(last_response.status) .to be(403) diff --git a/spec/services/authorization/user_allowed_service_spec.rb b/spec/services/authorization/user_allowed_service_spec.rb index 9408206eac..a832c7f955 100644 --- a/spec/services/authorization/user_allowed_service_spec.rb +++ b/spec/services/authorization/user_allowed_service_spec.rb @@ -137,6 +137,7 @@ describe Authorization::UserAllowedService do before do Array(context).each do |project| project.active = false + project.clear_changes_information end end @@ -151,6 +152,18 @@ describe Authorization::UserAllowedService do end end + context 'with the project being archived' do + before do + Array(context).each do |project| + project.active = false + end + end + + it 'is true' do + expect(subject).to be_truthy + end + end + context 'with the project not having the action enabled' do let(:project_allows_to) { false }