launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/gh-rate-limit-auth into lp:launchpad.

Commit message:
Fix GitHub bug sync rate limit check to not use the anonymous limit.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1650134 in Launchpad itself: "GitHub external bug tracker fails to authenticate rate limit checks"
  https://bugs.launchpad.net/launchpad/+bug/1650134

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/gh-rate-limit-auth/+merge/313309

Pass the GitHub bug sync auth down to GitHubRateLimit.

Previously all requests except the rate limit check were authenticated,
but the rate limit check omitted the token, so requests would be
cancelled when they would have succeeded.

-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/gh-rate-limit-auth into lp:launchpad.

=== modified file 'lib/lp/bugs/externalbugtracker/github.py'
--- lib/lp/bugs/externalbugtracker/github.py	2016-07-04 17:11:29 +0000
+++ lib/lp/bugs/externalbugtracker/github.py	2016-12-15 07:27:15 +0000
@@ -77,28 +77,39 @@
     def __init__(self):
         self.clearCache()
 
-    def _update(self, host, token=None):
+    def _update(self, host, auth_header=None):
         headers = {
             "User-Agent": LP_USER_AGENT,
             "Host": host,
             "Accept": "application/vnd.github.v3+json",
             }
-        if token is not None:
-            headers["Authorization"] = "token %s" % token
+        if auth_header is not None:
+            headers["Authorization"] = auth_header
         url = "https://%s/rate_limit"; % host
         try:
             response = requests.get(url, headers=headers)
             response.raise_for_status()
-            self._limits[(host, token)] = response.json()["resources"]["core"]
+            return response.json()["resources"]["core"]
         except requests.RequestException as e:
             raise BugTrackerConnectError(url, e)
 
     @ensure_no_transaction
     def makeRequest(self, method, url, token=None, **kwargs):
         """See `IGitHubRateLimit`."""
+        if token is not None:
+            auth_header = "token %s" % token
+            if "headers" in kwargs:
+                kwargs["headers"] = kwargs["headers"].copy()
+            else:
+                kwargs["headers"] = {}
+            kwargs["headers"]["Authorization"] = auth_header
+        else:
+            auth_header = None
+
         host = urlsplit(url).netloc
         if (host, token) not in self._limits:
-            self._update(host, token=token)
+            self._limits[(host, token)] = self._update(
+                host, auth_header=auth_header)
         limit = self._limits[(host, token)]
         if not limit["remaining"]:
             raise GitHubExceededRateLimit(host, limit["reset"])
@@ -216,9 +227,6 @@
     def _getHeaders(self, last_accessed=None):
         """See `ExternalBugTracker`."""
         headers = super(GitHub, self)._getHeaders()
-        token = self.credentials["token"]
-        if token is not None:
-            headers["Authorization"] = "token %s" % token
         headers["Accept"] = "application/vnd.github.v3+json"
         if last_accessed is not None:
             headers["If-Modified-Since"] = (
@@ -234,7 +242,8 @@
         try:
             response = getUtility(IGitHubRateLimit).makeRequest(
                 "GET", urljoin(self.baseurl + "/", page),
-                headers=self._getHeaders(last_accessed=last_accessed))
+                headers=self._getHeaders(last_accessed=last_accessed),
+                token=self.credentials["token"])
             response.raise_for_status()
             return response
         except requests.RequestException as e:

=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_github.py'
--- lib/lp/bugs/externalbugtracker/tests/test_github.py	2016-07-04 17:11:29 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_github.py	2016-12-15 07:27:15 +0000
@@ -165,6 +165,40 @@
             "content": {"resources": {"core": rate_limit}},
             }
 
+    def test__getPage_authenticated(self):
+        @urlmatch(path=r".*/test$")
+        def handler(url, request):
+            self.request = request
+            return {"status_code": 200, "content": json.dumps("success")}
+
+        self.pushConfig(
+            "checkwatches.credentials", **{"api.github.com.token": "sosekrit"})
+        tracker = GitHub("https://github.com/user/repository/issues";)
+        with HTTMock(self._rate_limit_handler, handler):
+            self.assertEqual("success", tracker._getPage("test").json())
+        self.assertEqual(
+            "https://api.github.com/repos/user/repository/test";,
+            self.request.url)
+        self.assertEqual(
+            "token sosekrit", self.request.headers["Authorization"])
+        self.assertEqual(
+            "token sosekrit", self.rate_limit_request.headers["Authorization"])
+
+    def test__getPage_unauthenticated(self):
+        @urlmatch(path=r".*/test$")
+        def handler(url, request):
+            self.request = request
+            return {"status_code": 200, "content": json.dumps("success")}
+
+        tracker = GitHub("https://github.com/user/repository/issues";)
+        with HTTMock(self._rate_limit_handler, handler):
+            self.assertEqual("success", tracker._getPage("test").json())
+        self.assertEqual(
+            "https://api.github.com/repos/user/repository/test";,
+            self.request.url)
+        self.assertNotIn("Authorization", self.request.headers)
+        self.assertNotIn("Authorization", self.rate_limit_request.headers)
+
     def test_getRemoteBug(self):
         @urlmatch(path=r".*/issues/1$")
         def handler(url, request):