feat: improved oidc flow on duplicate account registration #3151

Benehiko · 2023-03-07T14:32:50Z

This PR improves the OIDC registration flow when a duplicate account error happens.

Currently the flow looks as follows:

User registers with password (or other credentials)
User forgot they registered with password and tries to login through an OIDC provider (e.g. Google)
Kratos attempts a registration since the OIDC credentials do not exist
(optional) User needs to add missing traits (e.g. full name) which could not be retrieved from the OIDC provider
User gets a duplicate account error with a "Continue" button.
After submitting the "Continue" button the flow continues again to the OIDC provider, back to Kratos and redirects to UI with duplicate error (Steps 3 to 5)

Instead of causing a confusing redirect loop we should show the user the error with a fresh login flow (since the account exists). This also gives the user the option to do a recovery flow.

User registers with password (or other credentials)
User forgot they registered with password and tries to login through an OIDC provider (e.g. Google)
Kratos attempts a registration since the OIDC credentials do not exist
(optional) User needs to add missing traits
User is returned to a Login flow with the duplication error

Related issue(s)

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 the approval (please contact
security@ory.sh) 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

selfservice/flow/registration/error.go

selfservice/flow/registration/hook.go

selfservice/strategy/oidc/strategy.go

Benehiko · 2023-03-09T15:43:52Z

test/e2e/cypress/integration/profiles/oidc/registration/success.spec.ts

+          route: registration,
+        })
+
+        cy.get('[data-testid="ui/message/4000007"]').should("be.visible")


checks if the duplicate error message is present on the form

Benehiko · 2023-03-09T15:44:03Z

test/e2e/cypress/integration/profiles/oidc/registration/success.spec.ts

+        cy.get("[name='provider'][value='hydra']").should("be.visible")
+        cy.get("[name='provider'][value='google']").should("be.visible")
+        cy.get("[name='provider'][value='github']").should("be.visible")


ensure we have form values for logging in

Benehiko · 2023-03-09T15:44:20Z

test/e2e/cypress/integration/profiles/oidc/registration/success.spec.ts

+
+        cy.get("input[name='identifier']").type(email)
+        cy.get("input[name='password']").type(password)
+        cy.submitPasswordForm()


ensure we can actually log in after getting such an error.

Benehiko · 2023-03-09T15:44:57Z

test/e2e/profiles/oidc/.kratos.yml

+        password:
+          hooks:
+            - hook: session


In the spa profile we always have a session after registration. Just added it here to the oidc profile used by the express app.

Benehiko · 2023-03-10T11:10:55Z

Added a new error specific to OIDC linking to an existing account. I think this might help confused users

jonas-jonas

Looking good already. Just a few nits.

selfservice/strategy/oidc/strategy_registration.go

selfservice/strategy/oidc/strategy.go

codecov · 2023-03-13T12:38:24Z

Codecov Report

Merging #3151 (c47bbe1) into master (9252f5a) will increase coverage by 0.08%.
The diff coverage is 69.38%.

❗ Current head c47bbe1 differs from pull request most recent head 87d56ae. Consider uploading reports for the commit 87d56ae to get more accurate results

@@            Coverage Diff             @@
##           master    #3151      +/-   ##
==========================================
+ Coverage   77.61%   77.69%   +0.08%     
==========================================
  Files         317      317              
  Lines       20009    20036      +27     
==========================================
+ Hits        15530    15567      +37     
+ Misses       3289     3279      -10     
  Partials     1190     1190

Impacted Files	Coverage Δ
selfservice/strategy/oidc/strategy_registration.go	`65.08% <53.57%> (+0.27%)`	⬆️
selfservice/strategy/oidc/strategy.go	`63.71% <71.42%> (+0.23%)`	⬆️
cmd/clidoc/main.go	`68.05% <100.00%> (+0.14%)`	⬆️
selfservice/flow/login/handler.go	`79.26% <100.00%> (+0.38%)`	⬆️
selfservice/flow/registration/error.go	`70.76% <100.00%> (+0.92%)`	⬆️
selfservice/flow/registration/hook.go	`82.11% <100.00%> (ø)`
text/message_validation.go	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

selfservice/strategy/oidc/strategy_registration.go

jonas-jonas

Thanks for fixing the issues :)

LGTM, apart from the nil return in handleError.

jonas-jonas · 2023-03-13T15:31:45Z

selfservice/strategy/oidc/strategy.go

+				return err
+			}
+			x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())).String())
+			return nil


This seems wrong, right? This should either return the error, or not return at all, IIUC. If we return nil other callers "up" the line might end up with a situation, where err == nil, while there was an error.

Do we have a test covering this code path?

The x.AcceptToRedirectJSON doesn't return anything.

It redirects or writes json back to the browser.

the return nil just prevents the rest of the function from executing. In this case we don't want to propagate the error further since we want to return a login flow with the error message attached to the login flow.

We are always calling this function as part of the return part of functions, which return someValue, error. For example here https://github.com/ory/kratos/pull/3151/files/9b1b977a8bbc43e86687bb631fc46ac4f3d08241.

In that example, if you now return nil from handleError, the returned values of processRegistration will be nil, nil. And that is an issue, because the caller of processRegistration (here for example) will check whether err != nil, which is false (because you returned nil here) and then assume that the *login.Flow is a valid object and processRegistration completed successfully. It will then go on and reference the nil pointer (*login.Flow), which will panic.

I would really like to see a test that executes this code path. Did you already write one?

I have a test for this though.

Please see https://github.com/ory/kratos/pull/3151/files#diff-03d9986942bad676c4567865d479f219a54bb633d7ccc9eed5820703e3d296a3

and an e2e test https://github.com/ory/kratos/pull/3151/files#diff-2695d9d779ed70b5b6a2d9cc5111dcac495b41bfc8353c0e0a50154ff787f7a6

Remember this PR changes the behaviour of the registration flow with oidc when the account already exists.
Instead of just giving back an error code and message, it responds with a new Login flow and the error message inside the login flow.

I think you should return ErrAbortFlow here. This prevents further calls in case something changes at some point.

CaptainStandby

LGTM

aeneasr

Nicely done

aeneasr · 2023-03-14T09:55:10Z

selfservice/strategy/oidc/strategy.go

+				return err
+			}
+			x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())).String())
+			return nil


I think you should return ErrAbortFlow here. This prevents further calls in case something changes at some point.

aeneasr · 2023-03-14T09:57:26Z

Ups, please address the one comment with abort flow

…account

Benehiko force-pushed the feat-oidc-improvement branch 3 times, most recently from d040a53 to b4b9bb4 Compare March 9, 2023 14:38

Benehiko commented Mar 9, 2023

View reviewed changes

Benehiko marked this pull request as ready for review March 10, 2023 11:13

Benehiko force-pushed the feat-oidc-improvement branch from 272b57e to ee59362 Compare March 10, 2023 11:13

Benehiko requested review from aeneasr and zepatrik as code owners March 10, 2023 11:13

jonas-jonas requested changes Mar 13, 2023

View reviewed changes

jonas-jonas reviewed Mar 13, 2023

View reviewed changes

selfservice/strategy/oidc/strategy_registration.go Outdated Show resolved Hide resolved

Benehiko added 12 commits March 13, 2023 14:55

feat: handle oidc account duplicate

fce1ca8

test: oidc should return login flow if account registration is duplicate

78903ae

style: format

236a833

chore: cleanup

e35b147

fix(e2e): oidc registration with duplicate identifier

fe0889d

fix: lint error G601

e4fe27f

feat: add new error message for oidc duplicate account

d2d0deb

fix: document new oidc duplicate account error message

32b00a0

fix(e2e): oidc settings

e13e538

chore: refactor code

60b3e77

fix: webauthn duplicate credentials

c875244

chore: refactor code

9b1b977

Benehiko force-pushed the feat-oidc-improvement branch from 9d19fa6 to 9b1b977 Compare March 13, 2023 13:56

Benehiko requested a review from jonas-jonas March 13, 2023 15:24

jonas-jonas requested changes Mar 13, 2023

View reviewed changes

CaptainStandby previously approved these changes Mar 14, 2023

View reviewed changes

aeneasr previously approved these changes Mar 14, 2023

View reviewed changes

fix: prevent registration strategy from continuing on oidc duplicate …

87d56ae

…account

Benehiko dismissed stale reviews from aeneasr and CaptainStandby via 87d56ae March 14, 2023 10:02

Benehiko requested review from jonas-jonas, aeneasr and CaptainStandby March 14, 2023 12:32

aeneasr merged commit 4d2fda4 into master Mar 14, 2023

aeneasr deleted the feat-oidc-improvement branch March 14, 2023 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improved oidc flow on duplicate account registration #3151

feat: improved oidc flow on duplicate account registration #3151

Benehiko commented Mar 7, 2023

Benehiko Mar 9, 2023

Benehiko Mar 9, 2023

Benehiko Mar 9, 2023

Benehiko Mar 9, 2023

Benehiko commented Mar 10, 2023

jonas-jonas left a comment

codecov bot commented Mar 13, 2023 •

edited

Loading

jonas-jonas left a comment

jonas-jonas Mar 13, 2023

Benehiko Mar 13, 2023

jonas-jonas Mar 14, 2023

Benehiko Mar 14, 2023

Benehiko Mar 14, 2023

aeneasr Mar 14, 2023

CaptainStandby left a comment

aeneasr left a comment

aeneasr Mar 14, 2023

aeneasr commented Mar 14, 2023

feat: improved oidc flow on duplicate account registration #3151

feat: improved oidc flow on duplicate account registration #3151

Conversation

Benehiko commented Mar 7, 2023

Related issue(s)

Checklist

Further Comments

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

Benehiko commented Mar 10, 2023

jonas-jonas left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2023 • edited Loading

Codecov Report

jonas-jonas 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

CaptainStandby left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Mar 14, 2023

codecov bot commented Mar 13, 2023 •

edited

Loading