-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
added grape helper #567
added grape helper #567
Conversation
nil | ||
end | ||
|
||
def doorkeeper_error_renderer(error, options = {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused method argument - options
. If it's necessary, use _
or _options
as an argument name to indicate that it won't be used.
Thanks for your work. I want to do the opposite though: instead of adding framework specifics into doorkeeper, remove them, so new libraries can be built that bridge the gap between an app in any framework and doorkeeper. The refactors I envision are about removing Rails specifics, so doorkeeper runs without loading any Rails framework. If will of course expect some API, like |
For example, you mention in #563 (comment):
Detaching doorkeeper internals from Rails, and building a facade that other frameworks can implement would be nice. I'd leave for now Rails facade in as doorkeeper was born as a Rails plugin. |
I like your |
Just been playing with this and managed to get round the impact on the core Doorkeeper code by using a delegator, e.g.: module Doorkeeper
module Grape
class RequestDecorator < SimpleDelegator
def parameters
params
end
def authorization
env = __getobj__.env
env['HTTP_AUTHORIZATION'] ||
env['X-HTTP_AUTHORIZATION'] ||
env['X_HTTP_AUTHORIZATION'] ||
env['REDIRECT_X_HTTP_AUTHORIZATION']
end
end
end
end then wrapping up the request in that before passing it into e.g., in doorkeeper/grape/helpers.rb: def decorated_request
RequestDecorator.new(request)
end
def doorkeeper_token
@_doorkeeper_token ||= OAuth::Token.authenticate decorated_request, *Doorkeeper.configuration.access_token_methods
end The helper and decorator could then easily be pulled out into a separate gem to provide Grape support through the existing interface. |
You mean this one? 😉 That's what I based my helper on. It should probably be possible to create a common helper which implements the authorization methods, so that only the error rendering has to be implemented by the framework specific helper. |
That's exactly how it should be working indeed! Both helpers repeat methods that are quite trivial, I then don't feel strongly they shouldn't be repeated, although that's not ideal. The difference in My hesitation about merging the PR is that I don't use Grape, so I won't be able to maintain it well. Two questions:
|
Makes explicit what the public API is to inject doorkeeper behavior into application endpoints. This should be the only interface between Rails and doorkeeper. Other frameworks can define their own interface and use doorkeeper too. This is a step into decoupling doorkeeper from Rails, so that integration is easier. Related with #567.
@hobofan it would be nice if you can rebase your branch on top of latest master. See refactors in referenced commit. |
Makes explicit what the public API is to inject doorkeeper behavior into application endpoints. This should be the only interface between Rails and doorkeeper. Other frameworks can define their own interface and use doorkeeper too. This is a step into decoupling doorkeeper from Rails, so that integration is easier. Related with #567.
13da44a
to
839cc5f
Compare
end | ||
|
||
# endpoint specific scopes > parameter scopes > default scopes | ||
def doorkeeper_authorize!(*scopes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assignment Branch Condition size for doorkeeper_authorize! is too high. [36.07/15]
Perceived complexity for doorkeeper_authorize! is too high. [10/7]
839cc5f
to
fb098ac
Compare
|
||
def authorization | ||
env = __getobj__.env | ||
env['HTTP_AUTHORIZATION'] || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary spacing detected.
fb098ac
to
5c3c15d
Compare
As for I also noticed this piece of code:
|
👍
👍
There's samples in https://github.com/doorkeeper-gem/doorkeeper/#access-token-scopes. It accepts many strings/symbols.
Very true. To my surprise, it was buggy before, too; see https://github.com/doorkeeper-gem/doorkeeper/blob/v2.1.1/lib/doorkeeper/rails/helpers.rb#L29 (default scopes would not be taken into account). I started a branch to take care of this (https://github.com/doorkeeper-gem/doorkeeper/tree/tc-bugfix) but I want to be able to unit test it, couldn't do it yet. Thanks for your time and work! |
Your PR is looking better, I like the decorator. We now need to fix the scopes implementations, and unit test it. Cheers! |
endpoint_scopes = env['api.endpoint'].options[:route_options][:scopes] | ||
scope_string = Doorkeeper::OAuth::Scopes.from_array(scopes).all.inspect if scopes && !scopes.empty? | ||
scope_string = Doorkeeper::OAuth::Scopes.from_array(endpoint_scopes).all.inspect if endpoint_scopes | ||
scopes = Array.wrap(scope_string) if scope_string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the block level if
style, better.
Also, why do you use .inspect
?
@hobofan I'm liking this PR. Can you please rebase it on top of latest master? Thanks! |
includes decorated request as suggested by @littleowllabs
5c3c15d
to
ff0da09
Compare
The Either way, the |
Thank you! :) |
This feature looks great. I'm using doorkeeper as oauth provider for my grape api server. In my opinion it might be better to keep this feature in a standalone gem, like wine_bouncer and grape-doorkeeper. Since with grape, we also pay attention on its document generation, e.g. i use grape-swagger to generate a swagger spec for my apis. Wine_bouncer has a good feature that it can automatically inject oauth requirements into api endpoints' description. If put all these logics into doorkeeper will make it hard to maintain - it need tracks grape, swagger, and grape-swagger's new features to keep everything fine. |
one more comment on the usage: get '/', scopes: [:users] do
@users = Users.all
end it would be better move scopes: [:users]
get '/' do
@users = Users.all
end with this syntax we can easily inject scopes requirements into endpoint's description to generate better documentation. |
@calfzhou I agree this belongs in its own project, maintained by people who use Grape on a daily basis. I merged it anyway because it's simple enough that I can maintain it without significant effort and encapsulated enough to revert it when it's no longer needed. It's a useful addition for me also to understand better the API doorkeeper should expose to frameworks. All you mention sound good, and I'm happy to help make it happen. |
Using require 'doorkeeper/grape/helpers'
module API
module V1
class Users < Grape::API
before do
doorkeeper_authorize!
end
....
end
end
end causes a I have my class in api/api/v2/users.rb I'm trying to use it with rails 4.2.0, doorkeeper 2.1.3, grape 0.11.0 and grape-roar 0.3.0 Do you have any suggestions how I can make it work? Edit: I post it here because I think it is related and I'm trying to use it in practice. Sorry, if I'm wrong! |
@RinkeRiezebos you are doing it right, I didn't yet release 2.1.4 with Grape helpers. Can you point your Gemfile to GitHub's master? It should work fine. |
@tute Thanks for the quick reply! I pointed the gem to master ( However, I now get a |
Now it's probably @hobofan's territory. When either gets an app working with this, can you please post how you did it in the wiki? |
yes! Thanks! |
@RinkeRiezebos requiring the helpers isn't enough, you have to tell grape to also load the helpers from the module. You can do this with
I added this code snippet to the wiki. |
@hobofan Awesome, it works! Thanks! Think I had it at some point (but then removed it by accident?) |
Added description how to Protect your API with OAuth when using Grape after PR567 doorkeeper-gem#567 (comment)
Added description how to Protect your API with OAuth when using Grape after PR 567. #567 (comment)
Added description how to Protect your API with OAuth when using Grape after PR 567. #567 (comment) [ci skip]
Added description how to Protect your API with OAuth when using Grape after PR 567. #567 (comment)
Proof of concept for #563.
Should not be merged yet, since it still has a few rough edges:
[:private, :private_write]
into a comparable format forvalid_doorkeeper_token?(*scopes)
. The current implementation probably doesn't work well with multiple scopes.from_access_token_param
andfrom_bearer_token_param
access_token methods work, sindGrape::Request
doesn't provide a similiar method toActionDispatch::Request#authorization
small code sample on how to use it: