Initial version of RoleBase management

[smurthys]

This PR consists of the following commits to add support the new approach to user/team management:

• d99a211 Revises addHelpers.sql to meet needs of new user/team mgmt.
• 526d9ed Adds initial version of addRoleBaseMgmt.sql for new user/team mgmt.
• 746e9e2 Adds addRoleBaseMgmt.sql to prepareDB.psql
• 79ebb9a and 68c8a09 Update addUserMgmt.sql to replace calls to function isRoleDefined with calls to isServerRoleDefined because that function is renamed in addHelpers.sql
• fcdbc7f renames the script addUserMgmt.sql to addClassDBRolesMgmt.sql to make it easier to merge upcoming changes
• 287d420 Adds custom domain ClassDB.IDNameDomain to addHelpers.sql for use as the type for data representing user name, role name, schema name, and other identifier names
• 2aa6632 Adds function getSchemaName and uses ClassDB.IDNameDomain in addRoleBaseMgmt.sql. Also fixed the issues listed in the comments to commit 526d9ed.

I have yet to include the test scripts for the changes made. However, I am creating this PR pending that so we can get a head start on the review of these rather critical changes.

A major concern I have in these changes is whether case-folding is properly handled in addRoleBaseMgmt.sql.

EDIT: Moved my response to earlier comments to a new comment seeing that those comments appear later.

[smurthys]

All issues listed in earlier comments are fixed, but the following points by @afig are worth highlighting;

• Function ClassDB.getSchemaName is added to retrieve any role's schema

• Please review the code for function dropRole and see if you still have the question on order of calls to isServerRoleDefined and isClassDBRoleName. Note that the function isClassDBRoleName only tests if the parameter contains the name of a ClassDB group role.

• The M1 approach to schema management was rather simplistic: it assumed a role and its schema have the same name, which is not necessarily true. As a result, there is a particularly insidious bug that blindly drops the schema with the same name as the user name without checking who owns that schema. The M2 approach to schema management seems more robust due to dropping objects based on ownership, which requires schemas to be owned by roles.

However, the point about a user now being able to drop their own schema is very valid. It seems this issue can be addressed using a DDL trigger to prevent a student user from dropping their own schema. I assume this can be done.

For these and other reasons, I think a thorough review and analysis of the M2 approach is necessary.

[wildtayne]

With the revisions suggested by @afig done, I can't find any other issues with this code itself that jump out at me. I will continue to think about any possible effects the new system will have.

I believe there are several other places that rely on role name = schema name, such as removeFromDB.sql, which will need to be changed. The new functionality in this PR should make the uninstaller much more maintainable, however.

To confirm, as I understand it, each ClassDB user will be the owner of their schema. I don't think this will cause any issues (besides the aforementioned DROP SCHEMA issue).

It should be possible to prevent any role from executing DROP SCHEMA (or any arbitrary DDL command) using an event trigger based on the documentation here and here. The first link even has an example implementing this exact functionality.

[smurthys]

I am working on the test scripts but am delayed due to having to address existing test scripts which don't seem to have been revised for some changes that already went into M1. :(

I will need to fix/augment those scripts before testing the new functionality.

I intend to push more changed to this PR, so it might be better to hold approvals. However, please do review code and provide feedback in the meanwhile.

[smurthys]

I just pushed four commits with changes to test scripts that already existed: 9ff94ea, 5881387, b8d9900, and 38aa008. The following is a summary of changes by file:

*`testAddUserMgmt`**:

• Renamed files testAddUserMgmt* as testClassDBRolesMgmt* to match the renaming of the script file addUserMgmt.sql.
  • A new set of files testAddUserMgmt* should be added when a new script addUserMgmt.sql is added. (This addition is expected in M2.)
• In the renamed files, changed calls to ClassDB.isRoleDefined to ClassDB.isServerRoleDefined

testAddHelperFunctions.sql

• Add tests for new functions added to addHelpers.sql
• Corrected ClassDB role names: was still saying 'Instructor', 'Student', and 'Manager' though those names were changed before M1
• Used ClassDB.IDNameDomain
• Corrected comments

addHelpers.sql

• Fixed spelling error in ClassDB role name: changed 'classdb_manager' to 'classdb_dbmanager'
  • Running the revised tests exposed this issue.

With the changes in place, all test scripts included in these commits passed. 👍

FYI, I am still working on the test script for addRoleBaseMgmt.sql. It has been slow going.

[smurthys]

I just pushed four commits with changes: 87176d4, 9db4907, 592b6bb, and 4371a8d. The following is a summary of changes by file:

addHelpers.sql: Updated function attributes (relates to Issue #144)

testHelpers.sql: Renamed file testAddHelperFunctions.sql to testHelpers.sql

testRoleBaseMgmt.sql: Initial version of unit tests for the functionality in addRoleBaseMgmt.sql. Covers only nominal tests, and still need a plan to test cases that should cause exceptions. I estimate the present tests cover less than a third of the possible cases.

addRoleBaseMgmt.sql: Many changes since last commit. Needs very thorough review.

• Fixed issues found by unit tests. With the changes, the script passed all unit tests
• Improved how dropRole handles drop and assign of currently owned objects
• Updated function attributes (relates to Issue #144)
• Reordered code chunks to make it easier to read/follow

With these changes I think it is be safe for all to use the files in question in their own branches. Of course, it can be merged after all repo contributors approve (and all necessary changes are made).

Thank you all for your feedback on the earlier commits.

[smurthys]

I just pushed two more commits: 83a2f75 and 16b5a4b.

(Please ignore the "revert" commits. The first one was an accident; the second was a correction.)

The following is a summary of changes by file:

addHelpers.sql: Added function getSchemaOwnerName to return the name of the owner of a schema. This function is useful at many places in the system. It uses INFORMATION_SCHEMA.schemata.

Also added notes about casting ClassDB.IDNameDomain. Briefly, we must explicitly cast NAME type (e.g., value from CURRENT_USER and CURRENT_SCHEMA) to ClassDB.IDNameDomain though Postgres docs say the casting will be automatic.

testHelpers.sql: Added unit test for function getSchemaOwnerName.

addRoleBaseMgmt.sql: Used function getSchemaOwnerName. Also revised function dropRole as follows:

• changed default value for parameter okIfClassDBRoleMember to TRUE (because there is no downside)
• Eliminated a couple of notices because they seem unnecessary
• Trimmed parameter objectsDisposition

Using getSchemaOwnerName reduced LOC and also fixed the problem of INFORMATION_SCHEMA not being able to find schema owner name. I wonder if the original problem had something to do with casting to ClassDB.IDNameDomain

[wildtayne]

First, I want to say how useful the helpers defined in addRoleBaseMgmt.sql are in adapting the other parts of ClassDB to work with the new role management system. In many cases, they even improve the readability of the code.

I was also able to get the unit tests to run successfully on my local server, and I think the code looks great. I think this is good to be merged.

[afig]

I really like how well documented all of the the new code is, along with the unit tests created for it. It sets a great example of how other code should be documented and tested. I also agree with @srrollo's point about the helper functions being very useful 👍.

However, I was not able to successfully run the unit tests on a clean instance without some minor tinkering (see notes under testRoleBaseMgmt.sql). Everything else looks good, apart from some minor typos and inconsistencies in comments.

addRoleBaseMgmt.sql:

• L 12: Another newline should go here to separate prologue from contents

• L 46: uniquess -> uniqueness

• L 236: Unfinished/extraneous comment?

• L 241: role-spefic -> role-specific

• L 282: reliably -> reliable ?

• L 415: gurantees -> guarantees

• L 480: Extra newline needed

• Comments for sections where function privileges are being set are inconsistent. At times they are not present, sometimes one comment for the whole section, and sometimes there's one comment per statement in the section. This inconsistency also exists in other scripts. I don't believe we've decided as a group which to use, however, the statements are numerous and rather self explanatory, so I would side on using one comment for the section, or none at all to reduce clutter.

testRoleBaseMgmt.sql:

• L 69, 120: Test output shows isUserKnown() as being tested, but the function being tested is actually named isUser()

• L 73, 74, 124, 125: Test raises ERROR due to use of ClassDB.isTeamKnown() to determine whether a role is a team or not; it appears that this function was renamed to ClassDB.isTeam()

• L 238: Test raises ERROR here due to an issue somewhat unrelated to this PR; in order to reassign objects, "privileges are required on both the source role(s) and the target role", effectively, the executing user must be a member of them; the classdb role never has the classdb_instructor role granted to it; running the following statement prior to running the test allows it to pass: GRANT ClassDB_Instructor TO ClassDB;. A simple solution/workaround would be to effectively implement that statement into prepareServer.sql, but this does not entirely resolve the underlying issue. Here's the full error output:

...
psql:testRoleBaseMgmt.sql:297: INFO:  PASS   dropRole(u1, FALSE, FALSE, assign, s1)
psql:testRoleBaseMgmt.sql:297: INFO:  PASS   isRoleKnown(t1: before dropRole)
psql:testRoleBaseMgmt.sql:297: ERROR:  permission denied to reassign objects
CONTEXT:  SQL statement "REASSIGN OWNED BY t1 TO classdb_instructor"
PL/pgSQL function droprole(idnamedomain,boolean,boolean,character varying,idnamedomain) line 62 at EXECUTE
SQL statement "SELECT ClassDB.dropRole('t1', TRUE)"
PL/pgSQL function inline_code_block line 204 at PERFORM
ROLLBACK

addHelpers.sql:

• L 26: repating -> repeating (typo existed prior to the changes made in this PR)

[smurthys]

Thanks @srrollo and @afig for the feedback. I just pushed commits f3675a1 and d13d66b to address the issues.

• In function dropRole, I did think about assigning to current user both the roles in question and thought I had implemented it, but it seems I forgot to do so. That is now done.
• While at it, I have also changed prepareServer.sql to grant instructors, etc. to ClassDB role.

Function isTeamKnown working in unit tests when I run them was due to not cleaning out the old function after renaming it: the bane of all developers. We need containers right now.

[KevinKelly25]

The current branch very good and like @srrollo mentions the new helpers do make the code a lot easier to read in this PR and others. While there is still some small sections I do not fully feel familiarized to yet of the parts I did feel comfortable with I did not notice any issues, with one exception.

The only very very tiny issue, that may not even be an issue with this PR, just on my familiarity, is that the consistency of capitalization and spacing of comments seem irregular to me. This may be something better addressed in issue #133 but I compared this PR's comments to PRs #150 and #151 and they seem inconsistent with the use of capitalization and spacing. It may be helpful for setting the standard now before everything is finalized.

An example of this in addRoleBaseMgmt.sql being:

LN 40 - 41: --Define a table of users and teams recorded (made known) for this DB -- each user/team has their own DBMS role

LN 93 - 94: --make ClassDB the fn. owner, let only instructors and managers execute the fn. -- this pattern of ownership and grant/revoke applies to all functions in this

LN 42 - 44 in PR #151: --Check if the user is associated with the schema they are trying to list from. -- This is required because a user's schema name is not always the same as their -- user name.

Again, only a very tiny issue, just unsure if this consistency was intended.

[smurthys]

@KevinKelly25 Thanks for the feedback. You are correct the comment starting on L93 in addRoleBaseMgmt.sql was inconsistent.

I have pushed commit 5f7dd4d which fixes that and a couple of comment-related issues in addHelpers.sql.