Revoke Refresh Token on access token use #769

Fredar · 2015-12-29T21:38:45Z

Creating another PR based on #691. Copying description:

This PR allows refresh tokens to not either revoke at some point in the future (if you want to allow some wiggle room for network failures, etc) set refresh_token_revoked_in to some number of seconds in the future.

Also allows for refresh tokens to not be revoked until an access token created with that refresh token is successfully used once. Set refresh_token_revoked_on_use to true.

Both are off by default.

houndci-bot · 2015-12-29T21:38:57Z

spec/lib/oauth/refresh_token_request_spec.rb

+      end
+
+      context 'revoked token' do
+        time = DateTime.now + 1.day


Do not use DateTime.now without zone. Use one of Time.zone.now, DateTime.now.current, DateTime.now.in_time_zone, DateTime.now.utc, DateTime.now.getlocal, DateTime.now.iso8601, DateTime.now.jisx0301, DateTime.now.rfc3339, DateTime.now.to_i, DateTime.now.to_f instead.

Fredar · 2015-12-29T21:46:08Z

@tute I have updated code for all your comments from earlier PR, except this one.
I dont think we need to check for that, should be safe to revoke either way.
What do you think?

What about Time.now and Unnecessary spacing barks in lib/generators, should we ignore them?
Please have a general look, thanks!

tute · 2016-01-03T23:46:18Z

I have updated code for all your comments from earlier PR, except this one.
I dont think we need to check for that, should be safe to revoke either way.
What do you think?

You are probably right.

What about Time.now and Unnecessary spacing barks in lib/generators, should we ignore them?

Yep!

Please have a general look, thanks!

Doing it now. Thank you!

tute · 2016-01-03T23:49:13Z

spec/dummy/db/migrate/20150909133655_add_previous_refresh_token_to_access_tokens.rb

+  def change
+    add_column :oauth_access_tokens, :previous_refresh_token, :string, default: "", null: false
+  end
+end


Do you want to update this migration with the one with the new coding style? Thanks!

tute · 2016-01-03T23:58:13Z

Left a few comments, this is looking great. Thank you!

houndci-bot · 2016-01-07T00:44:50Z

spec/lib/oauth/refresh_token_request_spec.rb

+      end
+
+      before do
+        @time = Time.now.round


Do not use Time.now without zone. Use one of Time.zone.now, Time.now.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

Fredar · 2016-01-07T00:49:30Z

@tute Thanks for the comments.
I have updated or responded where needed.

Fredar · 2016-01-10T17:20:15Z

@tute I think we should instantly revoke token here:

def revoke_previous_refresh_token!
  if old_refresh_token && !old_refresh_token.revoked?
    old_refresh_token.revoke_in(refresh_token_revoked_in)
  end
  update_attribute :previous_refresh_token, ""
end

I will change the code to:

def revoke_previous_refresh_token!
  if old_refresh_token && !old_refresh_token.revoked?
    old_refresh_token.revoke
  end
  update_attribute :previous_refresh_token, ""
end

This method is only used during authentication.

houndci-bot · 2016-01-10T17:22:01Z

spec/lib/oauth/token_spec.rb

          token = ->(r) { 'token' }
          expect(AccessToken).to receive(:by_token).with('token')
          Token.authenticate double, token
        end
+
+        it 'revokes previous refresh_token if token was found' do
+          token = ->(r) { 'token' }


Unused block argument - r. If it's necessary, use _ or _r as an argument name to indicate that it won't be used. Also consider using a proc without arguments instead of a lambda if you want it to accept any arguments but don't care about them.

Fredar · 2016-01-18T10:20:18Z

@tute ping 😉

houndci-bot · 2016-01-20T10:09:50Z

spec/support/shared/controllers_shared_context.rb

-    double(Doorkeeper::AccessToken, accessible?: false, revoked?: false, expired?: false, includes_scope?: false, acceptable?: false)
+    double(Doorkeeper::AccessToken,
+           accessible?: false, revoked?: false, expired?: false, includes_scope?: false,
+           acceptable?: false, previous_refresh_token: "", revoke_previous_refresh_token!: true)


Line is too long. [96/80]

Fredar · 2016-04-19T16:20:33Z

@tute Thanks for a quick review!
Updated with your comments. I have only fixed line length for specs that are modified / added by this PR. Also didnt update in two cases where it was 81/80.

tute · 2016-04-20T00:37:12Z

Left a question around documentation for developers: how can they disable this option? Where should we document that?

Looking great, thanks for your work!

houndci-bot · 2016-04-20T06:41:17Z

lib/generators/doorkeeper/templates/migration.rb

+
+      # If you have a `previous_refresh_token` column, refresh tokens will be revoked
+      # only after a generated access token is used.
+      # Remove `previous_refresh_token` column, if you would like to have them revoked


Line is too long. [86/80]

Fredar · 2016-04-20T06:48:55Z

Thanks again for your time 😉
Added a comment to migration template and responded to your question here

tute · 2016-04-20T18:19:13Z

lib/generators/doorkeeper/templates/migration.rb

+      # If you have a `previous_refresh_token` column, refresh tokens
+      # will be revoked only after a generated access token is used.
+      # Remove `previous_refresh_token` column, if you would like
+      # to have them revoked as soon as a new access token is issued.


This is great! Suggested rewording:

If there is a previous_refresh_token column, refresh tokens will be revoked after a related access token is used. If there is no previous_refresh_token column, previous tokens are revoked as soon as a new access token is created. Comment out this line if you'd rather have refresh tokens instantly revoked.

tute · 2016-04-20T18:22:57Z

Two small comments, ready to squash commits together after those! Thank you again. :)

Fredar · 2016-04-20T20:37:41Z

@tute Updated migration comment, added news detail and squashed. Cheers!

tute · 2016-04-20T20:49:55Z

lib/generators/doorkeeper/previous_refresh_token_generator.rb

+require 'rails/generators/active_record'
+
+module Doorkeeper
+  class PreviousRefreshTokenGenerator < Rails::Generators::Base


Does Rails need a :: prefix? I got:

% rails generate doorkeeper:previous_refresh_token [WARNING] Could not load generator "generators/doorkeeper/previous_refresh_token_generator". Error: uninitialized constant Doorkeeper::Rails::Generators. /Users/tutec/Sites/opensource/doorkeeper-gem/doorkeeper/lib/generators/doorkeeper/previous_refresh_token_generator.rb:4:in `<module:Doorkeeper>'

Something must have changed since the time it was originally created.
Will move module directly into class definition.

tute · 2016-04-20T20:57:07Z

The migration didn't run in my spec/dummy app.

Also, suggestion for the commit message:

Revoke refresh token after access token use

When AccessToken#previous_refresh_token attribute exists, it is populated with the refresh token that issues the access token. This refresh token will be revoked when the access token is authenticated for the first time.

If the attribute doesn't exist in the database, doorkeeper will revoke the refresh token as soon as it issues a new access token, regardless of when is it used.

When AccessToken#previous_refresh_token attribute exists, it is populated with the refresh token that issues the access token. This refresh token will be revoked when the access token is authenticated for the first time. If the attribute doesn't exist in the database, doorkeeper will revoke the refresh token as soon as it issues a new access token, regardless of when is it used.

Fredar · 2016-04-20T21:18:09Z

Indeed, the migration in dummy app was scheduled before the table creation 😉
I have fixed the generator and recreated the migration.

tute · 2016-04-20T21:53:22Z

😃 🎉 ❤️❤️❤️

groe · 2016-04-22T10:15:02Z

Nice work – can't wait to use it in 4.0.0 stable! 👍

kevinelliott · 2018-03-21T18:23:28Z

I am attempting to use refresh_token_revoked_on_use in config/initializers/doorkeeper.rb but it is claiming that the setting does not exist. Thoughts here?

nbulaj · 2018-03-21T19:11:26Z

@kevinelliott all you need is just to add 'previous_refresh_token' column to Access Tokens (see Doorkeeper rake tasks, there must be a generator for migration).

hickford · 2023-05-12T19:34:46Z

@Fredar @tute Is there any documentation for the refresh_token_revoked_on_use option? I couldn't find it in https://doorkeeper.gitbook.io/guides/

Fredar · 2023-05-12T21:18:27Z

@hickford Dont think so, but it should be pretty straightforward to use.

Commit msg explains roughly how it works
https://github.com/doorkeeper-gem/doorkeeper/pull/769/commits

There is also a migration generator. (if you installed doorkeeper before it was part of default migration set)

houndci-bot reviewed Dec 29, 2015
View reviewed changes

tute mentioned this pull request Jan 3, 2016

Revoke Refresh Token on access token use #691

Closed

tute reviewed Jan 3, 2016
View reviewed changes

houndci-bot reviewed Jan 7, 2016
View reviewed changes

Fredar force-pushed the feature/revoke-on-use branch 2 times, most recently from df2f3a8 to 9b64f35 Compare January 10, 2016 15:08

houndci-bot reviewed Jan 10, 2016
View reviewed changes

houndci-bot reviewed Jan 20, 2016
View reviewed changes

Fredar force-pushed the feature/revoke-on-use branch from f2a34bb to eca7000 Compare April 19, 2016 17:34

houndci-bot reviewed Apr 20, 2016
View reviewed changes

Fredar force-pushed the feature/revoke-on-use branch from 87edb5e to 6bab9a5 Compare April 20, 2016 06:45

tute reviewed Apr 20, 2016
View reviewed changes

Fredar force-pushed the feature/revoke-on-use branch 2 times, most recently from ae8faad to 1abd87b Compare April 20, 2016 20:33

tute reviewed Apr 20, 2016
View reviewed changes

Fredar force-pushed the feature/revoke-on-use branch from 1abd87b to cb22b3c Compare April 20, 2016 21:16

tute merged commit 3d88a7b into doorkeeper-gem:master Apr 20, 2016

jimishjoban added a commit to parity/doorkeeper that referenced this pull request Sep 9, 2016

Pull Revoke refresh token on use change doorkeeper-gem#769

01819df

sagar18041 mentioned this pull request Sep 20, 2016

Multiple access token generation from one refresh token. #884

Closed

zenzei mentioned this pull request Aug 2, 2019

Password grant flow for mobile app + refresh tokens issues? #1292

Closed

nbulaj mentioned this pull request Apr 13, 2020

Refresh token apparently not being revoked after being used to create a new access token #1396

Closed

linhdangduy mentioned this pull request Dec 25, 2020

Refresh tokens are not being revoked when new access token is created #1462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revoke Refresh Token on access token use #769

Revoke Refresh Token on access token use #769

Fredar commented Dec 29, 2015

houndci-bot Dec 29, 2015

Fredar commented Dec 29, 2015

tute commented Jan 3, 2016

tute Jan 3, 2016

Fredar Jan 6, 2016

tute commented Jan 3, 2016

houndci-bot Jan 7, 2016

Fredar commented Jan 7, 2016

Fredar commented Jan 10, 2016

houndci-bot Jan 10, 2016

Fredar commented Jan 18, 2016

houndci-bot Jan 20, 2016

Fredar commented Apr 19, 2016 •

edited

Loading

tute commented Apr 20, 2016

houndci-bot Apr 20, 2016

Fredar commented Apr 20, 2016

tute Apr 20, 2016

tute commented Apr 20, 2016

Fredar commented Apr 20, 2016

tute Apr 20, 2016

Fredar Apr 20, 2016

tute commented Apr 20, 2016

Fredar commented Apr 20, 2016

tute commented Apr 20, 2016

groe commented Apr 22, 2016

kevinelliott commented Mar 21, 2018

nbulaj commented Mar 21, 2018

hickford commented May 12, 2023

Fredar commented May 12, 2023

Revoke Refresh Token on access token use #769

Revoke Refresh Token on access token use #769

Conversation

Fredar commented Dec 29, 2015

Choose a reason for hiding this comment

Fredar commented Dec 29, 2015

tute commented Jan 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tute commented Jan 3, 2016

Choose a reason for hiding this comment

Fredar commented Jan 7, 2016

Fredar commented Jan 10, 2016

Choose a reason for hiding this comment

Fredar commented Jan 18, 2016

Choose a reason for hiding this comment

Fredar commented Apr 19, 2016 • edited Loading

tute commented Apr 20, 2016

Choose a reason for hiding this comment

Fredar commented Apr 20, 2016

Choose a reason for hiding this comment

tute commented Apr 20, 2016

Fredar commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tute commented Apr 20, 2016

Fredar commented Apr 20, 2016

tute commented Apr 20, 2016

groe commented Apr 22, 2016

kevinelliott commented Mar 21, 2018

nbulaj commented Mar 21, 2018

hickford commented May 12, 2023

Fredar commented May 12, 2023

Fredar commented Apr 19, 2016 •

edited

Loading