diff --git a/app/contracts/work_packages/create_contract.rb b/app/contracts/work_packages/create_contract.rb index 4786eab80b..461c291fe7 100644 --- a/app/contracts/work_packages/create_contract.rb +++ b/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 diff --git a/app/models/work_package.rb b/app/models/work_package.rb index a8ca34007d..e637d2767e 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -57,8 +57,8 @@ class WorkPackage < ApplicationRecord has_many :time_entries, dependent: :delete_all - has_many :file_links, - dependent: :delete_all, class_name: 'Storages::FileLink', foreign_key: 'container_id', inverse_of: :container + has_many :file_links, dependent: :delete_all, class_name: 'Storages::FileLink', as: :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) - result + write_attribute(:type_id, tid) 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 diff --git a/app/services/work_packages/create_service.rb b/app/services/work_packages/create_service.rb index 629051c95d..ef5650b73d 100644 --- a/app/services/work_packages/create_service.rb +++ b/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,11 +51,13 @@ class WorkPackages::CreateService < ::BaseServices::BaseCallable def create(attributes, work_package) result = set_attributes(attributes, work_package) - result.success = if result.success - replace_attachments(work_package) - else - false - end + result.success = + if result.success + work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements + work_package.save + else + false + end if result.success? result.merge!(reschedule_related(work_package)) @@ -80,16 +82,10 @@ 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:, - work_package:) - .call + .new(user:, work_package:) + .call result.self_and_dependent.each do |r| unless r.result.save diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 905566821e..5e56ca961d 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/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 diff --git a/docs/api/apiv3/components/schemas/work_package_model.yml b/docs/api/apiv3/components/schemas/work_package_model.yml index 766c83a0c0..ba93583b9a 100644 --- a/docs/api/apiv3/components/schemas/work_package_model.yml +++ b/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 diff --git a/docs/api/apiv3/paths/work_packages.yml b/docs/api/apiv3/paths/work_packages.yml index a318f155c9..f45f79f182 100644 --- a/docs/api/apiv3/paths/work_packages.yml +++ b/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 - it. + - description: Indicates whether properties should be summed up if they support 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: |- diff --git a/lib/api/v3/work_packages/work_package_payload_representer.rb b/lib/api/v3/work_packages/work_package_payload_representer.rb index 204cb4b13d..de053c6214 100644 --- a/lib/api/v3/work_packages/work_package_payload_representer.rb +++ b/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 diff --git a/modules/storages/app/common/storages/peripherals/scopes.rb b/modules/storages/app/common/storages/peripherals/scopes.rb index 42450b7ca2..c41598c780 100644 --- a/modules/storages/app/common/storages/peripherals/scopes.rb +++ b/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 diff --git a/modules/storages/app/models/storages/file_link.rb b/modules/storages/app/models/storages/file_link.rb index 26154f0bb0..383630d686 100644 --- a/modules/storages/app/models/storages/file_link.rb +++ b/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 diff --git a/modules/storages/lib/api/v3/file_links/file_links_api.rb b/modules/storages/lib/api/v3/file_links/file_links_api.rb index dc93572d51..d82185dcec 100644 --- a/modules/storages/lib/api/v3/file_links/file_links_api.rb +++ b/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 diff --git a/modules/storages/lib/api/v3/file_links/work_packages_file_links_api.rb b/modules/storages/lib/api/v3/file_links/work_packages_file_links_api.rb index 159e73479d..9ec106c6ef 100644 --- a/modules/storages/lib/api/v3/file_links/work_packages_file_links_api.rb +++ b/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 - # Get a (potentially huge...) list of all FileLinks for the work package. - file_links = query.results - .where(id: visible_file_links - .where(container_id: @work_package.id, container_type: 'WorkPackage')) - - # 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 - .new(user: current_user) - .call(file_links) - + 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.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 + ::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: ) diff --git a/modules/storages/spec/factories/file_link_factory.rb b/modules/storages/spec/factories/file_link_factory.rb index 272c358f2f..2d9ff9243b 100644 --- a/modules/storages/spec/factories/file_link_factory.rb +++ b/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" } 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 8bbbaf2aa6..f55f86546b 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 @@ -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 diff --git a/spec/requests/api/v3/work_packages/create_resource_spec.rb b/spec/requests/api/v3/work_packages/create_resource_spec.rb index 34fa8509e2..094f7dc7b5 100644 --- a/spec/requests/api/v3/work_packages/create_resource_spec.rb +++ b/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