From fa150d44e16557dfa3b4d5c35015f923900ced21 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Fri, 25 Mar 2022 16:11:57 +0100 Subject: [PATCH 1/2] [bugfix] Added additional visibility check for file links - returning file link collections and single file links no longer return file links linking a work package within a project, which has no storage mapping to the file link's storage - filter on work package collections no longer return file links, that have the same mapping problem as above - added additional tests and test data to cover that behaviour --- .../filter/file_link_origin_id_filter.rb | 8 ++++++++ .../work_packages/filter/storage_id_filter.rb | 8 ++++++++ .../filter/storage_url_filter.rb | 6 +++++- .../filter/storages_filter_mixin.rb | 5 +++++ .../storages/app/models/storages/file_link.rb | 3 ++- .../api/v3/file_links/file_links_spec.rb | 18 ++++++++++++++--- .../work_packages_linked_filter_spec.rb | 20 +++++++++++++++++++ 7 files changed, 63 insertions(+), 5 deletions(-) diff --git a/modules/storages/app/models/queries/storages/work_packages/filter/file_link_origin_id_filter.rb b/modules/storages/app/models/queries/storages/work_packages/filter/file_link_origin_id_filter.rb index 2578997a6b..d1a3c18213 100644 --- a/modules/storages/app/models/queries/storages/work_packages/filter/file_link_origin_id_filter.rb +++ b/modules/storages/app/models/queries/storages/work_packages/filter/file_link_origin_id_filter.rb @@ -41,5 +41,13 @@ module Queries::Storages::WorkPackages::Filter def permission :view_file_links end + + def joins + %i[file_links storages] + end + + def additional_where_condition + "AND #{::Storages::FileLink.table_name}.storage_id = #{::Storages::Storage.table_name}.id" + end end end diff --git a/modules/storages/app/models/queries/storages/work_packages/filter/storage_id_filter.rb b/modules/storages/app/models/queries/storages/work_packages/filter/storage_id_filter.rb index b3a24593cf..f0733a41b7 100644 --- a/modules/storages/app/models/queries/storages/work_packages/filter/storage_id_filter.rb +++ b/modules/storages/app/models/queries/storages/work_packages/filter/storage_id_filter.rb @@ -41,5 +41,13 @@ module Queries::Storages::WorkPackages::Filter def permission :view_file_links end + + def joins + [:file_links, { project: :projects_storages }] + end + + def additional_where_condition + "AND #{::Storages::FileLink.table_name}.storage_id = #{::Storages::ProjectStorage.table_name}.storage_id" + end end end diff --git a/modules/storages/app/models/queries/storages/work_packages/filter/storage_url_filter.rb b/modules/storages/app/models/queries/storages/work_packages/filter/storage_url_filter.rb index 56de183c69..12b2c46419 100644 --- a/modules/storages/app/models/queries/storages/work_packages/filter/storage_url_filter.rb +++ b/modules/storages/app/models/queries/storages/work_packages/filter/storage_url_filter.rb @@ -47,7 +47,11 @@ module Queries::Storages::WorkPackages::Filter end def joins - { file_links: :storage } + [{ file_links: :storage }, { project: :projects_storages }] + end + + def additional_where_condition + "AND #{::Storages::FileLink.table_name}.storage_id = #{::Storages::ProjectStorage.table_name}.storage_id" end end end diff --git a/modules/storages/app/models/queries/storages/work_packages/filter/storages_filter_mixin.rb b/modules/storages/app/models/queries/storages/work_packages/filter/storages_filter_mixin.rb index 69b26ca71e..7bffb8123e 100644 --- a/modules/storages/app/models/queries/storages/work_packages/filter/storages_filter_mixin.rb +++ b/modules/storages/app/models/queries/storages/work_packages/filter/storages_filter_mixin.rb @@ -55,6 +55,7 @@ module Queries::Storages::WorkPackages::Filter::StoragesFilterMixin <<-SQL.squish #{::Queries::Operators::Equals.sql_for_field(where_values, filter_model.table_name, filter_column)} AND work_packages.project_id IN (#{Project.allowed_to(User.current, permission).select(:id).to_sql}) + #{additional_where_condition} SQL end @@ -62,6 +63,10 @@ module Queries::Storages::WorkPackages::Filter::StoragesFilterMixin values end + def additional_where_condition + '' + end + def joins filter_model.table_name.to_sym end diff --git a/modules/storages/app/models/storages/file_link.rb b/modules/storages/app/models/storages/file_link.rb index d411fc7091..79317938a9 100644 --- a/modules/storages/app/models/storages/file_link.rb +++ b/modules/storages/app/models/storages/file_link.rb @@ -64,9 +64,10 @@ class Storages::FileLink < ApplicationRecord # join projects through the container, and filter on projects visible from # the user includes(:container) - .includes(container: :project) + .includes(container: { project: :projects_storages }) .references(:projects) .merge(Project.allowed_to(user, :view_file_links)) + .where('projects_storages.storage_id = file_links.storage_id') } delegate :project, to: :container diff --git a/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb b/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb index 119b6dbca1..17a0ab2617 100644 --- a/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb +++ b/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb @@ -50,9 +50,14 @@ describe 'API v3 file links resource', type: :request do let(:file_link) do create(:file_link, creator: current_user, container: work_package, storage: storage) end - let(:another_file_link) do + let(:file_link_of_other_work_package) do create(:file_link, creator: current_user, container: another_work_package, storage: storage) end + # If a storage mapping between a project and a storage is removed, the file link still persist. This can occur on + # moving a work package to another project, too, if target project does not yet have the storage mapping. + let(:file_link_of_unlinked_storage) do + create(:file_link, creator: current_user, container: work_package, storage: another_storage) + end subject(:response) { last_response } @@ -65,7 +70,8 @@ describe 'API v3 file links resource', type: :request do before do file_link - another_file_link + file_link_of_other_work_package + file_link_of_unlinked_storage get path end @@ -312,11 +318,17 @@ describe 'API v3 file links resource', type: :request do it_behaves_like 'not found' end - context 'if no storage with that id exists' do + context 'if no file link with that id exists' do let(:path) { api_v3_paths.file_link(1337) } it_behaves_like 'not found' end + + context 'if file link is in a work package, while its project is not mapped to the file link\'s storage.' do + let(:path) { api_v3_paths.file_link(file_link_of_unlinked_storage.id) } + + it_behaves_like 'not found' + end end describe 'DELETE /api/v3/file_links/:file_link_id' do diff --git a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb index df8992c633..b3577eb61c 100644 --- a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb +++ b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb @@ -63,6 +63,9 @@ describe 'API v3 work packages resource with filters for linked storage file', storage: storage2, origin_id: file_link1.origin_id) end + # This link is considered invisible, as it is linking a work package to a file, where the work package's project + # and the file's storage are not linked together. + let(:file_link4) { create(:file_link, creator: current_user, container: work_package3, storage: storage1) } subject(:response) { last_response } @@ -73,6 +76,7 @@ describe 'API v3 work packages resource with filters for linked storage file', file_link1 file_link2 file_link3 + file_link4 login_as current_user end @@ -116,6 +120,14 @@ describe 'API v3 work packages resource with filters for linked storage file', let(:elements) { [] } end end + + context 'if the filter is set to a file linked to a work package in an unlinked project' do + let(:origin_id_value) { file_link4.origin_id.to_s } + + it_behaves_like 'API V3 collection response', 0, 0, 'WorkPackage', 'WorkPackageCollection' do + let(:elements) { [] } + end + end end context 'with single filter for storage id' do @@ -169,6 +181,14 @@ describe 'API v3 work packages resource with filters for linked storage file', let(:elements) { [work_package3] } end + context 'if any of the matching work packages is in a project without a mapping to that storage' do + let(:storage_url_value) { CGI.escape(storage1.host) } + + it_behaves_like 'API V3 collection response', 2, 2, 'WorkPackage', 'WorkPackageCollection' do + let(:elements) { [work_package1, work_package2] } + end + end + context 'if one project has not sufficient permissions' do let(:role2) { create :role, permissions: %i(view_work_packages) } From d1a738ca6a147cd3bf83242dd5dfbaeccce3bd03 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Wed, 30 Mar 2022 10:47:48 +0200 Subject: [PATCH 2/2] [bugfix] add spec for projects without storages module --- .../api/v3/file_links/file_links_spec.rb | 30 ++++++++++++++----- .../work_packages_linkable_filter_spec.rb | 16 ++++++++++ .../work_packages_linked_filter_spec.rb | 8 +++++ spec/factories/project_factory.rb | 6 ++-- 4 files changed, 50 insertions(+), 10 deletions(-) diff --git a/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb b/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb index 17a0ab2617..41d7663647 100644 --- a/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb +++ b/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb @@ -86,6 +86,14 @@ describe 'API v3 file links resource', type: :request do let(:elements) { [] } end end + + context 'if storages module is deactivated for the work package\'s project' do + let(:project) { create(:project, disable_modules: :storages) } + + it_behaves_like 'API V3 collection response', 0, 0, 'FileLink', 'Collection' do + let(:elements) { [] } + end + end end describe 'POST /api/v3/work_packages/:work_package_id/file_links' do @@ -168,15 +176,17 @@ describe 'API v3 file links resource', type: :request do context 'when some file link elements with matching origin_id, container, and storage already exist in database' do let(:existing_file_link) do - create(:file_link, origin_name: 'original name', - creator: current_user, - container: work_package, - storage: storage) + create(:file_link, + origin_name: 'original name', + creator: current_user, + container: work_package, + storage: storage) end let(:already_existing_file_link_payload) do - build(:file_link_element, origin_name: 'new name', - origin_id: existing_file_link.origin_id, - storage_url: existing_file_link.storage.host) + build(:file_link_element, + origin_name: 'new name', + origin_id: existing_file_link.origin_id, + storage_url: existing_file_link.storage.host) end let(:some_file_link_payload) do build(:file_link_element, storage_url: existing_file_link.storage.host) @@ -329,6 +339,12 @@ describe 'API v3 file links resource', type: :request do it_behaves_like 'not found' end + + context 'if file link is in a work package, while the storages module is deactivated in its project.' do + let(:project) { create(:project, disable_modules: :storages) } + + it_behaves_like 'not found' + end end describe 'DELETE /api/v3/file_links/:file_link_id' do diff --git a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb index 57ea1d9fb4..0c8d7f415e 100644 --- a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb +++ b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb @@ -106,6 +106,14 @@ describe 'API v3 work packages resource with filters for the linkable to storage end end + context 'if a project has the storages module deactivated' do + let(:project1) { create(:project, disable_modules: :storages, members: { current_user => role1 }) } + + it_behaves_like 'API V3 collection response', 2, 2, 'WorkPackage', 'WorkPackageCollection' do + let(:elements) { [work_package3, work_package4] } + end + end + context 'if the filter is set to an unknown storage id' do let(:storage_id) { "1337" } @@ -140,6 +148,14 @@ describe 'API v3 work packages resource with filters for the linkable to storage end end + context 'if a project has the storages module deactivated' do + let(:project1) { create(:project, disable_modules: :storages, members: { current_user => role1 }) } + + it_behaves_like 'API V3 collection response', 2, 2, 'WorkPackage', 'WorkPackageCollection' do + let(:elements) { [work_package3, work_package4] } + end + end + context 'if the filter is set to an unknown storage url' do let(:storage_url) { CGI.escape("https://not.my-domain.org") } diff --git a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb index b3577eb61c..69597929ff 100644 --- a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb +++ b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb @@ -113,6 +113,14 @@ describe 'API v3 work packages resource with filters for linked storage file', end end + context 'if a project has the storages module deactivated' do + let(:project1) { create(:project, disable_modules: :storages, members: { current_user => role1 }) } + + it_behaves_like 'API V3 collection response', 1, 1, 'WorkPackage', 'WorkPackageCollection' do + let(:elements) { [work_package3] } + end + end + context 'if the filter is set to an unknown file id from origin' do let(:origin_id_value) { "1337" } diff --git a/spec/factories/project_factory.rb b/spec/factories/project_factory.rb index 7beea9d8a7..aba405596e 100644 --- a/spec/factories/project_factory.rb +++ b/spec/factories/project_factory.rb @@ -36,14 +36,14 @@ FactoryBot.define do sequence(:name) { |n| "My Project No. #{n}" } sequence(:identifier) { |n| "myproject_no_#{n}" } - created_at { Time.now } - updated_at { Time.now } + created_at { Time.zone.now } + updated_at { Time.zone.now } enabled_module_names { OpenProject::AccessControl.available_project_modules } public { false } templated { false } callback(:after_build) do |project, evaluator| - disabled_modules = Array(evaluator.disable_modules) + disabled_modules = Array(evaluator.disable_modules).map(&:to_s) project.enabled_module_names = project.enabled_module_names - disabled_modules if !evaluator.no_types && project.types.empty?