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

Having a "known" role no longer exist on the server breaks most parts of the system (W) #179

Open afig opened 6 years ago

afig commented 6 years ago

If a role is "known", which is defined to be as having an entry in the ClassDB.RoleBase table, but it does not match a server role, then significant parts of ClassDB no longer function at all.

The issue traces down to an exception raised by isMember() if the role being tested does not exist in the server. This isMember() function is then used by isStudent(), isInstructor(), and isDBManager(), which are then used by the User view. This User view, and the derived Student, Instructor, and DBManager views are used by most other parts of the system, including logging.

Having a "known" role not exist on the server should not occur during regular usage of ClassDB. This is most likely to happen if the role is manually dropped using DROP ROLE, rather than through ClassDB's dropXYZ functions. However, issue #177 points out a situation where this non-correspondence can occur accidentally without running a deliberate DROP ROLE.

The solution is not necessarily to have isMember() no longer raise an exception. A different approach may be more suitable.

smurthys commented 6 years ago

This is an issue with pg_has_role and I believe I have documented this in a PR early on in this cycle.

The trouble is isMember cannot just return FALSE if the role is not defined on the server, because then the caller wouldn't know why FALSE is returned. It could return a NULL, but then that will likely cause an exception elsewhere.

So, I believe the existing behavior is the best outcome, unless we want to test if returning NULL from isMember can work.

afig commented 6 years ago

I see. I believe returning NULL might be more appropriate for isMember, however, like mentioned, it would simply cause exceptions elsewhere and dealing with them would involve a lot of extra checks.

Given that instructors cannot perform aDROP ROLE by default, I agree that the current behavior is best.