Ensure that user_name values for all User are lowercase and do not contain whitespace

[mishaschwartz]

Ziggurat foundations assumes that a User will not have a user_name that differs from another only in terms of case. The simplest way to enforce this is to ensure that all user_name values are lowercase.

Previously, this was not enforced so we could create two User which could not be differentiated properly.

This change includes a database migration that will convert all user_name that contain uppercase characters to lowercase. This may cause a database conflict if there are two user_name values that differ only in terms of case. For example "Test" and "test". If this occurs, please manually update those user_name values to no longer conflict and try the migration again.

This also prevents new users from being created that contain whitespace.

Replaces #596
Resolves #595

[fmigneault]

Looks good. Only minor potential patch, but not blocking.

[fmigneault]

I think this could be done with a single query:

session.execute(users.update().values({users.c.id: func.lower(users.c.id)})

[tlvu]

Oh, I only noticed this PR now. Great ! More harmonization with JupyterHub {username}.

Question about the DB migration, if it encounters existing username with uppercase and try to convert to a lowercase version, will it display which users were converted so the node admin can notify the users impacted they have to now use lowercase to login?

What if the conversion clashes with existing lowercase version?

[fmigneault]

@tlvu

will it display which users were converted so the node admin can notify the users impacted they have to now use lowercase to login?

It won't display it (in the current state).
That could be a good reason to keep the for user in session.execute(sa.select(users)): approach in the migration script. A LOGGER.warning could be employed in the loop to clearly indicate it to the admin within pavics-compose logs -f magpie.
Warning would be used only if user.user_name != user.user_name.lower() to avoid being unnecessarily noisy.

What if the conversion clashes with existing lowercase version?

The migration will purposely crash. Magpie will not be able to boot, and therefore the rest of the stack also.
It would be up to the admin to investigate and decide how they want to remedy the situation (inform the user of a rename, delete, etc.).

[tlvu]

What if the conversion clashes with existing lowercase version?

The migration will purposely crash. Magpie will not be able to boot, and therefore the rest of the stack also. It would be up to the admin to investigate and decide how they want to remedy the situation (inform the user of a rename, delete, etc.).

I assume when Magpie crashes, it will display/log the conflicting existing username causing the crash and the matching uppercase version?

[fmigneault]

I assume when Magpie crashes, it will display/log the conflicting existing username causing the crash and the matching uppercase version?

Magpie will report the error to boot after the configured number of DB connect/migration retries (default 5).
I assume sqlalchemy will report some kind of error being unable to update the value due to the "no duplicate" index rule.
However, the migration script itself could provide a log call to be more explicit about the known potential issue and how to address it.

[mishaschwartz]

@tlvu @fmigneault

Since the case insensitive unique index is created first (before existing usernames are converted to lowercase). Alembic will report an error indicating which values are in conflict before any conversion happens.

This will give the user a good idea of which names are in conflict and they can update the values manually. I've added a comment in the migration script as well with this information and added additional information to the error message.

[tlvu]

Since the case insensitive unique index is created first (before existing usernames are converted to lowercase). Alembic will report an error indicating which values are in conflict before any conversion happens.

This will give the user a good idea of which names are in conflict and they can update the values manually. I've added a comment in the migration script as well with this information and added additional information to the error message.

@mishaschwartz Thanks for logging the error when migration fails.

will it display which users were converted so the node admin can notify the users impacted they have to now use lowercase to login?

It won't display it (in the current state). That could be a good reason to keep the for user in session.execute(sa.select(users)): approach in the migration script. A LOGGER.warning could be employed in the loop to clearly indicate it to the admin within pavics-compose logs -f magpie. Warning would be used only if user.user_name != user.user_name.lower() to avoid being unnecessarily noisy.

When migration succeeds, please also log which users were migrated so the node admin can notify those users to use lowercase instead of uppercase.

[mishaschwartz]

When migration succeeds, please also log which users were migrated so the node admin can notify those users to use lowercase instead of uppercase.

Ok sounds good. Done now

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (436c7f9) 80.92% compared to head (427e724) 80.92%.
Report is 9 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #597   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files          73       73           
  Lines       10194    10196    +2     
  Branches     1824     1824           
=======================================
+ Hits         8249     8251    +2     
  Misses       1622     1622           
  Partials      323      323           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.