Resource owner not found authorizes automatically?

[MichaelXavier]

Is there a good reason why passing a nil resource owner to OAuth2::Provider::AccessToken validates?

https://github.com/songkick/oauth2-provider/blob/master/lib/songkick/oauth2/provider/access_token.rb#L59

You have an explicit test for it here: https://github.com/songkick/oauth2-provider/blob/master/spec/songkick/oauth2/provider/access_token_spec.rb#L55

So I'm assuming it was deliberate. Here's my scenario:

oauth2-provider is a separate service. It lives in another app from the resources being accessed. It maintains all the authorizations and resource owners.

Since AccessToken finds any access token that matches the hash: https://github.com/songkick/oauth2-provider/blob/master/lib/songkick/oauth2/provider/access_token.rb#L49

an attacker with an access token granted to him by User A (for all intents and purposes, that could be the attacker himself), could cause an auth request to be made for a bogus user User Fake, using the token he has from User A. The resource owner would then be nil, the token would exist and oauth2-provider would validate his token and let him proceed.

The workaround for my case seems to be to return a 404 if the resource owner can't be determined, but I was curious why oauth2-provider assumes everything is ok when resource owner is nil.

[jcoglan]

The nil case is to support functionality like the /me resource in the Facebook API, where the resource owner is not identified in the request and instead is inferred from the token.

If you pass a user, and the token does not belong to that user, then valid? should return false. So, if your API resources explicitly identify the user then a token that grants access to one user will not grant access to the other.

Doing something like this should be fine:

post '/users/:username/stuff' do
  user = User.find_by_username(params[:username])
  error 404 if user.nil?
  token = Songkick::OAuth2::Provider.access_token(user, [], request)
  if token.valid?
    # do things
  else
    headers token.response_headers
    status token.response_status
    'Get out!'
  end
end

[jcoglan]

Maybe it would be better to require explicitly allowing user to be nil in case people aren't doing the validation as above. e.g. token = Provider.access_token(user, scopes, request, :implicit_owner => true).

[MichaelXavier]

I really like the implicit_owner idea. I feel like I could implement that patch if its something you'd accept it. What do you think about having the default be false though? I think if we're going by principle of lease surprise, passing in a nil owner automatically granting is more surprising than not, or at least more error prone.

[jcoglan]

I have a better idea: if you want implicit owner, you need to pass a special value, call it Provider::IMPLICIT as the user. nil always raises an error.

[jcoglan]

It occurs to me: if user is nil, what damage could you do? Presumably the app is modifying/accessing some user-specific resource, so if there's no user that will fail. If a user is found, then AccessToken will be given that and notice it's not the user the token belongs to.

[MichaelXavier]

I like using a constant there. That way it is a conscious effort.

In your example app, no real harm would come from having an implicit owner because the resources live next to the resource owner. In my case, We have a dedicated app for OAuth2 for our whole platform. Each app passes along a token it gets to the OAuth2 app to verify it. The OAuth2 app figures out the owner and returns a 200 if its OK, a 401 if not. In that case, if we don't take precautions against the resource owner not being found, the OAuth2 app will just come back with an OK for any valid token, regardless of which user they ask for.

I realize both are corner cases but we caught this issue pretty much on accident. Its lucky we did because this could be a nasty little attack vector if someone figured it out.

[jcoglan]

Sorry it's been a while, but I just pushed a change that makes nil an illegal value for the user argument. If you intent implicit user lookup, you must pass :implicit as the user argument. See https://github.com/songkick/oauth2-provider/commit/927650971da40af17da3656136556dc82bab3867

[paniko0]

That solved the problem. Thanks.