Title
#dev
Or Tzabary

Or Tzabary

09/05/2022, 1:10 PM
hi all, I started working on the failure in loading a user with % in his name issue. first, I thought that we're missing encoding somewhere, but going deeper into the issue, it seems that it's something broken with react's history library and how it works with percent signs in the URL. Encoding this using
encodeURIComponent
or
encodeURI
doesn't help as % is used as part of the encoded value. Another approach is to encode using base64 which is URL-safe. I believe the right approach would be to disallow some characters for user/group/policy names as we use them in the URL and to avoid other potential bugs. Comparing to AWS IAM's restrictions, they allow only specific characters to be used in the (alphanumeric and '+=,.@-_' characters.) I think that restricting to these characters is a the right approach to take. this means it'll be a breaking change in case someone used % in previous versions, but to be honest, the UX was already broken with entries with % in its name. Thoughts? @Elad Lachmi thanks for the help
e

Elad Lachmi

09/05/2022, 1:26 PM
The maintainers of the history lib believe that this is the correct handling for this case, so fixing it would mean forking the history lib and maintaining it on our own. I don't believe that's good ROI for the time spent now and in the future. Disallowing some characters is a good compromise IMHO
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 1:40 PM
@Elad Lachmi if we do something like this it must happen on the auth server side. Otherwise the cli and even the api will still create these bad usernames. There's also some backwards compatibility issue: What do we do about existing usernames?
1:53 PM
Oh, and I expect some users will expect internationalised local-parts to work in their usernames. Those probably won't need a percent sign... but if its broken for esoteric ASCII characters I would verify that this library can handle common non-ASCII characters.
Or Tzabary

Or Tzabary

09/05/2022, 2:06 PM
@Ariel Shaqed (Scolnicov) I agree that it should be implemented on server-side. Regarding i18n, IDK if that's something we really need to support, if we follow the AWS IAM principles, they don't support i18n either.
2:06 PM
This will be a breaking change going forward for new users/group/policies, not for existing ones, that probably still have issues displaying those resources through the UI
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 2:07 PM
Yeah, I know. But AFAIU we effectively overload "username" and "email". I am not sure we can unilaterally decide what email addresses look like.
Or Tzabary

Or Tzabary

09/05/2022, 2:32 PM
How would you suggest to proceed? Changing to base64 for instance break API compatibility too
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 2:52 PM
Ooh, here's another one: We probably need to support backslashes, commas and other strange characters in order to support LDAP Distinguished Names.
Or Tzabary

Or Tzabary

09/05/2022, 2:54 PM
I don't think it's possible supporting all of these AND making our React router work with those limitations. Feels like a refactor is needed in the auth package.
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 2:54 PM
Alternatives: Do the React library people say what to do and how to handle it? Note that GUI <-> daemon upgrades are the easiest to get right, because you don't need as much backwards compatibility (the daemon servers the GUI).
Or Tzabary

Or Tzabary

09/05/2022, 2:57 PM
This doesn't solve the CLI/API missing encoding (which will break compatibility if we introduce it) Also, IDK if that's a good practice to take (forking a library to patch it only for us while the maintainers of the original package wouldn't fix it)
e

Elad Lachmi

09/05/2022, 2:58 PM
The problem isn't with react-router-dom, it's with the history package
2:59 PM
It's also not an issue with every escaped character, only
%25
specifically
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 2:59 PM
Are we talking about this PR?
e

Elad Lachmi

09/05/2022, 3:04 PM
@Ariel Shaqed (Scolnicov) One of several discussing the subject
3:05 PM
Here's another
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 3:09 PM
I'm confused. Using Firefox, I created a user named
a%percent
in my playground. When I go to that user, I see this JavaScript error in the console:
Uncaught URIError: Pathname "/auth/users/a%percent/groups" could not be decoded. This is likely caused by an invalid percent-encoding.
And indeed the URL in the browser is broken: https://apparent-panther.lakefs-demo.io/auth/users/a%percent. Is this an error in the history library and not in our encoding of the path?
Or Tzabary

Or Tzabary

09/05/2022, 3:09 PM
true
3:09 PM
encoding of % is %25
3:09 PM
which still contains %
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 3:11 PM
But if I manually surf to https://apparent-panther.lakefs-demo.io/auth/users/a%2525percent/groups it does find the user (still displays their name incorrectly on the page). Weird.
e

Elad Lachmi

09/05/2022, 3:13 PM
Because the issue is in pushing a new location to the history API If you navigate to it manually, that doesn't happen
Ariel Shaqed (Scolnicov)

Ariel Shaqed (Scolnicov)

09/05/2022, 3:15 PM
Worse... That url works because of a double decode - I specifically doubled up %2525 to get there.
e

Elad Lachmi

09/05/2022, 3:21 PM
Oh, I see... sorry, missed that
3:27 PM
IMHO the effort to fix this issue far outweighs the benefit of allowing the character set for user names and group names to include
%
Other characters are less of an issue and should be handled properly out-of-the-box
Or Tzabary

Or Tzabary

09/07/2022, 11:39 AM
due to the fact we can't use my original suggestion (allowing only specific characters), mainly due to the LDAP support (thanks @Ariel Shaqed (Scolnicov) for bringing that up), besides of refactoring the whole user-management and supporting different authentication methods and restrictions, I think that in order to make a progress with this ticket, I can't either disallow the use of '%' only and open another ticket to refactor the user-management. thoughts on this?