Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc cleanup + fixes #95

Merged
merged 1 commit into from
Aug 6, 2018
Merged

Conversation

dhirschfeld
Copy link
Collaborator

@dhirschfeld dhirschfeld commented Jul 19, 2018

This cleans up the code a bit and as such helps clarify the underlying logic. In the process I've hopefully resolved a couple of issues:

- Fixes jupyterhub#44
  Empty DN templates are now ignored
- Fixes jupyterhub#93
  search_filter and allowed_groups are no longer mutually exclusive
@dhirschfeld dhirschfeld requested a review from yuvipanda July 19, 2018 06:45
@dhirschfeld
Copy link
Collaborator Author

@djrobstep, @ermakovpetr - it would be great if you could verify if this branch fixes the problem you observed in #44

@dhirschfeld
Copy link
Collaborator Author

@mfriedemann - it would be great if you could verify if this branch fixes the problem you had in #93 or clarify what else needs to be done to support your usecase.

@ermakovpetr
Copy link

@dhirschfeld

it would be great if you could verify if this branch fixes the problem you observed in #44

ok! I will definitely look in the coming days

@djrobstep
Copy link

@dhirschfeld cool, i'll give it a look when I can

@mfriedemann
Copy link

mfriedemann commented Jul 19, 2018

@dhirschfeld
I just took a look at the patch, and while it now handles search and the group check sequentially, I think it will still not work for our case, as it expects the userdn (which it formats against the (list of) string(s) configured via bind_dn_template) to be a DN and later uses it for the group filter as-is (member={userdn}).

As hinted at earlier, we bind to AD, but aren't keen on using a technical account to do it. Instead, we take the username that is entered (basically the sAMAccountName), and by using the bind_dn_template to append an '@' and the AD domain, we construct the userPrincipalName (which looks like an email address, and is not a DN). For AD, you can authenticate the user with this userPrincipalName and the password. Note that in this case, you haven't gotten a DN for the user, yet, you would need to do a search first.
But neither the current nor the prior version actually update the userdn variable from the search results.

FWIW, I added the group check to the search filter (luckily, AD represents group membership on both ends) and made it work that way.

HTH,
M.

@dhirschfeld
Copy link
Collaborator Author

In the interests of moving forward I'll merge then iterate...

@dhirschfeld dhirschfeld merged commit 145fc55 into jupyterhub:master Aug 6, 2018
@dhirschfeld dhirschfeld deleted the misc-fixes branch August 6, 2018 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants