ci: Only allow appsmith.com emails to signup on DPs#37858
ci: Only allow appsmith.com emails to signup on DPs#37858
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
skip-changelog
![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 and nitpick comments (1)
scripts/deploy_preview.sh (1)
Line range hint
103-107: Consider moving sensitive configuration to secrets.The script contains hardcoded sensitive values like Sentry DSN and database URLs. Consider:
- Moving these to Kubernetes secrets
- Using environment variables or external configuration management
Example approach using Kubernetes secrets:
- --set applicationConfig.APPSMITH_SENTRY_DSN="https://abf15a075d1347969df44c746cca7eaa@o296332.ingest.sentry.io/1546547" \ + --set applicationConfig.APPSMITH_SENTRY_DSN="$(kubectl get secret appsmith-sentry -n "$NAMESPACE" -o jsonpath='{.data.dsn}' | base64 -d)" \
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/deploy_preview.sh(1 hunks)
🔇 Additional comments (2)
scripts/deploy_preview.sh (2)
103-103: LGTM: Domain restriction implementation aligns with PR objectives.
The addition of APPSMITH_SIGNUP_ALLOWED_DOMAINS=appsmith.com correctly implements the requirement to restrict signups to appsmith.com email addresses.
Line range hint 103-107: Verify the impact on existing deployments.
The domain restriction might affect existing preview deployments. Please ensure:
- Existing users can still access their accounts
- Clear error messages are shown to non-appsmith.com users
✅ Verification successful
Domain restriction is specific to preview deployments and won't affect existing users
The domain restriction (APPSMITH_SIGNUP_ALLOWED_DOMAINS=appsmith.com) is being set only in the preview deployment script and appears to be a new security measure for preview environments. Since this is isolated to preview deployments:
- It's not affecting production or other environments
- It's applied only to new preview deployments
- The configuration is properly set through Helm charts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing preview deployments
kubectl get ns -l "heritage=Helm,chart=appsmith" --no-headers | wc -l
# List any existing users in preview deployments (if accessible)
kubectl get pods -n "$NAMESPACE" -l "app.kubernetes.io/name=appsmith" -o jsonpath='{.items[*].metadata.name}'
Length of output: 367
Script:
#!/bin/bash
# Let's check the deployment script for any existing domain restrictions or user access configurations
rg -A 5 -B 5 "APPSMITH_SIGNUP_ALLOWED_DOMAINS" scripts/
# Check for any related configuration or environment files
fd -e env -e yaml -e yml -e json . | xargs rg "SIGNUP_ALLOWED_DOMAINS|signup.allowed"
# Look for any authentication-related code that might handle domain restrictions
ast-grep --pattern 'function $FUNC($$$) {
$$$
domain$$$
$$$
}'
Length of output: 1602
|
@sharat87 What's the rationale behind this change? |
|
@sharat87 Can you help me understand the rationale behind this change? |
|
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. |
|
Hey, @mohanarpit, sorry, seems I've missed this twice! The rationale is that DPs are an internal detail, and internal environments. With the POV of secure by default, and considering the expectation for such internal DPs is for the use of internal engineers, I'm setting this up. In fact, I wanted to setup email validation also to be enabled by default, but that felt like too much just right now. What say? |
|
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. |
2 participants
If we need someone without such an email to signup, let someone with such an email signup as superuser first, then they can add more admin users in Admin Settings.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
appsmith.com).Chores