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

add option to check allowed groups using the ldap connection of the search user #183 #185

Closed
wants to merge 5 commits into from

Conversation

tobi45
Copy link

@tobi45 tobi45 commented Nov 30, 2020

This pull request contains two things (Sorry, I know, bad practice):

  1. it adds an option to verify the membership of user being authenticated in the configured allowed_groups with the ldap connection of the search user instead of the ldap connection with the user being authenticated (default behavior) (see allowed_groups are looked up with authenticated user instead of search user #183)
  2. it adds an alternative guide and configuration files to setup a development environment using docker containers and visual studio code with the remote containers extension

Please inform me if you want separate pull requests or just one for 1).

The diff for 1) should be straight forward but the diff tools struggle a bit on the method resolve_username cause they compare their changes with the added option. Eventually a parameter was added to the method and the connection establishment was moved to method authenticate.

On the test side I decided to use mock objects for the connections and verify if the correct connection is used and the correct methods are called. This test doesn't require the ldap server running.

@welcome
Copy link

welcome bot commented Nov 30, 2020

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

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
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! 👋

Welcome to the Jupyter community! 🎉

@manics
Copy link
Member

manics commented Jan 25, 2021

Hi! Please could you update this PR so it contains just the fixes for #183 ? As mentioned in that issue we're lacking people with expertise in LDAP, but by simplifying this PR it'll at least make it slightly easier for others to review.

@tobi45
Copy link
Author

tobi45 commented Jan 26, 2021

Thanks for your quick response! I totally understand, I have no expertise with LDAP as well. As far as I understand github I am not able to change the branch (of my repository) of that pull request, thus I had to revert both commits related to the 2nd point mentioned in the description above. But I think that's somehow complicates the changesets.

I just created 2 branches in my repository targeting both points in the description individually and both base on the current master of this repository. Shall I create 2 new pull requests for a leaner commit history? Or do you prefer to continue on this one?

Regarding the failed github actions: I understand the failed lint task but I have no idea why the tests are failing. That seems to be a setup problem. Locally they all succeed.

@tobi45
Copy link
Author

tobi45 commented Jan 27, 2022

I replaced this PR with #207 and close it.

@tobi45 tobi45 closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants