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

Scope Policy functionality #45

Closed
wants to merge 13 commits into from
Closed

Scope Policy functionality #45

wants to merge 13 commits into from

Conversation

Spomky
Copy link

@Spomky Spomky commented Oct 21, 2013

In RFC6749' we can read

« If the client omits the scope parameter when requesting authorization, the authorization server MUST either process the request using a pre-defined default value or fail the request
indicating an invalid scope. »

This PR add this rule.
Supported scopes, default scopes and scope policy can be defined per client (eg: limitation of supported scope depending on the client's type).

New parameters have been added:

  • CONFIG_SCOPES_POLICY: set the policy of the server (either error or default)
  • CONFIG_DEFAULT_SCOPES: if policy is default, this array of scopes will be used as default.

This PR also fix bug #25

It changes the client interface, so that this PR breaks compatibility and requires to bump version number.

* @param string $scope
* The scopes to check.
*
* @return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing return type

@Spomky
Copy link
Author

Spomky commented Oct 23, 2013

@alanbem what about a new branch to go to the RFC while the master one is slowly updated?

@alanbem
Copy link
Member

alanbem commented Oct 25, 2013

what about a new branch to go to the RFC while the master one is slowly updated?

I'm little scared of amount of backporting we would have to manage.

@alanbem
Copy link
Member

alanbem commented Oct 25, 2013

Would it be a lot of work to make that feature backward compatible e.g. create new scope policy none and act like current master?

I could make a merge regardless of the result of RFC vs draft discussion (we could move it and continue somewhere else).

@Spomky
Copy link
Author

Spomky commented Oct 25, 2013

There is nothing to do to make that feature compatible with the current master.
By default, scope_policy is default and no default_scopes is null. So when the function checkScopePolicy is called, nothing is changed and no exception is thrown.

The only difference is that this PR also fixes #25, because it does not set all scopes to the token, but only those passed by the input (that is why I have modified the test).

@alanbem
Copy link
Member

alanbem commented Nov 3, 2013

Hi, I just tested your code and unfortunately it seems buggy.

When I requested token with grant_type=authorization_code, the scope linked with authorization code was lost.

@Spomky
Copy link
Author

Spomky commented Nov 4, 2013

@alanbem you are right. I have just added a new test and fix that issue.

@Swop
Copy link

Swop commented Dec 24, 2013

I agree, this is not draft20 compliant, but it's a really missing feature too.

As I explained in an issue ticket in the fos/oauth-server-bundle repo (FriendsOfSymfony/FOSOAuthServerBundle#201), serious issues could appear without scope policy management...

So I think I'm gonna vote +1 on this one 👍

Spomky added 13 commits December 26, 2013 10:51
The requested scope is only "scope1"in this test, so the access token
only contains "scope1" and not "scope1 scope2" as before.
Function documentation modified according to Stof's coments.
Tests added.
Hardcoded policy values are now constants
Unknown scope policy test added
invalid_scope_policy is not part of the RFC6749
This commit fixes the issue and adds a new test.
* Clients can override the default scopes and scope policy defined by
the application
* Tests updated
Policy of clients was not checked.
Test added
Supported scopes can be defined by clients (eg: limitation depending on
the client type).
@alanbem
Copy link
Member

alanbem commented Dec 27, 2013

@Spomky look at OAuth2::grantAccessTokenAuthCode, OAuth2::grantAccessTokenUserCredentials, OAuth2::grantAccessTokenClientCredentials, OAuth2::grantAccessTokenRefreshToken and OAuth2::grantAccessTokenExtension. All those methods can return available scopes (optionally) for its grant. We have to handle that otherwise we would BC.

So if no scope is requested we should take those methods scope into account first... only then, if those methods won't return any scope, we should enforce policy.

@Spomky I was sure I've added that comment few weeks ago... but I haven't... I think I was on "Preview" mode and forgot to send it :/ Sorry for that buddy!

@Spomky
Copy link
Author

Spomky commented Dec 27, 2013

In fact there are two functionnalities in this PR.
The first one is to implement scope policy. The second one allow clients to set their own scope policy.

Only the second needs to modify interfaces and breaks the compatibility.

I have read again the code of OAuth2.php and I can not figure out how OAuth2::grant* methods could handle these functionnalities.

@alanbem
Copy link
Member

alanbem commented Dec 27, 2013

Its because proprietary storage interfaces e.g. IOAuth2GrantExtension or IOAuth2GrantClient - which OAuth2 uses internally - as they were designed to return true, false or array('data' => data, 'scope' => scope) when authorizing their respective grant.

It is pretty easy to extend storage (or wrap it up inside another proxy-like object) and implement this functionality. For example I could store allowed scope for each client/application in database and inject it in MyStorage::checkClientCredentialsGrant().
Nevertheless we don't know if someone didn't do that already - we can't break it.

@Spomky
Copy link
Author

Spomky commented Feb 14, 2014

Hi @alanbem ,

I think this bundle is a good bundle, but it should really be updated from draft 20 to RFC.
I understand your point of view and I know there is a lot of backporting to manage, but there is also a lot of missing features such as scope policy, MAC Token support or Public/Confidential/Anonymous Clients that we need.

I have decided to start a new project based on this bundle. As you you are already a member of my fork and you really know the OAuth flow, I will be proud if you participate, even if you just give some advices.

BR

@Spomky Spomky closed this Apr 17, 2014
@Spomky Spomky deleted the Default-scopes branch April 17, 2014 16:25
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.

4 participants