Merge pull request #5931 from Shopify/permanent-redirect-error

Add better error handling for permanent redirect responses
rubygems · Sep 29, 2022 · 087c7de · 087c7de
2 parents ed73566 + 46990f3
commit 087c7de
Show file tree

Hide file tree

Showing 7 changed files with 245 additions and 85 deletions.
diff --git a/lib/rubygems/gemcutter_utilities.rb b/lib/rubygems/gemcutter_utilities.rb
@@ -201,7 +201,8 @@ def verify_api_key(key)
   # block was given or shows the response body to the user.
   #
   # If the response was not successful, shows an error to the user including
-  # the +error_prefix+ and the response body.
+  # the +error_prefix+ and the response body. If the response was a permanent redirect,
+  # shows an error to the user including the redirect location.
 
   def with_response(response, error_prefix = nil)
     case response
@@ -211,6 +212,12 @@ def with_response(response, error_prefix = nil)
       else
         say clean_text(response.body)
       end
+    when Net::HTTPPermanentRedirect, Net::HTTPRedirection then
+      message = "The request has redirected permanently to #{response['location']}. Please check your defined push host URL."
+      message = "#{error_prefix}: #{message}" if error_prefix
+
+      say clean_text(message)
+      terminate_interaction(ERROR_CODE)
     else
       message = response.body
       message = "#{error_prefix}: #{message}" if error_prefix

diff --git a/test/rubygems/test_gem_commands_owner_command.rb b/test/rubygems/test_gem_commands_owner_command.rb
@@ -36,7 +36,7 @@ def test_show_owners
 - id: 4
 EOF
 
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
 
     use_ui @stub_ui do
       @cmd.show_owners("freewill")
@@ -66,7 +66,7 @@ def test_show_owners_dont_load_objects
 - id: 4
 EOF
 
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
 
     assert_raise Psych::DisallowedClass do
       use_ui @ui do
@@ -80,7 +80,7 @@ def test_show_owners_setting_up_host_through_env_var
     host = "http://rubygems.example"
     ENV["RUBYGEMS_HOST"] = host
 
-    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners.yaml"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners.yaml"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
 
     use_ui @stub_ui do
       @cmd.show_owners("freewill")
@@ -95,7 +95,7 @@ def test_show_owners_setting_up_host
     host = "http://rubygems.example"
     @cmd.host = host
 
-    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners.yaml"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners.yaml"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
 
     use_ui @stub_ui do
       @cmd.show_owners("freewill")
@@ -107,7 +107,7 @@ def test_show_owners_setting_up_host
 
   def test_show_owners_denied
     response = "You don't have permission to push to this gem"
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = [response, 403, "Forbidden"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = HTTPResponseFactory.create(body: response, code: 403, msg: "Forbidden")
 
     assert_raise Gem::MockGemUi::TermError do
       use_ui @stub_ui do
@@ -118,9 +118,32 @@ def test_show_owners_denied
     assert_match response, @stub_ui.output
   end
 
+  def test_show_owners_permanent_redirect
+    host = "http://rubygems.example"
+    ENV["RUBYGEMS_HOST"] = host
+    path = "/api/v1/gems/freewill/owners.yaml"
+    redirected_uri = "https://rubygems.example#{path}"
+
+    @stub_fetcher.data["#{host}#{path}"] = HTTPResponseFactory.create(
+      body: "",
+      code: "301",
+      msg: "Moved Permanently",
+      headers: { "location" => redirected_uri }
+    )
+
+    assert_raise Gem::MockGemUi::TermError do
+      use_ui @stub_ui do
+        @cmd.show_owners("freewill")
+      end
+    end
+
+    response = "The request has redirected permanently to #{redirected_uri}. Please check your defined push host URL."
+    assert_match response, @stub_ui.output
+  end
+
   def test_show_owners_key
     response = "- email: user1@example.com\n"
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners.yaml"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
     File.open Gem.configuration.credentials_path, "a" do |f|
       f.write ":other: 701229f217cdf23b1344c7b4b54ca97"
     end
@@ -134,7 +157,7 @@ def test_show_owners_key
 
   def test_add_owners
     response = "Owner added successfully."
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
 
     use_ui @stub_ui do
       @cmd.add_owners("freewill", ["user-new1@example.com"])
@@ -149,21 +172,42 @@ def test_add_owners
 
   def test_add_owners_denied
     response = "You don't have permission to push to this gem"
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 403, "Forbidden"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 403, msg: "Forbidden")
+
+    use_ui @stub_ui do
+      @cmd.add_owners("freewill", ["user-new1@example.com"])
+    end
+
+    assert_match response, @stub_ui.output
+  end
+
+  def test_add_owners_permanent_redirect
+    host = "http://rubygems.example"
+    ENV["RUBYGEMS_HOST"] = host
+    path = "/api/v1/gems/freewill/owners"
+    redirected_uri = "https://rubygems.example#{path}"
+
+    @stub_fetcher.data["#{host}#{path}"] = HTTPResponseFactory.create(
+      body: "",
+      code: "308",
+      msg: "Permanent Redirect",
+      headers: { "location" => redirected_uri }
+    )
 
     use_ui @stub_ui do
       @cmd.add_owners("freewill", ["user-new1@example.com"])
     end
 
+    response = "The request has redirected permanently to #{redirected_uri}. Please check your defined push host URL."
     assert_match response, @stub_ui.output
   end
 
   def test_add_owner_with_host_option_through_execute
     host = "http://rubygems.example"
     add_owner_response = "Owner added successfully."
     show_owners_response = "- email: user1@example.com\n"
-    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners"] = [add_owner_response, 200, "OK"]
-    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners.yaml"] = [show_owners_response, 200, "OK"]
+    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: add_owner_response, code: 200, msg: "OK")
+    @stub_fetcher.data["#{host}/api/v1/gems/freewill/owners.yaml"] = HTTPResponseFactory.create(body: show_owners_response, code: 200, msg: "OK")
 
     @cmd.handle_options %W[--host #{host} --add user-new1@example.com freewill]
 
@@ -178,7 +222,7 @@ def test_add_owner_with_host_option_through_execute
 
   def test_add_owners_key
     response = "Owner added successfully."
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
     File.open Gem.configuration.credentials_path, "a" do |f|
       f.write ":other: 701229f217cdf23b1344c7b4b54ca97"
     end
@@ -192,7 +236,7 @@ def test_add_owners_key
 
   def test_remove_owners
     response = "Owner removed successfully."
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
 
     use_ui @stub_ui do
       @cmd.remove_owners("freewill", ["user-remove1@example.com"])
@@ -207,7 +251,7 @@ def test_remove_owners
 
   def test_remove_owners_denied
     response = "You don't have permission to push to this gem"
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 403, "Forbidden"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 403, msg: "Forbidden")
 
     use_ui @stub_ui do
       @cmd.remove_owners("freewill", ["user-remove1@example.com"])
@@ -216,9 +260,46 @@ def test_remove_owners_denied
     assert_match response, @stub_ui.output
   end
 
+  def test_remove_owners_permanent_redirect
+    host = "http://rubygems.example"
+    ENV["RUBYGEMS_HOST"] = host
+    path = "/api/v1/gems/freewill/owners"
+    redirected_uri = "https://rubygems.example#{path}"
+    @stub_fetcher.data["#{host}#{path}"] = HTTPResponseFactory.create(
+      body: "",
+      code: "308",
+      msg: "Permanent Redirect",
+      headers: { "location" => redirected_uri }
+    )
+
+    use_ui @stub_ui do
+      @cmd.remove_owners("freewill", ["user-remove1@example.com"])
+    end
+
+    response = "The request has redirected permanently to #{redirected_uri}. Please check your defined push host URL."
+    assert_match response, @stub_ui.output
+
+    path = "/api/v1/gems/freewill/owners"
+    redirected_uri = "https://rubygems.example#{path}"
+
+    @stub_fetcher.data["#{host}#{path}"] = HTTPResponseFactory.create(
+      body: "",
+      code: "308",
+      msg: "Permanent Redirect",
+      headers: { "location" => redirected_uri }
+    )
+
+    use_ui @stub_ui do
+      @cmd.add_owners("freewill", ["user-new1@example.com"])
+    end
+
+    response = "The request has redirected permanently to #{redirected_uri}. Please check your defined push host URL."
+    assert_match response, @stub_ui.output
+  end
+
   def test_remove_owners_key
     response = "Owner removed successfully."
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 200, msg: "OK")
     File.open Gem.configuration.credentials_path, "a" do |f|
       f.write ":other: 701229f217cdf23b1344c7b4b54ca97"
     end
@@ -232,7 +313,7 @@ def test_remove_owners_key
 
   def test_remove_owners_missing
     response = "Owner could not be found."
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 404, "Not Found"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 404, msg: "Not Found")
 
     use_ui @stub_ui do
       @cmd.remove_owners("freewill", ["missing@example"])
@@ -246,8 +327,8 @@ def test_otp_verified_success
     response_success = "Owner added successfully."
 
     @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [
-      [response_fail, 401, "Unauthorized"],
-      [response_success, 200, "OK"],
+      HTTPResponseFactory.create(body: response_fail, code: 401, msg: "Unauthorized"),
+      HTTPResponseFactory.create(body: response_success, code: 200, msg: "OK"),
     ]
 
     @otp_ui = Gem::MockGemUi.new "111111\n"
@@ -263,7 +344,7 @@ def test_otp_verified_success
 
   def test_otp_verified_failure
     response = "You have enabled multifactor authentication but your request doesn't have the correct OTP code. Please check it and retry."
-    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [response, 401, "Unauthorized"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = HTTPResponseFactory.create(body: response, code: 401, msg: "Unauthorized")
 
     @otp_ui = Gem::MockGemUi.new "111111\n"
     use_ui @otp_ui do
@@ -281,10 +362,10 @@ def test_remove_owners_unathorized_api_key
     response_success   = "Owner removed successfully."
 
     @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [
-      [response_forbidden, 403, "Forbidden"],
-      [response_success, 200, "OK"],
+      HTTPResponseFactory.create(body: response_forbidden, code: 403, msg: "Forbidden"),
+      HTTPResponseFactory.create(body: response_success, code: 200, msg: "OK"),
     ]
-    @stub_fetcher.data["#{Gem.host}/api/v1/api_key"] = ["", 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/api_key"] = HTTPResponseFactory.create(body: "", code: 200, msg: "OK")
     @cmd.instance_variable_set :@scope, :remove_owner
 
     @stub_ui = Gem::MockGemUi.new "some@mail.com\npass\n"
@@ -305,10 +386,10 @@ def test_add_owners_unathorized_api_key
     response_success   = "Owner added successfully."
 
     @stub_fetcher.data["#{Gem.host}/api/v1/gems/freewill/owners"] = [
-      [response_forbidden, 403, "Forbidden"],
-      [response_success, 200, "OK"],
+      HTTPResponseFactory.create(body: response_forbidden, code: 403, msg: "Forbidden"),
+      HTTPResponseFactory.create(body: response_success, code: 200, msg: "OK"),
     ]
-    @stub_fetcher.data["#{Gem.host}/api/v1/api_key"] = ["", 200, "OK"]
+    @stub_fetcher.data["#{Gem.host}/api/v1/api_key"] = HTTPResponseFactory.create(body: "", code: 200, msg: "OK")
     @cmd.instance_variable_set :@scope, :add_owner
 
     @stub_ui = Gem::MockGemUi.new "some@mail.com\npass\n"