search

KevinKelly25 / LearnSQL

LearnSQL, a website for learning SQL
1 stars 2 forks source link

Add role based access control #91

Closed KevinKelly25 closed 5 years ago

KevinKelly25 commented 6 years ago

In this PR I add role based access controls to the database and all objects within. To do this I created a role learnsql with CREATEDB and CREATEROLE. These attributed are/will be needed for some operations within the database.

For all tables and views I revoked all privileges from public and then reassigned the owner to the learnsql role. For the functions I did the same with also adding SECURITY DEFINER to all function so that the function is always executed as the learnsql role rather then the superuser that could have possibly created the functions.

Another addition is that only superusers and learnsql roles should be able to connect to this database now.

michaeltorres1 commented 5 years ago

Thank you for this PR @KevinKelly25. Because it seems outdated now due to the fact that some other PR's took care of updating some of the tables in initializeDB.sql and also added the schema qualifier name that was needed. It would probably be a good idea to go ahead and copy over what we currently have for our initializeDB.sql. Also, this may resolve some of the conflicting issues this PR may have. Other than that I have updated the userMgmt.sql in PR #92 so it will probably be a good idea to also go ahead and copy over what you have for that one to the current userMgmt.sql.

Once you have done this I will do a more thorough review of this PR for the initializeServer.sql. For now please make sure to include a space between -- and the start of your comment.

EDIT: Added the last sentence.

KevinKelly25 commented 5 years ago

Updated the PR to work with the current dev branch. Everything should be be working with role based access control now.

cinnaco commented 5 years ago

Thanks @KevinKelly25. This PR functions well and I see how these changes improve the security and integrity of the data. I will approve to merge.

Edit: Original comment was written under the assumption that PRs currently in progress were already merged.

KevinKelly25 commented 5 years ago

Thank you @michaeltorres1 and @cinnaco for reviewing the branch. While I see you changed your comment @cinnaco you are right that the branch should be at the most recent dev branch update for testing which I just checked and it works.