Skip to content
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

create a new user after creating a fresh db #672

Merged
merged 9 commits into from
May 18, 2023
Merged

Conversation

Ahmad-Wahid
Copy link
Contributor

@Ahmad-Wahid Ahmad-Wahid commented May 6, 2023

Changes proposed

  • Whenever, a user wants to create a fresh database and a user, then run the following command make clean-db db_name=flexmeasures-db db_user=flexmeasures.
  • Here, db_name is required, while db_user is optional.
  • If db_user is provided then after creating this user will be connected to the db for granting basic privileges.

Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid added the documentation Improvements or additions to documentation label May 6, 2023
@Ahmad-Wahid Ahmad-Wahid requested a review from nhoening May 6, 2023 10:26
@Ahmad-Wahid Ahmad-Wahid self-assigned this May 6, 2023
@Ahmad-Wahid Ahmad-Wahid changed the title crate a new user after creating a fresh db create a new user after creating a fresh db May 6, 2023
@nhoening
Copy link
Contributor

nhoening commented May 6, 2023

This looks good!

However, it got me thinking that a) I believe I was partly wrong and an existing user will survive a db drop and b) we have never (also not in documentation/host/data.rst ) told developers who should own the database.

If it's okay for the database to be owned by the postgres user and that every user on the server can connect to the databse, then leaving away the user is fine.
See https://stackoverflow.com/questions/40745559/user-can-always-connect-to-the-database-postgres

I am sorry I didn't know this exactly earlier.

I would:

  • make the user parameter optional, it's still a neat way to add a user for connecting, but not required
  • add a comment in data.rst to the effect described above (db owned by postgres user, all users can access, which might not be what you'd do in production)

Another thought :
If we add the flexmeasures db upgrade command, the new database is actually ready to be used.
We could even tell devs in the documentation to just use this make target to create a database?

@Ahmad-Wahid
Copy link
Contributor Author

Ahmad-Wahid commented May 10, 2023

I have already used flexmeasures db upgrade after creating the database and adding extensions.
I will make the user an optional param.

Is it a good idea, if we provide a user then only this user will have access to the new DB? see this https://medium.com/@mohammedhammoud/postgresql-create-user-create-database-grant-privileges-access-aabb2507c0aa

@nhoening
Copy link
Contributor

I have already used flexmeasures db upgrade after creating the database and adding extensions. I will make the user an optional param.

Yes, please.

Is it a good idea, if we provide a user then only this user will have access to the new DB? see this https://medium.com/@mohammedhammoud/postgresql-create-user-create-database-grant-privileges-access-aabb2507c0aa

That looks good. What is the difference to making that user the owner of the new database in the createdb call?

@Ahmad-Wahid
Copy link
Contributor Author

well, we create databases by using the default postgres user and don't give the access to the user we have created. Let me know, If I have understood it wrong.

Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
@nhoening
Copy link
Contributor

nhoening commented May 11, 2023

I haven't actually said this anywhere in this PR, true. My idea is to make the user the owner of the database like described here:

Normally, the database user who executes this command becomes the owner of the new database. However, a different owner can be specified via the -O option, if the executing user has appropriate privileges.

But just adding privileges for this new user is fine as well. Maybe we can not do all, but keep it to the ones needed for using it (e.g. read, update, enter, delete ...), but not the privilege to drop the database.

@Ahmad-Wahid
Copy link
Contributor Author

After creating DB and the user, I would take these steps.

  1. First connect user to the DB.
    GRANT CONNECT ON DATABASE database TO user
  2. Then grant the needed privileges as,
    GRANT USAGE, SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA public TO user
  3. Lastly, the user would not be allowed to drop the DB.
    REVOKE DROP ON DATABASE database FROM user

Let me know, if something is missing. @nhoening

@nhoening
Copy link
Contributor

Makes sense.
Not sure why you are revoking DROP, as it has not been granted. Just to make sure?

@Ahmad-Wahid
Copy link
Contributor Author

Ahmad-Wahid commented May 12, 2023

I thought, user can DROP the db by default. I will remove this step.

Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid requested review from nhoening and removed request for nhoening May 13, 2023 10:47
@Ahmad-Wahid
Copy link
Contributor Author

@nhoening Please review these new changes.

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, after I tested a bit I have some more ideas to make this work well.

flexmeasures/data/scripts/clean_database.sh Outdated Show resolved Hide resolved
flexmeasures/data/scripts/clean_database.sh Outdated Show resolved Hide resolved
flexmeasures/data/scripts/clean_database.sh Outdated Show resolved Hide resolved
nhoening added a commit that referenced this pull request May 13, 2023
Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid requested a review from nhoening May 16, 2023 07:31
Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
@Ahmad-Wahid
Copy link
Contributor Author

Right now, the extensions are created by the default postgres user. Should we create them by the newly created user.

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great step forward.
I tried it out and ran into a small issue that should be easy to fix.

flexmeasures/data/scripts/clean_database.sh Outdated Show resolved Hide resolved
flexmeasures/data/scripts/clean_database.sh Outdated Show resolved Hide resolved
flexmeasures/data/scripts/clean_database.sh Outdated Show resolved Hide resolved
@nhoening
Copy link
Contributor

Right now, the extensions are created by the default postgres user. Should we create them by the newly created user.

I don't think that matters.

Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks really good, but I ran into a small problem. The database was dropped, but not created. Only when I ran the script once more. See trace below, but I already indicated the potential cause in the code.

(fm-3.10) nicolas at pop-os in ~/workspace/seita/flexmeasures on docs-tutorial-improvement (2 untracked)
± make clean-db db_name=flexmeasures_test db_user=flexmeasures_test
./flexmeasures/data/scripts/clean_database.sh flexmeasures_test flexmeasures_test
[sudo] password for nicolas: 
flexmeasures_test database exists
Make a backup first? [y/N] 
This will drop your database and re-create a clean one. Continue?[y/N] y
Dropping database ...
flexmeasures_test database is dropped
(fm-3.10) nicolas at pop-os in ~/workspace/seita/flexmeasures on docs-tutorial-improvement (2 untracked)
± make clean-db db_name=flexmeasures_test db_user=flexmeasures_test
./flexmeasures/data/scripts/clean_database.sh flexmeasures_test flexmeasures_test
flexmeasures_test database does not exist
Creating a new database ...
flexmeasures_test database is created
User flexmeasures_test is already available.
Connect flexmeasures_test to flexmeasures_test 
GRANT
Grant required privileges
GRANT
Creating cube extension in flexmeasures_test ...
You are now connected to database "flexmeasures_test" as user "postgres".
CREATE EXTENSION
Creating earthdistance extension in flexmeasures_test ...
You are now connected to database "flexmeasures_test" as user "postgres".
CREATE EXTENSION
Updating database structure ...

flexmeasures/data/scripts/clean_database.sh Outdated Show resolved Hide resolved
Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid requested a review from nhoening May 17, 2023 12:44
Signed-off-by: Ahmad <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid requested a review from nhoening May 18, 2023 05:17
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I think this is a good example for bash scripting, which we can look at if we write more of them - very structured :)

@nhoening nhoening merged commit aecb395 into main May 18, 2023
7 checks passed
@nhoening nhoening deleted the docs-tutorial-improvement branch May 18, 2023 13:31
nhoening added a commit that referenced this pull request May 19, 2023
…676)

* docs: improve developer tutorial on how to run tests, incl. coverage

* add a docker command for running a database to test against

* exclude jinja templates from coverage, see nedbat/coveragepy#1553

* use the new make command from PR #672

* correct syntax type for docker call

* add dollar signs

* small text improvements
@Flix6x Flix6x added this to the 0.14.0 milestone Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Still Needs Changelog Entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants