No input validation for role names or schema names (M)

[afig]

Although our docs point to Postgres's docs for SQL identifiers, it is placed in a context that discusses case sensitivity and quoting, rather than valid characters.

Although it may not seem like a major issue, it is worth noting that the resulting error messages can be confusing or misleading. More importantly, it is possible for a role to be incorrectly recorded in the RoleBase table, such as in example 4, which is shown below.

A solution to this issue would likely involve two parts:

• Update documentation to inform users about allowed characters

• Verify that the role and schema name given to ClassDB by a user is a valid SQL identifier, and return a clear error message if it is not

Here are a few examples of invalid identifiers and their resulting messages:

1)
classdb=# SELECT ClassDB.createStudent('test.stu', 'Period in middle');
ERROR:  syntax error at or near "."
LINE 1: CREATE USER test.stu ENCRYPTED PASSWORD 'test.stu'
...
2)
classdb=# SELECT ClassDB.createStudent('Test stu', 'Space in middle');
ERROR:  unrecognized role option "stu"
LINE 1: CREATE USER Test stu ENCRYPTED PASSWORD 'test stu'
...
3)
classdb=# SELECT ClassDB.createStudent('TestStu ADMIN classdb', 'Relative to Little Bobby Tables');
ERROR:  syntax error at or near "ADMIN"
LINE 1: ALTER ROLE TestStu ADMIN classdb LOGIN
...
4)
classdb=# SELECT ClassDB.createStudent('teststu
classdb'# ', 'Newline at end');
 createstudent
---------------

(1 row)

classdb=# SELECT * FROM classdb.RoleBase;
 rolename |    fullname    | isteam | schemaname | extrainfo
----------+----------------+--------+------------+-----------
 teststu +| Newline at end | f      | teststu   +|
          |                |        |            |
(1 row)

classdb=# SELECT * FROM ClassDB.User;
ERROR:  role "teststu
" does not exist
CONTEXT:  SQL function "ismember" statement 1
PL/pgSQL function classdb.isinstructor(classdb.idnamedomain) line 3 at IF

classdb=# SELECT Classdb.dropStudent('teststu
classdb'# ');
NOTICE:  User "teststu
" is not defined in the server
 dropstudent
-------------

(1 row)

Notes:

Example 3: The role was in-fact temporarily created and had ADMIN privileges on the classdb role, however, the creation of the role is undone by an error that occurs later, when assigning LOGIN to the role. This error is raised by code at L274 of addRoleBaseMgmtCore.sql. This is not a major security issue since only trusted users can run createXYZ

Example 4: This is one of the more likely scenarios, where the user accidentally hits the enter key instead of '. Unfortunately, it is one that temporarily "breaks" the system, since the User view (along with all dependent views and functions) cannot be used while this user is registered in the RoleBase table. Dropping the student in the manner shown above does remove the broken user from the RoleBase table, repairing the system. However, that solution is not very obvious, and leaves a 'teststu' role defined in the server.

~However, this "breakage" may be considered a separate issue, where having a role in RoleBase that no longer exists in the server breaks the system.~

A separate issue was opened for this last point (#179). As a result, I have moved the priority of this issue down to low.

[smurthys]

Thanks for the excellent analysis @afig. The situation is Example 4 is a particularly hard because the user can so easily cause it.

I wonder if we could test unquoted identifiers against Postgre's rules, even if not for all possibilities.

[afig]

I agree that testing identifiers is the way to go. Given that the requirements are relatively restrictive, but clear, this should not be too big of a task. However, I do not think that it is necessary to implement the validation now. The documentation should likely be clarified before the release, however.

[smurthys]

I propose we make only the documentation changes for M3.

[afig]

Additional links to Postgres' rules for SQL identifiers were added to pages on adding users and teams near where they are more likely to be seen. I believe this change will be enough to remove it from the M3 designation.

[afig]

Removing my assignment for now (should have been removed on June 22 2018 when this issue was removed from M3). I believe that this still remains an issue, but not one that should be addressed now (in M4).