WIP: Rework "open storage" action

The previous implementation was a deeply nested decision tree for
different error cases or other unexpected states.

Most of this was necessary, because setting up correct permissions is partially
happening asynchronously. Some of the redirects would eventually have rendered a modal
that would periodically check whether user permissions appeared in the background.

All of this was replaced by an approach where we immediately (synchronously) ensure
that the setup is correct.

Author: NobodysNightmare

modules/storages/app/common/storages/peripherals/nextcloud_registry.rb

@@ -85,6 +85,11 @@ module Peripherals
         register(:managed_folder_identifier, ManagedFolderIdentifier::Nextcloud)
       end
 
+      namespace("services") do
+        register(:folder_create, ::Storages::NextcloudManagedFolderCreateService)
+        register(:folder_permissions, ::Storages::NextcloudManagedFolderPermissionsService)
+      end
+
       namespace("validators") do
         register(:connection, ConnectionValidators::NextcloudValidator)
       end

modules/storages/app/common/storages/peripherals/one_drive_registry.rb

@@ -76,6 +76,11 @@ module Peripherals
         register(:managed_folder_identifier, ManagedFolderIdentifier::OneDrive)
       end
 
+      namespace("services") do
+        register(:folder_create, ::Storages::OneDriveManagedFolderCreateService)
+        register(:folder_permissions, ::Storages::OneDriveManagedFolderPermissionsService)
+      end
+
       namespace("validations") do
         register(:connection, ConnectionValidators::OneDriveValidator)
       end

modules/storages/app/controllers/storages/project_storages_controller.rb

@@ -40,110 +40,97 @@ class Storages::ProjectStoragesController < ApplicationController
   before_action :render_403, unless: -> { User.current.allowed_in_project?(:view_file_links, @project) }
   no_authorization_required! :open
 
-  # rubocop:disable Metrics/AbcSize
+  before_action :ensure_remote_identity, only: :open
+  before_action :ensure_folder_created, only: :open
+  before_action :ensure_folder_permissions, only: :open
+
   def open
-    if @object.project_folder_automatic?
-      @storage = @object.storage
-      # check if user "see" project_folder
-      if @object.project_folder_id.present?
-        ::Storages::Peripherals::Registry
-          .resolve("#{@storage}.queries.file_info")
-          .call(storage: @storage, auth_strategy:, file_id: @object.project_folder_id)
-          .match(
-            on_success: user_can_read_project_folder,
-            on_failure: user_can_not_read_project_folder
-          )
-      else
-        respond_to do |format|
-          format.turbo_stream { head :no_content }
-          format.html do
-            redirect_to_project_overview_with_modal
-          end
-        end
-      end
-    else
-      redirect_to api_v3_project_storage_open
-    end
+    @project_storage.open(current_user).match(
+      on_success: ->(url) { redirect_to url, allow_other_host: true },
+      on_failure: ->(error) { raise error }
+    )
   end
 
-  # rubocop:enable Metrics/AbcSize
-
   private
 
-  def auth_strategy
-    Storages::Peripherals::Registry.resolve("#{@storage}.authentication.user_bound").call(user: current_user, storage: @storage)
-  end
+  def ensure_remote_identity
+    selector = Storages::Peripherals::StorageInteraction::AuthenticationMethodSelector.new(user: current_user, storage:)
+    return unless selector.storage_oauth?
 
-  def user_can_read_project_folder
-    ->(_) do
-      respond_to do |format|
-        format.turbo_stream do
-          render(
-            turbo_stream: OpTurbo::StreamComponent.new(
-              action: :update,
-              target: Storages::OpenProjectStorageModalComponent.dialog_body_id,
-              template: Storages::OpenProjectStorageModalComponent::Body.new(:success).render_in(view_context)
-            ).render_in(view_context)
-          )
-        end
-        format.html { redirect_to api_v3_project_storage_open }
-      end
+    case Storages::Peripherals::StorageInteraction::Authentication.authorization_state(storage:, user: current_user)
+    when :not_connected, :failed_authorization
+      redirect_to ensure_connection_url
+    when :error
+      show_error(I18n.t("project_storages.open.remote_identity_error"))
     end
   end
 
-  def user_can_not_read_project_folder
-    ->(result) do
-      respond_to do |format|
-        format.turbo_stream { head :no_content }
-        format.html do
-          case result.code
-          when :unauthorized
-            redirect_to(storage_fallback_url, allow_other_host: true)
-          when :forbidden
-            redirect_to_project_overview_with_modal
-          end
-        end
-      end
+  def ensure_folder_created
+    return unless @project_storage.project_folder_automatic?
+    return if @project_storage.project_folder_id.present?
+
+    folder_create_service.new(storage:, project_storages: project_storage_scope).call.on_failure do |result|
+      return show_error(result.errors.full_messages)
     end
+    @project_storage.reload
   end
 
-  def storage_fallback_url
-    selector = Storages::Peripherals::StorageInteraction::AuthenticationMethodSelector.new(user: current_user, storage: @storage)
-    if selector.sso?
-      # Maybe the user just can't read folder because they are not provisioned in (Nextcloud) storage. We redirect them
-      # to the storage and leave error handling up to storage. Ideally they will login to the storage and thus prevent
-      # the same error in the future.
-      # This would not work for OneDrive, but for OneDrive we don't have SSO (yet).
-      res = Storages::Peripherals::Registry.resolve("#{@storage}.queries.open_file_link").call(
-        storage: @storage,
-        auth_strategy:,
-        file_id: @object.project_folder_id
-      )
-      res.result_or { |errors| raise "Could not redirect SSO user to storage: #{errors}" }
-    else
-      oauth_clients_ensure_connection_url(
-        oauth_client_id: @storage.oauth_client.client_id,
-        storage_id: @storage.id,
-        destination_url: request.url
-      )
+  def ensure_folder_permissions
+    return unless @project_storage.project_folder_automatic?
+
+    result = test_folder_access
+    return if result.success? || result.errors.code != :forbidden
+
+    # Note: The time this operation takes may still scale with the number of users.
+    # If this becomes a problem, we will have to update downstream code to allow changing permissions for a few users instead of
+    # requiring to always set all permissions.
+    folder_permissions_service.new(storage:, project_storages: project_storage_scope).call.on_failure do |r|
+      return show_error(r.errors.full_messages)
     end
   end
 
-  def redirect_to_project_overview_with_modal
-    redirect_to(
-      project_overview_path(project_id: @project.identifier),
-      op_modal: {
-        component: Storages::OpenProjectStorageModalComponent.name,
-        parameters: {
-          project_storage_open_url: request.path,
-          redirect_url: api_v3_project_storage_open,
-          state: :waiting
-        }
-      }
+  def ensure_connection_url
+    oauth_clients_ensure_connection_url(
+      oauth_client_id: storage.oauth_client.client_id,
+      storage_id: storage.id,
+      destination_url: request.url
+    )
+  end
+
+  def folder_create_service
+    Storages::Peripherals::Registry.resolve("#{storage}.services.folder_create")
+  end
+
+  def folder_permissions_service
+    Storages::Peripherals::Registry.resolve("#{storage}.services.folder_permissions")
+  end
+
+  def file_info
+    Storages::Peripherals::Registry.resolve("#{storage}.queries.file_info")
+  end
+
+  def user_bound
+    Storages::Peripherals::Registry.resolve("#{storage}.authentication.user_bound")
+  end
+
+  def storage
+    @object.storage
+  end
+
+  def project_storage_scope
+    Storages::ProjectStorage.where(id: @project_storage.id)
+  end
+
+  def test_folder_access
+    file_info.call(
+      storage:,
+      auth_strategy: user_bound.call(storage:, user: current_user),
+      file_id: @project_storage.project_folder_id
     )
   end
 
-  def api_v3_project_storage_open
-    ::API::V3::Utilities::PathHelper::ApiV3Path.project_storage_open(@object.id)
+  def show_error(message)
+    flash[:error] = message
+    redirect_back(fallback_location: project_path(id: @project_storage.project_id))
   end
 end

modules/storages/app/models/storages/project_storage.rb

@@ -97,7 +97,7 @@ def open(user)
 
     def open_with_connection_ensured
       return unless storage.configured?
-      return open_project_storage_url if storage.authenticate_via_idp?
+      return open_project_storage_url if storage.authenticate_via_idp? # TODO: use auth method selector
 
       OpenProject::StaticRouting::StaticRouter.new.url_helpers.oauth_clients_ensure_connection_path(
         oauth_client_id: storage.oauth_client.client_id,
@@ -106,17 +106,12 @@ def open_with_connection_ensured
       )
     end
 
-    private
-
     def open_project_storage_url
-      OpenProject::StaticRouting::StaticRouter
-        .new
-        .url_helpers.open_project_storage_url(host: Setting.host_name,
-                                              protocol: "https",
-                                              project_id: project.identifier,
-                                              id:)
+      OpenProject::StaticRouting::StaticRouter.new.url_helpers.open_project_storage_url(project_id: project.identifier, id:)
     end
 
+    private
+
     def managed_folder_identifier
       @managed_folder_identifier ||=
         Peripherals::Registry.resolve("#{storage}.models.managed_folder_identifier").new(self)

modules/storages/app/common/storages/injector.rb renamed to modules/storages/app/services/storages/managed_folder_sync_service.rb

@@ -28,8 +28,49 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-require "dry/auto_inject"
-
 module Storages
-  Injector = Dry::AutoInject(Peripherals::Registry)
+  class ManagedFolderSyncService < BaseService
+    using Peripherals::ServiceResultRefinements
+
+    def initialize(storage)
+      super()
+      @storage = storage
+    end
+
+    def call
+      with_tagged_logger([self.class.name, "storage-#{@storage.id}"]) do
+        info "Starting AMPF Sync for Storage #{@storage.id}"
+        prepare_remote_folders.on_failure { return epilogue }
+        apply_permissions_to_folders
+        epilogue
+      end
+    end
+
+    private
+
+    def epilogue
+      info "Synchronization process for Storage #{@storage.id} has ended. #{@result.errors.count} errors found."
+      @result
+    end
+
+    def prepare_remote_folders
+      folder_create_service.new(storage: @storage).call.tap do |subresult|
+        @result.merge!(subresult)
+      end
+    end
+
+    def apply_permissions_to_folders
+      folder_permissions_service.new(storage: @storage).call.tap do |subresult|
+        @result.merge!(subresult)
+      end
+    end
+
+    def folder_create_service
+      Peripherals::Registry.resolve("#{@storage}.services.folder_create")
+    end
+
+    def folder_permissions_service
+      Peripherals::Registry.resolve("#{@storage}.services.folder_permissions")
+    end
+  end
 end