Initial version of addUserMgmt.sql

[smurthys]

Commit b4f416c adds the initial version of addUserMgmt.sql. Commit 714774d adds testUserMgmt.sql containing some initial, rudimentary, nominal unit tests.

File addUserMgmt.sql needs a through review because it has trigger definitions that can make or break the system.

Commit 73e0f5f opportunistically adds a test case previously identified.

[smurthys]

Commit ec199d1 corrects an error in addUserMgmt.sql: trigger function ClassDB.declineUpdate was being called instead of the trigger function ClassDB.rejectOperation.

[wildtayne]

This looks great so far @smurthys. Some initial observations I have:

General: L63: 'not prefaced ain' - should be 'again'? L74: References DDL activity - should be connection activity

ClassDB.rejectOperation():

• We need to verify that the DDL event trigger will never insert into ClassDB.DDLActivity or ClassDB.ConnectionActivity if the triggering user is unknown. Since ClassDB.rejectOperation() throws an exception if the user is unknown, I believe this would cause the unknown user's DDL operations to always fail. Currently, I belive the DDL event trigger checks that the user is known, but we definitely will want a unit test for this.

pg_temp.isTriggerDefined():

• I think this would be a useful function to have in addHelpers if we could extend it to look for event triggers as well. Possibly with a parameter chosing what type of trigger to check for (normal, event, or both). Unfortunately, I don't think event trigger metadata is stored anywhere in info schema, since it is non-standard functionality. They do appear to be stored in pg_event_trigger, however.

I like the approach used for rejecting operations. I think it would be good to use a similar approach to block other invalid operations, such as students dropping schemas.

[smurthys]

I just pushed two commits:

• 22ea351 fixes the issue @afig reported about missing WHERE clause when updating a known role.
• 5b909a5 fixed the issues @srrollo highlights. It also adds an initial definition of the User view. The view does not yet have the aggregate attributes related to user activity.

I have the following observations about two of @srrollo's items above:

The BEFORE INSERT trigger on table DDLActivity should reject activity from unknown users. This is not a problem because the DDL activity trigger does check if the user is known. However, we need to think about whether the BEFORE INSERT trigger on ConnectionActivity should apply this test. More importantly, we need to decide if function importLog should import activity only for known users: The log import is a batch process and users could have come and gone between two import runs.

As useful as function isTriggerDefined() could be in some situations, its utility to us depends on the semantics we need/want when creating triggers. The choices are:

• CREATE TRIGGER IF NOT EXISTS: the function is useful in this semantics
• CREATE OR REPLACE TRIGGER: the function is not useful, because creation can simply be preceded by a DROP IF EXISTS (which oddly enough exists). I believe we already have/had this approach in DDL monitoring.

[wildtayne]

The CREATE OR REPLACE pattern works fine, however I also currently use isTriggerDefined() 4 times in the ClassDB.enableDDLActivityLogging() and ClassDB.disableDDLActivityLogging() functions. This is to provide an exception if one tries to run these functions without the triggers existing. Another option is to skip the check, since running ALTER EVENT TRIGGER on a non-existent trigger gives the following error message: ERROR: event trigger "triggerddlcommandsqldrop" does not exist which is fairly detailed.

[wildtayne]

Also, I noticed that ClassDB.User is missing the LastDDLOperation column (unless that is intentional). This also seems to propagate to ClassDB.Student and the like.

[smurthys]

Thanks @srrollo for spotting the missing view attribute. The issue is traced back to a missing attribute in the ERD. I have opened Issue #156 about that.

The view itself should be changed when the aggregate fields are populated (they are presently commented out).

[smurthys]

Actually, the issue with the missing view attribute traces back to the design outline. @KevinKelly25 has fixed that problem.

[smurthys]

Just pushed commit 47c1721 to fix issue #158

[afig]

I gave this a review and everything looks pretty good, just a couple minor comment issues:

• L 18: Space needed after first comma
• L 91: paramers -> parameters
• L 160: DDLActivity -> ConnectionActivity

All unit test passed. I also tested with cases that should result in exceptions and they behaved as expected.

[wildtayne]

I also think this looks pretty much good to go. One final thing - should ClassDB.User be removed from addUserMgmt.sql prior to merging this?

[smurthys]

Thanks @afig and @srrollo for the feedback. Commit 0c56386 addresses them:

• Function isTriggerDefined is no longer defined/used because we want the CREATE OR REPLACE TRIGGER pattern
• Removed view User because that view is now in a different script: see PR #159
• Replaced COMMIT with ROLLBACK in testUserMgmt.sql: had been meaning to do this for a while. Can do this in other test as appropriate and when we have time.
• Comment fixes

[smurthys]

Thanks all for the reviews.