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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
Aug 29, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d17bf75
chore: synchronize workspaces
aeneasr Aug 25, 2023
569371f
chore: synchronize workspaces
aeneasr Aug 25, 2023
56dd054
chore: synchronize workspaces
aeneasr Aug 28, 2023
aa2703e
chore: remove code bloat
aeneasr Aug 28, 2023
ed4b552
chore: synchronize workspaces
aeneasr Aug 28, 2023
d2dab02
chore: synchronize workspaces
aeneasr Aug 28, 2023
193d33c
chore: synchronize workspaces
aeneasr Aug 28, 2023
b8cdb26
chore: synchronize workspaces
aeneasr Aug 28, 2023
bcd626b
chore: synchronize workspaces
aeneasr Aug 28, 2023
0582f6c
chore: synchronize workspaces
aeneasr Aug 28, 2023
55fa3eb
chore: synchronize workspaces
aeneasr Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions selfservice/strategy/code/code_sender.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,13 @@ 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
// generating different codes per address would reduce the search space for an attacker.
//
// See also [this discussion](https://github.com/ory/kratos/pull/3378#discussion_r1305436968).
rawCode := GenerateCode()

// send to all addresses
for _, address := range addresses {
// We have to generate a unique code per address, or otherwise it is not possible to link which
// address was used to verify the code.
//
// See also [this discussion](https://github.com/ory/kratos/pull/3456#discussion_r1307560988).
rawCode := GenerateCode()
Copy link
Member Author

Choose a reason for hiding this comment

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

My bad :D


switch f.GetFlowName() {
case flow.RegistrationFlow:
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/cypress/integration/profiles/code/login/success.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ context("Login success with code method", () => {
"traits.email2": email2,
},
})

// There are verification emails from the registration process in the inbox that we need to deleted
// for the assertions below to pass.
cy.deleteMail({ atLeast: 1 })

cy.visit(route)
Expand Down
25 changes: 16 additions & 9 deletions test/e2e/cypress/integration/profiles/code/registration/error.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,12 @@ 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(
"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")
Copy link
Member Author

Choose a reason for hiding this comment

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

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

cy.submitCodeForm()

cy.get('[data-testid="ui/message/4040003"]').should(
Expand All @@ -56,12 +55,15 @@ context("Registration error messages with code method", () => {

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

cy.url().should("contain", "registration")
cy.get('input[name="traits.email"]')
.clear()
.type("[email protected]")
cy.get(' input[name="code"]').type("invalid-code")
cy.get('input[name="code"]').type("invalid-code")
cy.submitCodeForm()

cy.get('[data-testid="ui/message/4000030"]').should(
Expand All @@ -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(
Copy link
Member Author

Choose a reason for hiding this comment

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

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

  1. Go to /registration
  2. Submit form
  3. Wait for URL to be registration (it already is registration though!)
  4. 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.

"contain",
"An email containing a code has been sent to the email address you provided",
)

cy.removeAttribute(['input[name="code"]'], "required")
cy.submitCodeForm()

cy.submitCodeForm()
cy.get('[data-testid="ui/message/4000002"]').should(
"contain",
"Property code is missing",
Expand All @@ -105,11 +109,14 @@ context("Registration error messages with code method", () => {
cy.visit(route)

const email = gen.email()

cy.get('input[name="traits.email"]').type(email)

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

cy.url().should("contain", "registration")
cy.getRegistrationCodeFromEmail(email).should((code) => {
cy.get('input[name="code"]').type(code)
cy.submitCodeForm()
Expand Down
63 changes: 36 additions & 27 deletions test/e2e/cypress/integration/profiles/code/registration/success.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,23 @@ context("Registration success with code method", () => {
it("should be able to resend the registration code", async () => {
const email = gen.email()

cy.get(` input[name='traits.email']`).type(email)
cy.get(`input[name='traits.email']`).type(email)

cy.submitCodeForm()

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

cy.getRegistrationCodeFromEmail(email).should((code) =>
cy.wrap(code).as("code1"),
)

cy.get(` input[name='traits.email']`).should("have.value", email)
cy.get(` input[name='method'][value='code'][type='hidden']`).should(
cy.get(`input[name='traits.email']`).should("have.value", email)
cy.get(`input[name='method'][value='code'][type='hidden']`).should(
"exist",
)
cy.get(` button[name='resend'][value='code']`).click()
cy.get(`button[name='resend'][value='code']`).click()

cy.getRegistrationCodeFromEmail(email).should((code) => {
cy.wrap(code).as("code2")
Expand All @@ -61,8 +63,8 @@ context("Registration success with code method", () => {
cy.get("@code1").then((code1) => {
// previous code should not work
cy.get('input[name="code"]').clear().type(code1.toString())
cy.submitCodeForm()

cy.submitCodeForm()
cy.get('[data-testid="ui/message/4040003"]').should(
"contain.text",
"The registration code is invalid or has already been used. Please try again.",
Expand All @@ -89,10 +91,13 @@ context("Registration success with code method", () => {
cy.get(` input[name='traits.email']`).type(email)

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

cy.url().should("contain", "registration")
cy.getRegistrationCodeFromEmail(email).should((code) => {
cy.get(` input[name=code]`).type(code)
cy.get(`input[name=code]`).type(code)
cy.get("button[name=method][value=code]").click()
})

Expand All @@ -109,29 +114,28 @@ context("Registration success with code method", () => {
cy.setPostCodeRegistrationHooks([])
const email = gen.email()

cy.get(` input[name='traits.email']`).type(email)
cy.get(`input[name='traits.email']`).type(email)

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

cy.url().should("contain", "registration")
cy.getRegistrationCodeFromEmail(email).should((code) => {
cy.get(` input[name=code]`).type(code)
cy.get(`input[name=code]`).type(code)
cy.get("button[name=method][value=code]").click()
})

cy.deleteMail({ atLeast: 1 })
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


cy.visit(login)
cy.get(` input[name=identifier]`).type(email)
cy.get(`input[name=identifier]`).type(email)
cy.get("button[name=method][value=code]").click()

cy.getLoginCodeFromEmail(email).then((code) => {
cy.get(`input[name = code]`).type(code)
cy.get(`input[name=code]`).type(code)
cy.get("button[name=method][value=code]").click()
})

cy.deleteMail({ atLeast: 1 })

cy.getSession().should((session) => {
const { identity } = session
expect(identity.id).to.not.be.empty
Expand Down Expand Up @@ -179,35 +183,40 @@ context("Registration success with code method", () => {
cy.get(`input[name='traits.username']`).type(Math.random().toString(36))

const email = gen.email()

cy.get(`input[name='traits.email']`).type(email)

const email2 = gen.email()

cy.get(`input[name='traits.email2']`).type(email2)

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

// intentionally use email 1 to verify the account
cy.url().should("contain", "registration")
cy.getRegistrationCodeFromEmail(email, { expectedCount: 2 }).should(
// intentionally use email 1 to sign up for the account
cy.getRegistrationCodeFromEmail(email, { expectedCount: 1 }).should(
(code) => {
cy.get(`input[name=code]`).type(code)
cy.get("button[name=method][value=code]").click()
},
)

cy.deleteMail({ atLeast: 2 })

cy.logout()

// There are verification emails from the registration process in the inbox that we need to deleted
// for the assertions below to pass.
cy.deleteMail({ atLeast: 1 })

// Attempt to sign in with email 2 (should fail)
cy.visit(login)
cy.get(` input[name=identifier]`).type(email2)
cy.get(`input[name=identifier]`).type(email2)

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

cy.getLoginCodeFromEmail(email2).should((code) => {
cy.getLoginCodeFromEmail(email2, {
expectedCount: 1,
}).should((code) => {
cy.get(`input[name=code]`).type(code)
cy.get("button[name=method][value=code]").click()
})
Expand Down
28 changes: 16 additions & 12 deletions test/e2e/cypress/support/commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,13 @@ Cypress.Commands.add(

Cypress.Commands.add(
"registerWithCode",
({ email = gen.email(), code = undefined, traits = {}, query = {} } = {}) => {
({
email = gen.email(),
code = undefined,
traits = {},
query = {},
expectedMailCount = 1,
} = {}) => {
cy.clearAllCookies()

cy.request({
Expand Down Expand Up @@ -416,7 +422,6 @@ Cypress.Commands.add(
})
.then(({ body }) => {
if (!code) {
console.log("registration with code", body)
expect(
body.ui.nodes.find(
(f: UiNode) =>
Expand All @@ -426,14 +431,9 @@ Cypress.Commands.add(
).attributes.value,
).to.eq(email)

const expectedCount =
Object.keys(traits)
.map((k) => (k.includes("email") ? k : null))
.filter(Boolean).length + 1

return cy
.getRegistrationCodeFromEmail(email, {
expectedCount: expectedCount,
expectedCount: expectedMailCount,
})
.then((code) => {
return cy.request({
Expand Down Expand Up @@ -1260,7 +1260,7 @@ Cypress.Commands.add(
const req = () =>
cy.request(`${MAIL_API}/mail`).then((response) => {
expect(response.body).to.have.property("mailItems")
const count = response.body.mailItems.length
let count = response.body.mailItems.length
if (count === 0 && tries < 100) {
tries++
cy.wait(pollInterval)
Expand All @@ -1269,19 +1269,23 @@ Cypress.Commands.add(

let mailItem: any
if (email) {
mailItem = response.body.mailItems.find((m: any) =>
const filtered = response.body.mailItems.filter((m: any) =>
m.toAddresses.includes(email),
)
if (!mailItem) {

if (filtered.length === 0) {
tries++
cy.wait(pollInterval)
return req()
Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

expect(filtered.length).to.equal(expectedCount)
mailItem = filtered[0]
} else {
expect(count).to.equal(expectedCount)
Comment on lines +1282 to +1285
Copy link
Member Author

Choose a reason for hiding this comment

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

Counting did not work correctly, caused flakes.

mailItem = response.body.mailItems[0]
}

expect(count).to.equal(expectedCount)
if (removeMail) {
return cy.deleteMail({ atLeast: count }).then(() => {
return Promise.resolve(mailItem)
Expand Down
3 changes: 2 additions & 1 deletion test/e2e/cypress/support/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ declare global {
code?: string
traits?: { [key: string]: any }
query?: { [key: string]: string }
expectedMailCount?: number
}): Chainable<Response<void>>

/**
Expand Down Expand Up @@ -731,7 +732,7 @@ declare global {
*/
getRegistrationCodeFromEmail(
email: string,
opts?: { expectedCount: number },
opts?: { expectedCount: number; removeMail?: boolean },
): Chainable<string>

/**
Expand Down
Loading