Skip to content

refresh the token lifetime in every access with that token#362

Closed
karakayasemi wants to merge 1 commit intomasterfrom
refresh-token
Closed

refresh the token lifetime in every access with that token#362
karakayasemi wants to merge 1 commit intomasterfrom
refresh-token

Conversation

@karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Oct 25, 2020

If the expiry is closer than configured time, refresh the token lifetime in next access with that token

Fixes https://github.com/owncloud/enterprise/issues/4237

@karakayasemi karakayasemi added this to the development milestone Oct 25, 2020
@karakayasemi karakayasemi self-assigned this Oct 25, 2020
Copy link
Contributor

@mrow4a mrow4a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not refresh on each access, check expiration time from row, and renew only when approaches expiry or e.g. after X hours of last renewal.

@codecov
Copy link

codecov bot commented Oct 25, 2020

Codecov Report

Merging #362 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #362      +/-   ##
===========================================
- Coverage      6.01%   5.98%   -0.04%     
- Complexity      307     309       +2     
===========================================
  Files            14      14              
  Lines          1197    1203       +6     
===========================================
  Hits             72      72              
- Misses         1125    1131       +6     
Impacted Files Coverage Δ Complexity Δ
lib/Db/Wopi.php 0.00% <0.00%> (ø) 7.00 <1.00> (+2.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f270ce1...ecfca4d. Read the comment docs.

@karakayasemi
Copy link
Contributor Author

@mrow4a thank you for review. I changed the logic and now it will refresh token only if the expiry is closer than TOKEN_REFRESH_THRESHOLD_SECONDS.

@karakayasemi
Copy link
Contributor Author

@mrow4a the pr is ready to review again, please help by reviewing it one more time. Thanks.

Copy link
Contributor

@mrow4a mrow4a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally on how token refresh usually works, and if we do this renewing I would do it properly.

  • on each access, check if token needs renewal
  • if can be renewed (e.g. token lifetime of x days was not exceeded), do renewal. Kerberos snd OAuth tokens e.g. can be renewed by 1 day up to 7 days. 1 token should not live infinitely for security reasons
  • next, check for expiry of the token

In your current code, you first check expiry, and then attempt renewal. Let me know your thoughts.

@mrow4a
Copy link
Contributor

mrow4a commented Oct 28, 2020

@karakayasemi I had a look again. Main point is not to have a single token to have possibly infinite lifetime (due to refreshes). What would be the point of expiry then, if you could renew the same token infinitelly ? In fact, probably user as part of refresh should receive new token to be used (or be somehow forced to relogin?).

what do you think about this all? just some suggestions/feedback

@karakayasemi
Copy link
Contributor Author

@mrow4a your points are reasonable, I read the wopi documentation about this topic again and I realized that we just need to return access_token_ttl value for wopi clients. In this way, wopi client will handle rest of the things like warning user when token close to expire and refreshing tokens. Currently, we do not return any access_token_ttl and because of that, wopi clients think that token lifetime is infinite.

I am working on a PR for access_token_ttl. I will update here when I finish it. Thanks.

@karakayasemi karakayasemi marked this pull request as draft November 1, 2020 21:15
@karakayasemi
Copy link
Contributor Author

Closing this one. Let's continue with #364 .

@karakayasemi karakayasemi deleted the refresh-token branch November 4, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants