-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Make auth middleware extendable #703
Make auth middleware extendable #703
Conversation
I always disliked how this was implemented. To me, it feels like |
auth_proc_context.instance_exec(*args, &auth_proc) | ||
end | ||
end | ||
Grape::Middleware::Auth::Basic.apply_auth_middleware(b, self, settings[:auth]) |
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.
I think this should become something like use Grape::Middleware::Auth::Basic
and without the if settings[:auth]
, simply the middleware would have access to the settings and can decide to pluck auth
.
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.
Ok, so include the Auth-Middleware every time
b.use Grape::Middleware::Auth self, settings[:auth]
class Grape::Middleware::Auth < Grape::Middleware::Base
def initialize(app, endpoint, options = {})
super(app, options)
@strategy = find_it_some_how # or raise error if not found?
end
def before
# here the magic happens
end
end
You mean |
Maybe the second try is a little bit more what you want |
It's better, but the fundamental issue remains. I think Grape::API proper should not know anything about I am open to making incremental improvements too, what does everyone think? |
So move auth to seperate gem and include the middleware in this gem via |
Yes. |
Ok, so I will wait until furthor feedback is given within this PR. Maybe @mbleigh can give some feedback as owner of warden-oauth2. His repo seem's to point into this direction. |
I think what's in this PR is actually merge-able. The DSL refactoring is a good start. You should fix Rubocop violations and stuff like that, check whether anything needs to be documented. |
The OAuth2 middleware has also been removed, right? It definitely needs to be in the CHANGELOG, please, even though it has never been documented. |
I changed the CHANGELOG.md |
I did remove Do you think these classes should still exists within grape? |
I'm with you @dspaeth-faber on removing this. The Rack classes are much better. I'm merging this, however I'd really appreciate a PR which adds a section to UPGRADING that describes how someone who has been using these classes should go from 0.8.0 to the next major release. |
…re_refactoring Make auth middleware extendable
Hi,
this PR is a probosal of mine how auth middleware can be extented by the grape user.
If it might be usefull I would finalize this draft with spec's and a DSL extenstion.
What I have in mind is at the end some thing like this: