Merge pull request #16853 from opf/implementation/57706-use-authentic…

…ation-in-remove_user_from_group_command [#57706] rework remove user command
opf · Oct 2, 2024 · df3cd50 · df3cd50
2 parents ada70fc + 0fba423
commit df3cd50
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 238 deletions.
diff --git a/...mmon/storages/peripherals/storage_interaction/nextcloud/internal/propfind_query_legacy.rb b/...mmon/storages/peripherals/storage_interaction/nextcloud/internal/propfind_query_legacy.rb
diff --git a/...mmon/storages/peripherals/storage_interaction/nextcloud/remove_user_from_group_command.rb b/...mmon/storages/peripherals/storage_interaction/nextcloud/remove_user_from_group_command.rb
@@ -34,61 +34,73 @@ module StorageInteraction
       module Nextcloud
         class RemoveUserFromGroupCommand
           include TaggedLogging
-          def self.call(storage:, user:, group: storage.group)
-            new(storage).call(user:, group:)
+
+          def self.call(storage:, auth_strategy:, user:, group:)
+            new(storage).call(auth_strategy:, user:, group:)
           end
 
           def initialize(storage)
             @storage = storage
-            @username = storage.username
-            @password = storage.password
-            @group = storage.group
           end
 
-          # rubocop:disable Metrics/AbcSize
-          def call(user:, group: @group)
+          def call(auth_strategy:, user:, group:)
             with_tagged_logger do
-              url = UrlBuilder.url(@storage.uri, "ocs/v1.php/cloud/users", user, "groups") +
-                "?groupid=#{CGI.escapeURIComponent(group)}"
+              Authentication[auth_strategy].call(storage: @storage, http_options:) do |http|
+                url = UrlBuilder.url(@storage.uri, "ocs/v1.php/cloud/users", user, "groups")
+                url += "?groupid=#{CGI.escapeURIComponent(group)}"
 
-              info "Removing #{user} from #{group} through #{url}"
+                info "Removing #{user} from #{group} through #{url}"
 
-              response = OpenProject.httpx.basic_auth(@username, @password)
-                                    .with(headers: { "OCS-APIRequest" => "true" })
-                                    .delete(url)
+                handle_response(http.delete(url))
+              end
+            end
+          end
 
-              error_data = StorageErrorData.new(source: self.class, payload: response)
+          private
 
-              case response
-              in { status: 200..299 }
-                statuscode = Nokogiri::XML(response.body.to_s).xpath("/ocs/meta/statuscode").text
-                case statuscode
-                when "100"
-                  info "User has been removed from group"
-                  ServiceResult.success
-                when "101"
-                  Util.error(:error, "No group specified", error_data)
-                when "102"
-                  Util.error(:group_does_not_exist, "Group does not exist", error_data)
-                when "103"
-                  Util.error(:user_does_not_exist, "User does not exist", error_data)
-                when "104"
-                  Util.error(:insufficient_privileges, "Insufficient privileges", error_data)
-                when "105"
-                  message = Nokogiri::XML(response.body).xpath("/ocs/meta/message").text
-                  Util.error(:failed_to_remove, message, error_data)
-                end
-              in { status: 405 }
-                Util.error(:not_allowed, "Outbound request method not allowed", error_data)
-              in { status: 401 }
-                Util.error(:unauthorized, "Outbound request not authorized", error_data)
-              in { status: 404 }
-                Util.error(:not_found, "Outbound request destination not found", error_data)
-              in { status: 409 }
-                Util.error(:conflict, Util.error_text_from_response(response), error_data)
-              else
-                Util.error(:error, "Outbound request failed", error_data)
-              end
+          def http_options
+            Util.ocs_api_request
+          end
+
+          def handle_response(response)
+            error_data = StorageErrorData.new(source: self.class, payload: response)
+
+            case response
+            in { status: 200..299 }
+              handle_success_response(response)
+            in { status: 405 }
+              Util.error(:not_allowed, "Outbound request method not allowed", error_data)
+            in { status: 401 }
+              Util.error(:unauthorized, "Outbound request not authorized", error_data)
+            in { status: 404 }
+              Util.error(:not_found, "Outbound request destination not found", error_data)
+            in { status: 409 }
+              Util.error(:conflict, Util.error_text_from_response(response), error_data)
+            else
+              Util.error(:error, "Outbound request failed", error_data)
+            end
+          end
+
+          # rubocop:disable Metrics/AbcSize
+          def handle_success_response(response)
+            error_data = StorageErrorData.new(source: self.class, payload: response)
+
+            statuscode = Nokogiri::XML(response.body.to_s).xpath("/ocs/meta/statuscode").text
+            case statuscode
+            when "100"
+              info "User has been removed from group"
+              ServiceResult.success
+            when "101"
+              Util.error(:error, "No group specified", error_data)
+            when "102"
+              Util.error(:group_does_not_exist, "Group does not exist", error_data)
+            when "103"
+              Util.error(:user_does_not_exist, "User does not exist", error_data)
+            when "104"
+              Util.error(:insufficient_privileges, "Insufficient privileges", error_data)
+            when "105"
+              message = Nokogiri::XML(response.body).xpath("/ocs/meta/message").text
+              Util.error(:failed_to_remove, message, error_data)
             end
           end
 

diff --git a/modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb b/modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb
@@ -122,10 +122,12 @@ def add_users_to_remote_group(users_to_add)
     end
 
     def remove_users_from_remote_group(users_to_remove)
+      group = @storage.group
+
       users_to_remove.each do |user|
-        remove_user_from_group.call(storage: @storage, user:).error_and do |error|
-          add_error(:remove_user_from_group, error, options: { user:, group: @storage.group, reason: error.log_message })
-          log_storage_error(error, group: @storage.group, user:, reason: error.log_message)
+        remove_user_from_group.call(storage: @storage, auth_strategy:, user:, group:).error_and do |error|
+          add_error(:remove_user_from_group, error, options: { user:, group:, reason: error.log_message })
+          log_storage_error(error, group:, user:, reason: error.log_message)
         end
       end
     end

diff --git a/modules/storages/spec/common/storages/peripherals/registry_spec.rb b/modules/storages/spec/common/storages/peripherals/registry_spec.rb
@@ -147,72 +147,6 @@
     end
   end
 
-  describe "#remove_user_from_group" do
-    let(:expected_response) do
-      {
-        status: 200,
-        body: expected_response_body,
-        headers: {}
-      }
-    end
-    let(:expected_response_body) do
-      <<~XML
-        <?xml version="1.0"?>
-        <ocs>
-          <meta>
-            <status>ok</status>
-            <statuscode>100</statuscode>
-            <message>OK</message>
-            <totalitems></totalitems>
-            <itemsperpage></itemsperpage>
-          </meta>
-          <data/>
-        </ocs>
-      XML
-    end
-
-    before do
-      stub_request(:delete, "https://example.com/ocs/v1.php/cloud/users/#{origin_user_id}/groups?groupid=#{storage.group}")
-        .with(
-          headers: {
-            "Authorization" => "Basic T3BlblByb2plY3Q6T3BlblByb2plY3RTZWN1cmVQYXNzd29yZA==",
-            "OCS-APIRequest" => "true"
-          }
-        )
-        .to_return(expected_response)
-    end
-
-    it "removes user from the group" do
-      result = registry.resolve("nextcloud.commands.remove_user_from_group").call(storage:, user: origin_user_id)
-      expect(result).to be_success
-    end
-
-    context "when Nextcloud reponds with 105 code in the response body" do
-      let(:expected_response_body) do
-        <<~XML
-          <?xml version="1.0"?>
-          <ocs>
-          <meta>
-            <status>failure</status>
-            <statuscode>105</statuscode>
-            <message>Not viable to remove user from the last group you are SubAdmin of</message>
-            <totalitems></totalitems>
-            <itemsperpage></itemsperpage>
-          </meta>
-          <data/>
-          </ocs>
-        XML
-      end
-
-      it "responds with a failure and parses message from the xml response" do
-        result = registry.resolve("nextcloud.commands.remove_user_from_group").call(storage:, user: origin_user_id)
-        expect(result).to be_failure
-        expect(result.errors.log_message)
-          .to eq("Not viable to remove user from the last group you are SubAdmin of")
-      end
-    end
-  end
-
   describe "#delete_folder_command" do
     let(:auth_strategy) { Storages::Peripherals::StorageInteraction::AuthenticationStrategies::BasicAuth.strategy }
 

diff --git a/modules/storages/spec/services/storages/nextcloud_managed_folder_sync_service_spec.rb b/modules/storages/spec/services/storages/nextcloud_managed_folder_sync_service_spec.rb
@@ -188,7 +188,11 @@ module Storages
         allow(group_users).to receive(:call).with(storage:, group: storage.group).and_return(group_users_result)
         # Updating the group users
         allow(add_user).to receive(:call).with(storage:, user: "single_project_user").and_return(add_user_result)
-        allow(remove_user).to receive(:call).with(storage:, user: "cookiemonster").and_return(remove_user_result)
+        allow(remove_user).to receive(:call).with(storage:,
+                                                  auth_strategy:,
+                                                  user: "cookiemonster",
+                                                  group: storage.group)
+                                            .and_return(remove_user_result)
       end
 
       it "applies changes to all project storages linked to the passed storage" do