fix!: raise error if there is a 301/302 redirection

Before we raised an error if there was a 301, 302 redirect but only
from an http URL to an https URL.  But we didn't raise an error for
any other redirects.

This caused two problems:

  1. PUT requests that are redirected get changed to GET requests
     which don't perform the desired action but raise no error. This
     is because the GET response succeeds but since it wasn't a PUT it
     doesn't update. See issue:
     https://github.com/python-gitlab/python-gitlab/issues/1432
  2. POST requests that are redirected also got changed to GET
     requests. They also caused hard to debug tracebacks for the user.
     See issue:
     https://github.com/python-gitlab/python-gitlab/issues/1477

Correct this by always raising a RedirectError exception and improve
the exception message to let them know what was redirected.

Closes: #1485
Closes: #1432
Closes: #1477

1 files changed, 89 insertions, 2 deletions

diff --git a/tests/unit/test_gitlab_http_methods.py b/tests/unit/test_gitlab_http_methods.py
index 5a3584e..ba57c31 100644
--- a/tests/unit/test_gitlab_http_methods.py
+++ b/tests/unit/test_gitlab_http_methods.py
@@ -2,7 +2,7 @@ import pytest
 import requests
 from httmock import HTTMock, response, urlmatch
 
-from gitlab import GitlabHttpError, GitlabList, GitlabParsingError
+from gitlab import GitlabHttpError, GitlabList, GitlabParsingError, RedirectError
 
 
 def test_build_url(gl):
@@ -123,9 +123,96 @@ def test_http_request_with_retry_on_class_and_method_for_transient_failures(gl_r
         assert call_count == 1
 
 
+def create_redirect_response(
+    *, request: requests.models.PreparedRequest, http_method: str, api_path: str
+) -> requests.models.Response:
+    """Create a Requests response object that has a redirect in it"""
+
+    assert api_path.startswith("/")
+    http_method = http_method.upper()
+
+    # Create a history which contains our original request which is redirected
+    history = [
+        response(
+            status_code=302,
+            content="",
+            headers={"Location": f"http://example.com/api/v4{api_path}"},
+            reason="Moved Temporarily",
+            request=request,
+        )
+    ]
+
+    # Create a "prepped" Request object to be the final redirect. The redirect
+    # will be a "GET" method as Requests changes the method to "GET" when there
+    # is a 301/302 redirect code.
+    req = requests.Request(
+        method="GET",
+        url=f"http://example.com/api/v4{api_path}",
+    )
+    prepped = req.prepare()
+
+    resp_obj = response(
+        status_code=200,
+        content="",
+        headers={},
+        reason="OK",
+        elapsed=5,
+        request=prepped,
+    )
+    resp_obj.history = history
+    return resp_obj
+
+
+def test_http_request_302_get_does_not_raise(gl):
+    """Test to show that a redirect of a GET will not cause an error"""
+
+    method = "get"
+    api_path = "/user/status"
+
+    @urlmatch(
+        scheme="http", netloc="localhost", path=f"/api/v4{api_path}", method=method
+    )
+    def resp_cont(
+        url: str, request: requests.models.PreparedRequest
+    ) -> requests.models.Response:
+        resp_obj = create_redirect_response(
+            request=request, http_method=method, api_path=api_path
+        )
+        return resp_obj
+
+    with HTTMock(resp_cont):
+        gl.http_request(verb=method, path=api_path)
+
+
+def test_http_request_302_put_raises_redirect_error(gl):
+    """Test to show that a redirect of a PUT will cause an error"""
+
+    method = "put"
+    api_path = "/user/status"
+
+    @urlmatch(
+        scheme="http", netloc="localhost", path=f"/api/v4{api_path}", method=method
+    )
+    def resp_cont(
+        url: str, request: requests.models.PreparedRequest
+    ) -> requests.models.Response:
+        resp_obj = create_redirect_response(
+            request=request, http_method=method, api_path=api_path
+        )
+        return resp_obj
+
+    with HTTMock(resp_cont):
+        with pytest.raises(RedirectError) as exc:
+            gl.http_request(verb=method, path=api_path)
+        error_message = exc.value.error_message
+        assert "Moved Temporarily" in error_message
+        assert "http://localhost/api/v4/user/status" in error_message
+        assert "http://example.com/api/v4/user/status" in error_message
+
+
 def test_get_request(gl):
     @urlmatch(scheme="http", netloc="localhost", path="/api/v4/projects", method="get")
-    def resp_cont(url, request):
+    def resp_cont(url: str, request: requests.models.PreparedRequest):
         headers = {"content-type": "application/json"}
         content = '{"name": "project1"}'
         return response(200, content, headers, None, 5, request)