-
Notifications
You must be signed in to change notification settings - Fork 3
fix: set token ttl correctly in redis #766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
You can access the deployment of this PR at https://renku-ci-gw-766.dev.renku.ch |
5a4c825 to
6a1a31b
Compare
6a1a31b to
368ee15
Compare
368ee15 to
40dea94
Compare
40dea94 to
c4ef65e
Compare
c4ef65e to
0bc86fb
Compare
0bc86fb to
a065913
Compare
a065913 to
b59c6e1
Compare
b59c6e1 to
dfb2ed1
Compare
dfb2ed1 to
60bf5cb
Compare
60bf5cb to
5a095f3
Compare
5a095f3 to
7f16823
Compare
7f16823 to
eb9d769
Compare
eb9d769 to
6eced31
Compare
| func (*SessionStore) getTokenStorageExpiration(tokens models.AuthTokenSet, session models.Session) time.Time { | ||
| providerID := tokens.AccessToken.ProviderID | ||
| if providerID == "renku" || providerID == "gitlab" { | ||
| return time.Time{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will break renku v1 if I understand it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forever tokens are needed for some v1 functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is purely for redis expiration/ttl, not the live time of the token itself or anything like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By doing this change, a TTL will be set on Keycloak and GitLab tokens, which some parts of renku v1 expect to be available forever (even if the user does no log in after session expiry). Feel free to run this as an experiment, but this is the reason redis is remembering tokens forever; I did not want to break renku v1 with the gateway refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I am just adding this here after our discussion. The main issue is that SSH sessions would be affected if we started to evict tokens. Because for these users do not need to be logged in once they have started.
|
Lets postpone this until we fully retire v1 sessions and all the v1 services. |
closes #765
/deploy #notest