chore: Add grants for appsmith user for embedded postgres #36664
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
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
Bug
abhvsn
added
ok-to-test
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11148781575. |
|
Deploy-Preview-URL: https://ce-36664.dp.appsmith.com |
abhvsn
added
ok-to-test
![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 (4)
deploy/docker/fs/opt/appsmith/pg-utils.sh (4)
3-9: Well done, class! Let's add a small improvement.Good job setting up default values for our database connection parameters. This will make our script more flexible and easier to configure. However, let's make a small enhancement to improve clarity.
Consider adding a comment to explain the purpose of the
postgres_admin_uservariable. For example:DB_NAME="appsmith" +# User with administrative privileges for PostgreSQL operations postgres_admin_user="postgres"This will help other developers understand the role of this variable in our script. Remember, clear documentation is key to maintaining good code!
130-131: Excellent addition, but let's make it more consistent!I'm pleased to see you've added the
grant_permissions_for_schemafunction call. This is a crucial step in ensuring our database user has the necessary permissions. However, let's make a small adjustment for consistency.Instead of hardcoding the schema name, let's use the
DB_SCHEMAvariable we defined earlier. This will make our code more maintainable and less prone to errors. Here's how we can improve it:- USER=$PG_DB_USER SCHEMA="appsmith" DB=$PG_DB_NAME HOST=$PG_DB_HOST PORT=$PG_DB_PORT grant_permissions_for_schema + USER=$PG_DB_USER SCHEMA=$DB_SCHEMA DB=$PG_DB_NAME HOST=$PG_DB_HOST PORT=$PG_DB_PORT grant_permissions_for_schemaRemember, consistency is key in programming. By using our predefined variables, we ensure that any future changes to the schema name only need to be made in one place.
148-166: Excellent work on the new function! Let's add a bit of error handling.I'm impressed with your
grant_permissions_for_schemafunction. The documentation is clear, and the use of local variables with defaults is a smart approach. You've covered all the necessary permissions for our application to function correctly.To make this function even more robust, let's add some basic error handling. This will help us identify and troubleshoot any issues that might occur during the permission granting process. Here's a suggestion:
grant_permissions_for_schema() { local user=${USER-$DB_USER} schema=${SCHEMA-$DB_SCHEMA} db=${DB-$DB_NAME} host=${HOST-$DB_HOST} port=${PORT-$DB_PORT} tlog "Granting permissions to user '${user}' on schema '$schema' in database '$db' on host '$host' and port '$port'..." - psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "GRANT ALL PRIVILEGES ON SCHEMA ${schema} TO ${user};" - psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA ${schema} TO ${user};" - psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "ALTER DEFAULT PRIVILEGES IN SCHEMA ${schema} GRANT ALL PRIVILEGES ON TABLES TO ${user};" - psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "GRANT CONNECT ON DATABASE ${db} TO ${user};" + if ! psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "GRANT ALL PRIVILEGES ON SCHEMA ${schema} TO ${user};" ; then + tlog "Error granting schema privileges to user '${user}'" + return 1 + fi + if ! psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA ${schema} TO ${user};" ; then + tlog "Error granting table privileges to user '${user}'" + return 1 + fi + if ! psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "ALTER DEFAULT PRIVILEGES IN SCHEMA ${schema} GRANT ALL PRIVILEGES ON TABLES TO ${user};" ; then + tlog "Error altering default privileges for user '${user}'" + return 1 + fi + if ! psql -h ${host} -p ${port} -U ${postgres_admin_user} -d ${db} -c "GRANT CONNECT ON DATABASE ${db} TO ${user};" ; then + tlog "Error granting connect privilege to user '${user}'" + return 1 + fi + tlog "Successfully granted all permissions to user '${user}'" }This modification will help us identify which specific permission grant failed, if any. It's always good practice to handle potential errors in our scripts. Keep up the great work!
171-172: Good job on adding examples! Let's make them even clearer.I appreciate your effort in providing example usage for our functions. This will be very helpful for other developers who might use this script in the future. Well done!
To make these examples even more clear and educational, let's add a brief comment explaining what each example does. Here's a suggestion:
-# init_pg_db -# USER="user" SCHEMA="schema" DB="db" HOST="host" PORT="port" grant_permissions_for_schema +# Initialize the PostgreSQL database: +# init_pg_db + +# Grant permissions for a specific user on a schema: +# USER="user" SCHEMA="schema" DB="db" HOST="host" PORT="port" grant_permissions_for_schemaRemember, good documentation is like a good lesson plan - it helps others understand and learn from our work. Keep up the excellent work in making our code more accessible!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/pg-utils.sh (3 hunks)
🧰 Additional context used
📓 Learnings (1)
deploy/docker/fs/opt/appsmith/pg-utils.sh (1)
Learnt from: abhvsn PR: appsmithorg/appsmith#36664 File: deploy/docker/fs/opt/appsmith/pg-utils.sh:130-131 Timestamp: 2024-10-03T02:38:50.045Z Learning: In `pg-utils.sh`, the schema is not part of the connection string and isn't available as an environment variable after `extract_postgres_db_params`.
| @@ -119,6 +127,8 @@ init_pg_db() { | |||
| echo "Schema 'appsmith' does not exist. Creating schema..." | |||
| psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "$PG_DB_NAME" -c "CREATE SCHEMA appsmith;" | |||
| fi | |||
|
|
|||
| USER=$PG_DB_USER SCHEMA="appsmith" DB=$PG_DB_NAME HOST=$PG_DB_HOST PORT=$PG_DB_PORT grant_permissions_for_schema | |||
There was a problem hiding this comment.
Why is the schema hardcoded here? Can you use the DB_SCHEMA here?
There was a problem hiding this comment.
Can be used, but kept it hardcoded as the other vars are exported env variables and don't wanted to mixup.
There was a problem hiding this comment.
If they can be modified then the concern is valid for the other fields as well like - user, database name
There was a problem hiding this comment.
Yeah that's why I'm using the env vars extracted from the other method.
Description
PR to add the necessary grants to
appsmithuser when user opts for Postgres embedded DB.fixes #36661
Automation
/test Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11155064003
Commit: 1fb82e5
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 03 Oct 2024 04:22:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements