[#53368] use active model validations in oauth config

- use only class, not instance in error payload

docker-compose.yml

@@ -1,5 +1,3 @@
-version: '3.8'
-
 networks:
   network:
   testing:

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

@@ -47,11 +47,11 @@ module Storages
         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',
-                'Accept' => 'application/json'
+                "Authorization" => "Bearer #{token}",
+                "OCS-APIRequest" => "true",
+                "Accept" => "application/json"
               }
             )
           end

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

@@ -49,17 +49,17 @@ module Storages
         def authorization_state_check(access_token)
           authorization_check_wrapper do
             OpenProject.httpx.get(
-              Util.join_uri_path(@uri, '/v1.0/me'),
-              headers: { 'Authorization' => "Bearer #{access_token}", 'Accept' => 'application/json' }
+              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)
           OpenProject.httpx.get(
-            Util.join_uri_path(@uri, '/v1.0/me'),
-            headers: { 'Authorization' => "Bearer #{rack_access_token.access_token}", 'Accept' => 'application/json' }
-          ).raise_for_status.json['id']
+            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

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

@@ -49,8 +49,8 @@ module Storages
           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)
+            log_message = "Cannot authenticate storage with basic auth. Password or username not configured."
+            data = ::Storages::StorageErrorData.new(source: self.class, payload: storage)
             Failures::Builder.call(code: :error, log_message:, data:)
           end
         end

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

@@ -40,7 +40,7 @@ module Storages
           def call(storage:, http_options: {}, &)
             config = storage.oauth_configuration.to_httpx_oauth_config
 
-            return build_failure(storage) unless config.complete?
+            return build_failure(storage) unless config.valid?
 
             create_http_and_yield(issuer: config.issuer,
                                   client_id: config.client_id,
@@ -59,13 +59,13 @@ module Storages
                                             client_id:,
                                             client_secret:,
                                             scope:,
-                                            token_endpoint_auth_method: 'client_secret_post')
+                                            token_endpoint_auth_method: "client_secret_post")
                                 .with_access_token
                                 .with(http_options)
             rescue HTTPX::HTTPError => e
-              data = ::Storages::StorageErrorData.new(source: self, payload: e.response.json)
+              data = ::Storages::StorageErrorData.new(source: self.class, payload: e.response.json)
               return Failures::Builder.call(code: :unauthorized,
-                                            log_message: 'Error while fetching OAuth access token.',
+                                            log_message: "Error while fetching OAuth access token.",
                                             data:)
             end
 
@@ -73,8 +73,8 @@ module Storages
           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)
+            log_message = "Cannot authenticate storage with client credential oauth flow. Storage not configured."
+            data = ::Storages::StorageErrorData.new(source: self.class, payload: storage)
             Failures::Builder.call(code: :error, log_message:, data:)
           end
         end

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

@@ -33,8 +33,12 @@ module Storages
     module StorageInteraction
       module AuthenticationStrategies
         class OAuthConfiguration
+          include ActiveModel::Validations
+
           attr_reader :scope, :issuer, :client_secret, :client_id
 
+          validates_presence_of :client_id, :client_secret, :issuer
+
           def initialize(client_id: nil,
                          client_secret: nil,
                          issuer: nil,
@@ -44,10 +48,6 @@ module Storages
             @issuer = issuer
             @scope = scope
           end
-
-          def complete?
-            [@client_id, @client_secret, @issuer].all?(&:present?)
-          end
         end
       end
     end

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

@@ -38,7 +38,6 @@ module Storages
           end
 
           def initialize(user)
-            super()
             @user = user
           end
 
@@ -47,19 +46,22 @@ module Storages
             config = storage.oauth_configuration
             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)
+              data = ::Storages::StorageErrorData.new(source: self.class)
               return Failures::Builder.call(code: :unauthorized,
-                                            log_message: 'Authorization failed. No user access token found.',
+                                            log_message: "Authorization failed. No user access token found.",
                                             data:)
             end
 
-            opts = http_options.merge({ headers: { 'Authorization' => "Bearer #{current_token.access_token}" } })
+            opts = http_options.merge({ headers: { "Authorization" => "Bearer #{current_token.access_token}" } })
             response_with_current_token = yield OpenProject.httpx.with(opts)
 
             if response_with_current_token.success? || response_with_current_token.result != :unauthorized
               response_with_current_token
             else
-              refresh_and_retry(config.to_httpx_oauth_config, http_options, current_token, &)
+              httpx_oauth_config = config.to_httpx_oauth_config
+              return build_failure(storage) unless httpx_oauth_config.valid?
+
+              refresh_and_retry(httpx_oauth_config, http_options, current_token, &)
             end
           end
 
@@ -76,13 +78,13 @@ module Storages
                                                     client_secret: config.client_secret,
                                                     scope: config.scope,
                                                     refresh_token: token.refresh_token,
-                                                    token_endpoint_auth_method: 'client_secret_post')
+                                                    token_endpoint_auth_method: "client_secret_post")
                                         .with_access_token
                                         .with(http_options)
             rescue HTTPX::HTTPError => e
-              data = ::Storages::StorageErrorData.new(source: self, payload: e.response.json)
+              data = ::Storages::StorageErrorData.new(source: self.class, payload: e.response.json)
               return Failures::Builder.call(code: :unauthorized,
-                                            log_message: 'Error while refreshing OAuth token.',
+                                            log_message: "Error while refreshing OAuth token.",
                                             data:)
             end
 
@@ -91,9 +93,9 @@ module Storages
             if response.success?
               success = update_refreshed_token(token, http_session)
               unless success
-                data = ::Storages::StorageErrorData.new(source: self)
+                data = ::Storages::StorageErrorData.new(source: self.class)
                 return Failures::Builder.call(code: :error,
-                                              log_message: 'Error while persisting updated access token.',
+                                              log_message: "Error while persisting updated access token.",
                                               data:)
               end
             end
@@ -110,6 +112,12 @@ module Storages
 
             token.update(access_token:, refresh_token:)
           end
+
+          def build_failure(storage)
+            log_message = "Cannot refresh user token for storage. Storage authentication credentials not configured."
+            data = ::Storages::StorageErrorData.new(source: self.class, payload: storage)
+            Failures::Builder.call(code: :error, log_message:, data:)
+          end
         end
       end
     end

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

@@ -28,7 +28,7 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-require 'spec_helper'
+require "spec_helper"
 require_module_spec_helper
 
 RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmock do
@@ -36,44 +36,44 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
 
   let(:user) { create(:user) }
 
-  shared_examples_for 'successful response' do |refreshed: false|
+  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') }
+        on_success: ->(r) { expect(r).to eq("EXPECTED_RESULT") }
       )
     end
   end
 
-  context 'with a Nextcloud storage' do
+  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(:request_url) { "#{storage.uri}ocs/v1.php/cloud/user" }
-    let(:http_options) { { headers: { 'OCS-APIRequest' => 'true', 'Accept' => 'application/json' } } }
+    let(:http_options) { { headers: { "OCS-APIRequest" => "true", "Accept" => "application/json" } } }
 
-    context 'with basic auth strategy' do
+    context "with basic auth strategy" do
       let(:auth_strategy) { Storages::Peripherals::StorageInteraction::AuthenticationStrategies::BasicAuth.strategy }
 
-      context 'with valid credentials', vcr: 'auth/nextcloud/basic_auth' do
+      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'
+          storage.username = "admin"
+          storage.password = "admin"
         end
 
-        it_behaves_like 'successful response'
+        it_behaves_like "successful response"
       end
 
-      context 'with empty username and password' do
-        it 'must return error' do
+      context "with empty username and password" do
+        it "must return error" do
           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)
+            .to be(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::BasicAuth)
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:error) },
@@ -82,17 +82,17 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid username and/or password', vcr: 'auth/nextcloud/basic_auth_password_invalid' do
+      context "with invalid username and/or password", vcr: "auth/nextcloud/basic_auth_password_invalid" do
         before do
           # Those values are only used to record the vcr cassette
-          storage.username = 'admin'
-          storage.password = 'YouShallNot(Multi)Pass'
+          storage.username = "admin"
+          storage.password = "YouShallNot(Multi)Pass"
         end
 
-        it 'must return unauthorized' do
+        it "must return unauthorized" do
           result = described_class[auth_strategy].call(storage:, http_options:) { |http| make_request(http) }
           expect(result).to be_failure
-          expect(result.error_source).to eq('EXECUTING_QUERY')
+          expect(result.error_source).to eq("EXECUTING_QUERY")
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -102,12 +102,12 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
       end
     end
 
-    context 'with user token strategy' do
+    context "with user token strategy" do
       let(:auth_strategy) do
         Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken.strategy.with_user(user)
       end
 
-      context 'with not existent oauth token' do
+      context "with not existent oauth token" do
         let(:user_without_token) { create(:user) }
         let(:auth_strategy) do
           Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken
@@ -115,11 +115,11 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
             .with_user(user_without_token)
         end
 
-        it 'must return unauthorized' do
+        it "must return unauthorized" do
           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)
+            .to be(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -128,12 +128,12 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth refresh token', vcr: 'auth/nextcloud/user_token_refresh_token_invalid' do
-        it 'must return unauthorized' do
+      context "with invalid oauth refresh token", vcr: "auth/nextcloud/user_token_refresh_token_invalid" do
+        it "must return unauthorized" do
           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)
+            .to be(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -142,32 +142,32 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth access token', vcr: 'auth/nextcloud/user_token_access_token_invalid' do
-        it_behaves_like 'successful response', refreshed: true
+      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
+  context "with a OneDrive/SharePoint storage" do
     let(:storage) { create(:sharepoint_dev_drive_storage, oauth_client_token_user: user) }
     let(:http_options) { {} }
 
-    context 'with client credentials strategy' do
+    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 valid oauth credentials', vcr: 'auth/one_drive/client_credentials' do
-        it_behaves_like 'successful response'
+      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
+      context "with invalid client secret", vcr: "auth/one_drive/client_credentials_invalid_client_secret" 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)
+            .to be(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthClientCredentials)
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -176,12 +176,12 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid client id', vcr: 'auth/one_drive/client_credentials_invalid_client_id' do
-        it 'must return unauthorized' do
+      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)
+            .to be(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthClientCredentials)
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -191,17 +191,17 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
       end
     end
 
-    context 'with user token strategy' do
+    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'
+      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
+      context "with not existent oauth token" do
         let(:user_without_token) { create(:user) }
         let(:auth_strategy) do
           Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken
@@ -209,11 +209,11 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
             .with_user(user_without_token)
         end
 
-        it 'must return unauthorized' 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::OAuthUserToken)
+            .to be(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -222,12 +222,12 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth refresh token', vcr: 'auth/one_drive/user_token_refresh_token_invalid' do
-        it 'must return unauthorized' do
+      context "with invalid oauth refresh token", vcr: "auth/one_drive/user_token_refresh_token_invalid" 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::OAuthUserToken)
+            .to be(Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken)
 
           result.match(
             on_failure: ->(error) { expect(error.code).to eq(:unauthorized) },
@@ -236,8 +236,8 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
         end
       end
 
-      context 'with invalid oauth access token', vcr: 'auth/one_drive/user_token_access_token_invalid' do
-        it_behaves_like 'successful response', refreshed: true
+      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
@@ -251,7 +251,7 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
   def handle_response(response)
     case response
     in { status: 200..299 }
-      ServiceResult.success(result: 'EXPECTED_RESULT')
+      ServiceResult.success(result: "EXPECTED_RESULT")
     in { status: 401 }
       error(:unauthorized)
     in { status: 403 }
@@ -264,7 +264,7 @@ RSpec.describe Storages::Peripherals::StorageInteraction::Authentication, :webmo
   end
 
   def error(code)
-    data = Storages::StorageErrorData.new(source: 'EXECUTING_QUERY')
+    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

@@ -28,29 +28,29 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-require 'spec_helper'
+require "spec_helper"
 require_module_spec_helper
 
 # rubocop:disable RSpec/DescribeClass
-RSpec.describe 'network errors for storage interaction' do
+RSpec.describe "network errors for storage interaction" do
   using Storages::Peripherals::ServiceResultRefinements
 
   let(:user) { create(:user) }
   let(:storage) { create(:sharepoint_dev_drive_storage, oauth_client_token_user: user) }
-  let(:request_url) { 'https://my.timeout.org/' }
+  let(:request_url) { "https://my.timeout.org/" }
   let(:auth_strategy) do
     Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken.strategy.with_user(user)
   end
 
-  context 'if a timeout happens' do
+  context "if a timeout happens" do
     before do
       request = HTTPX::Request.new(:get, request_url)
-      httpx_double = class_double(HTTPX, get: HTTPX::ErrorResponse.new(request, 'Timeout happens', {}))
+      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
+    it "must return an error with wrapped network error response" do
       result = Storages::Peripherals::StorageInteraction::Authentication[auth_strategy].call(storage:) do |http|
         make_request(http)
       end
@@ -70,7 +70,7 @@ RSpec.describe 'network errors for storage interaction' do
   def handle_response(response)
     case response
     in { status: 200..299 }
-      ServiceResult.success(result: 'EXPECTED_RESULT')
+      ServiceResult.success(result: "EXPECTED_RESULT")
     in { status: 401 }
       error(:unauthorized)
     in { status: 403 }
@@ -83,7 +83,7 @@ RSpec.describe 'network errors for storage interaction' do
   end
 
   def error(code, payload = nil)
-    data = Storages::StorageErrorData.new(source: 'EXECUTING_QUERY', payload:)
+    data = Storages::StorageErrorData.new(source: "EXECUTING_QUERY", payload:)
     ServiceResult.failure(result: code, errors: Storages::StorageError.new(code:, data:))
   end
 end