search

jupyterhub / ldapauthenticator

LDAP Authenticator Plugin for Jupyter
BSD 3-Clause "New" or "Revised" License
206 stars 178 forks source link

Fix for escape_userdn #226

Closed hammadab closed 2 months ago

hammadab commented 1 year ago

Solution to https://github.com/jupyterhub/ldapauthenticator/issues/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[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. welcome 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! :wave:
Welcome to the Jupyter community! :tada:

hammadab commented 1 year ago

@consideRatio Kindly review this PR

Nikolai-Hlubek commented 4 months ago

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

manics commented 4 months ago

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 commented 4 months ago

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 commented 2 months ago

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 commented 2 months ago

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

hammadab commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

I think maybe a bug remains still, considered in https://github.com/jupyterhub/ldapauthenticator/issues/243#issuecomment-2351140539.

consideRatio commented 2 months ago

Closing this in favor of #267