-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix for Issue 8701 #11759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix for Issue 8701 #11759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR adds functionality to create and manage a 'twenty' PostgreSQL user during Docker setup, along with corresponding documentation updates for manual permission management.
- Critical security issue: Hardcoded password 'twenty' in
/packages/twenty-docker/twenty/entrypoint.sh
needs to be replaced with environment variable - Syntax error in user existence check:
if [ ${PGPASS} psql...]
in/packages/twenty-docker/twenty/entrypoint.sh
is malformed - Incorrect variable usage:
TWENTYPASS
should bePGPASSWORD
in/packages/twenty-docker/twenty/entrypoint.sh
- Typo in success message: "Successfuly" should be "Successfully" in
/packages/twenty-docker/twenty/entrypoint.sh
💡 (4/5) You can add custom instructions or style guidelines for the bot here!
2 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @StormNinja17 and all ! Hope you're doing great
Unless I'm mistaken as it is script have several bugs ( also checking ci status )
Overall regarding the entrypoint.sh
updates, I don't think they can work at all.
In the first place if the user does not exist we won't be able to connect to the database the setup it. As by default the setup authentication is postgres:postgres
the user we wanna create here.
We should be logging recommendation related to suggested added documentation
@prastoin Thank you for the feedback! I switched all references of a "twenty" user to "postgres". Instead of attempting to grant all privileges, I modified the script to attempt to make a superuser instead. I also consolidated the troubleshooting documentation like you requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @StormNinja17, unfortunately as said above i don't think this can work.
The postgres
user does not exist occurs because that's the docker-compose.yml
default database credentials. This means that we could transpile your code as:
PGPASSWORD=postgres psql -h whatever -p 5432 -U postgres -d postgres -tc "SELECT 1 FROM pg_roles WHERE rolname='postgres'"
Is paradoxical
What we should do is trying to perform a very basic query using the provided credentials, such as a SELECT 1
. And log if we couldn't telling the user that the provided creds are wrong
Rebased on lastest |
We added some basic code that checks if a Twenty user exists in the Postgre database. If the user doesn't exist, we create it and try to give it necessary permissions. If we are unable to give it the necessary permissions, we log that issue to the console and redirect users to the Twenty documentation. We updated the documentation with a command users can run to manually grant permissions to the new Twenty user.
Close #8701