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.

Sign up for GitHub

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

Jump to bottom

[15.10] Checks for api_key before checking for header from SSO. #1801

Merged
merged 3 commits into from
Mar 22, 2016
Merged

[15.10] Checks for api_key before checking for header from SSO. #1801

merged 3 commits into from
Mar 22, 2016

Conversation

MatthewRalston
Copy link
Contributor

This pull request addresses an issue #972 found when using the Galaxy API and bioblend when also using an SSO. The principle issue was that SSO headers are checked before API keys are authenticated. This led to API/bioblend requests failing, as no headers are passed through bioblend. The solution was to check if the request was API-related before headers are checked; API-key validation is done downstream of this process. In the process, another issue was found when Galaxy receives no headers (i.e. accessing the port directly: http://server.example.com:1234, instead of the appropriate url http://galaxy.example.com). When no headers are passed, the Remoteuser class compares None to the GX_SECRET defined in the config. The received error is reported below. This is also handled by this patch.

File '.../galaxy-v15.05-production/eggs/WebError-0.8a-py2.7.egg/weberror/evalexception/middleware.py', line 364 in respond
  app_iter = self.application(environ, detect_start_response)
File '.../galaxy_env/lib/python2.7/site-packages/paste/recursive.py', line 84 in __call__
  return self.application(environ, start_response)
File '.../galaxy-v15.05-staging/lib/galaxy/web/framework/middleware/remoteuser.py', line 78 in __call__
  if not safe_str_cmp(environ.get('HTTP_GX_SECRET'), self.config_secret_header):
File '.../galaxy-v15.05-staging/lib/galaxy/util/__init__.py', line 1244 in safe_str_cmp
  if len(a) != len(b):
TypeError: object of type 'NoneType' has no len()

nsoranzo
nsoranzo reviewed Feb 25, 2016
View reviewed changes
lib/galaxy/web/framework/middleware/remoteuser.py
return self.app( environ, start_response )
elif self.config_secret_header is not None:
gxSecretIsNone = environ.get('HTTP_GX_SECRET') is None
if gxSecretIsNone or not safe_str_cmp(environ.get('HTTP_GX_SECRET'), self.config_secret_header):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An easier way to fix the NoneType problem is:

if not safe_str_cmp(environ.get('HTTP_GX_SECRET', ''), self.config_secret_header):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nsoranzo I like it.

@nsoranzo nsoranzo changed the title Checks for api_key before checking for header from SSO. [15.10] Checks for api_key before checking for header from SSO. Feb 26, 2016
nsoranzo
nsoranzo reviewed Feb 26, 2016
View reviewed changes
lib/galaxy/web/framework/middleware/remoteuser.py
# Check for API key before checking for header
return self.app( environ, start_response )
elif self.config_secret_header is not None:
if not safe_str_cmp(environ.get('HTTP_GX_SECRET',''), self.config_secret_header):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing a space between 'HTTP_GX_SECRET' and ''.

@mvdbeek
Copy link
Member

mvdbeek commented Mar 11, 2016

@galaxybot test this

@dannon
Copy link
Member

dannon commented Mar 14, 2016

This works for me, but can someone else with a testbed for this verify?

@hexylena hexylena self-assigned this Mar 14, 2016
@hexylena
Copy link
Member

Will test as soon as I'm free

@hexylena
Copy link
Member

Ping me if I haven't done this in 48h, @dannon

@dannon
Copy link
Member

dannon commented Mar 21, 2016

@erasche Ping (per your request).

@martenson
Copy link
Member

@galaxybot test this

@dannon
Copy link
Member

dannon commented Mar 22, 2016

@martenson Tests will continue to fail against 15.10 I think; this one needs manual validation.

@martenson
Copy link
Member

@dannon correct, I missed the target branch, poor Jenkins :/

@hexylena
Copy link
Member

@dannon testing now. Thanks for reminder :)

@dannon
Copy link
Member

dannon commented Mar 22, 2016

@erasche Thanks!

@hexylena
Copy link
Member

👍 from me

@dannon
Copy link
Member

dannon commented Mar 22, 2016

Hooray, thanks.

dannon added a commit that referenced this pull request Mar 22, 2016
@dannon
Merge pull request #1801 from MatthewRalston/api_key_patch
a2114fd 
[15.10] Checks for api_key before checking for header from SSO.
@dannon dannon merged commit a2114fd into galaxyproject:release_15.10 Mar 22, 2016
@dannon
Copy link
Member

dannon commented Mar 22, 2016

(and thanks @MatthewRalston for the patience with this one)

@MatthewRalston
Copy link
Contributor Author

Thanks everyone. @golharam ping.

@galaxybot
Copy link
Contributor

This PR was merged without a milestone attached.

@martenson martenson added this to the 16.04 milestone Apr 11, 2016
@carlfeberhard carlfeberhard mentioned this pull request Apr 14, 2016
Merged
@dannon dannon mentioned this pull request Apr 19, 2016
Merged
@dannon dannon mentioned this pull request Dec 14, 2016
Closed
@hexylena hexylena mentioned this pull request Sep 14, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@hexylena hexylena

Labels
area/framework kind/bug
Projects
None yet
Milestone
16.04
Development

Successfully merging this pull request may close these issues.
7 participants
@MatthewRalston @mvdbeek @dannon @hexylena @martenson @galaxybot @nsoranzo