-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: update token on login (rebase) #4447
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
76cb1b8 to
350d3f1
Compare
|
@aeneasr tests that have been passing on main are passing and I've given it my best shot to implement your suggestions given in #2428 (comment) A couple of them the original contributor already got to, I've taken it upon myself to introduce a per-provider feature flag. There's an E2E test that checks if this functionality is working but no unit test. In the case we need one, I'd greatly appreciate some input what tests exactly should be added. I have broken the token update logic into its own function to facilitate unit tests more easily though. |
|
@FaeyUmbrea We would still love to support this feature from our side - are there any points left open? @aeneasr Is there anything left open apart from resolving the conflicts with master? |
|
@luflow I've been using this in my production environment for a couple of months without a hitch, so from my POV there isn't anything that needs to be added. There is only one conflict right now, which should be easy to fix as it's just in the config schema. But in the interest of time I'll do that when the repo is actually about to be merged. |
|
@FaeyUmbrea awaeome! So you just built a docker image of Kratos from your branch basically and use this as your daily driver? Did you use the docket build instructions of Kratos or is anything else needed? I think we will do the same thing then because the feature is so crucial for us - without it there are many complaints of users why their meta data is outdated. |
|
Yeah pretty much @luflow |
|
@aeneasr Is there any hope that this feature will be merged into kratos one day? |
This PR rebases #2428 onto the current master as the original maintainer has gone missing.
The aim is the same as in the original PR. Ory should save tokens its getting on each login. This is helpful in scenarios where the user is encountering a consent screen and the refresh token issues to the backend was invalidated, i.e. through a scope change.
Further, this simplifies implementing applications that might not need to do any background processing and require a login to use. For those usecases the most recent token issued on login is sufficient.
Related issue(s)
#1912
#2428
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments