RoleBase compatable ClassDB GroupRole functions and views

[afig]

This Pull Request provides an implementation of the GroupRole-specific functions and views defined for M2.

This includes the Instructor, Student, and DBManager views, and the following functions defined for each group:

addClassDBRolesMgmt.sql:

• createXYZ
• revokeXYZ
• dropXYZ
• dropAllXYZ (the usefulness of dropAllInstructors() and less so dropAllDBManagers() is up for debate)

addHelpers.sql:

• isXYZ

The Git diff for addClassDBRolesMgmt.sql is not very useful, since practically all code in that file was replaced. It will likely be easier to compare the files separately (i.e. without highlighting).

The implementation for the three views is currently commented out as the User view they are based off of is not yet implemented

Incomplete, nominal tests have been created for createStudent() only. I will continue to redevelop the tests for the remaining functions.

Some tests exist for the new isXYZ functions, but they are not pushed yet since there is some odd behavior from pg_has_role that needs to be investigated.

[wildtayne]

Looks good so far. One observation I had:

addHelpers.sql

• Not sure if it will help with the pg_has_role issue or not, but it seems like it would be a little clearer for the isInstructor/Student/DBManager functions to call ClassDB.isMember instead of calling pg_has_role themselves.

[smurthys]

Quick comments:

• Yes, just dropAllStudents suffices
• I have just pushed an initial version of view User in PR #154
• I feel the IsXYZ functions should reside in addClassDBRolesMgmt.sql, because addHelpers.sql should contain only functionality that are broadly useful.

[wildtayne]

Related to this comment, I think ClassDB.Student and the like are missing the LastDDLOperation column.

Also, I get an error when calling dropStudent:

QUERY:  ALTER ROLE ddlStudent01 CONNECTION LIMIT TO -1
CONTEXT:  PL/pgSQL function revokestudent(idnamedomain) line 9 at EXECUTE
SQL statement "SELECT ClassDB.revokeStudent($1)"
PL/pgSQL function dropstudent(idnamedomain,boolean,boolean,character varying,idnamedomain) line 4 at PERFORM
SQL statement "SELECT ClassDB.dropStudent('ddlStudent01', true)"
PL/pgSQL function inline_code_block line 47 at PERFORM

I think in revokeStudent(), the statement should just be ALTER ROLE * CONNECTION LIMIT -1

[afig]

Thanks for the preliminary review @srrollo and @smurthys. I agree with all recommendations/observations made so far. I will implement those changes, add the remaining tests, and fix any other issues I observe in the process.

[smurthys]

I am reviewing the files, and have one quick comment:

I feel it is better to partially define Instructor and other views as is done with User view in addUserMgmt.sql. Then show branch add-UserMgmt as blocking this PR instead of branch add-user-view.

[smurthys]

Thanks @afig for the commits.

I have the following observations on addClassDBRolesMgmt.sql:

• L85-L87 contains very helpful comments. They make me wonder what the criteria is to be a student, etc. Should a user also be known to be a considered a student, etc? I believe yes because, a student is a user, who by definition is "a server role who is not a team" and team information is available only for recorded roles.
• Permissions on IsXYZ functions (for example, around L96) let anyone run those functions. On the face of it, this seems fine, but I wonder if these functions should be limited to instructors and dbmans. (It will have to be so if the functions also test for known user). I do realize, anyone can directly call isMember or for that matter query pg_roles.
• L142: The comment seems unrelated to the associated code. At the least, the comment is unclear.
• L151 and L154: Is the concatenation operator required?
• L151: Does granting SELECT on tables also grant select on views and execution on functions? (I need to brush up on this aspect of rights management.) Similar inquiry applies to to L153-L154.
• L153: I highly recommend moving the word GRANT to L154 so the default privileges being granted are clear. This recommendation applies to similar code elsewhere in this script.
• L208-209: Splice the lines because everything fits in under 80 characters. Likewise elsewhere as appropriate.
• L223: Paramater name okIfRemainsClassDBRoleMember connotes something quite different from its corresponding parameter in function dropRole.
• I wonder if function dropAllStudents could also receive parameters dropFromServer and okIfRemainsClassDBRoleMember instead of hard coding them.

Unrelated to this PR, but there is a bug in addRoleBaseMgmt.sql that impacts createStudent etc. I have created Issue #158.

[afig]

Thanks for the more detailed review @smurthys.

Thinking about it, it would in fact be less confusing if the isXYZ functions only returned true for users that are "known". It would also be consistent with the 3 views that are defined nearby and in practice, likely used together with these functions. As far as their permissions go, the current implementation (not checking if the user is known) might as well be executed by anyone since as mentioned, the ClassDB.isMember() function and pg_catalog.pg_roles view are accessible by anyone. However, I agree that the proposed implementation of ensuring that a user is known warrants stricter execution privileges, such that only users with the ClassDB_Instructor or ClassDB_DBManager (or ClassDB) roles can execute them.

Granting SELECT on ALL TABLES in a schema does grant SELECT on views, but not EXECUTE on functions. To do the latter, we would also have to add a statement that grants EXECUTE on ALL FUNCTIONS. It makes sense to also include EXECUTE on functions. Of course, the docs will have to be updated to reflect this (and practically every other change introduced with this PR).

I agree that okIfRemainsClassDBRoleMember communicates something different from the okIfClassDBRoleMember parameter in dropRole. However, this is because it is expected that the user being dropped is currently a ClassDB Role Member, otherwise, there's no purpose in using the dropXYZ functions (we still have to plan for this situation, but it is not a nominal case). Since the parameter in question is not passed to dropRole until after a corresponding revoke is performed, it effectively serves as "remains" a classdb role once the current role is revoked. However, a comment is likely needed that explains this behavior.

[smurthys]

Quickly, in addClassDBRolesMgmt.sql: L273-L280 use incorrect function signature (uses previous version) causing compilation error.

[wildtayne]

I think this looks pretty much good to go. Also, all of the functionality I have used works well. Similarly to this, should ClassDB.Instructor/Student/DBManager be removed prior to merging?

[smurthys]

Yes, the Instructor etc. views should be removed from addClassDBRolesMgmt.sql due to PR #159

[smurthys]

I have the following observations about addClassDBRolesMgmt.sql:

• isXYZ functions should test isUser instead of isRoleKnown, because instructor, etc. must be users (not teams).
• Minor: Though we use ClassDB_Student when using it as an identifier name, we have generally used classdb_student (all lower case) when using the id name as a string.

Otherwise, this is looking good.

[afig]

I believe this PR is now ready for a "merging" review. The unit tests now cover all of the functions defined in addClassDBRolesMgmt.sql, though the tests could use quite a bit of optimization in terms of code verbosity. Still, they pass and provide acceptable coverage.

The last few recommendations have been implemented, including removing the three views.

[smurthys]

Thanks @afig for making the changes. They look very good.

BTW, I recommend discussing function dropAllStudents in a C&C meeting. We could call that discussion "Why C++ runs circles around SQL" in readability, maintainability and execution efficiency. 👨‍🏫