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

4 files changed, 132 insertions, 23 deletions

diff --git a/docs/api-usage.rst b/docs/api-usage.rst
index 2f7166e..d4a4106 100644
--- a/docs/api-usage.rst
+++ b/docs/api-usage.rst
@@ -14,7 +14,9 @@ To connect to a GitLab server, create a ``gitlab.Gitlab`` object:
    import gitlab
 
    # private token or personal token authentication
-   gl = gitlab.Gitlab('http://10.0.0.1', private_token='JVNSESs8EwWRx5yDxM5q')
+   # Note that a 'url' that results in 301/302 redirects will cause an error
+   # (see below for more information).
+   gl = gitlab.Gitlab(url='https://gitlab.example.com', private_token='JVNSESs8EwWRx5yDxM5q')
 
    # oauth token authentication
    gl = gitlab.Gitlab('http://10.0.0.1', oauth_token='my_long_token_here')
@@ -47,6 +49,13 @@ configuration files.
    If the GitLab server you are using redirects requests from http to https,
    make sure to use the ``https://`` protocol in the URL definition.
 
+.. note::
+
+   It is highly recommended to use the final destination in the ``url`` field.
+   What this means is that you should not use a URL which redirects as it will
+   most likely cause errors. python-gitlab will raise a ``RedirectionError``
+   when it encounters a redirect which it believes will cause an error.
+
 Note on password authentication
 -------------------------------
 
diff --git a/docs/cli-usage.rst b/docs/cli-usage.rst
index 1a80bbc..e263ef2 100644
--- a/docs/cli-usage.rst
+++ b/docs/cli-usage.rst
@@ -89,6 +89,13 @@ You must define the ``url`` in each GitLab server section.
    If the GitLab server you are using redirects requests from http to https,
    make sure to use the ``https://`` protocol in the ``url`` definition.
 
+.. note::
+
+   It is highly recommended to use the final destination in the ``url`` field.
+   What this means is that you should not use a URL which redirects as it will
+   most likely cause errors. python-gitlab will raise a ``RedirectionError``
+   when it encounters a redirect which it believes will cause an error.
+
 Only one of ``private_token``, ``oauth_token`` or ``job_token`` should be
 defined. If neither are defined an anonymous request will be sent to the Gitlab
 server, with very limited permissions.
@@ -101,7 +108,7 @@ We recommend that you use `Credential helpers`_ to securely store your tokens.
    * - Option
      - Description
    * - ``url``
-     - URL for the GitLab server
+     - URL for the GitLab server. Do **NOT** use a URL which redirects.
    * - ``private_token``
      - Your user token. Login/password is not supported. Refer to `the
        official documentation
diff --git a/gitlab/client.py b/gitlab/client.py
index 47fae81..9db3a0e 100644
--- a/gitlab/client.py
+++ b/gitlab/client.py
@@ -29,8 +29,9 @@ import gitlab.exceptions
 from gitlab import utils
 
 REDIRECT_MSG = (
-    "python-gitlab detected an http to https redirection. You "
-    "must update your GitLab URL to use https:// to avoid issues."
+    "python-gitlab detected a {status_code} ({reason!r}) redirection. You must update "
+    "your GitLab URL to the correct URL to avoid issues. The redirection was from: "
+    "{source!r} to {target!r}"
 )
 
 
@@ -456,24 +457,29 @@ class Gitlab(object):
             return "%s%s" % (self._url, path)
 
     def _check_redirects(self, result: requests.Response) -> None:
-        # Check the requests history to detect http to https redirections.
-        # If the initial verb is POST, the next request will use a GET request,
-        # leading to an unwanted behaviour.
-        # If the initial verb is PUT, the data will not be send with the next
-        # request.
-        # If we detect a redirection to https with a POST or a PUT request, we
+        # Check the requests history to detect 301/302 redirections.
+        # If the initial verb is POST or PUT, the redirected request will use a
+        # GET request, leading to unwanted behaviour.
+        # If we detect a redirection with a POST or a PUT request, we
         # raise an exception with a useful error message.
-        if result.history and self._base_url.startswith("http:"):
-            for item in result.history:
-                if item.status_code not in (301, 302):
-                    continue
-                # GET methods can be redirected without issue
-                if item.request.method == "GET":
-                    continue
-                # Did we end-up with an https:// URL?
-                location = item.headers.get("Location", None)
-                if location and location.startswith("https://"):
-                    raise gitlab.exceptions.RedirectError(REDIRECT_MSG)
+        if not result.history:
+            return
+
+        for item in result.history:
+            if item.status_code not in (301, 302):
+                continue
+            # GET methods can be redirected without issue
+            if item.request.method == "GET":
+                continue
+            target = item.headers.get("location")
+            raise gitlab.exceptions.RedirectError(
+                REDIRECT_MSG.format(
+                    status_code=item.status_code,
+                    reason=item.reason,
+                    source=item.url,
+                    target=target,
+                )
+            )
 
     def _prepare_send_data(
         self,
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)