DASSL / ClassDB

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

Misleading notice given when dropping multi-role user from server (W) #192

Closed afig closed 6 years ago

afig commented 6 years ago

When revoking a ClassDB role from a user that has more than one group role, the following message is displayed

NOTICE:  User "%" remains a member of one or more additional ClassDB roles

However, this message also shown when dropping a multi-user role, even if the dropFromServer and okIfClassDBRoleMember arguments are TRUE. This is incorrect because upon completion of the process, the role is not a member of another ClassDB role, as the role no longer exists on the server or in the RoleBase table.

Although this notice is raised from L 386 of addRoleBaseMgmtCore.sql, it is correct for that NOTICE to be raised there, since it is located in the ClassDB.revokeRole() function, which expects the role to continue existing.

Instead, it seems that the dropXYZ functions in addClassDBRolesMgmtCore.sql need to be modified. However, simply blocking notices raised by the revokeRole() wouldn't be a feasible solution, as it would mask other useful notices, such as a user not being known or not a member of the group role attempting to be revoked. The dropXYZ functions call the revokeXYZ functions (which then call revokeRole()) in order to undo role-specific configuration, however, this may need to be rethought, as was mentioned by @smurthys in an earlier meeting.

Steps to reproduce:

classdb=# SELECT ClassDB.createStudent('multiroletest', 'Test user');
 createstudent
---------------

(1 row)

classdb=# SELECT ClassDB.createDBManager('multiroletest', 'Test user');
NOTICE:  Server role "multiroletest" already exists, password not modified
 createdbmanager
-----------------

(1 row)

classdb=# SELECT EXISTS (SELECT * FROM ClassDB.RoleBase WHERE RoleName = 'multiroletest');
 exists
--------
 t
(1 row)

classdb=# SELECT EXISTS (SELECT * FROM pg_catalog.pg_roles WHERE RolName = 'multiroletest');
 exists
--------
 t
(1 row)

classdb=# SELECT ClassDB.dropStudent('multiroletest', TRUE, TRUE, 'drop_c');
NOTICE:  User "multiroletest" remains a member of one or more additional ClassDB roles
 dropstudent
-------------

(1 row)

classdb=# SELECT EXISTS (SELECT * FROM ClassDB.RoleBase WHERE RoleName = 'multiroletest');
 exists
--------
 f
(1 row)

classdb=# SELECT EXISTS (SELECT * FROM pg_catalog.pg_roles WHERE RolName = 'multiroletest');
 exists
--------
 f
(1 row)
smurthys commented 6 years ago

Thanks @afig for the report.

I feel the notice the block containing L386 of addRoleBaseMgmtCore.sql can be entirely removed because it is not guarding any other condition that impacts system performance. (I recall pondering the need for that block as I wrote it.)

Alternatively, the NOTICE could be changed to INFO which can then be suppressed by callers.

afig commented 6 years ago

I don't feel strongly either way, though I suppose it would be better to simply remove the block. Changing it to an INFO would mean that we should create some sort of criteria of when a message should be INFO vs NOTICE. Also, since the INFO would simply be suppressed during most calls, it would not serve much purpose if implemented.

smurthys commented 6 years ago

Yes, in this use pattern, adding INFO would be wasteful because the caller will probably just suppress it.