feat: update token on login #2428

david972 · 2022-04-27T17:12:19Z

Currently Kratos record the token during the registration and the setting.

If the access_token recorded has expired in the meanwhile or tokens has been revoked it would be useful to update them.
I know in case of expiration we can use the refresh token but not in case of revocation.

Related issue(s)

#1912

Checklist

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
[email protected]) 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

codecov · 2022-04-27T18:48:33Z

Codecov Report

Attention: Patch coverage is 71.42857% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 78.03%. Comparing base (696cc1b) to head (b10fdf5).
Report is 7 commits behind head on master.

❗ Current head b10fdf5 differs from pull request most recent head 5cfe3a1. Consider uploading reports for the commit 5cfe3a1 to get more accurate results

Files	Patch %	Lines
selfservice/strategy/oidc/strategy_login.go	53.84%	10 Missing and 2 partials ⚠️
identity/manager.go	75.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2428      +/-   ##
==========================================
- Coverage   78.05%   78.03%   -0.03%     
==========================================
  Files         360      360              
  Lines       25241    25281      +40     
==========================================
+ Hits        19703    19728      +25     
- Misses       4032     4043      +11     
- Partials     1506     1510       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aeneasr

Looks like the commit history somehow got messed up?

Makefile

docs/docs/self-service/flows/code/registration/index.js

examples/go/selfservice/login/main.go

quickstar · 2022-09-06T06:26:00Z

Really looking forward to this. @david972 this should fix this issue: #2696 right?

AUSBird · 2022-09-06T20:16:22Z

Agreed, I’m also really keen for this PR
Hope it gets fixed up however it needs and merged soon~

quickstar · 2022-11-08T07:00:51Z

Hi @aeneasr
Anything which is blocking this PR to get merged?

aeneasr · 2022-12-06T17:57:45Z

I merged the conflicts :) So that's good now.

What I'm a bit concerned about is that this update might break existing credentials. So this needs, IMO, two test cases:

Write an end-to-end test which executes social sign up, then sign in, and checks the tokens
Write an end-to-end test which signs up using password, then connects a new social sign in provider, then signs in using social sign in. The account should still be able to sign in using password.

I think we should also add at least one e2e test case to cover this. If you could add those, that'd be wonderful!

david972 · 2022-12-07T06:58:06Z

Ok I'll do it

tdipadova3rd · 2023-01-16T23:28:42Z

@aeneasr @david972 Any blockers left? Really hoping to use this feature in an upcoming project, and happy to help out!

david972 · 2023-01-17T07:18:01Z

@aeneasr @david972 Any blockers left? Really hoping to use this feature in an upcoming project, and happy to help out!

One more e2e test to do

quickstar · 2023-02-08T06:44:35Z

This is going to be awesome once merged 💯❗️👍🔥

quickstar · 2023-02-27T14:12:30Z

@david972, @aeneasr can I help in some regard to get this merged?

AUSBird · 2023-03-11T00:14:37Z

Super hyped to see this merged! Any updates on when we will see this completed? I see a formatting pipeline failed

lunar-smh · 2023-05-15T13:49:09Z

@david972, @aeneasr I am really interested in this change. Can I help in any way?

luflow · 2023-11-02T09:10:10Z

We are also in need of this change - can we contribute in any way? @aeneasr @david972

david972 · 2024-02-09T07:13:08Z

@aeneasr
Hi,
If I do a rebase from master can we expect this PR to be merged ?

aeneasr · 2024-02-12T14:36:05Z

Absolutely, and tests would also need to pass :)

david972 · 2024-02-12T14:47:46Z

Absolutely, and tests would also need to pass :)

sure
I'm on it

kriscoleman

Would love to see these changes released to kratos

We're experiencing needs for this and it would be a big win to have this PR merged

david972 · 2024-04-07T18:30:59Z

All it's done

aeneasr

Looking pretty good already!

aeneasr · 2024-04-15T08:22:17Z

selfservice/strategy/oidc/strategy_login.go

+ if err := toUpdate.UnmarshalConfig(toUpdate); err != nil {
+ return err
+ }
+ var toUpdateConfig = oidcCredentials
+ k, found := toUpdateConfig.GetProvider(provider.Config().ID, claims.Subject)
+ if !found {
+ // Credentials are not found, we can ignore this.
+ return nil
+ }
+ toUpdateConfig.Providers[k].CurrentIDToken = token.GetIDToken()
+ toUpdateConfig.Providers[k].CurrentAccessToken = token.GetAccessToken()
+ toUpdateConfig.Providers[k].CurrentRefreshToken = token.GetRefreshToken()
+ toUpdate.Config, err = json.Marshal(toUpdateConfig)
+ return errors.WithStack(err)


I think this code has a few problems:

toUpdate.UnmarshalConfig(toUpdate) unmarshalls on itself?

To update the identity we are fetching it again (within UpdateCredentials) which will mean + ~200ms for every login. We already have the correct credentials from "FindByCredentials" so let's use that credentials instead.

Since this adds an update to every login it can increase the login time on some databases by up to 500ms. It would be great if this was behind a feature flag and maybe even per provider (selfservice.methods.oidc.providers.0.update_tokens_on_login)

Please add ample tests (maybe move this into a separate function) that ensures that the credentials are correctly updated. If there is a bug, it could break login for every user using social sign in (not saying it has a bug, but tests make sure we don't have any!

done

in progress

in progress

in progress

@aeneasr
Can I do the update into a go routine outside the processLogin ?
Like this the login won't be altered

That probably won't work well, because the request context will be canceled by then, which will lead to a rollback in the transaction. In my view it needs to be part of the request chain

aeneasr · 2024-04-15T08:22:22Z

Makefile

@@ -9,7 +9,7 @@ export PATH := .bin:${PATH}
 export PWD := $(shell pwd)
 export BUILD_DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ")
 export VCS_REF := $(shell git rev-parse HEAD)
-export QUICKSTART_OPTIONS ?= ""
+export QUICKSTART_OPTIONS ?=


Suggested change

export QUICKSTART_OPTIONS ?=

export QUICKSTART_OPTIONS ?= ""

this change is because that raising an issue if this variable is empty

Do I have your ok for this ?

Sorry, not sure if I understand :) Does this mean the makefile will fail if QUICKSTART_OPTIONS is unset?

yes it does, at least on mac

OK, then please revert it because the makefile is often called without these options available, for example:

make test

Thanks!

identity/credentials_oidc.go

[skip ci]

…3886)

… update-token-on-login

david972 marked this pull request as draft April 27, 2022 17:13

david972 marked this pull request as ready for review April 27, 2022 19:14

david972 requested review from aeneasr and zepatrik as code owners April 27, 2022 19:14

david972 force-pushed the update-token-on-login branch from 97e0b8e to ad2ca39 Compare April 27, 2022 19:18

aeneasr reviewed Apr 28, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

docs/docs/self-service/flows/code/registration/index.js Outdated Show resolved Hide resolved

examples/go/selfservice/login/main.go Outdated Show resolved Hide resolved

david972 force-pushed the update-token-on-login branch from ad2ca39 to 82c4bf3 Compare April 28, 2022 21:10

david972 requested a review from aeneasr April 28, 2022 21:12

aeneasr force-pushed the update-token-on-login branch from 82c4bf3 to fb6d6ca Compare May 27, 2022 21:56

david972 mentioned this pull request Jul 4, 2022

feat : update oidc tokens on login flow #1912

Closed

6 tasks

pr1ze mentioned this pull request Sep 28, 2022

feat: parse all id token claims into raw_claims, so they are availabl… #2765

Merged

7 tasks

TPausL mentioned this pull request Jan 20, 2023

get google access token TPausL/power-music-backend#2

Open

kriscoleman approved these changes Feb 14, 2024

View reviewed changes

luflow mentioned this pull request Mar 11, 2024

feat: update identity data based on id token claims on each login #3788

Open

2 tasks

david972 force-pushed the update-token-on-login branch from a18f91d to 82c4bf3 Compare April 1, 2024 13:06

david972 requested a review from hperl as a code owner April 1, 2024 13:06

david972 force-pushed the update-token-on-login branch 3 times, most recently from c8cb8c7 to 4c14a45 Compare April 7, 2024 18:27

aeneasr reviewed Apr 15, 2024

View reviewed changes

aeneasr mentioned this pull request Apr 19, 2024

feat: update tokens on login #1991

Closed

7 tasks

feat: update token on login

b137bf3

david972 force-pushed the update-token-on-login branch from 55c91e1 to b137bf3 Compare April 19, 2024 20:14

hperl and others added 10 commits April 22, 2024 14:27

fix: include all creds in duplicate credential err (ory#3881)

e06c241

autogen(openapi): regenerate swagger spec and internal client

473e17c

[skip ci]

autogen(docs): regenerate and update changelog

9c69ef2

[skip ci]

fix: enum type of session expandables (ory#3891)

63d785e

autogen(openapi): regenerate swagger spec and internal client

0b6f91e

[skip ci]

chore: render CLI doc messages into their own *.md file in docs (ory#…

17f9a4f

…3886)

improve update credential

062a011

feat: update token on login

81d400e

improve update credential

c7b5688

Merge branch 'update-token-on-login' of github.com:w6d-io/kratos into…

5cfe3a1

… update-token-on-login

gpoulin mentioned this pull request Apr 26, 2024

Managed third-party access_token zitadel/zitadel#7851

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update token on login #2428

feat: update token on login #2428

david972 commented Apr 27, 2022

codecov bot commented Apr 27, 2022 •

edited

Loading

aeneasr left a comment

quickstar commented Sep 6, 2022

AUSBird commented Sep 6, 2022

quickstar commented Nov 8, 2022

aeneasr commented Dec 6, 2022

david972 commented Dec 7, 2022

tdipadova3rd commented Jan 16, 2023

david972 commented Jan 17, 2023

quickstar commented Feb 8, 2023

quickstar commented Feb 27, 2023

AUSBird commented Mar 11, 2023

lunar-smh commented May 15, 2023

luflow commented Nov 2, 2023

david972 commented Feb 9, 2024

aeneasr commented Feb 12, 2024

david972 commented Feb 12, 2024

kriscoleman left a comment •

edited

Loading

david972 commented Apr 7, 2024

aeneasr left a comment

aeneasr Apr 15, 2024

david972 Apr 16, 2024

david972 Apr 17, 2024

aeneasr Apr 19, 2024

aeneasr Apr 15, 2024

david972 Apr 16, 2024

david972 Apr 17, 2024

aeneasr Apr 19, 2024

david972 Apr 19, 2024

aeneasr Apr 19, 2024

feat: update token on login #2428

Are you sure you want to change the base?

feat: update token on login #2428

Conversation

david972 commented Apr 27, 2022

Related issue(s)

Checklist

Further Comments

codecov bot commented Apr 27, 2022 • edited Loading

Codecov Report

aeneasr left a comment

Choose a reason for hiding this comment

quickstar commented Sep 6, 2022

AUSBird commented Sep 6, 2022

quickstar commented Nov 8, 2022

aeneasr commented Dec 6, 2022

david972 commented Dec 7, 2022

tdipadova3rd commented Jan 16, 2023

david972 commented Jan 17, 2023

quickstar commented Feb 8, 2023

quickstar commented Feb 27, 2023

AUSBird commented Mar 11, 2023

lunar-smh commented May 15, 2023

luflow commented Nov 2, 2023

david972 commented Feb 9, 2024

aeneasr commented Feb 12, 2024

david972 commented Feb 12, 2024

kriscoleman left a comment • edited Loading

Choose a reason for hiding this comment

david972 commented Apr 7, 2024

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2022 •

edited

Loading

kriscoleman left a comment •

edited

Loading