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

Only enable specific grant flows. #295

Merged
merged 5 commits into from May 3, 2014

Conversation

Projects
None yet
6 participants
@chitsaou
Copy link
Contributor

commented Oct 8, 2013

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:

  1. I don't know whether the commit chitsaou/doorkeeper@110c0d0 is necessary, since the "Unsupported Response Type" is filtered in Doorkeeper::Request. I can remove this commit if you think this is not necessary.
  2. Instead of RSpec tests, I've tested this feature with my demo app, and it seems work.
  3. 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!
  4. 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.
  5. 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)
@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2013

@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

This comment has been minimized.

Copy link
Member

commented Oct 17, 2013

@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.

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2013

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

@kevintom

This comment has been minimized.

Copy link

commented Oct 21, 2013

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

This comment has been minimized.

Copy link

commented Feb 12, 2014

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

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2014

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.

}
end

it "includes ':password' in token_grant_types" do

This comment has been minimized.

Copy link
@sjfbo

sjfbo Apr 28, 2014

Contributor

should be :client_credentials instead of :password

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 28, 2014

Author Contributor

Fixed. Thank you!

# grant_flows [
# :authorization_code,
# :implicit,
# :resource_owner_password_credentials,

This comment has been minimized.

Copy link
@sjfbo

sjfbo Apr 28, 2014

Contributor

if resource_owner_password_credentials is too long, maybe that password_credentials could be enough ?

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 28, 2014

Author Contributor

Good idea. I've changed to password_credentials. Thanks!

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2014

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

@sjfbo

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2014

It may help yes.

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2014

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

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2014

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

response_types = []

response_types << :authorization_code if flows.include? :authorization_code
response_types << :password if flows.include? :password_credentials

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 29, 2014

Line is too long. [83/80]

before do
Doorkeeper.configure {
orm DOORKEEPER_ORM
grant_flows [ :password_credentials ]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 29, 2014

Space inside square brackets detected.

types << :client_credentials if flows.include? :client_credentials

# FIXME: allow this response type accoring to refresh_token_enabled option
types << :refresh_token

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member

Can ask with refresh_token_enabled?.

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

thanks, will add it!

types = []

types << :authorization_code if flows.include? :authorization_code
types << :password if flows.include? :password_credentials

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member

If we name password_credentials flow to password, token_grant_types and grant_flows would be one and the same (excepting implicit and refresh token)? That could simplify code.

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

I should keep it consistent. I'll use :password_credentials instead.

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

OK, I know why I used :password instead of :password_credentials

Because in the OAuth 2 spec, when requesting token, the parameter will be grant_type=password. Therefore I think we should keep :password but add comments on why they're inconsistent.

see http://tools.ietf.org/html/rfc6749#section-4.3.2

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member

I'm ok with the simpler password. It's anyway inside of the grant_flows array, providing the context needed to understand what it is. Anyhow, the best way to know is to see the before/after code, your take.

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

You're right. It's much easier to understand. I'll take it!

%w[code token].include? response_type
# FIXME: the checking of strategies is done in Doorkeeper::Request.
# Is it proper to check the same logic here?
server.authorization_response_types.include? response_type.try(:to_sym)

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member
  1. Yes, let's check in both places (and remove the FIXME).
  2. Please change symbols to strings to avoid memory leaks: server.authorization_response_types.map(&:to_s).include? response_type.try(:to_s)

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

Thanks, will fix it!

@@ -170,6 +170,13 @@ def extended(base)
option :active_record_options, :default => {}
option :realm, :default => "Doorkeeper"
option :wildcard_redirect_uri, :default => false
option :grant_flows,

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member

I'd like this to read: default: %w(authorization_code implicit password client_credentials). That way they are strings and we avoid converting parameters to symbols (happens more than once in this PR). That allows memory leaks, and thus DoS attacks.

Also, array looks more like token_grant_types, and code can be simplified (see password change).

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member

Just read you'll use password_credentials instead of password, whichever as long as it's consistent! :)

rescue NameError
raise Errors::InvalidTokenStrategy
end

def get_strategy(strategy, available)
raise Errors::MissingRequestStrategy unless strategy.present?
raise NameError unless available.include?(strategy.to_s)
raise NameError unless available.include?(strategy.to_sym)

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member

Favor string over symbol.

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

Oh, that's my bad. Will fix it!

# :implicit,
# :password_credentials,
# :client_credentials
# ]

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member

If we go with using strings in this option, comment should reflect that.

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

I feel the syntax you proposed is better:

%w(authorization_code implicit password_credentials client_credentials)

and we can avoid all the potential symbol DoS's

@tute

This comment has been minimized.

Copy link
Member

commented Apr 30, 2014

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

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2014

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.

@@ -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

This comment has been minimized.

Copy link
@chitsaou

chitsaou Apr 30, 2014

Author Contributor

Accidentally changed this line. Will revert.


context 'refresh_token is not in use' do
scenario 'returns unsupported_grant_type for disabled grant flows' do
post token_endpoint_url(:code => @authorization.token, :client => @client, :grant_type => 'refresh_token')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2014

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


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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2014

Line is too long. [94/80]

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2014

  • 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.

@chitsaou chitsaou referenced this pull request Apr 30, 2014

Closed

Block Flows #238


types << 'refresh_token' if refresh_token_enabled?

types

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member
  • 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.
@@ -39,7 +39,8 @@ def error_response
private

def validate_response_type
%w[code token].include? response_type
# FIXME: the checking of strategies is done in Doorkeeper::Request.
server.authorization_response_types.include? response_type.try(:to_s)

This comment has been minimized.

Copy link
@tute

tute Apr 30, 2014

Member
  • We don't need the FIXME note.
  • We can always call to_s (no need of try). We probably don't need to_s call either.

This comment has been minimized.

Copy link
@chitsaou

chitsaou May 1, 2014

Author Contributor

thanks, will fix it.

@tute

This comment has been minimized.

Copy link
Member

commented May 2, 2014

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

Got it. Will dig into this.

@tute

This comment has been minimized.

Copy link
Member

commented May 2, 2014

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

@tute

This comment has been minimized.

Copy link
Member

commented May 2, 2014

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

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

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

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

@tute

This comment has been minimized.

Copy link
Member

commented May 2, 2014

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.

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

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

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')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2014

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


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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2014

Line is too long. [92/80]


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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2014

Line is too long. [92/80]

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

Fixed. Waiting for Travis CI :p

@tute

This comment has been minimized.

Copy link
Member

commented May 2, 2014

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

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

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

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

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

chitsaou added some commits Oct 8, 2013

add configuration "grant_flows"
defaults to all grant flows + 'refresh_token'
completely turn off disabled strategies on token endpoint (see note)
Note: I'm not sure whether this is necessary, as there is already a
validation in Doorkeeper::Request.

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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2014

Line is too long. [92/80]

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')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2014

Line is too long. [101/80]


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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2014

Line is too long. [92/80]

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

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.

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

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.

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

Merge pull request #295 from chitsaou/config-grant-flows
Only enable specific grant flows.

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

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@tute

This comment has been minimized.

Copy link
Member

commented May 3, 2014

Thank you! ❤️

@chitsaou

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2014

👍 Thanks a lot!

@sjfbo

This comment has been minimized.

Copy link
Contributor

commented May 3, 2014

Good job @chitsaou and @tute !

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