CCI-MOC / flocx-market

2 stars 9 forks source link

Keystone Authentication bugfix #79

Closed ljmcgann closed 5 years ago

ljmcgann commented 5 years ago

This commit fixes a bug introduced with the keystone authentication.The applications served when CONF.auth_enable was set to true was causing issues with the wsgi_service, which was expecting a Flask App object, as opposed to a keystonemiddleware Auth_Protocol object. TG-86

tzumainn commented 5 years ago

I'm unsure as to the usage of those flask libraries; if @larsks thinks that they're okay, then they're probably fine; otherwise I think we have to look at an alternative. One alternative may be to set a global app variable, and have a function return that?

tzumainn commented 5 years ago

It turns out there's a simpler solution: move https://github.com/CCI-MOC/flocx-market/blob/master/flocx_market/api/service.py#L30-L31 into app.py. Try that out, and let me know what you think! (It worked for me in testing)

ljmcgann commented 5 years ago

This works for the test cases but not for when I try to install and run the application

ljmcgann commented 5 years ago

The problem is all the test cases are run with auth_enable set to false. So the application is never wrapped in the keystonemiddleware. Test cases are passing but the actual application is not running

tzumainn commented 5 years ago

I tested with my proposed fix with both auth_enable set to true and to false, and everything seemed to work as expected. Does it not work for you?

We should probably add a test where auth_enable is set to true, and we make an API request and catch the 'Auth required' (or whatever) message and the appropriate return code.

ljmcgann commented 5 years ago

My bad I was being dumb. I got it to work now. But I'm going to try to write some kind of test case before commiting again

ljmcgann commented 5 years ago

Or should I commit now so the others can use the fix?

tzumainn commented 5 years ago

How about updating this PR with the fix and pointing people to it, but writing the test before merging?

codecov-io commented 5 years ago

Codecov Report

Merging #79 into master will increase coverage by 4.83%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage    92.8%   97.63%   +4.83%     
==========================================
  Files          28       29       +1     
  Lines         750      762      +12     
==========================================
+ Hits          696      744      +48     
+ Misses         54       18      -36
Impacted Files Coverage Δ
flocx_market/tests/unit/api/test_app.py 100% <ø> (ø) :arrow_up:
flocx_market/api/service.py 80% <ø> (+80%) :arrow_up:
flocx_market/tests/unit/conftest.py 100% <100%> (ø) :arrow_up:
flocx_market/api/app.py 100% <100%> (+5%) :arrow_up:
flocx_market/tests/unit/cmd/test_api_service.py 100% <100%> (ø)
flocx_market/cmd/api.py 92.85% <0%> (+92.85%) :arrow_up:
flocx_market/cmd/__init__.py 100% <0%> (+100%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 46c0c10...396a4aa. Read the comment docs.

tzumainn commented 5 years ago

Looks good - thanks!