Fix for escape_userdn

[hammadab]

Solution to #225

When escape_userdn = True the ldapauthenticator escapes special chars in userdn but does not escapes special chars in username. This does not cause an issue when allowed_groups is null but it does cause an issue when allowed_groups is not null. I suggest that the username is also escaped when escape_userdn = True or add another parameter dedicated to escape username

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[hammadab]

@consideRatio Kindly review this PR

[Nikolai-Hlubek]

We have this issue as well. Would it be possible to review this PR and if applicable merge it?

[manics]

I'm not sure whether the behaviour implemented in this PR is to be expected or not. Based on the function name resolve_username it seems reasonable for it to return the unmodified username, and that the username should be escaped wherever it's used.

[Nikolai-Hlubek]

Hi @manics Thank you for the response. To explain a bit more, I think we have a similar issue in our organization as the OP.
We have a distinguishedName that contains a special characters with something like:
CN=David Hasseloff (The One and Only)
Due to the current escape settings, when this user logs in, he gets a 500: internal server error. I think this PR might be a fix for this issue, but I don't know if it causes other issues or is well designed for the module.

[consideRatio]

Hey! I've never used this authenticator or worked properly with LDAP, so reviewing this is really complicated for me.

A key question in my mind: is this a breaking change? It is if any usernames that previously worked are changed, but it isn't if only usernames that previously failed now no longer fails. Is this a breaking change? Its perfectly acceptable if it is, but if it is we need to communicate about it in a changelog and release a major version if it is.

I'm just arriving to this project to do some chore maintenance to consider if its viable to use against jupyterhub 5 etc, and I'm not confident about the test suite coverage of this project. But anyhow, if this could go hand in hand with a test, that would be helpful.

[consideRatio]

I've preliminary marked this as breaking just in case.

[hammadab]

@consideRatio Yes, this is not a breaking change. I've been using this branch on production for over a year and have faced no issues. Before this fix, many users couldn't log in if there was a parenthesis in the full name (in case of duplicate names AAD automatically adds a hash in parenthesis).

[consideRatio]

Thank you @hammadab!!

I think #225 may be resolved by #238 that is now merged, and this PR could be closed. Are you able to confirm #225 is resolved already in the main branch @hammadab?

[consideRatio]

I think maybe a bug remains still, considered in #243 (comment).

[consideRatio]

Closing this in favor of #267