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.

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

Conversation

dspaeth-faber
Copy link
Contributor

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:

class MyAPI < Grape::API
  register_auth :my_digester, MyAuthMiddleWareClass, ->(settings) {  [settings[:a], settings[:b]]}

   auth :my_digester, a: "1", b: "2"  do |..., ... |

   end
end

@dblock
Copy link
Member

dblock commented Jul 30, 2014

I always disliked how this was implemented. To me, it feels like Base should not be aware of the word 'auth'. I'll make some comments inline.

auth_proc_context.instance_exec(*args, &auth_proc)
end
end
Grape::Middleware::Auth::Basic.apply_auth_middleware(b, self, settings[:auth])
Copy link
Member

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.

Copy link
Contributor Author

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

@dspaeth-faber
Copy link
Contributor Author

You mean Grape::API should not have a keyword auth? Or should Grape::API not have a keyowrd register_auth

@dspaeth-faber
Copy link
Contributor Author

Maybe the second try is a little bit more what you want

@dblock
Copy link
Member

dblock commented Jul 31, 2014

It's better, but the fundamental issue remains. I think Grape::API proper should not know anything about auth, but you should be able to mix it in. The fact that Base knows about anything authentication related is the weird part to me.

I am open to making incremental improvements too, what does everyone think?

@dspaeth-faber
Copy link
Contributor Author

So move auth to seperate gem and include the middleware in this gem via use ?
Then configure the middleware outside of the API-Class, like warden it does?

@dblock
Copy link
Member

dblock commented Jul 31, 2014

Yes.

@dspaeth-faber
Copy link
Contributor Author

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.

@dblock
Copy link
Member

dblock commented Aug 1, 2014

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.

@dblock
Copy link
Member

dblock commented Aug 6, 2014

The OAuth2 middleware has also been removed, right? It definitely needs to be in the CHANGELOG, please, even though it has never been documented.

@dspaeth-faber
Copy link
Contributor Author

I changed the CHANGELOG.md

@dspaeth-faber
Copy link
Contributor Author

I did remove Grape::Middleware::Auth::Basic, Grape::Middleware::Auth::Digest and Grape::Middleware::Auth::OAuth2 because they are not used within grape and not documented. I think it make no sence to maintain these classes when even grape uses Rack classes for this (before my refactoring).

Do you think these classes should still exists within grape?

@dblock
Copy link
Member

dblock commented Aug 6, 2014

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.

dblock added a commit that referenced this pull request Aug 6, 2014
…re_refactoring

Make auth middleware extendable
@dblock dblock merged commit c6cd3a1 into ruby-grape:master Aug 6, 2014
@dspaeth-faber dspaeth-faber deleted the probosal_for_auth_middelware_refactoring branch August 6, 2014 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants