Merge pull request #13754 from opf/implementation/49113-get-work-pack…

…age-file-links-for-a-onedrive-storage

[#49113] ensure to display file links for one drive storages

app/services/oauth_clients/connection_manager.rb

@@ -60,6 +60,8 @@ def get_access_token(state: nil)
       ServiceResult.failure(result: @redirect_url)
     end
 
+    # rubocop:disable Metrics/AbcSize
+
     # The bearer/access token has expired or is due for renew for other reasons.
     # Talk to OAuth2 Authorization Server to exchange the renew_token for a new bearer token.
     def refresh_token
@@ -79,11 +81,17 @@ def refresh_token
             ServiceResult.success(result: oauth_client_token)
           end
         else
-          service_result_with_error(I18n.t('oauth_client.errors.refresh_token_called_without_existing_token'))
+          storage_error = ::Storages::StorageError.new(
+            code: :error,
+            log_message: I18n.t('oauth_client.errors.refresh_token_called_without_existing_token')
+          )
+          ServiceResult.failure(result: :error, errors: storage_error)
         end
       end
     end
 
+    # rubocop:enable Metrics/AbcSize
+
     # Returns the URI of the "authorize" endpoint of the OAuth2 Authorization Server.
     # @param state (OAuth2 RFC) is a nonce referencing a cookie containing the calling page (URL + params) to which to
     # return to at the end of the whole flow.
@@ -93,6 +101,8 @@ def get_authorization_uri(state: nil)
       client.authorization_uri(scope: @config.scope, state:)
     end
 
+    # rubocop:disable Metrics/AbcSize
+
     # Called by callback_page with a cryptographic "code" that indicates
     # that the user has successfully authorized the OAuth2 Authorization Server.
     # We now are going to exchange this code to a token (bearer+refresh)
@@ -128,6 +138,8 @@ def code_to_token(code)
       ServiceResult.success(result: oauth_client_token)
     end
 
+    # rubocop:enable Metrics/AbcSize
+
     # Called by StorageRepresenter to inquire about the status of the OAuth2
     # authentication server.
     # Returns :connected/:authorization_failed or :error for a general error.
@@ -146,7 +158,7 @@ def authorization_state
         service_result = refresh_token
         if service_result.success?
           :connected
-        elsif service_result.result == 'invalid_request'
+        elsif service_result.errors.data.payload[:error] == 'invalid_request'
           :failed_authorization
         else
           :error
@@ -167,11 +179,7 @@ def request_with_token_refresh(oauth_client_token)
 
       if yield_service_result.failure? && yield_service_result.result == :unauthorized
         refresh_service_result = refresh_token
-        if refresh_service_result.failure?
-          failed_service_result = ServiceResult.failure(result: :error)
-          failed_service_result.merge!(refresh_service_result)
-          return failed_service_result
-        end
+        return refresh_service_result if refresh_service_result.failure?
 
         oauth_client_token.reload
         yield_service_result = yield(oauth_client_token) # Should contain result=<data> in case of success
@@ -189,27 +197,29 @@ def get_existing_token
       OAuthClientToken.find_by(user_id: @user, oauth_client_id: @oauth_client.id)
     end
 
-    # Calls client.access_token!
-    # Convert the various exceptions into user-friendly error strings.
+    # rubocop:disable Metrics/AbcSize
     def request_new_token(options = {})
       rack_access_token = rack_oauth_client(options).access_token!(:body)
 
       ServiceResult.success(result: rack_access_token)
     rescue Rack::OAuth2::Client::Error => e
-      service_result_with_error(i18n_rack_oauth2_error_message(e), e.message)
+      service_result_with_error(:bad_request, e.response, i18n_rack_oauth2_error_message(e))
     rescue Faraday::TimeoutError, Faraday::ConnectionFailed, Faraday::ParsingError, Faraday::SSLError => e
       service_result_with_error(
-        "#{I18n.t('oauth_client.errors.oauth_returned_http_error')}: #{e.class}: #{e.message.to_html}",
-        e.message
+        :internal_server_error,
+        e,
+        "#{I18n.t('oauth_client.errors.oauth_returned_http_error')}: #{e.class}: #{e.message.to_html}"
       )
     rescue StandardError => e
       service_result_with_error(
-        "#{I18n.t('oauth_client.errors.oauth_returned_standard_error')}: #{e.class}: #{e.message.to_html}",
-        e.message
+        :error,
+        e,
+        "#{I18n.t('oauth_client.errors.oauth_returned_standard_error')}: #{e.class}: #{e.message.to_html}"
       )
     end
 
-    # Localize the error message
+    # rubocop:enable Metrics/AbcSize
+
     def i18n_rack_oauth2_error_message(rack_oauth2_client_exception)
       i18n_key = "oauth_client.errors.rack_oauth2.#{rack_oauth2_client_exception.message}"
       if I18n.exists? i18n_key
@@ -250,12 +260,10 @@ def update_oauth_client_token(oauth_client_token, rack_oauth2_access_token)
       end
     end
 
-    # Shortcut method to convert an error message into an unsuccessful
-    # ServiceResult with that error message
-    def service_result_with_error(message, result = nil)
-      ServiceResult.failure(result:).tap do |r|
-        r.errors.add(:base, message)
-      end
+    def service_result_with_error(code, data, log_message = nil)
+      error_data = ::Storages::StorageErrorData.new(source: self, payload: data)
+      ServiceResult.failure(result: code,
+                            errors: ::Storages::StorageError.new(code:, data: error_data, log_message:))
     end
   end
 end

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

@@ -27,6 +27,11 @@
 #++
 
 module Storages::Peripherals
+  # rubocop:disable Lint/EmptyClass
+  class UnknownSource; end
+
+  # rubocop:enable Lint/EmptyClass
+
   module ServiceResultRefinements
     refine ServiceResult do
       def match(on_success:, on_failure:)
@@ -50,6 +55,18 @@ def >>(other)
 
         bind(&other)
       end
+
+      def error_source
+        if errors.is_a?(::Storages::StorageError) && errors.data&.source.present?
+          errors.data.source
+        else
+          UnknownSource
+        end
+      end
+
+      def error_payload
+        errors.data&.payload
+      end
     end
   end
 end

modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/files_info_query.rb

@@ -55,7 +55,14 @@ def call(user:, file_ids:)
             end
 
             result = file_ids.map do |file_id|
-              wrap_storage_file_error(file_id, FileInfoQuery.call(storage: @storage, user:, file_id:))
+              file_info_result = FileInfoQuery.call(storage: @storage, user:, file_id:)
+              if file_info_result.failure? &&
+                file_info_result.error_source.is_a?(::OAuthClients::ConnectionManager)
+                # errors in the connection manager must short circuit the query and return the error
+                return file_info_result
+              end
+
+              wrap_storage_file_error(file_id, file_info_result)
             end
 
             ServiceResult.success(result:)
@@ -67,11 +74,10 @@ def wrap_storage_file_error(file_id, query_result)
             if query_result.success?
               query_result.result
             else
-              storage_error = query_result.errors
               StorageFileInfo.new(
                 id: file_id,
-                status: storage_error.data.dig(:error, :code),
-                status_code: Rack::Utils::SYMBOL_TO_STATUS_CODE[storage_error.code]
+                status: query_result.error_payload.dig(:error, :code),
+                status_code: Rack::Utils::SYMBOL_TO_STATUS_CODE[query_result.errors.code]
               )
             end
           end

modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/internal/drive_item_query.rb

@@ -41,22 +41,23 @@ def make_file_request(drive_item_id, token, select_url_query)
 
             def handle_responses(response)
               json = MultiJson.load(response.body, symbolize_keys: true)
+              error_data = ::Storages::StorageErrorData.new(source: self, payload: json)
 
               case response
               when Net::HTTPSuccess
                 ServiceResult.success(result: json)
               when Net::HTTPNotFound
                 ServiceResult.failure(result: :not_found,
-                                      errors: ::Storages::StorageError.new(code: :not_found, data: json))
+                                      errors: ::Storages::StorageError.new(code: :not_found, data: error_data))
               when Net::HTTPForbidden
                 ServiceResult.failure(result: :forbidden,
-                                      errors: ::Storages::StorageError.new(code: :forbidden, data: json))
+                                      errors: ::Storages::StorageError.new(code: :forbidden, data: error_data))
               when Net::HTTPUnauthorized
                 ServiceResult.failure(result: :unauthorized,
-                                      errors: ::Storages::StorageError.new(code: :unauthorized, data: json))
+                                      errors: ::Storages::StorageError.new(code: :unauthorized, data: error_data))
               else
                 ServiceResult.failure(result: :error,
-                                      errors: ::Storages::StorageError.new(code: :error, data: json))
+                                      errors: ::Storages::StorageError.new(code: :error, data: error_data))
               end
             end
 

modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/util.rb

@@ -51,6 +51,7 @@ def using_user_token(storage, user, &)
               result: :unauthorized,
               errors: ::Storages::StorageError.new(
                 code: :unauthorized,
+                data: ::Storages::StorageErrorData.new(source: connection_manager),
                 log_message: 'Query could not be created! No access token found!'
               )
             )

modules/storages/app/models/storages/storage_error.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) 2012-2023 the OpenProject GmbH

modules/storages/app/models/storages/storage_error_data.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2023 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 COPYRIGHT and LICENSE files for more details.
+#++
+
+module Storages
+  class StorageErrorData
+    attr_reader :source, :payload
+
+    def initialize(source:, payload: nil)
+      @source = source
+      @payload = payload
+    end
+  end
+end

modules/storages/app/services/storages/file_link_sync_service.rb

@@ -38,7 +38,7 @@ def initialize(user:)
   def call(file_links)
     resulting_file_links = file_links
       .group_by(&:storage_id)
-      .map { |storage_id, storage_file_links| sync_nextcloud(storage_id, storage_file_links) }
+      .map { |storage_id, storage_file_links| sync_storage_data(storage_id, storage_file_links) }
       .reduce([]) do |state, sync_result|
         sync_result.match(
           on_success: ->(sr) { state + sr },
@@ -51,7 +51,7 @@ def call(file_links)
 
   private
 
-  def sync_nextcloud(storage_id, file_links)
+  def sync_storage_data(storage_id, file_links)
     storage = ::Storages::Storage.find(storage_id)
     ::Storages::Peripherals::Registry
       .resolve("queries.#{storage.short_provider_type}.files_info")

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

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) 2012-2023 the OpenProject GmbH
@@ -26,16 +28,10 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-# This class provides definitions for API routes and endpoints for the file_links namespace. It inherits the
-# functionality from the Grape REST API framework. It is mounted in lib/api/v3/work_packages/work_packages_api.rb,
-# 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
   # 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.
     get do
-      # API supports query filters on storages, for example { storage: { operator: '=', values: [storage_id] }
       query = ParamsToQueryService
                 .new(::Storages::Storage,
                      current_user,
@@ -48,12 +44,9 @@ class API::V3::FileLinks::WorkPackagesFileLinksAPI < API::OpenProjectAPI
       end
 
       result = if current_user.allowed_to?(:view_file_links, @work_package.project)
-                 # Get a (potentially huge...) list of all FileLinks for the work package.
                  file_links = query.results.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)

modules/storages/spec/common/storages/peripherals/storage_interaction/one_drive/file_info_query_spec.rb

@@ -90,7 +90,7 @@
 
       expect(storage_files).to be_failure
       expect(storage_files.result).to eq(:not_found)
-      expect(storage_files.errors.data.to_json).to eq(not_found_json)
+      expect(storage_files.errors.data.payload.to_json).to eq(not_found_json)
     end
 
     it 'returns a forbidden error if the API call returns a 403' do
@@ -102,7 +102,7 @@
 
       expect(storage_files).to be_failure
       expect(storage_files.result).to eq(:forbidden)
-      expect(storage_files.errors.data.to_json).to eq(forbidden_json)
+      expect(storage_files.errors.data.payload.to_json).to eq(forbidden_json)
     end
   end
 end

modules/storages/spec/common/storages/peripherals/storage_interaction/one_drive/open_link_query_spec.rb

@@ -84,7 +84,7 @@
 
       expect(open_link_result).to be_failure
       expect(open_link_result.result).to eq(:not_found)
-      expect(open_link_result.errors.data.to_json).to eq(not_found_json)
+      expect(open_link_result.errors.data.payload.to_json).to eq(not_found_json)
     end
 
     it 'returns a forbidden error if the API call returns a 403' do
@@ -96,7 +96,7 @@
 
       expect(open_link_result).to be_failure
       expect(open_link_result.result).to eq(:forbidden)
-      expect(open_link_result.errors.data.to_json).to eq(forbidden_json)
+      expect(open_link_result.errors.data.payload.to_json).to eq(forbidden_json)
     end
   end
 end