feat: login & registration via code #3378

Benehiko · 2023-07-12T15:57:09Z

Implements Login and Registration via the code strategy.

Related issue(s)

This feature adds passwordless email code login. When a user signs up, or signs in, a code is sent to their email address which they can use to complete the authentication process.

This feature is currently only working for browser facing APIs.

Closes #2029
Closes https://github.com/ory-corp/cloud/issues/3573

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/strategy/link/strategy_verification.go

selfservice/strategy/code/strategy_login.go

selfservice/strategy/code/strategy.go

selfservice/strategy/code/strategy_login.go

.schema/openapi/patches/selfservice.yaml

driver/config/config.go

driver/registry_default.go

driver/registry_default_login.go

selfservice/strategy/code/strategy.go

selfservice/strategy/code/strategy_verification.go

aeneasr

Very nice! I still need to review the login tests, the registration code, and the registration tests, as well as e2e and trying out the flow myself. In summary, this looks great! There are a few things we need to address:

Should we enable / disable login / regstration for the code method? If yes, then why not also for verification / recovery?
The introduced state parameters are not "backwards" looking - they only apply to the new flows which I find a bit strange?
The SQL migrations should be kept in as few files as possible and avoid DEFAULTs on existing large tables
The state machine we use in login/registration sets values in the callbacks of outer functions. If a callback is not called, these values might be nil, leading to panics

driver/registry_default.go

driver/config/config.go

driver/registry_default_login.go

driver/registry_default_registration.go

aeneasr · 2023-08-04T08:19:21Z

embedx/identity_extension.schema.json

@@ -38,6 +38,19 @@
                      "type": "boolean"
                    }
                  }
+                },
+                "code": {


I'm wondering if we should use a different credential name here. oob_otp (out of band one time password) is probably the "correct" terminology, but noone understands what this means.

What are your thoughts?

otp?. code is also okay since the method you need to enable is code.

selfservice/strategy/code/strategy_registration.go

selfservice/strategy/code/strategy_login.go

selfservice/strategy/code/strategy.go

selfservice/strategy/code/strategy_login.go

aeneasr

Nice ! I looked at everything now - see comments

aeneasr · 2023-08-04T11:22:25Z

selfservice/strategy/code/strategy_login_test.go

+		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+		req.Header.Set("Accept", "application/json")


Take a look at the password tests - we are testing various flows there (browser html, browser ajax, native api) and I think we need to do this here and in registration too. There are also some testhelpers you can use to easily submit these flows!

selfservice/strategy/code/strategy_registration.go

aeneasr · 2023-08-04T11:24:39Z

selfservice/strategy/code/strategy_registration.go

+
+		// Step 2: Check if the flow traits match the identity traits
+		for _, n := range container.NewFromJSON("", node.DefaultGroup, p.Traits, "traits").Nodes {
+			if !strings.EqualFold(f.GetUI().GetNodes().Find(n.ID()).Attributes.GetValue().(string), n.Attributes.GetValue().(string)) {


Are you sure these are all strings allways?

I actually don't know tbh. I also don't know what the best way would be to check if the traits are the same between the submitted UI nodes and the DB UI nodes :/

Maybe @zepatrik knows more about UI node checking ;)

aeneasr · 2023-08-04T11:25:17Z

test/e2e/cypress/integration/profiles/code/login/error.spec.ts

+      app: "express" as "express",
+      profile: "code",
+    },
+    // {


We should probably enable this :)

aeneasr · 2023-08-04T11:25:51Z

test/e2e/cypress/integration/profiles/code/registration/error.spec.ts

+      app: "express" as "express",
+      profile: "code",
+    },
+    // {


aeneasr · 2023-08-04T11:26:07Z

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

+      app: "express" as "express",
+      profile: "code",
+    },
+    // {


aeneasr · 2023-08-04T11:26:38Z

test/e2e/cypress/support/commands.ts

+        }
+        console.log({ mailItems: response.body.mailItems })
+        console.log({ mailItem })
+        console.log({ email })


Remove logs?

aeneasr · 2023-08-04T11:26:54Z

test/e2e/run.sh

@@ -136,7 +136,7 @@ prepare() {
  nc -zv localhost 4445 && exit 1
  nc -zv localhost 4446 && exit 1
  nc -zv localhost 4455 && exit 1
-  nc -zv localhost 4456 && exit 1
+  # nc -zv localhost 4456 && exit 1


aeneasr · 2023-08-04T11:27:01Z

test/e2e/run.sh

-        >"${base}/test/e2e/ui-node.e2e.log" 2>&1 &
-    )
-  fi
+  # if [ -z ${NODE_UI_PATH+x} ]; then


codecov · 2023-08-16T14:50:58Z

Codecov Report

Merging #3378 (267b07d) into master (37f1657) will increase coverage by 0.10%.
Report is 1 commits behind head on master.
The diff coverage is 81.29%.

❗ Current head 267b07d differs from pull request most recent head 99922c1. Consider uploading reports for the commit 99922c1 to get more accurate results

@@            Coverage Diff             @@
##           master    #3378      +/-   ##
==========================================
+ Coverage   78.14%   78.24%   +0.10%     
==========================================
  Files         327      339      +12     
  Lines       21742    22495     +753     
==========================================
+ Hits        16991    17602     +611     
- Misses       3492     3582      +90     
- Partials     1259     1311      +52

Files Changed	Coverage Δ
persistence/sql/persister_login.go	`100.00% <ø> (ø)`
persistence/sql/persister_recovery.go	`85.29% <ø> (+0.29%)`	⬆️
persistence/sql/persister_verification.go	`83.82% <ø> (+0.69%)`	⬆️
schema/extension.go	`73.33% <ø> (ø)`
selfservice/flow/flow.go	`100.00% <ø> (ø)`
selfservice/flow/login/hook.go	`87.74% <ø> (ø)`
selfservice/flow/recovery/strategy.go	`100.00% <ø> (ø)`
selfservice/flow/verification/error.go	`81.66% <ø> (ø)`
selfservice/flow/verification/strategy.go	`75.00% <ø> (ø)`
selfservice/strategy/lookup/settings.go	`62.50% <0.00%> (ø)`
... and 71 more

... and 2 files with indirect coverage changes

Benehiko · 2023-08-22T12:13:17Z

Some things to consider before merging:

The SPA experience isn't that great with this PR since the SDK for one returns the successfulNativeLogin object on login submit.
https://github.com/ory/kratos/blob/master/selfservice/flow/login/handler.go#L687

See this example:
https://github.com/ory/kratos-selfservice-ui-react-nextjs/pull/59/files#diff-efe206089575e5aa92a4bacded0c0c25b4ef6b11199e0d8620eecd1d65b66f50R79-R81

One alternative would be to return the continue_with property like we do in the registration flow when a verification flow is needed.

if (data.continue_with) {
...
}

The naming and configuration. Right now all of the UI nodes are under the code group which I think is fine since this is the same group we use in Recovery and Verification when they use the code method.
The configuration keys need a bit of adjustment. As mentioned by @aeneasr we could use something like this:

code:
  enabled: true # enables verification, recovery
  passwordless_enabled: true # additionally enables passwordless flows
  # aal2_enabled: true

Which I'm in favor of.

A lot of the scaffolding for the settings flow is already in this PR and can be done in a follow-up PR. This reduces the size of the PR and makes it easier to review.
We need to merge feat: registration & login with code elements#117 and feat: login and registration via code kratos-selfservice-ui-react-nextjs#59
To get the tests passing on kratos-selfserivce-ui-react-nextjs and kratos-selfservice-ui-node we would need to do an SDK pre-release off of this branch.

jonas-jonas · 2023-08-23T11:26:51Z

Where do you need to "redirect"/continue to?
That sounds good to me. In recovery/verification we have the use key that indicates whether to use link/code, maybe something like that is an option here, too?
SGTM

To get the tests passing on kratos-selfserivce-ui-react-nextjs and kratos-selfservice-ui-node we would need to do an SDK pre-release off of this branch.

Make sure to actually do a pre-release, if you just use a random id it is uploaded as the latest package. (I had that happen a while back.)

hperl

This looks very good! I just had some minor questions and one security concern regarding the code that checks the code.

hperl · 2023-08-25T07:41:30Z

courier/template/template.go

@@ -19,6 +19,8 @@ type (
 		CourierTemplatesVerificationValid() *config.CourierEmailTemplate
 		CourierTemplatesRecoveryInvalid() *config.CourierEmailTemplate
 		CourierTemplatesRecoveryValid() *config.CourierEmailTemplate
+		CourierTemplatesLoginValid() *config.CourierEmailTemplate


Where is this interface being used or implemented?

hperl · 2023-08-25T08:54:06Z

driver/config/config.go

@@ -739,6 +748,7 @@ func (p *Config) SelfServiceStrategy(ctx context.Context, strategy string) *Self
 	// we need to forcibly set these values here:
 	if !pp.Exists(enabledKey) {


missing fallthrough?

hperl · 2023-08-25T09:40:00Z

persistence/sql/persister_login.go

+	secrets:
+		for _, secret := range p.r.Config().SecretsSession(ctx) {
+			suppliedCode := []byte(p.hmacValueWithSecret(ctx, codeVal, secret))
+			for i := range loginCodes {


This for loop looks a bit concerning. If an attacker can create multiple login codes in quick succession, the probability that one of the codes matches a random guess rises.

From a security perspective we should only ever check against one login code, IMHO the latest.

hmm, i added this since you could have multiple identifiers listed under the code credentials type.

The user submits one of those identifiers, an email is sent out to all of their identifiers linked to the code credential. Each email contains a unique code. The second submit the user can submit any of the codes sent out.

See this identity schema https://github.com/ory/kratos/blob/f1a0fae19e32014a9151f320ed2ad6a903f129ea/test/e2e/profiles/code/identity.complex.traits.schema.json

I think you're both right. In the default case, where an identity has only one or two "login emails", I think that Alano is correct. As far as I can tell, multiple codes (related to one flow id) would only ever be sent out if the identity schema has more than one code credentials identifier.

However, if an identity schema allows an array of identifiers (so 1..n emails used for login), this could theoretically be abused.

I guess this can be fixed by sending EVERY address the same code. At the moment we send every address a different code, which makes brute forcing more likely because we reduce the search space.

Does that make sense?

hmm true, i didn't think of that 🤔

Yeah i agree, sending each the same code will prevent this.

I had to revert this logic, every address needs a unique code, otherwise we don't know which address got the code - and thus can't verify the correct email.

hperl · 2023-08-25T09:40:46Z

persistence/sql/persister_registration.go

+		}
+
+	secrets:
+		for _, secret := range p.r.Config().SecretsSession(ctx) {


Same concern about checking multiple codes applies here.

hperl · 2023-08-25T09:43:40Z

persistence/sql/persister_registration.go

+	return registrationCode, nil
+}
+
+func (p *Persister) UseRegistrationCode(ctx context.Context, flowID uuid.UUID, rawCode string, addresses ...string) (*code.RegistrationCode, error) {


This function is very similar to UseLoginCode. Can we extract this out into a generic function?

I did this already on review branch

style: format refactor: code cleanup and flow refinement feat: store code credential address type fix: login resend button and field errors fix: invalid code handling and error messages test(e2e): registration with code test: login and registration code fix: login and registration tests style: format test: registration with code error cases test: login with code error messages test: login with code test: login and registration code test: login errors fix: unit tests and verification flow fix: ui rendering on code group instead of default fix: sdk generation fix: sdk generation and tests chore: improve registration with code test chore: code review chore: code review chore: cleanup based on review comments

Benehiko force-pushed the feat-login-code branch 2 times, most recently from 33b1c05 to 62ce450 Compare July 13, 2023 08:31

github-advanced-security bot found potential problems Jul 13, 2023

View reviewed changes

selfservice/strategy/link/strategy_verification.go Fixed Show fixed Hide fixed

Benehiko force-pushed the feat-login-code branch 3 times, most recently from 41cfae9 to ffb2035 Compare July 17, 2023 15:20

github-advanced-security bot found potential problems Jul 24, 2023

View reviewed changes

selfservice/strategy/code/strategy_login.go Fixed Show fixed Hide fixed

Benehiko force-pushed the feat-login-code branch 2 times, most recently from 5c5d4c3 to 0666a33 Compare July 25, 2023 08:09

github-advanced-security bot found potential problems Jul 25, 2023

View reviewed changes

selfservice/strategy/code/strategy.go Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jul 26, 2023

View reviewed changes

selfservice/strategy/code/strategy_login.go Fixed Show fixed Hide fixed

Benehiko force-pushed the feat-login-code branch 3 times, most recently from 42a1173 to bd2bbeb Compare August 3, 2023 15:19

Benehiko commented Aug 4, 2023

View reviewed changes

aeneasr marked this pull request as ready for review August 4, 2023 09:51

aeneasr requested review from aeneasr and zepatrik as code owners August 4, 2023 09:51

aeneasr reviewed Aug 4, 2023

View reviewed changes

Benehiko force-pushed the feat-login-code branch 3 times, most recently from 92c2292 to 951485b Compare August 16, 2023 08:18

Benehiko requested a review from hperl as a code owner August 16, 2023 08:18

Benehiko force-pushed the feat-login-code branch 2 times, most recently from 726b76a to e117fef Compare August 21, 2023 15:13

Benehiko force-pushed the feat-login-code branch from f13a51f to 70b2914 Compare August 22, 2023 12:15

hperl requested changes Aug 25, 2023

View reviewed changes

Benehiko and others added 21 commits August 29, 2023 09:59

feat: login with code

b822073

fix: flow states

6eb0ab0

fix: golangci-lint

343365a

chore: move verification hook to before persist

e61c748

chore: refactor code state manager to use generics

5ebe91a

test: registration with spa and browser

2107a7e

test: registration with code on browser and spa

2f45939

test: login with code on spa and browser clients

361b316

test(e2e): login and registration with code errors

11183d5

style: format

ca77a83

test(e2e): login success with code

26690c3

test(e2e): recovery on identity with code

6e8a964

fix: sdk login and registrion should contain code method

aeee834

test(e2e): spa flows on login and registration code

4bdfc79

chore: rename config keys and return status 400 on first flow submit

2d48898

fix: verification handler test sql error

a0c8e75

chore: restore e2e test script

558bfe5

fix(test): verification fake strategy

35769fc

feat: login and registration email template config keys

4da4887

chore: code review for code strategy (magic code login) (#3456)

42ac455

aeneasr force-pushed the feat-login-code branch from d8a5c2f to 42ac455 Compare August 29, 2023 08:04

aeneasr added 3 commits August 29, 2023 10:26

chore: synchronize workspaces

d4a2583

chore: synchronize workspaces

cdccbef

chore: synchronize workspaces

99922c1

aeneasr merged commit eaaf375 into master Aug 29, 2023
26 checks passed

aeneasr deleted the feat-login-code branch August 29, 2023 13:43

aeneasr mentioned this pull request Sep 1, 2023

Register and login with magic link #2669

Closed

6 tasks

CNLHC mentioned this pull request Jan 28, 2024

feat: Passwordless SMS code authentication #2033

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: login & registration via code #3378

feat: login & registration via code #3378

Benehiko commented Jul 12, 2023 •

edited by aeneasr

Loading

aeneasr left a comment

aeneasr Aug 4, 2023

Benehiko Aug 4, 2023

aeneasr left a comment

aeneasr Aug 4, 2023

aeneasr Aug 4, 2023

Benehiko Aug 4, 2023

aeneasr Aug 4, 2023

aeneasr Aug 4, 2023

aeneasr Aug 4, 2023

aeneasr Aug 4, 2023

aeneasr Aug 4, 2023

aeneasr Aug 4, 2023

codecov bot commented Aug 16, 2023 •

edited

Loading

Benehiko commented Aug 22, 2023

jonas-jonas commented Aug 23, 2023

hperl left a comment

hperl Aug 25, 2023

hperl Aug 25, 2023

hperl Aug 25, 2023

Benehiko Aug 25, 2023

aeneasr Aug 25, 2023

Benehiko Aug 25, 2023

aeneasr Aug 29, 2023

hperl Aug 25, 2023

hperl Aug 25, 2023

aeneasr Aug 25, 2023

		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
		req.Header.Set("Accept", "application/json")

		@@ -739,6 +748,7 @@ func (p Config) SelfServiceStrategy(ctx context.Context, strategy string) Self
		// we need to forcibly set these values here:
		if !pp.Exists(enabledKey) {

feat: login & registration via code #3378

feat: login & registration via code #3378

Conversation

Benehiko commented Jul 12, 2023 • edited by aeneasr Loading

Related issue(s)

Checklist

Further Comments

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

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

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

Benehiko commented Aug 22, 2023

jonas-jonas commented Aug 23, 2023

hperl 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

Benehiko commented Jul 12, 2023 •

edited by aeneasr

Loading

codecov bot commented Aug 16, 2023 •

edited

Loading