fix: Handle login failures with clear error messages #11629
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR improves error handling for authentication failures in the Open Library API client by making login failures explicit rather than silent, preventing downstream 403 errors when operations are attempted with unauthenticated sessions.
Key changes:
- Modified
login()method inapi.pyto return boolean success/failure instead of silently swallowing authentication errors - Added login result validation and clear error messages in
copydocs.pyto guide users when authentication fails - Enhanced error messages provide actionable guidance for both development and production scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openlibrary/api.py | Modified login() to return True on success, False on auth failures (401/403), and re-raise non-auth errors; added comprehensive docstring documenting the new return value and exception behavior |
| scripts/copydocs.py | Added login result checking for both autologin() and login() calls with clear error messages and actionable guidance; exits gracefully with status code 1 on authentication failures; includes helpful instructions for setting up credentials |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def login(self, username, password): | ||
| """Login to Open Library with given credentials.""" | ||
| """Login to Open Library with given credentials. | ||
|
|
||
| Returns: | ||
| bool: True if login was successful, False otherwise. | ||
|
|
||
| Raises: | ||
| OLError: If a non-authentication HTTP error occurs. | ||
| """ | ||
| headers = {'Content-Type': 'application/json'} | ||
| try: | ||
| data = json.dumps({"username": username, "password": password}) | ||
| response = self._request( | ||
| '/account/login', method='POST', data=data, headers=headers | ||
| ) | ||
| except OLError as e: | ||
| response = e | ||
| if e.code in (401, 403): | ||
| return False | ||
| raise | ||
|
|
||
| if 'Set-Cookie' in response.headers: | ||
| cookies = response.headers['Set-Cookie'].split(',') | ||
| self.cookie = ';'.join([c.split(';')[0] for c in cookies]) | ||
| return True | ||
| logger.warning("Login request succeeded but no session cookie was received") | ||
| return False |
There was a problem hiding this comment.
The modified login() method that now returns a boolean value lacks test coverage. Tests should be added to verify the behavior when login succeeds (returns True), when authentication fails with 401/403 (returns False), and when other HTTP errors occur (re-raises OLError). The PR description mentions tests in openlibrary/tests/test_api.py, but this file was not included in the pull request.
|
PTAL @cdrini |
github-actions
bot
added
the
Needs: Response
|
Can you please wait to be assigned to issues before working on them? I've asked this of you at least once before... Issues with the Issues with the Any change to the login handlers is risky. Had you asked to be assigned, I would have advised against changing this. The I am closing this, and once again asking you to avoid working on issues unless you are assigned to them. |
|
sorry @jimchamp but recently I didn't created any PR these all are old PRs, really sorry again |
3 participants
Closes #11628
Technical
The
login()method inapi.pywas silently swallowing authentication failures, causingcopydocs.pyto proceed with unauthenticated requests that returned 403 errors.Changes:
api.py: Modifiedlogin()to returnTrue/Falseinstead of silently failing. Re-raises non-auth errors.copydocs.py: Now checks login result and exits with clear error messages on failureTesting
copydocs.pywith invalid credentials - should exit with helpful error messageStakeholders
@cdrini