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

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

@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?

@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.

Yeah, I know. But AFAIU we effectively overload "username" and "email". I am not sure we can unilaterally decide what email addresses look like.

How would you suggest to proceed? Changing to base64 for instance break API compatibility too

Ooh, here's another one: We probably need to support backslashes, commas and other strange characters in order to support LDAP Distinguished Names.

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.

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).

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)

The problem isn't with react-router-dom, it's with the history package

Are we talking about this PR?

@Ariel Shaqed (Scolnicov) One of several discussing the subject

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?

true

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.

Because the issue is in pushing a new location to the history API If you navigate to it manually, that doesn't happen

Worse... That url works because of a double decode - I specifically doubled up %2525 to get there.

Oh, I see... sorry, missed that

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?