Only enable specific grant flows. #295

yorkxin · 2013-10-08T12:48:15Z

I'd like to disable some unused grant flows in my API design, such as Resource Owner Password Credentials Grant and Client Credentials Grant. I found there is no way to disable it, so I implemented it.

In practice, the Authorization Endpoint and Token Endpoint would check for available response / grant types (the "strategies") according to the grant flows enabled. For example, if :implicit is not enabled, then Authorization Endpoint will return "Unsupported Response Type" error when response_type is set to token.

By default it enables all the four grant flows. I've also modified initializer file:

  # Change what grant flows are enabled
  # By default it enables all the four grant flows. Remove any of them from
  # the array to disable.
  #
  # grant_flows [
  #               :authorization_code,
  #               :implicit,
  #               :password_credentials,
  #               :client_credentials
  #             ]

Notes:

I don't know whether the commit chitsaou/doorkeeper@110c0d01be8afd8a7713a302333f5c0abb7bad85 is necessary, since the "Unsupported Response Type" is filtered in Doorkeeper::Request. I can remove this commit if you think this is not necessary.
Instead of RSpec tests, I've tested this feature with my demo app, and it seems work.
How do you think about the config values in my commit? ~~In fact I feel that :resource_owner_password_credentials is too long.~~ Update: changed to :password_credentials thanks to advise from @simonbnrd!
Token refreshing on Token Endpoint is currently always-enabled. I think it would make more sense if it is enabled only when use_refresh_token is enabled.
server_spec.rb cannot be run standalone, it would raise some exceptions. I instead tested this spec with rake spec.

Update

According to discussions below, the grant_flows setting is now in array of strings:

grant_flows %w(authorization_code implicit password client_credentials)

yorkxin · 2013-10-17T03:12:34Z

@tute Hi, sorry for pinging you here, but I'd like to know how this PR will be? Is it acceptable? Do I need to refactor it? Thanks. :)

tute · 2013-10-17T13:11:32Z

@chitsaou, thank you very much for your work and your poke. I've been on vacations and hanging around Github only for quick comments, your PR deserves more time that I expect to give next week!
Till next, thank you again,
Tute.

yorkxin · 2013-10-17T13:38:02Z

@tute oh I see, sorry for the interruption. Enjoy your vacation :)

kevintom · 2013-10-21T02:10:05Z

I was thinking of something similar but on the oauth_applications level,

having a list of supported grant_types column on the applications table (default :all ?)

and then doing the check against the applications supported list

not sure if this address the same need
(a little more versatile for me, since i'd like to permit password grant type for my iOS app and android only and have every other api consumer use the authorization_code grant type)

birarda · 2014-02-12T18:05:41Z

This would be a very useful feature to have. Is it against the OAuth spec to disable certain grant types for an application?

yorkxin · 2014-02-13T03:50:09Z

No, it does not against the spec. Most websites implement only one or two of them. For example, Authorization Code grant type is the only implemented type for most website, and Twitter does only implement Client Credentials grant type.

I've summarized a list of OAuth 2 implementation differences among popular websites on my blog, and you can see that none of them implement all the 4 built-in grant types. Sorry it's in Chinese; here are some brief translations: 自製 = self-made (non-OAuth 2); 半自製 = partially self-made; 無 scope = scope not implemented; spec 相容 = compatible with spec; client 認證 = client authentication

sjfbo · 2014-04-28T14:19:16Z

Really interesting @chitsaou ! / ping @tute 😄

This is also something I was looking for. Now that grant flows such as Resource Owner Password Credentials Grant do not require a client_id and a secret (thanks to OAuth2 spec !), I think that we should be able to disable these grant-types if we want to.

sjfbo · 2014-04-28T16:18:52Z

spec/lib/config_spec.rb

+        }
+      end
+
+      it "includes ':password' in token_grant_types" do


should be :client_credentials instead of :password

Fixed. Thank you!

yorkxin · 2014-04-28T16:32:24Z

I've left the master branch too far. Maybe I should rebase my changes onto the current version first?

sjfbo · 2014-04-28T16:42:30Z

It may help yes.

yorkxin · 2014-04-29T08:22:43Z

OK, I'll rebase onto master later today (living in UTC+8)

yorkxin · 2014-04-29T16:39:25Z

I found it is hard to rebase because I don't know how to fix unit tests :(

houndci-bot · 2014-04-29T16:42:06Z

lib/doorkeeper/config.rb

+      response_types = []
+
+      response_types << :authorization_code if flows.include? :authorization_code
+      response_types << :password           if flows.include? :password_credentials


Line is too long. [83/80]

tute · 2014-04-30T17:20:52Z

Good work, thank you! Sorry I delayed so much for the review, looking good!

yorkxin · 2014-04-30T17:26:31Z

No problem! I'm correcting my code according to your advise. Actually didn't know those symbol DoS issues at that time until recently :( Sorry for that.

yorkxin · 2014-04-30T18:21:07Z

spec/lib/config_spec.rb

@@ -154,7 +171,7 @@

  describe "wildcard_redirect_uri" do
    it "is disabled by default" do
-      Doorkeeper.configuration.wildcard_redirect_uri.should be_false
+      expect(Doorkeeper.configuration.wildcard_redirect_uri).to be_false


Accidentally changed this line. Will revert.

yorkxin · 2014-04-30T18:33:47Z

All parameters are now in strings instead of symbols.
Use a simpler term password for Resource Owner Password Credentials Grant Flow
Improved notes about grant_flows command in initializer template.
When requesting for token refresh but Doorkeeper's refresh token is not enabled, the server returns unsupported_grant_type error.

I think it's good to merge. Feel free to tell me if there is anything to improve.

tute · 2014-04-30T20:48:38Z

lib/doorkeeper/config.rb

+
+      types << 'refresh_token' if refresh_token_enabled?
+
+      types


This method's body can be replaced by:

types = flows - ['implicit'] types << 'refresh_token' if refresh_token_enabled? types

We don't need the comment of line 243 (unless we link to the four related parts of the spec, your take).

We don't need the flows parameter, as it's an accessible option in this scope.

tute · 2014-05-02T17:57:12Z

There's something wrong in this PR: https://travis-ci.org/doorkeeper-gem/doorkeeper/jobs/24286507

yorkxin · 2014-05-02T17:58:28Z

Got it. Will dig into this.

tute · 2014-05-02T17:58:54Z

Probably we do need orm DOORKEEPER_ORM. Sorry, it seems tests need it, I made you take them out.

tute · 2014-05-02T17:59:59Z

For ActiveRecord they pass because that's default, and when trying mongoid it fails. That will do the trick! Sorry again, my mistake.

yorkxin · 2014-05-02T18:01:30Z

Oh, no problem, and I will not blame on you 😄

I think we need a way to run tests locally using Mongoid.

tute · 2014-05-02T18:02:37Z

There is, we have to install and run mongoid, and define the ORM env variable. See https://github.com/doorkeeper-gem/doorkeeper/blob/master/.travis.yml.

yorkxin · 2014-05-02T18:04:39Z

Thanks, I'll run test through all mongo adapters and make sure that they pass.

houndci-bot · 2014-05-02T18:11:46Z

spec/requests/endpoints/token_spec.rb

+  end
+
+  scenario 'returns unsupported_grant_type when refresh_token is not in use' do
+    post token_endpoint_url(:code => @authorization.token, :client => @client, :grant_type => 'refresh_token')


Use the new Ruby 1.9 hash syntax.
Line is too long. [110/80]

yorkxin · 2014-05-02T18:12:31Z

Fixed. Waiting for Travis CI :p

tute · 2014-05-02T18:24:28Z

Well, just merged a big Pull Request. Rebase needed, sorry to bother, and thank you for being so responsive!

yorkxin · 2014-05-02T18:25:29Z

No problem. I think I can do another rebase again.

yorkxin · 2014-05-02T18:40:51Z

Some test are failed. I'll fix them and push again.

defaults to all grant flows + 'refresh_token'

Note: I'm not sure whether this is necessary, as there is already a validation in Doorkeeper::Request.

…when it is not enabled explicitly.

houndci-bot · 2014-05-02T18:57:00Z

spec/requests/endpoints/token_spec.rb

+
+    should_not_have_json 'access_token'
+    should_have_json 'error', 'unsupported_grant_type'
+    should_have_json 'error_description', translated_error_message('unsupported_grant_type')


Line is too long. [92/80]

yorkxin · 2014-05-02T18:58:40Z

Done rebasing! I've also converted hash to 1.9 style.

By the way according to test cases, it seems that if specifying grant_flows in symbols (i.e. %i(implicit password)) the grant flow restriction will also work.

yorkxin · 2014-05-02T19:00:29Z

Previously the tests are setting grant_flows in array of symbols. I've corrected them to array of strings, but the tests pass in both cases.

Only enable specific grant flows.

tute · 2014-05-03T13:28:02Z

Thank you! ❤️

yorkxin · 2014-05-03T13:28:38Z

👍 Thanks a lot!

sjfbo · 2014-05-03T15:03:59Z

Good job @chitsaou and @tute !

sjfbo reviewed Apr 28, 2014
View reviewed changes

houndci-bot reviewed Apr 29, 2014
View reviewed changes

yorkxin reviewed Apr 30, 2014
View reviewed changes

yorkxin mentioned this pull request Apr 30, 2014

Block Flows #238

Closed

tute reviewed Apr 30, 2014
View reviewed changes

houndci-bot reviewed May 2, 2014
View reviewed changes

yorkxin added 5 commits May 3, 2014 02:55

add configuration "grant_flows"

b80fd2b

defaults to all grant flows + 'refresh_token'

Hint default grant flows in initializer template

ea9803b

check for available strategies before processing on endpoints

bd4a8e4

completely turn off disabled strategies on token endpoint (see note)

c7654f2

Note: I'm not sure whether this is necessary, as there is already a validation in Doorkeeper::Request.

test that refresh_token request returns unsupported_grant_type error …

14f3e3a

…when it is not enabled explicitly.

houndci-bot reviewed May 2, 2014
View reviewed changes

tute added a commit that referenced this pull request May 3, 2014

Merge pull request #295 from chitsaou/config-grant-flows

0ce32ff

Only enable specific grant flows.

tute merged commit 0ce32ff into doorkeeper-gem:master May 3, 2014

tute mentioned this pull request Oct 29, 2015

Add bundler-audit gem to audit gems we use thoughtbot/upcase#1476

Merged

Only enable specific grant flows. #295

Only enable specific grant flows. #295

Conversation

yorkxin commented Oct 8, 2013

yorkxin commented Oct 17, 2013

tute commented Oct 17, 2013

yorkxin commented Oct 17, 2013

kevintom commented Oct 21, 2013

birarda commented Feb 12, 2014

yorkxin commented Feb 13, 2014

sjfbo commented Apr 28, 2014

sjfbo Apr 28, 2014

Choose a reason for hiding this comment

yorkxin Apr 28, 2014

Choose a reason for hiding this comment

yorkxin commented Apr 28, 2014

sjfbo commented Apr 28, 2014

yorkxin commented Apr 29, 2014

yorkxin commented Apr 29, 2014

houndci-bot Apr 29, 2014

Choose a reason for hiding this comment

tute commented Apr 30, 2014

yorkxin commented Apr 30, 2014

yorkxin Apr 30, 2014

Choose a reason for hiding this comment

yorkxin commented Apr 30, 2014

tute Apr 30, 2014

Choose a reason for hiding this comment

tute commented May 2, 2014

yorkxin commented May 2, 2014

tute commented May 2, 2014

tute commented May 2, 2014

yorkxin commented May 2, 2014

tute commented May 2, 2014

yorkxin commented May 2, 2014

houndci-bot May 2, 2014

Choose a reason for hiding this comment

yorkxin commented May 2, 2014

tute commented May 2, 2014

yorkxin commented May 2, 2014

yorkxin commented May 2, 2014

houndci-bot May 2, 2014

Choose a reason for hiding this comment

yorkxin commented May 2, 2014

yorkxin commented May 2, 2014

tute commented May 3, 2014

yorkxin commented May 3, 2014

sjfbo commented May 3, 2014