From 3146f40257a85c7a81c916a8d20eeaa4100b67d1 Mon Sep 17 00:00:00 2001 From: Phillip Baker Date: Mon, 29 Jun 2015 08:39:07 -0400 Subject: [PATCH] Wrap access token grant and refresh in transactions. It's possible to exchange a single grant/refresh token for multiple access tokens. This is definitely _unexpected_ behavior, whether its a vulnerability is a bit murkier. In a highly concurrent environment, it may provide unexpected results to consumers of the API as well. For the initial exchange of grant for access token, using a grant more than once seems to violate Section 4.2.1 [1]: 4.1.2. Authorization Response code The authorization code generated by the authorization server. ...The client MUST NOT use the authorization code more than once. In the transaction, the `lock!` and error raise is the important part to prevent multiple writes concurrently. In ActiveRecord, `lock!` reloads the record after doing a SELECT ... FOR UPDATE so if two parallel requests come, the first one to the DB locks and revokes the token, while the second waits for the lock to be be released, then acquires the lock, reloading the (now revoked) record and raises an error. A side effect of this change is that granting and refreshing tokens is now transactional - a token cannot be granted without also revoking the grant which should improve consistency. [1] https://tools.ietf.org/html/rfc6749#section-4.1.2 --- NEWS.md | 2 ++ lib/doorkeeper/errors.rb | 6 +++++ lib/doorkeeper/helpers/controller.rb | 4 +++ .../oauth/authorization_code_request.rb | 15 +++++++---- lib/doorkeeper/oauth/refresh_token_request.rb | 9 +++++-- .../requests/flows/authorization_code_spec.rb | 26 +++++++++++++++++++ spec/requests/flows/refresh_token_spec.rb | 8 ++++++ 7 files changed, 63 insertions(+), 7 deletions(-) diff --git a/NEWS.md b/NEWS.md index 9d5ee8236..412626da3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,8 @@ User-visible changes worth mentioning. --- +- [#712] Wrap exchange of grant token for access token and access token refresh in transactions. + ## 3.0.0 ### Other changes diff --git a/lib/doorkeeper/errors.rb b/lib/doorkeeper/errors.rb index 49cbbdef9..82bf44177 100644 --- a/lib/doorkeeper/errors.rb +++ b/lib/doorkeeper/errors.rb @@ -6,6 +6,12 @@ class DoorkeeperError < StandardError class InvalidAuthorizationStrategy < DoorkeeperError end + class InvalidTokenReuse < DoorkeeperError + end + + class InvalidGrantReuse < DoorkeeperError + end + class InvalidTokenStrategy < DoorkeeperError end diff --git a/lib/doorkeeper/helpers/controller.rb b/lib/doorkeeper/helpers/controller.rb index 614a6aed8..71e33a215 100644 --- a/lib/doorkeeper/helpers/controller.rb +++ b/lib/doorkeeper/helpers/controller.rb @@ -41,6 +41,10 @@ def get_error_response_from_exception(exception) :unsupported_response_type when Errors::MissingRequestStrategy :invalid_request + when Errors::InvalidTokenReuse + :invalid_request + when Errors::InvalidGrantReuse + :invalid_grant end OAuth::ErrorResponse.new name: error_name, state: params[:state] diff --git a/lib/doorkeeper/oauth/authorization_code_request.rb b/lib/doorkeeper/oauth/authorization_code_request.rb index 6663d82fd..73197b2fe 100644 --- a/lib/doorkeeper/oauth/authorization_code_request.rb +++ b/lib/doorkeeper/oauth/authorization_code_request.rb @@ -21,11 +21,16 @@ def initialize(server, grant, client, parameters = {}) private def before_successful_response - grant.revoke - find_or_create_access_token(grant.application, - grant.resource_owner_id, - grant.scopes, - server) + grant.transaction do + grant.lock! + raise Errors::InvalidGrantReuse if grant.revoked? + + grant.revoke + find_or_create_access_token(grant.application, + grant.resource_owner_id, + grant.scopes, + server) + end end def validate_attributes diff --git a/lib/doorkeeper/oauth/refresh_token_request.rb b/lib/doorkeeper/oauth/refresh_token_request.rb index 3718968d6..8782571f9 100644 --- a/lib/doorkeeper/oauth/refresh_token_request.rb +++ b/lib/doorkeeper/oauth/refresh_token_request.rb @@ -32,8 +32,13 @@ def initialize(server, refresh_token, credentials, parameters = {}) attr_reader :refresh_token_parameter def before_successful_response - refresh_token.revoke - create_access_token + refresh_token.transaction do + refresh_token.lock! + raise Errors::InvalidTokenReuse if refresh_token.revoked? + + refresh_token.revoke + create_access_token + end end def default_scopes diff --git a/spec/requests/flows/authorization_code_spec.rb b/spec/requests/flows/authorization_code_spec.rb index deb34f54c..435cc77b1 100644 --- a/spec/requests/flows/authorization_code_spec.rb +++ b/spec/requests/flows/authorization_code_spec.rb @@ -128,3 +128,29 @@ end end end + +describe 'Authorization Code Flow' do + before do + Doorkeeper.configure do + orm DOORKEEPER_ORM + use_refresh_token + end + client_exists + end + + context 'issuing a refresh token' do + before do + authorization_code_exists application: @client + end + + it 'second of simultaneous client requests get an error for revoked acccess token' do + authorization_code = Doorkeeper::AccessGrant.first.token + allow_any_instance_of(Doorkeeper::AccessGrant).to receive(:revoked?).and_return(false, true) + + post token_endpoint_url(code: authorization_code, client: @client) + + should_not_have_json 'access_token' + should_have_json 'error', 'invalid_grant' + end + end +end diff --git a/spec/requests/flows/refresh_token_spec.rb b/spec/requests/flows/refresh_token_spec.rb index cba411d28..6d3c53262 100644 --- a/spec/requests/flows/refresh_token_spec.rb +++ b/spec/requests/flows/refresh_token_spec.rb @@ -65,6 +65,14 @@ should_not_have_json 'refresh_token' should_have_json 'error', 'invalid_grant' end + + it 'second of simultaneous client requests get an error for revoked acccess token' do + allow_any_instance_of(Doorkeeper::AccessToken).to receive(:revoked?).and_return(false, true) + post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) + + should_not_have_json 'refresh_token' + should_have_json 'error', 'invalid_request' + end end context 'refreshing the token with multiple sessions (devices)' do