Comment updates

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.

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

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

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

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' }
 

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

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
 

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

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

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