Distinguish GitHub rate limiting from an invalid token

lib50/_errors.py

@@ -1,3 +1,4 @@
+import datetime
 import os
 from . import _
 
@@ -12,7 +13,8 @@
     "TimeoutError",
     "ConnectionError",
     "RejectedHonestyPromptError",
-    "InvalidTokenError"
+    "InvalidTokenError",
+    "RateLimitError"
 ]
 
 
@@ -116,4 +118,27 @@ class RejectedHonestyPromptError(Error):
 
 class InvalidTokenError(Error):
     """A ``lib50.Error`` signalling that the GitHub token is invalid or expired."""
-    pass
+    pass
+
+
+class RateLimitError(Error):
+    """
+    A ``lib50.Error`` signalling the GitHub API rate limit is exhausted.
+    Unlike ``InvalidTokenError`` the token is valid, so the remedy is to wait, not to
+    re-authenticate.
+    ``RateLimitError.payload["reset"]`` is the UNIX timestamp when the budget resets, if known.
+    ``RateLimitError.payload["user_id"]`` and ``["request_id"]`` identify the account and the
+    GitHub request, when GitHub reported them.
+    """
+
+    def __init__(self, reset=None, user_id=None, request_id=None):
+        message = _("You have reached GitHub's hourly API rate limit.")
+        if reset:
+            try:
+                when = datetime.datetime.fromtimestamp(int(reset)).strftime("%H:%M")
+                message = _("You have reached GitHub's hourly API rate limit."
+                            " Please try again after {}.").format(when)
+            except (TypeError, ValueError):
+                pass
+        super().__init__(message)
+        self.payload.update(reset=reset, user_id=user_id, request_id=request_id)

lib50/authentication.py

@@ -14,7 +14,7 @@
 
 from . import _
 from . import _api as api
-from ._errors import ConnectionError, InvalidBranchError, InvalidTokenError, RejectedHonestyPromptError
+from ._errors import ConnectionError, InvalidBranchError, InvalidTokenError, RateLimitError, RejectedHonestyPromptError
 
 __all__ = ["User", "authenticate", "logout"]
 
@@ -254,10 +254,17 @@ def _authenticate_https(org, repo=None):
         # Validate that the token is actually working
         try:
             _validate_github_token(password)
+        except RateLimitError as e:
+            # The token is valid, so keep the cached credentials: restarting cannot help and
+            # logout() would discard a working credential.
+            msg = str(e) + _identify(user_id=e.payload.get("user_id"),
+                                     request_id=e.payload.get("request_id"))
+            print(termcolor.colored(msg, color="yellow", attrs=["bold"]))
+            sys.exit(1)
         except InvalidTokenError:
             msg = _("There seems to be an issue authenticating with your GitHub token."\
                 " Please visit https://cs50.dev/restart to restart your codespace and try again.")
-            print(termcolor.colored(msg, color="yellow", attrs=["bold"]))
+            print(termcolor.colored(msg + _identify(), color="yellow", attrs=["bold"]))
             logout()
             sys.exit(1)
         except ConnectionError:
@@ -345,8 +352,48 @@ def _show_gh_changes_warning():
     _show_gh_changes_warning.showed = True
 
 
+def _identify(user_id=None, request_id=None):
+    """Render who this codespace is authenticated as, to append to an error message.
+
+    Students report these failures by screenshotting the terminal, so the message itself has
+    to carry enough to find them in our logs without a follow-up question. The GitHub login
+    matches the user context we attach to Sentry events.
+    """
+
+    # In a CS50 codespace the workspace directory is the student's numeric GitHub id
+    if user_id is None and os.environ.get("RepositoryName", "").isdigit():
+        user_id = os.environ["RepositoryName"]
+
+    details = []
+    if username := os.environ.get("CS50_GH_USER"):
+        details.append(f"user {username}")
+    if user_id:
+        details.append(f"id {user_id}")
+    if request_id:
+        details.append(f"request {request_id}")
+    return "\n" + _("Details: {}").format(", ".join(details)) if details else ""
+
+
+def _github_user_id(response):
+    """The numeric account id GitHub names in a rate-limit message, if present.
+
+    GitHub answers an exhausted budget with "API rate limit exceeded for user ID 12345", which
+    is the only place the id is available on a failed request.
+    """
+    try:
+        match = re.search(r"for user ID (\d+)", response.json().get("message", ""))
+    except (AttributeError, ValueError):
+        return None
+    return match.group(1) if match else None
+
+
 def _validate_github_token(token):
-    """Validate a GitHub token by making an authenticated request to the GitHub API."""
+    """Validate a GitHub token by making an authenticated request to the GitHub API.
+
+    Raises RateLimitError when the token is valid but the account's hourly budget is spent.
+    GitHub reports both as 403, so they are told apart by the rate-limit headers; conflating
+    them tells students to restart their codespace when only waiting helps.
+    """
     try:
         response = requests.get(
             "https://api.github.com/user",
@@ -358,7 +405,13 @@ def _validate_github_token(token):
             timeout=10
         )
 
-        if response.status_code in (401, 403):
+        if response.status_code == 403 and (
+                response.headers.get("x-ratelimit-remaining") == "0"
+                or response.headers.get("retry-after")):
+            raise RateLimitError(reset=response.headers.get("x-ratelimit-reset"),
+                                 user_id=_github_user_id(response),
+                                 request_id=response.headers.get("x-github-request-id"))
+        elif response.status_code in (401, 403):
             raise InvalidTokenError()
         elif not response.ok:
             raise ConnectionError(f"Could not validate GitHub token. Received status code: {response.status_code}")

setup.py

@@ -27,6 +27,6 @@
     python_requires=">= 3.10",
     packages=["lib50"],
     url="https://github.com/cs50/lib50",
-    version="3.2.1",
+    version="3.2.2",
     include_package_data=True
 )

tests/api_tests.py

@@ -257,13 +257,85 @@ def test_invalid_token_401(self):
                 lib50.authentication._validate_github_token("invalid_token")
 
     def test_forbidden_token_403(self):
-        """Test that a 403 response raises InvalidTokenError."""
+        """Test that a 403 response with rate limit remaining raises InvalidTokenError."""
         with mock.patch("lib50.authentication.requests.get") as mock_get:
             mock_get.return_value.status_code = 403
             mock_get.return_value.ok = False
+            mock_get.return_value.headers = {"x-ratelimit-remaining": "4980"}
             with self.assertRaises(lib50._errors.InvalidTokenError):
                 lib50.authentication._validate_github_token("forbidden_token")
 
+    def test_rate_limited_403(self):
+        """Test that a 403 response with an exhausted rate limit raises RateLimitError."""
+        with mock.patch("lib50.authentication.requests.get") as mock_get:
+            mock_get.return_value.status_code = 403
+            mock_get.return_value.ok = False
+            mock_get.return_value.headers = {
+                "x-ratelimit-remaining": "0",
+                "x-ratelimit-reset": "1700000000"
+            }
+            mock_get.return_value.json.return_value = {}
+            with self.assertRaises(lib50._errors.RateLimitError):
+                lib50.authentication._validate_github_token("valid_but_throttled_token")
+
+    def test_rate_limit_error_carries_identity(self):
+        """Test that the account id and request id are captured for support.
+
+        Students report these failures by screenshot, so the message has to identify them
+        without a follow-up question.
+        """
+        with mock.patch("lib50.authentication.requests.get") as mock_get:
+            mock_get.return_value.status_code = 403
+            mock_get.return_value.ok = False
+            mock_get.return_value.headers = {
+                "x-ratelimit-remaining": "0",
+                "x-github-request-id": "ABC1:2DEF:34567:89ABC:DEF012"
+            }
+            mock_get.return_value.json.return_value = {
+                "message": "API rate limit exceeded for user ID 123456789."
+            }
+            with self.assertRaises(lib50._errors.RateLimitError) as cm:
+                lib50.authentication._validate_github_token("valid_but_throttled_token")
+            self.assertEqual(cm.exception.payload["user_id"], "123456789")
+            self.assertEqual(cm.exception.payload["request_id"], "ABC1:2DEF:34567:89ABC:DEF012")
+
+    def test_identify_reports_user_and_id(self):
+        """Test that _identify() names the student, and stays quiet when it cannot."""
+        with mock.patch.dict(os.environ, {"CS50_GH_USER": "student50",
+                                          "RepositoryName": "123456789"}, clear=False):
+            details = lib50.authentication._identify(request_id="REQ123")
+            self.assertIn("user student50", details)
+            self.assertIn("id 123456789", details)
+            self.assertIn("request REQ123", details)
+
+        # Outside a codespace the workspace name is not a numeric id, so omit it
+        with mock.patch.dict(os.environ, {"CS50_GH_USER": "student50",
+                                          "RepositoryName": "myproject"}, clear=False):
+            self.assertNotIn("id ", lib50.authentication._identify())
+
+    def test_identify_is_empty_without_context(self):
+        """Test that _identify() adds nothing rather than a bare 'Details:' line."""
+        with mock.patch.dict(os.environ, {}, clear=True):
+            self.assertEqual(lib50.authentication._identify(), "")
+
+    def test_secondary_rate_limited_403(self):
+        """Test that a 403 response with retry-after raises RateLimitError."""
+        with mock.patch("lib50.authentication.requests.get") as mock_get:
+            mock_get.return_value.status_code = 403
+            mock_get.return_value.ok = False
+            mock_get.return_value.headers = {"retry-after": "60", "x-ratelimit-remaining": "42"}
+            mock_get.return_value.json.return_value = {}
+            with self.assertRaises(lib50._errors.RateLimitError):
+                lib50.authentication._validate_github_token("valid_but_throttled_token")
+
+    def test_rate_limit_error_reports_reset_time(self):
+        """Test that RateLimitError tells the user when to retry, and copes without a reset."""
+        error = lib50._errors.RateLimitError(reset="1700000000")
+        self.assertIn("try again after", str(error))
+        self.assertEqual(error.payload["reset"], "1700000000")
+        self.assertIn("rate limit", str(lib50._errors.RateLimitError()))
+        self.assertIn("rate limit", str(lib50._errors.RateLimitError(reset="not-a-timestamp")))
+
     def test_other_http_error(self):
         """Test that other HTTP errors (e.g., 500) raise ConnectionError."""
         with mock.patch("lib50.authentication.requests.get") as mock_get: