Fix consolidated api unauthorized check #40702
Conversation
WalkthroughThe Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (1)
89-176: 💡 Verification agent🧩 Analysis chain
Verify test coverage for the authentication logic.
Ensure there are unit tests that verify:
- Requests with defaultPageId but no authentication return 401
- Requests with defaultPageId and valid authentication proceed normally
- Requests without defaultPageId proceed regardless of authentication status
The PR mentioned test failures but indicated they were due to environment issues rather than code problems.
🏁 Script executed:
#!/bin/bash # Search for test files that might cover this controller echo "Searching for test files related to ConsolidatedAPIController..." fd -t f ".*Test.*" | grep -i consolidated | grep -i api | grep -i controllerLength of output: 217
Add unit tests for
ConsolidatedAPIControllerauthentication logicI didn’t find any existing tests covering the new auth checks in
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java(lines 89–176). Please add tests for:
- Requests with a non-null
defaultPageIdbut no authenticated user → HTTP 401- Requests with a non-null
defaultPageIdand an authenticated user → HTTP 200 + response body- Requests when
defaultPageIdis null (regardless of auth) → HTTP 200 + response body
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (2)
116-121: Authentication enforcement implemented correctly.This correctly implements the requirement to return 401 Unauthorized when a defaultPageId is provided but the user is not authenticated.
Consider adding an error message to the response for better API usability:
-new ResponseDTO<>(HttpStatus.UNAUTHORIZED, null); +new ResponseDTO<>(HttpStatus.UNAUTHORIZED, null, "Authentication required to access page with ID " + defaultPageId);
113-170: Consider extracting methods to improve readability.The code structure has become deeply nested, which makes it harder to follow. Consider extracting the authentication logic and response generation into separate private methods to improve readability.
For example:
private Mono<ResponseEntity<ResponseDTO<ConsolidatedAPIResponseDTO>>> handleUnauthenticatedRequest() { ResponseDTO<ConsolidatedAPIResponseDTO> responseDTO = new ResponseDTO<>(HttpStatus.UNAUTHORIZED, null); return Mono.just(new ResponseEntity<>(responseDTO, HttpStatus.UNAUTHORIZED)); } private Mono<ResponseEntity<ResponseDTO<ConsolidatedAPIResponseDTO>>> fetchAndProcessPageData(...) { // Current implementation from lines 123-169 }This would make the main method more concise:
return isAuthenticatedMono .flatMap(isAuthenticated -> { if (StringUtils.hasLength(defaultPageId) && !isAuthenticated) { return handleUnauthenticatedRequest(); } return fetchAndProcessPageData(...); })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (3)
11-11: Appropriate import for authentication.The SessionUserService import is correctly added to support the authentication check implementation.
39-39: Constructor dependency injection looks good.Proper constructor injection for the SessionUserService dependency. This follows Spring's recommended dependency injection pattern.
Also applies to: 41-47
108-112: Well-implemented reactive authentication check.The authentication check is implemented properly using reactive programming patterns. The
getCurrentUser()followed by mapping to a boolean and providing a default value handles both authenticated and unauthenticated scenarios correctly.
There was a problem hiding this comment.
Pull Request Overview
This PR adds an authentication guard to the consolidated API endpoint to return a 401 when defaultPageId is provided but no user is authenticated.
- Injects
SessionUserServiceintoConsolidatedAPIController - Wraps the existing API call in a check that returns 401 for unauthenticated requests with a
defaultPageId - Preserves original ETag computation and response logic under authenticated or no-
defaultPageIdconditions
Comments suppressed due to low confidence (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java:116
- Add unit and integration tests covering the unauthorized scenario when a defaultPageId is provided but the user is unauthenticated, to verify a 401 status is returned.
if (StringUtils.hasLength(defaultPageId) && !isAuthenticated) {
| return isAuthenticatedMono | ||
| .flatMap( | ||
| isAuthenticated -> { | ||
| if (StringUtils.hasLength(defaultPageId) && !isAuthenticated) { |
There was a problem hiding this comment.
[nitpick] Consider using StringUtils.hasText(...) instead of hasLength to avoid treating strings with only whitespace as a valid defaultPageId.
| if (StringUtils.hasLength(defaultPageId) && !isAuthenticated) { | |
| if (StringUtils.hasText(defaultPageId) && !isAuthenticated) { |
Copilot uses AI. Check for mistakes.
| if (StringUtils.hasLength(defaultPageId) && !isAuthenticated) { | ||
| ResponseDTO<ConsolidatedAPIResponseDTO> responseDTO = | ||
| new ResponseDTO<>(HttpStatus.UNAUTHORIZED, null); | ||
| return Mono.just( | ||
| new ResponseEntity<>(responseDTO, HttpStatus.UNAUTHORIZED)); | ||
| } |
There was a problem hiding this comment.
[nitpick] The nested flatMap→map structure is quite deep; consider extracting the unauthorized check and main service call into helper methods to improve readability.
| if (StringUtils.hasLength(defaultPageId) && !isAuthenticated) { | |
| ResponseDTO<ConsolidatedAPIResponseDTO> responseDTO = | |
| new ResponseDTO<>(HttpStatus.UNAUTHORIZED, null); | |
| return Mono.just( | |
| new ResponseEntity<>(responseDTO, HttpStatus.UNAUTHORIZED)); | |
| } | |
| return handleUnauthorizedCheck(defaultPageId, isAuthenticated) | |
| .switchIfEmpty( | |
| handleConsolidatedServiceCall( | |
| defaultPageId, | |
| applicationId, | |
| refType, | |
| refName, | |
| ifNoneMatch)); |
Copilot uses AI. Check for mistakes.
| }) | ||
| Mono<Boolean> isAuthenticatedMono = sessionUserService | ||
| .getCurrentUser() | ||
| .map(user -> true) |
There was a problem hiding this comment.
This is incorrect.You would always be returned a user (either the logged in user, or a user object whose email is anonymousUser
There was a problem hiding this comment.
@trishaanand In that case can we send a 401 for anonymous users? We should able to distinguish an anonymous user from a logged in user right?
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Summary
/consolidated-api/viewis called with adefaultPageIdbut without authenticationTesting
mvn test(fails: mvn not found)Warning
Tests have not run on the HEAD 923e8b4 yet
Tue, 20 May 2025 07:14:48 UTC
Summary by CodeRabbit