From 18b7ebf77c45d48d53eef28a3cda9b218152d090 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Thu, 20 Feb 2025 09:48:38 +0100 Subject: [PATCH 1/4] Allow admins to choose not exchanging for specific SSO token This is intended for simple setups, where the token returned by the IDP can be immediately used for authentication at a storage. --- .../storages/storage-audience.controller.ts | 58 +++++++++++++++++++ ...nextcloud_audience_form_component.html.erb | 6 +- .../nextcloud_audience_info_component.erb | 8 +-- .../nextcloud_audience_info_component.rb | 11 ++++ .../storages/admin/storages_controller.rb | 1 + .../admin/nextcloud_audience_input_form.rb | 43 +++++++++++--- .../storages/set_attributes_service.rb | 8 +++ modules/storages/config/locales/en.yml | 10 +++- .../storages/admin/create_storage_spec.rb | 2 +- 9 files changed, 129 insertions(+), 18 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts diff --git a/frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts b/frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts new file mode 100644 index 000000000000..ecbf280edcca --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts @@ -0,0 +1,58 @@ +/* + * -- copyright + * OpenProject is an open source project management software. + * Copyright (C) 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. + * ++ + */ + +import { Controller } from '@hotwired/stimulus'; + +export default class StorageAudienceController extends Controller { + static targets = [ + 'audienceInput', + 'audienceInputWrapper', + 'idpRadio', + ]; + + declare readonly audienceInputTarget:HTMLInputElement; + declare readonly audienceInputWrapperTarget:HTMLDivElement; + declare readonly idpRadioTarget:HTMLInputElement; + + connect() { + if (this.idpRadioTarget.checked) { + this.hideAudienceInput(); + } + } + + showAudienceInput() { + this.audienceInputTarget.value = ''; + this.audienceInputWrapperTarget.classList.remove('d-none'); + } + + hideAudienceInput() { + this.audienceInputWrapperTarget.classList.add('d-none'); + } +} diff --git a/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb b/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb index b720fe418f7d..939cdd5aedae 100644 --- a/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb +++ b/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb @@ -34,7 +34,11 @@ See COPYRIGHT and LICENSE files for more details. model:, url: form_url, method: :patch, - data: { turbo_frame: "page-content" } + data: { + turbo_frame: "page-content", + application_target: "dynamic", + controller: "storages--storage-audience" + } ) do |form| flex_layout do |general_info_row| general_info_row.with_row(mb: 3) do diff --git a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb index 8046080c0480..4688465bc306 100644 --- a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb +++ b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb @@ -36,13 +36,7 @@ See COPYRIGHT and LICENSE files for more details. end grid.with_area(:description, tag: :div, color: :subtle, test_selector: 'nextcloud-audience-description') do - concat(render(Primer::Beta::Text.new) { - if storage.nextcloud_audience.present? - I18n.t('storages.file_storage_view.nextcloud_audience_description', audience: storage.nextcloud_audience) - else - I18n.t('storages.file_storage_view.nextcloud_audience_blank') - end - }) + concat(render(Primer::Beta::Text.new) { audience_summary }) end if editable_storage? diff --git a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb index dc2d030d2dc2..6c0ff21cad76 100644 --- a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb +++ b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb @@ -32,6 +32,17 @@ module Storages module Admin class NextcloudAudienceInfoComponent < StorageInfoComponent def self.wrapper_key = :storage_nextcloud_audience_section + + def audience_summary + case storage.nextcloud_audience + when "" + I18n.t("storages.file_storage_view.nextcloud_audience_blank") + when OpenIDConnect::UserToken::IDP_AUDIENCE + I18n.t("storages.file_storage_view.nextcloud_audience_idp") + else + I18n.t("storages.file_storage_view.nextcloud_audience_description", audience: storage.nextcloud_audience) + end + end end end end diff --git a/modules/storages/app/controllers/storages/admin/storages_controller.rb b/modules/storages/app/controllers/storages/admin/storages_controller.rb index cd90f2138f9f..fed0763d292a 100644 --- a/modules/storages/app/controllers/storages/admin/storages_controller.rb +++ b/modules/storages/app/controllers/storages/admin/storages_controller.rb @@ -252,6 +252,7 @@ def permitted_storage_params(model_parameter_name = storage_provider_parameter_n "provider_type", "host", "authentication_method", + "audience_configuration", "nextcloud_audience", "oauth_client_id", "oauth_client_secret", diff --git a/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb b/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb index ffd6fd6fb1cc..266037c3b7aa 100644 --- a/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb +++ b/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb @@ -31,14 +31,41 @@ module Storages::Admin class NextcloudAudienceInputForm < ApplicationForm form do |storage_form| - storage_form.text_field( - name: :nextcloud_audience, - label: I18n.t("activerecord.attributes.storages/nextcloud_storage.nextcloud_audience"), - required: true, - caption: I18n.t("storages.instructions.nextcloud.nextcloud_audience"), - placeholder: I18n.t("storages.instructions.nextcloud.nextcloud_audience_placeholder"), - input_width: :large - ) + storage_form.radio_button_group(name: :audience_configuration) do |group| + group.radio_button( + value: :idp, + checked: idp?, + label: I18n.t("storages.storage_audience.idp.label"), + caption: I18n.t("storages.storage_audience.idp.helptext"), + data: { action: "storages--storage-audience#hideAudienceInput", "storages--storage-audience-target": "idpRadio" } + ) + + group.radio_button( + value: :manual, + checked: !idp?, + label: I18n.t("storages.storage_audience.manual.label"), + caption: I18n.t("storages.storage_audience.manual.helptext"), + data: { action: "storages--storage-audience#showAudienceInput" } + ) + end + + storage_form.group(data: { "storages--storage-audience-target": "audienceInputWrapper" }) do |toggleable_group| + toggleable_group.text_field( + name: :nextcloud_audience, + label: I18n.t("activerecord.attributes.storages/nextcloud_storage.nextcloud_audience"), + required: true, + caption: I18n.t("storages.instructions.nextcloud.nextcloud_audience"), + placeholder: I18n.t("storages.instructions.nextcloud.nextcloud_audience_placeholder"), + input_width: :large, + data: { "storages--storage-audience-target": "audienceInput" } + ) + end + end + + private + + def idp? + model.nextcloud_audience == OpenIDConnect::UserToken::IDP_AUDIENCE end end end diff --git a/modules/storages/app/services/storages/storages/set_attributes_service.rb b/modules/storages/app/services/storages/storages/set_attributes_service.rb index 63adc6f51e69..3f2eb6a90ef7 100644 --- a/modules/storages/app/services/storages/storages/set_attributes_service.rb +++ b/modules/storages/app/services/storages/storages/set_attributes_service.rb @@ -40,8 +40,12 @@ def set_default_attributes(_params) private def set_attributes(params) + audience_config = params.delete(:audience_configuration) + super + unset_nextcloud_application_credentials if nextcloud_storage? + set_idp_audience if audience_config == "idp" end def sanitize_host @@ -72,5 +76,9 @@ def storage def nextcloud_storage? storage.is_a?(Storages::NextcloudStorage) end + + def set_idp_audience + storage.nextcloud_audience = OpenIDConnect::UserToken::IDP_AUDIENCE + end end end diff --git a/modules/storages/config/locales/en.yml b/modules/storages/config/locales/en.yml index 90a3ddf3106a..29f14a8d3709 100644 --- a/modules/storages/config/locales/en.yml +++ b/modules/storages/config/locales/en.yml @@ -212,7 +212,8 @@ en: general_information: General information nextcloud_audience: Nextcloud Audience nextcloud_audience_blank: No audience has been configured - nextcloud_audience_description: Using audience %{audience} + nextcloud_audience_description: Obtaining tokens for audience "%{audience}". + nextcloud_audience_idp: Using first access token received by identity provider, regardless of audience. nextcloud_oauth: Nextcloud OAuth oauth_configuration: OAuth configuration one_drive_oauth: Azure OAuth @@ -405,6 +406,13 @@ en: ' label: Show attachments in the work packages files tab + storage_audience: + idp: + helptext: OpenProject will use the first access token received by the identity provider to authenticate requests to the storage. It will not try to obtain another token. + label: Use first access token obtained by identity provider + manual: + helptext: Allows you to set the audience of the storage. OpenProject will try to obtain a token for the given audience from the identity provider. + label: Define storage audience manually storage_list_blank_slate: description: Add a storage to see them here. heading: You don't have any storages yet. diff --git a/modules/storages/spec/features/storages/admin/create_storage_spec.rb b/modules/storages/spec/features/storages/admin/create_storage_spec.rb index 4f49bef352b5..4f80abc2704e 100644 --- a/modules/storages/spec/features/storages/admin/create_storage_spec.rb +++ b/modules/storages/spec/features/storages/admin/create_storage_spec.rb @@ -220,7 +220,7 @@ end expect(page).to have_test_selector("label-nextcloud_audience_configured-status", text: "Completed") - expect(page).to have_test_selector("nextcloud-audience-description", text: "Using audience nextcloud") + expect(page).to have_test_selector("nextcloud-audience-description", text: "Obtaining tokens for audience \"nextcloud\"") end aggregate_failures "Automatically managed project folders" do From f1ac9d90e4e3028a722fbb6319ceeb9c27dc5c8d Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Thu, 20 Feb 2025 11:25:01 +0100 Subject: [PATCH 2/4] Replace the term nextcloud audience with storage audience The components we are starting to build should be generally usable for different storages. Before we sink more cost into overly specific naming, we should generalize to a name that can be used anywhere, if we want to. Left out components that would require renaming files for now, to reduce the conflict potential a bit. --- .../peripherals/nextcloud_registry.rb | 6 ++-- .../peripherals/nextcloud_storage_wizard.rb | 4 +-- ...nextcloud_audience_form_component.html.erb | 2 +- .../nextcloud_audience_form_component.rb | 4 +-- .../nextcloud_audience_info_component.erb | 12 ++++---- .../nextcloud_audience_info_component.rb | 10 +++---- .../storages/nextcloud_audience_contract.rb | 6 ++-- .../storages/admin/storages_controller.rb | 6 ++-- .../admin/nextcloud_audience_input_form.rb | 10 +++---- .../app/models/storages/nextcloud_storage.rb | 6 ++-- .../storages/set_attributes_service.rb | 2 +- modules/storages/config/locales/en.yml | 18 ++++++------ modules/storages/config/routes.rb | 2 +- .../nextcloud_connection_validator_spec.rb | 2 +- .../nextcloud_storage_wizard_spec.rb | 28 +++++++++---------- .../storages/nextcloud_contract_spec.rb | 22 +++++++-------- .../spec/factories/storage_factory.rb | 6 ++-- .../storages/admin/create_storage_spec.rb | 12 ++++---- .../storages/admin/edit_storage_spec.rb | 24 ++++++++-------- .../models/storages/nextcloud_storage_spec.rb | 14 +++++----- 20 files changed, 98 insertions(+), 98 deletions(-) diff --git a/modules/storages/app/common/storages/peripherals/nextcloud_registry.rb b/modules/storages/app/common/storages/peripherals/nextcloud_registry.rb index 3c16587afcf2..a5842ff819ac 100644 --- a/modules/storages/app/common/storages/peripherals/nextcloud_registry.rb +++ b/modules/storages/app/common/storages/peripherals/nextcloud_registry.rb @@ -61,7 +61,7 @@ module Peripherals namespace("forms") do register(:automatically_managed_folders, ::Storages::Admin::Forms::AutomaticallyManagedProjectFoldersFormComponent) register(:general_information, ::Storages::Admin::Forms::GeneralInfoFormComponent) - register(:nextcloud_audience, ::Storages::Admin::Forms::NextcloudAudienceFormComponent) + register(:storage_audience, ::Storages::Admin::Forms::NextcloudAudienceFormComponent) register(:oauth_application, ::Storages::Admin::OAuthApplicationInfoCopyComponent) register(:oauth_client, ::Storages::Admin::Forms::OAuthClientFormComponent) end @@ -70,7 +70,7 @@ module Peripherals register(:automatically_managed_folders, ::Storages::Admin::AutomaticallyManagedProjectFoldersInfoComponent) register(:general_information, ::Storages::Admin::GeneralInfoComponent) - register(:nextcloud_audience, ::Storages::Admin::NextcloudAudienceInfoComponent) + register(:storage_audience, ::Storages::Admin::NextcloudAudienceInfoComponent) register(:oauth_application, ::Storages::Admin::OAuthApplicationInfoComponent) register(:oauth_client, ::Storages::Admin::OAuthClientInfoComponent) end @@ -78,7 +78,7 @@ module Peripherals namespace("contracts") do register(:storage, ::Storages::Storages::NextcloudContract) register(:general_information, ::Storages::Storages::NextcloudGeneralInformationContract) - register(:nextcloud_audience, ::Storages::Storages::NextcloudAudienceContract) + register(:storage_audience, ::Storages::Storages::NextcloudAudienceContract) end namespace("models") do diff --git a/modules/storages/app/common/storages/peripherals/nextcloud_storage_wizard.rb b/modules/storages/app/common/storages/peripherals/nextcloud_storage_wizard.rb index 65d2aa4f021f..22c33b0e9d75 100644 --- a/modules/storages/app/common/storages/peripherals/nextcloud_storage_wizard.rb +++ b/modules/storages/app/common/storages/peripherals/nextcloud_storage_wizard.rb @@ -35,10 +35,10 @@ class NextcloudStorageWizard < Wizard # OAuth 2.0 SSO - step :nextcloud_audience, + step :storage_audience, section: :oauth_configuration, if: ->(storage) { storage.authenticate_via_idp? }, - completed_if: ->(storage) { storage.nextcloud_audience.present? } + completed_if: ->(storage) { storage.storage_audience.present? } # Two-Way OAuth 2.0 diff --git a/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb b/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb index 939cdd5aedae..45b9345723cb 100644 --- a/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb +++ b/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.html.erb @@ -29,7 +29,7 @@ See COPYRIGHT and LICENSE files for more details. <%= component_wrapper(tag: "turbo-frame") do - render(Primer::Beta::Text.new(tag: :div, test_selector: "storage-nextcloud-audience-form")) do + render(Primer::Beta::Text.new(tag: :div, test_selector: "storage-audience-form")) do primer_form_with( model:, url: form_url, diff --git a/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.rb b/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.rb index 1f79f9521d14..c4aa2a3be7e9 100644 --- a/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.rb +++ b/modules/storages/app/components/storages/admin/forms/nextcloud_audience_form_component.rb @@ -30,12 +30,12 @@ # module Storages::Admin::Forms class NextcloudAudienceFormComponent < StorageFormComponent - def self.wrapper_key = :storage_nextcloud_audience_section + def self.wrapper_key = :storage_audience_section options submit_button_disabled: false def form_url - query = { origin_component: "nextcloud_audience" } + query = { origin_component: "storage_audience" } query[:continue_wizard] = storage.id if in_wizard admin_settings_storage_path(storage, query) diff --git a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb index 4688465bc306..155416c665dd 100644 --- a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb +++ b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.erb @@ -31,11 +31,11 @@ See COPYRIGHT and LICENSE files for more details. component_wrapper(tag: 'turbo-frame') do grid_layout('op-storage-view--row', tag: :div, align_items: :center) do |grid| grid.with_area(:item, tag: :div, mr: 3) do - concat(render(Primer::Beta::Text.new(font_weight: :bold, mr: 1, test_selector: 'nextcloud-audience-label')) { I18n.t('storages.file_storage_view.nextcloud_audience') }) - concat(configuration_check_label_for(:nextcloud_audience_configured)) + concat(render(Primer::Beta::Text.new(font_weight: :bold, mr: 1, test_selector: 'storage-audience-label')) { I18n.t('storages.file_storage_view.storage_audience') }) + concat(configuration_check_label_for(:storage_audience_configured)) end - grid.with_area(:description, tag: :div, color: :subtle, test_selector: 'nextcloud-audience-description') do + grid.with_area(:description, tag: :div, color: :subtle, test_selector: 'storage-audience-description') do concat(render(Primer::Beta::Text.new) { audience_summary }) end @@ -48,9 +48,9 @@ See COPYRIGHT and LICENSE files for more details. icon: :pencil, tag: :a, scheme: :invisible, - href: edit_nextcloud_audience_admin_settings_storage_path(storage), - aria: { label: I18n.t('storages.label_edit_nextcloud_audience') }, - test_selector: 'storage-edit-nextcloud-audience-button', + href: edit_storage_audience_admin_settings_storage_path(storage), + aria: { label: I18n.t('storages.label_edit_storage_audience') }, + test_selector: 'storage-edit-storage-audience-button', data: { turbo_stream: true } ) ) diff --git a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb index 6c0ff21cad76..70a49123e337 100644 --- a/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb +++ b/modules/storages/app/components/storages/admin/nextcloud_audience_info_component.rb @@ -31,16 +31,16 @@ module Storages module Admin class NextcloudAudienceInfoComponent < StorageInfoComponent - def self.wrapper_key = :storage_nextcloud_audience_section + def self.wrapper_key = :storage_audience_section def audience_summary - case storage.nextcloud_audience + case storage.storage_audience when "" - I18n.t("storages.file_storage_view.nextcloud_audience_blank") + I18n.t("storages.file_storage_view.storage_audience_blank") when OpenIDConnect::UserToken::IDP_AUDIENCE - I18n.t("storages.file_storage_view.nextcloud_audience_idp") + I18n.t("storages.file_storage_view.storage_audience_idp") else - I18n.t("storages.file_storage_view.nextcloud_audience_description", audience: storage.nextcloud_audience) + I18n.t("storages.file_storage_view.storage_audience_description", audience: storage.storage_audience) end end end diff --git a/modules/storages/app/contracts/storages/storages/nextcloud_audience_contract.rb b/modules/storages/app/contracts/storages/storages/nextcloud_audience_contract.rb index fb6f46690394..826989a7c187 100644 --- a/modules/storages/app/contracts/storages/storages/nextcloud_audience_contract.rb +++ b/modules/storages/app/contracts/storages/storages/nextcloud_audience_contract.rb @@ -30,10 +30,10 @@ module Storages::Storages class NextcloudAudienceContract < ::ModelContract - attribute :nextcloud_audience - validates :nextcloud_audience, presence: true, if: -> { nextcloud_storage_authenticate_via_idp? } + attribute :storage_audience + validates :storage_audience, presence: true, if: -> { nextcloud_storage_authenticate_via_idp? } - # Adding this to allow writing the nextcloud_audience + # Adding this to allow writing the storage_audience attribute :provider_fields private diff --git a/modules/storages/app/controllers/storages/admin/storages_controller.rb b/modules/storages/app/controllers/storages/admin/storages_controller.rb index fed0763d292a..c23db058f913 100644 --- a/modules/storages/app/controllers/storages/admin/storages_controller.rb +++ b/modules/storages/app/controllers/storages/admin/storages_controller.rb @@ -45,7 +45,7 @@ class Storages::Admin::StoragesController < ApplicationController # and set the @ variable to the object referenced in the URL. before_action :require_admin before_action :find_model_object, - only: %i[show_oauth_application destroy edit edit_host edit_nextcloud_audience confirm_destroy update + only: %i[show_oauth_application destroy edit edit_host edit_storage_audience confirm_destroy update change_health_notifications_enabled replace_oauth_application] before_action :ensure_valid_wizard_parameters, only: [:new] before_action :require_ee_token_for_one_drive, only: [:new] @@ -123,7 +123,7 @@ def edit_host respond_with_turbo_streams end - def edit_nextcloud_audience + def edit_storage_audience update_via_turbo_stream(component: Storages::Admin::Forms::NextcloudAudienceFormComponent.new(@storage)) respond_with_turbo_streams end @@ -253,7 +253,7 @@ def permitted_storage_params(model_parameter_name = storage_provider_parameter_n "host", "authentication_method", "audience_configuration", - "nextcloud_audience", + "storage_audience", "oauth_client_id", "oauth_client_secret", "tenant_id", diff --git a/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb b/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb index 266037c3b7aa..819e4584407a 100644 --- a/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb +++ b/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb @@ -51,11 +51,11 @@ class NextcloudAudienceInputForm < ApplicationForm storage_form.group(data: { "storages--storage-audience-target": "audienceInputWrapper" }) do |toggleable_group| toggleable_group.text_field( - name: :nextcloud_audience, - label: I18n.t("activerecord.attributes.storages/nextcloud_storage.nextcloud_audience"), + name: :storage_audience, + label: I18n.t("activerecord.attributes.storages/nextcloud_storage.storage_audience"), required: true, - caption: I18n.t("storages.instructions.nextcloud.nextcloud_audience"), - placeholder: I18n.t("storages.instructions.nextcloud.nextcloud_audience_placeholder"), + caption: I18n.t("storages.instructions.nextcloud.storage_audience"), + placeholder: I18n.t("storages.instructions.nextcloud.storage_audience_placeholder"), input_width: :large, data: { "storages--storage-audience-target": "audienceInput" } ) @@ -65,7 +65,7 @@ class NextcloudAudienceInputForm < ApplicationForm private def idp? - model.nextcloud_audience == OpenIDConnect::UserToken::IDP_AUDIENCE + model.storage_audience == OpenIDConnect::UserToken::IDP_AUDIENCE end end end diff --git a/modules/storages/app/models/storages/nextcloud_storage.rb b/modules/storages/app/models/storages/nextcloud_storage.rb index 05af29059f7b..f5c3afc37830 100644 --- a/modules/storages/app/models/storages/nextcloud_storage.rb +++ b/modules/storages/app/models/storages/nextcloud_storage.rb @@ -43,7 +43,7 @@ class NextcloudStorage < Storage 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 + store_attribute :provider_fields, :storage_audience, :string def oauth_configuration Peripherals::OAuthConfigurations::NextcloudConfiguration.new(self) @@ -67,7 +67,7 @@ def available_project_folder_modes end def audience - nextcloud_audience + storage_audience end def authenticate_via_idp? @@ -83,7 +83,7 @@ def configuration_checks storage_oauth_client_configured: !authenticate_via_storage? || oauth_client.present?, openproject_oauth_application_configured: !authenticate_via_storage? || oauth_application.present?, host_name_configured: host.present? && name.present?, - nextcloud_audience_configured: !authenticate_via_idp? || nextcloud_audience.present? + storage_audience_configured: !authenticate_via_idp? || storage_audience.present? } end diff --git a/modules/storages/app/services/storages/storages/set_attributes_service.rb b/modules/storages/app/services/storages/storages/set_attributes_service.rb index 3f2eb6a90ef7..15980a229831 100644 --- a/modules/storages/app/services/storages/storages/set_attributes_service.rb +++ b/modules/storages/app/services/storages/storages/set_attributes_service.rb @@ -78,7 +78,7 @@ def nextcloud_storage? end def set_idp_audience - storage.nextcloud_audience = OpenIDConnect::UserToken::IDP_AUDIENCE + storage.storage_audience = OpenIDConnect::UserToken::IDP_AUDIENCE end end end diff --git a/modules/storages/config/locales/en.yml b/modules/storages/config/locales/en.yml index 29f14a8d3709..89db20711688 100644 --- a/modules/storages/config/locales/en.yml +++ b/modules/storages/config/locales/en.yml @@ -9,7 +9,7 @@ en: oauth2_sso: Single-Sign-On through OpenID Connect Identity Provider oauth2_sso_with_two_way_oauth2_fallback: Single-Sign-On with Two-Way OAuth 2.0 as fallback two_way_oauth2: Two-way OAuth 2.0 authorization code flow - nextcloud_audience: Nextcloud Audience + storage_audience: Storage Audience storages/project_storage: storage: Storage storages/storage: @@ -210,16 +210,16 @@ en: access_management_section: Access and project folders automatically_managed_folders: Automatically managed folders general_information: General information - nextcloud_audience: Nextcloud Audience - nextcloud_audience_blank: No audience has been configured - nextcloud_audience_description: Obtaining tokens for audience "%{audience}". - nextcloud_audience_idp: Using first access token received by identity provider, regardless of audience. - nextcloud_oauth: Nextcloud OAuth oauth_configuration: OAuth configuration one_drive_oauth: Azure OAuth openproject_oauth: OpenProject OAuth project_folders: Project folders redirect_uri: Redirect URI + storage_audience: Storage Audience + storage_audience_blank: No audience has been configured + storage_audience_description: Obtaining tokens for audience "%{audience}". + storage_audience_idp: Using first access token received by identity provider, regardless of audience. + storage_oauth: Nextcloud OAuth storage_provider: Storage provider health: checked: Last checked %{datetime} @@ -276,10 +276,10 @@ en: nextcloud: application_link_text: application “Integration OpenProject” integration: Nextcloud Administration / OpenProject - nextcloud_audience: Please enter the Client ID that the Nextcloud instance uses to communicate with the identity provider. - nextcloud_audience_placeholder: e.g. nextcloud oauth_configuration: Copy these values from %{application_link_text}. provider_configuration: Please make sure you have administration privileges in your Nextcloud instance and the %{application_link_text} is installed before doing the setup. + storage_audience: Please enter the Client ID that the Nextcloud instance uses to communicate with the identity provider. + storage_audience_placeholder: e.g. nextcloud no_specific_folder: By default, each user will start at their own home folder when they upload a file. no_storage_set_up: There are no file storages set up yet. not_logged_into_storage: To select a project folder, please first login @@ -310,8 +310,8 @@ en: label_creation_time: Creation time label_creator: Creator label_delete_storage: Delete storage - label_edit_nextcloud_audience: Edit Nextcloud Audience label_edit_storage_access_management: Edit storage access management + label_edit_storage_audience: Edit Storage Audience label_edit_storage_automatically_managed_folders: Edit storage automatically managed folders label_edit_storage_host: Edit storage host label_existing_manual_folder: Existing folder with manually managed permissions diff --git a/modules/storages/config/routes.rb b/modules/storages/config/routes.rb index 54878cbbb890..3068788acddd 100644 --- a/modules/storages/config/routes.rb +++ b/modules/storages/config/routes.rb @@ -62,7 +62,7 @@ member do get :show_oauth_application get :edit_host - get :edit_nextcloud_audience + get :edit_storage_audience patch :change_health_notifications_enabled get :confirm_destroy delete :replace_oauth_application diff --git a/modules/storages/spec/common/storages/peripherals/nextcloud_connection_validator_spec.rb b/modules/storages/spec/common/storages/peripherals/nextcloud_connection_validator_spec.rb index 867502b35971..4a522b7e9d74 100644 --- a/modules/storages/spec/common/storages/peripherals/nextcloud_connection_validator_spec.rb +++ b/modules/storages/spec/common/storages/peripherals/nextcloud_connection_validator_spec.rb @@ -256,7 +256,7 @@ end it "returns a validation failure if storage audience isn't set" do - storage.update!(nextcloud_audience: nil) + storage.update!(storage_audience: nil) expect(subject.type).to eq(:none) expect(subject.error_code).to eq(:wrn_not_configured) diff --git a/modules/storages/spec/common/storages/peripherals/nextcloud_storage_wizard_spec.rb b/modules/storages/spec/common/storages/peripherals/nextcloud_storage_wizard_spec.rb index 2232ca82d332..041b015aa920 100644 --- a/modules/storages/spec/common/storages/peripherals/nextcloud_storage_wizard_spec.rb +++ b/modules/storages/spec/common/storages/peripherals/nextcloud_storage_wizard_spec.rb @@ -146,19 +146,19 @@ end it "has new steps pending in correct order" do - expect(wizard.pending_steps).to eq(%i[nextcloud_audience + expect(wizard.pending_steps).to eq(%i[storage_audience automatically_managed_folders]) end - context "and the nextcloud audience was set" do + context "and the storage audience was set" do before do wizard.prepare_next_step - model.nextcloud_audience = "nextcloud" + model.storage_audience = "nextcloud" end - it "finished the nextcloud_audience step" do + it "finished the storage_audience step" do expect(wizard.completed_steps).to eq(%i[general_information - nextcloud_audience]) + storage_audience]) end it "still didn't specify how to manage folders" do @@ -184,7 +184,7 @@ it "has all steps completed" do expect(wizard.completed_steps).to eq(%i[general_information - nextcloud_audience + storage_audience automatically_managed_folders]) end end @@ -204,20 +204,20 @@ end it "has new steps pending in correct order" do - expect(wizard.pending_steps).to eq(%i[nextcloud_audience + expect(wizard.pending_steps).to eq(%i[storage_audience oauth_application oauth_client automatically_managed_folders]) end - context "and the nextcloud audience was set" do + context "and the storage audience was set" do before do wizard.prepare_next_step - model.nextcloud_audience = "nextcloud" + model.storage_audience = "nextcloud" end - it "has nextcloud_audience step completed" do - expect(wizard.completed_steps).to eq(%i[general_information nextcloud_audience]) + it "has storage_audience step completed" do + expect(wizard.completed_steps).to eq(%i[general_information storage_audience]) end it "has no oauth_application yet" do @@ -231,7 +231,7 @@ it "automatically finished the oauth_application step" do expect(wizard.completed_steps).to eq(%i[general_information - nextcloud_audience + storage_audience oauth_application]) end @@ -250,7 +250,7 @@ it "finishes the oauth_client step" do expect(wizard.completed_steps).to eq(%i[general_information - nextcloud_audience + storage_audience oauth_application oauth_client]) end @@ -283,7 +283,7 @@ it "has all steps completed" do expect(wizard.completed_steps).to eq(%i[general_information - nextcloud_audience + storage_audience oauth_application oauth_client automatically_managed_folders]) 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 a2fee562eb46..d99745ee0c42 100644 --- a/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/storages/nextcloud_contract_spec.rb @@ -156,7 +156,7 @@ context "when the authentication method is oauth2_sso" do let(:authentication_method) { "oauth2_sso" } - before { storage.nextcloud_audience = "valid_audience" } + before { storage.storage_audience = "valid_audience" } it { is_expected.to be_valid } end @@ -174,22 +174,22 @@ end end - describe "nextcloud_audience validation" do + describe "storage_audience validation" do let(:storage) do - build(:nextcloud_storage, :as_not_automatically_managed, authentication_method:, nextcloud_audience:) + build(:nextcloud_storage, :as_not_automatically_managed, authentication_method:, storage_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 } + context "and there is no storage_audience" do + let(:storage_audience) { nil } it { is_expected.to be_valid } end - context "and there is a nextcloud_audience" do - let(:nextcloud_audience) { "nextcloud" } + context "and there is a storage_audience" do + let(:storage_audience) { "nextcloud" } it { is_expected.to be_valid } end @@ -198,14 +198,14 @@ 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 } + context "and there is no storage_audience" do + let(:storage_audience) { nil } it { is_expected.not_to be_valid } end - context "and there is a nextcloud_audience" do - let(:nextcloud_audience) { "nextcloud" } + context "and there is a storage_audience" do + let(:storage_audience) { "nextcloud" } it { is_expected.to be_valid } end diff --git a/modules/storages/spec/factories/storage_factory.rb b/modules/storages/spec/factories/storage_factory.rb index 7ee7564ce49e..a2e094e52f45 100644 --- a/modules/storages/spec/factories/storage_factory.rb +++ b/modules/storages/spec/factories/storage_factory.rb @@ -92,15 +92,15 @@ provider_type { Storages::Storage::PROVIDER_TYPE_NEXTCLOUD } sequence(:host) { |n| "https://host#{n}.example.com/" } authentication_method { "two_way_oauth2" } - nextcloud_audience { nil } + storage_audience { nil } trait :oidc_sso_enabled do - nextcloud_audience { "nextcloud" } + storage_audience { "nextcloud" } authentication_method { "oauth2_sso" } end trait :oidc_sso_with_fallback do - nextcloud_audience { "nextcloud" } + storage_audience { "nextcloud" } authentication_method { "oauth2_sso_with_two_way_oauth2_fallback" } end diff --git a/modules/storages/spec/features/storages/admin/create_storage_spec.rb b/modules/storages/spec/features/storages/admin/create_storage_spec.rb index 4f80abc2704e..c966d658a188 100644 --- a/modules/storages/spec/features/storages/admin/create_storage_spec.rb +++ b/modules/storages/spec/features/storages/admin/create_storage_spec.rb @@ -210,17 +210,17 @@ click_on "Save and continue" end - aggregate_failures "Nextcloud Audience" do - within_test_selector("storage-nextcloud-audience-form") do + aggregate_failures "Storage Audience" do + within_test_selector("storage-audience-form") do click_on "Save and continue" - expect(page).to have_text("Nextcloud Audience can't be blank.") + expect(page).to have_text("Storage Audience can't be blank.") - fill_in "Nextcloud Audience", with: "nextcloud" + fill_in "Storage Audience", with: "nextcloud" click_on "Save and continue" end - expect(page).to have_test_selector("label-nextcloud_audience_configured-status", text: "Completed") - expect(page).to have_test_selector("nextcloud-audience-description", text: "Obtaining tokens for audience \"nextcloud\"") + expect(page).to have_test_selector("label-storage_audience_configured-status", text: "Completed") + expect(page).to have_test_selector("storage-audience-description", text: "Obtaining tokens for audience \"nextcloud\"") end aggregate_failures "Automatically managed project folders" do diff --git a/modules/storages/spec/features/storages/admin/edit_storage_spec.rb b/modules/storages/spec/features/storages/admin/edit_storage_spec.rb index 1dab17fae53e..bb05d6dfa64e 100644 --- a/modules/storages/spec/features/storages/admin/edit_storage_spec.rb +++ b/modules/storages/spec/features/storages/admin/edit_storage_spec.rb @@ -247,7 +247,7 @@ :nextcloud_storage, :as_automatically_managed, authentication_method: "oauth2_sso", - nextcloud_audience: "", + storage_audience: "", name: "Cloud Storage" ) end @@ -273,10 +273,10 @@ expect(page).to have_test_selector("label-host_name_configured-status", text: "Completed") expect(page).to have_test_selector("storage-description", text: "Nextcloud - #{storage.name} - #{storage.host}") - # Nextcloud audience - expect(page).to have_test_selector("nextcloud-audience-label", text: "Nextcloud Audience") - expect(page).to have_test_selector("label-nextcloud_audience_configured-status", text: "Incomplete") - expect(page).to have_test_selector("nextcloud-audience-description", text: "No audience has been configured") + # Storage audience + expect(page).to have_test_selector("storage-audience-label", text: "Storage Audience") + expect(page).to have_test_selector("label-storage_audience_configured-status", text: "Incomplete") + expect(page).to have_test_selector("storage-audience-description", text: "No audience has been configured") # Automatically managed project folders expect(page).to have_test_selector("storage-managed-project-folders-label", @@ -290,18 +290,18 @@ # Only testing interaction with components not tested # in Two-Way OAuth 2.0 case - aggregate_failures "Nextcloud Audience" do - find_test_selector("storage-edit-nextcloud-audience-button").click - within_test_selector("storage-nextcloud-audience-form") do + aggregate_failures "Storage Audience" do + find_test_selector("storage-edit-storage-audience-button").click + within_test_selector("storage-audience-form") do click_on "Save and continue" - expect(page).to have_text("Nextcloud Audience can't be blank") + expect(page).to have_text("Storage Audience can't be blank") - fill_in "Nextcloud Audience", with: "schmaudience" + fill_in "Storage Audience", with: "schmaudience" click_on "Save and continue" end - expect(page).to have_test_selector("label-nextcloud_audience_configured-status", text: "Completed") - expect(page).to have_test_selector("nextcloud-audience-description", text: "Using audience schmaudience") + expect(page).to have_test_selector("label-storage_audience_configured-status", text: "Completed") + expect(page).to have_test_selector("storage-audience-description", text: "Using audience schmaudience") end end diff --git a/modules/storages/spec/models/storages/nextcloud_storage_spec.rb b/modules/storages/spec/models/storages/nextcloud_storage_spec.rb index 884dd0475964..cd79151931f5 100644 --- a/modules/storages/spec/models/storages/nextcloud_storage_spec.rb +++ b/modules/storages/spec/models/storages/nextcloud_storage_spec.rb @@ -65,7 +65,7 @@ aggregate_failures "configuration_checks" do expect(storage.configuration_checks) .to eq(host_name_configured: true, - nextcloud_audience_configured: true, + storage_audience_configured: true, storage_oauth_client_configured: true, openproject_oauth_application_configured: true) end @@ -85,10 +85,10 @@ end end - context "without nextcloud audience" do + context "without storage audience" do let(:storage) do build(:nextcloud_storage, - nextcloud_audience: "", + storage_audience: "", oauth_application: build(:oauth_application), oauth_client: build(:oauth_client)) end @@ -97,7 +97,7 @@ aggregate_failures do expect(storage.configured?).to be(true) aggregate_failures "configuration_checks" do - expect(storage.configuration_checks[:nextcloud_audience_configured]).to be(true) + expect(storage.configuration_checks[:storage_audience_configured]).to be(true) end end end @@ -106,7 +106,7 @@ let(:storage) do build(:nextcloud_storage, authentication_method: "oauth2_sso", - nextcloud_audience: "", + storage_audience: "", oauth_application: build(:oauth_application), oauth_client: build(:oauth_client)) end @@ -115,7 +115,7 @@ aggregate_failures do expect(storage.configured?).to be(false) aggregate_failures "configuration_checks" do - expect(storage.configuration_checks[:nextcloud_audience_configured]).to be(false) + expect(storage.configuration_checks[:storage_audience_configured]).to be(false) end end end @@ -138,7 +138,7 @@ let(:storage) do build(:nextcloud_storage, authentication_method: "oauth2_sso", - nextcloud_audience: "some") + storage_audience: "some") end it "returns true" do From d76c62a5bb92dde733aa43ea418229bd68c432cc Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Fri, 21 Feb 2025 08:43:29 +0100 Subject: [PATCH 3/4] Add feature tests around storage audience component Testing correct showing and hiding behaviour as well as persistence of the corresponding selection. --- .../storages/admin/edit_storage_spec.rb | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/modules/storages/spec/features/storages/admin/edit_storage_spec.rb b/modules/storages/spec/features/storages/admin/edit_storage_spec.rb index bb05d6dfa64e..18ad06120362 100644 --- a/modules/storages/spec/features/storages/admin/edit_storage_spec.rb +++ b/modules/storages/spec/features/storages/admin/edit_storage_spec.rb @@ -293,15 +293,46 @@ aggregate_failures "Storage Audience" do find_test_selector("storage-edit-storage-audience-button").click within_test_selector("storage-audience-form") do + expect(page.find(:radio_button, "Define storage audience manually")).to be_checked + expect(page).to have_field("Storage Audience") + click_on "Save and continue" expect(page).to have_text("Storage Audience can't be blank") fill_in "Storage Audience", with: "schmaudience" + + choose("Use first access token obtained by identity provider") + expect(page).to have_no_field("Storage Audience") + choose("Define storage audience manually") + expect(find_field("Storage Audience").value).to be_empty + + fill_in "Storage Audience", with: "schmaudience" + click_on "Save and continue" + end + + expect(page).to have_test_selector("label-storage_audience_configured-status", text: "Completed") + expect(page).to have_test_selector("storage-audience-description", text: "Obtaining tokens for audience \"schmaudience\"") + + find_test_selector("storage-edit-storage-audience-button").click + within_test_selector("storage-audience-form") do + expect(page.find(:radio_button, "Define storage audience manually")).to be_checked + expect(find_field("Storage Audience").value).to eq("schmaudience") + + choose("Use first access token obtained by identity provider") click_on "Save and continue" end expect(page).to have_test_selector("label-storage_audience_configured-status", text: "Completed") - expect(page).to have_test_selector("storage-audience-description", text: "Using audience schmaudience") + expect(page).to have_test_selector( + "storage-audience-description", + text: "Using first access token received by identity provider, regardless of audience." + ) + + find_test_selector("storage-edit-storage-audience-button").click + within_test_selector("storage-audience-form") do + expect(page.find(:radio_button, "Use first access token obtained by identity provider")).to be_checked + expect(page).to have_no_field("Storage Audience") + end end end From 74bad376576d9133815432c48b568dd92f1a58be Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Tue, 25 Feb 2025 12:33:34 +0100 Subject: [PATCH 4/4] Do not clear audience in JS This made the UX less than optimal, because users "playing" with the radio buttons could lose their input. The problem we wanted to solve was preventing that the magic internal audience name for the IDP audience would leak into the UI. This is now prevented earlier, by making sure it's never even filled into the input field. Thus the frontend side code now doesn't have to care about this at all. --- .../dynamic/storages/storage-audience.controller.ts | 1 - .../storages/admin/nextcloud_audience_input_form.rb | 9 ++++++++- .../spec/features/storages/admin/edit_storage_spec.rb | 6 ++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts b/frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts index ecbf280edcca..019452c60d99 100644 --- a/frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/storages/storage-audience.controller.ts @@ -48,7 +48,6 @@ export default class StorageAudienceController extends Controller { } showAudienceInput() { - this.audienceInputTarget.value = ''; this.audienceInputWrapperTarget.classList.remove('d-none'); } diff --git a/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb b/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb index 819e4584407a..3823c2fc7fcc 100644 --- a/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb +++ b/modules/storages/app/forms/storages/admin/nextcloud_audience_input_form.rb @@ -57,7 +57,8 @@ class NextcloudAudienceInputForm < ApplicationForm caption: I18n.t("storages.instructions.nextcloud.storage_audience"), placeholder: I18n.t("storages.instructions.nextcloud.storage_audience_placeholder"), input_width: :large, - data: { "storages--storage-audience-target": "audienceInput" } + data: { "storages--storage-audience-target": "audienceInput" }, + value: prefilled_audience ) end end @@ -67,5 +68,11 @@ class NextcloudAudienceInputForm < ApplicationForm def idp? model.storage_audience == OpenIDConnect::UserToken::IDP_AUDIENCE end + + def prefilled_audience + return "" if idp? + + model.storage_audience + end end end diff --git a/modules/storages/spec/features/storages/admin/edit_storage_spec.rb b/modules/storages/spec/features/storages/admin/edit_storage_spec.rb index 18ad06120362..9023ce1b8075 100644 --- a/modules/storages/spec/features/storages/admin/edit_storage_spec.rb +++ b/modules/storages/spec/features/storages/admin/edit_storage_spec.rb @@ -304,9 +304,8 @@ choose("Use first access token obtained by identity provider") expect(page).to have_no_field("Storage Audience") choose("Define storage audience manually") - expect(find_field("Storage Audience").value).to be_empty + expect(find_field("Storage Audience").value).to eq("schmaudience") - fill_in "Storage Audience", with: "schmaudience" click_on "Save and continue" end @@ -332,6 +331,9 @@ within_test_selector("storage-audience-form") do expect(page.find(:radio_button, "Use first access token obtained by identity provider")).to be_checked expect(page).to have_no_field("Storage Audience") + + choose("Define storage audience manually") + expect(find_field("Storage Audience").value).to be_empty end end end