Client application specific scopes #469

cpapazian · 2014-08-22T22:54:06Z

This PR attempts to address #317. We saw @tute's comments, but could not find a great way to implement this without either putting all of the logic into the authoriizations controller (doing an end run around the work that the OAuth::PreAuthorizations class does, or monkeypatching Doorkeeper a bit. So I created this PR in an attempt to implement this functionality in a way that fits with the rest of the Doorkeeper API.

It works by adding a scopes field to the Doorkeeper::Application model and validating the requested scopes against it during pre-authorization. When enable_application_scopes is set, the Doorkeeper::Models::Scopes mixin is added to Doorkeeper::Application and validate_scopes will check both the server/config scopes as well as the client application's scopes.

Still TODO: adding support to the admin pages and testing with ORM's besides ActiveRecord

In the meantime, I would greatly appreciate some feedback.

thanks,
-chris

houndci-bot · 2014-08-22T22:54:15Z

lib/doorkeeper/models/scopes_validator.rb

+    class ScopesValidator < ActiveModel::Validator
+      def validate(record)
+        return unless record.scopes_string.present?
+        unless Doorkeeper::OAuth::Helpers::ScopeChecker.valid?(record.scopes_string, Doorkeeper.configuration.scopes)


Line is too long. [117/80]

You can extract the condition to a private method.

christopherhein · 2014-09-22T19:15:03Z

I think overall it's an interesting idea (one that I would personally like to use) that being said it doesn't seem like @tute wants to support this in Doorkeeper, which I can understand. I'd be interested to see if there was a way to put it out into a gem? The Admin panel isn't easily plugable from what I've seen, but I may just be doing it wrong.

sunteya · 2014-09-25T05:27:32Z

Nice feature, my project also has the same problem.
In my project, the need for many kind of Application, but each application has a different scope limit for the client credentials flow.

tute · 2014-10-11T19:12:51Z

Changing my mind about this feature. I wish OAuth spec was not so broad. :)

Citing https://tools.ietf.org/html/rfc6749#section-10.3:

The authorization server SHOULD take the client identity into account when choosing how to honor the requested scope.

This implies that this PR should sooner or later land into Doorkeeper. @iangreenleaf your solution in #492 is more general, but in this case, given that section of the spec, we'll actually want to make client-specific scopes a first class citizen.

Thank you all for your work. Will keep this in the roadmap.

tute · 2014-10-11T19:22:38Z

lib/doorkeeper/config.rb

@@ -9,6 +9,7 @@ def self.configure(&block)
    @config = Config::Builder.new(&block).build
    enable_orm
    setup_application_owner if @config.enable_application_owner?
+    setup_application_scopes if @config.enable_application_scopes?


We don't need the config, it should always be enabled, as the spec expects this.

This file should remain mostly unchanged.

iangreenleaf · 2014-10-11T21:38:16Z

One thing that neither my PR nor this PR offers, but that probably should be implemented based on the spec, is the ability to ignore the scopes requested by the client and respond with a different set of scopes. That actually seems to be the preferred way of dealing with this, rather than throwing an error as we have been doing.

iangreenleaf · 2014-10-11T21:41:54Z

I'd also still advocate for a more generic solution, based on this part of the spec:

The authorization server MAY fully or partially ignore the scope
requested by the client, based on the authorization server policy or
the resource owner's instructions. If the issued access token scope
is different from the one requested by the client, the authorization
server MUST include the "scope" response parameter to inform the
client of the actual scope granted.

To me, "based on the authorization server policy" casts a pretty wide net, so it would be nice to support scope limits based on a variety of different policies.

cpapazian · 2014-10-12T05:45:26Z

excellent. i'll update this PR with your comments as soon as i can.

cpapazian · 2014-10-12T05:45:30Z

@iangreenleaf it's pretty easy for the application server to modify the scopes in the authorizations controller without getting too deep. we already do this, even going as far as allowing the user to deny scopes that the client applications request.

houndci-bot · 2014-10-14T16:23:06Z

lib/doorkeeper/oauth/scopes.rb

+        other_array = other.present? ? other.all : []
+        self.class.from_array(self.all & other_array)
+      end
+


Extra empty line detected at body end.

houndci-bot · 2014-10-14T16:24:31Z

lib/doorkeeper/oauth/scopes.rb

+        other_array = other.present? ? other.all : []
+        self.class.from_array(self.all & other_array)
+      end
+


Extra empty line detected at body end.

houndci-bot · 2014-10-31T21:42:01Z

lib/doorkeeper/models/application_mixin.rb

@@ -38,5 +39,6 @@ def generate_uid
    def generate_secret
      self.secret ||= UniqueToken.generate
    end
+


Extra empty line detected at body end.

tute · 2014-11-15T20:52:17Z

Left a few comments, let's also take all Hound comments into account. Thanks for your work!

houndci-bot · 2014-11-18T18:25:14Z

spec/lib/oauth/pre_authorization_spec.rb

+      application
+    end
+
+    let(:client) { double :client, redirect_uri: 'http://tst.com/auth', application: application }


Line is too long. [98/80]

tute · 2014-11-19T02:27:57Z

This is looking great, thanks for your work! Looking forward to merge it and release doorkeeper 2.

tute · 2014-11-21T01:27:17Z

@cpapazian regarding default behavior, I'll explain what I had in mind, and you please tell what what you have in mind until we solve it the best way.

I think of a transparent "it's there if you need it" feature, where the application level scopes are blank (empty string) in the DB by default. If you decide to use them, in the admin for oauth apps you just define which scopes can an application use. In that case, you enable this functionality.

Let's say I have an existing app, where I don't have this behavior yet (or even a new one which I'm now starting). I run the migrations, and, no matter what I configure as Doorkeeper scopes, the DB starts with blank scopes. I use everything, all works, then I decide to filter out write scope for an app, and enable it only for read. I seed in read as it's scope, and call it done.

The intersection, then, should happen if an application overrides the default empty string. To make this very clear from the code up to the admin interface, we could call the DB attribute allowed_scopes.

The reason I prefer to avoid needing to always set the scopes is that both they have to be in sync with the configuration of Doorkeeper, which may change app per app; and one may forget to seed in correct values.

What do you think of it? Thanks again for your work!

cpapazian · 2014-11-21T20:26:19Z

Yup, that all makes sense, and is what I had in mind when I originally set this up as a configuration setting. I'll make the changes to what I have to fit your description.

I I'd like to keep the database attribute as scopes so that I can reuse the mixin that AccessGrant and AccessToken use. I'm also going to remove the default: '' from the schema definition, and use nil to indicate that the client application does not restrict the scopes.

tute · 2014-11-21T20:39:17Z

All great!

I'm also going to remove the default: '' from the schema definition, and use nil to indicate that the client application does not restrict the scopes.

Better an empty string, so we keep the same type everywhere and we avoid NoMethodErrors.

cpapazian · 2014-11-21T21:10:24Z

I see what you mean ... Doorkeeper::OAuth::Scopes initializes @scopes as an empty array, and self.from_string(...) converts nil to '', which makes it difficult to deal with nil values.

The issue I am having is that I'd like to differentiate between a client application disallowing all scopes from a client application that does not want to specify scopes (and let the config/server manage that). If you don't think that's important, then it'll be simple to use the empty string to mean that the client application will not restrict scopes. Or maybe you have some thoughts on how to achieve this another way.

cpapazian · 2014-11-21T23:27:58Z

BTW, this is what I had in mind with nil application scopes: cpapazian#1

I'm keeping it out of this branch for now, as it required even more changes than expected, but it accomplishes the "it's there if you need it" requirement. Changing the default value of scopes on the access_grants and access_tokens table seems like a good thing, but there was quite a few hoops to jump through to handle nil scopes on application (to be fair, you did warn me). Anyway, curious your thoughts.

tute · 2014-11-23T00:53:45Z

The issue I am having is that I'd like to differentiate between a client application disallowing all scopes from a client application that does not want to specify scopes (and let the config/server manage that).

Why would such application be defined in the provider? Could it perform any OAuth flow, if it rejects any scope?

I think we are fine making empty string mean "accept all".

tute · 2014-11-23T00:54:52Z

spec/dummy/db/migrate/20140813024646_add_scopes_to_application.rb

@@ -0,0 +1,5 @@
+class AddScopesToApplication < ActiveRecord::Migration
+  def change
+    add_column :oauth_applications, :scopes, :string, default: ''


Forgot to mention: let's disallow NULL: null: false.

tute · 2014-11-23T00:57:27Z

I'm keeping it out of this branch for now, as it required even more changes than expected, but it accomplishes the "it's there if you need it" requirement. Changing the default value of scopes on the access_grants and access_tokens table seems like a good thing, but there was quite a few hoops to jump through to handle nil scopes on application (to be fair, you did warn me). Anyway, curious your thoughts.

nil should be avoided as a general rule. See:

cpapazian · 2014-11-24T04:04:29Z

Sounds good. I'll make the changes tomorrow.

FWIW, It was pointed out to me (by @yozlet) that If a provider really wants this null scope behavior, they could define a "null" scope that does not grant any interesting permissions.

houndci-bot · 2014-11-24T19:51:28Z

lib/doorkeeper/oauth/pre_authorization.rb

+        if client.application.scopes.empty?
+          Helpers::ScopeChecker.valid?(scope, server.scopes)
+        else
+          Helpers::ScopeChecker.valid?(scope, server.scopes & client.application.scopes)


Line is too long. [88/80]

tute · 2014-11-27T02:59:51Z

Looks great!

Can you pleas add notes to CHANGELOG.md about the addition, and to UPGRADING.md about the migration that needs to be run for the upgrade?
Can you squash the commits together and rebase on top of master?

Soon to merge. Thanks! 👏

cpapazian · 2014-12-01T19:30:38Z

Added a line to CHANGELOG.md. Didn't see an UPGRADING.md, so I created one ... assuming you'll move that to the wiki?

Rebased and sqaushed, too.

If there no scopes (empty string) defined in the Doorkeeper::Application record, PreAuthorization should only compare scopes against the config/server scopes. If there are scopes on the Doorkeeper::Application record, the set of valid scopes is the intersection of the config/server and the Doorkeeper::Application record scopes.

Client application specific scopes

tute · 2014-12-02T05:00:03Z

Thank you!

in order to restrict scopes that applications can grant to clients: doorkeeper-gem/doorkeeper#469

seanarnold · 2016-03-16T21:42:42Z

@tute @cpapazian Just stumbled upon this and it's saved me so much time. I thought I had to do this manually.

Is this documented anywhere? https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes Only mentions creating and using scopes, but not limiting per Application. I can write something in the Wiki about it unless it's documented somewhere else?

tute · 2016-03-17T00:02:57Z

I can write something in the Wiki

Please do! Thank you. :)

seanarnold · 2016-04-17T22:54:42Z

@tute Got around to adding this https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes#limiting-scopes-per-application

Feel free to make your own changes.

houndci-bot reviewed Aug 22, 2014
View reviewed changes

iangreenleaf mentioned this pull request Oct 11, 2014

Hook for custom validation rules in pre-authorization #492

Closed

tute reviewed Oct 11, 2014
View reviewed changes

cpapazian force-pushed the feature/application_scopes branch from 206e37a to b3f9172 Compare October 14, 2014 16:22

houndci-bot reviewed Oct 14, 2014
View reviewed changes

cpapazian force-pushed the feature/application_scopes branch from b3f9172 to cca7c2c Compare October 14, 2014 16:24

houndci-bot reviewed Oct 14, 2014
View reviewed changes

cpapazian force-pushed the feature/application_scopes branch from cca7c2c to 41f4605 Compare October 31, 2014 21:41

houndci-bot reviewed Oct 31, 2014
View reviewed changes

houndci-bot reviewed Nov 18, 2014
View reviewed changes

cpapazian force-pushed the feature/application_scopes branch from aa39f30 to 92094af Compare November 18, 2014 18:26

tute reviewed Nov 23, 2014
View reviewed changes

houndci-bot reviewed Nov 24, 2014
View reviewed changes

cpapazian force-pushed the feature/application_scopes branch from ab03577 to 8fe62b2 Compare December 1, 2014 19:29

cpapazian force-pushed the feature/application_scopes branch from 8fe62b2 to 1c89cc5 Compare December 2, 2014 00:26

tute added a commit that referenced this pull request Dec 2, 2014

Merge pull request #469 from cpapazian/feature/application_scopes

69621bd

Client application specific scopes

tute merged commit 69621bd into doorkeeper-gem:master Dec 2, 2014

cpapazian mentioned this pull request Dec 22, 2014

Check application-level scopes in client_credentials and password flow #538

Merged

kenn mentioned this pull request Dec 23, 2014

application scopes saved as '' instead of NULL #539

Closed

tute mentioned this pull request Jan 20, 2015

Scope checker allow application with scopes=nil to require any scope #559

Closed

vinayvinay pushed a commit to alphagov/signon that referenced this pull request Jan 30, 2015

Add a scopes field to doorkeeper applications

8172a10

in order to restrict scopes that applications can grant to clients: doorkeeper-gem/doorkeeper#469

vinayvinay pushed a commit to alphagov/signon that referenced this pull request Feb 2, 2015

Add a scopes field to doorkeeper applications

4975c63

in order to restrict scopes that applications can grant to clients: doorkeeper-gem/doorkeeper#469

tute mentioned this pull request Apr 21, 2015

Docs for using application-specific scopes #640

Closed

MonikaMahanthappa mentioned this pull request Jul 22, 2015

Client application specific scopes MavenHive/doorkeeper#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client application specific scopes #469

Client application specific scopes #469

cpapazian commented Aug 22, 2014

houndci-bot Aug 22, 2014

tute Nov 15, 2014

christopherhein commented Sep 22, 2014

sunteya commented Sep 25, 2014

tute commented Oct 11, 2014

tute Oct 11, 2014

tute Oct 11, 2014

iangreenleaf commented Oct 11, 2014

iangreenleaf commented Oct 11, 2014

cpapazian commented Oct 12, 2014

cpapazian commented Oct 12, 2014

houndci-bot Oct 14, 2014

houndci-bot Oct 14, 2014

houndci-bot Oct 31, 2014

tute commented Nov 15, 2014

houndci-bot Nov 18, 2014

tute commented Nov 19, 2014

tute commented Nov 21, 2014

cpapazian commented Nov 21, 2014

tute commented Nov 21, 2014

cpapazian commented Nov 21, 2014

cpapazian commented Nov 21, 2014

tute commented Nov 23, 2014

tute Nov 23, 2014

tute commented Nov 23, 2014

cpapazian commented Nov 24, 2014

houndci-bot Nov 24, 2014

tute commented Nov 27, 2014

cpapazian commented Dec 1, 2014

tute commented Dec 2, 2014

seanarnold commented Mar 16, 2016

tute commented Mar 17, 2016

seanarnold commented Apr 17, 2016

Client application specific scopes #469

Client application specific scopes #469

Conversation

cpapazian commented Aug 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherhein commented Sep 22, 2014

sunteya commented Sep 25, 2014

tute commented Oct 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iangreenleaf commented Oct 11, 2014

iangreenleaf commented Oct 11, 2014

cpapazian commented Oct 12, 2014

cpapazian commented Oct 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tute commented Nov 15, 2014

Choose a reason for hiding this comment

tute commented Nov 19, 2014

tute commented Nov 21, 2014

cpapazian commented Nov 21, 2014

tute commented Nov 21, 2014

cpapazian commented Nov 21, 2014

cpapazian commented Nov 21, 2014

tute commented Nov 23, 2014

Choose a reason for hiding this comment

tute commented Nov 23, 2014

cpapazian commented Nov 24, 2014

Choose a reason for hiding this comment

tute commented Nov 27, 2014

cpapazian commented Dec 1, 2014

tute commented Dec 2, 2014

seanarnold commented Mar 16, 2016

tute commented Mar 17, 2016

seanarnold commented Apr 17, 2016