From eca7000d1f54cc23124d60fe48abbb09b03e1df5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20Krzemi=C5=84ski-Freda?= Date: Tue, 19 Apr 2016 18:09:01 +0200 Subject: [PATCH] Comment updates --- NEWS.md | 4 +- lib/doorkeeper/models/concerns/revocable.rb | 7 +++ lib/doorkeeper/oauth/refresh_token_request.rb | 30 ++++++--- lib/doorkeeper/oauth/token.rb | 8 +-- .../protected_resources_controller_spec.rb | 24 ++++--- spec/lib/models/revocable_spec.rb | 12 +++- spec/lib/oauth/refresh_token_request_spec.rb | 4 +- spec/lib/oauth/token_spec.rb | 4 +- spec/requests/flows/refresh_token_spec.rb | 62 ++++++++++++++----- .../shared/controllers_shared_context.rb | 13 ++-- 10 files changed, 117 insertions(+), 51 deletions(-) diff --git a/NEWS.md b/NEWS.md index a024708fd..0fa9ad1ea 100644 --- a/NEWS.md +++ b/NEWS.md @@ -3,13 +3,11 @@ User-visible changes worth mentioning. --- - -- [#769] Revoke Refresh Token on access token use - ## 4.0.0.rc3 - Toughen parameters filter with exact match - Drop `attr_accessible` from models +- [#769] Revoke Refresh Token on access token use ### Backward incompatible changes - Force all timezones to use UTC to prevent comparison issues. diff --git a/lib/doorkeeper/models/concerns/revocable.rb b/lib/doorkeeper/models/concerns/revocable.rb index e423311ac..1d52cd530 100644 --- a/lib/doorkeeper/models/concerns/revocable.rb +++ b/lib/doorkeeper/models/concerns/revocable.rb @@ -10,14 +10,21 @@ def revoked? end def revoke_previous_refresh_token! + return unless refresh_token_revoked_on_use? old_refresh_token.revoke if old_refresh_token update_attribute :previous_refresh_token, "" end + private + def old_refresh_token @old_refresh_token ||= AccessToken.by_refresh_token(previous_refresh_token) end + + def refresh_token_revoked_on_use? + Doorkeeper.configuration.refresh_token_revoked_on_use? + end end end end diff --git a/lib/doorkeeper/oauth/refresh_token_request.rb b/lib/doorkeeper/oauth/refresh_token_request.rb index 15a0244be..80e169f8e 100644 --- a/lib/doorkeeper/oauth/refresh_token_request.rb +++ b/lib/doorkeeper/oauth/refresh_token_request.rb @@ -35,30 +35,40 @@ def before_successful_response refresh_token.transaction do refresh_token.lock! raise Errors::InvalidTokenReuse if refresh_token.revoked? - refresh_token.revoke unless server.refresh_token_revoked_on_use? + refresh_token.revoke unless refresh_token_revoked_on_use? create_access_token end end + def refresh_token_revoked_on_use? + server.refresh_token_revoked_on_use? + end + def default_scopes refresh_token.scopes end def create_access_token - expires_in = Authorization::Token.access_token_expires_in( - server, - client - ) + @access_token = AccessToken.create!(access_token_attributes) + end - @access_token = AccessToken.create!( + def access_token_attributes + { application_id: refresh_token.application_id, resource_owner_id: refresh_token.resource_owner_id, scopes: scopes.to_s, - expires_in: expires_in, - use_refresh_token: true, - previous_refresh_token: refresh_token.refresh_token - ) + expires_in: access_token_expires_in, + use_refresh_token: true + }.tap do |attributes| + if refresh_token_revoked_on_use? + attributes[:previous_refresh_token] = refresh_token.refresh_token + end + end + end + + def access_token_expires_in + Authorization::Token.access_token_expires_in(server, client) end def validate_token_presence diff --git a/lib/doorkeeper/oauth/token.rb b/lib/doorkeeper/oauth/token.rb index d2dd9eb20..0c40510fa 100644 --- a/lib/doorkeeper/oauth/token.rb +++ b/lib/doorkeeper/oauth/token.rb @@ -41,10 +41,6 @@ def token_from_header(header, pattern) def match?(header, pattern) header && header.match(pattern) end - - def refresh_token_revoked_on_use? - Doorkeeper.configuration.refresh_token_revoked_on_use? - end end extend Methods @@ -60,9 +56,7 @@ def self.from_request(request, *methods) def self.authenticate(request, *methods) if token = from_request(request, *methods) access_token = AccessToken.by_token(token) - if access_token && refresh_token_revoked_on_use? - access_token.revoke_previous_refresh_token! - end + access_token.revoke_previous_refresh_token! if access_token access_token end end diff --git a/spec/controllers/protected_resources_controller_spec.rb b/spec/controllers/protected_resources_controller_spec.rb index 28cb48a0f..3d6f63e36 100644 --- a/spec/controllers/protected_resources_controller_spec.rb +++ b/spec/controllers/protected_resources_controller_spec.rb @@ -29,7 +29,8 @@ def index let(:token_string) { '1A2BC3' } let(:token) do double(Doorkeeper::AccessToken, - acceptable?: true, previous_refresh_token: "", revoke_previous_refresh_token!: true) + acceptable?: true, previous_refresh_token: "", + revoke_previous_refresh_token!: true) end it 'access_token param' do @@ -108,19 +109,25 @@ def index it 'allows if the token has particular scopes' do token = double(Doorkeeper::AccessToken, - accessible?: true, scopes: %w(write public), previous_refresh_token: "", + accessible?: true, scopes: %w(write public), + previous_refresh_token: "", revoke_previous_refresh_token!: true) expect(token).to receive(:acceptable?).with([:write]).and_return(true) - expect(Doorkeeper::AccessToken).to receive(:by_token).with(token_string).and_return(token) + expect( + Doorkeeper::AccessToken + ).to receive(:by_token).with(token_string).and_return(token) get :index, access_token: token_string expect(response).to be_success end it 'does not allow if the token does not include given scope' do token = double(Doorkeeper::AccessToken, - accessible?: true, scopes: ['public'], revoked?: false, expired?: false, - previous_refresh_token: "", revoke_previous_refresh_token!: true) - expect(Doorkeeper::AccessToken).to receive(:by_token).with(token_string).and_return(token) + accessible?: true, scopes: ['public'], revoked?: false, + expired?: false, previous_refresh_token: "", + revoke_previous_refresh_token!: true) + expect( + Doorkeeper::AccessToken + ).to receive(:by_token).with(token_string).and_return(token) expect(token).to receive(:acceptable?).with([:write]).and_return(false) get :index, access_token: token_string expect(response.status).to eq 403 @@ -212,8 +219,9 @@ def doorkeeper_forbidden_render_options(*) let(:token) do double(Doorkeeper::AccessToken, - accessible?: true, scopes: ['public'], revoked?: false, expired?: false, - previous_refresh_token: "", revoke_previous_refresh_token!: true) + accessible?: true, scopes: ['public'], revoked?: false, + expired?: false, previous_refresh_token: "", + revoke_previous_refresh_token!: true) end let(:token_string) { '1A2DUWE' } diff --git a/spec/lib/models/revocable_spec.rb b/spec/lib/models/revocable_spec.rb index 67b6d098f..1d533da45 100644 --- a/spec/lib/models/revocable_spec.rb +++ b/spec/lib/models/revocable_spec.rb @@ -36,14 +36,20 @@ end describe :revoke_previous_refresh_token! do - it "revokes the previous token if existing, and resets the `previous_refresh_token` attribute" do - previous_token = FactoryGirl.create(:access_token, refresh_token: "refresh_token") + it "revokes the previous token if existing, and resets the + `previous_refresh_token` attribute" do + previous_token = FactoryGirl.create( + :access_token, + refresh_token: "refresh_token" + ) current_token = FactoryGirl.create( :access_token, previous_refresh_token: previous_token.refresh_token ) - expect_any_instance_of(Doorkeeper::AccessToken).to receive(:revoke).and_call_original + expect_any_instance_of( + Doorkeeper::AccessToken + ).to receive(:revoke).and_call_original current_token.revoke_previous_refresh_token! expect(current_token.previous_refresh_token).to be_empty diff --git a/spec/lib/oauth/refresh_token_request_spec.rb b/spec/lib/oauth/refresh_token_request_spec.rb index 41f5920ab..dca07fdf2 100644 --- a/spec/lib/oauth/refresh_token_request_spec.rb +++ b/spec/lib/oauth/refresh_token_request_spec.rb @@ -86,7 +86,9 @@ module Doorkeeper::OAuth it 'sets the previous refresh token in the new access token' do subject.authorize - expect(client.access_tokens.last.previous_refresh_token).to eq(refresh_token.refresh_token) + expect( + client.access_tokens.last.previous_refresh_token + ).to eq(refresh_token.refresh_token) end end diff --git a/spec/lib/oauth/token_spec.rb b/spec/lib/oauth/token_spec.rb index f88d67923..70300aa84 100644 --- a/spec/lib/oauth/token_spec.rb +++ b/spec/lib/oauth/token_spec.rb @@ -104,7 +104,9 @@ module OAuth it 'revokes previous refresh_token if token was found' do token = ->(_r) { 'token' } - expect(AccessToken).to receive(:by_token).with('token').and_return(token) + expect( + AccessToken + ).to receive(:by_token).with('token').and_return(token) expect(token).to receive(:revoke_previous_refresh_token!) Token.authenticate double, token end diff --git a/spec/requests/flows/refresh_token_spec.rb b/spec/requests/flows/refresh_token_spec.rb index 1ad840166..d1ebd2b3f 100644 --- a/spec/requests/flows/refresh_token_spec.rb +++ b/spec/requests/flows/refresh_token_spec.rb @@ -37,20 +37,33 @@ context 'refreshing the token' do before do - @token = FactoryGirl.create(:access_token, application: @client, resource_owner_id: 1, use_refresh_token: true) + @token = FactoryGirl.create( + :access_token, + application: @client, + resource_owner_id: 1, + use_refresh_token: true + ) end context "refresh_token revoked on use" do it 'client request a token with refresh token' do - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) - should_have_json 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + post refresh_token_endpoint_url( + client: @client, refresh_token: @token.refresh_token + ) + should_have_json( + 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + ) expect(@token.reload).not_to be_revoked end it 'client request a token with expired access token' do @token.update_attribute :expires_in, -100 - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) - should_have_json 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + post refresh_token_endpoint_url( + client: @client, refresh_token: @token.refresh_token + ) + should_have_json( + 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + ) expect(@token.reload).not_to be_revoked end end @@ -61,15 +74,23 @@ end it 'client request a token with refresh token' do - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) - should_have_json 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + post refresh_token_endpoint_url( + client: @client, refresh_token: @token.refresh_token + ) + should_have_json( + 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + ) expect(@token.reload).to be_revoked end it 'client request a token with expired access token' do @token.update_attribute :expires_in, -100 - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) - should_have_json 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + post refresh_token_endpoint_url( + client: @client, refresh_token: @token.refresh_token + ) + should_have_json( + 'refresh_token', Doorkeeper::AccessToken.last.refresh_token + ) expect(@token.reload).to be_revoked end end @@ -100,18 +121,29 @@ before do # enable password auth to simulate other devices config_is_set(:grant_flows, ["password"]) - config_is_set(:resource_owner_from_credentials) { User.authenticate! params[:username], params[:password] } + config_is_set(:resource_owner_from_credentials) do + User.authenticate! params[:username], params[:password] + end create_resource_owner - _another_token = post password_token_endpoint_url(client: @client, resource_owner: @resource_owner) + _another_token = post password_token_endpoint_url( + client: @client, resource_owner: @resource_owner + ) last_token.update_attribute :created_at, 5.seconds.ago - @token = FactoryGirl.create(:access_token, application: @client, resource_owner_id: @resource_owner.id, use_refresh_token: true) + @token = FactoryGirl.create( + :access_token, + application: @client, + resource_owner_id: @resource_owner.id, + use_refresh_token: true + ) @token.update_attribute :expires_in, -100 end context "refresh_token revoked on use" do it 'client request a token after creating another token with the same user' do - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) + post refresh_token_endpoint_url( + client: @client, refresh_token: @token.refresh_token + ) should_have_json 'refresh_token', last_token.refresh_token expect(@token.reload).not_to be_revoked @@ -124,7 +156,9 @@ end it 'client request a token after creating another token with the same user' do - post refresh_token_endpoint_url(client: @client, refresh_token: @token.refresh_token) + post refresh_token_endpoint_url( + client: @client, refresh_token: @token.refresh_token + ) should_have_json 'refresh_token', last_token.refresh_token expect(@token.reload).to be_revoked diff --git a/spec/support/shared/controllers_shared_context.rb b/spec/support/shared/controllers_shared_context.rb index 2aec613a7..eed039874 100644 --- a/spec/support/shared/controllers_shared_context.rb +++ b/spec/support/shared/controllers_shared_context.rb @@ -10,7 +10,9 @@ end before :each do - allow(Doorkeeper::AccessToken).to receive(:by_token).with(token_string).and_return(token) + allow( + Doorkeeper::AccessToken + ).to receive(:by_token).with(token_string).and_return(token) end end @@ -21,12 +23,15 @@ let :token do double(Doorkeeper::AccessToken, - accessible?: false, revoked?: false, expired?: false, includes_scope?: false, - acceptable?: false, previous_refresh_token: "", revoke_previous_refresh_token!: true) + accessible?: false, revoked?: false, expired?: false, + includes_scope?: false, acceptable?: false, + previous_refresh_token: "", revoke_previous_refresh_token!: true) end before :each do - allow(Doorkeeper::AccessToken).to receive(:by_token).with(token_string).and_return(token) + allow( + Doorkeeper::AccessToken + ).to receive(:by_token).with(token_string).and_return(token) end end