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

Be more conservative when validating scopes. #72

Closed
wants to merge 1 commit into from

Conversation

@clintecker
Copy link
Contributor

commented Feb 3, 2014

Returning True here seems needlessly permissive and can result in scope escalation if developers do not declare a validate_scopes method on their client objects.

Imagine that a client requests a token with scopes ['read', 'write', 'destroy'] but the default scopes for this client is ['read', 'write']. If a validate_scopes method is not defined on the client object, the first conditional here will return False:

def validate_scopes(self, client_id, scopes, client, request,
                    *args, **kwargs):
    """Ensure the client is authorized access to requested scopes."""
    if set(client.default_scopes).issuperset(set(scopes)):
        return True
    if hasattr(client, 'validate_scopes'):
        return client.validate_scopes(scopes)
    return True

If I haven't defined that method on the second conditional, a provider using this default method will approve the generation of a token with more scopes than it should be allowed to have.


I realize developers can currently avoid this behavior in two ways:

  • Subclassing the request validator and implement their own validate_scopes method to be less permissive
  • Adding a validate_scopes method to their client objects.

Regardless, it seems weird to have the most permissive and possibly dangerous option be the default one.

@clintecker

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2014

I have a follow up question concerning why default_scopes is being checked here and not scopes. It's my understanding that scopes is the maximum amount of scopes and default_scopes is equal to scopes or a subset of scopes.

Should client.scopes be checked here instead of client.default_scopes ?

@lepture lepture added bug labels Feb 5, 2014

@lepture

This comment has been minimized.

Copy link
Owner

commented Feb 5, 2014

I think you are right. I think the validation flow should be:

  1. client. validate_scopes
  2. client.scopes validation
  3. client.default_scopes
@lepture

This comment has been minimized.

Copy link
Owner

commented Feb 5, 2014

Oh, no. There is no client.scopes. Only access_token has scopes.

lepture added a commit that referenced this pull request Feb 5, 2014

lepture added a commit that referenced this pull request Feb 5, 2014

@lepture lepture closed this Feb 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.