Merge pull request #16538 from opf/implementation/57286-amend-nextclo…

…ud-set_permissions_command-to-use-the-same-interface-and-tests

[#57286] refactored nextcloud command for set_permission

modules/storages/app/common/storages/peripherals/storage_interaction/inputs/set_permissions.rb

@@ -33,15 +33,17 @@ module Peripherals
     module StorageInteraction
       module Inputs
         # user_permissions - A list of user specific file permissions.
-        # IMPORTANT: the user ids are considered to be the ids of the remote identities.
+        # IMPORTANT: the user ids are considered to be the ids of the remote identities. If user permissions should be
+        # set via a group, a `group_id` must be provided instead of a `user_id`.
         #   Example:
         #     [
         #       {user_id: "d6e00f6d-1ae7-43e6-b0af-15d99a56d4ce", permissions: [ :read_files,
         #                                                                        :write_files,
         #                                                                        :create_files,
         #                                                                        :delete_files,
         #                                                                        :share_files ]},
-        #       {user_id: "f6e00f6d-1ae7-43e6-b0af-15d99a56d4ce", permissions: [:read_files, :write_files]}
+        #       {user_id: "f6e00f6d-1ae7-43e6-b0af-15d99a56d4ce", permissions: [:read_files, :write_files]},
+        #       {group_id: "fee9cd49-17e2-4430-9235-2060e7372568", permissions: [:read_files]},
         #     ]
         SetPermissions = Data.define(:file_id, :user_permissions) do
           private_class_method :new

modules/storages/app/common/storages/peripherals/storage_interaction/inputs/set_permissions_contract.rb

@@ -36,10 +36,19 @@ class SetPermissionsContract < Dry::Validation::Contract
           params do
             required(:file_id).filled(:string)
             required(:user_permissions).array(:hash) do
-              required(:user_id).filled(:string)
-              required(:permissions).array(:symbol, included_in?: OpenProject::Storages::Engine.permissions)
+              optional(:user_id).filled(:string)
+              optional(:group_id).filled(:string)
+              required(:permissions)
+                .array(:symbol, included_in?: OpenProject::Storages::Engine.external_file_permissions)
             end
           end
+
+          rule(:user_permissions).each do
+            both = value.key?(:user_id) && value.key?(:group_id)
+            none = !value.key?(:user_id) && !value.key?(:group_id)
+
+            key.failure("must have either user_id or group_id") if both || none
+          end
         end
       end
     end

modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/internal/propfind_query.rb

@@ -44,8 +44,7 @@ class PropfindQuery
             #   d:getcontenttype
             #   d:resourcetype
             #   d:getcontentlength
-            #   d:permissions
-            #   d:size
+            #   oc:permissions
             #   oc:id
             #   oc:fileid
             #   oc:favorite
@@ -62,6 +61,10 @@ class PropfindQuery
             #   nc:contained-folder-count
             #   nc:contained-file-count
             #   nc:acl-list
+            #   nc:inherited-acl-list
+            #   nc:group-folder-id
+            #   nc:acl-enabled
+            #   nc:acl-can-manage
             # ].freeze
 
             def self.call(storage:, http:, username:, path:, props:)

modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/set_permissions_command.rb

@@ -32,42 +32,53 @@ module Storages
   module Peripherals
     module StorageInteraction
       module Nextcloud
-        # TODO: refactor to be consistent with OneDrive::SetPermissionsCommand interface
-        # And streamline test cases.
         class SetPermissionsCommand
           include TaggedLogging
           using ServiceResultRefinements
 
+          PERMISSIONS_MAP = { read_files: 1, write_files: 2, create_files: 4, delete_files: 8, share_files: 16 }.freeze
+          PERMISSIONS_KEYS = OpenProject::Storages::Engine.external_file_permissions
           SUCCESS_XPATH = "/d:multistatus/d:response/d:propstat[d:status[text() = 'HTTP/1.1 200 OK']]/d:prop/nc:acl-list"
 
-          def self.call(storage:, auth_strategy:, path:, permissions:)
-            new(storage).call(auth_strategy:, path:, permissions:)
+          # Instantiates the command and executes it.
+          #
+          # @param storage [Storage] The storage to interact with.
+          # @param auth_strategy [AuthenticationStrategy] The authentication strategy to use.
+          # @param input_data [Inputs::SetPermissions] The data needed for setting permissions, containing the file id
+          # and the permissions for an array of users.
+          def self.call(storage:, auth_strategy:, input_data:)
+            new(storage).call(auth_strategy:, input_data:)
           end
 
           def initialize(storage)
             @storage = storage
           end
 
           # rubocop:disable Metrics/AbcSize
-          def call(auth_strategy:, path:, permissions:)
-            validate_input_data(path).on_failure { return _1 }
-
+          def call(auth_strategy:, input_data:)
             username = Util.origin_user_id(caller: self.class, storage: @storage, auth_strategy:)
                            .on_failure { return _1 }
                            .result
 
+            permissions = parse_permission_mask(input_data.user_permissions)
+
             Authentication[auth_strategy].call(storage: @storage) do |http|
               with_tagged_logger do
-                info "Setting permissions #{permissions.inspect} on #{path}"
+                info "Getting the folder information"
+                folder_info = FileInfoQuery.call(storage: @storage, auth_strategy:, file_id: input_data.file_id)
+                                           .on_failure { return _1 }
+                                           .result
+
+                info "Setting permissions #{permissions.inspect} on #{folder_info.location}"
 
                 body = request_xml_body(permissions[:groups], permissions[:users])
                 # This can raise KeyErrors, we probably should just default to empty Arrays.
-                response = http
-                             .request(
-                               "PROPPATCH",
-                               UrlBuilder.url(@storage.uri, "remote.php/dav/files", username, path),
-                               xml: body
-                             )
+                response = http.request("PROPPATCH",
+                                        UrlBuilder.url(@storage.uri,
+                                                       "remote.php/dav/files",
+                                                       username,
+                                                       CGI.unescape(folder_info.location)),
+                                        xml: body)
 
                 handle_response(response)
               end
@@ -78,11 +89,15 @@ def call(auth_strategy:, path:, permissions:)
 
           private
 
-          def validate_input_data(path)
-            if path.blank?
-              ServiceResult.failure(errors: StorageError.new(code: :invalid_path))
-            else
-              ServiceResult.success
+          def parse_permission_mask(user_permissions)
+            user_permissions.each_with_object({ groups: {}, users: {} }) do |entry, aggregate|
+              if entry.key?(:user_id)
+                aggregate[:users][entry[:user_id]] =
+                  PERMISSIONS_MAP.values_at(*(PERMISSIONS_KEYS & entry[:permissions])).sum
+              else
+                aggregate[:groups][entry[:group_id]] =
+                  PERMISSIONS_MAP.values_at(*(PERMISSIONS_KEYS & entry[:permissions])).sum
+              end
             end
           end
 

modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb

@@ -32,10 +32,7 @@ module Storages
   class NextcloudManagedFolderSyncService < BaseService
     using Peripherals::ServiceResultRefinements
 
-    PERMISSIONS_MAP = { read_files: 1, write_files: 2, create_files: 4, delete_files: 8, share_files: 16 }.freeze
-    PERMISSIONS_KEYS = PERMISSIONS_MAP.keys.freeze
-    ALL_PERMISSIONS = PERMISSIONS_MAP.values.sum
-    NO_PERMISSIONS = 0
+    FILE_PERMISSIONS = OpenProject::Storages::Engine.external_file_permissions
 
     include Injector["nextcloud.commands.create_folder", "nextcloud.commands.rename_file", "nextcloud.commands.set_permissions",
                      "nextcloud.queries.group_users", "nextcloud.queries.file_ids", "nextcloud.authentication.userless",
@@ -79,7 +76,7 @@ def prepare_remote_folders
       remote_folders = remote_root_folder_map(@storage.group_folder).on_failure { return _1 }.result
       info "Found #{remote_folders.count} remote folders"
 
-      ensure_root_folder_permissions(@storage.group_folder, @storage.group, @storage.username).on_failure { return _1 }
+      ensure_root_folder_permissions(remote_folders["/#{@storage.group_folder}/"]["fileid"]).on_failure { return _1 }
 
       ensure_folders_exist(remote_folders).on_success { hide_inactive_folders(remote_folders) }
     end
@@ -89,7 +86,7 @@ def apply_permissions_to_folders
       remote_admins = admin_remote_identities_scope.pluck(:origin_user_id)
 
       active_project_storages_scope.where.not(project_folder_id: nil).find_each do |project_storage|
-        set_folders_permissions(remote_admins, project_storage)
+        set_folder_permissions(remote_admins, project_storage)
       end
 
       info "Updating user access on automatically managed project folders"
@@ -129,38 +126,38 @@ def remove_users_from_remote_group(users_to_remove)
     end
 
     # rubocop:disable Metrics/AbcSize
-    def set_folders_permissions(remote_admins, project_storage)
-      admin_permissions = remote_admins.to_set.map do |username|
-        [username, ALL_PERMISSIONS]
-      end.unshift([@storage.username, ALL_PERMISSIONS])
+    def set_folder_permissions(remote_admins, project_storage)
+      system_user = [{ user_id: @storage.username, permissions: FILE_PERMISSIONS }]
 
-      users_permissions = project_remote_identities(project_storage).each_with_object({}) do |identity, hash|
-        permissions = identity.user.all_permissions_for(project_storage.project)
+      admin_permissions = remote_admins.to_set.map { |username| { user_id: username, permissions: FILE_PERMISSIONS } }
 
-        hash[identity.origin_user_id] = PERMISSIONS_MAP.values_at(*(PERMISSIONS_KEYS & permissions)).sum
+      users_permissions = project_remote_identities(project_storage).map do |identity|
+        permissions = identity.user.all_permissions_for(project_storage.project) & FILE_PERMISSIONS
+        { user_id: identity.origin_user_id, permissions: }
       end
 
-      folder = project_storage.managed_project_folder_path
+      group_permissions = [{ group_id: @storage.group, permissions: [] }]
 
-      command_params = {
-        path: folder,
-        permissions: {
-          users: admin_permissions.to_h.merge(users_permissions),
-          groups: { "#{@storage.group}": NO_PERMISSIONS }
-        }
-      }
+      permissions = system_user + admin_permissions + users_permissions + group_permissions
+      project_folder_id = project_storage.project_folder_id
 
-      set_permissions.call(storage: @storage, auth_strategy:, **command_params).on_failure do |service_result|
-        log_storage_error(service_result.errors, folder:)
-        add_error(:set_folder_permission, service_result.errors, options: { folder: })
+      input_data = build_set_permissions_input_data(project_folder_id, permissions).value_or do |failure|
+        log_validation_error(failure, project_folder_id:, permissions:)
+        return # rubocop:disable Lint/NonLocalExitFromIterator
+      end
+
+      set_permissions.call(storage: @storage, auth_strategy:, input_data:).on_failure do |service_result|
+        log_storage_error(service_result.errors, folder: project_folder_id)
+        add_error(:set_folder_permission, service_result.errors, options: { folder: project_folder_id })
       end
     end
+
     # rubocop:enable Metrics/AbcSize
 
     def project_remote_identities(project_storage)
       remote_identities = remote_identities_scope.where.not(id: admin_remote_identities_scope).order(:id)
 
-      if project_storage.project.public? && ProjectRole.non_member.permissions.intersect?(PERMISSIONS_KEYS)
+      if project_storage.project.public? && ProjectRole.non_member.permissions.intersect?(FILE_PERMISSIONS)
         remote_identities
       else
         remote_identities.where(user: project_storage.project.users)
@@ -173,21 +170,28 @@ def hide_inactive_folders(remote_folders)
       project_folder_ids = active_project_storages_scope.pluck(:project_folder_id).compact
 
       remote_folders.except("/#{@storage.group_folder}/").each do |(path, attrs)|
-        next if project_folder_ids.include?(attrs["fileid"])
-
-        info "Hiding folder #{path} as it does not belong to any active project"
-        command_params = { path:,
-                           permissions: {
-                             users: { "#{@storage.username}": ALL_PERMISSIONS },
-                             groups: { "#{@storage.group}": NO_PERMISSIONS }
-                           } }
-
-        set_permissions.call(storage: @storage, auth_strategy:, **command_params).on_failure do |service_result|
-          log_storage_error(service_result.errors, folder: path, context: "hide_folder")
-          add_error(:hide_inactive_folders, service_result.errors, options: { path: })
+        folder_id = attrs["fileid"]
+
+        next if project_folder_ids.include?(folder_id)
+
+        info "Hiding folder #{folder_id} (#{path}) as it does not belong to any active project"
+        permissions = [
+          { user_id: @storage.username, permissions: FILE_PERMISSIONS },
+          { group_id: @storage.group, permissions: [] }
+        ]
+
+        input_data = build_set_permissions_input_data(folder_id, permissions).value_or do |failure|
+          log_validation_error(failure, folder_id:, permissions:)
+          return # rubocop:disable Lint/NonLocalExitFromIterator
+        end
+
+        set_permissions.call(storage: @storage, auth_strategy:, input_data:).on_failure do |service_result|
+          log_storage_error(service_result.errors, folder_id:, context: "hide_folder")
+          add_error(:hide_inactive_folders, service_result.errors, options: { folder_id: })
         end
       end
     end
+
     # rubocop:enable Metrics/AbcSize
 
     def ensure_folders_exist(remote_folders)
@@ -246,33 +250,37 @@ def create_remote_folder(project_storage)
     def audit_last_project_folder(last_project_folder, project_folder_id)
       ApplicationRecord.transaction do
         success = last_project_folder.update(origin_folder_id: project_folder_id) &&
-                  last_project_folder.project_storage.update(project_folder_id:)
+          last_project_folder.project_storage.update(project_folder_id:)
 
         raise ActiveRecord::Rollback unless success
       end
     end
 
-    # @param group_folder [string] name of the Group Folder in Nextcloud.
-    # @param username [String] username for the integration user
-    # @param group [String] group that the user should be part of
+    # rubocop:disable Metrics/AbcSize
+    # @param root_folder_id [String] the id of the root folder
     # @return [ServiceResult]
-    def ensure_root_folder_permissions(group_folder, username, group)
-      info "Setting needed permissions for user #{username} and group #{group} on #{group_folder} group folder"
-
-      command_params = {
-        path: group_folder,
-        permissions: {
-          users: { username.to_sym => ALL_PERMISSIONS },
-          groups: { group.to_sym => PERMISSIONS_MAP[:read_files] }
-        }
-      }
-
-      set_permissions.call(storage: @storage, auth_strategy:, **command_params).on_failure do |service_result|
-        log_storage_error(service_result.errors, { folder: group_folder })
+    def ensure_root_folder_permissions(root_folder_id)
+      username = @storage.username
+      group = @storage.group
+      info "Setting needed permissions for user #{username} and group #{group} on the root group folder."
+      permissions = [
+        { user_id: username, permissions: FILE_PERMISSIONS },
+        { group_id: group, permissions: [:read_files] }
+      ]
+
+      input_data = build_set_permissions_input_data(root_folder_id, permissions).value_or do |failure|
+        log_validation_error(failure, root_folder_id:, permissions:)
+        return ServiceResult.failure(result: failure.errors.to_h) # rubocop:disable Rails/DeprecatedActiveModelErrorsMethods
+      end
+
+      set_permissions.call(storage: @storage, auth_strategy:, input_data:).on_failure do |service_result|
+        log_storage_error(service_result.errors, folder: "root", root_folder_id:)
         add_error(:ensure_root_folder_permissions, service_result.errors, options: { group:, username: }).fail!
       end
     end
 
+    # rubocop:enable Metrics/AbcSize
+
     def remote_root_folder_map(group_folder)
       info "Retrieving already existing folders under #{group_folder}"
       file_ids.call(storage: @storage, path: group_folder).on_failure do |service_result|
@@ -281,6 +289,10 @@ def remote_root_folder_map(group_folder)
       end
     end
 
+    def build_set_permissions_input_data(file_id, user_permissions)
+      Peripherals::StorageInteraction::Inputs::SetPermissions.build(file_id:, user_permissions:)
+    end
+
     def remote_group_users
       info "Retrieving users that a part of the #{@storage.group} group"
       group_users.call(storage: @storage, group: @storage.group)

modules/storages/lib/open_project/storages/engine.rb

@@ -38,8 +38,8 @@
 
 module OpenProject::Storages
   class Engine < ::Rails::Engine
-    def self.permissions
-      @permissions ||= Storages::NextcloudManagedFolderSyncService::PERMISSIONS_KEYS
+    def self.external_file_permissions
+      %i[read_files write_files create_files delete_files share_files].freeze
     end
 
     # engine name is used as a default prefix for module tables when generating
@@ -90,15 +90,15 @@ def self.permissions
         OpenProject::Notifications.subscribe(
           OpenProject::Events::ROLE_UPDATED
         ) do |payload|
-          if payload[:permissions_diff]&.intersect?(OpenProject::Storages::Engine.permissions)
+          if payload[:permissions_diff]&.intersect?(OpenProject::Storages::Engine.external_file_permissions)
             ::Storages::ManageStorageIntegrationsJob.debounce
           end
         end
 
         OpenProject::Notifications.subscribe(
           OpenProject::Events::ROLE_DESTROYED
         ) do |payload|
-          if payload[:permissions]&.intersect?(OpenProject::Storages::Engine.permissions)
+          if payload[:permissions]&.intersect?(OpenProject::Storages::Engine.external_file_permissions)
             ::Storages::ManageStorageIntegrationsJob.debounce
           end
         end
@@ -166,7 +166,7 @@ def self.permissions
                      "storages/project_settings/project_storage_members": %i[index] },
                    permissible_on: :project,
                    dependencies: %i[]
-        OpenProject::Storages::Engine.permissions.each do |p|
+        OpenProject::Storages::Engine.external_file_permissions.each do |p|
           permission(p, {}, permissible_on: :project, dependencies: %i[])
         end
       end
@@ -219,10 +219,10 @@ def self.permissions
           prj.project_storages.each do |prj_storage|
             storage = prj_storage.storage
             hide_from_menu = !storage.configured? ||
-                             # the following check is required for ensure access modal final check being possible
-                             # the modal waiting for read_files permission on the project folder
-                             # otherwise polls backend until eternity
-                             (prj_storage.project_folder_automatic? && !u.allowed_in_project?(:read_files, prj))
+              # the following check is required for ensure access modal final check being possible
+              # the modal waiting for read_files permission on the project folder
+              # otherwise polls backend until eternity
+              (prj_storage.project_folder_automatic? && !u.allowed_in_project?(:read_files, prj))
             next if hide_from_menu
 
             icon = storage.provider_type_nextcloud? ? "op-mark-nextcloud" : "file-directory"