Merge pull request #1486 from JohnVillalovos/jlvillal/prohibit_redirection

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

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)