search

galaxyproject / galaxy

Data intensive science for everyone.
https://galaxyproject.org
Other
1.41k stars 1.01k forks source link

404 when sharing history if username has capital letters #19087

Open laperlej opened 2 weeks ago

laperlej commented 2 weeks ago

Describe the bug Users from external auth can have usernames that are not in line with current restrictions used for routing.

Related to https://github.com/galaxyproject/galaxy/issues/16194 and https://github.com/galaxyproject/galaxy/pull/16251

From the discussions, I don't think anyone at the time realized this also affects capital letters.

This is the requirement used for routing:

https://github.com/galaxyproject/galaxy/blob/03307f660aea983c7e2574c0727b6a56fae31c65/lib/galaxy/webapps/galaxy/buildapp.py#L145

https://github.com/galaxyproject/galaxy/blob/03307f660aea983c7e2574c0727b6a56fae31c65/lib/galaxy/security/validate_user_input.py#L38

This means that it's not enough to just make the usernames valid for URL encoding purposes (see https://github.com/galaxyproject/galaxy/pull/16251)

I guess this can be fixed either by changing the code that creates the username or by loosening the requirements on the routing side.

Galaxy Version and/or server at which you observed the bug Galaxy Version: 24.1 Commit: Bug is still on dev (currently c6a10d6)

Browser and Operating System Operating System: Windows, Linux, macOS Browser: Firefox, Chrome, Chrome-based, Safari

To Reproduce Steps to reproduce the behavior:

  1. Impersonate a user with capitals in their username
  2. share or publish history
  3. toggle Make history accessible
  4. follow the link
  5. 404

Expected behavior Should see the shared history

arash77 commented 4 days ago

@mvdbeek If uppercase characters aren’t allowed in usernames, do you have suggestions for fixing this bug? One option could be to allow uppercase in /u/{username}/... URLs and use ilike for case-insensitive searches in the database. Alternatively, should we consider casting the username to lowercase from authnz before saving it to the database?

laperlej commented 4 days ago

case-insensitive searches won't work because some users have both FirstLast and firstlast accounts because of the CIlogon federated auth, there is no constraint preventing this at the moment.

I think forcing lowercase at account creation is probably the cleanest solution but I think that would require a migration.

Just changing the limitations on routing (use a different regex than user creation, or just remove it) is the quickest solution. In fact I'm not sure how necessary this pre-validation at the routing level really is.

mvdbeek commented 3 days ago

@arash77 just cast them to lowercase on creation? @laperlej append an incrementing index if these are really separate users, but I don't think they can be separate users?

laperlej commented 3 days ago

@mvdbeek I'm not sure what you mean by "separate" users in this context.

firstlast@domain1.com, firstlast@domain2.com, FirstLast@domain3.com results in 3 users in the DB: firstlast, firstlast0, FirstLast.

If you mean if they are different humans, in this example they are the same person but 2 people with the same name across all CILogon institutions is possible.

But I think the larger question is what about users that already exist if we maintain the routing restrictions. Is the username recalculated on every login? If not, I could go through my database manually but I'm pretty sure anyone else using federated auth will have a lot of already existing uppercase names in their database.