Skip to content
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

feat: link oidc credentials when login #3563

Merged
merged 36 commits into from
Nov 8, 2023

Conversation

hperl
Copy link
Contributor

@hperl hperl commented Oct 9, 2023

Original PR: #3222

When user tries to login with OIDC for the first time but has already registered before with email/password a credentials identifier conflict may be detected by Kratos. In this case user needs to login with email/password first and then link OIDC credentials on a settings screen.
This PR simplifies UX and allows user to link OIDC credentials to existing account right in the login flow, without
switching to settings flow.

Related issue(s)

#2727

Checklist

  • [ x] I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • [ x] I am following the
    contributing code guidelines.
  • [ x] I have read the security policy.
  • [ x] 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 the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • [ x] I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@hperl hperl requested review from aeneasr and zepatrik as code owners October 9, 2023 10:45
@hperl hperl self-assigned this Oct 9, 2023
@hperl hperl mentioned this pull request Oct 9, 2023
2 tasks
@hperl hperl marked this pull request as draft October 9, 2023 12:38
Signed-off-by: Henning Perl <henning.perl@ory.sh>
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #3563 (38c0f57) into master (3b75f37) will decrease coverage by 0.02%.
The diff coverage is 75.37%.

❗ Current head 38c0f57 differs from pull request most recent head 51ed466. Consider uploading reports for the commit 51ed466 to get more accurate results

@@            Coverage Diff             @@
##           master    #3563      +/-   ##
==========================================
- Coverage   78.17%   78.15%   -0.02%     
==========================================
  Files         342      342              
  Lines       23295    23295              
==========================================
- Hits        18210    18206       -4     
- Misses       3709     3712       +3     
- Partials     1376     1377       +1     
Files Coverage Δ
cmd/clidoc/main.go 70.08% <100.00%> (ø)
schema/errors.go 91.16% <100.00%> (ø)
selfservice/flow/continue_with.go 88.88% <ø> (ø)
selfservice/flow/flow.go 100.00% <ø> (ø)
selfservice/flow/login/flow.go 95.60% <100.00%> (ø)
selfservice/flow/login/handler.go 78.80% <100.00%> (ø)
selfservice/flow/login/strategy.go 80.00% <ø> (ø)
selfservice/flow/registration/flow.go 97.70% <100.00%> (ø)
selfservice/flow/registration/handler.go 71.70% <ø> (ø)
selfservice/strategy/oidc/strategy_registration.go 69.43% <100.00%> (ø)
... and 11 more

... and 2 files with indirect coverage changes

@hperl hperl removed their assignment Oct 13, 2023
@hperl hperl self-assigned this Oct 20, 2023
@hperl hperl force-pushed the hperl/link-credentials-when-login branch from 1ca8889 to c540231 Compare October 24, 2023 06:12
@hperl hperl force-pushed the hperl/link-credentials-when-login branch 2 times, most recently from ae5c9bb to b0f32e9 Compare October 24, 2023 10:20
@hperl hperl force-pushed the hperl/link-credentials-when-login branch from b0f32e9 to cd14390 Compare October 24, 2023 10:35
@hperl hperl force-pushed the hperl/link-credentials-when-login branch from 7775f54 to 5bff9ef Compare October 30, 2023 09:46
@hperl hperl requested a review from aeneasr October 30, 2023 13:43
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Very nice. I think code-wise this is good to go. Would be nice to see two or three more tests. I also really would like to have a cypress test which would make reviewing code changes (and seeing how the feature works) related to this much easier :/

identity/manager.go Show resolved Hide resolved
schema/errors.go Outdated Show resolved Hide resolved
selfservice/flow/login/flow.go Outdated Show resolved Hide resolved
selfservice/flow/login/hook.go Show resolved Hide resolved
selfservice/flow/login/hook.go Show resolved Hide resolved
selfservice/flow/registration/flow.go Outdated Show resolved Hide resolved
aeneasr and others added 6 commits October 31, 2023 10:43
Signed-off-by: Henning Perl <henning.perl@ory.sh>
Signed-off-by: Henning Perl <henning.perl@ory.sh>
Signed-off-by: Henning Perl <henning.perl@ory.sh>
Signed-off-by: Henning Perl <henning.perl@ory.sh>
Signed-off-by: Henning Perl <henning.perl@ory.sh>
aeneasr
aeneasr previously approved these changes Nov 6, 2023
@hperl hperl requested a review from jonas-jonas November 7, 2023 15:35
@hperl hperl enabled auto-merge (squash) November 7, 2023 15:38
@aeneasr aeneasr disabled auto-merge November 8, 2023 09:11
@aeneasr aeneasr merged commit b784949 into master Nov 8, 2023
26 checks passed
@aeneasr aeneasr deleted the hperl/link-credentials-when-login branch November 8, 2023 09:12
moose115 pushed a commit to moose115/kratos that referenced this pull request Dec 7, 2023
When user tries to login with OIDC for the first time but has already registered before with email/password a credentials identifier conflict may be detected by Kratos. In this case user needs to login with email/password first and then link OIDC credentials on a settings screen.
This PR simplifies UX and allows user to link OIDC credentials to existing account right in the login flow, without
switching to settings flow.

Closes ory#2727
Closes ory#3222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants