From 8486b2cacb9ef9f250eba7972ef07dace904f73b Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Wed, 15 Jan 2025 10:11:14 +0100 Subject: [PATCH] Add configuration for authentication method Preparing for users to choose how they want to authenticate their nextcloud storage and if using a common OAuth 2.0 IDP, what the client_id of nextcloud is. --- .../storages/storages/nextcloud_contract.rb | 10 +++ .../app/models/storages/nextcloud_storage.rb | 8 ++ ...00336_set_default_authentication_method.rb | 9 +++ .../storages/nextcloud_contract_spec.rb | 78 ++++++++++++++++--- .../storages/shared_contract_examples.rb | 3 +- .../spec/factories/storage_factory.rb | 2 + 6 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 modules/storages/db/migrate/20250115100336_set_default_authentication_method.rb diff --git a/modules/storages/app/contracts/storages/storages/nextcloud_contract.rb b/modules/storages/app/contracts/storages/storages/nextcloud_contract.rb index 7f718f7e9d5e..d9d218132dd0 100644 --- a/modules/storages/app/contracts/storages/storages/nextcloud_contract.rb +++ b/modules/storages/app/contracts/storages/storages/nextcloud_contract.rb @@ -36,6 +36,12 @@ class NextcloudContract < ::ModelContract # But only do so if the validations above for URL were successful. validates :host, secure_context_uri: true, nextcloud_compatible_host: true, unless: -> { errors.include?(:host) } + attribute :authentication_method + validates :authentication_method, presence: true, inclusion: { in: ::Storages::NextcloudStorage::AUTHENTICATION_METHODS } + + attribute :nextcloud_audience + validates :nextcloud_audience, presence: true, if: :nextcloud_storage_authenticate_via_idp? + attribute :automatically_managed attribute :username @@ -68,6 +74,10 @@ def nextcloud_default_storage_username? @model.username == @model.provider_fields_defaults[:username] end + def nextcloud_storage_authenticate_via_idp? + nextcloud_storage? && @model.authenticate_via_idp? + end + def nextcloud_storage? @model.is_a?(Storages::NextcloudStorage) end diff --git a/modules/storages/app/models/storages/nextcloud_storage.rb b/modules/storages/app/models/storages/nextcloud_storage.rb index 29feae123673..51ddfb1f1f51 100644 --- a/modules/storages/app/models/storages/nextcloud_storage.rb +++ b/modules/storages/app/models/storages/nextcloud_storage.rb @@ -35,11 +35,15 @@ class NextcloudStorage < Storage username: "OpenProject" }.freeze + AUTHENTICATION_METHODS = %w[two_way_oauth2 oauth2_sso].freeze + store_attribute :provider_fields, :automatically_managed, :boolean store_attribute :provider_fields, :username, :string store_attribute :provider_fields, :password, :string store_attribute :provider_fields, :group, :string store_attribute :provider_fields, :group_folder, :string + store_attribute :provider_fields, :authentication_method, :string, default: "two_way_oauth2" + store_attribute :provider_fields, :nextcloud_audience, :string def oauth_configuration Peripherals::OAuthConfigurations::NextcloudConfiguration.new(self) @@ -62,6 +66,10 @@ def available_project_folder_modes end end + def authenticate_via_idp? + authentication_method == "oauth2_sso" + end + def configuration_checks { storage_oauth_client_configured: oauth_client.present?, diff --git a/modules/storages/db/migrate/20250115100336_set_default_authentication_method.rb b/modules/storages/db/migrate/20250115100336_set_default_authentication_method.rb new file mode 100644 index 000000000000..be849e539a2f --- /dev/null +++ b/modules/storages/db/migrate/20250115100336_set_default_authentication_method.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class SetDefaultAuthenticationMethod < ActiveRecord::Migration[7.1] + def change + auth_json = { authentication_method: "two_way_oauth2" }.to_json + Storages::NextcloudStorage.where("NOT provider_fields ? 'authentication_method'") + .update_all("provider_fields = provider_fields || '#{auth_json}'::jsonb") + end +end 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 147626b4055d..8499a238cba2 100644 --- a/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb @@ -33,16 +33,17 @@ RSpec.describe Storages::Storages::NextcloudContract, :storage_server_helpers, :webmock do let(:current_user) { create(:admin) } - let(:storage_host) { "https://host1.example.com" } - let(:storage) { build(:nextcloud_storage, host: storage_host) } + let(:storage) { build(:nextcloud_storage) } + let(:mocked_host) { storage.host } + + let!(:capabilities_request) { mock_server_capabilities_response(mocked_host) } + let!(:host_request) { mock_server_config_check_response(mocked_host) } # As the NextcloudContract is selected by the BaseContract to make writable attributes available, # the BaseContract needs to be instantiated here. subject { Storages::Storages::BaseContract.new(storage, current_user) } it "checks the storage url only when changed" do - capabilities_request = mock_server_capabilities_response(storage_host) - host_request = mock_server_config_check_response(storage_host) subject.validate expect(capabilities_request).to have_been_made.once expect(host_request).to have_been_made.once @@ -58,11 +59,6 @@ context "with valid credentials" do let(:storage) { build(:nextcloud_storage, :as_automatically_managed) } - before do - mock_server_capabilities_response(storage.host) - mock_server_config_check_response(storage.host) - end - it "passes validation" do credentials_request = mock_nextcloud_application_credentials_validation(storage.host) @@ -137,6 +133,7 @@ context "when the storage host is nil" do let(:storage) { build(:nextcloud_storage, :as_automatically_managed, host: nil) } + let(:mocked_host) { "https://example.com/unrelated" } before do allow(NextcloudApplicationCredentialsValidator).to receive(:new).and_call_original @@ -149,4 +146,67 @@ end end end + + describe "authentication_method validation" do + let(:storage) { build(:nextcloud_storage, :as_not_automatically_managed, authentication_method:) } + let(:authentication_method) { "two_way_oauth2" } + + it { is_expected.to be_valid } + + context "when the authentication method is oauth2_sso" do + let(:authentication_method) { "oauth2_sso" } + + it { is_expected.to be_valid } + end + + context "when the authentication method is unknown" do + let(:authentication_method) { "magic_unicorns" } + + it { is_expected.not_to be_valid } + end + + context "when the authentication method is missing" do + let(:authentication_method) { nil } + + it { is_expected.not_to be_valid } + end + end + + describe "nextcloud_audience validation" do + let(:storage) do + build(:nextcloud_storage, :as_not_automatically_managed, authentication_method:, nextcloud_audience:) + end + + context "when authentication happens through bidirectional OAuth 2.0" do + let(:authentication_method) { "two_way_oauth2" } + + context "and there is no nextcloud_audience" do + let(:nextcloud_audience) { nil } + + it { is_expected.to be_valid } + end + + context "and there is a nextcloud_audience" do + let(:nextcloud_audience) { "nextcloud" } + + it { is_expected.to be_valid } + end + end + + context "when authentication happens through a common IDP" do + let(:authentication_method) { "oauth2_sso" } + + context "and there is no nextcloud_audience" do + let(:nextcloud_audience) { nil } + + it { is_expected.not_to be_valid } + end + + context "and there is a nextcloud_audience" do + let(:nextcloud_audience) { "nextcloud" } + + it { is_expected.to be_valid } + end + end + end end 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 ee4f284a5d96..4ffb31a273ff 100644 --- a/modules/storages/spec/contracts/storages/storages/shared_contract_examples.rb +++ b/modules/storages/spec/contracts/storages/storages/shared_contract_examples.rb @@ -294,8 +294,7 @@ context "when not automatically managed, no username or password" do before do - storage.provider_fields = {} - storage.assign_attributes(automatic_management_enabled: false) + storage.assign_attributes(automatic_management_enabled: false, username: nil, password: nil) end it_behaves_like "contract is valid" diff --git a/modules/storages/spec/factories/storage_factory.rb b/modules/storages/spec/factories/storage_factory.rb index f31ed752dd2b..8dae7ab07147 100644 --- a/modules/storages/spec/factories/storage_factory.rb +++ b/modules/storages/spec/factories/storage_factory.rb @@ -91,6 +91,8 @@ class: "::Storages::NextcloudStorage" do provider_type { Storages::Storage::PROVIDER_TYPE_NEXTCLOUD } sequence(:host) { |n| "https://host#{n}.example.com/" } + authentication_method { "two_way_oauth2" } + nextcloud_audience { "nextcloud" } trait :as_automatically_managed do automatic_management_enabled { true }