Auth: request-link and verify flows - #46
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Graphite Automations"Request reviewers once CI passes" took an action on this PR • (07/10/26)2 reviewers were added to this PR based on Henry Chen's automation. |
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.patinanetwork.patchats.email.EmailSender; |
There was a problem hiding this comment.
We should put this dependency behind a client and have a standardized way of calling it.
Otherwise spaghetti will ensue with dependencies.
| final String email = normalize(rawEmail); | ||
| if (!rateLimiter.tryAcquire(email, clientIp)) { | ||
| log.warn("Rate-limited magic-link request for {} from {}", email, clientIp); | ||
| return; |
There was a problem hiding this comment.
#49 (comment) related comment on your docs PR that I think could be a better approach here.
e49fcf1 to
789997b
Compare
9ebd548 to
2a9e88a
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| public boolean tryAcquire(final String email, final String clientIp) { | ||
| final boolean emailAllowed = emailBuckets.getUnchecked(email).tryConsume(1); | ||
| final boolean ipAllowed = ipBuckets.getUnchecked(clientIp).tryConsume(1); | ||
| return emailAllowed && ipAllowed; | ||
| } |
There was a problem hiding this comment.
Critical rate limiting bug: When emailAllowed is true but ipAllowed is false, the email bucket has already consumed a token but the method returns false (request denied). This means the email loses a token from their budget even though the request was blocked by IP rate limiting.
Example: Email has 3 tokens left, IP has 0 tokens. Call tryAcquire() → email consumes 1 (now 2 left) → IP fails → returns false → email unfairly lost a token.
Fix by checking both buckets before consuming:
public boolean tryAcquire(final String email, final String clientIp) {
final Bucket emailBucket = emailBuckets.getUnchecked(email);
final Bucket ipBucket = ipBuckets.getUnchecked(clientIp);
if (emailBucket.tryConsume(1)) {
if (ipBucket.tryConsume(1)) {
return true;
}
// Rollback: refill the email bucket since IP failed
emailBucket.addTokens(1);
}
return false;
}| public boolean tryAcquire(final String email, final String clientIp) { | |
| final boolean emailAllowed = emailBuckets.getUnchecked(email).tryConsume(1); | |
| final boolean ipAllowed = ipBuckets.getUnchecked(clientIp).tryConsume(1); | |
| return emailAllowed && ipAllowed; | |
| } | |
| public boolean tryAcquire(final String email, final String clientIp) { | |
| final Bucket emailBucket = emailBuckets.getUnchecked(email); | |
| final Bucket ipBucket = ipBuckets.getUnchecked(clientIp); | |
| if (emailBucket.tryConsume(1)) { | |
| if (ipBucket.tryConsume(1)) { | |
| return true; | |
| } | |
| // Rollback: refill the email bucket since IP failed | |
| emailBucket.addTokens(1); | |
| } | |
| return false; | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
- AuthService.requestLink: normalize email, Bucket4j rate limits (3/email + 10/IP per 15 min) enforced silently, invalidate prior tokens, email a 15-minute single-use link via the EmailSender port (dev profile logs the link instead of sending) - AuthService.verify: atomic UPDATE..RETURNING consume, shell member creation on first login (ON CONFLICT DO NOTHING + re-select) - Generic response contract: request-link always answers the same way, so account existence and rate limiting are unobservable - InvalidMagicLinkException -> 400 failure envelope Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
789997b to
71f627d
Compare
2a9e88a to
04a095e
Compare
|
3 participants
(3/email + 10/IP per 15 min) enforced silently, invalidate prior
tokens, email a 15-minute single-use link via the EmailSender port
(dev profile logs the link instead of sending)
creation on first login (ON CONFLICT DO NOTHING + re-select)
so account existence and rate limiting are unobservable
Co-Authored-By: Claude Fable 5 noreply@anthropic.com