Change default value of objectsDisposition in dropRole

[smurthys]

Fixes #191

Commit a6802a7 makes the following changes:

• Change default value of objectsDisposition in function dropRole to 'assign' from 'assign_i' and remove choices of 'assign_i' and 'assign_m' for that parameter.
• Prevent a ClassDB group role from becoming the new owner.
• Make SESSION_USER the default new owner if no new owner is specified.

The changes turned out to be a bit more involved than I'd thought it would be. Everything appears to work in the limited testing I have done so far, but I need to review the code again tomorrow with fresh eyes and run broader tests. In the meanwhile, let me know if any of you find any issue.

BTW, I believe I need to revise testRoleBaseMgmt.sql because one of the test cases there no longer applies.

[smurthys]

Here is an interesting error message I got when I tried dropping a student s1:

ERROR:  must be superuser to alter superusers
CONTEXT:  SQL statement "GRANT postgres TO CURRENT_USER"

The error is reported on L486 in addRoleBaseMgmt.sql, and it is correct: I am running as postgres but the function executes as ClassDB, and this line (also correctly) wants to grant SESSION_USER (postgres) to ClassDB because a new owner was not specified. Of course, the non-super ClassDB can't grant super to another user or to itself.

I wonder if there should be a check that new owner cannot be a superuser, but then we could always have another situation where the function executor does not have enough privilege to make a grant. 😢

[wildtayne]

I was able to replicate this issue on my install, and I also don't have any great ideas for a solution. One solution, for now, would be to add an error handler in dropRole that still causes dropXYZ to fail, but adds a more helpful error message, like:

Error: Can't assign dropped objects to <SESSION_USER>, because ClassDB doesn't have permission to alter <SESSION_USER>. Please try dropping <user being dropped> as an instructor or dbmanager instead.

I think in most day-to-day usage, instructors or DBManagers will be dropping students, so this is more of an edge case. However, it seems this solution would still cause issues with our current test suite, so it's less than ideal.

[smurthys]

I just pushed some commits with the following collective changes:

• A new function grantRole in addHelpers.sql for use instead of directly executing GRANT ROLE and guarding such role grants. The new function also uses an exception handler to clarify the situation where the executing user does not sufficient privilege to make the grant.
• Use the new function grantRole in both addRoleBaseMgmt.sql and addClassDBRolesMgmt.sql
  • This change reduces chance of run-time errors in addClassDBRolesMgmt.sql because grant role used to be unguarded.
• testRoleBaseMgmt.sql is revised to handle the fact that 'assign_i' is no longer supported and SESSION_USER is the default new owner.

I notice L694 testClassDBRolesMgmt.sql fails due to the new behavior of function dropRole. This failure is correct and the tests need to updated to execute in the context of an appropriate user. An example of the change required is in testRoleBaseMgmt.sql, but the change in testClassDBRolesMgmt.sql will be simpler: the test just needs to be done in the context of an instructor role.

[smurthys]

@srrollo Good thinking about exception handling. The recent commits do the same thing in a modular way so role grants are can be managed a little better everywhere.

[wildtayne]

I like how grantRole is implemented. I these changes look good as long as the tests pass after they are updated.

[smurthys]

@afig Will you be able change testClassDBRolesMgmt.sql? Also, I haven't investigated if the privilege tests also need some attention.

With these changes, we might be ready to give the whole thing another test and hopefully create the release.

[afig]

The changes made to the object disposition behavior look great.

In respond to the last comment: Yes, I'll update the tests to reflect these changes. ~The privilege tests will in fact need to be updated as well.~~ I'll assign myself to this PR while I make these modifications. The docs will also need to be updated to reflect the new default behavior and removal of assign_i and assign_m, I will handle that too.

Edit: The privilege tests did not need to be updated, since any drops already used drop_c as their object disposition arguments.

[afig]

Upon further review, it seems that it would be more appropriate to raise a NOTICE on L505, rather than INFO.

It turns out that the documentation for INFO is somewhat unclear. When I first read the documentation, I saw INFO as a regular level for informational output that is "less important" than NOTICE. However, it is actually designed to be used for (often lengthy) output that was requested by the user. Postgres' internal documentation is actually clearer on this (it also explains the other issue with using INFO here):

L 1-13, 33-36 elog.h - PostgreSQL:

/*-------------------------------------------------------------------------
 *
 * elog.h
 *    POSTGRES error reporting/logging definitions.
 *
 *
 * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California
 *
 * src/include/utils/elog.h
 *
 *-------------------------------------------------------------------------
 */

...

 #define INFO        17          /* Messages specifically requested by user (eg
                                  * VACUUM VERBOSE output); always sent to
                                  * client regardless of client_min_messages,
                                  * but by default not sent to server log. */

In practice, it becomes an issue because INFO messages cannot be suppressed by the client_min_messages configuration option, which is useful during testing.

[afig]

The privilege tests did not need to be updated since they already used drop_c as their object disposition arguments.

The tests now pass on my system, though with the INFO messages unsuppressed.

[smurthys]

Thanks @afig for the note about INFO. I have revised the code to use NOTICE.

Also, the revised testClassDBRolesMgmt.sql runs without errors.

[smurthys]

Thanks all for reviewing.