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

aeneasr · 2023-08-25T13:52:03Z

Unify code validation logic in the persister in one method
Remove state engine and simplifies code structure
Remove SMS fillers (we will add them when they are needed)
Write tests for email address normalization (capitalization)
Add unit tests
Fix failing e2e tests
Fix flaking e2e mail processing
~~Mobile e2e tests~~

aeneasr · 2023-08-25T13:53:47Z

persistence/sql/persister_code.go

+	GetHMACCode() string
+}
+
+func useOneTimeCode[P any, U interface {


This code was the same for login, verification, recovery, and sign up, so I moved it into a common function. Uses some generic magic to make it work nicely. Tried it with duck typing only at first, but it caused issues and would have required reflection.

codecov · 2023-08-28T13:01:50Z

Codecov Report

❗ No coverage uploaded for pull request base (feat-login-code@f1a0fae). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head fb06b82 differs from pull request most recent head 55fa3eb. Consider uploading reports for the commit 55fa3eb to get more accurate results

@@                Coverage Diff                 @@
##             feat-login-code    #3456   +/-   ##
==================================================
  Coverage                   ?   78.14%           
==================================================
  Files                      ?      339           
  Lines                      ?    22370           
  Branches                   ?        0           
==================================================
  Hits                       ?    17481           
  Misses                     ?     3577           
  Partials                   ?     1312

aeneasr

@Benehiko fyi

aeneasr · 2023-08-28T15:12:28Z

identity/extension_credentials_test.go

@@ -72,6 +72,21 @@ func TestSchemaExtensionCredentials(t *testing.T) {
 			},
 			ct: identity.CredentialsTypeWebAuthn,
 		},
+		{


These did not have tests

aeneasr · 2023-08-28T15:14:00Z

selfservice/strategy/code/code_sender.go

@@ -70,9 +70,14 @@ func (s *Sender) SendCode(ctx context.Context, f flow.Flow, id *identity.Identit
 		WithSensitiveField("address", addresses).
 		Debugf("Preparing %s code", f.GetFlowName())

+	// We generate the code once and use it for all addresses. This is important because


Generate one code -> less likely to be brute forced

My bad, we actually need to generate unique codes per address, or it will not be possible to distinguish which address was verified, when multiple addresses are sent a code. So yeah, we must make sure to send an individual code for every distinct address.

aeneasr · 2023-08-28T15:16:52Z

persistence/sql/persister_code.go

+func useOneTimeCode[P any, U interface {
+	*P
+	oneTimeCodeProvider
+}](ctx context.Context, p *Persister, flowID uuid.UUID, userProvidedCode string, flowTableName string, foreignKeyName string, opts ...codeOption) (U, error) {


Here, Generics can not be avoided because we need to work with and modify the concrete underlying data type, and interfaces have certain limitations such as dealing with uninitialized values.

aeneasr · 2023-08-28T15:17:18Z

selfservice/strategy/code/strategy.go


-func (s *Strategy) NextFlowState(f flow.Flow) {


This code already exists in flow.NextState()

aeneasr · 2023-08-28T15:18:34Z

selfservice/strategy/code/strategy_login.go

+	case flow.StateChooseMethod:
+		if err := s.loginSendEmail(ctx, w, r, f, &p); err != nil {
+			return nil, s.HandleLoginError(r, f, &p, err)
+		}
+		return nil, nil
+	case flow.StateEmailSent:
+		i, err := s.loginVerifyCode(ctx, r, f, &p)
 		if err != nil {
-			return err
+			return nil, s.HandleLoginError(r, f, &p, err)
 		}
+		return i, nil
+	case flow.StatePassedChallenge:
+		return nil, s.HandleLoginError(r, f, &p, errors.WithStack(schema.NewNoLoginStrategyResponsible()))
+	}


IMO, this is easier to read and understand what's going on. Is state choose_method? Do this. Is state passed_challenge? Do that.

With the callbacks, it wasn't very clear what is executed when, because CreateCodeHandler does not indicate that the state is choose_method. Plus, we save a lot of code complexity :)

aeneasr · 2023-08-28T15:19:28Z

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

@@ -20,9 +20,9 @@ context("Login error messages with code method", () => {
  ].forEach(({ route, profile, app }) => {
    describe(`for app ${app}`, () => {
      before(() => {
-        cy.proxy(app)
        cy.useConfigProfile(profile)


Load the config profile first, as that takes the longest to propagate.

aeneasr · 2023-08-28T15:19:59Z

test/e2e/cypress/support/commands.ts

-            return req
+            tries++
+            cy.wait(pollInterval)
+            return req()


This caused flakes - because no waiting and req returned as a function, not a function call.

aeneasr · 2023-08-28T16:03:16Z

selfservice/strategy/code/code_sender.go

+		// address was used to verify the code.
+		//
+		// See also [this discussion](https://github.com/ory/kratos/pull/3456#discussion_r1307560988).
+		rawCode := GenerateCode()


aeneasr · 2023-08-28T16:03:31Z

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

        cy.get('[data-testid="ui/message/1040005"]').should(
          "contain",
          "An email containing a code has been sent to the email address you provided",
        )

-        cy.get(' input[name="code"]').type("invalid-code")
+        cy.get('input[name="code"]').type("invalid-code")


Not sure what the spaces are here, but they're not needed.

aeneasr · 2023-08-28T16:04:40Z

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

@@ -75,12 +77,14 @@ context("Registration error messages with code method", () => {

        cy.get('input[name="traits.email"]').type(email)
        cy.submitCodeForm()
-
-        cy.url().should("contain", "registration")
+        cy.get('[data-testid="ui/message/1040005"]').should(


@Benehiko the most important aspect of e2e tests are wait assertions. Before:

Go to /registration

Submit form

Wait for URL to be registration (it already is registration though!)

Flaking test failure because sometimes the wait condition is done before the submit is done

Instead, use proper wait conditions that wait for e.g. specific elements such as this one.

aeneasr · 2023-08-28T16:05:12Z

test/e2e/cypress/support/commands.ts

+          expect(filtered.length).to.equal(expectedCount)
+          mailItem = filtered[0]
        } else {
+          expect(count).to.equal(expectedCount)


Counting did not work correctly, caused flakes.

aeneasr · 2023-08-28T16:05:51Z

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

          cy.get("button[name=method][value=code]").click()
        })

-        cy.deleteMail({ atLeast: 1 })


All methods like cy.getRegistrationCodeFromEmail remove mail automatically. This condition leads to us waiting for about a minute to delete an email, which then times out (and passes silently) because no email was found.

aeneasr commented Aug 25, 2023

View reviewed changes

aeneasr changed the title ~~Login code review~~ chore: code review for code strategy (magic code login) Aug 25, 2023

aeneasr added 4 commits August 28, 2023 12:00

chore: synchronize workspaces

d17bf75

chore: synchronize workspaces

569371f

chore: synchronize workspaces

56dd054

chore: remove code bloat

aa2703e

aeneasr force-pushed the login-code-review branch from a1677c6 to aa2703e Compare August 28, 2023 10:00

chore: synchronize workspaces

ed4b552

aeneasr added 3 commits August 28, 2023 15:23

chore: synchronize workspaces

d2dab02

chore: synchronize workspaces

193d33c

chore: synchronize workspaces

b8cdb26

aeneasr commented Aug 28, 2023

View reviewed changes

chore: synchronize workspaces

bcd626b

aeneasr commented Aug 28, 2023

View reviewed changes

aeneasr added 2 commits August 29, 2023 08:59

chore: synchronize workspaces

0582f6c

chore: synchronize workspaces

55fa3eb

aeneasr marked this pull request as ready for review August 29, 2023 07:06

aeneasr requested review from zepatrik and hperl as code owners August 29, 2023 07:06

aeneasr merged commit d8a5c2f into feat-login-code Aug 29, 2023
17 checks passed

aeneasr deleted the login-code-review branch August 29, 2023 07:58

aeneasr added a commit that referenced this pull request Aug 29, 2023

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

42ac455

github-actions bot mentioned this pull request Aug 30, 2023

upstream reference closed: github.com/ory/kratos/pull/3456 #3465

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aeneasr commented Aug 25, 2023 •

edited

Loading

aeneasr Aug 25, 2023

codecov bot commented Aug 28, 2023 •

edited

Loading

aeneasr left a comment

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

aeneasr Aug 28, 2023

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

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

Conversation

aeneasr commented Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 28, 2023 • edited Loading

Codecov Report

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

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

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

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

aeneasr commented Aug 25, 2023 •

edited

Loading

codecov bot commented Aug 28, 2023 •

edited

Loading