feat: make notification to unknown recipients configurable #3075

jonas-jonas · 2023-02-06T13:28:35Z

BREAKING CHANGES: Added the ability to configure whether the system should notify unknown recipients, if some tries to recover their account or verify their address ("anti-account-enumeration measures"). 
By default, Kratos no longer sends out these Emails. 
If you want to keep notifying unknown addresses (keep the current behavior), set `selfservice.flows.recovery.notify_unknown_recipients` to `true` for recovery, or `selfservice.flows.verification.notify_unknown_recipients` for verification flows.

Related issue(s)

Closes #2345 & supersedes #2585

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

codecov · 2023-02-06T14:58:03Z

Codecov Report

Merging #3075 (e9985e3) into master (e3eb39e) will decrease coverage by 0.16%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #3075      +/-   ##
==========================================
- Coverage   77.45%   77.30%   -0.16%     
==========================================
  Files         315      315              
  Lines       19955    19849     -106     
==========================================
- Hits        15457    15345     -112     
- Misses       3297     3309      +12     
+ Partials     1201     1195       -6

Impacted Files	Coverage Δ
selfservice/strategy/code/code_sender.go	`78.12% <78.57%> (+0.05%)`	⬆️
selfservice/strategy/link/sender.go	`71.17% <80.00%> (+1.06%)`	⬆️
driver/config/config.go	`82.87% <100.00%> (+0.10%)`	⬆️
selfservice/strategy/code/strategy_recovery.go	`70.19% <100.00%> (ø)`
selfservice/strategy/link/strategy_recovery.go	`62.71% <100.00%> (ø)`
text/message_recovery.go	`100.00% <100.00%> (ø)`
text/message_verification.go	`100.00% <100.00%> (ø)`
cmd/courier/watch.go	`63.01% <0.00%> (-9.59%)`	⬇️
selfservice/strategy/link/strategy_verification.go	`58.59% <0.00%> (-7.92%)`	⬇️
selfservice/strategy/code/strategy_verification.go	`75.11% <0.00%> (-4.33%)`	⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

driver/config/config.go

…sableMailDispatch

zepatrik

Found a few minor issues to improve this

driver/config/config.go

selfservice/strategy/code/code_sender.go

selfservice/strategy/code/code_sender_test.go

selfservice/strategy/code/code_sender.go

selfservice/strategy/code/code_sender_test.go

selfservice/strategy/link/sender.go

selfservice/strategy/link/sender_test.go

…sableMailDispatch

aeneasr

Looking great! Should we add an e2e test covering this case? Or does it not make sense? I'm fine with both

text/message_recovery.go

jonas-jonas · 2023-02-08T19:16:07Z

Tests are failing, and I can't repro locally. Going to take another look tomorrow... 😓

aeneasr · 2023-02-08T21:35:36Z

Maybe the feature is enabled on accident for that e2e test, it looks like the test is expecting an email but not receiving one

jonas-jonas · 2023-02-09T07:49:35Z

Maybe the feature is enabled on accident for that e2e test, it looks like the test is expecting an email but not receiving one

The config schema didn't match the config for the key. I fixed that.

Just to double-check, the config schema is manually written, right? And I assume, there is no practical way to generate it?

aeneasr · 2023-02-09T08:11:25Z

Just to double-check, the config schema is manually written, right? And I assume, there is no practical way to generate it?

Correct and correct

jonas-jonas · 2023-02-09T09:07:32Z

Should we add an e2e test covering this case?

I thought about this a bit, and IMO it doesn't make sense to test for an email not arriving. As in theory, we'd need to wait an indefinite amount of time to be sure that no email arrives. This would slow down the E2e test pipeline.

I have updated the existing tests that test for the email arriving to enable this feature.

We already test this config in the go-tests, by checking the DB, which is good enough, IMO.

jonas-jonas · 2023-02-09T09:18:08Z

test/e2e/cypress/integration/profiles/mfa/totp.spec.ts

@@ -292,7 +292,7 @@ context("2FA TOTP", () => {

      // The React app keeps using the same flow. The following scenario used to be broken,
      // because the internal context wasn't populated properly in the flow after settings were saved.
-      it.only("should allow changing other settings and then setting up totp", () => {
+      it("should allow changing other settings and then setting up totp", () => {


Looks like this was forgotten a few weeks ago. These tests are still passing, though. So crisis averted.

Would be great to have an ESLint rule or some mechanism to prevent .onlys ending up on master. But AFAICT, there is nothing of that sort for cypress :(

Yes please! :)

jonas-jonas · 2023-02-09T09:19:43Z

test/e2e/cypress/integration/profiles/verification/verify/errors.spec.ts

@@ -138,12 +140,15 @@ context("Account Verification Error", () => {
            })
          })

-          it("unable to verify non-existent account", async () => {
-            cy.get('input[name="email"]').type(gen.identity().email)
+          it("unable to verify non-existent account", () => {


async prevented this test from being executed properly. So it never actually tested what it was supposed to.

I checked, and this seemed to be the only instance of an async test body.

aeneasr

LGTM, as discussed on Slack, one test change

…sableMailDispatch

jonas-jonas · 2023-02-13T11:17:18Z

LGTM, as discussed on Slack, one test change

Added the test to the appropriate places.

mszekiel and others added 4 commits October 12, 2022 15:04

feat: disable email dispatch for invalid recovery

b4ef1dd

fix: formatting

901a7f9

Merge remote-tracking branch 'origin/master' into pr/mszekiel/2585

93f4c54

chore: disable mail dispatch for all flows

4707472

chore: u

803e193

jonas-jonas requested a review from mszekiel February 6, 2023 15:01

jonas-jonas commented Feb 6, 2023

View reviewed changes

driver/config/config.go Outdated Show resolved Hide resolved

chore: update message

161fc74

jonas-jonas self-assigned this Feb 6, 2023

mszekiel mentioned this pull request Feb 6, 2023

feat: disable mail dispatch for invalid recovery email #2585

Closed

7 tasks

jonas-jonas added 2 commits February 7, 2023 22:38

Merge remote-tracking branch 'origin/master' into jonas-jonas/feat/di…

a6c648c

…sableMailDispatch

chore: rename config key

5537667

jonas-jonas marked this pull request as ready for review February 7, 2023 21:39

jonas-jonas requested review from aeneasr and zepatrik as code owners February 7, 2023 21:39

zepatrik reviewed Feb 8, 2023

View reviewed changes

jonas-jonas and others added 3 commits February 8, 2023 10:44

Merge remote-tracking branch 'origin/master' into jonas-jonas/feat/di…

f8e9806

…sableMailDispatch

chore: cleanup

c0ca43c

chore: use test context

641c4c1

aeneasr reviewed Feb 8, 2023

View reviewed changes

text/message_recovery.go Outdated Show resolved Hide resolved

jonas-jonas added 2 commits February 8, 2023 14:35

chore: tests

6c4f4b7

chore: messages

aab70a9

jonas-jonas changed the title ~~feat: disable mail dispatch for "invalid" recovery/verification emails~~ feat(changelog): make notification to unknown recipients configurable Feb 8, 2023

jonas-jonas changed the title ~~feat(changelog): make notification to unknown recipients configurable~~ feat: make notification to unknown recipients configurable Feb 9, 2023

chore: fix e2e tests

b532a67

jonas-jonas requested a review from aeneasr February 9, 2023 09:12

jonas-jonas commented Feb 9, 2023

View reviewed changes

Merge branch 'master' into jonas-jonas/feat/disableMailDispatch

a5e846a

aeneasr previously approved these changes Feb 13, 2023

View reviewed changes

jonas-jonas added 2 commits February 13, 2023 10:33

Merge remote-tracking branch 'origin/master' into jonas-jonas/feat/di…

1106f70

…sableMailDispatch

test: add e2e test

501912e

jonas-jonas dismissed aeneasr’s stale review via 501912e February 13, 2023 11:11

jonas-jonas added 2 commits February 13, 2023 13:47

chore: fix test

a073126

chore: fix test ordering

e9985e3

aeneasr approved these changes Feb 14, 2023

View reviewed changes

aeneasr merged commit 1a5ead4 into master Feb 14, 2023

aeneasr deleted the jonas-jonas/feat/disableMailDispatch branch February 14, 2023 08:49

uvinw mentioned this pull request Apr 11, 2023

"Account access attempted"emails sent by default and "notify_unknown_recipients" is ignored #3226

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make notification to unknown recipients configurable #3075

feat: make notification to unknown recipients configurable #3075

jonas-jonas commented Feb 6, 2023 •

edited

codecov bot commented Feb 6, 2023 •

edited

zepatrik left a comment

aeneasr left a comment

jonas-jonas commented Feb 8, 2023

aeneasr commented Feb 8, 2023

jonas-jonas commented Feb 9, 2023

aeneasr commented Feb 9, 2023

jonas-jonas commented Feb 9, 2023

jonas-jonas Feb 9, 2023

aeneasr Feb 13, 2023

jonas-jonas Feb 9, 2023

aeneasr left a comment

jonas-jonas commented Feb 13, 2023

feat: make notification to unknown recipients configurable #3075

feat: make notification to unknown recipients configurable #3075

Conversation

jonas-jonas commented Feb 6, 2023 • edited

Related issue(s)

Checklist

Further Comments

codecov bot commented Feb 6, 2023 • edited

Codecov Report

zepatrik left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

jonas-jonas commented Feb 8, 2023

aeneasr commented Feb 8, 2023

jonas-jonas commented Feb 9, 2023

aeneasr commented Feb 9, 2023

jonas-jonas commented Feb 9, 2023

jonas-jonas Feb 9, 2023

Choose a reason for hiding this comment

aeneasr Feb 13, 2023

Choose a reason for hiding this comment

jonas-jonas Feb 9, 2023

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

jonas-jonas commented Feb 13, 2023

jonas-jonas commented Feb 6, 2023 •

edited

codecov bot commented Feb 6, 2023 •

edited