Allow revoked_at to be set in the future, for future expiry

[radar]

Hello,

I'm working with IFTTT integration on a project and they've requested that a refresh token is not revoked immediately. I have no idea how to implement this in doorkeeper without setting revoked_at to a future time. I noticed that the revoked? method merely checks if that field is set, and not if that time has yet passed. This PR attempts to rectify that behaviour.

If you know of a better way to keep refresh tokens around, I would love to hear about it.

Thanks!

[tute]

It is adviced in the spec that refresh tokens get invalidated: http://tools.ietf.org/html/rfc6749#section-10.4.

However, I am open to discuss if it should be an option, to allow developers to extend longer-lived refresh tokens. Open question for me now, will research.

Regarding revoked_at datetime, it acts indeed as a boolean, with no logic on its value (it's presence imply it was revoked, and it has some extra information). Your use of the attribute would be better named revoke_at.

Do you know why IFTT requests that refresh tokens are not revoked immediately?

[tute]

Note: specific bit of code that revokes a refresh token upon use: https://github.com/doorkeeper-gem/doorkeeper/blob/master/lib/doorkeeper/oauth/refresh_token_request.rb#L31-L32

[radar]

Thank you @tute.

IFTTT says:

We have an issue with rollover refresh tokens as the transaction is non-atomic. There is a chance that we’ll make a request to you to obtain a new refresh token yet fail to persist that new token in our datastore.

This means that they may attempt to use an old refresh token, which means that their integration would not work. We would like a way for refresh tokens to not expire immediately, but at some future date ourselves that we set. This way, IFTTT would be able to use any recent token within the system.

[tute]

Your solution seems a good workaround (I'd add a configuration option to set when should they get revoked –10.minutes.from_now, etc). It only uses older naming, probably I'd set an alias or something so it reads better. Will continue reading to see if it's a good option to have in master.

Thanks for your input!

[tute]

@radar I like your change, can you please fix the tests (there's a type error), and rebase so I merge? Thanks for your work!

[radar]

Thanks @tute. I've force-pushed on my branch now and the commit for this PR has all the tests passing locally for me.

[tute]

Thanks! 👍

[francesle]

@radar @tute Is there a pull request for actually allowing developers to set revoked_at to a future time? I'm also using this gem to integrate with IFTTT and was wondering whether there's a way to achieve that with the current version...

[tute]

@francesle see the options:

• access_token_expires_in
• custom_access_token_expires_in
• authorization_code_expires_in

[francesle]

I'm actually referring to the refresh token. This pull request seems to be the most relevant: #578

[tute]

Right, that's WIP right now.

[voltechs]

For anyone else still struggling with this, the trick for us (we're integrating with IFTTT as well) is to ensure you are using the previous_refresh_token. Make sure you have this column in your oauth_access_tokens table.

See (line 35)

doorkeeper/lib/doorkeeper/oauth/refresh_token_request.rb

Lines 30 to 39 in bf36149

	def before_successful_response
	refresh_token.transaction do
	refresh_token.lock!
	raise Errors::InvalidTokenReuse if refresh_token.revoked?
	refresh_token.revoke unless refresh_token_revoked_on_use?
	create_access_token
	end
	super
	end

and (line 44)

doorkeeper/lib/doorkeeper/orm/active_record/access_token.rb

Lines 43 to 45 in 07d1555

	def self.refresh_token_revoked_on_use?
	column_names.include?('previous_refresh_token')
	end