From c1b82bad00e45f19c8c650db6df20c3d2264cf7a Mon Sep 17 00:00:00 2001 From: Markus Kahl Date: Fri, 3 Jul 2020 13:17:16 +0100 Subject: [PATCH] direct uploads to S3 for attachments including IFC models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Oliver Günther --- Gemfile | 1 + Gemfile.lock | 4 + app/models/attachment.rb | 25 +++ app/uploaders/direct_fog_uploader.rb | 40 ++++ .../attachments/cleanup_uncontainered_job.rb | 5 + .../attachments/finish_direct_upload_job.rb | 53 +++++ config/initializers/carrierwave.rb | 2 + config/initializers/secure_headers.rb | 4 +- docs/development/running-tests/README.md | 16 ++ .../configuration/README.md | 16 ++ frontend/src/app/angular4-modules.ts | 2 + .../op-direct-file-upload.service.ts | 153 ++++++++++++++ .../op-file-upload.service.spec.ts | 2 + .../work-package-filter-values.spec.ts | 2 + .../work_packages/work-package-cache.spec.ts | 4 +- .../common/config/configuration.service.ts | 8 + .../hal/resources/mixins/attachable-mixin.ts | 29 ++- .../resources/work-package-resource.spec.ts | 2 + .../hal/resources/work-package-resource.ts | 12 -- lib/api/errors/not_found.rb | 6 +- .../attachable_representer_mixin.rb | 9 + .../attachment_metadata_representer.rb | 4 + .../attachment_upload_representer.rb | 158 ++++++++++++++ lib/api/v3/attachments/attachments_api.rb | 30 ++- .../attachments_by_container_api.rb | 58 ++++- .../v3/attachments/attachments_by_post_api.rb | 4 + .../attachments_by_wiki_page_api.rb | 4 + .../attachments_by_work_package_api.rb | 4 + .../configuration_representer.rb | 9 + lib/api/v3/utilities/path_helper.rb | 11 + lib/open_project/configuration.rb | 4 + lib/open_project/configuration/helpers.rb | 28 +++ .../contracts/bim/ifc_models/base_contract.rb | 8 +- .../bim/ifc_models/ifc_models_controller.rb | 87 +++++++- .../bim/ifc_models/set_attributes_service.rb | 14 +- .../bim/ifc_models/ifc_models/_form.html.erb | 52 ++++- modules/bim/config/routes.rb | 2 + .../features/bcf/direct_ifc_upload_spec.rb | 57 +++++ .../v3/attachments/attachments_by_grid_api.rb | 4 + .../attachments_by_meeting_content_api.rb | 4 + .../attachments/attachment_upload_spec.rb | 105 +++++---- .../attachment_metadata_representer_spec.rb | 23 +- spec/lib/open_project/configuration_spec.rb | 35 +++ spec/models/attachment_spec.rb | 33 +-- spec/rails_helper.rb | 6 + .../attachment_resource_shared_examples.rb | 111 ++++++++++ spec/requests/api/v3/attachments_spec.rb | 103 +++++++++ spec/support/carrierwave.rb | 38 ++-- spec/support/shared/with_config.rb | 76 ++++--- spec/support/shared/with_direct_uploads.rb | 199 ++++++++++++++++++ spec/views/layouts/base.html.erb_spec.rb | 13 ++ ...anup_uncontainered_job_integration_spec.rb | 16 +- 52 files changed, 1551 insertions(+), 144 deletions(-) create mode 100644 app/uploaders/direct_fog_uploader.rb create mode 100644 app/workers/attachments/finish_direct_upload_job.rb create mode 100644 frontend/src/app/components/api/op-file-upload/op-direct-file-upload.service.ts create mode 100644 lib/api/v3/attachments/attachment_upload_representer.rb create mode 100644 modules/bim/spec/features/bcf/direct_ifc_upload_spec.rb create mode 100644 spec/requests/api/v3/attachments_spec.rb create mode 100644 spec/support/shared/with_direct_uploads.rb diff --git a/Gemfile b/Gemfile index 7e51a1cde7..9e830a1b7f 100644 --- a/Gemfile +++ b/Gemfile @@ -169,6 +169,7 @@ gem 'puma', '~> 4.3.5' # used for development and optionally for production gem 'nokogiri', '~> 1.10.8' gem 'carrierwave', '~> 1.3.1' +gem 'carrierwave_direct', '~> 2.1.0' gem 'fog-aws' gem 'aws-sdk-core', '~> 3.91.0' diff --git a/Gemfile.lock b/Gemfile.lock index e41cad4613..2565d4cd56 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -334,6 +334,9 @@ GEM activemodel (>= 4.0.0) activesupport (>= 4.0.0) mime-types (>= 1.16) + carrierwave_direct (2.1.0) + carrierwave (>= 1.0.0) + fog-aws cells (4.1.7) declarative-builder (< 0.2.0) declarative-option (< 0.2.0) @@ -989,6 +992,7 @@ DEPENDENCIES capybara (~> 3.32.0) capybara-screenshot (~> 1.0.17) carrierwave (~> 1.3.1) + carrierwave_direct (~> 2.1.0) cells-erb (~> 0.1.0) cells-rails (~> 0.0.9) commonmarker (~> 0.21.0) diff --git a/app/models/attachment.rb b/app/models/attachment.rb index a44d18cd1e..ba9cc36cf5 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -251,6 +251,31 @@ class Attachment < ApplicationRecord end end + def self.pending_direct_uploads + where(digest: "", downloads: -1) + end + + def self.create_pending_direct_upload(file_name:, author:, container: nil, content_type: nil, file_size: 0) + a = create( + container: container, + author: author, + content_type: content_type || "application/octet-stream", + filesize: file_size, + digest: "", + downloads: -1 + ) + + # We need to use update_column because `file` is an uploader which expects a File (not a string) + # to upload usually. But in this case the data has already been uploaded and we just point to it. + a.update_column :file, file_name + + a.reload + end + + def pending_direct_upload? + digest == "" && downloads == -1 + end + private def schedule_cleanup_uncontainered_job diff --git a/app/uploaders/direct_fog_uploader.rb b/app/uploaders/direct_fog_uploader.rb new file mode 100644 index 0000000000..f4ddd3ac03 --- /dev/null +++ b/app/uploaders/direct_fog_uploader.rb @@ -0,0 +1,40 @@ +require_relative 'fog_file_uploader' + +class DirectFogUploader < FogFileUploader + include CarrierWaveDirect::Uploader + + def self.for_attachment(attachment) + for_uploader attachment.file + end + + def self.for_uploader(fog_file_uploader) + raise ArgumentError, "FogFileUploader expected" unless fog_file_uploader.is_a? FogFileUploader + + uploader = self.new + + uploader.instance_variable_set "@file", fog_file_uploader.file + uploader.instance_variable_set "@key", fog_file_uploader.path + + uploader + end + + def self.direct_fog_hash(attachment:, success_action_redirect: nil, success_action_status: "201") + uploader = for_attachment attachment + + if success_action_redirect.present? + uploader.success_action_redirect = success_action_redirect + uploader.use_action_status = false + else + uploader.success_action_status = success_action_status + uploader.use_action_status = true + end + + hash = uploader.direct_fog_hash(enforce_utf8: false) + + if success_action_redirect.present? + hash.merge(success_action_redirect: success_action_redirect) + else + hash.merge(success_action_status: success_action_status) + end + end +end diff --git a/app/workers/attachments/cleanup_uncontainered_job.rb b/app/workers/attachments/cleanup_uncontainered_job.rb index 7c599a3b0c..5bba5cff87 100644 --- a/app/workers/attachments/cleanup_uncontainered_job.rb +++ b/app/workers/attachments/cleanup_uncontainered_job.rb @@ -36,6 +36,11 @@ class Attachments::CleanupUncontaineredJob < ApplicationJob .where(container: nil) .where(too_old) .destroy_all + + Attachment + .pending_direct_uploads + .where(too_old) + .destroy_all # prepared direct uploads that never finished end private diff --git a/app/workers/attachments/finish_direct_upload_job.rb b/app/workers/attachments/finish_direct_upload_job.rb new file mode 100644 index 0000000000..f4b0cd874a --- /dev/null +++ b/app/workers/attachments/finish_direct_upload_job.rb @@ -0,0 +1,53 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2020 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +class Attachments::FinishDirectUploadJob < ApplicationJob + queue_with_priority :high + + def perform(attachment_id) + attachment = Attachment.pending_direct_uploads.where(id: attachment_id).first + local_file = attachment && attachment.file.local_file + + if local_file.nil? + return Rails.logger.error("File for attachment #{attachment_id} was not uploaded.") + end + + begin + attachment.downloads = 0 + attachment.set_file_size local_file unless attachment.filesize && attachment.filesize > 0 + attachment.set_content_type local_file unless attachment.content_type.present? + attachment.set_digest local_file unless attachment.digest.present? + + attachment.save! if attachment.changed? + ensure + File.unlink(local_file.path) if File.exist?(local_file.path) + end + end +end diff --git a/config/initializers/carrierwave.rb b/config/initializers/carrierwave.rb index dab69c87e2..49dcc4a54d 100644 --- a/config/initializers/carrierwave.rb +++ b/config/initializers/carrierwave.rb @@ -47,6 +47,8 @@ module CarrierWave config.fog_credentials = { provider: provider }.merge(credentials) config.fog_directory = directory config.fog_public = public + + config.use_action_status = true end end end diff --git a/config/initializers/secure_headers.rb b/config/initializers/secure_headers.rb index aeffb0ce84..dfb343a85b 100644 --- a/config/initializers/secure_headers.rb +++ b/config/initializers/secure_headers.rb @@ -21,7 +21,7 @@ SecureHeaders::Configuration.default do |config| frame_src << OpenProject::Configuration[:security_badge_url] # Default src - default_src = %w('self') + default_src = %w('self') + [OpenProject::Configuration.remote_storage_host].compact # Allow requests to CLI in dev mode connect_src = default_src @@ -56,7 +56,7 @@ SecureHeaders::Configuration.default do |config| # Allow fonts from self, asset host, or DATA uri font_src: assets_src + %w(data:), # Form targets can only be self - form_action: %w('self'), + form_action: default_src, # Allow iframe from vimeo (welcome video) frame_src: frame_src + %w('self'), frame_ancestors: %w('self'), diff --git a/docs/development/running-tests/README.md b/docs/development/running-tests/README.md index d3ee9df1aa..5df82f4351 100644 --- a/docs/development/running-tests/README.md +++ b/docs/development/running-tests/README.md @@ -72,6 +72,22 @@ Due to flaky test results on Travis (`No output has been received in the last 10 Firefox tests through Selenium are run with Chrome as `--headless` by default. To override this and watch the Chrome instance set the ENV variable `OPENPROJECT_TESTING_NO_HEADLESS=1`. +##### Troubleshooting + +``` +Failure/Error: raise ActionController::RoutingError, "No route matches [#{env['REQUEST_METHOD']}] #{env['PATH_INFO'].inspect}" + + ActionController::RoutingError: + No route matches [GET] "/javascripts/locales/en.js" +``` + +If you get an error like this when running feature specs it means your assets have not been built. +You can fix this either by accessing a page locally (if the rails server is running) once or by precompiling the assets like this: + +``` +bundle exec rake assets:precompile +``` + ### Cucumber **Note:** *We do not write new cucumber features. The current plan is to move away from diff --git a/docs/installation-and-operations/configuration/README.md b/docs/installation-and-operations/configuration/README.md index aa12eea045..c8af4b70bc 100644 --- a/docs/installation-and-operations/configuration/README.md +++ b/docs/installation-and-operations/configuration/README.md @@ -38,6 +38,7 @@ Configuring OpenProject through environment variables is detailed [in this separ * [`omniauth_direct_login_provider`](#omniauth-direct-login-provider) (default: nil) * [`disable_password_login`](#disable-password-login) (default: false) * [`attachments_storage`](#attachments-storage) (default: file) +* [`direct_uploads`](#direct-uploads) (default: true) * [`hidden_menu_items`](#hidden-menu-items) (default: {}) * [`disabled_modules`](#disabled-modules) (default: []) * [`blacklisted_routes`](#blacklisted-routes) (default: []) @@ -172,6 +173,21 @@ In the case of fog you only have to configure everything under `fog`, however. D to `fog` just yet. Instead leave it as `file`. This is because the current attachments storage is used as the source for the migration. +### direct uploads + +*default: true* + +When using fog attachments uploaded in the frontend will be posted directly +to the cloud rather than going through the OpenProject servers. This allows large attachments to be uploaded +without the need to increase the `client_max_body_size` for the proxy in front of OpenProject. +Also it prevents web processes from being blocked through long uploads. + +If, for what ever reason, this is undesirable, you can disable this option. +In that case attachments will be posted as usual to the OpenProject server which then uploads the file +to the remote storage in an extra step. + +**Note**: This only works for S3 right now. When using fog with another provider this configuration will be `false`. The same goes for when no fog storage is configured. + ### Overriding the help link You can override the default help menu of OpenProject by specifying a `force_help_link` option to diff --git a/frontend/src/app/angular4-modules.ts b/frontend/src/app/angular4-modules.ts index 65a6f77046..25338624f9 100644 --- a/frontend/src/app/angular4-modules.ts +++ b/frontend/src/app/angular4-modules.ts @@ -48,6 +48,7 @@ import {OpenprojectPluginsModule} from "core-app/modules/plugins/openproject-plu import {ConfirmFormSubmitController} from "core-components/modals/confirm-form-submit/confirm-form-submit.directive"; import {ProjectMenuAutocompleteComponent} from "core-components/projects/project-menu-autocomplete/project-menu-autocomplete.component"; import {OpenProjectFileUploadService} from "core-components/api/op-file-upload/op-file-upload.service"; +import {OpenProjectDirectFileUploadService} from './components/api/op-file-upload/op-direct-file-upload.service'; import {LinkedPluginsModule} from "core-app/modules/plugins/linked-plugins.module"; import {HookService} from "core-app/modules/plugins/hook-service"; import {DynamicBootstrapper} from "core-app/globals/dynamic-bootstrapper"; @@ -143,6 +144,7 @@ import {RevitAddInSettingsButtonService} from "core-app/modules/bim/revit_add_in { provide: APP_INITIALIZER, useFactory: initializeServices, deps: [Injector], multi: true }, PaginationService, OpenProjectFileUploadService, + OpenProjectDirectFileUploadService, // Split view CommentService, ConfirmDialogService, diff --git a/frontend/src/app/components/api/op-file-upload/op-direct-file-upload.service.ts b/frontend/src/app/components/api/op-file-upload/op-direct-file-upload.service.ts new file mode 100644 index 0000000000..700460c65d --- /dev/null +++ b/frontend/src/app/components/api/op-file-upload/op-direct-file-upload.service.ts @@ -0,0 +1,153 @@ +//-- copyright +// OpenProject is an open source project management software. +// Copyright (C) 2012-2020 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 docs/COPYRIGHT.rdoc for more details. +//++ + +import {Injectable} from "@angular/core"; +import {HttpEvent, HttpResponse} from "@angular/common/http"; +import {HalResource} from "core-app/modules/hal/resources/hal-resource"; +import {from, Observable, of} from "rxjs"; +import {share, switchMap} from "rxjs/operators"; +import {OpenProjectFileUploadService, UploadBlob, UploadFile, UploadInProgress} from './op-file-upload.service'; + +interface PrepareUploadResult { + url:string; + form:FormData; + response:any; +} + +@Injectable() +export class OpenProjectDirectFileUploadService extends OpenProjectFileUploadService { + /** + * Upload a single file, get an UploadResult observable + * @param {string} url + * @param {UploadFile} file + * @param {string} method + */ + public uploadSingle(url:string, file:UploadFile|UploadBlob, method:string = 'post', responseType:'text'|'json' = 'text') { + const observable = from(this.getDirectUploadFormFrom(url, file)) + .pipe( + switchMap(this.uploadToExternal(file, method, responseType)), + share() + ); + + return [file, observable] as UploadInProgress; + } + + private uploadToExternal(file:UploadFile|UploadBlob, method:string, responseType:string):(result:PrepareUploadResult) => Observable> { + return result => { + result.form.append('file', file, file.customName || file.name); + + return this + .http + .request( + method, + result.url, + { + body: result.form, + // Observe the response, not the body + observe: 'events', + // This is important as the CORS policy for the bucket is * and you can't use credentals then, + // besides we don't need them here anyway. + withCredentials: false, + responseType: responseType as any, + // Subscribe to progress events. subscribe() will fire multiple times! + reportProgress: true + } + ) + .pipe(switchMap(this.finishUpload(result))); + }; + } + + private finishUpload(result:PrepareUploadResult):(result:HttpEvent) => Observable> { + return event => { + if (event instanceof HttpResponse) { + return this + .http + .get( + result.response._links.completeUpload.href, + { + observe: 'response' + } + ); + } + + // Return as new observable due to switchMap + return of(event); + }; + } + + public getDirectUploadFormFrom(url:string, file:UploadFile|UploadBlob):Promise { + const formData = new FormData(); + const metadata = { + description: file.description, + fileName: file.customName || file.name, + fileSize: file.size, + contentType: file.type + }; + + /* + * @TODO We could calculate the MD5 hash here too and pass that. + * The MD5 hash can be used as the `content-md5` option during the upload to S3 for instance. + * This way S3 can verify the integrity of the file which we currently don't do. + */ + + // add the metadata object + formData.append( + 'metadata', + JSON.stringify(metadata), + ); + + const result = this + .http + .request( + "post", + url, + { + body: formData, + withCredentials: true, + responseType: "json" as any + } + ) + .toPromise() + .then((res) => { + let form = new FormData(); + + _.each(res._links.addAttachment.form_fields, (value, key) => { + form.append(key, value); + }); + + return { url: res._links.addAttachment.href, form: form, response: res }; + }) + .catch((err) => { + console.log(err); + + return new FormData(); + }); + + return result; + } +} diff --git a/frontend/src/app/components/api/op-file-upload/op-file-upload.service.spec.ts b/frontend/src/app/components/api/op-file-upload/op-file-upload.service.spec.ts index 7d0d5716f3..d65743c14f 100644 --- a/frontend/src/app/components/api/op-file-upload/op-file-upload.service.spec.ts +++ b/frontend/src/app/components/api/op-file-upload/op-file-upload.service.spec.ts @@ -27,6 +27,7 @@ //++ import {OpenProjectFileUploadService, UploadFile, UploadResult} from './op-file-upload.service'; +import {OpenProjectDirectFileUploadService} from "core-components/api/op-file-upload/op-direct-file-upload.service"; import {HttpClientTestingModule, HttpTestingController} from "@angular/common/http/testing"; import {getTestBed, TestBed} from "@angular/core/testing"; import {HalResourceService} from "core-app/modules/hal/services/hal-resource.service"; @@ -45,6 +46,7 @@ describe('opFileUpload service', () => { {provide: States, useValue: new States()}, I18nService, OpenProjectFileUploadService, + OpenProjectDirectFileUploadService, HalResourceService ] }); diff --git a/frontend/src/app/components/wp-edit-form/work-package-filter-values.spec.ts b/frontend/src/app/components/wp-edit-form/work-package-filter-values.spec.ts index 35038b99a1..c548f7b677 100644 --- a/frontend/src/app/components/wp-edit-form/work-package-filter-values.spec.ts +++ b/frontend/src/app/components/wp-edit-form/work-package-filter-values.spec.ts @@ -48,6 +48,7 @@ import {PathHelperService} from "core-app/modules/common/path-helper/path-helper import {UIRouterModule} from "@uirouter/angular"; import {LoadingIndicatorService} from "core-app/modules/common/loading-indicator/loading-indicator.service"; import {OpenProjectFileUploadService} from "core-components/api/op-file-upload/op-file-upload.service"; +import {OpenProjectDirectFileUploadService} from "core-components/api/op-file-upload/op-direct-file-upload.service"; import {HookService} from "core-app/modules/plugins/hook-service"; import {IsolatedQuerySpace} from "core-app/modules/work_packages/query-space/isolated-query-space"; import {HalEventsService} from "core-app/modules/hal/services/hal-events.service"; @@ -83,6 +84,7 @@ describe('WorkPackageFilterValues', () => { CurrentUserService, HookService, OpenProjectFileUploadService, + OpenProjectDirectFileUploadService, LoadingIndicatorService, HalResourceService, NotificationsService, diff --git a/frontend/src/app/modules/apiv3/endpoints/work_packages/work-package-cache.spec.ts b/frontend/src/app/modules/apiv3/endpoints/work_packages/work-package-cache.spec.ts index 6c342ac97e..32078c47b5 100644 --- a/frontend/src/app/modules/apiv3/endpoints/work_packages/work-package-cache.spec.ts +++ b/frontend/src/app/modules/apiv3/endpoints/work_packages/work-package-cache.spec.ts @@ -35,6 +35,7 @@ import {OpenprojectHalModule} from 'core-app/modules/hal/openproject-hal.module' import {WorkPackageResource} from 'core-app/modules/hal/resources/work-package-resource'; import {HalResourceService} from 'core-app/modules/hal/services/hal-resource.service'; import {OpenProjectFileUploadService} from 'core-components/api/op-file-upload/op-file-upload.service'; +import {OpenProjectDirectFileUploadService} from "core-components/api/op-file-upload/op-direct-file-upload.service"; import {SchemaCacheService} from 'core-components/schemas/schema-cache.service'; import {States} from 'core-components/states.service'; import {HalResourceNotificationService} from "core-app/modules/hal/services/hal-resource-notification.service"; @@ -70,7 +71,8 @@ describe('WorkPackageCache', () => { {provide: NotificationsService, useValue: {}}, {provide: HalResourceNotificationService, useValue: {handleRawError: () => false}}, {provide: WorkPackageNotificationService, useValue: {}}, - {provide: OpenProjectFileUploadService, useValue: {}} + {provide: OpenProjectFileUploadService, useValue: {}}, + {provide: OpenProjectDirectFileUploadService, useValue: {}}, ] }); diff --git a/frontend/src/app/modules/common/config/configuration.service.ts b/frontend/src/app/modules/common/config/configuration.service.ts index 9b8d4c4613..be4c37c345 100644 --- a/frontend/src/app/modules/common/config/configuration.service.ts +++ b/frontend/src/app/modules/common/config/configuration.service.ts @@ -65,6 +65,14 @@ export class ConfigurationService { return this.userPreference('timeZone'); } + public isDirectUploads() { + return !!this.prepareAttachmentURL; + } + + public get prepareAttachmentURL() { + return _.get(this.configuration, ['prepareAttachment', 'href']); + } + public get maximumAttachmentFileSize() { return this.systemPreference('maximumAttachmentFileSize'); } diff --git a/frontend/src/app/modules/hal/resources/mixins/attachable-mixin.ts b/frontend/src/app/modules/hal/resources/mixins/attachable-mixin.ts index df07cf0e6c..1841792b22 100644 --- a/frontend/src/app/modules/hal/resources/mixins/attachable-mixin.ts +++ b/frontend/src/app/modules/hal/resources/mixins/attachable-mixin.ts @@ -32,8 +32,10 @@ import {OpenProjectFileUploadService, UploadFile} from 'core-components/api/op-f import {HalResourceNotificationService} from "core-app/modules/hal/services/hal-resource-notification.service"; import {PathHelperService} from 'core-app/modules/common/path-helper/path-helper.service'; import {NotificationsService} from 'core-app/modules/common/notifications/notifications.service'; +import {ConfigurationService} from 'core-app/modules/common/config/configuration.service'; import {HttpErrorResponse} from "@angular/common/http"; import {APIV3Service} from "core-app/modules/apiv3/api-v3.service"; +import { OpenProjectDirectFileUploadService } from 'core-app/components/api/op-file-upload/op-direct-file-upload.service'; type Constructor = new (...args:any[]) => T; @@ -44,8 +46,10 @@ export function Attachable>(Base:TBase) { private NotificationsService:NotificationsService; private halNotification:HalResourceNotificationService; private opFileUpload:OpenProjectFileUploadService; + private opDirectFileUpload:OpenProjectDirectFileUploadService; private pathHelper:PathHelperService; private apiV3Service:APIV3Service; + private config:ConfigurationService; /** * Can be used in the mixed in class to disable @@ -168,9 +172,11 @@ export function Attachable>(Base:TBase) { } private performUpload(files:UploadFile[]) { - let href = ''; + let href: string = this.directUploadURL || ''; - if (this.isNew || !this.id || !this.attachmentsBackend) { + if (href) { + return this.opDirectFileUpload.uploadAndMapResponse(href, files); + } else if (this.isNew || !this.id || !this.attachmentsBackend) { href = this.apiV3Service.attachments.path; } else { href = this.addAttachment.$link.href; @@ -179,6 +185,18 @@ export function Attachable>(Base:TBase) { return this.opFileUpload.uploadAndMapResponse(href, files); } + private get directUploadURL():string|null { + if (this.$links.prepareAttachment) { + return this.$links.prepareAttachment.href; + } + + if (this.isNew) { + return this.config.prepareAttachmentURL + } else { + return null; + } + } + private updateState() { if (this.state) { this.state.putValue(this as any); @@ -195,7 +213,12 @@ export function Attachable>(Base:TBase) { if (!this.opFileUpload) { this.opFileUpload = this.injector.get(OpenProjectFileUploadService); } - + if (!this.opDirectFileUpload) { + this.opDirectFileUpload = this.injector.get(OpenProjectDirectFileUploadService); + } + if (!this.config) { + this.config = this.injector.get(ConfigurationService); + } if (!this.pathHelper) { this.pathHelper = this.injector.get(PathHelperService); } diff --git a/frontend/src/app/modules/hal/resources/work-package-resource.spec.ts b/frontend/src/app/modules/hal/resources/work-package-resource.spec.ts index a0cd64a9ed..169b4524d1 100644 --- a/frontend/src/app/modules/hal/resources/work-package-resource.spec.ts +++ b/frontend/src/app/modules/hal/resources/work-package-resource.spec.ts @@ -42,6 +42,7 @@ import {ConfigurationService} from 'core-app/modules/common/config/configuration import {I18nService} from "core-app/modules/common/i18n/i18n.service"; import {StateService} from "@uirouter/core"; import {OpenProjectFileUploadService} from "core-components/api/op-file-upload/op-file-upload.service"; +import {OpenProjectDirectFileUploadService} from "core-components/api/op-file-upload/op-direct-file-upload.service"; import {WorkPackageCreateService} from 'core-app/components/wp-new/wp-create.service'; import {WorkPackageNotificationService} from "core-app/modules/work_packages/notifications/work-package-notification.service"; import {WorkPackagesActivityService} from "core-components/wp-single-view-tabs/activity-panel/wp-activity.service"; @@ -76,6 +77,7 @@ describe('WorkPackage', () => { NotificationsService, ConfigurationService, OpenProjectFileUploadService, + OpenProjectDirectFileUploadService, LoadingIndicatorService, PathHelperService, I18nService, diff --git a/frontend/src/app/modules/hal/resources/work-package-resource.ts b/frontend/src/app/modules/hal/resources/work-package-resource.ts index 27eb7d7816..92dcbf718b 100644 --- a/frontend/src/app/modules/hal/resources/work-package-resource.ts +++ b/frontend/src/app/modules/hal/resources/work-package-resource.ts @@ -178,18 +178,6 @@ export class WorkPackageBaseResource extends HalResource { return fieldName === 'description' ? 'full' : 'constrained'; } - private performUpload(files:UploadFile[]) { - let href = ''; - - if (this.isNew) { - href = this.apiV3Service.attachments.path; - } else { - href = this.attachments.$href!; - } - - return this.opFileUpload.uploadAndMapResponse(href, files); - } - public isParentOf(otherWorkPackage:WorkPackageResource) { return otherWorkPackage.parent?.$links.self.$link.href === this.$links.self.$link.href; } diff --git a/lib/api/errors/not_found.rb b/lib/api/errors/not_found.rb index 395a836263..4894532573 100644 --- a/lib/api/errors/not_found.rb +++ b/lib/api/errors/not_found.rb @@ -33,8 +33,10 @@ module API identifier 'NotFound' code 404 - def initialize(*) - super I18n.t('api_v3.errors.code_404') + def initialize(*args) + opts = args.last.is_a?(Hash) ? args.last : {} + + super opts[:message] || I18n.t('api_v3.errors.code_404') end end end diff --git a/lib/api/v3/attachments/attachable_representer_mixin.rb b/lib/api/v3/attachments/attachable_representer_mixin.rb index a3d92d9ce7..264f1f64bb 100644 --- a/lib/api/v3/attachments/attachable_representer_mixin.rb +++ b/lib/api/v3/attachments/attachable_representer_mixin.rb @@ -43,6 +43,15 @@ module API } end + link :prepareAttachment do + next unless OpenProject::Configuration.direct_uploads? + + { + href: attachments_by_resource + '/prepare', + method: :post + } + end + link :addAttachment, cache_if: -> do represented.attachments_addable?(current_user) diff --git a/lib/api/v3/attachments/attachment_metadata_representer.rb b/lib/api/v3/attachments/attachment_metadata_representer.rb index 4ca101f060..422fc7166b 100644 --- a/lib/api/v3/attachments/attachment_metadata_representer.rb +++ b/lib/api/v3/attachments/attachment_metadata_representer.rb @@ -46,6 +46,10 @@ module API }, setter: ->(fragment:, **) { self.description = fragment['raw'] }, render_nil: true + + property :content_type, render_nil: false + property :file_size, render_nil: false + property :digest, render_nil: false end end end diff --git a/lib/api/v3/attachments/attachment_upload_representer.rb b/lib/api/v3/attachments/attachment_upload_representer.rb new file mode 100644 index 0000000000..fa186bc6e8 --- /dev/null +++ b/lib/api/v3/attachments/attachment_upload_representer.rb @@ -0,0 +1,158 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2020 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +require 'roar/decorator' +require 'roar/json/hal' + +module API + module V3 + module Attachments + class AttachmentUploadRepresenter < ::API::Decorators::Single + include API::Decorators::DateProperty + include API::Decorators::FormattableProperty + include API::Decorators::LinkedResource + + self_link title_getter: ->(*) { represented.filename } + + associated_resource :author, + v3_path: :user, + representer: ::API::V3::Users::UserRepresenter + + def self.associated_container_getter + ->(*) do + next unless embed_links && container_representer + + container_representer + .new(represented.container, current_user: current_user) + end + end + + def self.associated_container_link + ->(*) do + return nil unless v3_container_name == 'nil_class' || api_v3_paths.respond_to?(v3_container_name) + + ::API::Decorators::LinkObject + .new(represented, + path: v3_container_name, + property_name: :container, + title_attribute: container_title_attribute) + .to_hash + end + end + + attr_reader :form_url + attr_reader :form_fields + + attr_reader :attachment + + def initialize(attachment, options = {}) + super + + fog_hash = DirectFogUploader.direct_fog_hash attachment: attachment + + @form_url = fog_hash[:uri] + @form_fields = fog_hash.except :uri + @attachment = attachment + end + + associated_resource :container, + getter: associated_container_getter, + link: associated_container_link + + link :addAttachment do + { + href: form_url, + method: :post, + form_fields: form_fields + } + end + + link :delete do + { + href: api_v3_paths.attachment_upload(represented.id), + method: :delete + } + end + + link :staticDownloadLocation do + { + href: api_v3_paths.attachment_content(attachment.id) + } + end + + link :downloadLocation do + location = if attachment.external_storage? + attachment.external_url + else + api_v3_paths.attachment_content(attachment.id) + end + { + href: location + } + end + + link :completeUpload do + { + href: api_v3_paths.attachment_uploaded(attachment.id) + } + end + + property :id + property :file_name, + getter: ->(*) { filename } + + formattable_property :description, + plain: true + + date_time_property :created_at + + def _type + 'AttachmentUpload' + end + + def container_representer + name = v3_container_name.camelcase + + "::API::V3::#{name.pluralize}::#{name}Representer".constantize + rescue NameError + nil + end + + def v3_container_name + ::API::Utilities::PropertyNameConverter.from_ar_name(represented.container.class.name.underscore).underscore + end + + def container_title_attribute + represented.container.respond_to?(:subject) ? :subject : :title + end + end + end + end +end diff --git a/lib/api/v3/attachments/attachments_api.rb b/lib/api/v3/attachments/attachments_api.rb index 6cfc1a6667..384cbaac23 100644 --- a/lib/api/v3/attachments/attachments_api.rb +++ b/lib/api/v3/attachments/attachments_api.rb @@ -39,13 +39,25 @@ module API def container nil end + + def check_attachments_addable + raise API::Errors::Unauthorized if Redmine::Acts::Attachable.attachables.none?(&:attachments_addable?) + end end post do - raise API::Errors::Unauthorized if Redmine::Acts::Attachable.attachables.none?(&:attachments_addable?) + check_attachments_addable - ::API::V3::Attachments::AttachmentRepresenter.new(parse_and_create, - current_user: current_user) + ::API::V3::Attachments::AttachmentRepresenter.new(parse_and_create, current_user: current_user) + end + + namespace :prepare do + post do + require_direct_uploads + check_attachments_addable + + ::API::V3::Attachments::AttachmentUploadRepresenter.new(parse_and_prepare, current_user: current_user) + end end route_param :id, type: Integer, desc: 'Attachment ID' do @@ -70,6 +82,18 @@ module API respond_with_attachment @attachment, cache_seconds: 604799 end end + + namespace :uploaded do + get do + attachment = Attachment.pending_direct_uploads.where(id: params[:id]).first! + + raise API::Errors::NotFound unless attachment.file.readable? + + ::Attachments::FinishDirectUploadJob.perform_later attachment.id + + ::API::V3::Attachments::AttachmentRepresenter.new(attachment, current_user: current_user) + end + end end end end diff --git a/lib/api/v3/attachments/attachments_by_container_api.rb b/lib/api/v3/attachments/attachments_by_container_api.rb index 4e987ed9f7..10ee4be59a 100644 --- a/lib/api/v3/attachments/attachments_by_container_api.rb +++ b/lib/api/v3/attachments/attachments_by_container_api.rb @@ -56,13 +56,40 @@ module API unless metadata.file_name raise ::API::Errors::Validation.new( :file_name, - "fileName #{I18n.t('activerecord.errors.messages.blank')}." + "fileName #{I18n.t('activerecord.errors.messages.blank')}" ) end metadata end + def parse_and_prepare + metadata = parse_metadata params[:metadata] + + unless metadata + raise ::API::Errors::InvalidRequestBody.new(I18n.t('api_v3.errors.multipart_body_error')) + end + + unless metadata.file_size + raise ::API::Errors::Validation.new( + :file_size, + "fileSize #{I18n.t('activerecord.errors.messages.blank')}" + ) + end + + create_attachment metadata + end + + def create_attachment(metadata) + Attachment.create_pending_direct_upload( + file_name: metadata.file_name, + container: container, + author: current_user, + content_type: metadata.content_type, + file_size: metadata.file_size + ) + end + def parse_and_create metadata = parse_metadata params[:metadata] file = params[:file] @@ -87,6 +114,20 @@ module API end end + def check_permissions(permissions) + if permissions.empty? + raise API::Errors::Unauthorized unless container.attachments_addable?(current_user) + else + authorize_any(permissions, projects: container.project) + end + end + + def require_direct_uploads + unless OpenProject::Configuration.direct_uploads? + raise API::Errors::NotFound, message: "Only available if direct uploads are enabled." + end + end + def with_handled_create_errors yield rescue ActiveRecord::RecordInvalid => error @@ -125,16 +166,21 @@ module API def self.create(permissions = []) -> do - if permissions.empty? - raise API::Errors::Unauthorized unless container.attachments_addable?(current_user) - else - authorize_any(permissions, projects: container.project) - end + check_permissions permissions ::API::V3::Attachments::AttachmentRepresenter.new(parse_and_create, current_user: current_user) end end + + def self.prepare(permissions = []) + -> do + require_direct_uploads + check_permissions permissions + + ::API::V3::Attachments::AttachmentUploadRepresenter.new(parse_and_prepare, current_user: current_user) + end + end end end end diff --git a/lib/api/v3/attachments/attachments_by_post_api.rb b/lib/api/v3/attachments/attachments_by_post_api.rb index 99826c7858..31458c554f 100644 --- a/lib/api/v3/attachments/attachments_by_post_api.rb +++ b/lib/api/v3/attachments/attachments_by_post_api.rb @@ -45,6 +45,10 @@ module API get &API::V3::Attachments::AttachmentsByContainerAPI.read post &API::V3::Attachments::AttachmentsByContainerAPI.create([:edit_messages]) + + namespace :prepare do + post &API::V3::Attachments::AttachmentsByContainerAPI.prepare([:edit_messages]) + end end end end diff --git a/lib/api/v3/attachments/attachments_by_wiki_page_api.rb b/lib/api/v3/attachments/attachments_by_wiki_page_api.rb index 0d21dfef26..742c5be6fb 100644 --- a/lib/api/v3/attachments/attachments_by_wiki_page_api.rb +++ b/lib/api/v3/attachments/attachments_by_wiki_page_api.rb @@ -45,6 +45,10 @@ module API get &API::V3::Attachments::AttachmentsByContainerAPI.read post &API::V3::Attachments::AttachmentsByContainerAPI.create + + namespace :prepare do + post &API::V3::Attachments::AttachmentsByContainerAPI.prepare + end end end end diff --git a/lib/api/v3/attachments/attachments_by_work_package_api.rb b/lib/api/v3/attachments/attachments_by_work_package_api.rb index f11325c841..bab4ea4666 100644 --- a/lib/api/v3/attachments/attachments_by_work_package_api.rb +++ b/lib/api/v3/attachments/attachments_by_work_package_api.rb @@ -54,6 +54,10 @@ module API # :edit_work_packages in this endpoint and require clients to upload uncontainered work packages # first and attach them on wp creation. post &API::V3::Attachments::AttachmentsByContainerAPI.create([:edit_work_packages]) + + namespace :prepare do + post &API::V3::Attachments::AttachmentsByContainerAPI.prepare([:edit_work_packages]) + end end end end diff --git a/lib/api/v3/configuration/configuration_representer.rb b/lib/api/v3/configuration/configuration_representer.rb index ff1031b304..864d616049 100644 --- a/lib/api/v3/configuration/configuration_representer.rb +++ b/lib/api/v3/configuration/configuration_representer.rb @@ -46,6 +46,15 @@ module API } end + link :prepareAttachment do + next unless OpenProject::Configuration.direct_uploads? + + { + href: api_v3_paths.prepare_new_attachment_upload, + method: :post + } + end + property :maximum_attachment_file_size, getter: ->(*) { attachment_max_size.to_i.kilobyte } diff --git a/lib/api/v3/utilities/path_helper.rb b/lib/api/v3/utilities/path_helper.rb index 6c07b4e54c..41e8d5017d 100644 --- a/lib/api/v3/utilities/path_helper.rb +++ b/lib/api/v3/utilities/path_helper.rb @@ -123,6 +123,17 @@ module API "#{wiki_page(id)}/attachments" end + def self.prepare_new_attachment_upload + "#{root}/attachments/prepare" + end + + index :attachment_upload + show :attachment_upload + + def self.attachment_uploaded(attachment_id) + "#{root}/attachments/#{attachment_id}/uploaded" + end + def self.available_assignees(project_id) "#{project(project_id)}/available_assignees" end diff --git a/lib/open_project/configuration.rb b/lib/open_project/configuration.rb index 41c195aee9..63b664b6b7 100644 --- a/lib/open_project/configuration.rb +++ b/lib/open_project/configuration.rb @@ -46,6 +46,10 @@ module OpenProject 'autologin_cookie_path' => '/', 'autologin_cookie_secure' => false, 'database_cipher_key' => nil, + # only applicable in conjunction with fog (effectively S3) attachments + # which will be uploaded directly to the cloud storage rather than via OpenProject's + # server process. + 'direct_uploads' => true, 'show_community_links' => true, 'log_level' => 'info', 'scm_git_command' => nil, diff --git a/lib/open_project/configuration/helpers.rb b/lib/open_project/configuration/helpers.rb index b100f8cf63..1f2763208c 100644 --- a/lib/open_project/configuration/helpers.rb +++ b/lib/open_project/configuration/helpers.rb @@ -41,6 +41,20 @@ module OpenProject (self['attachments_storage'] || 'file').to_sym end + ## + # We only allow direct uploads to S3 as we are using the carrierwave_direct + # gem which only supports S3 for the time being. + def direct_uploads + return false unless remote_storage? + return false unless remote_storage_aws? + + self['direct_uploads'] + end + + def direct_uploads? + direct_uploads + end + # Augur connect host def enterprise_trial_creation_host if Rails.env.production? @@ -54,6 +68,20 @@ module OpenProject attachments_storage == :file end + def remote_storage? + attachments_storage == :fog + end + + def remote_storage_aws? + fog_credentials[:provider] == "AWS" + end + + def remote_storage_host + if remote_storage_aws? + "#{fog_directory}.s3.amazonaws.com" + end + end + def attachments_storage_path Rails.root.join(self['attachments_storage_path'] || 'files') end diff --git a/modules/bim/app/contracts/bim/ifc_models/base_contract.rb b/modules/bim/app/contracts/bim/ifc_models/base_contract.rb index f613fc727a..22f2477284 100644 --- a/modules/bim/app/contracts/bim/ifc_models/base_contract.rb +++ b/modules/bim/app/contracts/bim/ifc_models/base_contract.rb @@ -68,16 +68,20 @@ module Bim end def ifc_attachment_is_ifc - return unless model.ifc_attachment&.new_record? + return unless model.ifc_attachment&.new_record? || model.ifc_attachment&.pending_direct_upload? - firstline = File.open(model.ifc_attachment.file.file.path, &:readline) + file_path = model.ifc_attachment.file.local_file.path begin + firstline = File.open(file_path, &:readline) + unless firstline.match?(/^ISO-10303-21;/) errors.add :base, :invalid_ifc_file end rescue ArgumentError errors.add :base, :invalid_ifc_file + ensure + FileUtils.rm file_path if File.exists? file_path end end diff --git a/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb b/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb index 37d2465283..61706953ed 100644 --- a/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb +++ b/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb @@ -33,11 +33,12 @@ module Bim class IfcModelsController < BaseController helper_method :gon - before_action :find_project_by_project_id, only: %i[index new create show defaults edit update destroy] + before_action :find_project_by_project_id, only: %i[index new create show defaults edit update destroy direct_upload_finished] before_action :find_ifc_model_object, only: %i[edit update destroy] before_action :find_all_ifc_models, only: %i[show defaults index] - before_action :authorize + before_action :authorize, except: [:set_direct_upload_file_name, :direct_upload_finished] + skip_before_action :verify_authenticity_token, only: [:set_direct_upload_file_name] menu_item :ifc_models @@ -48,9 +49,25 @@ module Bim def new @ifc_model = @project.ifc_models.build + + if OpenProject::Configuration.direct_uploads? + @pending_upload = Attachment.create_pending_direct_upload(file_name: "model.ifc", author: current_user) + @form = DirectFogUploader.direct_fog_hash( + attachment: @pending_upload, + success_action_redirect: direct_upload_finished_bcf_project_ifc_models_url + ) + end end - def edit; + def edit + if OpenProject::Configuration.direct_uploads? + @pending_upload = Attachment.create_pending_direct_upload(file_name: "model.ifc", author: current_user) + @form = DirectFogUploader.direct_fog_hash( + attachment: @pending_upload, + success_action_redirect: direct_upload_finished_bcf_project_ifc_models_url + ) + session[:pending_ifc_model_ifc_model_id] = @ifc_model.id + end end def show @@ -61,6 +78,70 @@ module Bim frontend_redirect @ifc_models.defaults.pluck(:id).uniq end + def set_direct_upload_file_name + session[:pending_ifc_model_title] = params[:title] + session[:pending_ifc_model_is_default] = params[:isDefault] + end + + def direct_upload_finished + id = request.params[:key].scan(/\/file\/(\d+)\//).flatten.first + attachment = Attachment.pending_direct_uploads.where(id: id).first + + if attachment.nil? # this should not happen + flash[:error] = "Direct upload failed." + + redirect_to action: :new + end + + params = { + title: session[:pending_ifc_model_title], + project: @project, + ifc_attachment: attachment, + is_default: session[:pending_ifc_model_is_default] + } + + new_model = true + + if session[:pending_ifc_model_ifc_model_id] + ifc_model = Bim::IfcModels::IfcModel.find_by id: session[:pending_ifc_model_ifc_model_id] + new_model = false + + call = ::Bim::IfcModels::UpdateService + .new(user: current_user, model: ifc_model) + .call(params.with_indifferent_access) + + @ifc_model = call.result + else + call = ::Bim::IfcModels::CreateService + .new(user: current_user) + .call(params.with_indifferent_access) + + @ifc_model = call.result + end + + session.delete :pending_ifc_model_title + session.delete :pending_ifc_model_is_default + session.delete :pending_ifc_model_ifc_model_id + + if call.success? + ::Attachments::FinishDirectUploadJob.perform_later attachment.id + + if new_model + flash[:notice] = t('ifc_models.flash_messages.upload_successful') + else + flash[:notice] = t(:notice_successful_update) + end + + redirect_to action: :index + else + attachment.destroy + + flash[:error] = call.errors.full_messages.join(" ") + + redirect_to action: :new + end + end + def create combined_params = permitted_model_params .to_h diff --git a/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb b/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb index 2f65b6d1d5..8db71e651b 100644 --- a/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb +++ b/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb @@ -39,7 +39,7 @@ module Bim super change_by_system do - model.uploader = model.ifc_attachment&.author if model.ifc_attachment&.new_record? + model.uploader = model.ifc_attachment&.author if model.ifc_attachment&.new_record? || model.ifc_attachment&.pending_direct_upload? end end @@ -61,14 +61,22 @@ module Bim end def set_title - model.title = model.ifc_attachment&.file&.filename&.gsub(/\.\w+$/, '') + model.title ||= model.ifc_attachment&.file&.filename&.gsub(/\.\w+$/, '') end def set_ifc_attachment(ifc_attachment) return unless ifc_attachment model.attachments.each(&:mark_for_destruction) - model.attach_files('first' => {'file' => ifc_attachment, 'description' => 'ifc'}) + + if ifc_attachment.is_a?(Attachment) + ifc_attachment.description = "ifc" + ifc_attachment.save! unless ifc_attachment.new_record? + + model.attachments << ifc_attachment + else + model.attach_files('first' => {'file' => ifc_attachment, 'description' => 'ifc'}) + end end end end diff --git a/modules/bim/app/views/bim/ifc_models/ifc_models/_form.html.erb b/modules/bim/app/views/bim/ifc_models/ifc_models/_form.html.erb index 72998a2838..674d99685f 100644 --- a/modules/bim/app/views/bim/ifc_models/ifc_models/_form.html.erb +++ b/modules/bim/app/views/bim/ifc_models/ifc_models/_form.html.erb @@ -35,9 +35,59 @@ See doc/COPYRIGHT.rdoc for more details. <%= f.text_field :title, required: true, container_class: '-wide' %> <% end %> + +<% if OpenProject::Configuration.direct_uploads? %> + <% Hash(@form).each do |key, value| %> + + <% end %> +<% end %> +
- <%= f.file_field :ifc_attachment %> + <% if OpenProject::Configuration.direct_uploads? %> + + <% else %> + <%= f.file_field :ifc_attachment %> + <% end %>
+
<%= f.check_box 'is_default' %>
+ +<% if OpenProject::Configuration.direct_uploads? %> + +<%= nonced_javascript_tag do %> + jQuery(document).ready(function() { + jQuery("input[type=file]").change(function(e){ + var fileName = e.target.files[0].name; + + jQuery.post( + "<%= set_direct_upload_file_name_bcf_project_ifc_models_path %>", + { + title: fileName, + isDefault: jQuery("#bim_ifc_models_ifc_model_is_default").is(":checked") ? 1 : 0 + } + ); + + // rebuild form to post to S3 directly + if (jQuery("input[name=utf8]").length == 1) { + jQuery("input[name=utf8]").remove(); + jQuery("input[name=authenticity_token]").remove(); + jQuery("input[name=_method]").remove(); + + var url = jQuery("input[name=uri]").val(); + + jQuery("form").attr("action", url); + jQuery("form").attr("enctype", "multipart/form-data"); + + jQuery("input[name=uri]").remove(); + + jQuery("form").submit(function() { + jQuery("#bim_ifc_models_ifc_model_title").prop("disabled", "disabled"); + }); + } + }); + }); +<% end %> + +<% end %> diff --git a/modules/bim/config/routes.rb b/modules/bim/config/routes.rb index edfeb49978..1174d96f6c 100644 --- a/modules/bim/config/routes.rb +++ b/modules/bim/config/routes.rb @@ -46,6 +46,8 @@ OpenProject::Application.routes.draw do resources :ifc_models, controller: 'bim/ifc_models/ifc_models' do collection do get :defaults + get :direct_upload_finished + post :set_direct_upload_file_name end end end diff --git a/modules/bim/spec/features/bcf/direct_ifc_upload_spec.rb b/modules/bim/spec/features/bcf/direct_ifc_upload_spec.rb new file mode 100644 index 0000000000..3a3878a94f --- /dev/null +++ b/modules/bim/spec/features/bcf/direct_ifc_upload_spec.rb @@ -0,0 +1,57 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2020 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe 'direct IFC upload', type: :feature, js: true, with_direct_uploads: :redirect, with_config: { edition: 'bim' } do + let(:user) { FactoryBot.create :admin } + let(:project) { FactoryBot.create :project, enabled_module_names: %i[bim] } + let(:ifc_fixture) { Rails.root.join('modules/bim/spec/fixtures/files/minimal.ifc') } + + before do + login_as user + + allow_any_instance_of(Bim::IfcModels::BaseContract).to receive(:ifc_attachment_is_ifc).and_return true + end + + it 'should work' do + visit new_bcf_project_ifc_model_path(project_id: project.identifier) + + page.attach_file("file", ifc_fixture, visible: :all) + + click_on "Create" + + expect(page).to have_content("Upload succeeded") + + expect(Attachment.count).to eq 1 + expect(Attachment.first[:file]).to eq 'model.ifc' + + expect(Bim::IfcModels::IfcModel.count).to eq 1 + expect(Bim::IfcModels::IfcModel.first.title).to eq "minimal.ifc" + end +end diff --git a/modules/grids/app/controllers/api/v3/attachments/attachments_by_grid_api.rb b/modules/grids/app/controllers/api/v3/attachments/attachments_by_grid_api.rb index c9222df66f..880a7ee991 100644 --- a/modules/grids/app/controllers/api/v3/attachments/attachments_by_grid_api.rb +++ b/modules/grids/app/controllers/api/v3/attachments/attachments_by_grid_api.rb @@ -45,6 +45,10 @@ module API get &API::V3::Attachments::AttachmentsByContainerAPI.read post &API::V3::Attachments::AttachmentsByContainerAPI.create + + namespace :prepare do + post &API::V3::Attachments::AttachmentsByContainerAPI.prepare + end end end end diff --git a/modules/meeting/lib/api/v3/attachments/attachments_by_meeting_content_api.rb b/modules/meeting/lib/api/v3/attachments/attachments_by_meeting_content_api.rb index 70a2a15d5e..5e92b87925 100644 --- a/modules/meeting/lib/api/v3/attachments/attachments_by_meeting_content_api.rb +++ b/modules/meeting/lib/api/v3/attachments/attachments_by_meeting_content_api.rb @@ -45,6 +45,10 @@ module API get &API::V3::Attachments::AttachmentsByContainerAPI.read post &API::V3::Attachments::AttachmentsByContainerAPI.create + + namespace :prepare do + post &API::V3::Attachments::AttachmentsByContainerAPI.prepare + end end end end diff --git a/spec/features/work_packages/attachments/attachment_upload_spec.rb b/spec/features/work_packages/attachments/attachment_upload_spec.rb index f7dc0be610..eccf380d96 100644 --- a/spec/features/work_packages/attachments/attachment_upload_spec.rb +++ b/spec/features/work_packages/attachments/attachment_upload_spec.rb @@ -77,57 +77,86 @@ describe 'Upload attachment to work package', js: true do end context 'on a new page' do - let!(:new_page) { Pages::FullWorkPackageCreate.new } - let!(:type) { FactoryBot.create(:type_task) } - let!(:status) { FactoryBot.create(:status, is_default: true) } - let!(:priority) { FactoryBot.create(:priority, is_default: true) } - let!(:project) do - FactoryBot.create(:project, types: [type]) - end + shared_examples 'it supports image uploads via drag & drop' do + let!(:new_page) { Pages::FullWorkPackageCreate.new } + let!(:type) { FactoryBot.create(:type_task) } + let!(:status) { FactoryBot.create(:status, is_default: true) } + let!(:priority) { FactoryBot.create(:priority, is_default: true) } + let!(:project) do + FactoryBot.create(:project, types: [type]) + end - before do - visit new_project_work_packages_path(project.identifier, type: type.id) - end + let(:post_conditions) { nil } - it 'can upload an image via drag & drop (Regression #28189)' do - subject = new_page.edit_field :subject - subject.set_value 'My subject' + before do + visit new_project_work_packages_path(project.identifier, type: type.id) + end - target = find('.ck-content') - attachments.drag_and_drop_file(target, image_fixture) + it 'can upload an image via drag & drop (Regression #28189)' do + subject = new_page.edit_field :subject + subject.set_value 'My subject' - sleep 2 - expect(page).not_to have_selector('notification-upload-progress') + target = find('.ck-content') + attachments.drag_and_drop_file(target, image_fixture) - editor.in_editor do |container, editable| - expect(editable).to have_selector('img[src*="/api/v3/attachments/"]', wait: 20) - end + sleep 2 + expect(page).not_to have_selector('notification-upload-progress') - sleep 2 + editor.in_editor do |container, editable| + expect(editable).to have_selector('img[src*="/api/v3/attachments/"]', wait: 20) + end - # Besides testing caption functionality this also slows down clicking on the submit button - # so that the image is properly embedded - caption = page.find('figure.image figcaption') - caption.click(x: 10, y: 10) - sleep 0.2 - caption.base.send_keys('Some image caption') + sleep 2 - sleep 2 + # Besides testing caption functionality this also slows down clicking on the submit button + # so that the image is properly embedded + caption = page.find('figure.image figcaption') + caption.click(x: 10, y: 10) + sleep 0.2 + caption.base.send_keys('Some image caption') - scroll_to_and_click find('#work-packages--edit-actions-save') + scroll_to_and_click find('#work-packages--edit-actions-save') - wp_page.expect_notification( - message: 'Successful creation.' - ) + wp_page.expect_notification( + message: 'Successful creation.' + ) - field = wp_page.edit_field :description + field = wp_page.edit_field :description - expect(field.display_element).to have_selector('img') - expect(field.display_element).to have_content('Some image caption') + expect(field.display_element).to have_selector('img') + expect(field.display_element).to have_content('Some image caption') + + wp = WorkPackage.last + expect(wp.subject).to eq('My subject') + expect(wp.attachments.count).to eq(1) - wp = WorkPackage.last - expect(wp.subject).to eq('My subject') - expect(wp.attachments.count).to eq(1) + post_conditions + end + end + + it_behaves_like 'it supports image uploads via drag & drop' + + # We do a complete integration test for direct uploads on this example. + # If this works all parts in the backend and frontend work properly together. + # Technically one could test this not only for new work packages, but also for existing + # ones, and for new and existing other attachable resources. But the code is the same + # everywhere so if this works it should work everywhere else too. + context 'with direct uploads', with_direct_uploads: true do + before do + allow_any_instance_of(Attachment).to receive(:diskfile).and_return Struct.new(:path).new(image_fixture.to_s) + end + + it_behaves_like 'it supports image uploads via drag & drop' do + let(:post_conditions) do + # check the attachment was created successfully + expect(Attachment.count).to eq 1 + a = Attachment.first + expect(a[:file]).to eq image_fixture.basename.to_s + + # check /api/v3/attachments/:id/uploaded was called + expect(::Attachments::FinishDirectUploadJob).to have_been_enqueued + end + end end end end diff --git a/spec/lib/api/v3/attachments/attachment_metadata_representer_spec.rb b/spec/lib/api/v3/attachments/attachment_metadata_representer_spec.rb index d855840e7d..6c1802b1bd 100644 --- a/spec/lib/api/v3/attachments/attachment_metadata_representer_spec.rb +++ b/spec/lib/api/v3/attachments/attachment_metadata_representer_spec.rb @@ -31,14 +31,22 @@ require 'spec_helper' describe ::API::V3::Attachments::AttachmentMetadataRepresenter do include API::V3::Utilities::PathHelper - let(:metadata) { + let(:metadata) do data = Hashie::Mash.new data.file_name = original_file_name data.description = original_description + data.content_type = original_content_type + data.file_size = original_file_size + data.digest = original_digest data - } + end + let(:original_file_name) { 'a file name' } let(:original_description) { 'a description' } + let(:original_content_type) { 'text/plain' } + let(:original_file_size) { 42 } + let(:original_digest) { "0xFF" } + let(:representer) { ::API::V3::Attachments::AttachmentMetadataRepresenter.new(metadata) } describe 'generation' do @@ -49,6 +57,9 @@ describe ::API::V3::Attachments::AttachmentMetadataRepresenter do end it { is_expected.to be_json_eql(original_file_name.to_json).at_path('fileName') } + it { is_expected.to be_json_eql(original_content_type.to_json).at_path('contentType') } + it { is_expected.to be_json_eql(original_file_size.to_json).at_path('fileSize') } + it { is_expected.to be_json_eql(original_digest.to_json).at_path('digest') } it_behaves_like 'API V3 formattable', 'description' do let(:format) { 'plain' } @@ -60,7 +71,10 @@ describe ::API::V3::Attachments::AttachmentMetadataRepresenter do let(:parsed_hash) { { 'fileName' => 'the parsed name', - 'description' => { 'raw' => 'the parsed description' } + 'description' => { 'raw' => 'the parsed description' }, + 'contentType' => 'text/html', + 'fileSize' => 43, + 'digest' => '0x00' } } @@ -72,5 +86,8 @@ describe ::API::V3::Attachments::AttachmentMetadataRepresenter do it { expect(subject.file_name).to eql('the parsed name') } it { expect(subject.description).to eql('the parsed description') } + it { expect(subject.content_type).to eql('text/html') } + it { expect(subject.file_size).to eql(43) } + it { expect(subject.digest).to eql('0x00') } end end diff --git a/spec/lib/open_project/configuration_spec.rb b/spec/lib/open_project/configuration_spec.rb index 2e264dbf2d..f4bdafff54 100644 --- a/spec/lib/open_project/configuration_spec.rb +++ b/spec/lib/open_project/configuration_spec.rb @@ -480,4 +480,39 @@ describe OpenProject::Configuration do end end end + + context 'helpers' do + describe '#direct_uploads?' do + let(:value) { OpenProject::Configuration.direct_uploads? } + + it 'should be false by default' do + expect(value).to be false + end + + context 'with remote storage' do + def self.storage(provider) + { + attachments_storage: :fog, + fog: { + credentials: { + provider: provider + } + } + } + end + + context 'AWS', with_config: storage('AWS') do + it 'should be true' do + expect(value).to be true + end + end + + context 'Azure', with_config: storage('azure') do + it 'should be false' do + expect(value).to be false + end + end + end + end + end end diff --git a/spec/models/attachment_spec.rb b/spec/models/attachment_spec.rb index cc8f9a1c53..5e00a5ccf7 100644 --- a/spec/models/attachment_spec.rb +++ b/spec/models/attachment_spec.rb @@ -226,39 +226,18 @@ describe Attachment, type: :model do end end - describe "#external_url" do + # We just use with_direct_uploads here to make sure the + # FogAttachment class is defined and Fog is mocked. + describe "#external_url", with_direct_uploads: true do let(:author) { FactoryBot.create :user } let(:image_path) { Rails.root.join("spec/fixtures/files/image.png") } let(:text_path) { Rails.root.join("spec/fixtures/files/testfile.txt") } let(:binary_path) { Rails.root.join("spec/fixtures/files/textfile.txt.gz") } - let(:fog_attachment_class) do - class FogAttachment < Attachment - # Remounting the uploader overrides the original file setter taking care of setting, - # among other things, the content type. So we have to restore that original - # method this way. - # We do this in a new, separate class, as to not interfere with any other specs. - alias_method :set_file, :file= - mount_uploader :file, FogFileUploader - alias_method :file=, :set_file - end - - FogAttachment - end - - let(:image_attachment) { fog_attachment_class.new author: author, file: File.open(image_path) } - let(:text_attachment) { fog_attachment_class.new author: author, file: File.open(text_path) } - let(:binary_attachment) { fog_attachment_class.new author: author, file: File.open(binary_path) } - - before do - Fog.mock! - - connection = Fog::Storage.new provider: "AWS" - connection.directories.create key: "my-bucket" - - CarrierWave::Configuration.configure_fog! credentials: {}, directory: "my-bucket", public: false - end + let(:image_attachment) { FogAttachment.new author: author, file: File.open(image_path) } + let(:text_attachment) { FogAttachment.new author: author, file: File.open(text_path) } + let(:binary_attachment) { FogAttachment.new author: author, file: File.open(binary_path) } shared_examples "it has a temporary download link" do let(:url_options) { {} } diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index e2ce744046..cdc56ee527 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -47,6 +47,12 @@ require 'test_prof/recipes/rspec/before_all' # directory. Alternatively, in the individual `*_spec.rb` files, manually # require only the support files necessary. # +# The files are sorted before requiring them to ensure the load order is the same +# everywhere. There are certain helpers that depend on a expected order. +# The CI may load the files in a different order than what you see locally which +# may lead to broken specs on the CI, if we don't sort here +# (example: with_config.rb has to precede with_direct_uploads.rb). +# Dir[Rails.root.join('spec/support/**/*.rb')].each { |f| require f } Dir[Rails.root.join('spec/features/support/**/*.rb')].each { |f| require f } Dir[Rails.root.join('spec/lib/api/v3/support/**/*.rb')].each { |f| require f } diff --git a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb index 75e485db8a..cf4a6fbb82 100644 --- a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb +++ b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb @@ -29,6 +29,112 @@ require 'spec_helper' require 'rack/test' +shared_examples 'it supports direct uploads' do + include Rack::Test::Methods + include API::V3::Utilities::PathHelper + include FileHelpers + + let(:container_href) { raise "define me!" } + let(:request_path) { raise "define me!" } + + before do + allow(User).to receive(:current).and_return current_user + end + + describe 'POST /prepare', with_settings: { attachment_max_size: 512 } do + let(:request_parts) { { metadata: metadata, file: file } } + let(:metadata) { { fileName: 'cat.png', fileSize: file.size }.to_json } + let(:file) { mock_uploaded_file(name: 'original-filename.txt') } + + def request! + post request_path, request_parts + end + + subject(:response) { last_response } + + context 'with local storage' do + before do + request! + end + + it 'should respond with HTTP Not Found' do + expect(subject.status).to eq(404) + end + end + + context 'with remote AWS storage', with_direct_uploads: true do + before do + request! + end + + context 'with no filesize metadata' do + let(:metadata) { { fileName: 'cat.png' }.to_json } + + it 'should respond with 422 due to missing file size metadata' do + expect(subject.status).to eq(422) + expect(subject.body).to include 'fileSize' + end + end + + context 'with the correct parameters' do + let(:json) { JSON.parse subject.body } + + it 'should prepare a direct upload' do + expect(subject.status).to eq 201 + + expect(json["_type"]).to eq "AttachmentUpload" + expect(json["fileName"]).to eq "cat.png" + end + + describe 'response' do + describe '_links' do + describe 'container' do + let(:link) { json.dig "_links", "container" } + + before do + expect(link).to be_present + end + + it "it points to the expected container" do + expect(link["href"]).to eq container_href + end + end + + describe 'addAttachment' do + let(:link) { json.dig "_links", "addAttachment" } + + before do + expect(link).to be_present + end + + it 'should point to AWS' do + expect(link["href"]).to eq "https://#{MockCarrierwave.bucket}.s3.amazonaws.com/" + end + + it 'should have the method POST' do + expect(link["method"]).to eq "post" + end + + it 'should include form fields' do + fields = link["form_fields"] + + expect(fields).to be_present + expect(fields).to include( + "key", "acl", "policy", + "X-Amz-Signature", "X-Amz-Credential", "X-Amz-Algorithm", "X-Amz-Date", + "success_action_status" + ) + + expect(fields["key"]).to end_with "cat.png" + end + end + end + end + end + end + end +end + shared_examples 'an APIv3 attachment resource', type: :request, content_type: :json do |include_by_container = true| include Rack::Test::Methods include API::V3::Utilities::PathHelper @@ -366,6 +472,11 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j end context 'by container', if: include_by_container do + it_behaves_like 'it supports direct uploads' do + let(:request_path) { "/api/v3/#{attachment_type}s/#{container.id}/attachments/prepare" } + let(:container_href) { "/api/v3/#{attachment_type}s/#{container.id}" } + end + subject(:response) { last_response } describe '#get' do diff --git a/spec/requests/api/v3/attachments_spec.rb b/spec/requests/api/v3/attachments_spec.rb new file mode 100644 index 0000000000..3940f03423 --- /dev/null +++ b/spec/requests/api/v3/attachments_spec.rb @@ -0,0 +1,103 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2020 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' +require_relative 'attachments/attachment_resource_shared_examples' + +describe API::V3::Attachments::AttachmentsAPI, type: :request do + include Rack::Test::Methods + include API::V3::Utilities::PathHelper + include FileHelpers + + let(:current_user) { FactoryBot.create(:user, member_in_project: project, member_through_role: role) } + + let(:project) { FactoryBot.create(:project, public: false) } + let(:role) { FactoryBot.create(:role, permissions: permissions) } + let(:permissions) { [:add_work_packages] } + + context( + 'with missing permissions', + with_config: { + attachments_storage: :fog, + fog: { credentials: { provider: 'AWS' } } + } + ) do + let(:permissions) { [] } + + let(:request_path) { api_v3_paths.prepare_new_attachment_upload } + let(:request_parts) { { metadata: metadata, file: file } } + let(:metadata) { { fileName: 'cat.png' }.to_json } + let(:file) { mock_uploaded_file(name: 'original-filename.txt') } + + before do + post request_path, request_parts + end + + it 'should forbid to prepare attachments' do + expect(last_response.status).to eq 403 + end + end + + it_behaves_like 'it supports direct uploads' do + let(:request_path) { api_v3_paths.prepare_new_attachment_upload } + let(:container_href) { nil } + + describe 'GET /uploaded' do + let(:digest) { "" } + let(:attachment) { FactoryBot.create :attachment, digest: digest, author: current_user, container: nil, container_type: nil, downloads: -1 } + + before do + get "/api/v3/attachments/#{attachment.id}/uploaded" + end + + context 'with no pending attachments' do + let(:digest) { "0xFF" } + + it 'should return 404' do + expect(last_response.status).to eq 404 + end + end + + context 'with a pending attachment' do + it 'should enqueue a FinishDirectUpload job' do + expect(::Attachments::FinishDirectUploadJob).to have_been_enqueued.at_least(1) + end + + it 'should respond with HTTP OK' do + expect(last_response.status).to eq 200 + end + + it 'should return the attachment representation' do + json = JSON.parse last_response.body + + expect(json["_type"]).to eq "Attachment" + end + end + end + end +end diff --git a/spec/support/carrierwave.rb b/spec/support/carrierwave.rb index 72de19221d..bf7a280eea 100644 --- a/spec/support/carrierwave.rb +++ b/spec/support/carrierwave.rb @@ -26,17 +26,31 @@ # See docs/COPYRIGHT.rdoc for more details. #++ -mock_credentials = { - provider: 'AWS', - aws_access_key_id: 'someaccesskeyid', - aws_secret_access_key: 'someprivateaccesskey', - region: 'us-east-1' -} -mock_bucket = 'test-bucket' +module MockCarrierwave + extend self -Fog.mock! -Fog.credentials = mock_credentials -CarrierWave::Configuration.configure_fog! directory: mock_bucket, credentials: mock_credentials + def apply + Fog.mock! + Fog.credentials = credentials -connection = Fog::Storage.new provider: mock_credentials[:provider] -connection.directories.create key: mock_bucket + CarrierWave::Configuration.configure_fog! directory: bucket, credentials: credentials + + connection = Fog::Storage.new provider: credentials[:provider] + connection.directories.create key: bucket + end + + def bucket + 'test-bucket' + end + + def credentials + { + provider: 'AWS', + aws_access_key_id: 'someaccesskeyid', + aws_secret_access_key: 'someprivateaccesskey', + region: 'us-east-1' + } + end +end + +MockCarrierwave.apply diff --git a/spec/support/shared/with_config.rb b/spec/support/shared/with_config.rb index fae1294c8e..f96a036abe 100644 --- a/spec/support/shared/with_config.rb +++ b/spec/support/shared/with_config.rb @@ -27,36 +27,64 @@ # See docs/COPYRIGHT.rdoc for more details. #++ -def aggregate_mocked_configuration(example, config) - # We have to manually check parent groups for with_config:, - # since they are being ignored otherwise - example.example_group.module_parents.each do |parent| - if parent.respond_to?(:metadata) && parent.metadata[:with_config] - config.reverse_merge!(parent.metadata[:with_config]) +class WithConfig + attr_reader :context + + def initialize(context) + @context = context + end + + ## + # We need this so calls to rspec mocks (allow, expect etc.) will work here as expected. + def method_missing(method, *args, &block) + if context.respond_to?(method) + context.send method, *args, &block + else + super end end - config + ## + # Stubs the given configurations. + # + # @config [Hash] Hash containing the configurations with keys as seen in `configuration.rb`. + def before(example, config) + allow(OpenProject::Configuration).to receive(:[]).and_call_original + + aggregate_mocked_configuration(example, config) + .with_indifferent_access + .each(&method(:stub_key)) + end + + def stub_key(key, value) + allow(OpenProject::Configuration) + .to receive(:[]) + .with(key.to_s) + .and_return(value) + + allow(OpenProject::Configuration) + .to receive(:[]) + .with(key.to_sym) + .and_return(value) + end + + def aggregate_mocked_configuration(example, config) + # We have to manually check parent groups for with_config:, + # since they are being ignored otherwise + example.example_group.module_parents.each do |parent| + if parent.respond_to?(:metadata) && parent.metadata[:with_config] + config.reverse_merge!(parent.metadata[:with_config]) + end + end + + config + end end RSpec.configure do |config| config.before(:each) do |example| - config = example.metadata[:with_config] - if config.present? - config = aggregate_mocked_configuration(example, config).with_indifferent_access - - allow(OpenProject::Configuration).to receive(:[]).and_call_original - config.each do |k, v| - allow(OpenProject::Configuration) - .to receive(:[]) - .with(k.to_s) - .and_return(v) - - allow(OpenProject::Configuration) - .to receive(:[]) - .with(k.to_sym) - .and_return(v) - end - end + with_config = example.metadata[:with_config] + + WithConfig.new(self).before example, with_config if with_config.present? end end diff --git a/spec/support/shared/with_direct_uploads.rb b/spec/support/shared/with_direct_uploads.rb new file mode 100644 index 0000000000..c0ebd68325 --- /dev/null +++ b/spec/support/shared/with_direct_uploads.rb @@ -0,0 +1,199 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2020 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +class WithDirectUploads + attr_reader :context + + def initialize(context) + @context = context + end + + ## + # We need this so calls to rspec mocks (allow, expect etc.) will work here as expected. + def method_missing(method, *args, &block) + if context.respond_to?(method) + context.send method, *args, &block + else + super + end + end + + def before(example) + stub_config example + + mock_attachment + stub_frontend redirect: redirect?(example) if stub_frontend?(example) + + stub_uploader + end + + def stub_frontend?(example) + example.metadata[:js] + end + + def redirect?(example) + example.metadata[:with_direct_uploads] == :redirect + end + + def around(example) + example.metadata[:driver] = :headless_firefox_billy + + csp_config = SecureHeaders::Configuration.instance_variable_get("@default_config").csp + csp_config.connect_src = ["'self'", "my-bucket.s3.amazonaws.com"] + + begin + example.run + ensure + csp_config.connect_src = %w('self') + end + end + + def mock_attachment + allow(Attachment).to receive(:create) do |*args| + # We don't use create here because this would cause an infinite loop as FogAttachment's #create + # uses the base class's #create which is what we are mocking here. All this is necessary to begin + # with because the Attachment class is initialized with the LocalFileUploader before this test + # is ever run and we need remote attachments using the FogFileUploader in this scenario. + record = FogAttachment.new *args + record.save + record + end + + # This is so the uploaded callback works. Since we can't actually substitute the Attachment class + # used there we get a LocalFileUploader file for the attachment which is not readable when + # everything else is mocked to be remote. + allow_any_instance_of(FileUploader).to receive(:readable?).and_return true + end + + def stub_frontend(redirect: false) + proxy.stub("https://" + OpenProject::Configuration.remote_storage_host + ":443/", method: 'options').and_return( + headers: { + 'Access-Control-Allow-Methods' => 'POST', + 'Access-Control-Allow-Origin' => '*' + }, + code: 200 + ) + + if redirect + stub_with_redirect + else # use status response instead of redirect by default + stub_with_status + end + end + + def stub_with_redirect + proxy + .stub("https://" + OpenProject::Configuration.remote_storage_host + ":443/", method: 'post') + .and_return(Proc.new { |params, headers, body, url, method| + key = body.scan(/key"\s*([^\s]+)\s/m).flatten.first + redirect_url = body.scan(/success_action_redirect"\s*(http[^\s]+)\s/m).flatten.first + ok = body =~ /X-Amz-Signature/ # check that the expected post to AWS was made with the form fields + + { + code: ok ? 302 : 403, + headers: { + 'Location' => ok ? redirect_url + '?key=' + CGI.escape(key) : nil, + 'Access-Control-Allow-Methods' => 'POST', + 'Access-Control-Allow-Origin' => '*' + } + } + }) + end + + def stub_with_status + proxy + .stub("https://" + OpenProject::Configuration.remote_storage_host + ":443/", method: 'post') + .and_return(Proc.new { |params, headers, body, url, method| + { + code: (body =~ /X-Amz-Signature/) ? 201 : 403, # check that the expected post to AWS was made with the form fields + headers: { + 'Access-Control-Allow-Methods' => 'POST', + 'Access-Control-Allow-Origin' => '*' + } + } + }) + end + + def stub_uploader + creds = config[:fog][:credentials] + + allow_any_instance_of(FogFileUploader).to receive(:fog_credentials).and_return creds + + allow_any_instance_of(FogFileUploader).to receive(:aws_access_key_id).and_return creds[:aws_access_key_id] + allow_any_instance_of(FogFileUploader).to receive(:aws_secret_access_key).and_return creds[:aws_secret_access_key] + allow_any_instance_of(FogFileUploader).to receive(:provider).and_return creds[:provider] + allow_any_instance_of(FogFileUploader).to receive(:region).and_return creds[:region] + allow_any_instance_of(FogFileUploader).to receive(:directory).and_return config[:fog][:directory] + + allow(OpenProject::Configuration).to receive(:direct_uploads?).and_return(true) + end + + def stub_config(example) + WithConfig.new(context).before example, config + end + + def config + { + attachments_storage: :fog, + fog: { + directory: MockCarrierwave.bucket, + credentials: MockCarrierwave.credentials + } + } + end +end + +RSpec.configure do |config| + config.before(:each) do |example| + next unless example.metadata[:with_direct_uploads] + + WithDirectUploads.new(self).before example + + class FogAttachment < Attachment + # Remounting the uploader overrides the original file setter taking care of setting, + # among other things, the content type. So we have to restore that original + # method this way. + # We do this in a new, separate class, as to not interfere with any other specs. + alias_method :set_file, :file= + mount_uploader :file, FogFileUploader + alias_method :file=, :set_file + end + end + + config.around(:each) do |example| + enabled = example.metadata[:with_direct_uploads] + + if enabled + WithDirectUploads.new(self).around example + else + example.run + end + end +end diff --git a/spec/views/layouts/base.html.erb_spec.rb b/spec/views/layouts/base.html.erb_spec.rb index 7729e17fa6..5d38cbc5c2 100644 --- a/spec/views/layouts/base.html.erb_spec.rb +++ b/spec/views/layouts/base.html.erb_spec.rb @@ -267,4 +267,17 @@ describe 'layouts/base', type: :view do end end end + + describe 'openproject_initializer meta tag' do + let(:current_user) { anonymous } + let(:base) { 'meta[name=openproject_initializer]' } + + before do + render + end + + it 'has the meta tag' do + expect(rendered).to have_selector(base, visible: false) + end + end end diff --git a/spec/workers/attachments/cleanup_uncontainered_job_integration_spec.rb b/spec/workers/attachments/cleanup_uncontainered_job_integration_spec.rb index b8b242294c..4242ee269e 100644 --- a/spec/workers/attachments/cleanup_uncontainered_job_integration_spec.rb +++ b/spec/workers/attachments/cleanup_uncontainered_job_integration_spec.rb @@ -32,6 +32,7 @@ require 'spec_helper' describe Attachments::CleanupUncontaineredJob, type: :job do let(:grace_period) { 120 } + let!(:containered_attachment) { FactoryBot.create(:attachment) } let!(:old_uncontainered_attachment) do FactoryBot.create(:attachment, container: nil, created_at: Time.now - grace_period.minutes) @@ -39,6 +40,17 @@ describe Attachments::CleanupUncontaineredJob, type: :job do let!(:new_uncontainered_attachment) do FactoryBot.create(:attachment, container: nil, created_at: Time.now - (grace_period - 1).minutes) end + + let!(:finished_upload) do + FactoryBot.create(:attachment, created_at: Time.now - grace_period.minutes, digest: "0x42") + end + let!(:old_pending_upload) do + FactoryBot.create(:attachment, created_at: Time.now - grace_period.minutes, digest: "", downloads: -1) + end + let!(:new_pending_upload) do + FactoryBot.create(:attachment, created_at: Time.now - (grace_period - 1).minutes, digest: "", downloads: -1) + end + let(:job) { described_class.new } before do @@ -47,10 +59,10 @@ describe Attachments::CleanupUncontaineredJob, type: :job do .and_return(grace_period) end - it 'removes all uncontainered attachments that are older than the grace period' do + it 'removes all uncontainered attachments and pending uploads that are older than the grace period' do job.perform expect(Attachment.all) - .to match_array([containered_attachment, new_uncontainered_attachment]) + .to match_array([containered_attachment, new_uncontainered_attachment, finished_upload, new_pending_upload]) end end