Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request improves the authentication flow by standardizing OAuth2 token refresh endpoints and enhances code readability through formatting improvements. The changes focus on simplifying the refresh token function interface and aligning with OAuth2 standards.
- Updates the refresh token endpoint to use proper Form parameters instead of parsing request bodies
- Fixes exception handling by adding missing parentheses to exception instantiations
- Reformats code across multiple files for better readability and PEP 8 compliance
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/unipoll_api/routes/authentication.py | Updates refresh token endpoint to use Form parameters and re-enables authentication schemas import |
| src/unipoll_api/actions/authentication.py | Simplifies refresh_token_with_clientID function signature and fixes exception instantiation |
| src/unipoll_api/documents.py | Reformats imports and function definitions for improved code readability |
| # Make sure the access token exists in the database | ||
| if token_data is None: | ||
| raise AuthExceptions.InvalidAccessToken() | ||
| raise AuthExceptions.InvalidAccessToken |
There was a problem hiding this comment.
Exception class is not being instantiated. Should be raise AuthExceptions.InvalidAccessToken() to properly raise the exception.
| raise AuthExceptions.InvalidAccessToken | |
| raise AuthExceptions.InvalidAccessToken() |
| client_id = base64.b64decode(client_id) | ||
| if PydanticObjectId(str(client_id, "utf-8")[:-1]) != user.id: | ||
| raise AuthExceptions.InvalidClientID() | ||
| raise AuthExceptions.InvalidClientID |
There was a problem hiding this comment.
Exception class is not being instantiated. Should be raise AuthExceptions.InvalidClientID() to properly raise the exception.
| raise AuthExceptions.InvalidClientID | |
| raise AuthExceptions.InvalidClientID() |
| # If not, delete all tokens associated with the user and return an error | ||
| await strategy.destroy_token_family(user) | ||
| raise AuthExceptions.refreshTokenExpired() | ||
| raise AuthExceptions.refreshTokenExpired |
There was a problem hiding this comment.
Exception class is not being instantiated. Should be raise AuthExceptions.refreshTokenExpired() to properly raise the exception.
| raise AuthExceptions.refreshTokenExpired | |
| raise AuthExceptions.refreshTokenExpired() |
| if not parent: | ||
| ResourceNotFound(self.parent_resource.ref.collection, | ||
| self.parent_resource.ref.id) | ||
| ResourceNotFound( |
There was a problem hiding this comment.
Exception class is not being instantiated properly. Should be raise ResourceNotFound(...) to properly raise the exception.
Signed-off-by: Michael Pisman <mpisman@ucmerced.edu>
This pull request introduces several updates to the authentication flow and code structure, focusing on improving token refresh functionality, refining exception handling, and enhancing code readability. The changes include modifications to the
refresh_token_with_clientIDfunction, updates to the authentication route, and various formatting improvements in the documents module.Authentication Flow Updates:
refresh_token_with_clientIDto acceptrefresh_tokenas a direct parameter instead of parsing it from the body, simplifying the function's input handling (src/unipoll_api/actions/authentication.py).InvalidAccessToken,InvalidClientID, andrefreshTokenExpiredto ensure proper exception instantiation (src/unipoll_api/actions/authentication.py) [1] [2].Route Updates:
/jwt/postman_refreshendpoint to useFormparameters (refresh_tokenandgrant_type) instead of a JSON body, aligning with OAuth2 standards (src/unipoll_api/routes/authentication.py) [1] [2].authenticationschemas for better schema validation (src/unipoll_api/routes/authentication.py).Code Formatting and Readability:
src/unipoll_api/documents.py[1] [2] [3] [4] [5] [6] [7].src/unipoll_api/documents.pyandsrc/unipoll_api/routes/authentication.pyto follow PEP 8 import grouping standards [1] [2].