Make CreateXYZ functions Safe

[KevinKelly25]

This pull request is for issue #201 and the CreateXYZ functions were changed to incorporate those changes suggested by @smurthys in that issue thread.

This is not ready to be approved but I wanted to get opinions on how to handle the changed test script for the new CreateXYZ functions. All I did was change the test script testClassDBRolesMgmt.sql so that the tests on the passwords were testing on the initial password. However, this should raise 3 warnings for each test for a total of 9 warnings. It may be useful to see that a warning pops up to see that the new addition is working correctly but I am not sure it is worth doing 9 times.

Maybe we should change it so each test function incorrectly supplies a password once. Also suppressing the warning might be a good idea since it will be testing the password to make sure it was set to default later.

[smurthys]

Thanks @KevinKelly25 for the commits.

I recommend leaving out the last parameter in calls to function createRole (instead of explicitly passing parameter 1). This approach permits us to easily change function createRole.

Also, please use the following code to raise warning:

RAISE WARNING 'For security reasons, parameter "initialPwd" is ignored and user password has been set using the default password policy described in the API documentation.' 
      USING HINT = 'Parameter "initialPwd" will be dropped in the next API release. Please update your code.';

[KevinKelly25]

@smurthys Thank you for the suggested additions. My latest commit reflects those suggestions.

[wildtayne]

The functions seem to work well in casual usage, however the unit tests fail in several places. There's a bit too much log to dump into this comment, I will try and narrow down where the tests are failed. All the errors are error code 3.

EDIT:

• There is an error trying to create testStu4 ("Invalid argument: server role "testStu4" already exists")
• createDBManager/Instructor/StudentTest() all fail with error code 3

[afig]

The error creating testStu4 is actually a bug with the tests. I will log an issue for this, but essentially, the tests do not perform necessary cleanup if they exit with a FAIL. (Eventually they will cleanup because of the ROLLBACK at the end of the script, but as is occurring here, they can inadvertently cause other tests to fail.)

Error code 3 occurs when users are not created with the expected password. I will investigate this later if needed.

[KevinKelly25]

I haven't tested it yet but I suspect these errors are due to the fact that I just copied and pasted the username into the fields where there was a custom password that needed to be changed. However, I think this leads to an error because of the capitalization and the fail in the 'pg_temp.checkEncryptedPwd()` . I believe that I just need to make sure they are all lowercase.

[smurthys]

The test scripts needs to have two sections:

In section 1 ("nominal section"):

• Revise existing creation calls to leave out the password parameter to test the API that way it should be used
• Check passwords against ClassDB.foldPgID(username), because the default pwd policy implemented in function createRole sets ClassDB.foldPgID(username) as the pwd

In section 2:

• Create a student, instructor, and dbman with a custom pwd and expect to see warnings only for these three creation attempts. Nothing in section 1 should cause a warning
• Check password as in section 1 to verify that the new API ignores custom pwd
• This section should be clearly marked and set off from section 1 so it can be removed easily in the next release (when the pwd parameter will be removed)

[smurthys]

After reading the error-message guide @afig helpfully provided in another thread, I feel the following code is better to raise warning:

RAISE WARNING 'parameter "initialPwd" ignored and password set to default value'
      USING DETAIL = 'Parameter "initialPwd" is deprecated and will be dropped in the next release. Please update your code.',
      HINT = 'Consult the API documentation for details on the password policy.';

[KevinKelly25]

With the new commits the new format for testClassDBRolesMgmt.sql follows what @smurthys suggested above. The warning has also been moved to to be at the end of the functions.

The code has been tested and results match what is expected. IE, Section 1 of the tests runs smoothly and without warnings. Section 2 tests each CreateXYZ function with a non-Null password and then checks that each test role was actually set to the ClassDB.FoldPgID(username). This also creates 3 warnings which is intended for each function.

Also, Section 2 is clearly marked at the end of the test script so that it can be easily removed when no longer needed.

This PR is ready for reviews.

[smurthys]

Thanks for the changes @KevinKelly25. They look good.

If it is not too much effort, would you please change the hint text as follows? Consult ClassDB documentation for details on password policy.

I like the new section in testClassDBRolesMgmt.sql and have the following observations:

• The section can benefit from a comment that it should be removed when parameter intiailPwd is removed from functions createXYZ
• L1001 has an extra leading space
• L1018-L1029: Remove this code because the later ROLLBACK does that automatically. Such code elsewhere in this script should also be removed, but that is not the focus of this PR. Instead, I suggest focusing on getting just the new code right.
• L1035 should be RAISE INFO instead of SELECT. L976 provides a model.
• Consider renaming the function defaultPasswordTest to something like rejectCustomPasswordTest to better communicate intent.
• I recommend using more unique role names in calls to createXYZ, for example, testStuCustomPwd

[afig]

This PR is looking pretty good, thanks @KevinKelly25.

In addition to the comments made by @smurthys , I would only add that we may want to slightly change the description of section 2, since the tests are not actually verifying that a WARNING is raised, only that the passwords are rejected. Currently it reads:

tests each of the createXYZ functions to make sure that if supplied with a password that the function raises a warning and sets the password to the default

[KevinKelly25]

Thank you @smurthys and @afig for the suggestions. I addressed those issues in the last commits.

[KevinKelly25]

Thanks @afig, fixed that small issue

[smurthys]

Thanks @KevinKelly25 for your persistence,

First, thank you for updating the contributor list in testClassDBRolesMgmt.sql. I apologize I did not recognize the need earlier.

I think testClassDBRolesMgmt.sql will be polished with the following changes:

• Usie something other than wrong name for full name. I realize preexisting sections of the script also use wrong name as full name, but in a different context.
• L1013-L1020: Revise the code as follows (I am only showing the structure, not content or format):

IF a AND b AND c  THEN --a, b, c are proxies for existing function calls
   RETURN `PASS`;
ELSE
   RETURN `FAIL`;
END IF;

Other than these observations, this PR looks pretty good.

[KevinKelly25]

@smurthys thank you for the comments. They will be included in the next commit.

I know that section 2 is just temporary until a later update to the createXYZ functions. However, after running the test script many times I wonder if it may be better to RAISE a notice at the beginning of the pg_temp.rejectCustomPasswordTest()?

This way the user knows that the 3 warnings are normal and there is less chance of a user misunderstanding the test and thinking that is may have failed in some way.

Adding something along the lines of: RAISE NOTICE 'The following test should RAISE three warnings';

[smurthys]

I like the idea of raising a notice as @KevinKelly25 suggests. I wonder if others sees a downside.

[smurthys]

Just curious if @afig or @srrollo have any thoughts on raising a notice as @KevinKelly25 suggests.

[afig]

I think raising a NOTICE is a good idea. The sample message provided looks pretty good, I would just add something that states the NOTICEs should be related to password rejection.

So adapting @KevinKelly25's examples, something like:

RAISE NOTICE 'The following test should RAISE three warnings regarding ignoring of an initial password';

[wildtayne]

I also think the notice would be helpful, similar to what @afig suggested.

[KevinKelly25]

Thank you for the feedback. I added the notice to the last commit.

[wildtayne]

I think the changes look good. Just want to re-afirm my approval.

[afig]

Looks good. I can't test right now, but not sure if a space is needed at the beginning of the second line of the warning. I think the concatenation does not insert a space, so when issued, the warning will print out regardingignoring as one word.

[KevinKelly25]

@afig thank you for the catch! I didn't notice that when testing.