feat: Validate token api #19
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded@amaziahub has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis update introduces token-based authentication to both the service endpoints and the test infrastructure. The service router now requires a bearer token for POST and GET entry endpoints, using a new dependency to extract and verify the token. Correspondingly, the test support classes are updated to handle authentication: the test driver logs in to obtain a token, and the client methods for creating and retrieving entries are modified to include the token in the Authorization header. These changes ensure that all entry-related API interactions are authenticated. Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
service/entries/router.py(1 hunks)tests/support/app_driver.py(3 hunks)tests/support/client.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/support/app_driver.py (3)
tests/support/client.py (3)
login(72-78)create_entry(38-43)get_entries(45-49)service/login/router.py (1)
login(26-30)service/entries/router.py (2)
create_entry(22-29)get_entries(33-37)
tests/support/client.py (5)
tests/support/app_driver.py (3)
is_healthy(34-35)create_entry(37-38)get_entries(40-41)service/login/token.py (1)
verify(47-55)service/entries/router.py (2)
create_entry(22-29)get_entries(33-37)service/entries/model/entry.py (2)
Entry(6-15)from_json(11-12)service/entries/persistence/entries_store.py (1)
get_entries(32-34)
🪛 Ruff (0.8.2)
service/entries/router.py
17-17: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
17-17: Do not perform function call HTTPBearer in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (9)
tests/support/client.py (3)
38-43: LGTM: Authentication support for create_entryThe addition of the token parameter and authorization header to the create_entry method correctly implements the authentication requirements for this endpoint.
45-49: LGTM: Authentication support for get_entriesThe addition of the token parameter and authorization header to the get_entries method correctly implements the authentication requirements for this endpoint.
21-21: LGTM: Token parameter in constructorAdding an optional token parameter to the constructor provides flexibility in how the client is initialized and used, supporting both authenticated and unauthenticated scenarios.
tests/support/app_driver.py (3)
12-12: LGTM: Token instance variableAdding a token instance variable with initial value of None is a good practice for storing the authentication token after login.
37-38: LGTM: Token usage in create_entryThe method now correctly passes the token to the client's create_entry method, enabling authenticated requests.
40-41: LGTM: Token usage in get_entriesThe method now correctly passes the token to the client's get_entries method, enabling authenticated requests.
service/entries/router.py (3)
22-29: LGTM: Token verification in create_entry endpointThe create_entry endpoint now correctly extracts and verifies the access token before processing the request. The updated use of model_dump() instead of dict() also follows modern Pydantic practices.
32-37: LGTM: Token verification in get_entries endpointThe get_entries endpoint now correctly extracts and verifies the access token before returning entries, ensuring proper authentication.
4-6: LGTM: Security importsThe added imports for FastAPI security components and the token module are necessary and appropriate for implementing token-based authentication.
Also applies to: 10-10
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/entries/router.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
service/entries/router.py
18-18: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (6)
service/entries/router.py (6)
4-6: Appropriate imports added for authenticationThese imports correctly bring in the necessary FastAPI security components needed for token-based authentication.
10-10: Token module import added correctlyThe token module import is appropriately placed to support the new authentication functionality.
15-16: Good implementation of module-level security schemeThe bearer_scheme variable follows the recommended FastAPI pattern and addresses the static analysis warning about function calls in parameter defaults that was flagged in previous reviews.
18-19: Well-designed token extraction functionThis helper function cleanly extracts the token from the credentials object. The implementation is concise and follows FastAPI best practices.
🧰 Tools
🪛 Ruff (0.8.2)
18-18: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
24-25: Authentication dependency correctly added to endpointThe token extraction is properly integrated as a dependency for the create_entry endpoint.
34-37: Authentication consistently applied across endpointsAuthentication has been correctly implemented for the GET endpoint, maintaining consistency across the API. The token verification occurs before accessing the data, which is the proper security sequence.
However, consider adding logging here as well for consistency with the POST endpoint:
def get_entries( access_token: str = Depends(extract_token) ): + logger.debug(f"Verifying access token for get_entries request") token.verify(access_token) + logger.debug(f"Token verification successful") return persistence.get_entries()
service/entries/router.py
Outdated
| logger.info(f"Received entry data: {entry.model_dump()}") | ||
| token.verify(access_token) | ||
| entry = persistence.add_entry(entry.model_dump()) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify potential error handling for token verification
While the token verification and model_dump() usage are appropriate, consider adding error handling or logging around the token verification process to aid in troubleshooting authentication failures.
Consider expanding with:
logger.info(f"Received entry data: {entry.model_dump()}")
+ logger.debug(f"Verifying access token")
token.verify(access_token)
+ logger.debug(f"Token verification successful")
entry = persistence.add_entry(entry.model_dump())Run this to check if the token module already has sufficient error handling:
🏁 Script executed:
#!/bin/bash
# Check if the token.verify method has error handling
cat ../login/token.py | grep -A 10 "def verify"Length of output: 99
🏁 Script executed:
#!/bin/bash
# Locate any token.py files in the repository
echo "Searching for token.py files:"
find . -type f -name "token.py"
# Grep all Python files for verify method definitions
echo -e "\nSearching for 'def verify' in Python files:"
grep -R -n "def verify" --include '*.py' .Length of output: 326
🏁 Script executed:
#!/bin/bash
# Display the implementation of token.verify to assess error handling
grep -R -n "def verify" -A 20 service/login/token.pyLength of output: 320
Enhance logging around token verification
The token.verify method already catches JWT errors (expired or invalid tokens) and raises appropriate exceptions. To improve traceability during authentication failures, add debug‑level logs immediately before and after verification:
logger.info(f"Received entry data: {entry.model_dump()}")
+ logger.debug("Verifying access token")
token.verify(access_token)
+ logger.debug("Token verification successful")
entry = persistence.add_entry(entry.model_dump())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info(f"Received entry data: {entry.model_dump()}") | |
| token.verify(access_token) | |
| entry = persistence.add_entry(entry.model_dump()) | |
| logger.info(f"Received entry data: {entry.model_dump()}") | |
| logger.debug("Verifying access token") | |
| token.verify(access_token) | |
| logger.debug("Token verification successful") | |
| entry = persistence.add_entry(entry.model_dump()) |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
service/entries/router.py (1)
22-28: Implement previously suggested debug logging around token verificationThe token verification is correctly implemented, but the previously suggested debug logging isn't included. Adding these logs would improve traceability during authentication failures.
+ logger.debug("Verifying access token") token.verify(access_token) + logger.debug("Token verification successful") entry = persistence.add_entry(entry.model_dump())
🧹 Nitpick comments (1)
service/entries/router.py (1)
32-37: Consider making get_entries async for consistencyThe create_entry endpoint is async but get_entries is not. Consider making both endpoints async for consistency in the codebase.
-def get_entries( +async def get_entries( access_token: str = Depends(extract_token) ): token.verify(access_token) return persistence.get_entries()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/entries/router.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
service/entries/router.py (4)
service/entries/model/entry.py (2)
Entry(6-15)to_json(14-15)service/entries/persistence/entries_store.py (3)
EntriesStore(7-38)add_entry(24-30)get_entries(32-34)tests/support/client.py (3)
login(74-80)create_entry(38-43)get_entries(45-49)service/login/token.py (1)
verify(47-55)
🪛 Ruff (0.8.2)
service/entries/router.py
18-18: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (4)
service/entries/router.py (4)
15-19: Well-implemented token extraction dependencyThe implementation follows FastAPI best practices by:
- Creating a module-level security scheme variable
- Using a dedicated dependency function to extract tokens
This addresses the previous B008 lint warning by avoiding function calls in parameter defaults.
🧰 Tools
🪛 Ruff (0.8.2)
18-18: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
24-25: Proper authentication integration with the entry creation endpointThe implementation correctly requires authentication by adding the access_token dependency parameter.
28-28: Good use of model_dump() instead of dict()Using
model_dump()follows the recommended Pydantic approach for serializing models and matches theto_json()implementation in the Entry model.
33-36: Properly secured get_entries endpointThe implementation correctly requires and verifies authentication tokens before returning entries.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/support/app_driver.py (1)
28-28:⚠️ Potential issueHardcoded secret used for authentication
The secret "Creeper" is hardcoded in the source code, which is a security risk. Secrets should be managed through environment variables or secure configuration management.
Apply this change to improve security:
- self.token = self.login("Creeper")["access_token"] + test_secret = os.getenv("TEST_SECRET", "Creeper") # Fallback for local testing only + self.token = self.login(test_secret)["access_token"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
service/entries/router.py(1 hunks)tests/support/app_driver.py(3 hunks)tests/support/client.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/support/client.py (5)
tests/support/app_driver.py (3)
is_healthy(34-35)create_entry(37-38)get_entries(40-41)service/login/token.py (1)
verify(47-55)service/entries/router.py (2)
create_entry(23-29)get_entries(33-37)service/entries/model/entry.py (2)
Entry(6-15)from_json(11-12)service/entries/persistence/entries_store.py (1)
get_entries(32-34)
tests/support/app_driver.py (4)
service/login/router.py (1)
login(26-30)tests/support/client.py (3)
login(74-80)create_entry(38-43)get_entries(45-49)service/entries/router.py (2)
create_entry(23-29)get_entries(33-37)service/entries/persistence/entries_store.py (1)
get_entries(32-34)
🪛 Ruff (0.8.2)
service/entries/router.py
18-18: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (9)
tests/support/client.py (3)
51-57: Good implementation of the auth header builderThe implementation correctly handles the case when no token is provided by checking if the token exists before adding it to the headers. This prevents sending invalid
Bearer Noneheaders.
38-41: API authentication properly implementedThe method now correctly passes authentication headers when a token is provided. This aligns with the authentication changes in the endpoints.
45-47: API authentication properly implemented for GET methodThe implementation correctly includes the token in the authorization header when fetching entries, matching the authentication requirements in the service layer.
tests/support/app_driver.py (2)
37-38: Token properly passed to client methodsThe method now correctly forwards the authentication token to the client, ensuring that test requests are properly authenticated.
40-41: Token properly passed to get_entriesThe method now correctly forwards the authentication token to the client, ensuring that test requests are properly authenticated.
service/entries/router.py (4)
15-15: Good module-level security schemeCreating a module-level variable for the security scheme follows best practices and addresses the B008 issue from static analysis.
18-19: Well-implemented token extractionThe
extract_tokenfunction is a clean implementation to extract credentials from the authorization header.🧰 Tools
🪛 Ruff (0.8.2)
18-18: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
23-29: Token verification added to entry creationThe endpoint now properly verifies the token before processing the request. Also noticed a change from dictionary conversion to using the Pydantic
model_dump()method, which is the recommended approach.Consider adding logging around token verification for better troubleshooting:
+ logger.debug("Verifying access token") token.verify(access_token) + logger.debug("Token verification successful")
33-37: Token verification added to get entries endpointThe endpoint now properly requires and verifies an authentication token before returning entries, which improves security.
Consider adding logging here as well:
+ logger.debug("Verifying access token for get_entries") token.verify(access_token) + logger.debug("Token verification successful")
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Summary by CodeRabbit