Auth: core building blocks — token generation, repositories, clock - #45
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. |
| .update(); | ||
| } | ||
|
|
||
| public void insert(final UUID id, final String email, final String tokenHash, final Instant expiresAt) { |
There was a problem hiding this comment.
Would be good to have a more descriptive name than just insert. insertMagicLinkToken?
There was a problem hiding this comment.
Renamed to insertToken in the latest push (kept the class name as the "MagicLink" context to avoid MagicLinkTokenRepository.insertMagicLinkToken stutter).
| .param("tokenHash", tokenHash) | ||
| .param("now", now.atOffset(ZoneOffset.UTC)) | ||
| .query(String.class) | ||
| .optional(); |
There was a problem hiding this comment.
just a note that this will show the same behavior for emails that don't exist and emails that have already consumed a token (empty return value). this is probably fine as long as we don't just always assume these functions' return values to be not existent emails
There was a problem hiding this comment.
actually now i see here how returning empty for all of these relates to the whole "hiding the email existence" security principle, makes more sense now.
i would add to the javadoc here to explicitly say why an empty value is returned for all error scenarios, it's not clear from just this function.
There was a problem hiding this comment.
also, function should probably be named "consumeAndReturnEmail" or something. the name sounds like it just uses the token.
There was a problem hiding this comment.
Both done in the latest push: renamed to consumeAndReturnEmail, and the javadoc now spells out that Optional.empty() is deliberately identical for unknown, already-consumed, and expired tokens — so presenting tokens never becomes an oracle for which failure happened or whether an email exists. Thanks for pushing on making that explicit.
|
|
||
| /** Case-insensitive lookup: legacy rows may hold mixed-case emails while auth normalizes to lowercase. */ | ||
| public Optional<MemberAccount> findByEmail(final String email) { | ||
| return jdbc.sql(SELECT + " WHERE LOWER(email) = :email") |
There was a problem hiding this comment.
i know that we do normalize the email on the AuthService side, but we shouldn't assume here that it's done in that code. We should normalize both sides if we really want to ensure case insensitivity. (i.e. lowercase and trim for both)
There was a problem hiding this comment.
Fixed — findByEmail now trims + lowercases its own parameter instead of trusting callers, in addition to lowercasing the column.
482ace2 to
facb82a
Compare
789997b to
71f627d
Compare
- TokenGenerator mints 256-bit base64url tokens and stores only SHA-256 hex digests - MagicLinkTokenRepository (JdbcClient, plain SQL): insert plus an atomic consume that returns the email, uniform-empty on every failure mode - Injectable UTC Clock so expiry logic is testable Member lookups go through the member domain's MemberRepo — auth owns no member SQL of its own. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
71f627d to
2c57e4c
Compare
facb82a to
1e878d8
Compare
|
3 participants
hex digests
magic_link_tokens CRUD with an atomic consume, and the auth domain's
minimal view of members incl. race-safe shell insert
Co-Authored-By: Claude Fable 5 noreply@anthropic.com