chore: Remove CS URL in client #37665
Conversation
WalkthroughThe pull request introduces several modifications across various GitHub Actions workflows and application files. Key changes include enhancements to error handling and logging in CI workflows, the removal of the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@sharat87 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 40 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. 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
|
|
/build-deploy-preview skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11992867651. |
|
Deploy-Preview-URL: https://ce-37665.dp.appsmith.com |
…ith/server/controllers/SaasController.java
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12005167956. |
|
Deploy-Preview-URL: https://ce-37665.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12010633230. |
|
Deploy-Preview-URL: https://ce-37665.dp.appsmith.com |
sharat87
requested review from
mohanarpit,
nidhi-nair,
trishaanand and
sneha122
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
app/client/src/api/CloudServicesApi.ts (1)
1-2: LGTM! Good architectural improvement.The change properly moves URL management to the server side, improving security and maintainability by making the server the source of truth for service URLs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
.github/workflows/ci-test-custom-script.yml(2 hunks).github/workflows/ci-test-limited-with-count.yml(3 hunks).github/workflows/ci-test-limited.yml(0 hunks)app/client/docker/templates/nginx-app.conf.template(0 hunks)app/client/public/index.html(0 hunks)app/client/src/api/CloudServicesApi.ts(1 hunks)app/client/src/ce/configs/index.ts(0 hunks)app/client/src/ce/configs/types.ts(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/SaasController.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java(3 hunks)
💤 Files with no reviewable changes (5)
- .github/workflows/ci-test-limited.yml
- app/client/docker/templates/nginx-app.conf.template
- app/client/public/index.html
- app/client/src/ce/configs/index.ts
- app/client/src/ce/configs/types.ts
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/SaasController.java (1)
3-3: LGTM: Clean import addition
The CloudServicesConfig import follows standard conventions.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java (3)
5-5: LGTM! Clean dependency management.
The switch to @RequiredArgsConstructor and addition of required imports follows Spring best practices.
Also applies to: 12-12, 15-16, 28-28
33-33: LGTM! Proper dependency injection.
The addition of CloudServicesConfig as a final field aligns with the PR objective to manage CS URLs server-side.
68-78: Verify OAuth flow integration and security headers.
Please ensure:
- Error scenarios are properly handled in
AuthenticationService - Security headers are set appropriately for the redirect
- The OAuth flow works end-to-end with this change
✅ Verification successful
Let me gather more information about the OAuth flow integration.
Let me check the security configuration for this OAuth endpoint.
Let me check the OAuth flow implementation more thoroughly.
OAuth redirect implementation is secure and follows best practices
The implementation:
- Uses proper HTTP status code (307 Temporary Redirect)
- Sets Location header correctly for the redirect
- Is protected by Spring Security's filter chain
- Handles errors appropriately through the global error handlers
- Uses proper content security headers from the SecurityConfig
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper error handling in AuthenticationService
ast-grep --pattern 'class AuthenticationService {
$$$
getAppsmithToken($$$) {
$$$
}
$$$
}'
# Check for security headers configuration
rg -A 5 "SecurityWebFilterChain|WebSecurityConfigurerAdapter|SecurityFilterChain"
Length of output: 5906
Script:
#!/bin/bash
# Check OAuth related configurations and services
rg -A 5 "cloudServicesConfig|CloudServicesConfig"
# Check for OAuth related security configurations
rg -A 10 "oauth2" -i
# Check for any CORS configurations that might affect the redirect
rg -A 5 "CorsConfig|cors.allow"
Length of output: 108650
Script:
#!/bin/bash
# Check for security configurations in SaasController and related classes
ast-grep --pattern 'class SecurityConfig {
$$$
securityWebFilterChain($$$) {
$$$
}
$$$
}'
# Check for any CORS or security headers configuration
rg -A 10 "setStatusCode|setHeaders|getHeaders|ServerHttpResponse"
Length of output: 105158
Script:
#!/bin/bash
# Check for OAuth flow and security configurations
rg -A 5 "class SaasControllerCE|class SaasController"
# Check for any error handling in the OAuth flow
rg -A 10 "redirectForAuthorize|authorize"
# Check for any CORS or security configurations specific to this endpoint
rg -A 10 "WebFilter|SecurityWebFilterChain.*saas|/api/v1/saas"
Length of output: 100375
.github/workflows/ci-test-limited-with-count.yml (2)
20-22: LGTM: Parameter definition is clear and follows best practices
376-376: LGTM: Artifact upload configuration is well-defined
| public SaasController(AuthenticationService authenticationService, CloudServicesConfig cloudServicesConfig) { | ||
| super(authenticationService, cloudServicesConfig); |
There was a problem hiding this comment.
💡 Codebase verification
Constructor injection needs to be updated in SaasControllerCE
The parent class SaasControllerCE is using @RequiredArgsConstructor for field injection, but the child class SaasController is trying to use constructor injection. This mismatch needs to be addressed by either:
- Adding an explicit constructor in SaasControllerCE to match the child class, or
- Using
@RequiredArgsConstructorin the child class instead
🔗 Analysis chain
LGTM: Constructor properly handles new dependency
The constructor correctly injects and passes the CloudServicesConfig to the parent class, aligning with the goal of making the server the source of truth for CS URLs.
Let's verify the parent class is ready for this change:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SaasControllerCE has matching constructor signature
ast-grep --pattern 'class SaasControllerCE {
$$$
SaasControllerCE($_, CloudServicesConfig $_) {
$$$
}
$$$
}'
Length of output: 122
Script:
#!/bin/bash
# Let's try to find the parent class first
rg -l "SaasControllerCE" --type java
# Then examine its content
rg "class SaasControllerCE" -A 10 --type java
Length of output: 1762
Script:
#!/bin/bash
# Let's also check for any CloudServicesConfig usage in the parent class
rg "CloudServicesConfig" --type java -A 2
Length of output: 21203
Script:
#!/bin/bash
# Let's find the actual file path for SaasControllerCE
fd "SaasControllerCE.java"
Length of output: 124
Script:
#!/bin/bash
# Let's check the constructor in SaasControllerCE
cat app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java
Length of output: 3719
...erver/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java
Outdated
Show resolved
Hide resolved
…ontrollers/ce/SaasControllerCE.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java (1)
68-83: Consider additional security measuresWhile the basic implementation is sound, consider these security enhancements:
- Add more robust token validation (format, length)
- Add request logging for security auditing
- Consider rate limiting to prevent abuse
Here's a suggested enhancement:
@GetMapping("authorize") public Mono<Void> redirectForAuthorize(ServerWebExchange exchange, @RequestParam String appsmithToken) { + log.debug("Received authorization redirect request for token: {}", maskToken(appsmithToken)); + if (appsmithToken == null || appsmithToken.isEmpty()) { + log.warn("Received invalid token in authorization redirect"); exchange.getResponse().setStatusCode(HttpStatus.BAD_REQUEST); return exchange.getResponse().setComplete(); } + + // Add more robust token validation + if (!isValidToken(appsmithToken)) { + log.warn("Token validation failed in authorization redirect"); + exchange.getResponse().setStatusCode(HttpStatus.BAD_REQUEST); + return exchange.getResponse().setComplete(); + } final String url = cloudServicesConfig.getBaseUrl() + "/api/v1/integrations/oauth/authorize?appsmithToken=" + URLEncoder.encode(appsmithToken, StandardCharsets.UTF_8); ServerHttpResponse response = exchange.getResponse(); response.setStatusCode(HttpStatus.TEMPORARY_REDIRECT); response.getHeaders().set("Location", url); + log.debug("Redirecting to cloud services for authorization"); return response.setComplete(); } + +private boolean isValidToken(String token) { + return token.matches("^[a-zA-Z0-9-_]{8,}$"); +} + +private String maskToken(String token) { + if (token == null) return null; + return token.substring(0, Math.min(4, token.length())) + "***"; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java(3 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java (3)
5-5: LGTM - Import changes are appropriate
The new imports support the added functionality and Lombok improvements.
Also applies to: 12-12, 15-16
28-28: LGTM - Good use of Lombok
The @requiredargsconstructor annotation improves code maintainability by automatically generating a constructor for final fields.
33-33: LGTM - Proper dependency injection
The CloudServicesConfig dependency aligns with the PR objective of making the server the source of truth for CS URLs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java (1)
71-86: Consider additional security measuresWhile basic security measures are in place, consider:
- Add rate limiting to prevent abuse
- Validate token format (not just null/empty)
- Add security event logging for audit trails
Example logging addition:
@GetMapping("authorize") public Mono<Void> redirectForAuthorize(ServerWebExchange exchange, @RequestParam String appsmithToken) { if (appsmithToken == null || appsmithToken.isEmpty()) { + log.warn("Authorization attempt with invalid token"); exchange.getResponse().setStatusCode(HttpStatus.BAD_REQUEST); return exchange.getResponse().setComplete(); } + log.info("Processing authorization redirect for token: {}", appsmithToken.substring(0, 8) + "..."); final String url = cloudServicesConfig.getBaseUrl() + "/api/v1/integrations/oauth/authorize?appsmithToken=" + URLEncoder.encode(appsmithToken, StandardCharsets.UTF_8);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java(3 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/SaasControllerCE.java (2)
5-5: LGTM: Clean import additions
The new imports are appropriate for the added functionality.
Also applies to: 12-12, 15-16, 26-27
31-31: LGTM: Clean dependency injection
Good use of Lombok's @requiredargsconstructor and proper injection of CloudServicesConfig.
Also applies to: 36-36
The server should be the source of truth and owner for the current CS URL, and the client having direct access to the CS URL is (almost) an abstraction leak. We're using it on client for one purpose only, to redirect to CS for Google sheets authorization. That's just as well achieved with another redirect via the server. This PR does that redirection and removes access to the CS URL to the client code. Not used anywhere else, and shouldn't be needed. ## Automation /test sanity datasource ### 🔍 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/12029489682> > Commit: 0a1937e > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12029489682&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.Datasource` > Spec: > <hr>Tue, 26 Nov 2024 12:21:11 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 ## Release Notes - **New Features** - Enhanced CI workflow with improved error handling and logging. - Added a new authorization redirection endpoint in the SaaS controller. - **Improvements** - Database URL validation step added to CI tests. - Artifact management for test results has been clarified and improved. - Removal of `cloudServicesBaseUrl` from various configurations to streamline cloud service integration. - **Bug Fixes** - Refined logic for rerunning tests based on previous results. These updates contribute to a more robust and efficient testing and configuration environment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The server should be the source of truth and owner for the current CS URL, and the client having direct access to the CS URL is (almost) an abstraction leak. We're using it on client for one purpose only, to redirect to CS for Google sheets authorization. That's just as well achieved with another redirect via the server. This PR does that redirection and removes access to the CS URL to the client code. Not used anywhere else, and shouldn't be needed. ## Automation /test sanity datasource ### 🔍 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/12029489682> > Commit: 0a1937e > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12029489682&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.Datasource` > Spec: > <hr>Tue, 26 Nov 2024 12:21:11 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 ## Release Notes - **New Features** - Enhanced CI workflow with improved error handling and logging. - Added a new authorization redirection endpoint in the SaaS controller. - **Improvements** - Database URL validation step added to CI tests. - Artifact management for test results has been clarified and improved. - Removal of `cloudServicesBaseUrl` from various configurations to streamline cloud service integration. - **Bug Fixes** - Refined logic for rerunning tests based on previous results. These updates contribute to a more robust and efficient testing and configuration environment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The server should be the source of truth and owner for the current CS URL, and the client having direct access to the CS URL is (almost) an abstraction leak. We're using it on client for one purpose only, to redirect to CS for Google sheets authorization. That's just as well achieved with another redirect via the server.
This PR does that redirection and removes access to the CS URL to the client code. Not used anywhere else, and shouldn't be needed.
Automation
/test sanity datasource
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12029489682
Commit: 0a1937e
Cypress dashboard.
Tags:
@tag.Sanity, @tag.DatasourceSpec:
Tue, 26 Nov 2024 12:21:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
cloudServicesBaseUrlfrom various configurations to streamline cloud service integration.Bug Fixes
These updates contribute to a more robust and efficient testing and configuration environment.