do not allow conflicting usernames where only difference is case

[mishaschwartz]

Do not allow users to have the same username that only differs in terms of upper/lowercase
Ziggurat foundations assumes that users will not have usernames that differ in terms of case.
This means that if the database contains a user with the username "Test" and another with the username "test". The login procedure will not know to differentiate the two.

Resolves #595

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
+ Coverage   80.92%   81.01%   +0.09%     
==========================================
  Files          73       73              
  Lines       10194    10194              
  Branches     1824     1824              
==========================================
+ Hits         8249     8259      +10     
+ Misses       1622     1616       -6     
+ Partials      323      319       -4     

Files	Coverage Δ
magpie/__meta__.py	100.00% <100.00%> (ø)
magpie/api/exception.py	92.45% <100.00%> (ø)
magpie/api/management/user/user_utils.py	97.23% <100.00%> (ø)
magpie/models.py	90.12% <100.00%> (ø)
magpie/ui/utils.py	73.74% <100.00%> (+2.35%)	⬆️

... and 1 file with indirect coverage changes

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

[fmigneault]

The valid user name regex should be updated to directly disallow uppercase.
We shouldn't have to go back everywhere to apply lower().

There should also be a new migration script (see the makefile migrate/revision target to generate the next script) that applies lower() to any existing user name just to make sure.

Finally, I propose that another more explicit message is added to check_user utility to verify if user_name != user_name.lower() and raise the relevant error that will better guide the registering user why their user name is considered invalid.

[fmigneault]

We must avoid tweaking the values. That can give timing attack information. If lowercase is enforced anyway from the next version, that will not be necessary.

[mishaschwartz]

see #596 (comment)

[fmigneault]

Not needed here. Enforce it directly from the DB/API.

[mishaschwartz]

What is the point of these checks then?

[fmigneault]

The best would be to avoid duplicating the logic in the UI and leave all validations on the API side that can report more specific error messages.
However, given how small this change is, it is fine to leave it as is.
Disregard my previous comment.

[fmigneault]

Use format `User` and ``user_name`` as employed for the rest of the docs.

Missing dot at the end of the first line.
Move the (fixes ...) definition after the first line (after upper/lowercase), no dot in between.

Move under section

Bug Fixes
~~~~~~~~~~~~~~~~~~~~~

[fmigneault]

@tlvu
Just FYI to validate on your side if this is already applied in your platforms.
The (eventual) migration script could break deployment if there is a conflicting case-insensitive user name. In the meantime, if there are such users, they probably get funky responses anyway.

[tlvu]

@tlvu Just FYI to validate on your side if this is already applied in your platforms. The (eventual) migration script could break deployment if there is a conflicting case-insensitive user name. In the meantime, if there are such users, they probably get funky responses anyway.

@fmigneault Thanks for the heads up. Due to the sanitized {username} from JupyterHub, we do not have duplicate capital case in our usernames.

[mishaschwartz]

The valid user name regex should be updated to directly disallow uppercase.

I'm very confused now. The whole point of the strategy that you suggested here: bird-house/birdhouse-deploy#396 (especially overriding the normalize_username function) was to allow there to be usernames with uppercase characters.

If we now want to disallow all uppercase characters here, we could have avoided making those changes in the first place.

We shouldn't have to go back everywhere to apply lower().

This is what ziggurat foundations does, if we want to conform with that code then we have to ensure that we're matching names in a case-insensitive way.

[mishaschwartz]

Finally, I propose that another more explicit message is added to check_user utility to verify if user_name != user_name.lower() and raise the relevant error that will better guide the registering user why their user name is considered invalid.

The easiest way to do this would be to just remove the soft checks here: https://github.com/Ouranosinc/Magpie/pull/596/files#diff-5de931472d59cdf71cb9fe436020658d35e0d8ec69aea8d7aeb0cd79e4314024R570

I'm not really sure why these checks are needed in the first place

[mishaschwartz]

Closed in favour of #597