feat: require verified address #1355

dadrus · 2021-05-18T09:03:52Z

Related issue

@aeneasr: This is a promised PR for #1328.

Proposed changes

As discussed in #1328. The only difference is the configuration. Instead of making the hook available in before property, the after is used as the information about the identity is only available there.

flows:
    login:
      ui_url: http://127.0.0.1:9090/identity/login
      lifespan: 10m
      after:
        hooks:
          - hook: require_verified_address

Checklist

I have read the contributing 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 green light (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.

dadrus · 2021-05-18T09:28:11Z

@aeneasr: In addition to the documentation update (which will follow), I would like to clarify one thing:

An identity can have multiple verifiable addresses. The current implementation of the PR expects all of these to be verified. This might be an issue. One could change the behavior to make it expect at least one verified address, but this might be problematic as well. In our case, we would like to have the eMail address used for the current login attempt verified. So maybe kind of a configuration with useful defaults is required. So something like this:

  hook: require_verified_address
  config:
    verify: all # or alternatively 'one', or 'current'

If the config is omitted, one might be a useful default.

If we go this way, how can I get the information about the email address used for the login? But there are also other related topics. In our case, we also support logins with user names. In this case, the expectation is, that at least one email address is verified. This however would not be possible to configure given the above configuration approach.

Any ideas from your side?

Zageron · 2021-05-18T17:56:39Z

What happens if someone's session expires immediately after changing their primary email, and they misspell it? Blocked from logging in because email is unverified and unable to verify because wrong email? Does the email change flow have a verification component in order to successfully apply?

radekg · 2021-05-18T18:07:18Z

Interesting question the one above. I’d expect the app to not allow changing an email address if there is no another confirmed address on the account.

aeneasr · 2021-05-21T20:20:29Z

Hello there, this is a great contribution! Sorry for the late review but heading off to holidays and had a ton of stuff on my plate :)

So generally I would suggest to avoid the one / all / current mechanism. If we add more authentication methods, this will get blurier. What for example about OpenID Connect logins that do not have an email address associated?

I think the "one" strategy is good enough. If the user has verified an address I think it is safe to assume that the user has passed that challenge. I'm not sure if having multiple addresses be verified would add an additional benefit - on the contrary! Users might be wondering "why do I need to verify several email addresses?" or "I verified my email address why can I not log in?".

From my side - the one strategy as the default would be absolutely good enough! :)

radekg · 2021-05-22T00:31:01Z

On the other hand, if I signed up using an email and a password, I should be able to recover using one of the social registrations via account linking.

I did that once with meetup.com. I created an account using facebook sso but I deleted my facebook account and was unable to sign in. I was able to recover the account by signing up with google directly. This linked the account to the old one and all was good.

The second thought I have, isn't signing up with social login an implied verification? If I signed up with Google SSO, Google just told you that the identity is genuine. If someone else was able to sign up using my Google account, well, that was a game over on my side because my Google account was already compromised. So a registration via SSO linking implies a verified address, IMO.

Enjoy your holidays.

dadrus · 2021-05-22T16:44:46Z

@aeneasr: Yeah, simpler is better :) I'll update the code accordingly next week and will also add documentation.

Enjoy the long weekend!

dadrus · 2021-05-22T16:49:08Z

@radekg: Maybe I miss something, but how do you see this PR is related to recovery?

Regarding your thoughts about social logins:

The second thought I have, isn't signing up with social login an implied verification?

I see it exactly as you do: a registration via SSO linking implies a verified address. So this configuration option should not have any effects for social logins.

However I don't know how kratos behaves here. @aeneasr: If the user consent kratos to receive the email address from the AP, will it be added to VerifiableAddresses at all. And if yes, is it then marked as verified?

radekg · 2021-05-22T22:39:29Z

@radekg: Maybe I miss something, but how do you see this PR is related to recovery?

Sorry, I think I’ve mixed stuff up with #1323 (comment)

…ss is verified if password method is used

dadrus · 2021-05-26T17:22:27Z

@aeneasr: Please take a look. There are some questions (as comments), which I would like to clarify, before finalizing the implementation and based on that updating the documentation.

aeneasr

This looks great! Only thing we need to add is some documentation to: https://www.ory.sh/kratos/docs/next/self-service/hooks/#after

It probably also makes sense to only allow this hook for password flows? For OIDC flows I don't think it makes sense?

aeneasr · 2021-05-27T16:33:51Z

selfservice/flow/login/hook.go

+	// why (is there any reason) is it (Flow.Active) not set in the implementation of the corresponding Login
+	// method (strategy_login.go:L91 ff and login.go:L51 ff)?
+	// this can be easily achieved by the following lines
+	//
+	// f.Active = identity.CredentialsTypePassword // or identity.CredentialsTypeOidc
+	//
+	// if err = s.d.LoginFlowPersister().UpdateLoginFlow(r.Context(), f); err != nil {
+	// 	return nil, s.handleLoginError(w, r, f, &p, errors.WithStack(herodot.ErrInternalServerError.WithReason("Could not update flow").WithDebug(err.Error())))
+	// }
+	//
+	// in the right place. Otherwise the Flow.Active property is never set (only in tests).
+	// If done as described above, there wouldn't be a need to pass around the `ct` argument as well, like in this method.
+	// So, FMPOV, the following line is just a hack due to the missing implementation described above.


So Active refers to the method the user used to sign in / up / verify / ...

So once you e.g. click "sign in with username / password" this would be set to "password". If you use OIDC this would be "oidc". It's basically a hint for the UI implementor.

Therefore, I don't think we need to set it here!

This is exactly the point. Active is never set. If it would, I would not have implemented the workaround after the comment. Just look for the places Active is used in (where it is set).
The purpose of that property is clear. I just wanted to know why it is never set. If this is a bug, I would then fix it as part of this PR. The relevant places are obvious for me.

@aeneasr: Since Active is never set, I consider it as a bug and will fix it as part of the next commits.

dadrus · 2021-05-27T17:19:44Z

It probably also makes sense to only allow this hook for password flows? For OIDC flows I don't think it makes sense?

Yes, I was thinking to adjust the schema for this purpose. Will come with the next commits

…ured for password login only

…od. Due to this, there is no need for an additional CredentialType attribute, as this information is now available from the flow object itself

dadrus · 2021-06-07T16:35:02Z

@aeneasr: Please take a look. If I haven't forgotten anything, this PR should be complete.

aeneasr · 2021-06-08T09:16:30Z

Rerunning flaky CI :)

aeneasr · 2021-06-08T09:25:32Z

The last thing we could add would be an e2e test which verifies that this is also working e2e. It could be added for completeness - we're already doing the check in login_test. If you want to give that a shot, here's how you run e2e tests: https://github.com/ory/kratos#end-to-end-tests

You could add a new file login here: https://github.com/ory/kratos/tree/master/test/e2e/cypress/integration/profiles/verification

To create a user to this: https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/email/login/success.spec.js#L8

To verify an email, do this: https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/verification/settings/success.spec.js#L34

To go to login use: https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/email/login/success.spec.js#L11-L19

You also need to add your hook to: https://github.com/ory/kratos/blob/master/test/e2e/profiles/verification/.kratos.yml

If that has other tests failing now (because they need this hook to be disabled) you can add a command that updates the hook like so: https://github.com/ory/kratos/blob/master/test/e2e/cypress/support/commands.js#L96-L101

And then use that command in your test: https://github.com/ory/kratos/blob/master/test/e2e/cypress/support/commands.js#L439

Make sure to reset it after you test ran!

The file tests if login works when the email is not verified (it should not) and when it is verified (it should).

dadrus · 2021-06-10T11:49:13Z

@aeneasr: I need your help. This is first time I have to deal with JS and cypress. I tried to implement the test cases, but unfortunately there were issues running the registration scenario locally. Only after completely rewriting the register command in commands.js the registration was successful. This way I was able to finalize the negative test. But now I run into issues while trying to verify the email in a successful case. The mailslurper just doesn't receive any emails and I don't understand why. Could you please support here? Thank you in advance.
If it helps: The services were started by running ./test/e2e/run.sh sqlite verification

aeneasr · 2021-07-14T08:38:22Z

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

dadrus · 2021-07-28T13:04:03Z

@Quasarman: I was on vacation until yesterday and before it, there were some urgent topics in a project I'm working for, which I had to address.
The actual implementation is actually complete. The things which are missing are the e2e tests, which I will address asap

Quasarman · 2021-07-28T20:01:33Z

Hello @dadrus ,

no worries thanks a lot for your efforts!

aeneasr · 2021-07-29T14:35:31Z

Nice, I hope you enjoyed your days off @dadrus :)

dadrus · 2021-07-30T10:47:37Z

Definitely :) Recharging batteries feels always good ;)

…ified_address # Conflicts: # test/e2e/cypress/integration/profiles/verification/registration/errors.spec.js # test/e2e/cypress/integration/profiles/verification/settings/success.spec.js

dadrus · 2021-08-03T16:20:01Z

@aeneasr: There are no much changes in the last commit - only some simplifications compared to what you've done during your call.
Regarding the hooks reset after a test: After taking a closer look at all the tests, I feel, there is not need to reset the configuration changes done by the new tests. According to my understanding of the code, each test makes use of the cy.useConfigProfile command in the before fixture. That basically resets the entire configuration to its initial state. Then the test modifies what is required and that's all. The only one thing which makes me a bit curious are the following lines in the commands.js file:

let previousProfile = ''
Cypress.Commands.add('useConfigProfile', (profile) => {
  if (profile === previousProfile) {
    return
  }

  cy.readFile(`test/e2e/kratos.${profile}.yml`).then((contents) =>
    cy.writeFile(configFile, contents)
  )
  cy.wait(100)
})

Initially I thought, the usage of previousProfile would require the reset, you were asking for. But it looks like previousProfile is never set (so it is a dead code). If this is indeed true, then the reset happens as I've written above by just calling cy.useConfigProfile with the name of the required profile. Those kratos.${profile}.yml files are never modified by a test code.

aeneasr · 2021-08-04T08:27:30Z

@dadrus yes you are right, looks like we don't need the reset method at all :)

dadrus · 2021-08-04T09:09:28Z

I well then remove the above said dead code, also to avoid confusion in the future. When this is done and all the tests are green (my expectation 😉), this PR will be complete.

codecov · 2021-08-04T09:44:58Z

Codecov Report

Merging #1355 (5cdfad5) into master (1aaf6c0) will decrease coverage by 0.02%.
The diff coverage is 87.03%.

@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
- Coverage   74.19%   74.16%   -0.03%     
==========================================
  Files         258      259       +1     
  Lines       12577    12618      +41     
==========================================
+ Hits         9331     9358      +27     
- Misses       2627     2637      +10     
- Partials      619      623       +4

Impacted Files	Coverage Δ
driver/registry_default.go	`87.05% <ø> (ø)`
selfservice/flow/login/error.go	`77.41% <ø> (ø)`
selfservice/strategy/oidc/strategy_login.go	`63.33% <25.00%> (-1.58%)`	⬇️
selfservice/strategy/password/login.go	`82.97% <33.33%> (-3.39%)`	⬇️
selfservice/flow/login/handler.go	`68.99% <60.00%> (+1.02%)`	⬆️
driver/registry_default_hooks.go	`90.69% <100.00%> (+1.50%)`	⬆️
schema/errors.go	`56.69% <100.00%> (+2.91%)`	⬆️
selfservice/flow/login/hook.go	`90.90% <100.00%> (+1.51%)`	⬆️
selfservice/hook/address_verifier.go	`100.00% <100.00%> (ø)`
text/message_validation.go	`64.70% <100.00%> (+2.80%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3eb87bc...5cdfad5. Read the comment docs.

…le is removed

…ode review

aeneasr · 2021-08-04T11:55:56Z

Sorry for the breaking test - I had a fix locally but looks like you were faster 😅

dadrus · 2021-08-04T11:56:34Z

@aeneasr: If you're not going to break any further tests in a review :), I would say I've done everything required for this feature. Last commits have a self explanatory commit comments :), so feel free to merge or ask for further updates ;)

aeneasr · 2021-08-04T12:05:00Z

yeah sorry about that again 😓 also LGTM from my side

dadrus · 2021-08-04T12:22:20Z

Don't worry ;)

aeneasr · 2021-08-04T13:16:00Z

🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳

dadrus added 3 commits May 18, 2021 10:46

schema update to support require_verified_address hook

a702187

the actual implementation of the require_verified_address hook

c9a07a4

integration of the require_verified_address & tests

7535bcb

dadrus added 3 commits May 18, 2021 11:32

code formatted

6bff1fa

address_verifier error now leads to a proper ui handling

9d30ce5

code formatting

0d06d7d

Merge branch 'master' into feature/require_verified_address

a9a4439

address_verifier behavior changed to check whether at least one addre…

adebf24

…ss is verified if password method is used

aeneasr reviewed May 27, 2021

View reviewed changes

dadrus added 4 commits June 7, 2021 16:04

schema adjusted to allow 'require_verified_address' hook to be config…

24ad172

…ured for password login only

Documentation updated

5eb0504

flow.Active is now set by the implementation of a strategy Login meth…

fcfc020

…od. Due to this, there is no need for an additional CredentialType attribute, as this information is now available from the flow object itself

schema tests

26dbb45

Merge branch 'master' into feature/require_verified_address

2b4f81e

aeneasr approved these changes Jun 8, 2021

View reviewed changes

first attempt to have proper e2e tests

df11f78

aeneasr marked this pull request as draft July 14, 2021 08:38

aeneasr marked this pull request as ready for review August 3, 2021 13:05

aeneasr requested a review from zepatrik as a code owner August 3, 2021 13:05

dadrus and others added 4 commits August 3, 2021 15:06

next e2e test update

a3649e0

Merge remote-tracking branch 'origin/master' into feature/require_ver…

4cf4a8d

…ified_address # Conflicts: # test/e2e/cypress/integration/profiles/verification/registration/errors.spec.js # test/e2e/cypress/integration/profiles/verification/settings/success.spec.js

chore: code review

2f38626

tests simplified

81b253c

dadrus added 2 commits August 3, 2021 18:54

make format applied

3d67c94

fixing test after merge

3c9e315

aeneasr and others added 5 commits August 4, 2021 12:40

chore: code review

929c477

As it is dead code and leads to confusion, the usage of previousProfi…

473474c

…le is removed

address verifier test fixed after being broken after changes in the c…

b0906d7

…ode review

make format applied

023297b

documentation updated

5cdfad5

aeneasr merged commit 1cf61cd into ory:master Aug 4, 2021

dadrus deleted the feature/require_verified_address branch August 4, 2021 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: require verified address #1355

feat: require verified address #1355

dadrus commented May 18, 2021 •

edited

Loading

dadrus commented May 18, 2021

Zageron commented May 18, 2021

radekg commented May 18, 2021

aeneasr commented May 21, 2021

radekg commented May 22, 2021 •

edited

Loading

dadrus commented May 22, 2021

dadrus commented May 22, 2021 •

edited

Loading

radekg commented May 22, 2021

dadrus commented May 26, 2021

aeneasr left a comment

aeneasr May 27, 2021

dadrus May 27, 2021 •

edited

Loading

dadrus Jun 4, 2021

dadrus commented May 27, 2021

dadrus commented Jun 7, 2021

aeneasr commented Jun 8, 2021

aeneasr commented Jun 8, 2021 •

edited

Loading

dadrus commented Jun 10, 2021 •

edited

Loading

aeneasr commented Jul 14, 2021

dadrus commented Jul 28, 2021

Quasarman commented Jul 28, 2021

aeneasr commented Jul 29, 2021

dadrus commented Jul 30, 2021

dadrus commented Aug 3, 2021 •

edited

Loading

aeneasr commented Aug 4, 2021

dadrus commented Aug 4, 2021

codecov bot commented Aug 4, 2021 •

edited

Loading

aeneasr commented Aug 4, 2021

dadrus commented Aug 4, 2021

aeneasr commented Aug 4, 2021

dadrus commented Aug 4, 2021

aeneasr commented Aug 4, 2021

feat: require verified address #1355

feat: require verified address #1355

Conversation

dadrus commented May 18, 2021 • edited Loading

Related issue

Proposed changes

Checklist

dadrus commented May 18, 2021

Zageron commented May 18, 2021

radekg commented May 18, 2021

aeneasr commented May 21, 2021

radekg commented May 22, 2021 • edited Loading

dadrus commented May 22, 2021

dadrus commented May 22, 2021 • edited Loading

radekg commented May 22, 2021

dadrus commented May 26, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr May 27, 2021

Choose a reason for hiding this comment

dadrus May 27, 2021 • edited Loading

Choose a reason for hiding this comment

dadrus Jun 4, 2021

Choose a reason for hiding this comment

dadrus commented May 27, 2021

dadrus commented Jun 7, 2021

aeneasr commented Jun 8, 2021

aeneasr commented Jun 8, 2021 • edited Loading

dadrus commented Jun 10, 2021 • edited Loading

aeneasr commented Jul 14, 2021

dadrus commented Jul 28, 2021

Quasarman commented Jul 28, 2021

aeneasr commented Jul 29, 2021

dadrus commented Jul 30, 2021

dadrus commented Aug 3, 2021 • edited Loading

aeneasr commented Aug 4, 2021

dadrus commented Aug 4, 2021

codecov bot commented Aug 4, 2021 • edited Loading

Codecov Report

aeneasr commented Aug 4, 2021

dadrus commented Aug 4, 2021

aeneasr commented Aug 4, 2021

dadrus commented Aug 4, 2021

aeneasr commented Aug 4, 2021

dadrus commented May 18, 2021 •

edited

Loading

radekg commented May 22, 2021 •

edited

Loading

dadrus commented May 22, 2021 •

edited

Loading

dadrus May 27, 2021 •

edited

Loading

aeneasr commented Jun 8, 2021 •

edited

Loading

dadrus commented Jun 10, 2021 •

edited

Loading

dadrus commented Aug 3, 2021 •

edited

Loading

codecov bot commented Aug 4, 2021 •

edited

Loading