Flow error corner case

[eliaslevy]

From looking at the examples and docs, it would seem the recommended use it to have two action end points, say /authorize and /authorize/allow. The first would receive connections from both the user's agent and from clients, while the second would receive connections from the user'agent authorizing or not a client.

In the case of /authorize it appears one should use OAuth2::Provider.parse to route the request, which determines whether the request for authorization or exchange by looking for the grant_type param.

The examples then use the redirect? method on the object that Provider.parse returns to determine whether to cause the redirect. If there was no redirect, the output of the response_body method is used determine whether the reply to the requestor directly using JSON or to render the view that asks the user whether to authorize the client or not.

OAuth2::Provider::Authorization will cause a redirect in almost all error situations. The one exception is if client_id is invalid. Presumably you are not using the request's redirect uri param in that case, as it would create an open redirector. But this means that if client_id is invalid response_body will return JSON, which will be displayed to the user.

Similarly, if SSL enforcement has been turned on and for some reason the user agent connects to the /authorize end point over HTTP rather than HTTPS, OAuth2::Provider.parse will return a OAuth2::Provider::Error, which responds to redirect? with false and to response_body with JSON. This would then be displayed to the user.

Seems like there is a need to distinguish responses meant for the client, which are in JSON, and responses for the user agent, which need to be passed to the view for rendering, and response_body is not fully filling that purpose. For now I am checking the class of the object returned by OAuth2::Provider.parse, and if no redirect was indicated, when showing the view if the class is Authorization, and otherwise responding with whatever respond_body return.

Not sure if this is the best approach.

[jcoglan]

I've made a few changes to try and fix this. Certainly, the cases where the application should be responsible for the response body are too difficult to handle with the current API. Here's what I've done.

First, on the master branch, I've made Provider.parse() never return Error objects -- it will always return either an Authorization or an Exchange. These objects still expose the failure modes that Error represented before, but it means we can differentiate between cases where an error should provide its own response body (Exchange) and when it should not (Authorization).

Second, on this branch I've made it so that Authorization#response_body always returns nil. The prefered method of handling the request in this case is then to check if @oauth2 returns a body, and return it if so. If not, we check if the request is valid, and if so we render the login flow and if not we render an error. The error page can show the value of @oauth2.error_description to the user. i.e.:

[:get, :post].each do |method|
  __send__ method, '/oauth/authorize' do
    @user = User.find_by_id(session[:user_id])
    @oauth2 = OAuth2::Provider.parse(@user, request)

    if @oauth2.redirect?
      redirect @oauth2.redirect_uri, @oauth2.response_status
    end

    headers @oauth2.response_headers
    status  @oauth2.response_status

    if body = @oauth2.response_body
      body
    elsif @oauth2.valid?
      erb :login
    else
      erb :error
    end
  end
end

Let me know if this makes sense, and any other feedback you have.

[eliaslevy]

All the changes look sensible to me. I say commit.

[jcoglan]

Have merged, finally. Getting a bunch of changes in before we release as a proper gem.