Disallow student-initiated schema drop

[smurthys]

Draft of DDL monitoring to prevent issues such as users being able to drop their own schema. Creating this PR to get early feedback. Please do NOT approve until I indicate the work is complete.

I will add a comment a few hours from now detailing some observations and options, but please review the conversation in Issue #175 in the meanwhile.

[smurthys]

I just pushed some changes that prevent student-initiated schema drop. Fixes #175. Please review.

Change highlights:

• Due to Postgres limitations discussed in Issue #175, the solution is rather heavy-handed in that, by default, students are not allowed to execute DROP SCHEMA and DROP OWNED statements.
• The solution is made part of recommended scripts
• Functions to allow and disallow schema drop
• Function to determine if schema drop is presently allowed
• Test scripts

The list of files changed is large because this branch was created before M2 was completed. The actual files changed are:

• src/db/reco/addDisallowSchemaDropReco.sql
• src/db/core/addClassDBRolesMgmtCore.sql: changed a comment line
• tests/testDisallowSchemaDrop.sql

I will update the Wiki docs over the next few days with an eye on the discussion here.

[afig]

I believe this "heavy-handed" solution of restricting DROP SCHEMA and DROP OWNED will work out, especially since it is easy to switch the restriction on and off. I agree that the added script should go under recommended scripts. Overall, the changes look good, unit tests pass, and the behavior seems to work as expected.

I noticed a couple of minor comment issues and some possible missed test cases:

addDisallowSchemaDropReco.sql:

• L 102: 'schema schema' -> 'schema drop', remove double space after 'is'
• L 118: Unfinished comment '--D means...'

testDisallowSchemaDrop.sql:

• L 17-18: Does this comment still apply? A test that intentionally raises an exception is present.
• Tests for DROP OWNED are not present (may already be known)

However, there also seems to be an issue that will come up when merging this PR. The list of files changed should not be large, and should only show those files that were changed in this PR. I tested the merge in a copy of this repository, and merging this into dev will result in any changes since d2cdb4666a3d12ec42b9caadbd9f26334d4143e3 (where monitor-schema-drop originally branched off of dev) being undone. I would guess that this is caused by the revert c9a5259c7fde6e804b45a1e4c91cd68b667ee1ea, which reverted the update from dev.

Logically, this should be able to be fixed by re-merging dev into monitor-schema-drop. However, the GitHub Desktop client does not seem to be allowing this. Git seems to be in a state where it thinks that monitor-schema-drop is up to date with dev when it is not. I will look into forcing an update from dev or some other solution to this problem, but merging this PR should be avoided for the time being.

[wildtayne]

I can try fixing this branch using the method outlined by Andrew, however it does look like the revert causes some issues. If it looks really bad, I could make a new branch off of dev and selectively apply commits from this branch to get it to the desired state.

I think the changes look good overall, I will be able to do a more detailed review later.

[wildtayne]

OK, so looking at the repo using gitk gives some insight. Basically, what has happened is that dev was merged into this branch, and then that merge commit was reverted. The result of this is that all changes from dev have been removed from this branch AND git thinks that monitor-schema-drop is "more up to date" than dev because of the merge (even though it was reverted). I will look into the best way to remedy this.

Picture of the issue:

Edit: I've made a test branch that reverts the revert here. If the diff looks correct, the only change that needs to be made to monitor-schema-drop is to revert c9a5259c7fde6e804b45a1e4c91cd68b667ee1ea.

Edit 2: 3bc1b58b538cecd1846fb9b641dca6c4db897cb2 seems to introduce some changes other than those specified to src/db/core/addClassDBRolesMgmtCore.sql.

[wildtayne]

I was also able to get the unit tests to pass. In addition to Andrew's observations, the only issue I noticed was that removeAllFromDB.sql should drop triggerDropSchemaDDLStart.

[smurthys]

Many thanks for the reviews.

Indeed the revert of merging dev is a problem. Not sure why I reverted the merge (I don't recall reverting, but obviously I did). I have scheduled time with @afig to work on this together to be sure the merge works as intended.

I will address all the issues observed, and complete the test scripts in the next few commits.

[smurthys]

@afig and I worked together to resolve the merge issues. Thanks @afig for your time.

In the process, we decided to forego a comment change made to addClassDBRolesMgmt.sql, because the issue of incorrect file references (in comments) needs to be specifically and broadly examined. I have created a new issue #212 for that purpose.

I will continue to work on this branch after completing tasks on other projects. I will let you all know when this is ready for approval review. In the meanwhile, please feel free to share other issues you may notice in this branch.

[smurthys]

I just pushed some commits that expand the tests and also add event handlers to the removal script.

One of the tests fails: new schema creation by student user (fail code 3). This failure is expected because a Teams discussion establishes users are presently unable to create own schemas. That issue is being discussed. I will leave the failing test in because it will (should) pass if users are allowed to create schema.

[smurthys]

I think this PR is ready to be reviewed for approval.

[afig]

Looks great, tests (except for the one that results in code 3) pass. Manually granting students CREATE on the database results in all tests passing. Really only one minor issue in testDisallowSchemaDrop.sql:

• L74, 94: A few letters in keywords are not capitalized

However, granted that we are now paying a bit more attention to error messages, would it make more sense to use DETAIL rather than HINT when sending SQLERRM to the client in the tests?

~Actually, on closer inspection, it looks like one instance (L69) does use DETAIL, while the others do not.~ Never mind this last sentence, this was the result my own testing.

[smurthys]

Thanks @afig for the review. The "misspelled" keywords are intentional as the end-of-line comments state. Yes, it is better to use DETAIL.

I just pushed the following changes:

• Clarify why keywords are "misspelled" in two cases [to test if the event handler detects these commands written in any character case]
• Replace HINT with DETAIL
• Distinguish between raise_exception, the exception raised by the drop-schema event handler, and other exceptions. This change tightens the test cases where student-initiated schema drop should cause an exception.

[smurthys]

Thank you all for the reviews