[#53368] added changes to oauth configurations

- fixed unit tests for storage authentication
- fixed client credentials strategy

modules/storages/app/common/storages/peripherals/oauth_configurations/configuration_interface.rb

@@ -38,6 +38,8 @@ module Storages
 
         def basic_rack_oauth_client = raise ::Storages::Errors::SubclassResponsibility
 
+        def to_httpx_oauth_config = raise ::Storages::Errors::SubclassResponsibility
+
         private
 
         def authorization_check_wrapper

modules/storages/app/common/storages/peripherals/oauth_configurations/nextcloud_configuration.rb

@@ -32,19 +32,22 @@ module Storages
   module Peripherals
     module OAuthConfigurations
       class NextcloudConfiguration < ConfigurationInterface
+        Util = StorageInteraction::Nextcloud::Util
+
         attr_reader :oauth_client
 
+        # rubocop:disable Lint/MissingSuper
         def initialize(storage)
           @uri = storage.uri
           @oauth_client = storage.oauth_client.freeze
         end
 
-        def authorization_state_check(token)
-          util = ::Storages::Peripherals::StorageInteraction::Nextcloud::Util
+        # rubocop:enable Lint/MissingSuper
 
+        def authorization_state_check(token)
           authorization_check_wrapper do
             OpenProject.httpx.get(
-              util.join_uri_path(@uri, '/ocs/v1.php/cloud/user'),
+              Util.join_uri_path(@uri, '/ocs/v1.php/cloud/user'),
               headers: {
                 'Authorization' => "Bearer #{token}",
                 'OCS-APIRequest' => 'true',
@@ -58,6 +61,15 @@ module Storages
           rack_access_token.raw_attributes[:user_id]
         end
 
+        def to_httpx_oauth_config
+          StorageInteraction::AuthenticationStrategies::OAuthConfiguration.new(
+            client_id: @oauth_client.client_id,
+            client_secret: @oauth_client.client_secret,
+            issuer: URI(Util.join_uri_path(@uri, "/index.php/apps/oauth2/api/v1")).normalize,
+            scope: []
+          )
+        end
+
         def scope
           []
         end
@@ -70,8 +82,8 @@ module Storages
             scheme: @uri.scheme,
             host: @uri.host,
             port: @uri.port,
-            authorization_endpoint: File.join(@uri.path, "/index.php/apps/oauth2/authorize"),
-            token_endpoint: File.join(@uri.path, "/index.php/apps/oauth2/api/v1/token")
+            authorization_endpoint: Util.join_uri_path(@uri.path, "/index.php/apps/oauth2/authorize"),
+            token_endpoint: Util.join_uri_path(@uri.path, "/index.php/apps/oauth2/api/v1/token")
           )
         end
       end

modules/storages/app/common/storages/peripherals/oauth_configurations/one_drive_configuration.rb

@@ -32,39 +32,47 @@ module Storages
   module Peripherals
     module OAuthConfigurations
       class OneDriveConfiguration < ConfigurationInterface
-        DEFAULT_SCOPES = %w[offline_access files.readwrite.all user.read sites.readwrite.all].freeze
+        Util = StorageInteraction::OneDrive::Util
 
         attr_reader :oauth_client
 
+        # rubocop:disable Lint/MissingSuper
         def initialize(storage)
           @storage = storage
           @uri = storage.uri
           @oauth_client = storage.oauth_client
-          @oauth_uri = URI('https://login.microsoftonline.com/').normalize
+          @oauth_uri = URI("https://login.microsoftonline.com/#{@storage.tenant_id}/oauth2/v2.0").normalize
         end
 
-        def authorization_state_check(access_token)
-          util = ::Storages::Peripherals::StorageInteraction::OneDrive::Util
+        # rubocop:enable Lint/MissingSuper
 
+        def authorization_state_check(access_token)
           authorization_check_wrapper do
             OpenProject.httpx.get(
-              util.join_uri_path(@uri, '/v1.0/me'),
+              Util.join_uri_path(@uri, '/v1.0/me'),
               headers: { 'Authorization' => "Bearer #{access_token}", 'Accept' => 'application/json' }
             )
           end
         end
 
         def extract_origin_user_id(rack_access_token)
-          util = ::Storages::Peripherals::StorageInteraction::OneDrive::Util
-
           OpenProject.httpx.get(
-            util.join_uri_path(@uri, '/v1.0/me'),
+            Util.join_uri_path(@uri, '/v1.0/me'),
             headers: { 'Authorization' => "Bearer #{rack_access_token.access_token}", 'Accept' => 'application/json' }
           ).raise_for_status.json['id']
         end
 
+        def to_httpx_oauth_config
+          StorageInteraction::AuthenticationStrategies::OAuthConfiguration.new(
+            client_id: @oauth_client.client_id,
+            client_secret: @oauth_client.client_secret,
+            issuer: @oauth_uri,
+            scope: %w[https://graph.microsoft.com/.default]
+          )
+        end
+
         def scope
-          DEFAULT_SCOPES
+          %w[https://graph.microsoft.com/.default]
         end
 
         def basic_rack_oauth_client
@@ -75,8 +83,8 @@ module Storages
             scheme: @oauth_uri.scheme,
             host: @oauth_uri.host,
             port: @oauth_uri.port,
-            authorization_endpoint: "/#{@storage.tenant_id}/oauth2/v2.0/authorize",
-            token_endpoint: "/#{@storage.tenant_id}/oauth2/v2.0/token"
+            authorization_endpoint: "#{@oauth_uri.path}/authorize",
+            token_endpoint: "#{@oauth_uri.path}/token"
           )
         end
       end

modules/storages/app/common/storages/peripherals/storage_interaction/authentication_strategies/basic_auth.rb

@@ -41,14 +41,18 @@ module Storages
             username = storage.username
             password = storage.password
 
-            if username.blank? || password.blank?
-              log_message = 'Cannot authenticate storage with basic auth. Password or username not configured.'
-              data = ::Storages::StorageErrorData.new(source: self, payload: storage)
-              return Error.create(code: :error, log_message:, data:)
-            end
+            return build_failure(storage) if username.blank? || password.blank?
 
             yield OpenProject.httpx.basic_auth(username, password).with(http_options)
           end
+
+          private
+
+          def build_failure(storage)
+            log_message = 'Cannot authenticate storage with basic auth. Password or username not configured.'
+            data = ::Storages::StorageErrorData.new(source: self, payload: storage)
+            Failures::Builder.call(code: :error, log_message:, data:)
+          end
         end
       end
     end

modules/storages/app/common/storages/peripherals/storage_interaction/authentication_strategies/failures.rb

@@ -32,8 +32,8 @@ module Storages
   module Peripherals
     module StorageInteraction
       module AuthenticationStrategies
-        class Error
-          def self.create(code:, log_message:, data:)
+        module Failures
+          Builder = ->(code:, log_message:, data:) do
             storage_error = ::Storages::StorageError.new(code:, log_message:, data:)
             ServiceResult.failure(result: code, errors: storage_error)
           end

modules/storages/app/common/storages/peripherals/storage_interaction/authentication_strategies/o_auth_client_credentials.rb

@@ -37,20 +37,17 @@ module Storages
             Strategy.new(:oauth_client_credentials)
           end
 
-          def call(storage:, http_options: {})
+          def call(storage:, http_options: {}, &)
             config = storage.oauth_configuration.to_httpx_oauth_config
 
-            if config.complete?
-              create_http_and_yield(issuer: config.issuer,
-                                    client_id: config.client_id,
-                                    client_secret: config.client_secret,
-                                    scope: config.scope,
-                                    http_options:)
-            else
-              log_message = 'Cannot authenticate storage with client credential oauth flow. Storage not configured.'
-              data = ::Storages::StorageErrorData.new(source: self, payload: storage)
-              Error.create(code: :error, log_message:, data:)
-            end
+            return build_failure(storage) unless config.complete?
+
+            create_http_and_yield(issuer: config.issuer,
+                                  client_id: config.client_id,
+                                  client_secret: config.client_secret,
+                                  scope: config.scope,
+                                  http_options:,
+                                  &)
           end
 
           private
@@ -67,13 +64,19 @@ module Storages
                                 .with(http_options)
             rescue HTTPX::HTTPError => e
               data = ::Storages::StorageErrorData.new(source: self, payload: e.response.json)
-              return Error.create(code: :unauthorized,
-                                  log_message: 'Error while fetching OAuth access token.',
-                                  data:)
+              return Failures::Builder.call(code: :unauthorized,
+                                            log_message: 'Error while fetching OAuth access token.',
+                                            data:)
             end
 
             yield http
           end
+
+          def build_failure(storage)
+            log_message = 'Cannot authenticate storage with client credential oauth flow. Storage not configured.'
+            data = ::Storages::StorageErrorData.new(source: self, payload: storage)
+            Failures::Builder.call(code: :error, log_message:, data:)
+          end
         end
       end
     end

modules/storages/app/common/storages/peripherals/storage_interaction/authentication_strategies/o_auth_user_token.rb

@@ -48,9 +48,9 @@ module Storages
             current_token = OAuthClientToken.find_by(user_id: @user, oauth_client_id: config.oauth_client.id)
             if current_token.nil?
               data = ::Storages::StorageErrorData.new(source: self)
-              return Error.create(code: :unauthorized,
-                                  log_message: 'Authorization failed. No user access token found.',
-                                  data:)
+              return Failures::Builder.call(code: :unauthorized,
+                                            log_message: 'Authorization failed. No user access token found.',
+                                            data:)
             end
 
             opts = http_options.merge({ headers: { 'Authorization' => "Bearer #{current_token.access_token}" } })
@@ -81,9 +81,9 @@ module Storages
                                         .with(http_options)
             rescue HTTPX::HTTPError => e
               data = ::Storages::StorageErrorData.new(source: self, payload: e.response.json)
-              return Error.create(code: :unauthorized,
-                                  log_message: 'Error while refreshing OAuth token.',
-                                  data:)
+              return Failures::Builder.call(code: :unauthorized,
+                                            log_message: 'Error while refreshing OAuth token.',
+                                            data:)
             end
 
             response = yield http_session
@@ -92,9 +92,9 @@ module Storages
               success = update_refreshed_token(token, http_session)
               unless success
                 data = ::Storages::StorageErrorData.new(source: self)
-                return Error.create(code: :error,
-                                    log_message: 'Error while persisting updated access token.',
-                                    data:)
+                return Failures::Builder.call(code: :error,
+                                              log_message: 'Error while persisting updated access token.',
+                                              data:)
               end
             end
 

modules/storages/spec/common/storages/peripherals/storage_interaction/authentication_spec.rb

@@ -36,26 +36,41 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
 
   let(:user) { create(:user) }
 
+  shared_examples_for 'successful response' do |refreshed: false|
+    it "must #{refreshed ? 'refresh token and ' : ''}return success" do
+      result = described_class[auth_strategy].call(storage:, http_options:) { |http| make_request(http) }
+      expect(result).to be_success
+
+      result.match(
+        on_failure: ->(error) { fail "Expected success, got #{error}" },
+        on_success: ->(r) { expect(r).to eq('EXPECTED_RESULT') }
+      )
+    end
+  end
+
   context 'with a Nextcloud storage' do
     let(:storage) do
       create(:nextcloud_storage_with_local_connection, :as_not_automatically_managed, oauth_client_token_user: user)
     end
-    let(:folder) { Storages::Peripherals::ParentFolder.new('/') }
-
-    subject do
-      Storages::Peripherals::StorageInteraction::Nextcloud::FilesQuery.new(storage)
-    end
+    let(:request_url) { "#{storage.uri}ocs/v1.php/cloud/user" }
+    let(:http_options) { { headers: { 'OCS-APIRequest' => 'true', 'Accept' => 'application/json' } } }
 
     context 'with basic auth strategy' do
       let(:auth_strategy) { Storages::Peripherals::StorageInteraction::AuthenticationStrategies::BasicAuth.strategy }
 
-      subject do
-        Storages::Peripherals::StorageInteraction::Nextcloud::GroupUsersQuery.new(storage)
+      context 'with valid credentials', vcr: 'auth/nextcloud/basic_auth' do
+        before do
+          # Those values are only used to record the vcr cassette
+          storage.username = 'admin'
+          storage.password = 'admin'
+        end
+
+        it_behaves_like 'successful response'
       end
 
       context 'with empty username and password' do
         it 'must return error' do
-          result = subject.call(auth_strategy:, group: storage.group)
+          result = described_class[auth_strategy].call(storage:, http_options:) { |http| make_request(http) }
           expect(result).to be_failure
           expect(result.error_source)
             .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::BasicAuth)
@@ -67,16 +82,17 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid username and password', vcr: 'auth/nextcloud/storage_query_basic_auth_password_invalid' do
+      context 'with invalid username and/or password', vcr: 'auth/nextcloud/basic_auth_password_invalid' do
         before do
-          storage.password = 'IAmInvalid'
+          # Those values are only used to record the vcr cassette
+          storage.username = 'admin'
+          storage.password = 'YouShallNot(Multi)Pass'
         end
 
         it 'must return unauthorized' do
-          result = subject.call(auth_strategy:, group: storage.group)
+          result = described_class[auth_strategy].call(storage:, http_options:) { |http| make_request(http) }
           expect(result).to be_failure
-          expect(result.error_source)
-            .to be_a(Storages::Peripherals::StorageInteraction::Nextcloud::GroupUsersQuery)
+          expect(result.error_source).to eq('EXECUTING_QUERY')
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -100,7 +116,7 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
 
         it 'must return unauthorized' do
-          result = subject.call(auth_strategy:, folder:)
+          result = described_class[auth_strategy].call(storage:, http_options:) { |http| make_request(http) }
           expect(result).to be_failure
           expect(result.error_source)
             .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
@@ -112,9 +128,9 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth refresh token', vcr: 'auth/nextcloud/storage_query_user_token_refresh_token_invalid' do
+      context 'with invalid oauth refresh token', vcr: 'auth/nextcloud/user_token_refresh_token_invalid' do
         it 'must return unauthorized' do
-          result = subject.call(auth_strategy:, folder:)
+          result = described_class[auth_strategy].call(storage:, http_options:) { |http| make_request(http) }
           expect(result).to be_failure
           expect(result.error_source)
             .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
@@ -126,36 +142,43 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth access token', vcr: 'auth/nextcloud/storage_query_user_token_access_token_invalid' do
-        it 'must refresh token and return success' do
-          result = subject.call(auth_strategy:, folder:)
-          expect(result).to be_success
-
-          result.match(
-            on_failure: ->(error) { fail "Expected success, got #{error}" },
-            on_success: ->(file_infos) { expect(file_infos).to be_a(Storages::StorageFiles) }
-          )
-        end
+      context 'with invalid oauth access token', vcr: 'auth/nextcloud/user_token_access_token_invalid' do
+        it_behaves_like 'successful response', refreshed: true
       end
     end
   end
 
   context 'with a OneDrive/SharePoint storage' do
     let(:storage) { create(:sharepoint_dev_drive_storage, oauth_client_token_user: user) }
-    let(:folder) { Storages::Peripherals::ParentFolder.new('/') }
-
-    subject do
-      Storages::Peripherals::StorageInteraction::OneDrive::FilesQuery.new(storage)
-    end
+    let(:http_options) { {} }
 
     context 'with client credentials strategy' do
+      let(:request_url) { "#{storage.uri}v1.0/drives" }
       let(:auth_strategy) do
         Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthClientCredentials.strategy
       end
 
-      context 'with invalid oauth credentials', vcr: 'auth/one_drive/storage_query_client_credentials_invalid' do
+      context 'with valid oauth credentials', vcr: 'auth/one_drive/client_credentials' do
+        it_behaves_like 'successful response'
+      end
+
+      context 'with invalid client secret', vcr: 'auth/one_drive/client_credentials_invalid_client_secret' do
         it 'must return unauthorized' do
-          result = subject.call(auth_strategy:, folder:)
+          result = described_class[auth_strategy].call(storage:) { |http| make_request(http) }
+          expect(result).to be_failure
+          expect(result.error_source)
+            .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthClientCredentials)
+
+          result.match(
+            on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
+            on_success: ->(file_infos) { fail "Expected failure, got #{file_infos}" }
+          )
+        end
+      end
+
+      context 'with invalid client id', vcr: 'auth/one_drive/client_credentials_invalid_client_id' do
+        it 'must return unauthorized' do
+          result = described_class[auth_strategy].call(storage:) { |http| make_request(http) }
           expect(result).to be_failure
           expect(result.error_source)
             .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthClientCredentials)
@@ -169,10 +192,15 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
     end
 
     context 'with user token strategy' do
+      let(:request_url) { "#{storage.uri}v1.0/me" }
       let(:auth_strategy) do
         Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken.strategy.with_user(user)
       end
 
+      context 'with valid access token', vcr: 'auth/one_drive/user_token' do
+        it_behaves_like 'successful response'
+      end
+
       context 'with not existent oauth token' do
         let(:user_without_token) { create(:user) }
         let(:auth_strategy) do
@@ -182,7 +210,7 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
 
         it 'must return unauthorized' do
-          result = subject.call(auth_strategy:, folder:)
+          result = described_class[auth_strategy].call(storage:) { |http| make_request(http) }
           expect(result).to be_failure
           expect(result.error_source)
             .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
@@ -194,9 +222,9 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth refresh token', vcr: 'auth/one_drive/storage_query_user_token_refresh_token_invalid' do
+      context 'with invalid oauth refresh token', vcr: 'auth/one_drive/user_token_refresh_token_invalid' do
         it 'must return unauthorized' do
-          result = subject.call(auth_strategy:, folder:)
+          result = described_class[auth_strategy].call(storage:) { |http| make_request(http) }
           expect(result).to be_failure
           expect(result.error_source)
             .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
@@ -208,19 +236,35 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth access token', vcr: 'auth/one_drive/storage_query_user_token_access_token_invalid' do
-        it 'must return unauthorized' do
-          result = subject.call(auth_strategy:, folder:)
-          expect(result).to be_failure
-          expect(result.error_source)
-            .to be_a(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
-
-          result.match(
-            on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
-            on_success: ->(file_infos) { fail "Expected failure, got #{file_infos}" }
-          )
-        end
+      context 'with invalid oauth access token', vcr: 'auth/one_drive/user_token_access_token_invalid' do
+        it_behaves_like 'successful response', refreshed: true
       end
     end
   end
+
+  private
+
+  def make_request(http)
+    handle_response http.get(request_url)
+  end
+
+  def handle_response(response)
+    case response
+    in { status: 200..299 }
+      ServiceResult.success(result: 'EXPECTED_RESULT')
+    in { status: 401 }
+      error(:unauthorized)
+    in { status: 403 }
+      error(:forbidden)
+    in { status: 404 }
+      error(:not_found)
+    else
+      error(:error)
+    end
+  end
+
+  def error(code)
+    data = Storages::StorageErrorData.new(source: 'EXECUTING_QUERY')
+    ServiceResult.failure(result: code, errors: Storages::StorageError.new(code:, data:))
+  end
 end

modules/storages/spec/common/storages/peripherals/storage_interaction/network_error_spec.rb

@@ -37,29 +37,54 @@ RSpec.describe 'network errors for storage interaction' do
 
   let(:user) { create(:user) }
   let(:storage) { create(:sharepoint_dev_drive_storage, oauth_client_token_user: user) }
-  let(:folder) { Storages::Peripherals::ParentFolder.new('/') }
+  let(:request_url) { 'https://my.timeout.org/' }
   let(:auth_strategy) do
     Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken.strategy.with_user(user)
   end
 
-  subject do
-    Storages::Peripherals::StorageInteraction::OneDrive::FilesQuery.new(storage)
-  end
-
   context 'if a timeout happens' do
     before do
-      request = HTTPX::Request.new(:get, 'https://my.timeout.org/')
+      request = HTTPX::Request.new(:get, request_url)
       httpx_double = class_double(HTTPX, get: HTTPX::ErrorResponse.new(request, 'Timeout happens', {}))
       allow(httpx_double).to receive(:with).and_return(httpx_double)
       allow(OpenProject).to receive(:httpx).and_return(httpx_double)
     end
 
     it 'must return an error with wrapped network error response' do
-      error = subject.call(auth_strategy:, folder:)
-      expect(error).to be_failure
-      expect(error.result).to eq(:error)
-      expect(error.error_payload).to be_a(HTTPX::ErrorResponse)
+      result = Storages::Peripherals::StorageInteraction::Authentication[auth_strategy].call(storage:) do |http|
+        make_request(http)
+      end
+
+      expect(result).to be_failure
+      expect(result.result).to eq(:error)
+      expect(result.error_payload).to be_a(HTTPX::ErrorResponse)
     end
   end
+
+  private
+
+  def make_request(http)
+    handle_response http.get(request_url)
+  end
+
+  def handle_response(response)
+    case response
+    in { status: 200..299 }
+      ServiceResult.success(result: 'EXPECTED_RESULT')
+    in { status: 401 }
+      error(:unauthorized)
+    in { status: 403 }
+      error(:forbidden)
+    in { status: 404 }
+      error(:not_found)
+    else
+      error(:error, response)
+    end
+  end
+
+  def error(code, payload = nil)
+    data = Storages::StorageErrorData.new(source: 'EXECUTING_QUERY', payload:)
+    ServiceResult.failure(result: code, errors: Storages::StorageError.new(code:, data:))
+  end
 end
 # rubocop:enable RSpec/DescribeClass

spec/services/oauth_clients/one_drive_connection_manager_spec.rb

@@ -104,7 +104,7 @@ RSpec.describe OAuthClients::ConnectionManager, :webmock, type: :model do
     it 'always add the necessary scopes' do
       uri = connection_manager.get_authorization_uri(state: nil)
 
-      expect(uri).to include storage.oauth_configuration.scope.join('%20')
+      expect(uri).to include CGI.escape(storage.oauth_configuration.scope.join(' '))
     end
 
     it 'adds the state if present' do