diff --git a/lib50/_errors.py b/lib50/_errors.py index 0c45705..b113347 100644 --- a/lib50/_errors.py +++ b/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 \ No newline at end of file + 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) diff --git a/lib50/authentication.py b/lib50/authentication.py index 6b5ff4b..57e6645 100644 --- a/lib50/authentication.py +++ b/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}") diff --git a/setup.py b/setup.py index e9c35c3..7752a74 100644 --- a/setup.py +++ b/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 ) diff --git a/tests/api_tests.py b/tests/api_tests.py index f72f9f1..ad1f61a 100644 --- a/tests/api_tests.py +++ b/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: