db-api #10

Merged
merenber merged 22 commits from db-api into v1 2021-08-29 13:08:36 -04:00
Contributor

May need to install libpq-dev and/or postgresql-server-dev-11 for psycopg2

May need to install libpq-dev and/or postgresql-server-dev-11 for psycopg2
a268wang added 8 commits 2021-08-25 21:50:46 -04:00
continuous-integration/drone/push Build is failing Details
bb7539dcb6
wip: db api endpoints
continuous-integration/drone/push Build is failing Details
10f7aab3ed
cleaning up
continuous-integration/drone/push Build is failing Details
af87d6a7e6
fixes to logic
continuous-integration/drone/push Build is failing Details
2b41423a88
add delete_db for testing
continuous-integration/drone/push Build is failing Details
90818e5b1f
fix
continuous-integration/drone/push Build is failing Details
5f74302b17
fix
continuous-integration/drone/pr Build is failing Details
dc4d60fba2
testing
merenber requested changes 2021-08-25 23:14:07 -04:00
Dismissed
merenber left a comment
Owner

Excellent work. Just a few minor adjustments, and unit tests.

Excellent work. Just a few minor adjustments, and unit tests.
README.md Outdated
@ -37,0 +49,4 @@
REVOKE ALL ON SCHEMA public FROM public;
GRANT ALL ON SCHEMA public TO postgres;
```
create a new `pg_hba.conf` to force password authentication and reject non local
Owner

I think it would be useful for CSC members to access their PostgreSQL database from any CSC machine. (We should also do the same for MySQL at some point).

I think it would be useful for CSC members to access their PostgreSQL database from any CSC machine. (We should also do the same for MySQL at some point).
Author
Contributor

Will it be fine if I just change the postgres config to allow connections from any host?

Will it be fine if I just change the postgres config to allow connections from any host?
Owner

I think so.
For MySQL, I think it should be possible to allow connections for users from a specific subnet: https://stackoverflow.com/questions/11742963/how-to-grant-remote-access-to-mysql-for-a-whole-subnet/38389851#38389851

I think so. For MySQL, I think it should be possible to allow connections for users from a specific subnet: https://stackoverflow.com/questions/11742963/how-to-grant-remote-access-to-mysql-for-a-whole-subnet/38389851#38389851
Author
Contributor

I've changed both mysql and postgres to allow connections from any source.

Is there a subnet that you want me to use instead?

I've changed both mysql and postgres to allow connections from any source. Is there a subnet that you want me to use instead?
Owner

129.97.134.0/24, 2620:101:f000:4901::/64 (MC VLAN 134)
This should be configurable from ceod.ini.
For the dev environment, use 192.168.100.0/24.

129.97.134.0/24, 2620:101:f000:4901::/64 (MC VLAN 134) This should be configurable from ceod.ini. For the dev environment, use 192.168.100.0/24.
Author
Contributor

Hmm now that I look into it I think filtering by subnet is a bit iffy.

  • mysql's config will be in pyceo while psql's subnet config must be set in pg_hba.conf
  • mysql does not accept CIDR subnet notation (need to use 192.168.100.% or 192.168.100.0/255.255.255.0)
  • need to add extra condition for localhost and 127.0.0.1 (overwise MySQL will try to create the same user twice)
Hmm now that I look into it I think filtering by subnet is a bit iffy. - mysql's config will be in pyceo while psql's subnet config must be set in `pg_hba.conf` - mysql does not accept CIDR subnet notation (need to use `192.168.100.%` or `192.168.100.0/255.255.255.0`) - need to add extra condition for localhost and `127.0.0.1` (overwise MySQL will try to create the same user twice)
Owner

Alright, in that case, don't bother. Just let members connect from any IP address.

Alright, in that case, don't bother. Just let members connect from any IP address.
a268wang marked this conversation as resolved
@ -0,0 +13,4 @@
def create_db_from_type(db_type: str, username: str):
try:
if not username.isalnum(): # username should not contain symbols
raise UserNotFoundError
Owner

I don't think this is the right type of error to raise here. Should probably be something like 'InvalidUsername'.
Also, you want to raise an instance of the Exception class, not the class itself:

raise InvalidUsername()
I don't think this is the right type of error to raise here. Should probably be something like 'InvalidUsername'. Also, you want to raise an instance of the Exception class, not the class itself: ```python raise InvalidUsername() ```
a268wang marked this conversation as resolved
@ -0,0 +44,4 @@
except DatabasePermissionError:
return {'error': 'unable to perform action due to permissions'}, 502
except:
return {'error': 'Unexpected error'}, 500
Owner

You don't need this last case - unhandled exceptions are handled in error_handlers.py.

You don't need this last case - unhandled exceptions are handled in error_handlers.py.
a268wang marked this conversation as resolved
@ -0,0 +11,4 @@
@implementer(IDatabaseService)
class MySQLService:
def __init__(self):
self.type = 'mysql'
Owner

I suggest creating a class variable, like so:

class MySQLService:
    
    type = 'mysql'
    
    def __init__(self):
    	...
I suggest creating a class variable, like so: ```python class MySQLService: type = 'mysql' def __init__(self): ... ```
a268wang marked this conversation as resolved
@ -0,0 +20,4 @@
password = gen_password()
search_for_user = f"SELECT user FROM mysql.user WHERE user='{username}'"
search_for_db = f"SHOW DATABASES LIKE '{username}'"
create_user = f"CREATE USER '{username}'@'localhost' IDENTIFIED BY %(password)s"
Owner

I think we should allow CSC members to access their database from any CSC machine.

I think we should allow CSC members to access their database from any CSC machine.
a268wang marked this conversation as resolved
@ -0,0 +36,4 @@
if response_is_empty(search_for_user, con):
cursor.execute(create_user, {'password': password})
else:
cursor.execute(reset_password, {'password': password})
Owner

I'm not sure if this is intuitive behaviour - if the user wants to reset their database password, there should be a separate endpoint for that.
Maybe something like POST /api/db/mysql/<username>/pwreset.

I'm not sure if this is intuitive behaviour - if the user wants to reset their database password, there should be a separate endpoint for that. Maybe something like `POST /api/db/mysql/<username>/pwreset`.
Owner

On second thought, I don't think it's necessary to implement password reset behaviour, since the old ceo didn't have it.
The password will be written to a file in their home directory anyways, so they don't really have an excuse to lose it.

On second thought, I don't think it's necessary to implement password reset behaviour, since the old ceo didn't have it. The password will be written to a file in their home directory anyways, so they don't really have an excuse to lose it.
Author
Contributor

The old ceo would reset the database password if you already had a database and tried to create a mysql database.
I was kind of basing it off that when I wrote this.

The old ceo would reset the database password if you already had a database and tried to create a mysql database. I was kind of basing it off that when I wrote this.
Owner

I see. I still think this is non-intuitive behaviour, to be honest. If I clicked a button titled "create database", and it reset my DB password instead, I would be pretty confused.
I think we should just return an error here. If we really want to, we can add an extra endpoint for resetting DB passwords, but this should happen very rarely. Up to you.

I see. I still think this is non-intuitive behaviour, to be honest. If I clicked a button titled "create database", and it reset my DB password instead, I would be pretty confused. I think we should just return an error here. If we really want to, we can add an extra endpoint for resetting DB passwords, but this should happen very rarely. Up to you.
Author
Contributor

Ok I'll return a UserAlreadyExistsError (as in mysql/psql user already exists) and add the password reset endpoint since I wrote most of the code already.

Ok I'll return a UserAlreadyExistsError (as in mysql/psql user already exists) and add the password reset endpoint since I wrote most of the code already.
a268wang marked this conversation as resolved
@ -0,0 +38,4 @@
else:
cursor.execute(reset_password, {'password': password})
if response_is_empty(search_for_db, con):
cursor.execute(create_database)
Owner

If the user already exists but somehow their database doesn't, we should return an error.

If the user already exists but somehow their database doesn't, we should return an error.
Author
Contributor

I think it would be better if it just did nothing because it is possible for the mysql users to drop their database (and recreate it themselves)

But for postgresql if someone somehow dropped their database they wouldn't be able to even login

I think it would be better if it just did nothing because it is possible for the mysql users to drop their database (and recreate it themselves) But for postgresql if someone somehow dropped their database they wouldn't be able to even login
Owner

Sounds good.

Sounds good.
a268wang marked this conversation as resolved
@ -0,0 +56,4 @@
) as con:
with con.cursor() as cursor:
cursor.execute(drop_user)
cursor.execute(drop_db)
Owner

Not that it matters, but doesn't it make more sense to drop the database before dropping the user? Since the user logically "owns" the database?

Not that it matters, but doesn't it make more sense to drop the database before dropping the user? Since the user logically "owns" the database?
a268wang marked this conversation as resolved
@ -0,0 +37,4 @@
if response_is_empty(search_for_user, con):
cursor.execute(create_user, {'password': password})
else:
cursor.execute(reset_password, {'password': password})
Owner

Same comments as for MySQL.

Same comments as for MySQL.
a268wang marked this conversation as resolved
merenber added 1 commit 2021-08-25 23:21:32 -04:00
continuous-integration/drone/pr Build is failing Details
fce58cebee
Merge branch 'v1' into db-api
merenber added 1 commit 2021-08-25 23:24:46 -04:00
continuous-integration/drone/pr Build is failing Details
57180df040
add libpq-dev as dependency
a268wang added 2 commits 2021-08-26 02:03:14 -04:00
a268wang added 1 commit 2021-08-26 15:42:23 -04:00
continuous-integration/drone/pr Build is failing Details
ef3d130f78
add pwreset endpoint
a268wang added 1 commit 2021-08-26 16:45:29 -04:00
continuous-integration/drone/pr Build is failing Details
29305168c3
allow db users to login remotely
a268wang added 1 commit 2021-08-27 21:01:45 -04:00
continuous-integration/drone/pr Build is failing Details
d6dbfd5d3f
add tests
Author
Contributor

I've haven't added unit tests because the api maps pretty directly to the methods but I'm not sure if they work since the build has always been failing on this branch.

Also do I need to add tests to check for handling of invalid configs?

I've haven't added unit tests because the api maps pretty directly to the methods but I'm not sure if they work since the build has always been failing on this branch. Also do I need to add tests to check for handling of invalid configs?
Owner

I've haven't added unit tests because the api maps pretty directly to the methods but I'm not sure if they work since the build has always been failing on this branch.

We'll need to add a service container to Drone for the database. I'll try to take care of it this weekend.
Also, you appear to be failing the lint stage: https://ci.csclub.uwaterloo.ca/public/pyceo/41/1/3

Also do I need to add tests to check for handling of invalid configs?

No, I don't think so. Although it would be a good idea to check for an invalid config at startup time.

> I've haven't added unit tests because the api maps pretty directly to the methods but I'm not sure if they work since the build has always been failing on this branch. We'll need to add a service container to Drone for the database. I'll try to take care of it this weekend. Also, you appear to be failing the lint stage: https://ci.csclub.uwaterloo.ca/public/pyceo/41/1/3 > Also do I need to add tests to check for handling of invalid configs? No, I don't think so. Although it would be a good idea to check for an invalid config at startup time.
a268wang added 1 commit 2021-08-27 23:01:41 -04:00
continuous-integration/drone/pr Build is failing Details
562c6aca96
fixes
a268wang closed this pull request 2021-08-27 23:37:07 -04:00
a268wang reopened this pull request 2021-08-27 23:37:11 -04:00
a268wang added 1 commit 2021-08-28 00:02:02 -04:00
continuous-integration/drone/pr Build is failing Details
409894a07d
check db config on startup
merenber added 2 commits 2021-08-29 12:34:47 -04:00
Owner

@a268wang since we can't guarantee that caffeine will start up first on the real CSC machines, I decided to take out the initial DB check at startup. Sorry for making you do extra work.

@a268wang since we can't guarantee that caffeine will start up first on the real CSC machines, I decided to take out the initial DB check at startup. Sorry for making you do extra work.
merenber added 1 commit 2021-08-29 12:38:28 -04:00
continuous-integration/drone/pr Build is passing Details
56a80a7567
fix lint errors
merenber added 2 commits 2021-08-29 12:59:04 -04:00
merenber dismissed merenber’s review 2021-08-29 13:07:26 -04:00
merenber approved these changes 2021-08-29 13:08:08 -04:00
merenber merged commit eb5d632606 into v1 2021-08-29 13:08:36 -04:00
merenber referenced this issue from a commit 2021-08-29 13:08:36 -04:00
merenber deleted branch db-api 2021-08-29 13:08:48 -04:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#10
No description provided.