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

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

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
 

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]

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

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

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

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