chore: Code split for logout success handler #39296
Conversation
WalkthroughThe pull request revises the logout success handling in Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LS as LogoutSuccessHandler
participant GC as URI Generator
participant OS as OAuth Session Manager
U->>LS: Initiate logout
LS->>LS: Build ResponseDTO & serialize JSON
LS->>GC: generatePostLogoutRedirectUri()
GC-->>LS: Return redirect URI
LS->>OS: clearOAuthSessionIfRequired() if needed
OS-->>LS: Session cleared
LS-->>U: Send response with redirect URI
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)`app/client/cypress/**/**.*`: Review the following e2e test ...
🔇 Additional comments (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
github-actions
bot
added
Enhancement
![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
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/LogoutSuccessHandlerCE.java (3)
48-61: Consider refactoring duplicated buffer allocation logic.The buffer allocation code is duplicated in both the error handling and success paths. Consider extracting this into a helper method.
+ private DataBuffer createResponseBuffer(ServerWebExchange exchange, String responseStr) { + return exchange.getResponse() + .bufferFactory() + .allocateBuffer(responseStr.length()) + .write(responseStr.getBytes()); + } @Override public Mono<Void> onLogoutSuccess(WebFilterExchange webFilterExchange, Authentication authentication) { // ... existing code ... try { responseStr = objectMapper.writeValueAsString(responseBody); } catch (JsonProcessingException e) { log.error("Unable to write to response json. Cause: ", e); responseStr = "{\"responseMeta\":{\"status\":500,\"success\":false},\"data\":false}"; - DataBuffer buffer = exchange.getResponse() - .bufferFactory() - .allocateBuffer(responseStr.length()) - .write(responseStr.getBytes()); + DataBuffer buffer = createResponseBuffer(exchange, responseStr); return response.writeWith(Mono.just(buffer)); } - DataBuffer buffer = exchange.getResponse() - .bufferFactory() - .allocateBuffer(responseStr.length()) - .write(responseStr.getBytes()); + DataBuffer buffer = createResponseBuffer(exchange, responseStr);Also applies to: 63-66
80-98: LGTM! Well-structured URI handling methods.The URI manipulation logic is robust with proper null checks. Consider adding validation for port number handling in
postLogoutRedirectUri.protected String postLogoutRedirectUri(ServerHttpRequest request) { UriComponents uriComponents = getUriComponents(request); String scheme = uriComponents.getScheme(); String host = uriComponents.getHost(); + int port = uriComponents.getPort(); return UriComponentsBuilder.newInstance() .scheme((scheme != null) ? scheme : "") .host((host != null) ? host : "") + .port(port == -1 ? null : port) .path(this.getPostLogoutRedirectUri()) .build() .toUriString(); }
100-102: Add documentation for OAuth session clearing hook.This template method would benefit from JavaDoc explaining its purpose and when subclasses should override it.
+ /** + * Hook method for clearing OAuth session data if required. + * Subclasses should override this method to implement OAuth-specific cleanup. + * + * @param logoutRedirectUri the URI to redirect to after logout + * @return Mono<Void> completing when cleanup is done + */ protected Mono<Void> clearOAuthSessionIfRequired(String logoutRedirectUri) { return Mono.empty(); }
📜 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/authentication/handlers/ce/LogoutSuccessHandlerCE.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/LogoutSuccessHandlerCE.java (1)
32-33: LGTM! Well-structured field declaration.The protected field with restricted getter access aligns with good encapsulation practices.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13328114565. |
|
Deploy-Preview-URL: https://ce-39296.dp.appsmith.com |
albinAppsmith
requested review from
ApekshaBhosale,
sagar-qa007,
a team and
KelvinOm
as code owners
albinAppsmith
force-pushed
the
logout-redirect-uri
branch
from
bf4ad89 to
8688f88
Compare
albinAppsmith
removed request for
a team,
ApekshaBhosale,
KelvinOm and
sagar-qa007
github-actions
bot
added
Enhancement
## Description ## Description Added redirect URL on user logout Fixes #38933 ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13369328042> > Commit: 5b4bbe9 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13369328042&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Mon, 17 Feb 2025 12:14:39 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Users are now automatically redirected to the login page after logout for a smoother navigation experience. - **Refactor** - Simplified the logout request process to consistently use the "POST" method, enhancing reliability. - Streamlined the interception logic for logout API calls, improving overall consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Albin <[email protected]> (cherry picked from commit e4ed590)
Description
Description
Added redirect URL on user logout
Fixes #38933
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13369328042
Commit: 5b4bbe9
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 17 Feb 2025 12:14:39 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit