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

Fix oauth2 compatibility with draft-ietf-oauth-v2-31 #708

Closed
wants to merge 1 commit into from
Closed

Fix oauth2 compatibility with draft-ietf-oauth-v2-31 #708

wants to merge 1 commit into from

Conversation

etehtsea
Copy link
Contributor

@etehtsea etehtsea commented Aug 5, 2014

http://tools.ietf.org/id/draft-ietf-oauth-v2-31.html#rfc.section.5.2
As far as I see status codes for invalid_grant should be 400.

@dspaeth-faber
Copy link
Contributor

@etehtsea How do you use this middleware? It is not documented and used directly with grape. Because of this I though I can safely delte this classes with my PR #703

@etehtsea
Copy link
Contributor Author

etehtsea commented Aug 6, 2014

@dspaeth-faber I'm using it like any other middleware

module API
  class Root < Grape::API
    class Protected < Grape::API
      use Grape::Middleware::Auth::OAuth2,
          token_class: 'AccessToken',
          parameter: %w(access_token api_key)

      use Rack::ConditionalGet
      use Rack::ETag

      mount API::EndpointName
      <..>

To be honest, I didn't understand what the problem?

@dspaeth-faber
Copy link
Contributor

@etehtsea

The Auth-Middleware is not documented within grape. Internaly it is also not used. For instance instead of Grape::Middleware::Auth::Basic grape uses internaly Rack::Auth::Basic. Also the documentation says

Use warden-oauth2 or rack-oauth2 for OAuth2 support

So I thought it is save to delete all the Auth-Middleware. But now you use this middleware. Maybe my auth refactoring within PR #703 has to be reconsidered.

@dblock What do you mean?

@etehtsea
Copy link
Contributor Author

etehtsea commented Aug 6, 2014

@dspaeth-faber I think if you delete them, I'll simply put this code into my repo. :)

@dblock
Copy link
Member

dblock commented Aug 6, 2014

I haven't merged #703 yet. If it's not hard, implement the OAuth2 middleware on top of #703 instead of removing it. It does work. I am however all for removing this functionality from Grape proper given that other gems do it better.

@dblock
Copy link
Member

dblock commented Aug 6, 2014

So to be clear, I will close this PR if/when #703 is merged - @etehtsea did you consider the other gems mentioned here before settling on Grape's implementation?

@etehtsea etehtsea closed this Aug 12, 2014
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.

3 participants