Skip to content

Commit 5f18a39

Browse files
committed
Add review notes for Multiple Email Support MR
1 parent 76dd26e commit 5f18a39

File tree

1 file changed

+40
-0
lines changed

1 file changed

+40
-0
lines changed
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
# MR Review: Multiple Email Support
2+
3+
## Findings
4+
5+
### 1) Case-sensitive uniqueness check can create ambiguous identities (high)
6+
7+
`link-number-to-email` treats emails as case-sensitive when deciding whether an email is already in use (`event.email === command.email`).
8+
At login time, lookup normalises domains (`normaliseEmailAddress`) and then rejects if more than one member is returned.
9+
10+
Impact:
11+
- Two different member numbers can be linked to effectively the same mailbox by varying only domain case.
12+
- Login then fails with "Multiple members associated..." and no login email is sent.
13+
- This is both an availability risk and account-management footgun.
14+
15+
Recommendation:
16+
- Compare normalised addresses in `link-number-to-email` for duplicate/uniqueness checks.
17+
- Add a regression test for linking `user@example.com` and `user@EXAMPLE.COM` on different member numbers.
18+
19+
### 2) Missing test coverage for the new ambiguous-member branch (medium)
20+
21+
`sendLogInLink` has an explicit branch for `members.length > 1`, but current tests do not exercise that branch.
22+
23+
Impact:
24+
- Important behavior (error handling and logging around ambiguous accounts) can regress silently.
25+
26+
Recommendation:
27+
- Add an authentication test that seeds two unlinked accounts resolving to the same normalised email and asserts:
28+
- `sendEmail` is not called,
29+
- a `Left` is returned with the expected message,
30+
- logging occurs (if practical).
31+
32+
### 3) PII in error logs for failed lookup path (low)
33+
34+
The ambiguous-user error logs the submitted email address directly.
35+
36+
Impact:
37+
- Increases PII exposure in logs and can make audit/compliance handling harder.
38+
39+
Recommendation:
40+
- Either redact/hash email before logging, or log only a stable fingerprint/correlation ID.

0 commit comments

Comments
 (0)