DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Frequent views/functions required by instructors and students are missing #134

Closed smurthys closed 6 years ago

smurthys commented 7 years ago

Views and/or functions are required to support frequent instructor and student activity.

The following script defines some utility views (some of which might have to be defined as functions).:

--student activity
CREATE VIEW ClassDB.StudentActivityAll AS
(
SELECT username, lastddlactivity, lastddloperation, lastddlobject, ddlcount, lastconnection, connectioncount 
FROM ClassDB.student
ORDER BY username
);

--student activity: anonymized
CREATE VIEW ClassDB.StudentActivityAnon AS
(
SELECT lastddlactivity, lastddloperation, SUBSTRING(lastddlobject, POSITION('.' IN lastddlobject)+1) lastddlobject, ddlcount, lastconnection, connectioncount 
FROM ClassDB.student
ORDER BY connectioncount
);

--my activity
CREATE VIEW ClassDB.MyActivity AS
(
SELECT username, lastddlactivity, lastddloperation, lastddlobject, ddlcount, lastconnection, connectioncount 
FROM ClassDB.student
WHERE username = current_user
);

--list of tables/views, ordered by student
CREATE VIEW ClassDB.StudentTable AS
(
SELECT table_schema, table_name, table_type 
FROM information_schema.tables JOIN ClassDB.student ON table_schema = username
ORDER BY table_schema
);

--number of tables/views by student
CREATE VIEW ClassDB.StudentTableCount AS
(
SELECT table_schema, COUNT(*) 
FROM information_schema.tables JOIN ClassDB.student ON table_schema = username
GROUP BY table_schema
ORDER BY table_schema
);
wildtayne commented 7 years ago

@afig and I took a look at the myActivity view, and made a wrapper function so students can access it. The function is placed in public, because students are unable to access any object in the ClassDB schema, regardless of object permissions. We also changed current_user to session_user in the view, because the function executes with SECURITY DEFINER.

We created a test database classdb_test on the local VM that is clone of the cs205 database (@~9:00AM on 11/1/17). We have created the myActivity function there for testing purposes if you want to try it out.

If you think the function looks good, executing the following code on the cs205 database should update the myActivity view and create the myActivity() function with the correct permissions.

--my activity
DROP VIEW IF EXISTS ClassDB.myActivity;
CREATE VIEW ClassDB.myActivity AS
(
     SELECT username, lastddlactivity, lastddloperation, lastddlobject, ddlcount, lastconnection, connectioncount
     FROM ClassDB.student
     WHERE username = session_user
);

REVOKE ALL ON VIEW
     ClassDB.myActivity
     FROM PUBLIC;

ALTER VIEW
     ClassDB.myActivity
     OWNER TO ClassDB;

CREATE OR REPLACE FUNCTION public.myActivity()
RETURNS TABLE (userName VARCHAR(63), lastDDLActivity TIMESTAMP, lstDDLOperation VARCHAR(64),
     lastDDLObject VARCHAR(256), DDLCount INT, lastConnection TIMESTAMP, connectionCount INT) AS
$$
     SELECT *
     FROM classdb.myActivity;
$$
LANGUAGE sql
SECURITY DEFINER;

REVOKE ALL ON FUNCTION
   public.myActivity()
   FROM PUBLIC;

ALTER FUNCTION
   public.myActivity()
   OWNER TO ClassDB;

GRANT EXECUTE ON FUNCTION
   public.myActivity()
   TO ClassDB_Instructor, ClassDB_DBManager, ClassDB_Student;
smurthys commented 7 years ago

Yes, using session_user is better.

I wonder if public.myActivity could be the view that was proposed as classDB.myActivity so there is no additional layer at all.

Alternatively, could public.myActivity invoke ClassDB.StudentActivityAll and filter on username? Then, there would be no need for classDB.myActivity at all.

wildtayne commented 7 years ago

I agree that having public.myActivity() query ClassDB.StudentActivityAll sounds like the best plan. The following function should accomplish this:

CREATE OR REPLACE FUNCTION public.MyActivity()
RETURNS TABLE (userName VARCHAR(63), lastDDLActivity TIMESTAMP, lstDDLOperation VARCHAR(64),
     lastDDLObject VARCHAR(256), DDLCount INT, lastConnection TIMESTAMP, connectionCount INT) AS
$$
     SELECT username, lastddlactivity, lastddloperation, lastddlobject, ddlcount, lastconnection, connectioncount
     FROM classdb.StudentActivityAll
     WHERE username = session_user;
$$
LANGUAGE sql
SECURITY DEFINER;

REVOKE ALL ON FUNCTION
   public.myActivity()
   FROM PUBLIC;

ALTER FUNCTION
   public.myActivity()
   OWNER TO ClassDB;

GRANT EXECUTE ON FUNCTION
   public.myActivity()
   TO ClassDB_Instructor, ClassDB_DBManager, ClassDB_Student;
wildtayne commented 7 years ago

Also, for consistency, it would be a good idea to give each of these views permissions matching other ClassDB objects. For example, ClassDB.StudentActivityAll:

REVOKE ALL PRIVILEGES ON ClassDB.StudentActivityAll FROM PUBLIC;
ALTER VIEW ClassDB.studentActivityAll OWNER TO ClassDB;
GRANT SELECT ON ClassDB.StudentActivityALL TO ClassDB_Instructor;

(This assumes we do not want DBManagers to have access to student data)

This also allows ClassDB users to access these views if they are made by a superuser.

wildtayne commented 7 years ago

I have deployed an updated version of these views on the local VM. The script used to create them can be found in this gist: https://gist.github.com/srrollo/a739fa014bf724a1d613d38e9503dab6

It appears that there is no way for a view to access a table the SELECTing user doesn't have permissions for, so I have kept public.MyActivity() as a function.

smurthys commented 7 years ago

Thanks @srrollo. I tested public.myActivity() and it works.

I will have to ask a student to test it. Any chance you tested it using your test student account?

wildtayne commented 7 years ago

I was able to test it with a student account, and it does appear to work correctly.

smurthys commented 7 years ago

myActivity works as expected. Thank you.

Edit: removed the additional view requirement. Remembered that I have already listed it at the beginning of the issue.

smurthys commented 6 years ago

I propose the following actions:

I believe this issue can be closed once all the items in the check list above are completed.

EDIT: Re-formatted the list to correctly show a checklist

smurthys commented 6 years ago

This issue is related to #48.

smurthys commented 6 years ago

I am traveling and don't have a proper setup to type detailed docs. So, it will be helpful if @afig or @srrollo took a stab at the two sets of criteria in separate Wiki pages. We can move them later to docs.

wildtayne commented 6 years ago

I've added the list of views and documentation to be created to the M2 wiki page. I've also created a page for function vs. view criteria and classdb vs. public schema criteria. I created some initial criteria based on our findings when we tested these views at on the local VM.

smurthys commented 6 years ago

Awesome start @srrollo. I will look at the pages later today,

wildtayne commented 6 years ago

I just pushed a branch add-frequent-views which adds a creation script for the frequent views in addFrequentViews.sql. The script is the same as the final one posted in the gist in this thread, and currently deployed on the local VM. I also added a line to prepareDB.psql to run to script during installation.

I am open to name suggestions for addFrequentViews.sql, since I'm not sure that is the best name for. I also think we should add the changes discussed to the documentation in this branch before merging it.

afig commented 6 years ago

In term of the UTC time zone issue, I have a proposed implementation for formatting the timestamp string and adjusting for a given timezone. Once reviewed and adjusted as necessary, this implementation can be integrated into any necessary function(s) in ClassDB.

I have created a gist for the implementation. I think it would be best to have discussion related to the implementation on the gist, and only discuss integration with ClassDB functions here.

wildtayne commented 6 years ago

I've pushed a new version of addFrequentViews to add-frequent-views that I believe adds all the objects currently enumerated in the wiki. It also uses the latest version of ChangeTimeZone from the gist, however, I have not added the CREATE FUNCTION statement for it yet (I believe this should be addressed in a separate branch). So keep in mind that you have to manually create that function first.