[#43692] Part 2. Modify API to be able to handle file_links without a container. (#11992)

* [#43692] Modify API to be able to handle file_links without a container.

https://community.openproject.org/work_packages/43692

* Set work_packages--file_links association to be polymorphic.

- Replace the usages of Storages::FileLinks.visible, because
  eager_loading of polymorphic associations is not easy.
- Remove WorkPackages::CreateService#replace_attachments method.
  It does actual saving of a work_package. Replaced the call with actual
  code.

app/contracts/work_packages/create_contract.rb

@@ -38,14 +38,20 @@ module WorkPackages
     default_attribute_permission :add_work_packages
 
     validate :user_allowed_to_add
+    validate :user_allowed_to_manage_file_links
 
     private
 
     def user_allowed_to_add
       if (model.project && !@user.allowed_to?(:add_work_packages, model.project)) ||
          !@user.allowed_to_globally?(:add_work_packages)
+        errors.add(:base, :error_unauthorized)
+      end
+    end
 
-        errors.add :base, :error_unauthorized
+    def user_allowed_to_manage_file_links
+      if model.file_links.present? && model.project.present? && !user.allowed_to?(:manage_file_links, model.project)
+        errors.add(:base, :error_unauthorized)
       end
     end
 

app/models/work_package.rb

@@ -57,8 +57,8 @@ class WorkPackage < ApplicationRecord
 
   has_many :time_entries, dependent: :delete_all
 
-  has_many :file_links,
+  has_many :file_links, dependent: :delete_all, class_name: 'Storages::FileLink', as: :container
-           dependent: :delete_all, class_name: 'Storages::FileLink', foreign_key: 'container_id', inverse_of: :container
+
   has_many :storages, through: :project
 
   has_and_belongs_to_many :changesets, -> { # rubocop:disable Rails/HasAndBelongsToMany
@@ -310,8 +310,7 @@ class WorkPackage < ApplicationRecord
 
   def type_id=(tid)
     self.type = nil
-    result = write_attribute(:type_id, tid)
+    write_attribute(:type_id, tid)
-    result
   end
 
   # Overrides attributes= so that type_id gets assigned first
@@ -478,7 +477,7 @@ class WorkPackage < ApplicationRecord
     key = 'activity_id'
     id = attributes[key]
     default_id = if id&.present?
-                   Enumeration.exists? id: id, is_default: true, type: 'TimeEntryActivity'
+                   Enumeration.exists? id:, is_default: true, type: 'TimeEntryActivity'
                  else
                    true
                  end

app/services/work_packages/create_service.rb

@@ -26,7 +26,7 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-class WorkPackages::CreateService < ::BaseServices::BaseCallable
+class WorkPackages::CreateService < BaseServices::BaseCallable
   include ::WorkPackages::Shared::UpdateAncestors
   include ::Shared::ServiceContext
 
@@ -51,8 +51,10 @@ class WorkPackages::CreateService < ::BaseServices::BaseCallable
   def create(attributes, work_package)
     result = set_attributes(attributes, work_package)
 
-    result.success = if result.success
+    result.success =
-                       replace_attachments(work_package)
+      if result.success
+        work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements
+        work_package.save
       else
         false
       end
@@ -80,15 +82,9 @@ class WorkPackages::CreateService < ::BaseServices::BaseCallable
       .call(attributes)
   end
 
-  def replace_attachments(work_package)
-    work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements
-    work_package.save
-  end
-
   def reschedule_related(work_package)
     result = WorkPackages::SetScheduleService
-             .new(user:,
+               .new(user:, work_package:)
-                  work_package:)
                .call
 
     result.self_and_dependent.each do |r|

app/services/work_packages/set_attributes_service.rb

@@ -26,12 +26,15 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes
+class WorkPackages::SetAttributesService < BaseServices::SetAttributes
   include Attachments::SetReplacements
 
   private
 
   def set_attributes(attributes)
+    file_links_ids = attributes.delete(:file_links_ids)
+    model.file_links = Storages::FileLink.where(id: file_links_ids) if file_links_ids
+
     set_attachments_attributes(attributes)
     set_static_attributes(attributes)
 
@@ -382,6 +385,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes
     max = max_child_date
 
     return unless max
+
     days.duration(min_child_date, max_child_date)
   end
 

docs/api/apiv3/components/schemas/work_package_model.yml

@@ -24,7 +24,7 @@ properties:
     readOnly: true
   description:
     allOf:
-      - "$ref": "./formattable.yml"
+      - $ref: "./formattable.yml"
       - description: The work package description
   scheduleManually:
     type: boolean
@@ -120,7 +120,7 @@ properties:
     properties:
       addAttachment:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Attach a file to the WP
               
@@ -130,7 +130,7 @@ properties:
             readOnly: true
       addComment:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Post comment to WP
               
@@ -140,7 +140,7 @@ properties:
             readOnly: true
       addRelation:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Adds a relation to this work package.
               
@@ -150,7 +150,7 @@ properties:
             readOnly: true
       addWatcher:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Add any user to WP watchers
               
@@ -163,7 +163,7 @@ properties:
         readOnly: true
         items:
           allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               A predefined action that can be applied to the work package.
               
@@ -171,13 +171,13 @@ properties:
             readOnly: true
       previewMarkup:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: Post markup (in markdown) here to receive an HTML-rendered
               response
             readOnly: true
       removeWatcher:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Remove any user from WP watchers
               
@@ -187,7 +187,7 @@ properties:
             readOnly: true
       unwatch:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Remove current user from WP watchers
               
@@ -197,7 +197,7 @@ properties:
             readOnly: true
       update:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Form endpoint that aids in preparing and performing edits on a WP
               
@@ -207,7 +207,7 @@ properties:
             readOnly: true
       updateImmediately:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Directly perform edits on a work package
               
@@ -217,7 +217,7 @@ properties:
             readOnly: true
       watch:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Add current user to WP watchers
               
@@ -227,7 +227,7 @@ properties:
             readOnly: true
       self:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               This work package
               
@@ -235,7 +235,7 @@ properties:
             readOnly: true
       schema:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The schema of this work package
               
@@ -246,7 +246,7 @@ properties:
         readOnly: true
         items:
           allOf:
-            - "$ref": "./link.yml"
+            - $ref: "./link.yml"
             - description: |-
                 A visible ancestor work package of the current work package.
                 
@@ -258,14 +258,14 @@ properties:
               readOnly: true
       attachments:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The files attached to this work package
               
               **Resource**: Collection
       author:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The person that created the work package
               
@@ -273,14 +273,14 @@ properties:
             readOnly: true
       assignee:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The person that is intended to work on the work package
               
               **Resource**: User
       availableWatchers:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               All users that can be added to the work package as watchers.
               
@@ -292,7 +292,7 @@ properties:
             readOnly: true
       budget:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The budget this work package is associated to
               
@@ -303,7 +303,7 @@ properties:
               **Permission** view cost objects
       category:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The category of the work package
               
@@ -313,7 +313,7 @@ properties:
         readOnly: true
         items:
           allOf:
-            - "$ref": "./link.yml"
+            - $ref: "./link.yml"
             - description: |-
                 A visible child work package of the current work package.
                 
@@ -343,35 +343,35 @@ properties:
               **Permission**: view file links
       parent:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Parent work package
               
               **Resource**: WorkPackage
       priority:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The priority of the work package
               
               **Resource**: Priority
       project:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The project to which the work package belongs
               
               **Resource**: Project
       responsible:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The person that is responsible for the overall outcome
               
               **Resource**: User
       relations:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Relations this work package is involved in
               
@@ -383,7 +383,7 @@ properties:
             readOnly: true
       revisions:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               Revisions that are referencing the work package
               
@@ -395,14 +395,14 @@ properties:
             readOnly: true
       status:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The current status of the work package
               
               **Resource**: Status
       timeEntries:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               All time entries logged on the work package. Please note that this is a link to an HTML resource for now and as such, the link is subject to change.
               
@@ -414,21 +414,21 @@ properties:
             readOnly: true
       type:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The type of the work package
               
               **Resource**: Type
       version:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               The version associated to the work package
               
               **Resource**: Version
       watchers:
         allOf:
-          - "$ref": "./link.yml"
+          - $ref: "./link.yml"
           - description: |-
               All users that are currently watching this work package
               

docs/api/apiv3/paths/work_packages.yml

@@ -108,8 +108,7 @@ get:
       required: false
       schema:
         type: string
-    - description: Indicates whether properties should be summed up if they support
+    - description: Indicates whether properties should be summed up if they support it.
-        it.
       example: true
       in: query
       name: showSums
@@ -150,7 +149,7 @@ get:
 
 post:
   summary: Create Work Package
-  operationId: Create_Work_Package
+  operationId: create_work_package
   tags:
     - Work Packages
   description: |-

lib/api/v3/work_packages/work_package_payload_representer.rb

@@ -35,6 +35,25 @@ module API
 
         cached_representer disabled: true
 
+        property :file_links,
+                 exec_context: :decorator,
+                 getter: ->(*) {},
+                 setter: ->(fragment:, **) do
+                   next unless fragment.is_a?(Array)
+
+                   ids = fragment.map do |link|
+                     ::API::Utilities::ResourceLinkParser.parse_id link['href'],
+                                                                   property: :file_link,
+                                                                   expected_version: '3',
+                                                                   expected_namespace: :file_links
+                   end
+
+                   represented.file_links_ids = ids
+                 end,
+                 skip_render: ->(*) { true },
+                 linked_resource: true,
+                 uncacheable: true
+
         def writable_attributes
           super + %w[date]
         end

modules/storages/app/common/storages/peripherals/scopes.rb

@@ -31,9 +31,5 @@ module Storages::Peripherals
     def visible_storages
       ::Storages::Storage.visible(current_user)
     end
-
-    def visible_file_links
-      ::Storages::FileLink.visible(current_user)
-    end
   end
 end

modules/storages/app/models/storages/file_link.rb

@@ -47,7 +47,7 @@ class Storages::FileLink < ApplicationRecord
 
   # FileLinks are attached to a container ()currently a WorkPackage)
   # Wieland: This needs to become more flexible in the future
-  belongs_to :container, class_name: 'WorkPackage'
+  belongs_to :container, polymorphic: true
 
   # Currently only WorkPackages are supported as containers for FileLinks
   # For some reason this case is not handled in the belongs_to above.
@@ -64,20 +64,6 @@ class Storages::FileLink < ApplicationRecord
     @origin_permission || nil
   end
 
-  # A standard Rails custom query:
-  # https://www.rubyguides.com/2019/10/scopes-in-ruby-on-rails/
-  # Purpose: limit to FileLink visible by given user.
-  # Used by: FileLinksAPI#visible_file_links and WorkPackagesFileLinksAPI#visible_file_links
-  scope :visible, ->(user = User.current) {
-    # join projects through the container, and filter on projects visible from
-    # the user
-    includes(:container)
-      .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
 
   def name

modules/storages/lib/api/v3/file_links/file_links_api.rb

@@ -30,10 +30,6 @@
 # functionality from the Grape REST API framework. It is mounted in lib/api/v3/root.rb.
 # -> /api/v3/file_links/...
 class API::V3::FileLinks::FileLinksAPI < API::OpenProjectAPI
-  # helpers is defined by the grape framework. They make methods from the
-  # module available from within the endpoint context.
-  helpers Storages::Peripherals::Scopes
-
   # The `:resources` keyword defines the API namespace -> /api/v3/file_links/...
   resources :file_links do
     post &::API::V3::FileLinks::CreateEndpoint
@@ -50,7 +46,13 @@ class API::V3::FileLinks::FileLinksAPI < API::OpenProjectAPI
       # The after validation hook executes after the validation of the request format, but before any execution
       # inside the endpoint context. Hence, it is a good place to actually fetch the handled resource.
       after_validation do
-        @file_link = visible_file_links.find(params[:file_link_id])
+        @file_link = Storages::FileLink.find(params[:file_link_id])
+
+        unless @file_link.container.present? &&
+               current_user.allowed_to?(:view_file_links, @file_link.project) &&
+               @file_link.project.storage_ids.include?(@file_link.storage_id)
+          raise ::API::Errors::NotFound.new
+        end
       end
 
       # A helper is used to define the behaviour at GET /api/v3/file_links/:file_link_id

modules/storages/lib/api/v3/file_links/work_packages_file_links_api.rb

@@ -31,10 +31,6 @@
 # which puts the file_links namespace behind the provided namespace of the work packages api
 # -> /api/v3/work_packages/:id/file_links/...
 class API::V3::FileLinks::WorkPackagesFileLinksAPI < API::OpenProjectAPI
-  # helpers is defined by the grape framework. They make methods from the
-  # module available from within the endpoint context.
-  helpers Storages::Peripherals::Scopes
-
   # The `:resources` keyword defines the API namespace -> /api/v3/work_packages/:id/file_links/...
   resources :file_links do
     # Get the list of FileLinks related to a work package, with updated information from Nextcloud.
@@ -51,19 +47,22 @@ class API::V3::FileLinks::WorkPackagesFileLinksAPI < API::OpenProjectAPI
         raise ::API::Errors::InvalidQuery.new(message)
       end
 
+      result = if current_user.allowed_to?(:view_file_links, @work_package.project)
                  # Get a (potentially huge...) list of all FileLinks for the work package.
-      file_links = query.results
+                 file_links = query.results.where(container_id: @work_package.id,
-                        .where(id: visible_file_links
+                                                  container_type: 'WorkPackage',
-                                     .where(container_id: @work_package.id, container_type: 'WorkPackage'))
+                                                  storage: @work_package.project.storages)
-
                  # Synchronize with Nextcloud. StorageAPI has handled OAuth2 for us before.
                  # We ignore the result, because partial errors (storage network issues) are written to each FileLink
-      service_result = ::Storages::FileLinkSyncService
+                 ::Storages::FileLinkSyncService
                    .new(user: current_user)
                    .call(file_links)
-
+                   .result
+               else
+                 []
+               end
       ::API::V3::FileLinks::FileLinkCollectionRepresenter.new(
-        service_result.result,
+        result,
         self_link: api_v3_paths.file_links(@work_package.id),
         current_user:
       )

modules/storages/spec/factories/file_link_factory.rb

@@ -26,11 +26,10 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-# Expects parameters: storage, container_id
+# Expects parameters: storage
 FactoryBot.define do
   factory :file_link, class: '::Storages::FileLink' do
     creator factory: :user
-    container_type { 'WorkPackage' }
     sequence(:origin_id) { |n| "10000#{n}" } # ID within external storage (i.e. Nextcloud)
     sequence(:origin_name) { |n| "file_name_#{n}.txt" } # File name within external storage (i.e. Nextcloud)
     origin_created_by_name { "Peter Pan" }

modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb

@@ -479,6 +479,12 @@ describe 'API v3 file links resource' do
 
       it_behaves_like 'not found'
     end
+
+    context 'if file link does not have a container.' do
+      let(:file_link) { create(:file_link) }
+
+      it_behaves_like 'not found'
+    end
   end
 
   describe 'DELETE /api/v3/file_links/:file_link_id' do

spec/requests/api/v3/work_packages/create_resource_spec.rb

@@ -37,7 +37,8 @@ describe 'API v3 Work package resource',
     create(:project, identifier: 'test_project', public: false)
   end
   let(:role) { create(:role, permissions:) }
-  let(:permissions) { %i[add_work_packages view_project view_work_packages] }
+  let(:permissions) { %i[add_work_packages view_project view_work_packages] + extra_permissions }
+  let(:extra_permissions) { [] }
 
   current_user do
     create(:user, member_in_project: project, member_through_role: role)
@@ -128,7 +129,7 @@ describe 'API v3 Work package resource',
       expect(WorkPackage.first.type).to eq(type)
     end
 
-    context 'no permissions' do
+    context 'without any permissions' do
       let(:current_user) { create(:user) }
 
       it 'hides the endpoint' do
@@ -136,7 +137,7 @@ describe 'API v3 Work package resource',
       end
     end
 
-    context 'view_project permission' do
+    context 'when view_project permission is enabled' do
       # Note that this just removes the add_work_packages permission
       # view_project is actually provided by being a member of the project
       let(:permissions) { [:view_project] }
@@ -146,7 +147,7 @@ describe 'API v3 Work package resource',
       end
     end
 
-    context 'empty parameters' do
+    context 'with empty parameters' do
       let(:parameters) { {} }
 
       it_behaves_like 'multiple errors', 422
@@ -156,7 +157,7 @@ describe 'API v3 Work package resource',
       end
     end
 
-    context 'bogus parameters' do
+    context 'with bogus parameters' do
       let(:parameters) do
         {
           bogus: 'bogus',
@@ -180,7 +181,7 @@ describe 'API v3 Work package resource',
       end
     end
 
-    context 'schedule manually' do
+    context 'when scheduled manually' do
       let(:work_package) { WorkPackage.first }
 
       context 'with true' do
@@ -207,7 +208,7 @@ describe 'API v3 Work package resource',
       end
     end
 
-    context 'invalid value' do
+    context 'with invalid value' do
       let(:parameters) do
         {
           subject: nil,
@@ -231,7 +232,7 @@ describe 'API v3 Work package resource',
       end
     end
 
-    context 'claiming attachments' do
+    context 'when attachments are being claimed' do
       let(:attachment) { create(:attachment, container: nil, author: current_user) }
       let(:parameters) do
         {
@@ -251,7 +252,7 @@ describe 'API v3 Work package resource',
       end
 
       it 'creates the work package and assigns the attachments' do
-        expect(WorkPackage.all.count).to eq(1)
+        expect(WorkPackage.count).to eq(1)
 
         work_package = WorkPackage.last
 
@@ -259,5 +260,60 @@ describe 'API v3 Work package resource',
           .to match_array(attachment)
       end
     end
+
+    context 'when file links are being claimed' do
+      let(:storage) { create(:storage) }
+      let(:file_link) do
+        create(:file_link,
+               container_id: nil,
+               container_type: nil,
+               storage:,
+               creator: current_user)
+      end
+      let(:parameters) do
+        {
+          subject: 'subject',
+          _links: {
+            type: {
+              href: api_v3_paths.type(project.types.first.id)
+            },
+            project: {
+              href: api_v3_paths.project(project.id)
+            },
+            fileLinks: [
+              href: api_v3_paths.file_link(file_link.id)
+            ]
+          }
+        }
+      end
+      let(:extra_permissions) do
+        %i[view_file_links]
+      end
+
+      it 'does not create a work packages and responds with an error ' \
+         'when user is not allowed to manage file links', :aggregate_failtures do
+        expect(WorkPackage.count).to eq(0)
+        expect(last_response.body).to be_json_eql(
+          'urn:openproject-org:api:v3:errors:MissingPermission'.to_json
+        ).at_path('errorIdentifier')
+      end
+
+      context 'when user is allowed to manage file links' do
+        let(:extra_permissions) do
+          %i[view_file_links manage_file_links]
+        end
+
+        it 'creates a work package and assigns the file links', :aggregate_failtures do
+          expect(WorkPackage.count).to eq(1)
+          work_package = WorkPackage.first
+          expect(work_package.file_links).to eq([file_link])
+          expect(work_package.file_links.first.container_type).to eq('WorkPackage')
+          expect(last_response.body).to be_json_eql(
+            api_v3_paths.file_links(work_package.id).to_json
+          ).at_path('_links/fileLinks/href')
+          expect(last_response.body).to be_json_eql("1").at_path('_embedded/fileLinks/total')
+        end
+      end
+    end
   end
 end