-
-
Notifications
You must be signed in to change notification settings - Fork 964
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
Conversation
e9a356b
to
57b6a6f
Compare
57b6a6f
to
84fff3c
Compare
Codecov ReportAttention: Patch coverage is
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. |
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 }) => { |
There was a problem hiding this comment.
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:
- Enter email
- Account with email is found that has OIDC set up
- Show only available oidc providers if account enumeration protection if off
- Show all available oidc providers if account enumeration protectionif on
I think we should cover that case here too
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
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.
works.
Further Comments