Use intersection of application and default server scopes #1002

[rishabhsairawat]

Summary

This PR aims to solve the issue #1002 .

Reference

Commit b5bf40e by @nbulaj missed the following case:
Default scopes: public, admin
Application scopes: read, write, public
And no scopes are passed.
Expected output: public
Output by b5bf40e : invalid scope

[nbulaj]

What about other grant flows, ie client credentials? I think they must work the same way

[rishabhsairawat]

@nbulaj Yes, Other grant flows must work the same way.

[nbulaj]

@rishabhsairawat yep, this is a good strategy. Implemented in 976b235

[rishabhsairawat]

@nbulaj What about other grant flows i.e. authorization_code & implicit? I think they must work the same way.

And you missed following case for client_credentials flow:
When application scopes are present but differs from configured default scopes
Output should be an error and it should not issue a token But according to you commit 976b235 it does not give error and issues a token.

[nbulaj]

What about other grant flows i.e. authorization_code & implici

I'm currently looking into them.

And you missed following case for client_credentials flow:

I will check it tomorrow.

[rishabhsairawat]

@nbulaj Checkout following scenario:
When there is no application scope and no default scope present
Currently, Doorkeeper issues AccessToken without any scope
Wouldn't it be better to show error ?
And Same is With AccessGrant

[nbulaj]

When there is no application scope and no default scope present
Currently, Doorkeeper issues AccessToken without any scope
Wouldn't it be better to show error ?
And Same is With AccessGrant

I don't sure if it must work this way. Some endpoints can require OAuth authentication, but without any particular scopes (just to be sure client is authenticated). So if client doesn't request any scopes - he will not get this scopes and can't request something protected with the scopes, but can interact with scopesless endpoints.

[rishabhsairawat]

@nbulaj If you want this then I think In following case also, scopeless token and grant should be issued.

If intersection of application scopes and default scopes is blank and No scopes are passed in request
Intersection of application scopes and default scopes will be blank in following cases

1. If default scopes are not present then intersection will automatically be blank
2. If default scopes are present but differs from application scopes

[nbulaj]

@rishabhsairawat of I understood you correctly:

If intersection of application scopes and default scopes is blank and No scopes are passed in request
Intersection of application scopes and default scopes will be blank in following cases

If default scopes are not present then intersection will automatically be blank
If default scopes are present but differs from application scopes

then I think it already works this way.

context 'when application scopes contain some of the default scopes and no scope is passed' do
  before do
    client.update_attributes(scopes: 'value1 value2')
  end

  it 'issues new token with one default scope that are present in application scopes' do
    default_scopes_exist :value3

    headers = authorization client.uid, client.secret
    params  = { grant_type: 'client_credentials' }

    expect do
      post '/oauth/token', params: params, headers: headers
    end.to change { Doorkeeper::AccessToken.count }.by(1)

    token = Doorkeeper::AccessToken.first

    expect(token.application_id).to eq client.id
    expect(token.scopes).to be_empty

    should_have_json 'access_token', token.token
    should_not_have_json 'scope'
 end
end

[rishabhsairawat]

@nbulaj Currently it works this way only for client_credentials flow as changes for client_credentials was done in your commit 976b235. But in my commits for other grant flows , I used other approach (showing error in place of token without any scope).

[nbulaj]

Ah, thats ok @rishabhsairawat, I've checked only client credentials thinking that every grant works the same way. Thanks!