db-api #10
No reviewers
Labels
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: public/pyceo#10
Loading…
Reference in New Issue
No description provided.
Delete Branch "db-api"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
May need to install libpq-dev and/or postgresql-server-dev-11 for psycopg2
Excellent work. Just a few minor adjustments, and unit tests.
@ -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
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).
Will it be fine if I just change the postgres config to allow connections from any host?
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've changed both mysql and postgres to allow connections from any source.
Is there a subnet that you want me to use instead?
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.
Hmm now that I look into it I think filtering by subnet is a bit iffy.
pg_hba.conf
192.168.100.%
or192.168.100.0/255.255.255.0
)127.0.0.1
(overwise MySQL will try to create the same user twice)Alright, in that case, don't bother. Just let members connect from any IP address.
@ -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
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:
@ -0,0 +44,4 @@
except DatabasePermissionError:
return {'error': 'unable to perform action due to permissions'}, 502
except:
return {'error': 'Unexpected error'}, 500
You don't need this last case - unhandled exceptions are handled in error_handlers.py.
@ -0,0 +11,4 @@
@implementer(IDatabaseService)
class MySQLService:
def __init__(self):
self.type = 'mysql'
I suggest creating a class variable, like so:
@ -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"
I think we should allow CSC members to access their database from any CSC machine.
@ -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})
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
.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.
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.
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.
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.
@ -0,0 +38,4 @@
else:
cursor.execute(reset_password, {'password': password})
if response_is_empty(search_for_db, con):
cursor.execute(create_database)
If the user already exists but somehow their database doesn't, we should return an error.
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
Sounds good.
@ -0,0 +56,4 @@
) as con:
with con.cursor() as cursor:
cursor.execute(drop_user)
cursor.execute(drop_db)
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?
@ -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})
Same comments as for MySQL.
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?
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
No, I don't think so. Although it would be a good idea to check for an invalid config at startup time.
@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.