Fixes #21605 - more authorization options #639
Conversation
Since the `/pubkey` endpoint is not authorized, we need to add an ability to smart proxy theforeman/smart-proxy#639 and then we can use `do_authorize` method at the beginning of our protected method.
lib/sinatra/authorization.rb
Outdated
| @@ -40,6 +55,27 @@ def authorize_with_ssl_client | |||
| end | |||
| end | |||
|
|
|||
| # Common set of authorization methods used in foreman-proxy | |||
| def do_authorize | |||
There was a problem hiding this comment.
You've seen some old code of mine, so you know I don't mind long names, but I still, prefer a bit shorter variant:
["do_authorize_with_ssl_client", "perform_authorization_with_ssl_client"].map(&:size)
=> [28, 37]
Let's see if there is anyone else that would choose what version is better?
There was a problem hiding this comment.
Hmmm I kinda don't like this very generic do_authorize because it does not tell anything about how the authorization is performed. I suggest do_authorize_any because basically this is the actual semantics.
There was a problem hiding this comment.
Changed to do_authorize_any
|
Works for me. Rubocop failed. |
|
Rubocop failures unrelated |
| require 'json' | ||
| require 'sinatra/base' | ||
|
|
||
| ENV['RACK_ENV'] = 'test' |
There was a problem hiding this comment.
I've seen this in multiple places, but the test_helper also sets this. Isn't it really redundant?
There was a problem hiding this comment.
Removed and seemed it still worked. Also removed the require 'json' as we didn't need it actually
| get '/public' | ||
| assert last_response.ok? | ||
| get '/private' | ||
| assert last_response.ok? |
There was a problem hiding this comment.
This is interesting and not something I thought about. We added the v2 features endpoint in root, but I suspect that's exposed over https without authentication. Perhaps we need some authentication method that enforces some authentication.
There was a problem hiding this comment.
Having tested this: this relies on localhost being in the trusted_hosts. On a HTTP call it uses reverse DNS + forward DNS. This isn't very secure but since the default deployment only enabled modules on HTTP when they are designed to assume no security it's not that big of a deal.
There was a problem hiding this comment.
Yes, this is how it have been done for a while: using authenticated http is not very secure and people should count on it. Anyway, not trying to address it here
There was a problem hiding this comment.
I do think it would be good to capture this in a test. Perhaps set the trusted hosts to empty or something different and assert that the response it not ok? Could be separate tests.
There was a problem hiding this comment.
The subject of this test is ensuring the authorization helpers are available. I'm not touching the authorization algorigth itself, and the tests for it are somewhere else: I would prefer keeping this PR focused on thing only: which is making the methods available outside of the before filters, that right now we are constraint with.
There was a problem hiding this comment.
Agreed, more tests is good but I want this to be merged ASAP, let's improve somewhere else.
There was a problem hiding this comment.
My argument was that if the response is ok, you still don't really know if the authorization was properly called.
|
|
||
| before do | ||
| def do_authorize_with_trusted_hosts |
There was a problem hiding this comment.
Why do you add the do_ prefix to it?
There was a problem hiding this comment.
to make it different than already existing methods: I don't like to have methods with the same name doing a bit different thing
There was a problem hiding this comment.
Ah, I missed that it's the helpers module vs methods on the higher level. This part of Ruby is still rather new to me.
Before this change, when using authorization helpers, one got all or nothing, without any chance to use the authorization just of a subset of the requests. This patch introduces `Sinatra::Authorization::Helpers` that provide `do_authorize*` methods that are not wrapped in the before block. so that they their usage is more flexible.
| @@ -40,6 +55,27 @@ def authorize_with_ssl_client | |||
| end | |||
| end | |||
|
|
|||
| # Common set of authorization methods used in foreman-proxy | |||
| def do_authorize_any | |||
There was a problem hiding this comment.
IMHO any is a bit ambiguous. Any could also mean authorized any client without authentication. Perhaps do_authorize_any_method would be clearer but naming is hard.
There was a problem hiding this comment.
Yeah I was thinking maybe do_authorize_all because we actually do not authorize but rather refuse unauthorized requests (the opposite). It's complicated, I was thinking like any reads a bit cleaner in a sense "any method will do it".
Before this change, when using authorization helpers, one got all or
nothing, without any chance to use the authorization just of a subset of
the requests.
This patch introduces
Sinatra::Authorization::Helpersthat providedo_authorize*methods that are not wrapped in the before block. so thatthey their usage is more flexible.
There are at least 3 places I know of that would benefit from this change: