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

Updated parameters check for OpenID #4039

Merged
merged 4 commits into from Aug 10, 2017

Conversation

Projects
None yet
5 participants
@VJalili
Copy link
Member

commented May 5, 2017

When you have the OpenID URL, you don't need to recreate the provider. Only create the provider, if its not already created.

Updated missing parameters check for OpenID
When you have the OpenID URL, you don't need to recreate the provider. Only create the provider, if its not already created.

@galaxybot galaxybot added the triage label May 5, 2017

@galaxybot galaxybot added this to the 17.09 milestone May 5, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented May 5, 2017

@VJalili This could use some refactoring, for sure, thanks for looking at it. At first glance, it looks like this is set to avoid a validation error -- which may be better handled if we just check for an openid_url first and set that openid_provider variable correctly. Does that make sense? I can dig deeper if not.

@VJalili

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

I totally agree on separating the provider check from the URL check. I'll add a URL check to better handle validation.

VJalili added some commits May 12, 2017

Corrected Yahoo OpenID endpoint
At the time of making this change, Yahoo OpenID endpoint has issues--It's not accessible. 
This should be checked again later.
@VJalili

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

@dannon I updated all openid_url and openid_provider checks. All are simplified to 3 checks.

I think at this point, we would better return with something like:
return trans.show_error_message( 'An OpenID provider was not specified' )

However, Since the previous flow would only update the message if neither a provider nor a provider URL is specific, then I followed the same logic in this commit. But maybe it would be better if we just return an error message saying that "a provider is not specified".

Another point: I think we should remove OpenID URL textbox and all the related parameters/arguments. Because (1) each of the providers we support has a URL associated with, so we don't need a URL provided by the user, (2) I think its a strong expectation that a user should know what the "OpenID" is, search for its discovery endpoint, and enter the endpoints in the URL textbox.

@VJalili VJalili changed the title Updated missing parameters check for OpenID Updated parameters check for OpenID May 12, 2017

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 10, 2017

I think we should remove OpenID URL textbox and all the related parameters/arguments. Because (1) each of the providers we support has a URL associated with, so we don't need a URL provided by the user, (2) I think its a strong expectation that a user should know what the "OpenID" is, search for it's discovery endpoint, and enter the endpoints in the URL textbox.

Agreed - that would be awesome. I've had a 👍 on this for a while so I'm going to go ahead and merge. Sorry for the delay.

@jmchilton jmchilton merged commit 6353704 into galaxyproject:dev Aug 10, 2017

5 checks passed

api test Build finished. 276 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 148 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@VJalili

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2017

@jmchilton Thanks.

If interested, you may have a look at this branch (it will soon be ready for a PR). This branch leverages on OAuth2.0 and OpenID Connect protocols (current trending authentication/authorization protocols) for user authentication/authorization flow.

redirect = kwd.get('redirect', '').strip()
if not redirect:
redirect = ' '
if openid_provider_obj:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 10, 2017

Member

Sorry I'm late on this!
I think that if both openid_url and openid_provider are empty, openid_provider_obj would not be defined, resulting in a NameError exception at this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.