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: add tests for two step login #3959

Merged
merged 11 commits into from
Jul 5, 2024

Conversation

jonas-jonas
Copy link
Member

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

@jonas-jonas jonas-jonas self-assigned this Jun 19, 2024
@jonas-jonas jonas-jonas force-pushed the jonas-jonas/e2eTestsTwoStepLogin branch 2 times, most recently from e9a356b to 57b6a6f Compare June 20, 2024 12:32
@aeneasr aeneasr force-pushed the jonas-jonas/e2eTestsTwoStepLogin branch from 57b6a6f to 84fff3c Compare June 24, 2024 08:33
Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.33%. Comparing base (11a28a1) to head (78c3a9f).

Files Patch % Lines
selfservice/strategy/password/login.go 71.42% 2 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           two-step-login    #3959      +/-   ##
==================================================
+ Coverage           78.31%   78.33%   +0.01%     
==================================================
  Files                 367      367              
  Lines               25876    25875       -1     
==================================================
+ Hits                20265    20269       +4     
+ Misses               4070     4066       -4     
+ Partials             1541     1540       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonas-jonas jonas-jonas marked this pull request as ready for review June 26, 2024 12:57
Comment on lines +397 to +403
return errors.WithStack(idfirst.ErrNoCredentialsFound)
}
o := login.NewFormHydratorOptions(opts)

// If the identity hint is nil and account enumeration mitigation is disabled, we return an error.
if o.IdentityHint == nil && !s.deps.Config().SecurityAccountEnumerationMitigate(r.Context()) {
return errors.WithStack(idfirst.ErrNoCredentialsFound)
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem here was, that if the user does not exist, we still trigger the "second step" which shows the "Send code" button, but after clicking that you get an error message that the account does not exist.

This change "short-circuits" that, and immediately shows the "account doesn't exist" error after entering the email address (if account enumeration shouldn't be mitigated).
If enumeration should be mitigated, this works the same way, the password strategy works.

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.

Looks great, but I think there's a couple of tests missing

configOverride: toConfig({ mitigateEnumeration }),
})

test("login", async ({ page, config, kratosPublicURL }) => {
Copy link
Member

Choose a reason for hiding this comment

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

OIDC also works for two step:

  1. Enter email
  2. Account with email is found that has OIDC set up
  3. Show only available oidc providers if account enumeration protection if off
  4. Show all available oidc providers if account enumeration protectionif on

I think we should cover that case here too

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good, point. Will add.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

})

await page.goto("/login")

Copy link
Member

Choose a reason for hiding this comment

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

This test isn't doing anything besides registration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Passkeys autofills when you visit the page and submits the login automatically. This might feel unexpected for some users, though, so we might need to adjust this based on user fedback.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, there is no user interaction required where i click on the email field?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also added a test for the second step.

@aeneasr aeneasr merged commit 232db44 into two-step-login Jul 5, 2024
24 checks passed
@aeneasr aeneasr deleted the jonas-jonas/e2eTestsTwoStepLogin branch July 5, 2024 11:31
aeneasr pushed a commit that referenced this pull request Jul 8, 2024
aeneasr pushed a commit that referenced this pull request Jul 11, 2024
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.

2 participants