diff --git a/app/controllers/doorkeeper/tokens_controller.rb b/app/controllers/doorkeeper/tokens_controller.rb index bc09c0bd6..8d7510a7c 100644 --- a/app/controllers/doorkeeper/tokens_controller.rb +++ b/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 diff --git a/spec/controllers/tokens_controller_spec.rb b/spec/controllers/tokens_controller_spec.rb index 85eb2d752..8068af201 100644 --- a/spec/controllers/tokens_controller_spec.rb +++ b/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 diff --git a/spec/requests/flows/revoke_token_spec.rb b/spec/requests/flows/revoke_token_spec.rb index f89a76fd1..fa2f00a1b 100644 --- a/spec/requests/flows/revoke_token_spec.rb +++ b/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