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

JohnVillalovos · JohnVillalovos · commit d56a4345c1ae · 2021-09-08T09:27:41.000-07:00
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: #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: #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
diff --git 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
@@ -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
@@ -29,8 +29,9 @@
 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 @@ def _build_url(self, path: str) -> str:
             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
@@ -2,7 +2,7 @@
 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 resp_cont(url, request):
         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)
