Uninstall could grant schema ownership to users with no access to that schema (W)

[smurthys]

The ALTER SCHEMA statements in removeClassDBFromServer.sql (also see Issue #88) does not test if the user being granted ownership actually has access to that schema.

(Also, the ALTER statements use %I to format schema names. See #86.)

Although it is customary for a schema with the same name as a user to belong to that user, that is not a guarantee.

If we do not want to test if user has access to the schema (say for complexity/priority reasons), a safer alternative might be to perform this operation only with members of ClassDB specific roles instead of all roles. The logic is that ClassDB is responsible for schemas corresponding to users it created.

We could use tables Student and Instructor to easily find users in that role, but will have to depend on the DBMS for DBManager members.

[wildtayne]

This should be fixed in a709a7402e80909c2ce53c552220299111ce25de. I changed the query to find schemas to this:

SELECT r1.rolname
FROM pg_auth_members am
JOIN pg_roles r1 ON am.member = r1.oid
JOIN pg_roles r2 ON am.roleid = r2.oid
WHERE (r2.rolname = 'classdb_instructor'
OR r2.rolname = 'classdb_dbmanager'
OR r2.rolname = 'classdb_student')
AND EXISTS(SELECT schema_name
                    FROM INFORMATION_SCHEMA.SCHEMATA
                    WHERE schema_name = r1.rolname
                    AND catalog_name = current_database())

The query to pg_roles and pg_auth_members gets the username of all users that are an Instructor, DBManager, or Student. The subquery on INFO_SCHEMA.SCHEMATA checks that the schema belonging to that user is in the current DB. This also checks if the selected user even has a schema associated with them. This won't drop other schemas owned by ClassDB, but as we discussed, these would have to be manually created. It stands to reason they would need to be manually removed as well.

[wildtayne]

Also, I think %I and %s are effectively equivalent in this case, similarly to the current_database() situation. Since the query is directly getting the user name, both %I and %s will preserve whatever case it was in. However, if we want to change this for consistency purposes (or if I'm overlooking something), that is OK too.

[smurthys]

Either %I or %s in this case, but I recommend %s for consistency and just in case someone overlooks to make the change in a different future scenario.

The query looks good. I suggest using the IN operator and adding back the check that schema is owned by ClassDB. The check for catalog_name seems redundant because schemas are within a db ("catalog"), but is not harmful. I suppose the column exists to comply with ISO spec. Also, I recommend not using a sub-query (harder to maintain), because a join with SCHEMATA does the same work.

SELECT r1.rolname
FROM pg_auth_members am
     JOIN pg_roles r1 ON am.member = r1.oid
     JOIN pg_roles r2 ON am.roleid = r2.oid
     JOIN INFORMATION_SCHEMA.SCHEMATA iss ON iss.schema_name = r1.rolname 
WHERE r2.rolname IN ('classdb_instructor', 'classdb_dbmanager', 'classdb_student')
      AND iss.schema_owner = 'classdb'
      AND iss.catalog_name = current_database();

[wildtayne]

Just has an interesting discovery about %I and %s here - the ALTER SCHEMA statements fails when using %s in this situation. For example, one generated statement is ALTER SCHEMA testStudent0 OWNER TO testStudent0;, which folds both identifiers to teststudent0 when executed. This fails, since testStudent0 is actually "testStudent0".

I propose we keep using %I in this case. Also, it seems like we can expand upon our established usage of format types:

• %s should be used for functions taking user input, because it mimics Postgres' native behavior - names are folded unless they are double quoted.
• %I should be used when identifiers are retrieved from a query. This will preserve the case, ensuring it matches the value inside the DBMS. If an identifier was folded, it is stored as lower case, so there is no risk of it gaining the wrong case.

Otherwise, I really like the suggested improvements to the query. After thinking about it, I believe we should remove catalog_name from the WHERE clause. I was thinking this could be sort of a sanity check if Postgres changed the view to display schemas from all DBs in the future. Of course, this isn't possible without major changes to Postgres, so there's not much risk in omitting that check.

[smurthys]

Good find.