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 an admin form to pre-populate an ldap user #5238

Merged
merged 10 commits into from Jan 20, 2018

Conversation

Projects
None yet
4 participants
@scholtalbers
Contributor

scholtalbers commented Dec 20, 2017

Includes PR #5136 and that could therefor be closed.

With the added galaxy option show_prepopulate_form the following form behavior is shown:

screen shot 2017-12-20 at 21 07 44

⬇️

screen shot 2017-12-20 at 21 09 41

⬇️

screen shot 2017-12-20 at 21 51 01

@scholtalbers scholtalbers changed the title from On top of #5129 this adds an admin form to pre-populate an ldap user to On top of #5136 this adds an admin form to pre-populate an ldap user Dec 20, 2017

@martenson martenson changed the title from On top of #5136 this adds an admin form to pre-populate an ldap user to add an admin form to pre-populate an ldap user Dec 20, 2017

@martenson martenson added this to the 18.01 milestone Dec 20, 2017

@martenson

This comment has been minimized.

Member

martenson commented Dec 21, 2017

@galaxybot test this

@scholtalbers

This comment has been minimized.

Contributor

scholtalbers commented Dec 22, 2017

@martenson anything I can do to speed up a review/possible merge - i.e. are there any major things that should be changed to consider this and the related PR?

@martenson

This comment has been minimized.

Member

martenson commented Dec 22, 2017

@scholtalbers It seems fine to me but I do not know much about LDAP. It is holiday season so it might take some time to get this merged. Are you trying to get this into 18.01?

@scholtalbers

This comment has been minimized.

Contributor

scholtalbers commented Dec 22, 2017

I would like to get it in as quick as possible 😉 - to avoid having my own production instance code base too diverged from main. But I am aware of the holiday season🎅🏿 and due to the changes to the login system it may require some thorough review. Basically wanted to know if anything obvious was wrong with this 🙂

@martenson

This comment has been minimized.

Member

martenson commented Dec 22, 2017

@scholtalbers Are you running dev in production? I cannot guarantee it will get to 18.01 but I will try to help making it happen.

@scholtalbers

This comment has been minimized.

Contributor

scholtalbers commented Dec 22, 2017

No, running 17.09 at the moment actually - but things can change..
Thanks @martenson!

@scholtalbers

This comment has been minimized.

Contributor

scholtalbers commented Jan 2, 2018

Maybe this can help a reviewer: https://github.com/scholtalbers/docker-galaxy-openldap
No automated test, and not ideal (e.g. the .ldif can certainly be extended), but its a start.

@martenson martenson requested a review from natefoo Jan 17, 2018

@natefoo

Thanks @scholtalbers! See also my comments in #5136, which I guess we can close in favor of this one.

@@ -1045,6 +1045,10 @@ use_interactive = True
# Allow administrators to log in as other users (useful for debugging)
#allow_user_impersonation = False
# When using LDAP for authentication, allow administrators to pre-populate users
# using an additional form on 'Create new user'
#show_prepopulate_form = False

This comment has been minimized.

@natefoo

natefoo Jan 18, 2018

Member

Maybe show_user_prepopulate_form just to make it a bit more domain specific.

return authenticators
def parse_auth_results(trans, auth_results, options):

This comment has been minimized.

@natefoo

natefoo Jan 18, 2018

Member

Moving this to a function I guess allays some of my concerns about this data structure raised in #5136

"""
Do the actual authentication by binding as the user to check their credentials
"""
import ldap

This comment has been minimized.

@natefoo

natefoo Jan 18, 2018

Member

Typically we put this at the top with the regular imports:

try:
    import ldap
except ImportError as exc:
    ldap = None
    ldap_import_exc = exc

And then here you'd do something like:

if ldap is None:
    raise RuntimeError("Failed to load LDAP module: %s", str(ldap_import_exc))
@scholtalbers

This comment has been minimized.

Contributor

scholtalbers commented Jan 19, 2018

Thanks for the comments @natefoo - hope I mitigated (most of) your concerns.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 19, 2018

Thanks! I went ahead and did the merge to resolve the conflict since that was essentially my doing in the first place.

+1 from me, just going to wait for tests.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 19, 2018

@galaxybot test this

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 20, 2018

The failing tests are unrelated and have been fixed in dev, so I'll go ahead and merge this. Thanks a lot @scholtalbers, this is pretty cool. We should make use of the docker container for some integration tests in the future.

@mvdbeek mvdbeek merged commit 42b87dd into galaxyproject:dev Jan 20, 2018

5 of 6 checks passed

framework test Build finished. 171 tests run, 0 skipped, 2 failed.
Details
api test Build finished. 343 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
integration test Build finished. 79 tests run, 4 skipped, 0 failed.
Details
selenium test Build finished. 118 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@scholtalbers

This comment has been minimized.

Contributor

scholtalbers commented on lib/galaxy/security/__init__.py in 8be4add Feb 20, 2018

@natefoo should this actually be type = type or self.model.Role.types.SYSTEM

I'm now actually receiving errors on my side where some Roles where manually created, and then they are of type system. and now the check of get_role() or create_role() does not work..

This comment has been minimized.

Member

natefoo replied Feb 20, 2018

Yes, good catch, I believe you're right. Also it could just be the default in the method signature.

martenson added a commit to martenson/galaxy that referenced this pull request May 1, 2018

revert the erroneous rewrite to call security
TS security does not have this method
introduced in galaxyproject#5238
else:
num_in_groups = len(in_groups)
trans.sa_session.flush()
role, num_in_groups = trans.app.security_agent.create_role(

This comment has been minimized.

@martenson

martenson May 1, 2018

Member

Toolshed does not have this method in its security agent (which is different than Galaxy's). Should be fixed in 40d0d99

@scholtalbers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment