Properly implement optional authentication for token revocation

Previously, Doorkeeper failed to implement OAuth 2.0 Token Revocation (RFC 7009) in the following ways:

1. Public clients making valid, unauthenticated calls to revoke a token would not have their token revoked
2. Requests were not properly authenticating the *client credentials* but were, instead, looking at the access token in a second location
3. Because of 2, the requests were also not authorizing confidential clients' ability to revoke a given token. It should only revoke tokens that belong to it.

This patch assumes that all public clients issue tokens with a NULL application_id, which may or may not be completely correct.

CVE-2016-6582 has been assigned due to the security issues raised. An attacker, thanks to 1, can replay a hijacked session after a victim logs out/revokes their token. Additionally, thanks to 2 & 3, an attacker via a compromised confidential client could "grief" other clients by revoking their tokens (albeit this is an exceptionally narrow attack with little value).

This patch:

1. Let's public clients revoke their access & refresh tokens anonymously (as per spec)
2. Authenticates OAuth 2.0 client/application credentials before revoking a token if it belongs to a confidential client (as per spec)
3. Verifies that an authenticated client owns the token it wishes to revoke in the request. (as per spec)

[fixes #875]

app/controllers/doorkeeper/tokens_controller.rb

@@ -11,29 +11,65 @@ def create
 
     # OAuth 2.0 Token Revocation - http://tools.ietf.org/html/rfc7009
     def revoke
-      # The authorization server first validates the client credentials
-      if doorkeeper_token && doorkeeper_token.accessible?
-        # Doorkeeper does not use the token_type_hint logic described in the RFC 7009
-        # due to the refresh token implementation that is a field in the access token model.
-        revoke_token(request.POST['token']) if request.POST['token']
+      # The authorization server, if applicable, first authenticates the client
+      # and checks its ownership of the provided token.
+      #
+      # Doorkeeper does not use the token_type_hint logic described in the
+      # RFC 7009 due to the refresh token implementation that is a field in
+      # the access token model.
+      if authorized?
+        revoke_token
       end
-      # The authorization server responds with HTTP status code 200 if the
-      # token has been revoked successfully or if the client submitted an invalid token
+
+      # The authorization server responds with HTTP status code 200 if the token
+      # has been revoked successfully or if the client submitted an invalid
+      # token
       render json: {}, status: 200
     end
 
     private
 
-    def revoke_token(token)
-      token = AccessToken.by_token(token) || AccessToken.by_refresh_token(token)
-      if token && doorkeeper_token.same_credential?(token)
+    # OAuth 2.0 Section 2.1 defines two client types, "public" & "confidential".
+    # Public clients (as per RFC 7009) do not require authentication whereas
+    # confidential clients must be authenticated for their token revocation.
+    #
+    # Once a confidential client is authenticated, it must be authorized to
+    # revoke the provided access or refresh token. This ensures one client
+    # cannot revoke another's tokens.
+    #
+    # Doorkeeper determines the client type implicitly via the presence of the
+    # OAuth client associated with a given access or refresh token. Since public
+    # clients authenticate the resource owner via "password" or "implicit" grant
+    # types, they set the application_id as null (since the claim cannot be
+    # verified).
+    #
+    # https://tools.ietf.org/html/rfc6749#section-2.1
+    # https://tools.ietf.org/html/rfc7009
+    def authorized?
+      if token.present?
+        # Client is confidential, therefore client authentication & authorization
+        # is required
+        if token.application_id?
+          # We authorize client by checking token's application
+          server.client && server.client.application == token.application
+        else
+          # Client is public, authentication unnecessary
+          true
+        end
+      end
+    end
+
+    def revoke_token
+      if token.accessible?
         token.revoke
-        true
-      else
-        false
       end
     end
 
+    def token
+      @token ||= AccessToken.by_token(request.POST['token']) ||
+        AccessToken.by_refresh_token(request.POST['token'])
+    end
+
     def strategy
       @strategy ||= server.token_request params[:grant_type]
     end

spec/controllers/tokens_controller_spec.rb

@@ -64,7 +64,7 @@
   describe 'when revoke authorization has failed' do
     # http://tools.ietf.org/html/rfc7009#section-2.2
     it 'returns no error response' do
-      token = double(:token, authorize: false)
+      token = double(:token, authorize: false, application_id?: true)
       allow(controller).to receive(:token) { token }
 
       post :revoke

spec/requests/flows/revoke_token_spec.rb

@@ -8,135 +8,149 @@
   context 'with default parameters' do
     let(:client_application) { FactoryGirl.create :application }
     let(:resource_owner) { User.create!(name: 'John', password: 'sekret') }
-    let(:authorization_access_token) do
+    let(:access_token) do
       FactoryGirl.create(:access_token,
                          application: client_application,
                          resource_owner_id: resource_owner.id,
                          use_refresh_token: true)
     end
-    let(:headers) { { 'HTTP_AUTHORIZATION' => "Bearer #{authorization_access_token.token}" } }
 
-    context 'With invalid token to revoke' do
-      it 'client wants to revoke the given access token' do
-        post revocation_token_endpoint_url, { token: 'I_AM_AN_INVALIDE_TOKEN' }, headers
-
-        authorization_access_token.reload
-        # The authorization server responds with HTTP status code 200 if the token
-        # has been revoked successfully or if the client submitted an invalid token.
-        expect(response).to be_success
-        expect(authorization_access_token).to_not be_revoked
+    context 'with authenticated, confidential OAuth 2.0 client/application' do
+      let(:headers) do
+        client_id = client_application.uid
+        client_secret = client_application.secret
+        credentials = Base64.encode64("#{client_id}:#{client_secret}")
+        { 'HTTP_AUTHORIZATION' => "Basic #{credentials}" }
       end
-    end
-
-    context 'The access token to revoke is the same than the authorization access token' do
-      let(:token_to_revoke) { authorization_access_token }
 
-      it 'client wants to revoke the given access token' do
-        post revocation_token_endpoint_url, { token: token_to_revoke.token }, headers
+      it 'should revoke the access token provided' do
+        post revocation_token_endpoint_url, { token: access_token.token }, headers
 
-        token_to_revoke.reload
-        authorization_access_token.reload
+        access_token.reload
 
         expect(response).to be_success
-        expect(token_to_revoke.revoked?).to be_truthy
-        expect(Doorkeeper::AccessToken.by_refresh_token(token_to_revoke.refresh_token).revoked?).to be_truthy
+        expect(access_token.revoked?).to be_truthy
       end
 
-      it 'client wants to revoke the given access token using the POST query string' do
-        url_with_query_string = revocation_token_endpoint_url + '?' + Rack::Utils.build_query(token: token_to_revoke.token)
-        post url_with_query_string, {}, headers
+      it 'should revoke the refresh token provided' do
+        post revocation_token_endpoint_url, { token: access_token.refresh_token }, headers
 
-        token_to_revoke.reload
-        authorization_access_token.reload
+        access_token.reload
 
         expect(response).to be_success
-        expect(token_to_revoke.revoked?).to be_falsey
-        expect(Doorkeeper::AccessToken.by_refresh_token(token_to_revoke.refresh_token).revoked?).to be_falsey
-        expect(authorization_access_token.revoked?).to be_falsey
+        expect(access_token.revoked?).to be_truthy
       end
-    end
 
-    context 'The access token to revoke app and owners are the same than the authorization access token' do
-      let(:token_to_revoke) do
-        FactoryGirl.create(:access_token,
-                           application: client_application,
-                           resource_owner_id: resource_owner.id,
-                           use_refresh_token: true)
+      context 'with invalid token to revoke' do
+        it 'should not revoke any tokens and respond successfully' do
+          num_prev_revoked_tokens = Doorkeeper::AccessToken.where(revoked_at: nil).count
+          post revocation_token_endpoint_url, { token: 'I_AM_AN_INVALID_TOKEN' }, headers
+
+          # The authorization server responds with HTTP status code 200 even if
+          # token is invalid
+          expect(response).to be_success
+          expect(Doorkeeper::AccessToken.where(revoked_at: nil).count).to eq(num_prev_revoked_tokens)
+        end
       end
 
-      it 'client wants to revoke the given access token' do
-        post revocation_token_endpoint_url, { token: token_to_revoke.token }, headers
+      context 'with bad credentials and a valid token' do
+        let(:headers) do
+          client_id = client_application.uid
+          credentials = Base64.encode64("#{client_id}:poop")
+          { 'HTTP_AUTHORIZATION' => "Basic #{credentials}" }
+        end
+        it 'should not revoke any tokens and respond successfully' do
+          post revocation_token_endpoint_url, { token: access_token.token }, headers
 
-        token_to_revoke.reload
-        authorization_access_token.reload
+          access_token.reload
 
-        expect(response).to be_success
-        expect(token_to_revoke.revoked?).to be_truthy
-        expect(Doorkeeper::AccessToken.by_refresh_token(token_to_revoke.refresh_token).revoked?).to be_truthy
-        expect(authorization_access_token.revoked?).to be_falsey
+          expect(response).to be_success
+          expect(access_token.revoked?).to be_falsey
+        end
       end
-    end
 
-    context 'The access token to revoke authorization owner is the same than the authorization access token' do
-      let(:other_client_application) { FactoryGirl.create :application }
-      let(:token_to_revoke) do
-        FactoryGirl.create(:access_token,
-                           application: other_client_application,
-                           resource_owner_id: resource_owner.id,
-                           use_refresh_token: true)
+      context 'with no credentials and a valid token' do
+        it 'should not revoke any tokens and respond successfully' do
+          post revocation_token_endpoint_url, { token: access_token.token }
+
+          access_token.reload
+
+          expect(response).to be_success
+          expect(access_token.revoked?).to be_falsey
+        end
       end
 
-      it 'client wants to revoke the given access token' do
-        post revocation_token_endpoint_url, { token: token_to_revoke.token }, headers
+      context 'with valid token for another client application' do
+        let(:other_client_application) { FactoryGirl.create :application }
+        let(:headers) do
+          client_id = other_client_application.uid
+          client_secret = other_client_application.secret
+          credentials = Base64.encode64("#{client_id}:#{client_secret}")
+          { 'HTTP_AUTHORIZATION' => "Basic #{credentials}" }
+        end
 
-        token_to_revoke.reload
-        authorization_access_token.reload
+        it 'should not revoke the token as its unauthorized' do
+          post revocation_token_endpoint_url, { token: access_token.token }, headers
 
-        expect(response).to be_success
-        expect(token_to_revoke.revoked?).to be_falsey
-        expect(Doorkeeper::AccessToken.by_refresh_token(token_to_revoke.refresh_token).revoked?).to be_falsey
-        expect(authorization_access_token.revoked?).to be_falsey
+          access_token.reload
+
+          expect(response).to be_success
+          expect(access_token.revoked?).to be_falsey
+        end
       end
     end
 
-    context 'The access token to revoke app is the same than the authorization access token' do
-      let(:other_resource_owner) { User.create!(name: 'Matheo', password: 'pareto') }
-      let(:token_to_revoke) do
+    context 'with public OAuth 2.0 client/application' do
+      let(:access_token) do
         FactoryGirl.create(:access_token,
-                           application: client_application,
-                           resource_owner_id: other_resource_owner.id,
+                           application: nil,
+                           resource_owner_id: resource_owner.id,
                            use_refresh_token: true)
       end
 
-      it 'client wants to revoke the given access token' do
-        post revocation_token_endpoint_url, { token: token_to_revoke.token }, headers
+      it 'should revoke the access token provided' do
+        post revocation_token_endpoint_url, { token: access_token.token }
 
-        token_to_revoke.reload
-        authorization_access_token.reload
+        access_token.reload
 
         expect(response).to be_success
-        expect(token_to_revoke.revoked?).to be_falsey
-        expect(Doorkeeper::AccessToken.by_refresh_token(token_to_revoke.refresh_token).revoked?).to be_falsey
-        expect(authorization_access_token.revoked?).to be_falsey
+        expect(access_token.revoked?).to be_truthy
       end
-    end
 
-    context 'With valid refresh token to revoke' do
-      let(:token_to_revoke) do
-        FactoryGirl.create(:access_token,
-                           application: client_application,
-                           resource_owner_id: resource_owner.id,
-                           use_refresh_token: true)
-      end
+      it 'should revoke the refresh token provided' do
+        post revocation_token_endpoint_url, { token: access_token.refresh_token }
 
-      it 'client wants to revoke the given refresh token' do
-        post revocation_token_endpoint_url, { token: token_to_revoke.refresh_token, token_type_hint: 'refresh_token' }, headers
-        authorization_access_token.reload
-        token_to_revoke.reload
+        access_token.reload
 
         expect(response).to be_success
-        expect(Doorkeeper::AccessToken.by_refresh_token(token_to_revoke.refresh_token).revoked?).to be_truthy
-        expect(authorization_access_token).to_not be_revoked
+        expect(access_token.revoked?).to be_truthy
+      end
+
+      context 'with a valid token issued for a confidential client' do
+        let(:access_token) do
+          FactoryGirl.create(:access_token,
+                             application: client_application,
+                             resource_owner_id: resource_owner.id,
+                             use_refresh_token: true)
+        end
+
+        it 'should not revoke the access token provided' do
+          post revocation_token_endpoint_url, { token: access_token.token }
+
+          access_token.reload
+
+          expect(response).to be_success
+          expect(access_token.revoked?).to be_falsey
+        end
+
+        it 'should not revoke the refresh token provided' do
+          post revocation_token_endpoint_url, { token: access_token.token }
+
+          access_token.reload
+
+          expect(response).to be_success
+          expect(access_token.revoked?).to be_falsey
+        end
       end
     end
   end