Project

General

Profile

Actions

Bug #18281

closed

[login-sync] automatically replaces expired tokens

Added by Ward Vandewege 10 months ago. Updated 10 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
10/21/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Subtasks 1 (0 open1 closed)

Task #18282: Review 18281-login-sync-expired-tokensResolvedWard Vandewege10/21/2021

Actions
Actions #1

Updated by Ward Vandewege 10 months ago

  • Status changed from New to In Progress
Actions #2

Updated by Ward Vandewege 10 months ago

  • Release set to 45
Actions #3

Updated by Ward Vandewege 10 months ago

Ready for review in ccb603fe5a8ca989d6db97cc723ccfcaba2781f5 on branch 18281-login-sync-expired-tokens

Actions #4

Updated by Lucas Di Pentima 10 months ago

  • I'm not sure if I'm following correctly, but with this change, the --rotate-tokens flag seems to me to be superfluous: If the tokenfile exists, it checks the token's validity; if it doesn't exist, it'll create a new one no matter if the --rotate-tokens flag was used or not, right?
  • Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
  • If my appreciation is correct, I think we can safely remove the flag.
Actions #5

Updated by Ward Vandewege 10 months ago

Lucas Di Pentima wrote:

  • I'm not sure if I'm following correctly, but with this change, the --rotate-tokens flag seems to me to be superfluous: If the tokenfile exists, it checks the token's validity; if it doesn't exist, it'll create a new one no matter if the --rotate-tokens flag was used or not, right?

Right - but if you give it --rotate-tokens it will force a token rotation, even if there is an existing token and it is valid. I don't know why you'd want to do that, but I guess it could still be useful when you want to force-rotate all the tokens? The comment is misleading though, I've pushed e1c4967befc7b4dd273b3d9d047a4e4262f5ba2f to fix that.

  • Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
  • If my appreciation is correct, I think we can safely remove the flag.

If it's OK with you I'd keep it with the updated help text?

Actions #6

Updated by Lucas Di Pentima 10 months ago

Ward Vandewege wrote:

Right - but if you give it --rotate-tokens it will force a token rotation, even if there is an existing token and it is valid. I don't know why you'd want to do that, but I guess it could still be useful when you want to force-rotate all the tokens? The comment is misleading though, I've pushed e1c4967befc7b4dd273b3d9d047a4e4262f5ba2f to fix that.

I kind of remember that the train of thought went something like this: On a expiring tokens installation, pass --token-lifetime N and --rotate-tokens while calling the script every N-X where X is a certain amount of minutes of acceptable grace period. But this doesn't work with auto-expiring tokens. (maybe this got lost when we changed the token expiration approach)

  • Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
  • If my appreciation is correct, I think we can safely remove the flag.

If it's OK with you I'd keep it with the updated help text?

I don't think it provides any benefit (other than don't error out on existing installations using the flag) besides what your updates provide, but also it doesn't do any harm to be left there, so it LGTM. :)

Actions #7

Updated by Ward Vandewege 10 months ago

Lucas Di Pentima wrote:

I don't think it provides any benefit (other than don't error out on existing installations using the flag) besides what your updates provide, but also it doesn't do any harm to be left there, so it LGTM. :)

Thanks, merged!

Actions #8

Updated by Ward Vandewege 10 months ago

  • Status changed from In Progress to Resolved
Actions #9

Updated by Ward Vandewege 10 months ago

  • Release changed from 45 to 42
Actions

Also available in: Atom PDF