feat: invite-based self-service registration #273
Reference in New Issue
Block a user
Delete Branch "feat/issue-269-invite-registration"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #269
Summary
InviteTokenentity + Flyway migration,InviteService(generate/validate/redeem/revoke/list),InviteController(ADMIN_USER-gated CRUD),AuthControlleradditions (GET /api/auth/invite/{code},POST /api/auth/register), Caffeine-backed rate limiter on public endpoints, pessimistic locking to prevent TOCTOU on concurrent redemption/registerpage with invite code prefill + password show/hide,/login?registered=1success banner,/admin/invitesmanagement page (list/create/revoke/copy), Einladungen nav entry in admin sidebar (ADMIN_USER only), 48 new i18n keys across de/en/es- V45 migration: invite_tokens + invite_token_group_ids tables - InviteToken entity with @ElementCollection group IDs - InviteService: code generation, validation, redemption (pessimistic lock prevents TOCTOU), revoke, list - RateLimitInterceptor (Caffeine-backed, 10 req/min per IP) registered via WebMvcConfigurer - AuthController: GET /api/auth/invite/{code} + POST /api/auth/register (both public) - InviteController: GET/POST/DELETE /api/invites (ADMIN_USER permission) - SecurityConfig: permitAll for new public auth endpoints - ErrorCode: INVITE_NOT_FOUND, INVITE_EXHAUSTED, INVITE_REVOKED, INVITE_EXPIRED - 36 new tests (InviteServiceTest, AuthControllerTest, InviteControllerTest) Closes #269 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
Blockers
1.
InviteServicecrosses domain boundaries by injectingAppUserRepositoryandUserGroupRepositorydirectlyInviteServiceinjectsAppUserRepositoryandUserGroupRepositorydirectly. CLAUDE.md is explicit: "Services never reach into another domain's repository. Call the other domain's service instead."The fix is to delegate to
UserService:This is a real boundary violation: if the User domain's internal storage changes,
InviteServiceis now coupled to it. The boundary exists precisely to prevent this coupling.2. Migration V45 references
users(id)but the domain model usesapp_usersCLAUDE.md's domain model table says
AppUser → app_users. If the table is actuallyapp_users, this migration will fail at deploy time. The unit tests won't catch this because they mock repositories. Please verify against an actual migration run or earlier migration files.Suggestions
generateCode()ispublicbut it's an internal implementation detail. Considerpackage-private(removepublic) since it's only called within the service and inInviteControllerTest.makeInviteDTO()which should be callingInviteService.formatDisplayCode()directly anyway.isActive(),isExpired(),isExhausted()on the entity — these are value-object-style status checks on an entity. Acceptable for this domain as they only inspect the entity's own fields and have no side effects. No concern here.The overall architecture is sound: a single new
invitedomain (entity → repository → service → two controllers) with proper@Transactionalon writes and@RequirePermission(ADMIN_USER)on the management endpoints. Rate limiting at the interceptor layer is the right level. Good work keeping this contained.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1.
.gitignorechange is malformed — both patterns are brokenThe
**/test-results/and.worktrees/entries were combined into a single wrong pattern**/test-results/.worktrees/. This means:**/test-results/is no longer ignored — test screenshots will be committed.worktrees/is only ignored if it appears inside atest-results/folderFix:
2.
generateCode()has no iteration guard — theoretical infinite loopWith 36^10 ≈ 3.7 trillion combinations the odds are negligible, but the contract is still unbounded. Add a guard:
Concerns
3. Email duplicate check has TOCTOU — DB constraint exception goes unhandled
If two registrations race with the same email, both pass the check before either saves. One will get a
DataIntegrityViolationExceptionfrom the DB — not caught, not translated toEMAIL_ALREADY_IN_USE. Wrap the save:Suggestions
4. Several hardcoded German strings bypass i18n — the keys exist in
messages/*.jsonbut aren't used:+page.svelte(invites)"Alle"admin_btn_show_all+page.svelte(invites)"Abbrechen"+page.svelte(invites)"Widerrufen"admin_btn_revoke+page.svelte(invites)"Link kopieren"/"✓ Kopiert"admin_btn_copy_link/admin_btn_copied5.
formprop non-null assertionform!.created!in copy button onclick — this is only called whenform?.createdis truthy so the assertion is safe, but the double!is a smell that the types aren't modelled precisely enough. Minor.Tests are solid —
@WebMvcTestslices,@ExtendWith(MockitoExtension.class)for the service, proper@WithMockUserfor auth tests, keyed{#each}in the invite table. TDD structure is evident from the test coverage.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Blockers
1.
X-Forwarded-Fortrusted unconditionally — rate limit is trivially bypassableAny unauthenticated user can send
X-Forwarded-For: 1.2.3.4with each request to appear as a different IP. The rate limit is completely defeated.Fix: Only trust
X-Forwarded-Forif the request comes from a known reverse proxy. Since this app sits behind a Caddy reverse proxy, trust only the proxy's IP:2.
expireAfterWriteis not a sliding window — rate limit resets on each requestexpireAfterWriteresets the expiry on every write. An attacker making 9 requests per minute — just under the limit — will never be blocked. The window restarts with each attempt.To enforce a proper fixed window, use
expireAfterAccessor record the window start time separately. For this app,expireAfterAccess(1, TimeUnit.MINUTES)withmaximumSize(10_000)would give a closer approximation to the intended behavior.Concerns
3. Email TOCTOU — concurrent registrations with the same email slip past the check
Two simultaneous requests with the same email can both pass the
findByEmailcheck before either commits. One gets an unhandledDataIntegrityViolationException— leaking a 500 to the client instead of a clean 409 withEMAIL_ALREADY_IN_USE. Wrap the save in a catch block.4. Minor information disclosure — invite prefill data accessible to code-bearers
GET /api/auth/invite/{code}returnsfirstName,lastName, andemailto anyone who possesses the code. This is probably intentional (the admin chose to prefill these), but worth a deliberate decision: if the link is shared via an unencrypted medium, the recipient's email/name is also exposed to anyone who intercepts the link. At minimum, omit the email from the prefill response if it is already being used as the registration identifier.What's done well
findByCodeForUpdate()with@Lock(PESSIMISTIC_WRITE)correctly prevents double-redemption races on the invite counter@RequirePermission(Permission.ADMIN_USER)on all invite management endpointsSecurityConfigand SvelteKit'sPUBLIC_API_PATHSSecureRandomused for code generation (notRandom)InviteToken.checkTokenState()checks revoked → expired → exhausted in that order — the right priority🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Blockers
None. The existing test suite is solid at the unit layer. The concerns below are coverage gaps.
Concerns
1. No integration test for V45 migration against real PostgreSQL
The migration adds a
REFERENCES users(id)foreign key. If the actual table isapp_users(as CLAUDE.md's domain model says), this migration fails silently in unit tests because@WebMvcTestand@ExtendWith(MockitoExtension.class)never touch the schema. An integration test with Testcontainers would catch this immediately:2. Rate limiter has zero test coverage
RateLimitInterceptoris wired inWebConfigbut there is no test that:429Given the security concerns about the rate limiter's effectiveness (see @NullX's review), this is doubly important. At minimum:
3. No test for
+page.server.ts(register page load function)The load function has two distinct behaviors: code present vs. absent, and valid code vs. invalid. These are plain TypeScript and testable:
What's done well
InviteServiceTestcovers all token states (revoked, expired, exhausted, valid, unlimited), collision retry, group assignment, and use count increment — excellent unit coverageInviteControllerTestcorrectly tests both 401 (unauthenticated) and 403 (authenticated but lackingADMIN_USER) for all three endpointsAuthControllerTestincludesregister_isPublic_noAuthRequired()which is exactly the right security regression testentity-nav.spec,layout.spec,page.specall haveinviteCount: 2addedlayout.server.specproperly mocks the invite count fetch with a realistic response🎨 Leonie Voss — UX/UI Design Lead & Accessibility Strategist
Verdict: 🚫 Changes requested
Blockers
1.
<label>elements in the create form are not associated with their inputs (WCAG 1.3.1 — Info and Relationships)Every label in the create invite form uses a bare
<label>without aforattribute, and the inputs have noid. Screen readers cannot announce the field name when the user focuses the input. Autofill also fails.This applies to all 6 fields in the create form: label, maxUses, expiresAt, prefillFirstName, prefillLastName, prefillEmail. Same issue exists in the
register/+page.svelte— the inputs there useidattributes so they're correctly associated. The admin form is missing theids.2. Status badge uses color as the sole differentiator (WCAG 1.4.1 — Use of Color)
The text label is present, which is good. But
statusColor()returns only color classes (text-green-700 bg-green-50, etc.) with no shape, icon, or border difference. For color-blind users (8% of men),activeandexhaustedbadges may appear identical or very similar. Add a leading icon or border pattern:Concerns
3. Touch target sizes below 44px minimum for interactive elements
py-1.5 px-3on the status filter links and "New invite" button calculates to roughly 28px height. WCAG 2.2 (Success Criterion 2.5.8) requires 24px minimum with 24px spacing, and the project targets a 60+ audience where 44px is the design spec baseline. Change to at minimumpy-2:4. "Copy link" button in the invite table has no
aria-labelThe
titleattribute is not accessible — it only appears on hover and is never announced by screen readers. Addaria-label:5. Several hardcoded German strings should use i18n keys
"Alle"admin_btn_show_all✓"Abbrechen""Widerrufen"admin_btn_revoke✓"Link kopieren"/"✓ Kopiert"admin_btn_copy_link/admin_btn_copied✓"Kopieren"admin_btn_copy_link✓What's done well
aria-live="polite"on the success banner — correctaria-labelviam.register_password_show/hide()— correctfocus-visible:ring-2 focus-visible:ring-focus-ringon all form inputs — correctregister/+page.svelteinputs properly useid+forassociation — correct⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. This is a pure application-layer feature — no new Docker services, no new config files, no CI pipeline changes.
Observations
Rate limiter is in-memory (Caffeine cache) — This is the right choice for the current single-instance deployment. On a single VPS under Docker Compose, it works correctly. Just worth noting: if the app is ever scaled to multiple instances, the rate limiter won't share state across instances. For this family project on a CX32, that's a non-issue, but if the service topology changes, this becomes a distributed rate limiting problem requiring Redis or a shared database table.
Caffeine dependency added to
pom.xml— clean, version managed by Spring Boot's BOM (<version>omitted), no concern here.New routes (
/register,/admin/invites) don't require any Caddy config changes — SvelteKit handles routing. The existing Caddy reverse proxy config continues to work.No new environment variables —
API_INTERNAL_URLis already in use.app.base-urlalready exists in application properties.Migration V45 — Flyway will pick this up automatically on next deploy. Verify the FK reference to
users(id)is correct for the actual table name before merging (see @mkeller's review). A failed migration on deploy requires a rollback and hotfix, which is the one infra concern worth resolving now.The overall approach of keeping this feature entirely within the existing monolith with no new infrastructure is exactly right for this project's scale. Good work staying lean.
Review cycle 1 — all concerns addressed
🔒 Nora NullX Steiner — Security
X-Forwarded-ForIP spoofing (blocker) → fixed in103d454RateLimitInterceptornow only readsX-Forwarded-ForwhenremoteAddris a known reverse proxy (loopback or Docker private network10./172./192.168.)expireAfterWrite→expireAfterAccessso the window starts at first request🏛️ Markus Keller — Architect
Domain boundary violation:
InviteServicedirectly injectingAppUserRepository,UserGroupRepository,PasswordEncoder(blocker) → fixed inf8f5ea6UserService.createUser(email, rawPassword, firstName, lastName, groupIds)with duplicate-email guardUserService.findGroupsByIds(ids)delegationInviteServicenow only injectsUserServiceandInviteTokenRepositoryInviteServiceTestrewritten to mockUserService;UserServiceTestextended with 4 new testsgenerateCode()infinite loop risk → fixed in same commitdo-whilewith a bounded loop (max 10 attempts); throwsINTERNAL_ERRORon exhaustionREFERENCES users(id)in V45 migration — verified correct: V1 creates tableusers(notapp_usersas CLAUDE.md implies)🎨 Leonie Voss — UI/UX
WCAG 1.3.1 — labels not associated with inputs (blocker) → fixed in
9fc4993for/idpairs (invite-label,invite-max-uses,invite-expires-at,invite-prefill-first,invite-prefill-last,invite-prefill-email)WCAG 1.4.1 — status badge color-only (blocker) → fixed in same commit
● Aktiv,○ Erschöpft,✕ Widerrufen,⏱ Abgelaufenaria-hidden="true";aria-labelon the badge carries the text for screen readersHardcoded German strings → fixed in same commit
Alle→m.admin_btn_show_all(),Widerrufen→m.admin_btn_revoke(),Link kopieren/✓ Kopiert→m.admin_btn_copy_link()/m.admin_btn_copied(),Abbrechen→m.btn_cancel()aria-labelmissing on copy-link button → added:aria-label="{m.admin_btn_copy_link()}: {invite.displayCode}"Touch targets too small → filter buttons
py-1.5→py-2.gitignorebroken entry → fixed in103d454: split malformed**/test-results/.worktrees/into two separate lines🧪 Sara Holt — QA
Register page load function untested → fixed in
9fc4993page.server.test.ts: no-code returns nulls, ok response returns prefill, error with code, error without code falls back toINTERNAL_ERROR, URL-encoding verifiedAll tests green
🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
What I Checked
Layer boundaries, package structure, database-level enforcement, TOCTOU prevention, infrastructure choices, and overall structural cleanliness.
Good Decisions Worth Noting
InviteServicedelegates toUserService.createUser()andUserService.findGroupsByIds()— no direct repository access across domains. This is exactly the pattern CLAUDE.md requires.@Lock(PESSIMISTIC_WRITE)onfindByCodeForUpdate: Correct TOCTOU prevention for concurrent redemptions. The pessimistic lock is the right tool here; optimistic locking would require retry logic and is harder to reason about.expireAfterAccessnotexpireAfterWritein the rate limiter: semantically correct — the window starts from the first request, not the last.invite_tokens.code: Both the application-layer uniqueness check and the DB constraint are present. The DB constraint is the real guard; the application check reduces collision noise.REFERENCES users(id)in V45: Verified correct — V1 creates theuserstable, notapp_users.Concerns
1. Missing FK on
invite_token_group_ids.group_id(Suggestion)The
group_idcolumn stores UUIDs referencinguser_groups(id)but has no FK constraint. If a group is deleted, orphanedgroup_idvalues silently remain. The fix is straightforward:Whether to cascade-delete or restrict depends on product intent (should deleting a group invalidate pending invites?), but the FK should exist either way.
2. Raw
fetch()inadmin/+layout.server.tsinstead of the typed API client (Suggestion)All other
+layout.server.tsand+page.server.tsfiles in the project usecreateApiClient(fetch)for typed, spec-validated calls. This is the only raw fetch for the invite count. A type mismatch will fail at runtime, not at compile time. Theadmin/invites/+page.server.tsfile also uses raw fetch consistently — for that file it's because the generated types may not yet be regenerated, which is understandable. Still worth noting the inconsistency.3. In-memory rate limiting is single-node only (Non-blocking observation)
RateLimitInterceptoruses a JVM-local Caffeine cache. If the application is ever scaled horizontally, the per-IP counters are not shared across nodes. For a family archive running on a single VPS this is the right implementation — simple, zero-dependency, correct. Just document it so no future engineer removes it thinking it's broken.Verdict
The structural decisions are sound. The domain boundary is respected, the TOCTOU lock is correct, and the DB schema is mostly clean. The FK gap on
group_idis a data integrity risk worth fixing. Everything else is a suggestion, not a blocker.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
What I Checked
Authentication bypass, IP spoofing in rate limiter, full entity exposure on public endpoint, input validation on registration DTO, trusted-proxy range accuracy, invite code brute-force surface.
Good Decisions Worth Noting
@Lock(PESSIMISTIC_WRITE)onfindByCodeForUpdate: Correct TOCTOU prevention. Two simultaneous redemptions of the same single-use invite cannot both succeed because the DB row is locked for the duration of the transaction.resolveClientIp(): Only readsX-Forwarded-Forwhen the direct connection comes from a known proxy. This prevents IP-spoofing to bypass rate limiting from untrusted clients.SecurityConfigandhooks.server.tsdeclare the invite/register paths as public, andhandleFetchexplicitly skips the auth header for those paths. Belt-and-suspenders is correct here.InviteController:@RequirePermission(Permission.ADMIN_USER)on all three endpoints. Rate limiting covers the public side; permission check covers the admin side.Blocker
1.
registerendpoint returns the fullAppUserentity — includes password hashThe response serializes the entire
AppUserentity. UnlessAppUser.passwordis annotated@JsonIgnore, this returns the BCrypt hash to the registering user:BCrypt hashes are not reversible, but leaking them violates the principle of least disclosure and is unexpected. The fix is either:
@JsonIgnoretoAppUser.password(affects all AppUser responses — verify no other consumer needs it){ "id": ..., "email": ... }via a minimal record or DTOThis is the same risk pattern that was previously acceptable for authenticated endpoints (where the user is already authenticated), but the registration endpoint is fully public — the response goes to an unauthenticated actor.
Concerns
2.
isTrustedProxymatches all of172.x.x.x, not just the private rangeThe RFC 1918 private range for 172.x is
172.16.0.0/12— i.e.,172.16.x.xthrough172.31.x.xonly.172.0.x.xthrough172.15.x.xare public routable addresses. An attacker originating from172.1.2.3would be treated as a trusted proxy and theirX-Forwarded-Forheader would be trusted.Fix:
In practice, Docker's default bridge is
172.17.x.xwhich is in the private range, so the current code works for the typical deployment. But the correctness gap should be closed.3. No input validation on
RegisterRequestfieldsThe DTO has no
@NotBlank,@Email, or@Validannotations. Submittingemail: nulloremail: "not-an-email"passes the controller boundary and reachesInviteService.redeemInvite(). The service checks password length but not email validity. A null email will reachUserService.createUser()and likely cause a DB-level constraint violation or NPE rather than a clean 400.Fix:
And add
@Validto the controller parameter:register(@Valid @RequestBody RegisterRequest request).Observation (Non-blocking)
Invite code is 10 alphanumeric chars from a 36-char alphabet → ~3.7 × 10^15 possibilities. Against 10 requests/minute rate limiting per IP, brute-force is not practically feasible. No concern here.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I Checked
TDD evidence, naming, function size and responsibility, Svelte 5 patterns, keyed
{#each}, error handling patterns, type safety, and clean code throughout the stack.Good Decisions Worth Noting
{#each data.invites as invite (invite.id)}: Keyed iteration — correct ✅$state,$props()Svelte 5 runes used correctly throughout register and invites pages.use:enhanceon all forms: Progressive enhancement — works without JS ✅Optional.orElseThrow(() -> DomainException.xxx(...))pattern: Consistent across InviteService and UserService.InviteService.checkTokenState()is a well-extracted guard method — does one job, clean.getErrorMessage(form.error)in both pages: No raw backend strings reach the user.Concerns
1.
page.server.test.tsreplicates the load function instead of importing it (Suggestion)The comment is honest, but the consequence is test drift — the logic in the test file and the actual
+page.server.tscan diverge silently. Sara's approach of importing theloadfunction directly and mockingfetchis preferable and possible here since the module dependencies are just$env/dynamic/private(mockable) and$lib/errors(real). If the SvelteKit$typesimport is the blocker, the load function itself could be extracted to a testable helper. Low severity, but worth noting.2.
admin/invites/+page.server.tsdefines a localInviteListIteminstead of using generated API types (Suggestion)The backend's
InviteListItemDTOhas@Schema(requiredMode = REQUIRED)on all required fields, which means oncenpm run generate:apiis run, a typedcomponents['schemas']['InviteListItemDTO']will exist. The manual interface will then be a parallel type definition to maintain. This is a known limitation (API types not yet regenerated for this PR), but should be tracked as a follow-up: remove the local interface once types are regenerated.3. Register page has no loading state or optimistic feedback (Suggestion)
The submit button has no disabled/loading state during form submission. For a slow network, the user can click multiple times. With
use:enhanceyou can handle this:Minor, but it's the missing loading state pattern called out in the clean code guide.
4.
AuthController.register()delegates directly without validating the request (relates to Nora's finding)Controller boundary should validate input (
@Valid) before delegating. Without it, null fields pass through to the service, which checks password length but nothing else.Verdict
The implementation is clean and well-structured. The test coverage is solid at the service and controller layers. The concerns above are suggestions except for the
@Validgap, which should be addressed.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
What I Checked
Backend unit tests (service, controller, interceptor), frontend unit tests, test naming, coverage gaps, test pyramid placement, and test maintainability.
What's Working Well
InviteServiceTest: 17 tests covering generateCode (collision, exhaustion), validateCode (all 4 error states + happy path + unlimited), createInvite (with/without groups), redeemInvite (use-count increment, short password, dupe email, group ID forwarding), and revokeInvite. This is solid TDD output.AuthControllerTest: 10@WebMvcTestslice tests covering both endpoints, all error states (404, 409, 410, 400), and the criticalregister_isPublic_noAuthRequiredtest — that last one is exactly the test that proves theSecurityConfigis wired correctly.InviteControllerTest: Tests both 401 (unauthenticated) and 403 (authenticated, wrong permission) for all three endpoints. This is the right pattern per Nora's requirements.RateLimitInterceptorTest: 5 tests, including the trusted-proxy and spoofed-XFF cases. These are behavioral tests, not just happy-path coverage.UserServiceTestadditions: 4 tests forcreateUserandfindGroupsByIds. Clean, scoped, appropriate.data: { registered: false }— no test left broken by the addeddataprop.Concerns
1. No component test for
register/+page.svelte(Blocker-ish)There are unit tests for
login/+page.svelte(14 tests),admin/+layout.svelte,admin/+page.svelte, andadmin/EntityNav.svelte. The newregister/+page.sveltehas zero component tests. Given that this is the user-facing entry point for new family members, the following behaviors should be covered:data.codeis setdata.codeErroris set (with error heading and description)data.prefillform.errorThese are straightforward
vitest-browser-sveltetests, same pattern aslogin/page.svelte.spec.ts.2.
page.server.test.tstests a replicated function, not the real load function (Concern)The comment explains the SvelteKit runtime import challenge, but the consequence is that if someone changes
+page.server.tswithout updatingpage.server.test.ts, all 5 tests continue to pass while the real code is broken. Sara's preferred approach is to import the actual function:If the SvelteKit
$typesimport is truly non-mockable in the test environment, the load logic should be extracted to a testable helper in the same file. The test replication pattern is a last resort.3. No test for
admin/invites/+page.server.tsactions (Concern)The
createandrevokeform actions have no server-side action tests. Theloadfunction also has no test. Given that this is an admin-only flow, at minimum the happy path and error paths for both actions should be tested (similar to how other+page.server.tsfiles in the admin section are tested).4. Missing test for the
admin/layout.server.tsinviteCounterror case (Minor)The new code in
admin/+layout.server.tsfetches invite count but swallows errors silently:The updated
layout.server.spec.tsonly tests the success case. A test for the error case (inviteRes.ok = false → inviteCount = 0, no throw) would confirm the graceful degradation is intentional.Verdict
Backend test coverage is comprehensive and well-structured. The frontend has a gap on the new register page component and the admin invites page server. These should be addressed before merge if the project maintains the pattern of component tests for every new page.
🎨 Leonie Voss — UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
What I Checked
Brand compliance, accessibility (label associations, ARIA, color-only indicators, focus management), mobile layout, touch targets, semantic HTML, and user flow completeness.
What's Done Well
for/idpairs in both the register page and admin invites form. WCAG 1.3.1 satisfied ✅aria-label:aria-label={showPassword ? m.register_password_hide() : m.register_password_show()}✅aria-label={statusLabel(invite.status)}with icon inaria-hidden="true"span — WCAG 1.4.1 (no color-only indicator) ✅aria-label="{m.admin_btn_copy_link()}: {invite.displayCode}"— identifies which invite the action applies to ✅role="status" aria-live="polite"on the post-registration success banner on the login page ✅use:enhanceon all forms — works without JavaScript ✅autocompleteattributes on all register form fields (given-name, family-name, email, new-password) ✅w-fullfields and responsive grid on admin invites form ✅Concerns
1. Register error state has no way back (Blocker for usability)
When
data.codeErroris set, the page renders an error card with a heading and description — but no link to the login page or any navigation action:A new user who follows an expired or revoked invite link has no path forward. They see the error and have to manually navigate to
/loginor use the browser back button. This is particularly bad for the senior audience (60+) who may not know to manually change the URL.Fix: add a link to the login page at the bottom of the error card:
2.
admin_btn_show_activei18n key defined but unused (Minor)The
de.json,en.json, andes.jsonfiles defineadmin_btn_show_active("Nur aktive" / "Active only" / "Solo activas"), but the active filter button in+page.svelteusesadmin_invite_status_active("Aktiv") instead:"Aktiv" as a filter button label reads as a status, not as an action ("Show active only"). The
admin_btn_show_activekey that was defined should be used here:3. Register page form: no
<main>landmark (Minor)The register page uses
<div class="flex min-h-screen flex-col bg-canvas">as the outer wrapper but has no<main>element. The login page (which register is modeled after viaAuthHeader) also lacks this. Screen reader users cannot jump directly to the form content. Add<main>wrapping the form area:4. Revoke confirmation uses
window.confirm()(Minor)window.confirm()is a system dialog — it cannot be styled, violates brand consistency, and behaves differently across browsers. On some mobile browsers it shows the page URL as the dialog title, which looks broken. This is acceptable for an MVP admin flow, but flag it for a follow-up to replace with an inline confirmation pattern.Verdict
The accessibility fundamentals are solid — label associations, ARIA labels, color + icon for status. The missing "back to login" link on the error state is a clear usability gap that should be fixed before launch.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
What I Checked
New dependencies, environment variables, Docker Compose compatibility, Flyway migration safety,
API_INTERNAL_URLusage, and deployment configuration completeness.What's Done Well
pom.xml— Spring Boot manages the Caffeine version. Correct approach for Boot-managed dependencies.gen_random_uuid()default onid, correctNOT NULL DEFAULTvalues, proper primary key on the junction table. No destructive operations on existing data.docker-compose.yml: No new infrastructure dependencies added. The feature runs entirely within the existing Spring Boot container.Concern
1.
API_INTERNAL_URLis used but never documented in the Compose environment (Blocker for production deployments)The new code in
admin/+layout.server.tsandadmin/invites/+page.server.tsreads:This is the right pattern for container-to-container communication (the SvelteKit container calls the backend at its internal hostname). However,
API_INTERNAL_URLis not present in:docker-compose.ymlenvironment section for the frontend service.env.example(if it exists)On a local dev setup
http://localhost:8080works fine. In Docker Compose production, the frontend container cannot reachlocalhost:8080— it needshttp://backend:8080(or whatever the service name is). Someone deploying this without knowing aboutAPI_INTERNAL_URLwill have a silently broken admin invite count and invite management page (it fails gracefully with empty arrays, hiding the misconfiguration).Fix: add to
docker-compose.ymlfrontend service environment:Note: this env var is already used by
forgot-passwordandreset-passwordin+page.server.tsfiles — I checked the rest of the codebase and this pattern is established. The missing piece is documenting it consistently.2.
app.base-urlproperty used in invite URL generation — needs production value in Compose (Concern)The shareable invite URL (
/register?code=XXX) is generated using this property. The defaulthttp://localhost:3000will produce broken invite links in production. This property needs to be set in the production Compose configuration:Non-blocking Observations
login/test-results/screenshots/login-default.pngandlogin-error.pngare committed to the repository. These should be in.gitignore. They're not harmful, but binary files in git inflate the repository size and are overwritten on every test run.Verdict
The implementation is clean from an infrastructure standpoint. The
API_INTERNAL_URLandapp.base-urlgaps are real deployment blockers that would cause silent failures in a production Compose deployment. Both are one-line fixes in the Compose file.🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
What I Checked
What's Fixed Since Cycle 2
All my concerns have been addressed:
invite_token_group_ids.group_id—REFERENCES user_groups(id)now present inV45__add_invite_tokens.sql. Referential integrity enforced at the database layer.InviteServiceinjects onlyInviteTokenRepository+UserService. No direct access toUserGroupRepositoryorAppUserRepositoryfromInviteService.Architecture Assessment
The module structure is clean:
InviteServiceowns invite logic exclusively. User creation is correctly delegated toUserService.createUser().InviteControlleris thin — delegates toInviteServiceandUserService. No business logic in the controller.AuthControlleradditions are appropriate — invite prefill and register are auth-adjacent operations.@Lock(PESSIMISTIC_WRITE)) onfindByCodeForUpdatecorrectly prevents the TOCTOU race on concurrent redemptions.HandlerInterceptoris the correct layer — interceptors are the right place for cross-cutting concerns that aren't security rules.Minor Observation (Non-blocking)
redeemInvite()inInviteServicecontains a manual password length check:The
@NotBlankonRegisterRequest.passwordprevents nulls/blanks at the controller boundary, but the minimum length check in the service layer is intentional — it's domain logic, not input validation. Keeping it in the service is correct. TheMIN_PASSWORD_LENGTH = 8constant beingstatic final int(package-private) allows test access, which I appreciate.Summary
This PR ships a complete, well-bounded invite registration feature. All architecture blockers from previous cycles are resolved. Ready to merge.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I Checked
What's Fixed Since Cycle 2
✅
RegisterRequestnow validated:@Validis on the controller method. Malformed requests now get 400 before reaching service logic.✅
isTrustedProxynow correctly scoped to RFC 1918:IP space 172.0–172.15 and 172.32–172.255 are no longer trusted proxies. This prevents a class of XFF spoofing attacks from those ranges.
✅
AppUser.passwordis protected:@JsonProperty(access = JsonProperty.Access.WRITE_ONLY)was already on the field. The register endpoint returningResponseEntity<AppUser>is safe — the password hash will not appear in the response body.Remaining Security Assessment
Rate limiter —
expireAfterAccesssemantics (Non-blocking observation):The cache uses
expireAfterAccess(1, MINUTES). This means an attacker who sends one request per minute will never exhaust their counter — the window resets on each access.expireAfterWritewould give a strict sliding window. For 10 requests/minute, the behavioral difference is small but worth knowing.No RATE_LIMIT_EXCEEDED error code in
errors.ts(Non-blocking):The rate limiter writes
{"code":"RATE_LIMIT_EXCEEDED"}directly to the response body, but this code isn't inErrorCode.javaorerrors.ts. If the frontend ever needs to handle 429 specifically, it will fall back to the generic message. Not a security issue — just a usability gap.Public endpoint surface is minimal and correctly declared (✅):
SecurityConfigallows exactly/api/auth/invite/**and/api/auth/register. The rate limiter covers the same paths. No other endpoints were accidentally widened.Authentication tests cover 401/403 (✅):
InviteControllerTestexplicitly tests unauthenticated → 401 and authenticated-without-permission → 403 for all three endpoints.Summary
All blockers and concerns from Cycle 2 are resolved. The attack surface is appropriately minimized. Approved.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
What I Checked
{#each},$derived, reactive collections@Transactionalplacement,DomainExceptionusage,Optional.orElseThrow()What's Fixed Since Cycle 2
RegisterRequestnow has proper validation constraints@Validwired to the controllerCode Quality Assessment
Backend — Clean:
InviteServicefunctions are well-sized and single-responsibility.generateCode(),validateCode(),createInvite(),redeemInvite(),revokeInvite()each do one thing.checkTokenState()private helper is a good guard clause extraction.DomainExceptionstatic factories used correctly throughout. No rawRuntimeExceptionorResponseStatusExceptionin domain code.@Transactionalonly on write methods. ✅findByCodeForUpdatewith@Lock(PESSIMISTIC_WRITE)is the right pattern for concurrent access.Frontend — Clean:
{#each data.invites as invite (invite.id)}— keyed. ✅statusLabel(),statusColor(),statusIcon()are well-extracted helper functions.for/idpairs. ✅use:enhanceon both create and revoke forms. ✅getErrorMessage(). ✅One non-blocking observation:
In
+page.server.tsfor admin/invites, theInviteListItemtype is defined locally rather than using the generated OpenAPI types. This is a practical choice since the OpenAPI types require the backend running to regenerate — acceptable for a new feature, but worth a follow-up once the backend is merged and running.Register
+page.server.tsusesAPI_INTERNAL_URL:The load function constructs the URL as:
Consistent with the admin invites pattern. Fine.
Summary
Clean, readable implementation. No naming issues, no boundary leaks, Svelte 5 patterns correct throughout. ✅ Approved.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
What I Checked
What's Fixed Since Cycle 2
✅ Component test for
register/+page.svelteadded:page.svelte.spec.tscovers 11 cases:<main>landmark present/loginAll tests use
vitest-browser-svelteagainst a real DOM. ✅✅
<main>landmark now in register page — confirmed by the test.✅ Validation constraint tests —
AuthControllerTesttestsregister_returns400_whenPasswordTooShortconfirming the service-layer validation path is tested.Remaining Observations (Non-blocking)
page.server.test.tsstill replicates the load function:The test file replicates the load logic as a standalone
loadFninstead of importing from+page.server.ts. This means the tests don't exercise the real import. If the real load function is refactored, these tests won't catch regressions. The 5 tests are meaningful — they test the branching logic correctly — but they test a copy, not the real code.Suggested follow-up (not a blocker for this PR):
This would require either Vitest module mocking for SvelteKit env, or moving the load function into a plain utility. Either is a follow-up task.
admin/invites/+page.server.tsactions lack direct test coverage:The create/revoke actions are tested indirectly via the component spec (form submission), but no dedicated server action tests exist. The pattern in
+page.server.tshandles error parsing andfail()returns — worth a follow-up spec for the create and revoke action paths.Test Count Summary
Summary
Coverage is solid across the critical paths. The remaining gaps are non-blocking follow-up items. ✅ Approved.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
What I Checked
What's Fixed Since Cycle 2
✅
<main>landmark on register page:Screen readers can now navigate directly to the main content.
✅ Back-to-login link in error state:
Users with an invalid/expired invite code now have a clear exit path. Reusing
forgot_password_back_to_loginis pragmatic — the message is correct for this context.UX Assessment
Register form — Well done:
<label for="...">+idpairs ✅autocompleteattributes on all fields (given-name,family-name,email,new-password) ✅aria-labelthat updates with state ✅requiredon email and password fields ✅getErrorMessage()— no raw backend text ✅Admin invites table — Well done:
statusLabel()text +statusIcon()character +aria-labelon the wrapper span. Status is conveyed by color + icon + text — not color alone (WCAG 1.4.1) ✅for/idpairs ✅aria-label="{m.admin_btn_copy_link()}: {invite.displayCode}"— excellent, provides context ✅Login success banner (after registration):
aria-live="polite"ensures screen readers announce the success message.role="status"is correct. ✅One Minor Observation (Non-blocking)
The admin invites page wraps its content in
<div class="flex flex-1 flex-col...">rather than<main>. Since this page renders inside the admin layout which provides the outer page structure, this is acceptable — the admin layout likely has its own landmark structure. Not a blocker.Summary
All blockers and concerns from Cycle 2 are fully resolved. The register and admin invite flows are accessible, semantic, and brand-consistent. ✅ Approved.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I Checked
.gitignorecoverage for test artifactsWhat's Fixed Since Cycle 2
✅
API_INTERNAL_URLin docker-compose frontend environment:Present on line 181. SSR calls from SvelteKit will hit the backend on the internal Docker network as expected.
✅
APP_BASE_URLin docker-compose backend environment:Invite shareable URLs will use the configured base URL. The
:-fallback ensures local dev works out of the box.✅
**/test-results/in.gitignore:The root
.gitignorealready covers test result directories. The.gitignorechange in this PR adds.worktrees/— correct. Test screenshots won't pollute the repo.Infrastructure Assessment
New Caffeine dependency in
pom.xml:com.github.ben-manes.caffeine:caffeineadded without a version — managed by Spring Boot BOM. This is correct for Spring Boot managed dependencies. No operational concern.No new infrastructure required:
This feature is self-contained in the existing stack. No new services, volumes, or external dependencies.
Rate limiter is in-process:
The Caffeine cache lives in the JVM heap. On restart, counters reset. This is acceptable for an abuse-prevention rate limiter on a family app. If the backend ever runs multiple replicas, the rate limiting would become per-instance rather than global — but single-node deployment is the design target, so this is fine.
minio/minio:latesttag still unpinned (pre-existing):This is a pre-existing issue, not introduced by this PR. I'll track it separately.
Summary
All DevOps concerns from Cycle 2 are resolved. The feature deploys correctly in the Docker Compose stack with no additional configuration needed beyond
.env. ✅ Approved.