feat: Mark recovery email address verified #1665

dadrus · 2021-08-19T15:55:58Z

Related issue(s)

Checklist

I have read the contributing guidelines.
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 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.

Further Comments

The implementation idea is still the same as described in #1662. This PR does however introduce a separate function for marking the address as verified instead of misusing the recoveryIssueSession method.

dadrus · 2021-08-20T07:33:53Z

@aeneasr: if this is the way to go, I'll add the required tests and extend the documentation. Please take a look.

aeneasr · 2021-08-23T12:17:21Z

Yeah that looks good! :)

…address_verified

…ed for recovery, the status of it is set to StatusSent

dadrus · 2021-09-01T15:45:02Z

@aeneasr : Functionality wise I consider this PR to be complete. So please take a look. After taking a closer look at already existing documentation, it already states, that the verification is initiated by the registration and verification flows. This way there is no need for documentation update.

…address_verified

codecov · 2021-09-01T16:27:20Z

Codecov Report

Merging #1665 (c201dae) into master (5b456b3) will increase coverage by 0.00%.
The diff coverage is 69.23%.

@@           Coverage Diff           @@
##           master    #1665   +/-   ##
=======================================
  Coverage   74.10%   74.11%           
=======================================
  Files         260      260           
  Lines       12715    12733   +18     
=======================================
+ Hits         9423     9437   +14     
- Misses       2667     2669    +2     
- Partials      625      627    +2

Impacted Files	Coverage Δ
selfservice/strategy/link/strategy_recovery.go	`67.92% <69.23%> (+0.91%)`	⬆️

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 5b456b3...c201dae. Read the comment docs.

aeneasr

Nice, this looks pretty much mergeable! I’ve got some follow up questions and some ideas for improvement!

aeneasr · 2021-09-04T13:06:22Z

selfservice/strategy/link/strategy_recovery.go

-	if err := s.d.RecoveryFlowPersister().UpdateRecoveryFlow(r.Context(), req); err != nil {
-		return s.HandleRecoveryError(w, r, req, body, err)
+	if address != nil && !address.Verified {
+		address.Status = identity.VerifiableAddressStatusSent


While I think it makes sense to mark the verified address as verified if we perform recovery with it, I think that it should stay untouched otherwise. For example, we are not marking the recovery email as status sent if we verify it. What do you think?

The recovery address is actually a separate struct. As such it does even not have such fields. But if the recovery eMail address is the same, as the eMail address used for verification purposes. In such case, IMHO the verifiable address object should receive the same updates as it receives in a regular verification flow. In both cases an eMail verification is done by sending an email to the user and letting it click the corresponding link. This is also why there is a check for address != nil. So I would not change the current implementation on this PR.

Are you not conviced with my answer? ;)

Sorry, the 🏝 got in the way of me reading it 😅

Half half, I still see them as separate things. I agree that the flows work the same way (OOB TOTP over email) implying that solving one challenge is line solving the other. But then again they have different behaviors, lifespans, timeouts, and might have different security guards in the future.

At the same time, the link strategy is on the way out due to it’s many issues so to me it’s not that important!

If I was a dev and would do a recovery flow, and then see „hhmm now verification was also impacted“, I would probably open a bug report about it 😉

Everything is fine. I'm not pushing ;)

But then again they have different behaviors, lifespans, timeouts, and might have different security guards in the future.

They definitely do and still have. But we're talking about the end effect. And that can be the same, which is also stated by the current account-activation-email-verification documentation. The verify-email-account-activation document does not differentiate recovery and verification flows effects for email verification as well.

Actually, I fully agree with you if the email address used for recovery purposes differs from the email for account identification purposes (thus for login). And this situation is taken into account by the given PR. If they are however the same, I personally would expect the same state changes. And this is IMHO not related to lifespans, timeouts etc. They can for sure differ.

Btw: This is not the single place where the state is updated. The lines 423ff do this as well.

Anyhow, I can change the implementation as you're asking for (even I'm not convinced, you can see a couple of line below why exactly), but then there is a need for documentation update to make the differences obvious and clear. In addition there is a need then to find a solution for the following:
When a new VerifiableAddress is created, its status is set to identity.VerifiableAddressStatusPending. When no recovery address is configured, the recovery address is set to the value of the email used for VerifiableAddress. When a recovery flow is executed the VerifiableAddress' state will still be in the identity.VerifiableAddressStatusPending status, even the same email address has been used and this way the VerifiableAddress was actually verified. And now take please the position of

a potential user of such a system (which requires the password_identifier to be a verified email for login purposes): You register and do not follow the link in the verification email (for whatever reasons, say you just forgot it). Some time later you come back and would like to login. Since the email is still not verified, you most probably will start the "password forgotten" (recovery flow). You change the password and are logged in. Some time later you come back and try to login again - but it doesn't work (as the system handles the same email address differently for different flows). - Ok, to be honest, I'm lying here, as the given PR makes use of the Verified attribute and not the Stateattribute.

of a potential developer. You discover the VerifiableAddress, which has the fields State and Verified. Both seems to be kind of redundant, as Verified=true could be considered to correspond to State=identity.VerifiableAddressStatusVerified. But this actually not the case, Verified could be true even the State would still be identity.VerifiableAddressStatusPending. This is actually my answer to your comment:

If I was a dev and would do a recovery flow, and then see „hhmm now verification was also impacted“, I would probably open a bug report about it.

So I were this dev I would rather open a bug report about that mismatch 😄 . How would you react?

Actually, since we have this discussion, I would rather remove the Verified attribute and rely on the State attribute. This would make the implementation much simpler (see point 2 above) and without redundancy, but still be open for having different behavior for the recovery and verification flows as well as open for potential enhancements, like security guards in the future.

The verify-email-account-activation document does not differentiate recovery and verification flows effects for email verification as well.

That is definitely a copy/paste error :D

I agree with you here - if the address is verified the state should reflect that! And yes it’s definitely redundant. I thought it might be useful to have a simple bool flag reflecting the true/false status rather than the state, which is used internally.

By the way, I believe that unless you do not have the verify hook, the state here will almost always be „sent“! We use it internally to decide if a verification mail should be sent or not, for example when you update your email.

Regarding your recovery use case - it makes sense. It would be confusing to the user if they do recovery and still can‘t sign in because they should have done verification. I don’t really know how other sites are handling this. In general I think the problem is that we issue a session here, without doing any hook checks. This logic in general is still pretty akward IMO.

For the emails themselves I think there is a difference - you would e.g. want to send a double consent email (email verify) rather than mingle that with recovery.

In the end both approaches look acceptable to me here. I still feel that we are tricking the system here a bit - we are sending a recovery email and say that we sent the verification email too. Then again most people have this status set to sent already if the hook is used.

One thing is for sure though, if the status is verified already, this shouldn’t be touched!

ps: thank you for taking the input seriously even if the change is minimal 😅

selfservice/strategy/link/strategy_recovery.go

selfservice/strategy/link/strategy_recovery_test.go

dadrus · 2021-09-07T08:21:59Z

@aeneasr: Could you please restart the test job? There is some issue with the db not related to the latest commits at all.

zepatrik · 2021-09-07T09:36:13Z

Done ✔️

dadrus · 2021-09-07T10:25:07Z

Looks much better now ;) Thank you. Please let me know if you would like to have something more changed/enhanced.

…atusSent when an email for recovery purposes was sent

dadrus · 2021-09-22T08:59:37Z

@aeneasr: The 41ce12c commit removes the status update as you've asked for. The other commits just ensure everything is working with the current master state.

aeneasr · 2021-09-22T09:06:31Z

Awesome, thanks! Could you please add a note to the docs in the recovery part saying that we will mark the address as verified also if they match? Then this is mergable :)

Initial implementations. Tests to be added

0d7092a

dadrus requested review from aeneasr and zepatrik as code owners August 19, 2021 15:55

aeneasr marked this pull request as draft August 24, 2021 12:45

dadrus added 3 commits August 25, 2021 09:57

Merge remote-tracking branch 'ory/master' into feature/mark_recovery_…

bbeda57

…address_verified

If there is a not verified verifiable address for an email address us…

8b8e21b

…ed for recovery, the status of it is set to StatusSent

tests

7025759

dadrus added 2 commits September 1, 2021 17:52

goimports and gosec findings fixed

7e580a7

Merge remote-tracking branch 'ory/master' into feature/mark_recovery_…

debb59e

…address_verified

dadrus marked this pull request as ready for review September 3, 2021 11:51

aeneasr requested changes Sep 4, 2021

View reviewed changes

dadrus added 3 commits September 6, 2021 10:31

making use of index

8a149ab

tests refactored

850c61d

Merge branch 'master' into feature/mark_recovery_address_verified

d95d363

dadrus added 3 commits September 22, 2021 10:48

don't set the status of the verifiable address to VerifiableAddressSt…

41ce12c

…atusSent when an email for recovery purposes was sent

Merge branch 'master' into feature/mark_recovery_address_verified

3d30860

fixing code and tests after merge with master

bec4fe6

docu update

c201dae

aeneasr merged commit e3efc5d into ory:master Sep 22, 2021

dadrus deleted the feature/mark_recovery_address_verified branch September 22, 2021 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Mark recovery email address verified #1665

feat: Mark recovery email address verified #1665

dadrus commented Aug 19, 2021 •

edited

dadrus commented Aug 20, 2021

aeneasr commented Aug 23, 2021

dadrus commented Sep 1, 2021

codecov bot commented Sep 1, 2021 •

edited

aeneasr left a comment

aeneasr Sep 4, 2021

dadrus Sep 6, 2021

dadrus Sep 8, 2021

aeneasr Sep 10, 2021

dadrus Sep 10, 2021 •

edited

aeneasr Sep 11, 2021

aeneasr Sep 11, 2021

dadrus commented Sep 7, 2021

zepatrik commented Sep 7, 2021

dadrus commented Sep 7, 2021

dadrus commented Sep 22, 2021

aeneasr commented Sep 22, 2021

feat: Mark recovery email address verified #1665

feat: Mark recovery email address verified #1665

Conversation

dadrus commented Aug 19, 2021 • edited

Related issue(s)

Checklist

Further Comments

dadrus commented Aug 20, 2021

aeneasr commented Aug 23, 2021

dadrus commented Sep 1, 2021

codecov bot commented Sep 1, 2021 • edited

Codecov Report

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Sep 4, 2021

Choose a reason for hiding this comment

dadrus Sep 6, 2021

Choose a reason for hiding this comment

dadrus Sep 8, 2021

Choose a reason for hiding this comment

aeneasr Sep 10, 2021

Choose a reason for hiding this comment

dadrus Sep 10, 2021 • edited

Choose a reason for hiding this comment

aeneasr Sep 11, 2021

Choose a reason for hiding this comment

aeneasr Sep 11, 2021

Choose a reason for hiding this comment

dadrus commented Sep 7, 2021

zepatrik commented Sep 7, 2021

dadrus commented Sep 7, 2021

dadrus commented Sep 22, 2021

aeneasr commented Sep 22, 2021

dadrus commented Aug 19, 2021 •

edited

codecov bot commented Sep 1, 2021 •

edited

dadrus Sep 10, 2021 •

edited