Thread
#dev
    Or Tzabary

    Or Tzabary

    3 weeks ago
    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

    3 weeks ago
    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)

    3 weeks ago
    @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?
    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

    3 weeks ago
    @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.
    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)

    3 weeks ago
    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

    3 weeks ago
    How would you suggest to proceed? Changing to base64 for instance break API compatibility too
    Ariel Shaqed (Scolnicov)

    Ariel Shaqed (Scolnicov)

    3 weeks ago
    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

    3 weeks ago
    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)

    3 weeks ago
    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

    3 weeks ago
    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

    3 weeks ago
    The problem isn't with react-router-dom, it's with the history package
    It's also not an issue with every escaped character, only
    %25
    specifically
    Ariel Shaqed (Scolnicov)

    Ariel Shaqed (Scolnicov)

    3 weeks ago
    Are we talking about this PR?
    e

    Elad Lachmi

    3 weeks ago
    @Ariel Shaqed (Scolnicov) One of several discussing the subject
    Here's another
    Ariel Shaqed (Scolnicov)

    Ariel Shaqed (Scolnicov)

    3 weeks ago
    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

    3 weeks ago
    true
    encoding of % is %25
    which still contains %
    Ariel Shaqed (Scolnicov)

    Ariel Shaqed (Scolnicov)

    3 weeks ago
    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

    3 weeks ago
    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)

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

    Elad Lachmi

    3 weeks ago
    Oh, I see... sorry, missed that
    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

    3 weeks ago
    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?