feat: Refresh token expiration window #2827

bill-robbins-ss · 2021-10-29T22:44:26Z

Introduce optional configuration value oauth2.refresh_token_rotation.grace_period that can be set as a duration (1m,
Add a new migration: boolean used SQL column in hydra_oauth2_refresh

When grace_period has positive duration the act of using a refresh token will not invalidate the token and instead mark it as used in the database and set its expiration time to the UTCNOW + the value of the grace period. This will allow a client to continue to use a refresh token for the duration of the grace period.

Related issue(s)

#1831
Fosite 255

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

This change includes a modification to Fosite in PR 634

aeneasr

Looking good :) Could you please also add some docs for this? :)

scripts/create-migration.sh

persistence/sql/persister_oauth2.go

create migrations with command: ory dev pop migration create persistence/sql/migrations/ add_refresh_token_used_flag

aeneasr

Great work, I've got a couple of questions and ideas :)

docs/docs/guides/token-expiration.mdx

persistence/sql/migrations/20211101093405_add_refresh_token_used_flag.up.sql

persistence/sql/models/models.go

persistence/sql/persister_oauth2.go

aeneasr · 2021-11-19T08:13:33Z

persistence/sql/persister_oauth2.go

@@ -280,13 +325,27 @@ func (p *Persister) deactivateSessionByRequestID(ctx context.Context, id string,
 	return sqlcon.HandleError(
 		p.Connection(ctx).
 			RawQuery(
-				fmt.Sprintf("UPDATE %s SET active=false WHERE request_id=?", OAuth2RequestSQL{Table: table}.TableName()),
+				fmt.Sprintf("UPDATE %s SET active=false, used=true WHERE request_id=?", OAuth2RequestSQL{Table: table}.TableName()),


Do we actually need used when we have active already?

I don't think so, findSessionBySignature will return errors when a session is marked inactive.

persistence/sql/persister_oauth2.go

oauth2/fosite_store_helpers.go

aeneasr · 2021-11-19T08:21:08Z

persistence/sql/persister_oauth2.go

+
+	if ! used {
+		session := requester.GetSession()
+		session.SetExpiresAt(fosite.RefreshToken, time.Now().UTC().Add(gracePeriod))


So to understand this a bit better - when refreshing a token we do not deactivate it (active: false) immediately. Instead, if grace is enabled, we extend the expiry time by X from NOW, and we also set used: true. The next time we refresh the token, and we are still in the refresh grace period, nothing is updated.

When we refresh the token again, and NOW + x has passed, fosite will see the key as expired, implying that it is no longer active.

I think this has some serious implications. I am not 100% any more how token reuse detection works, but I think it doesn't trigger on expired tokens, only on inactive ones. Because an expired token has not been reused, it just expired! And since it's expired, you can't use it. A used (active=false) token though has been used, thus, reuse means invalidation of all other tokens.

I think you're saying

this will work but the error will be wrong (expired not reused)

when reused and expired, since it isn't detected as reused other tokens are not revoked

I mentioned in an earlier comment that setting active=false won't work for grace periods, since calls to findSessionBySignature will not succeed. To properly detect that case (active==true, in_grace_period==true, expired) something could be modified to deal with it, I'm worried that changing findSessionBySignature to throw an expired error may have undesirable repercussions. From what I can tell token validation, which includes expiration checking, is done at a different layer and is part of a Strategy. The persistence layer does not have access to token strategies.

That was what I was trying to say :)

The best way to prove/disprove this hypothesis is to introduce a test which covers this. If there's no regression on revokation, this should be fine! It would theoretically also be fine to revoke the token chain on expiration also, not just on reuse.

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

use reflection to control configuring the persister during tests

aeneasr · 2021-12-26T14:28:56Z

Is this good for another review @bill-robbins-ss ? :)

aeneasr · 2022-01-11T17:15:39Z

@bill-robbins-ss I fixed the conflicts and got parts of the CI to pass again, however there seems to be an linting issue. Would it be possible for you to take a look?

https://app.circleci.com/pipelines/github/ory/hydra/3404/workflows/bd1ecd98-8d78-4ea0-835a-5b45ff9a7aae/jobs/34581

ps: For motivation, we have merged several large-scale changes to Ory Hydra so there's a good chance this will land very quickly as well :)

bill-robbins-ss · 2022-01-12T14:57:09Z

@aeneasr this is great, I'll be checking it out this week hopefully getting those rules to pass. Do you know how releasing will be done? We'll want to upgrade our instance to take advantage of the great new things!

aeneasr · 2022-01-12T16:57:31Z

Once this is merged we'll do a new minor release

CLAassistant · 2022-01-20T23:37:52Z

All committers have signed the CLA.

bill-robbins-ss · 2022-01-20T23:56:24Z

hey @aeneasr I fixed the linting errors that came from me, but he other 3 are on code that I didn't touch. Please advise.

See ory/hydra#2827

…ation-window # Conflicts: # CONTRIBUTORS # docs/docs/guides/token-expiration.mdx # docs/docs/reference/configuration.md

aeneasr

Thank you for the work and sorry for the late review time. I did fix some things in the PR but while working on it, I realized that there is still some way to go. In particular:

The new in_grace_period column breaks all tables that do not have this column as they all re-use the same struct to load stuff from the database
The feature is not end-to-end tested
Documentation is missing

Given the sensitivity of this feature we need to ensure that everything works end-to-end. So:

Refreshing a token within the grace period works
Revoking a token revokes also all grace tokens
Rotating a grace token works and does not affect other tokens
Token reuse still works after grace period and revokes all of those tokens also

Let me know if you need any help with this!

bill-robbins-ss · 2022-02-14T23:34:29Z

Hey @aeneasr thank you for checking in on this.

I can introduce a new struct to handle the new table schema, does that seem reasonable?
I would be delighted to add e2e tests, I think they belong here?
It looks like you added documentation with this change. If there is more that needs to be added just let me know where and I'll add it !

Also, I saw that you removed the SetConfig method - I introduced it for the test refresh token enters grace period when configured. Is that same method available somewhere else ?

aeneasr · 2022-02-15T06:24:02Z

Thank you @bill-robbins-ss for the fast response and keeping engaged. I have this PR on my prio list now and will be more active reviewing it. You can also ping me on Slack if I am too slow :)

Regarding your questions:

I can introduce a new struct to handle the new table schema, does that seem reasonable?

It does! I would encourage you though to maybe rebase this PR right away with #2796 to avoid any big conflicts during merge.

I would be delighted to add e2e tests, I think they belong here?

Exactly, I would suggest to cover the following use cases:

Refreshing a refresh token without grace period (I believe we have this already)
Refreshing a refresh token without grace period that is expired (I believe we have this already)
Refreshing a refresh token with grace period
Refreshing a refresh token with grace period that is expired
Refreshing a refresh token 1 (before expiry) 2 (after expiry) 3 (after expiry) with grace period

We also need to cover revokation and deleting the consent:

Revoke a refresh token that was granted before expiry
Revoke consent of refresh token that was granted before expiry
Revoke a refresh token that was granted before expiry with grace period
Revoke consent of refresh token that was granted before expiry with grace period
Revoke a refresh token that was granted before expiry with grace period that was expired
Revoke consent of refresh token that was granted before expiry with grace period that was expired
Revoke a 1 (before expiry) 2 (after expiry) 3 (after expiry) refresh token grace period or consent
Revoke consent of 1 (before expiry) 2 (after expiry) 3 (after expiry) refresh token grace period or consent
For the token revokation and consent deletion we need to check whether all tokens have been revoked. It should not be possible to revoke just one refresh token "chain".

Checking whether we can refresh graceful tokens is important too:

Refresh a 1 (before expiry) 2 (after expiry) 3 (after expiry) with grace period or consent that was granted normally with grace period. We should refresh at least 2 or 3 times to ensure that the "refresh token tree" works correctly.

As well as introspection:

Refreshing a 1 (before expiry) 2 (after expiry) 3 (after expiry) times with grace period and introspecting their access tokens

I believe most functions are there already to test this, so it should not be too much work hopefully.

Besides e2e tests, it probably also makes sense to start with some Go test first, as that will make debugging so much easier for you!

hydra/oauth2/oauth2_auth_code_test.go

Line 277 in 8a966e8

    
           t.Run("case=perform authorize code flow with ID token and refresh tokens", func(t *testing.T) {

It looks like you added documentation with this change. If there is more that needs to be added just let me know where and I'll add it !

Apologies, I got in review mode and am not fully used to the new docs system yet 😓

Also, I saw that you removed the SetConfig method - I introduced it for the test refresh token enters grace period when configured. Is that same method available somewhere else ?

Yes, it's now defined on the interface level so no reflection is needed!

See ory/hydra#2827

…iration-window

aeneasr · 2022-04-11T19:27:48Z

@bill-robbins-ss please let me know when you think that this is good for another review :)

yuekun0707 · 2022-07-26T07:49:17Z

Hi, any update here? Is configuration described here https://www.ory.sh/docs/hydra/guides/token-expiration#refresh-token-rotation ready for using?

aeneasr · 2022-07-26T08:01:14Z

Looks like that made it to prod on mistake, the feature is not yet usable!

See ory/hydra#2827

yuekun0707 · 2022-07-26T08:27:40Z

@aeneasr thank you for your reply! I really need feature like this. We use hydra in our project and it works well. But now we want to connect to google assistant by account linking. There is a problem with the refresh token rotation. The error from google like this:
"Refresh token has been rotated. This is not forbidden, however we do not see much benefit that rotating the refresh token can provide but the potential problem it has. We also tried to refresh token with the old refresh token after it has been rotated. Refresh didn't work, this means partner invalidated the old refresh token right after the rotation. Partners shall only invalidate the old refresh token after seeing we use the new one to ensure we got it successfully."
So, is there any way I can try for this?

misl6 · 2022-11-29T08:22:06Z

Hi @aeneasr and @bill-robbins-ss !
As other users pointed out, this is pretty much a requirement to pass the Google Assistant Actions certification.

Is this feature something scheduled and that we may expect in a near future?
There's anything we can do on our side (as users) to help you to make that happen?

StarAurryon · 2023-06-01T07:50:51Z

Hi @aeneasr, I am willing to continue the work on the coming months as we are currently experiencing network token loss when Windows goes to sleep with our app and also with our android / ios app.

Did @bill-robbins-ss signed the Ory Code Agreement so that I can reuse his code and pursue the implementation ?

Kind regards,

joshuaflanagan · 2024-05-18T15:59:42Z

The issue was closed because the original fork was mistakenly deleted. I created a new fork and pushed the branch, but it would not let me push to the same branch name, so it is now at https://github.com/shipstation/hydra/tree/refresh-token-expiration-window-2. Unfortunately, there does not appear to be a way to change the branch for this PR to reference the new branch. Either way, it looks like the code is still accessible in this PR - maybe an ory/hydra project owner can re-open the PR

bill-robbins-ss added 3 commits October 27, 2021 17:24

add refresh_token grace period

751dbb5

add migration for used on refresh token

219a58e

add refresh token grace period

efa3b8e

aeneasr reviewed Nov 1, 2021

View reviewed changes

scripts/create-migration.sh Outdated Show resolved Hide resolved

aeneasr requested changes Nov 1, 2021

View reviewed changes

persistence/sql/persister_oauth2.go Outdated Show resolved Hide resolved

persistence/sql/persister_oauth2.go Outdated Show resolved Hide resolved

persistence/sql/persister_oauth2.go Show resolved Hide resolved

bill-robbins-ss added 5 commits November 1, 2021 09:38

replace migration with ory dev CLI cmd

7a52b36

create migrations with command: ory dev pop migration create persistence/sql/migrations/ add_refresh_token_used_flag

use existing GetRefreshtTokenSession

45b9aea

make FositeStorer include oauth2.TokenRevocationStorage

7d572aa

WIP mabye grace period tests

9573041

use test run instead of named functions

a203f4b

bill-robbins-ss mentioned this pull request Nov 1, 2021

feat: add new function to TokenRevocationStorage to support refresh token grace-period ory/fosite#635

Merged

6 tasks

bill-robbins-ss added 3 commits November 1, 2021 14:06

add documentation for example config

1ebc99d

add grace period to internal config

3bb1c20

add refresh token grace period to token-expiration doc

8de23dc

bill-robbins-ss changed the title ~~Refresh token expiration window~~ feat: Refresh token expiration window Nov 1, 2021

bill-robbins-ss marked this pull request as ready for review November 1, 2021 20:22

prettier --write

249264f

aeneasr requested changes Nov 19, 2021

View reviewed changes

bill-robbins-ss and others added 10 commits November 22, 2021 09:17

Update persistence/sql/persister_oauth2.go

821fba4

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

update docs: consequences of reusing a used refresh token

2e14277

add parent key for refresh_token_rotation

07375a9

move refresh token rotation to proper parent

09d6801

update documentation

89527fd

remove unneeded file

69ca392

make encryption of session more obvious

8fa6c8c

rename used to in_grace_period

bd8a15e

when deactivating a refresh token, in_grace_period should be false

13cdea8

add testing the refresh token store when grace period is configured

1f642d3

use reflection to control configuring the persister during tests

npx prettier --write {test,cypress}/**/*.js

f244b61

chore: update fosite

e992907

yangm97 mentioned this pull request Jan 20, 2022

Refresh tokens need a grace period to deal with network errors and similar issues #1831

Closed

fix linting errors

d1db135

aeneasr added a commit to ory/docs that referenced this pull request Feb 14, 2022

docs(hydra): add docs for ory/hydra#2827

37f1722

See ory/hydra#2827

aeneasr added 3 commits February 14, 2022 12:13

Merge remote-tracking branch 'origin/master' into refresh-token-expir…

45a8cff

…ation-window # Conflicts: # CONTRIBUTORS # docs/docs/guides/token-expiration.mdx # docs/docs/reference/configuration.md

fix: add max lifetime

2c7b95f

fix: move migration to latest

b1d37ca

aeneasr requested changes Feb 14, 2022

View reviewed changes

aeneasr marked this pull request as draft February 14, 2022 14:31

remove reflection

42de645

aeneasr added a commit to ory/docs that referenced this pull request Feb 23, 2022

docs: do not use emails as the primary identifier (#580)

ecc370a

See ory/hydra#2827

Merge remote-tracking branch 'upstream/master' into refresh-token-exp…

bd2d446

…iration-window

aeneasr added a commit to ory/docs that referenced this pull request Jul 26, 2022

docs(hydra): add refresh token rotation

1075e59

See ory/hydra#2827

shipstation closed this by deleting the head repository May 18, 2024

StarAurryon mentioned this pull request May 25, 2024

feat: Refresh token expiration window #3770

Closed

1 task

hhow09 mentioned this pull request Sep 21, 2024

feat: refresh token notes after surveying ory/fosite hhow09/hhow09.github.io#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refresh token expiration window #2827

feat: Refresh token expiration window #2827

bill-robbins-ss commented Oct 29, 2021 •

edited

Loading

aeneasr left a comment

aeneasr left a comment

aeneasr Nov 19, 2021

bill-robbins-ss Nov 22, 2021

aeneasr Nov 19, 2021

bill-robbins-ss Nov 22, 2021 •

edited

Loading

aeneasr Nov 28, 2021

aeneasr commented Dec 26, 2021

aeneasr commented Jan 11, 2022

bill-robbins-ss commented Jan 12, 2022

aeneasr commented Jan 12, 2022

CLAassistant commented Jan 20, 2022 •

edited

Loading

bill-robbins-ss commented Jan 20, 2022

aeneasr left a comment

bill-robbins-ss commented Feb 14, 2022

aeneasr commented Feb 15, 2022

aeneasr commented Apr 11, 2022

yuekun0707 commented Jul 26, 2022

aeneasr commented Jul 26, 2022

yuekun0707 commented Jul 26, 2022

misl6 commented Nov 29, 2022

StarAurryon commented Jun 1, 2023

joshuaflanagan commented May 18, 2024

feat: Refresh token expiration window #2827

feat: Refresh token expiration window #2827

Conversation

bill-robbins-ss commented Oct 29, 2021 • edited Loading

Related issue(s)

Further Comments

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Nov 19, 2021

Choose a reason for hiding this comment

bill-robbins-ss Nov 22, 2021

Choose a reason for hiding this comment

aeneasr Nov 19, 2021

Choose a reason for hiding this comment

bill-robbins-ss Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

aeneasr Nov 28, 2021

Choose a reason for hiding this comment

aeneasr commented Dec 26, 2021

aeneasr commented Jan 11, 2022

bill-robbins-ss commented Jan 12, 2022

aeneasr commented Jan 12, 2022

CLAassistant commented Jan 20, 2022 • edited Loading

bill-robbins-ss commented Jan 20, 2022

aeneasr left a comment

Choose a reason for hiding this comment

bill-robbins-ss commented Feb 14, 2022

aeneasr commented Feb 15, 2022

aeneasr commented Apr 11, 2022

yuekun0707 commented Jul 26, 2022

aeneasr commented Jul 26, 2022

yuekun0707 commented Jul 26, 2022

misl6 commented Nov 29, 2022

StarAurryon commented Jun 1, 2023

joshuaflanagan commented May 18, 2024

bill-robbins-ss commented Oct 29, 2021 •

edited

Loading

bill-robbins-ss Nov 22, 2021 •

edited

Loading

CLAassistant commented Jan 20, 2022 •

edited

Loading