From 264c8ac998da511e0b73ca508d69832c644cce0b Mon Sep 17 00:00:00 2001 From: Pavel Balashou Date: Fri, 9 Feb 2024 00:06:00 +0100 Subject: [PATCH] Handle possible timeouts in host_validator. --- lib/open_project.rb | 2 +- .../nextcloud_compatible_host_validator.rb | 38 ++++---------- .../storages/nextcloud_contract_spec.rb | 15 ++++++ .../storages/shared_contract_examples.rb | 51 ++++++++++++++----- .../spec/support/storage_server_helpers.rb | 50 +++++++++++------- 5 files changed, 98 insertions(+), 58 deletions(-) diff --git a/lib/open_project.rb b/lib/open_project.rb index 31cb5245df1..3597fbcd3b5 100644 --- a/lib/open_project.rb +++ b/lib/open_project.rb @@ -47,7 +47,7 @@ module OpenProject def self.httpx HTTPX - .plugin(:persistent) + .plugin(:persistent) # persistent plugin enables retries plugin under the hood .plugin(:basic_auth) .plugin(:webdav) end diff --git a/modules/storages/app/validator/nextcloud_compatible_host_validator.rb b/modules/storages/app/validator/nextcloud_compatible_host_validator.rb index 53881af9c1f..1b522a65100 100644 --- a/modules/storages/app/validator/nextcloud_compatible_host_validator.rb +++ b/modules/storages/app/validator/nextcloud_compatible_host_validator.rb @@ -43,14 +43,9 @@ class NextcloudCompatibleHostValidator < ActiveModel::EachValidator def validate_capabilities(contract, attribute, value) uri = URI.parse(File.join(value, '/ocs/v2.php/cloud/capabilities')) - response = - begin - OpenProject.httpx - .with(HTTPX_TIMEOUT_SETTINGS) - .get(uri, headers: { "Ocs-Apirequest" => "true", "Accept" => "application/json" }) - rescue StandardError => e - e - end + response = OpenProject.httpx + .with(HTTPX_TIMEOUT_SETTINGS) + .get(uri, headers: { "Ocs-Apirequest" => "true", "Accept" => "application/json" }) error_type = check_capabilities_response(response) if error_type @@ -60,7 +55,7 @@ class NextcloudCompatibleHostValidator < ActiveModel::EachValidator end def check_capabilities_response(response) - return :cannot_be_connected_to if response.is_a? StandardError + return :cannot_be_connected_to if response.error.present? return :cannot_be_connected_to unless response.status.in? 200..299 return :not_nextcloud_server unless read_version(response) return :minimal_nextcloud_version_unmet unless major_version_sufficient?(response) @@ -75,14 +70,9 @@ class NextcloudCompatibleHostValidator < ActiveModel::EachValidator # https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/oauth2.html def validate_setup_completeness(contract, attribute, value) uri = URI.parse(File.join(value, 'index.php/apps/integration_openproject/check-config')) - response = - begin - OpenProject.httpx - .with(HTTPX_TIMEOUT_SETTINGS) - .get(uri, headers: { "Authorization" => AUTHORIZATION_HEADER }) - rescue StandardError => e - e - end + response = OpenProject.httpx + .with(HTTPX_TIMEOUT_SETTINGS) + .get(uri, headers: { "Authorization" => AUTHORIZATION_HEADER }) error_type = check_setup_completeness_response(response) if error_type @@ -92,7 +82,7 @@ class NextcloudCompatibleHostValidator < ActiveModel::EachValidator end def check_setup_completeness_response(response) - return :cannot_be_connected_to if response.is_a? StandardError + return :cannot_be_connected_to if response.error.present? return :op_application_not_installed if response.status.in? 300..399 return :cannot_be_connected_to unless response.status.in? 200..299 return :authorization_header_missing if read_authorization_header(response) != AUTHORIZATION_HEADER @@ -100,19 +90,13 @@ class NextcloudCompatibleHostValidator < ActiveModel::EachValidator nil end - def message(host, response_or_exception, error_type) - if response_or_exception.is_a?(HTTPX::Response) - response = response_or_exception - else - exception = response_or_exception - end - + def message(host, response, error_type) message = "Nextcloud server invalid host=#{host.inspect} error_type=#{error_type}" - message << " http_status=#{response.status}" if response + message << " http_status=#{response.status}" if response.respond_to?(:status) case error_type when :cannot_be_connected_to - message << ": exception #{exception.class}: #{exception}" if exception + message << ": #{response.class}: #{response}" when :not_nextcloud_server message << ": either was not valid json, or value at 'ocs/data/version/major' was not defined" when :minimal_nextcloud_version_unmet diff --git a/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb b/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb index ff8d1c28788..a7675b402fb 100644 --- a/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb @@ -83,6 +83,21 @@ RSpec.describe Storages::Storages::NextcloudContract, :storage_server_helpers, : end end + context 'with timeout' do + let(:storage) { build(:nextcloud_storage, :as_automatically_managed) } + + it 'fails validation' do + credentials_request = mock_nextcloud_application_credentials_validation(storage.host, timeout: true) + + expect(subject).not_to be_valid + expect(subject.errors.to_hash) + .to eq({ password: ["could not be validated. Please check your storage connection and try again."] }) + + # twice due to httpx retries plugin enabled. + expect(credentials_request).to have_been_made.twice + end + end + context 'with unknown error' do let(:storage) { build(:nextcloud_storage, :as_automatically_managed) } diff --git a/modules/storages/spec/contracts/storages/storages/shared_contract_examples.rb b/modules/storages/spec/contracts/storages/storages/shared_contract_examples.rb index 04c6673f9cf..2db6675f36f 100644 --- a/modules/storages/spec/contracts/storages/storages/shared_contract_examples.rb +++ b/modules/storages/spec/contracts/storages/storages/shared_contract_examples.rb @@ -157,30 +157,55 @@ RSpec.shared_examples_for 'nextcloud storage contract', :storage_server_helpers, let(:check_config_response_body) { nil } # use default let(:check_config_response_code) { nil } # use default let(:check_config_response_headers) { nil } # use default + let(:timeout_server_capabilities) { false } + let(:timeout_server_config_check) { false } + let(:stub_server_capabilities) do + mock_server_capabilities_response(storage_host, + response_code: capabilities_response_code, + response_headers: capabilities_response_headers, + response_body: capabilities_response_body, + timeout: timeout_server_capabilities, + response_nextcloud_major_version: capabilities_response_major_version) + end + let(:stub_config_check) do + mock_server_config_check_response(storage_host, + response_code: check_config_response_code, + response_headers: check_config_response_headers, + timeout: timeout_server_config_check, + response_body: check_config_response_body) + end before do # simulate host value changed to have GET request sent to check host URL validity storage.host_will_change! # simulate http response returned upon GET request - mock_server_capabilities_response(storage_host, - response_code: capabilities_response_code, - response_headers: capabilities_response_headers, - response_body: capabilities_response_body, - response_nextcloud_major_version: capabilities_response_major_version) - - mock_server_config_check_response(storage_host, - response_code: check_config_response_code, - response_headers: check_config_response_headers, - response_body: check_config_response_body) + stub_server_capabilities + stub_config_check end context 'when connection fails' do - before do - allow_any_instance_of(HTTPX::Session).to receive(:get).and_raise(HTTPX::TimeoutError, 'SIMULATED Timeout') + context 'when server capabilities request times out' do + let(:timeout_server_capabilities) { true } + + include_examples 'contract is invalid', host: :cannot_be_connected_to + + it 'retries failed request once' do + contract.validate + expect(stub_server_capabilities).to have_been_made.twice + end end - include_examples 'contract is invalid', host: :cannot_be_connected_to + context 'when server config check request times out' do + let(:timeout_server_config_check) { true } + + include_examples 'contract is invalid', host: :cannot_be_connected_to + + it 'retries failed request once' do + contract.validate + expect(stub_config_check).to have_been_made.twice + end + end end context 'when response code is a 404 NOT FOUND' do diff --git a/modules/storages/spec/support/storage_server_helpers.rb b/modules/storages/spec/support/storage_server_helpers.rb index 08ecc926637..33d5d91c314 100644 --- a/modules/storages/spec/support/storage_server_helpers.rb +++ b/modules/storages/spec/support/storage_server_helpers.rb @@ -31,6 +31,7 @@ module StorageServerHelpers response_code: nil, response_headers: nil, response_body: nil, + timeout: false, response_nextcloud_major_version: 22) response_code ||= 200 response_headers ||= { @@ -53,20 +54,25 @@ module StorageServerHelpers } } } - - stub_request( + stub = stub_request( :get, File.join(nextcloud_host, '/ocs/v2.php/cloud/capabilities') - ).to_return( - status: response_code, - headers: response_headers, - body: response_body ) + if timeout + stub.to_timeout + else + stub.to_return( + status: response_code, + headers: response_headers, + body: response_body + ) + end end def mock_server_config_check_response(nextcloud_host, response_code: nil, response_headers: nil, + timeout: false, response_body: nil) response_code ||= 200 response_headers ||= { @@ -80,20 +86,25 @@ module StorageServerHelpers "authorization_header": "Bearer TESTBEARERTOKEN" } } - - stub_request( + stub = stub_request( :get, File.join(nextcloud_host, 'index.php/apps/integration_openproject/check-config') - ).to_return( - status: response_code, - headers: response_headers, - body: response_body ) + if timeout + stub.to_timeout + else + stub.to_return( + status: response_code, + headers: response_headers, + body: response_body + ) + end end def mock_nextcloud_application_credentials_validation(nextcloud_host, username: 'OpenProject', password: 'Password123', + timeout: false, response_code: nil, response_headers: nil, response_body: nil) @@ -103,14 +114,19 @@ module StorageServerHelpers 'Authorization' => "Basic #{Base64::strict_encode64("#{username}:#{password}")}" } - stub_request( + stub = stub_request( :head, File.join(nextcloud_host, 'remote.php/dav') - ).to_return( - status: response_code, - headers: response_headers, - body: response_body ) + if timeout + stub.to_timeout + else + stub.to_return( + status: response_code, + headers: response_headers, + body: response_body + ) + end end end