Merge pull request #10396 from opf/bugfix/file-links-visible-even-with-disabled-projects_storages-mapping

[bugfix] Added additional visibility check for file links

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

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

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

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

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

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
 
@@ -80,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
@@ -162,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)
@@ -312,11 +328,23 @@ 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
+
+    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

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") }
 

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
@@ -109,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" }
 
@@ -116,6 +128,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 +189,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) }
 

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?