Ouranosinc / Magpie

AuthN/AuthZ services
https://pavics-magpie.readthedocs.io
Apache License 2.0
1 stars 5 forks source link

do not allow conflicting usernames where only difference is case #596

Closed mishaschwartz closed 10 months ago

mishaschwartz commented 10 months ago

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[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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 ```diff @@ 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](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/596?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc) | Coverage Δ | | |---|---|---| | [magpie/\_\_meta\_\_.py](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/596?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc#diff-bWFncGllL19fbWV0YV9fLnB5) | `100.00% <100.00%> (ø)` | | | [magpie/api/exception.py](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/596?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc#diff-bWFncGllL2FwaS9leGNlcHRpb24ucHk=) | `92.45% <100.00%> (ø)` | | | [magpie/api/management/user/user\_utils.py](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/596?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc#diff-bWFncGllL2FwaS9tYW5hZ2VtZW50L3VzZXIvdXNlcl91dGlscy5weQ==) | `97.23% <100.00%> (ø)` | | | [magpie/models.py](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/596?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc#diff-bWFncGllL21vZGVscy5weQ==) | `90.12% <100.00%> (ø)` | | | [magpie/ui/utils.py](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/596?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc#diff-bWFncGllL3VpL3V0aWxzLnB5) | `73.74% <100.00%> (+2.35%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/596/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fmigneault commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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: https://github.com/bird-house/birdhouse-deploy/pull/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 commented 10 months ago

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 commented 10 months ago

Closed in favour of #597