marcel/wannsee-kram
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

Build Admin login and sidebar layout with auth guard #8

New Issue
Open
opened 2026-05-05 10:57:43 +02:00 by marcel · 8 comments
marcel commented 2026-05-05 10:57:43 +02:00
Owner
Copy Link

Task 8 — Plan reference: docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md

User story (US-AUTH-003):
As an admin, I log in with my username and password and see the admin interface with sidebar navigation.

Acceptance criteria

  • GET /admin/* (except /admin/login) redirects to /admin/login if no admin_session cookie
  • Login form: username field + password field; POST action calls verifyAdminPassword, sets admin_session cookie on success
  • Wrong credentials → "Ungültige Zugangsdaten." (same message for wrong name and wrong password — no user enumeration)
  • Admin sidebar (dark, bg-admin-bg, 155px wide) shows 4 nav items: Inventar, Codes, Reservierungen, Übersicht
  • Active nav item: bg-white/10 text-white border-l-[3px] border-accent left border
  • Sidebar collapses to hamburger on mobile (≤767px) — md:flex pattern
  • "Abmelden" button at sidebar bottom posts to /admin/logout, clears cookie, redirects to /admin/login
  • Valid family_code cookie grants zero access to /admin/* (NFR-SEC-004)
  • No admin_session cookie grants zero access to the family gallery — separate auth systems

Files to create

  • src/routes/admin/login/+page.svelte
  • src/routes/admin/login/+page.server.ts
  • src/routes/admin/+layout.server.ts
  • src/routes/admin/+layout.svelte
  • src/routes/admin/logout/+page.server.ts

Depends on: #3 | Size: S | Spec: system-design §4 (Auth), reservierung-design §5.1, views spec View 04, NFR-SEC-004

## Task 8 — Plan reference: `docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md` **User story (US-AUTH-003):** As an admin, I log in with my username and password and see the admin interface with sidebar navigation. ### Acceptance criteria - [ ] `GET /admin/*` (except `/admin/login`) redirects to `/admin/login` if no `admin_session` cookie - [ ] Login form: username field + password field; POST action calls `verifyAdminPassword`, sets `admin_session` cookie on success - [ ] Wrong credentials → "Ungültige Zugangsdaten." (same message for wrong name and wrong password — no user enumeration) - [ ] Admin sidebar (dark, `bg-admin-bg`, 155px wide) shows 4 nav items: Inventar, Codes, Reservierungen, Übersicht - [ ] Active nav item: `bg-white/10 text-white border-l-[3px] border-accent` left border - [ ] Sidebar collapses to hamburger on mobile (≤767px) — `md:flex` pattern - [ ] "Abmelden" button at sidebar bottom posts to `/admin/logout`, clears cookie, redirects to `/admin/login` - [ ] Valid `family_code` cookie grants zero access to `/admin/*` (NFR-SEC-004) - [ ] No `admin_session` cookie grants zero access to the family gallery — separate auth systems ### Files to create - `src/routes/admin/login/+page.svelte` - `src/routes/admin/login/+page.server.ts` - `src/routes/admin/+layout.server.ts` - `src/routes/admin/+layout.svelte` - `src/routes/admin/logout/+page.server.ts` **Depends on:** #3 | **Size:** S | **Spec:** system-design §4 (Auth), reservierung-design §5.1, views spec View 04, NFR-SEC-004
marcel added this to the v1.0 — MVP milestone 2026-05-05 10:57:43 +02:00
marcel commented 2026-05-05 11:26:28 +02:00
Author
Owner
Copy Link

👤 Markus Keller — Application Architect

Observations

  • The issue correctly places the auth guard in +layout.server.ts and not in individual routes — this is the right enforcement point per the architecture.
  • The file list matches the system design spec exactly (hooks.server.ts remains the cookie-to-locals layer; the layout guard is the redirect layer).
  • The admin_session cookie design (store admin name, whitelist against ADMIN_NAMES derived from env vars) is documented in both the system design and the persona files. The issue implies this but doesn't spell out the implementation shape.
  • The sidebar width in the issue body (155px) differs from the views spec impl-ref table (w-44 = 176px). These need to be consistent before coding starts.
  • NFR-SEC-004 (family code grants zero access to /admin/*) is explicitly listed as an acceptance criterion — this is correctly tested at the +layout.server.ts guard, not duplicated per route.
  • The acceptance criterion "Valid admin_session cookie grants zero access to the family gallery" is the inverse check. This is enforced in galerie/+page.server.ts via locals.familyCode, which has nothing to do with locals.admin. These are truly independent — the architecture handles this cleanly without extra code in this issue.
  • The "Abmelden" logout flow (POST to /admin/logout, clear cookie, redirect) is clean; the logout route needs to clear only the admin_session cookie, not family_code.

Recommendations

  • Resolve the sidebar width discrepancy before implementation: the issue says 155px, the views spec says w-44 (176px). Pick one and update the other. The impl-ref table in the spec is the binding source of truth; update the issue body to 176px / w-44.
  • The +layout.server.ts guard must redirect to /admin/login, never return a 403 (Nora's rule: 403 reveals the route exists). Write this as the first line: if (!locals.admin) redirect(303, '/admin/login').
  • The +layout.svelte must use <slot /> (SvelteKit 2) not {@render children()} — confirm the SvelteKit version in package.json once the project is scaffolded. The plan uses Svelte 5 runes, where the snippet pattern applies: {@render children?.()}.
  • Keep sidebar active-state detection server-side via page.url.pathname (SvelteKit $page store), not a prop passed from each sub-route — this avoids per-page boilerplate.
  • The logout route only needs a POST action in +page.server.ts — no +page.svelte is needed if it immediately redirects.

Open Decisions (omit if none)

  • Sidebar width: 155px (issue) vs. 176px (views spec w-44). The implementation cannot split the difference — pick one. If the spec mockup is the source of truth (which it should be), the issue body needs correction to w-44 / 176px.
## 👤 Markus Keller — Application Architect ### Observations - The issue correctly places the auth guard in `+layout.server.ts` and not in individual routes — this is the right enforcement point per the architecture. - The file list matches the system design spec exactly (`hooks.server.ts` remains the cookie-to-locals layer; the layout guard is the redirect layer). - The `admin_session` cookie design (store admin name, whitelist against `ADMIN_NAMES` derived from env vars) is documented in both the system design and the persona files. The issue implies this but doesn't spell out the implementation shape. - The sidebar width in the issue body (`155px`) differs from the views spec impl-ref table (`w-44` = 176px). These need to be consistent before coding starts. - NFR-SEC-004 (family code grants zero access to `/admin/*`) is explicitly listed as an acceptance criterion — this is correctly tested at the `+layout.server.ts` guard, not duplicated per route. - The acceptance criterion "Valid `admin_session` cookie grants zero access to the family gallery" is the inverse check. This is enforced in `galerie/+page.server.ts` via `locals.familyCode`, which has nothing to do with `locals.admin`. These are truly independent — the architecture handles this cleanly without extra code in this issue. - The "Abmelden" logout flow (POST to `/admin/logout`, clear cookie, redirect) is clean; the logout route needs to clear only the `admin_session` cookie, not `family_code`. ### Recommendations - Resolve the sidebar width discrepancy **before** implementation: the issue says `155px`, the views spec says `w-44` (176px). Pick one and update the other. The impl-ref table in the spec is the binding source of truth; update the issue body to `176px` / `w-44`. - The `+layout.server.ts` guard must redirect to `/admin/login`, never return a 403 (Nora's rule: 403 reveals the route exists). Write this as the first line: `if (!locals.admin) redirect(303, '/admin/login')`. - The `+layout.svelte` must use `<slot />` (SvelteKit 2) not `{@render children()}` — confirm the SvelteKit version in `package.json` once the project is scaffolded. The plan uses Svelte 5 runes, where the snippet pattern applies: `{@render children?.()}`. - Keep sidebar active-state detection server-side via `page.url.pathname` (SvelteKit `$page` store), not a prop passed from each sub-route — this avoids per-page boilerplate. - The logout route only needs a `POST` action in `+page.server.ts` — no `+page.svelte` is needed if it immediately redirects. ### Open Decisions _(omit if none)_ - **Sidebar width: 155px (issue) vs. 176px (views spec `w-44`).** The implementation cannot split the difference — pick one. If the spec mockup is the source of truth (which it should be), the issue body needs correction to `w-44` / 176px.
marcel commented 2026-05-05 11:26:48 +02:00
Author
Owner
Copy Link

👤 Felix Brandt — Fullstack Developer

Observations

  • No source code exists yet in /home/marcel/Desktop/wannsee-kram/ — this is a greenfield implementation of the five listed files.
  • The five files to create are clearly scoped: +page.svelte (login form UI), +page.server.ts (bcrypt verify + cookie set), +layout.server.ts (auth guard), +layout.svelte (sidebar), logout/+page.server.ts (cookie clear + redirect). This is a well-bounded S-size task.
  • The verifyAdminPassword function is mentioned in the acceptance criteria but not yet in lib/auth.ts (which doesn't exist yet). It belongs there, not inline in +page.server.ts.
  • The issue specifies the German error string "Ungültige Zugangsdaten." — but the error display in the login +page.svelte must map form?.error to this string, not echo it raw.
  • The sidebar needs a $page store reference to mark the active link — this is a $derived or computed value in +layout.svelte.
  • The logout +page.server.ts has no UI — it just runs an action. It needs at minimum a default export action function (SvelteKit won't accept a file with only export const actions and no load if accessed via GET accidentally). A load that redirects to /admin/login covers the GET case gracefully.
  • The mobile hamburger behavior is specified as md:flex pattern — on mobile the sidebar is hidden and a hamburger toggle is needed. This implies $state(false) for sidebarOpen in +layout.svelte. This reactive state management must use Svelte 5 runes, not legacy $:.

Recommendations

  • Implement verifyAdminPassword(username: string, password: string): Promise<boolean> in lib/auth.ts before writing the login action. The action should call it, not re-implement bcrypt inline.
  • The login form action shape: 
    export const actions = {
  default: async ({ request, cookies }) => {
    const data = await request.formData();
    const username = String(data.get('username') ?? '');
    const password = String(data.get('password') ?? '');
    const ok = await verifyAdminPassword(username, password);
    if (!ok) return fail(401, { error: 'Ungültige Zugangsdaten.' });
    cookies.set('admin_session', username, {
      httpOnly: true, sameSite: 'strict', secure: !dev, path: '/', maxAge: 60 * 60 * 8
    });
    redirect(303, '/admin/inventar');
  }
};
  • In +page.svelte for login, bind the error display to the generic message directly from form.error, since the action already returns the German string: {#if form?.error}<p class="text-xs text-status-taken mt-2">{form.error}</p>{/if}.
  • Use {#each} with (link.href) key on the sidebar nav items.
  • The logout action must delete the cookie with the same options (path: '/') it was set with, otherwise the browser won't honour the deletion.
  • Write the failing tests first (TDD):
    1. Unit test: verifyAdminPassword returns false for unknown username, wrong password, empty string.
    2. Integration test: +layout.server.ts load redirects to /admin/login when locals.admin is null.
    3. E2E: admin login → sidebar visible → logout → redirected to login.
## 👤 Felix Brandt — Fullstack Developer ### Observations - No source code exists yet in `/home/marcel/Desktop/wannsee-kram/` — this is a greenfield implementation of the five listed files. - The five files to create are clearly scoped: `+page.svelte` (login form UI), `+page.server.ts` (bcrypt verify + cookie set), `+layout.server.ts` (auth guard), `+layout.svelte` (sidebar), `logout/+page.server.ts` (cookie clear + redirect). This is a well-bounded S-size task. - The `verifyAdminPassword` function is mentioned in the acceptance criteria but not yet in `lib/auth.ts` (which doesn't exist yet). It belongs there, not inline in `+page.server.ts`. - The issue specifies the German error string `"Ungültige Zugangsdaten."` — but the error display in the login `+page.svelte` must map `form?.error` to this string, not echo it raw. - The sidebar needs a `$page` store reference to mark the active link — this is a `$derived` or computed value in `+layout.svelte`. - The logout `+page.server.ts` has no UI — it just runs an action. It needs at minimum a default export action function (SvelteKit won't accept a file with only `export const actions` and no load if accessed via GET accidentally). A `load` that redirects to `/admin/login` covers the GET case gracefully. - The mobile hamburger behavior is specified as `md:flex` pattern — on mobile the sidebar is hidden and a hamburger toggle is needed. This implies `$state(false)` for `sidebarOpen` in `+layout.svelte`. This reactive state management must use Svelte 5 runes, not legacy `$:`. ### Recommendations - Implement `verifyAdminPassword(username: string, password: string): Promise<boolean>` in `lib/auth.ts` before writing the login action. The action should call it, not re-implement bcrypt inline. - The login form action shape: ```typescript export const actions = { default: async ({ request, cookies }) => { const data = await request.formData(); const username = String(data.get('username') ?? ''); const password = String(data.get('password') ?? ''); const ok = await verifyAdminPassword(username, password); if (!ok) return fail(401, { error: 'Ungültige Zugangsdaten.' }); cookies.set('admin_session', username, { httpOnly: true, sameSite: 'strict', secure: !dev, path: '/', maxAge: 60 * 60 * 8 }); redirect(303, '/admin/inventar'); } }; ``` - In `+page.svelte` for login, bind the error display to the generic message directly from `form.error`, since the action already returns the German string: `{#if form?.error}<p class="text-xs text-status-taken mt-2">{form.error}</p>{/if}`. - Use `{#each}` with `(link.href)` key on the sidebar nav items. - The logout action must delete the cookie with the same options (`path: '/'`) it was set with, otherwise the browser won't honour the deletion. - Write the failing tests first (TDD): 1. Unit test: `verifyAdminPassword` returns `false` for unknown username, wrong password, empty string. 2. Integration test: `+layout.server.ts` load redirects to `/admin/login` when `locals.admin` is null. 3. E2E: admin login → sidebar visible → logout → redirected to login.
marcel commented 2026-05-05 11:27:12 +02:00
Author
Owner
Copy Link

👤 Nora "NullX" Steiner — Security Engineer

Observations

  • User enumeration prevention (CWE-204): The acceptance criterion "Wrong credentials → 'Ungültige Zugangsdaten.' (same message for wrong name and wrong password)" is correctly specified. The implementation must NOT branch on unknown username before the bcrypt call. The correct pattern:

    const hash = process.env[`ADMIN_${username.toUpperCase()}_PASSWORD_HASH`];
// undefined hash: bcrypt.compare will be called with undefined — always returns false
if (!hash || !(await bcrypt.compare(password, hash))) {
  return fail(401, { error: 'Ungültige Zugangsdaten.' });
}

    The !hash || guard is acceptable here because both branches return the identical error. What is NOT acceptable is returning different error messages for each branch.

  • NFR-SEC-004 test surface: "Valid family_code cookie grants zero access to /admin/*" is enforced by the +layout.server.ts guard checking locals.admin, not locals.familyCode. Since hooks.server.ts populates these independently, the guard works correctly as long as it only checks locals.admin. No cross-contamination risk in this design.

  • Cookie security for admin_session: The acceptance criterion doesn't specify HttpOnly, SameSite, or Secure. These are required. The 8-hour maxAge from the system design doc is correct. The cookie must NOT store any sensitive value beyond the admin username, and it must be validated against the env-var whitelist on every request in hooks.server.ts — the cookie value alone must never be trusted without this check.

  • Redirect vs. 403 for unauthenticated admin access: The guard must use redirect(303, '/admin/login'), never error(403, ...). A 403 discloses that the route exists. This is correctly implied by the issue but should be explicit in the acceptance criteria.

  • Logout POST-only: The logout route must only respond to POST. A GET to /admin/logout must not clear the session (CSRF risk if logout were triggered by a <img src="/admin/logout">). SvelteKit Form Actions are POST-only by default — correct. The logout +page.server.ts should have a load that redirects away so a stray GET doesn't render a blank page.

  • admin_session cookie value scope: Storing the plaintext admin username in the cookie is acceptable for this project's threat model because it is validated against ADMIN_NAMES on every request in hooks.server.ts. The cookie cannot be forged to grant access as a non-whitelisted name.

Recommendations

  • Add an explicit acceptance criterion: GET /admin/logout redirects to /admin/login (does NOT clear the session cookie).
  • Add to the Definition of Done: admin session cookie is set with httpOnly: true, sameSite: 'strict', secure: !dev, maxAge: 60 * 60 * 8.
  • The hooks.server.ts implementation (in a later task, or Task 2 which must precede this one) must validate the admin_session cookie value against ADMIN_NAMES — not simply set locals.admin = cookieValue without checking.
  • Write this security test before the fix: 
    it('returns identical error text for unknown user and wrong password', async () => {
  const r1 = await submitLoginForm(page, 'nobody', 'anypassword');
  const r2 = await submitLoginForm(page, 'Marcel', 'wrongpassword');
  expect(r1.errorText).toBe(r2.errorText);
});

Open Decisions (omit if none)

  • Brute-force rate limiting on the admin login endpoint. The spec doesn't mention it. bcrypt WF12 (~250ms/attempt) is a speed brake but not a lockout. For a short-lived family app on a private server this is probably acceptable — but should be a conscious decision, not an oversight. Options: (a) accept the risk given the small attack surface and short project lifespan, (b) add a simple in-memory counter with a 15-minute lockout after 5 failed attempts. (Raised by: Nora Steiner)
## 👤 Nora "NullX" Steiner — Security Engineer ### Observations - **User enumeration prevention (CWE-204):** The acceptance criterion "Wrong credentials → 'Ungültige Zugangsdaten.' (same message for wrong name and wrong password)" is correctly specified. The implementation must NOT branch on unknown username before the bcrypt call. The correct pattern: ```typescript const hash = process.env[`ADMIN_${username.toUpperCase()}_PASSWORD_HASH`]; // undefined hash: bcrypt.compare will be called with undefined — always returns false if (!hash || !(await bcrypt.compare(password, hash))) { return fail(401, { error: 'Ungültige Zugangsdaten.' }); } ``` The `!hash ||` guard is acceptable here because both branches return the identical error. What is NOT acceptable is returning different error messages for each branch. - **NFR-SEC-004 test surface:** "Valid `family_code` cookie grants zero access to `/admin/*`" is enforced by the `+layout.server.ts` guard checking `locals.admin`, not `locals.familyCode`. Since `hooks.server.ts` populates these independently, the guard works correctly as long as it only checks `locals.admin`. No cross-contamination risk in this design. - **Cookie security for `admin_session`:** The acceptance criterion doesn't specify `HttpOnly`, `SameSite`, or `Secure`. These are required. The 8-hour `maxAge` from the system design doc is correct. The cookie must NOT store any sensitive value beyond the admin username, and it must be validated against the env-var whitelist on every request in `hooks.server.ts` — the cookie value alone must never be trusted without this check. - **Redirect vs. 403 for unauthenticated admin access:** The guard must use `redirect(303, '/admin/login')`, never `error(403, ...)`. A 403 discloses that the route exists. This is correctly implied by the issue but should be explicit in the acceptance criteria. - **Logout POST-only:** The logout route must only respond to POST. A GET to `/admin/logout` must not clear the session (CSRF risk if logout were triggered by a `<img src="/admin/logout">`). SvelteKit Form Actions are POST-only by default — correct. The logout `+page.server.ts` should have a `load` that redirects away so a stray GET doesn't render a blank page. - **`admin_session` cookie value scope:** Storing the plaintext admin username in the cookie is acceptable for this project's threat model because it is validated against `ADMIN_NAMES` on every request in `hooks.server.ts`. The cookie cannot be forged to grant access as a non-whitelisted name. ### Recommendations - Add an explicit acceptance criterion: `GET /admin/logout` redirects to `/admin/login` (does NOT clear the session cookie). - Add to the Definition of Done: admin session cookie is set with `httpOnly: true`, `sameSite: 'strict'`, `secure: !dev`, `maxAge: 60 * 60 * 8`. - The `hooks.server.ts` implementation (in a later task, or Task 2 which must precede this one) must validate the `admin_session` cookie value against `ADMIN_NAMES` — not simply set `locals.admin = cookieValue` without checking. - Write this security test before the fix: ```typescript it('returns identical error text for unknown user and wrong password', async () => { const r1 = await submitLoginForm(page, 'nobody', 'anypassword'); const r2 = await submitLoginForm(page, 'Marcel', 'wrongpassword'); expect(r1.errorText).toBe(r2.errorText); }); ``` ### Open Decisions _(omit if none)_ - **Brute-force rate limiting on the admin login endpoint.** The spec doesn't mention it. bcrypt WF12 (~250ms/attempt) is a speed brake but not a lockout. For a short-lived family app on a private server this is probably acceptable — but should be a conscious decision, not an oversight. Options: (a) accept the risk given the small attack surface and short project lifespan, (b) add a simple in-memory counter with a 15-minute lockout after 5 failed attempts. _(Raised by: Nora Steiner)_
marcel commented 2026-05-05 11:27:26 +02:00
Author
Owner
Copy Link

👤 Sara Holt — QA Engineer

Observations

  • The acceptance criteria cover 9 distinct behaviors. Each needs a dedicated test. Only 2 have an obvious unit/integration home; the rest require either E2E or component tests.
  • The "Valid family_code cookie grants zero access to /admin/*" criterion is a security boundary test — it must be in the permanent regression suite, not just in a smoke test.
  • The "Abmelden" button posts to /admin/logout — this needs an E2E test that verifies the cookie is actually cleared (not just that the redirect happens).
  • No test strategy is specified in the issue. For an S-size task with 5 new files, the test surface is:
    • Unit (Vitest): verifyAdminPassword (3 cases: unknown user, wrong password, correct credentials)
    • Integration (Vitest + :memory:): +layout.server.ts load function redirects when locals.admin is null
    • E2E (Playwright): Full admin login journey + logout; family_code cookie does not access /admin/inventar

Recommendations

  • Define and track these specific test cases in the issue before implementation begins:

    Unit — lib/auth.ts:

    it('verifyAdminPassword returns false for unknown username', ...)
it('verifyAdminPassword returns false for wrong password', ...)
it('verifyAdminPassword returns true for correct credentials', ...)

    Integration — +layout.server.ts:

    it('redirects to /admin/login when locals.admin is null', async () => {
  const event = makeTestEvent({ locals: { admin: null, familyCode: null } });
  await expect(load(event)).rejects.toMatchObject({ status: 303, location: '/admin/login' });
});

    E2E (Playwright):

    test('admin can log in and sees sidebar', async ({ page }) => { ... });
test('wrong credentials show Ungültige Zugangsdaten', async ({ page }) => { ... });
test('family code cookie does not grant /admin/inventar access', async ({ page }) => {
  // set family_code cookie, navigate to /admin/inventar, expect redirect to /admin/login
});
test('logout clears session and redirects to login', async ({ page }) => { ... });

  • The user enumeration test (same error text for unknown user and wrong password) belongs permanently in the regression suite. Add it to the Playwright E2E suite, not just as a one-time check.

  • Use Playwright page.context().cookies() to assert that admin_session is absent after logout — don't only check the URL.

  • The mobile hamburger behavior (sidebar collapse at ≤767px) needs a component or Playwright test at 375px viewport width.

Open Decisions (omit if none)

  • Test credentials for E2E. The E2E admin login tests need a real bcrypt hash in the test environment. Options: (a) inject a test-only env var ADMIN_TEST_PASSWORD_HASH in CI/Playwright config, (b) use a fixed known-hash for a test admin TestAdmin only in test runs. The production admin names (Marcel, Renate, Berit) must not appear in committed test files. (Raised by: Sara Holt)
## 👤 Sara Holt — QA Engineer ### Observations - The acceptance criteria cover 9 distinct behaviors. Each needs a dedicated test. Only 2 have an obvious unit/integration home; the rest require either E2E or component tests. - The "Valid `family_code` cookie grants zero access to `/admin/*`" criterion is a security boundary test — it must be in the permanent regression suite, not just in a smoke test. - The "Abmelden" button posts to `/admin/logout` — this needs an E2E test that verifies the cookie is actually cleared (not just that the redirect happens). - No test strategy is specified in the issue. For an S-size task with 5 new files, the test surface is: - **Unit (Vitest):** `verifyAdminPassword` (3 cases: unknown user, wrong password, correct credentials) - **Integration (Vitest + `:memory:`):** `+layout.server.ts` load function redirects when `locals.admin` is null - **E2E (Playwright):** Full admin login journey + logout; family_code cookie does not access `/admin/inventar` ### Recommendations - Define and track these specific test cases in the issue before implementation begins: **Unit — `lib/auth.ts`:** ```typescript it('verifyAdminPassword returns false for unknown username', ...) it('verifyAdminPassword returns false for wrong password', ...) it('verifyAdminPassword returns true for correct credentials', ...) ``` **Integration — `+layout.server.ts`:** ```typescript it('redirects to /admin/login when locals.admin is null', async () => { const event = makeTestEvent({ locals: { admin: null, familyCode: null } }); await expect(load(event)).rejects.toMatchObject({ status: 303, location: '/admin/login' }); }); ``` **E2E (Playwright):** ```typescript test('admin can log in and sees sidebar', async ({ page }) => { ... }); test('wrong credentials show Ungültige Zugangsdaten', async ({ page }) => { ... }); test('family code cookie does not grant /admin/inventar access', async ({ page }) => { // set family_code cookie, navigate to /admin/inventar, expect redirect to /admin/login }); test('logout clears session and redirects to login', async ({ page }) => { ... }); ``` - The user enumeration test (same error text for unknown user and wrong password) belongs permanently in the regression suite. Add it to the Playwright E2E suite, not just as a one-time check. - Use Playwright `page.context().cookies()` to assert that `admin_session` is absent after logout — don't only check the URL. - The mobile hamburger behavior (sidebar collapse at ≤767px) needs a component or Playwright test at 375px viewport width. ### Open Decisions _(omit if none)_ - **Test credentials for E2E.** The E2E admin login tests need a real bcrypt hash in the test environment. Options: (a) inject a test-only env var `ADMIN_TEST_PASSWORD_HASH` in CI/Playwright config, (b) use a fixed known-hash for a test admin `TestAdmin` only in test runs. The production admin names (Marcel, Renate, Berit) must not appear in committed test files. _(Raised by: Sara Holt)_
marcel commented 2026-05-05 11:27:50 +02:00
Author
Owner
Copy Link

👤 Leonie Voss — UI/UX Design Lead

Observations

  • View 04 (Admin Login) is fully specified in the views spec. The login card uses bg-admin background, surface-colored card with rounded-xl, p-[22px], max-w-xs. The "Anmelden" button is bg-admin hover:bg-[#1E2C24] — not the primary green. This distinction matters: the login form is in the admin dark world, not the family green world.
  • Sidebar width discrepancy: The issue says 155px wide but the views spec impl-ref table specifies w-44 (176px) with hidden md:flex. This is a measurable inconsistency that will create a visual difference from the approved mockup.
  • Active nav item spec: The issue says bg-white/10 text-white border-l-[3px] border-accent. The views spec says: flex items-center gap-2 px-3 py-1.5 text-[10px] font-semibold text-white bg-white/10 border-l-[3px] border-accent. These match — good. The inactive state is text-white/45 border-l-[3px] border-transparent hover:text-white/75 — the hover state and transparent border placeholder are both needed to prevent layout shift on hover.
  • Mobile sidebar: The issue says md:flex pattern (sidebar hidden below md:, hamburger visible). The views spec confirms: "Sidebar Mobil: Overlay-Drawer — Toggle via Svelte $state(false)" with a hamburger in a mobile top bar. The +layout.svelte needs both the desktop sidebar and a mobile top bar with hamburger — two distinct layout regions.
  • "Abmelden" button: Not shown in the views spec mockup. Position is specified as "sidebar bottom". It must use mt-auto to push to the bottom of the flex column. Style should match inactive sidebar links (no prominent color — this is a utility action, not navigation).
  • Form accessibility: The views spec impl-ref table states "Immer <label for="…"> verknüpfen". Both username and password inputs must have <label> elements with matching for/id pairs — not placeholder-only labeling.
  • Password input type: Not mentioned in the issue. Must be type="password" to prevent browser autocomplete exposing it and to trigger password manager UX.

Recommendations

  • Correct the sidebar width to w-44 (176px) in the issue body to match the views spec — this is the binding implementation reference.
  • The admin login page background is bg-admin-bg (token: #2A3B30). The tailwind config in the plan uses 'admin-bg': '#2A3B30' as the token name, so the class is bg-admin-bg. The views spec references bg-admin. Confirm the Tailwind config key matches before implementing — it should be admin (shorter alias) or admin-bg (descriptive). Pick one and be consistent across the login page and the sidebar.
  • The "Abmelden" button at the very bottom of the sidebar should be a <form method="POST" action="/admin/logout"> with a submit button — not a link, to ensure it's a POST action.
  • Add autocomplete="username" to the username input and autocomplete="current-password" to the password input so password managers work correctly.
  • The sidebar logo region ("Admin" kicker + "Erbstücke Wannsee" in Lora) must use the exact spec markup: <span> for the "Admin" kicker in small caps, then the serif title — these are two visually distinct elements.

Open Decisions (omit if none)

  • Tailwind token name for admin background: admin or admin-bg. The views spec HTML uses bg-admin as the CSS class; the Tailwind config in the plan file defines the key as 'admin-bg'. These produce different class names (bg-admin vs bg-admin-bg). One must be chosen before Task 1 (scaffold) creates the Tailwind config, or corrected before this task starts. (Raised by: Leonie Voss)
## 👤 Leonie Voss — UI/UX Design Lead ### Observations - **View 04 (Admin Login) is fully specified in the views spec.** The login card uses `bg-admin` background, surface-colored card with `rounded-xl`, `p-[22px]`, `max-w-xs`. The "Anmelden" button is `bg-admin hover:bg-[#1E2C24]` — not the primary green. This distinction matters: the login form is in the admin dark world, not the family green world. - **Sidebar width discrepancy:** The issue says `155px wide` but the views spec impl-ref table specifies `w-44` (176px) with `hidden md:flex`. This is a measurable inconsistency that will create a visual difference from the approved mockup. - **Active nav item spec:** The issue says `bg-white/10 text-white border-l-[3px] border-accent`. The views spec says: `flex items-center gap-2 px-3 py-1.5 text-[10px] font-semibold text-white bg-white/10 border-l-[3px] border-accent`. These match — good. The inactive state is `text-white/45 border-l-[3px] border-transparent hover:text-white/75` — the `hover` state and transparent border placeholder are both needed to prevent layout shift on hover. - **Mobile sidebar:** The issue says `md:flex` pattern (sidebar hidden below `md:`, hamburger visible). The views spec confirms: "Sidebar Mobil: Overlay-Drawer — Toggle via Svelte `$state(false)`" with a hamburger in a mobile top bar. The `+layout.svelte` needs both the desktop sidebar and a mobile top bar with hamburger — two distinct layout regions. - **"Abmelden" button:** Not shown in the views spec mockup. Position is specified as "sidebar bottom". It must use `mt-auto` to push to the bottom of the flex column. Style should match inactive sidebar links (no prominent color — this is a utility action, not navigation). - **Form accessibility:** The views spec impl-ref table states "Immer `<label for="…">` verknüpfen". Both username and password inputs must have `<label>` elements with matching `for`/`id` pairs — not placeholder-only labeling. - **Password input type:** Not mentioned in the issue. Must be `type="password"` to prevent browser autocomplete exposing it and to trigger password manager UX. ### Recommendations - Correct the sidebar width to `w-44` (176px) in the issue body to match the views spec — this is the binding implementation reference. - The admin login page background is `bg-admin-bg` (token: `#2A3B30`). The tailwind config in the plan uses `'admin-bg': '#2A3B30'` as the token name, so the class is `bg-admin-bg`. The views spec references `bg-admin`. Confirm the Tailwind config key matches before implementing — it should be `admin` (shorter alias) or `admin-bg` (descriptive). Pick one and be consistent across the login page and the sidebar. - The "Abmelden" button at the very bottom of the sidebar should be a `<form method="POST" action="/admin/logout">` with a submit button — not a link, to ensure it's a POST action. - Add `autocomplete="username"` to the username input and `autocomplete="current-password"` to the password input so password managers work correctly. - The sidebar logo region ("Admin" kicker + "Erbstücke Wannsee" in Lora) must use the exact spec markup: `<span>` for the "Admin" kicker in small caps, then the serif title — these are two visually distinct elements. ### Open Decisions _(omit if none)_ - **Tailwind token name for admin background: `admin` or `admin-bg`.** The views spec HTML uses `bg-admin` as the CSS class; the Tailwind config in the plan file defines the key as `'admin-bg'`. These produce different class names (`bg-admin` vs `bg-admin-bg`). One must be chosen before Task 1 (scaffold) creates the Tailwind config, or corrected before this task starts. _(Raised by: Leonie Voss)_
marcel commented 2026-05-05 11:28:06 +02:00
Author
Owner
Copy Link

👤 Elicit — Requirements Engineer

Observations

  • US-AUTH-003 is well-formed. The user story is clear and the acceptance criteria are testable. The issue references the spec correctly and identifies the dependency on #3.
  • Missing precondition: The issue depends on #3 but doesn't state what #3 provides. Based on the plan, #3 is likely hooks.server.ts (the cookie-to-locals layer). Without hooks.server.ts populating locals.admin, the +layout.server.ts guard has nothing to check. This dependency should be explicit in the issue: "Depends on: #3 (hooks.server.ts with locals.admin populated from admin_session cookie)."
  • Acceptance criterion gap — cookie attributes: The criterion "sets admin_session cookie on success" omits httpOnly, sameSite, maxAge. These are NFR-SEC attributes, not implementation details — they are testable and should be explicit: "sets admin_session cookie (httpOnly, SameSite=Strict, 8h TTL) on success."
  • Ambiguity in mobile criterion: "Sidebar collapses to hamburger on mobile (≤767px) — md:flex pattern" — this tells the developer what the CSS pattern is, but doesn't specify the hamburger behavior: is the drawer a full overlay, a slide-in panel, or something else? The views spec says "Overlay-Drawer" — that level of precision should be in the acceptance criterion or referenced explicitly.
  • "Abmelden" button — scope question: The criterion says the button "posts to /admin/logout, clears cookie, redirects to /admin/login". It doesn't specify what happens if the admin_session cookie is already absent (e.g., expired session, then user clicks logout). This edge case should resolve gracefully — a redirect to /admin/login is correct, but this should be explicit.
  • Size: S — Confirmed appropriate. Five new files, no new database tables, no new queries, small UI surface.

Recommendations

  • Update the "Depends on" note to be explicit: "Depends on: #3 (hooks.server.ts — locals.admin populated from admin_session cookie validation against env-var whitelist)."
  • Strengthen the cookie acceptance criterion: replace "sets admin_session cookie on success" with "sets admin_session cookie (httpOnly: true, sameSite: 'strict', maxAge: 8h, path: '/') on success."
  • Add a missing criterion: "POST to /admin/logout with absent or invalid admin_session cookie still redirects to /admin/login without error."
  • Add a missing criterion: "GET requests to /admin/login when locals.admin is already set redirect to /admin/inventar (skip the login form for already-authenticated admins)."
  • Cross-referencing NFR-SEC-004 explicitly in the issue is correct. Consider also referencing the views spec section: "View 04 — /admin/login" for the precise UI spec.
## 👤 Elicit — Requirements Engineer ### Observations - **US-AUTH-003 is well-formed.** The user story is clear and the acceptance criteria are testable. The issue references the spec correctly and identifies the dependency on #3. - **Missing precondition:** The issue depends on #3 but doesn't state what #3 provides. Based on the plan, #3 is likely `hooks.server.ts` (the cookie-to-locals layer). Without `hooks.server.ts` populating `locals.admin`, the `+layout.server.ts` guard has nothing to check. This dependency should be explicit in the issue: "Depends on: #3 (hooks.server.ts with `locals.admin` populated from `admin_session` cookie)." - **Acceptance criterion gap — cookie attributes:** The criterion "sets `admin_session` cookie on success" omits `httpOnly`, `sameSite`, `maxAge`. These are NFR-SEC attributes, not implementation details — they are testable and should be explicit: "sets `admin_session` cookie (`httpOnly`, `SameSite=Strict`, 8h TTL) on success." - **Ambiguity in mobile criterion:** "Sidebar collapses to hamburger on mobile (≤767px) — `md:flex` pattern" — this tells the developer what the CSS pattern is, but doesn't specify the hamburger behavior: is the drawer a full overlay, a slide-in panel, or something else? The views spec says "Overlay-Drawer" — that level of precision should be in the acceptance criterion or referenced explicitly. - **"Abmelden" button — scope question:** The criterion says the button "posts to `/admin/logout`, clears cookie, redirects to `/admin/login`". It doesn't specify what happens if the `admin_session` cookie is already absent (e.g., expired session, then user clicks logout). This edge case should resolve gracefully — a redirect to `/admin/login` is correct, but this should be explicit. - **Size: S** — Confirmed appropriate. Five new files, no new database tables, no new queries, small UI surface. ### Recommendations - Update the "Depends on" note to be explicit: "Depends on: #3 (`hooks.server.ts` — `locals.admin` populated from `admin_session` cookie validation against env-var whitelist)." - Strengthen the cookie acceptance criterion: replace "sets `admin_session` cookie on success" with "sets `admin_session` cookie (`httpOnly: true`, `sameSite: 'strict'`, `maxAge: 8h`, `path: '/'`) on success." - Add a missing criterion: "POST to `/admin/logout` with absent or invalid `admin_session` cookie still redirects to `/admin/login` without error." - Add a missing criterion: "GET requests to `/admin/login` when `locals.admin` is already set redirect to `/admin/inventar` (skip the login form for already-authenticated admins)." - Cross-referencing NFR-SEC-004 explicitly in the issue is correct. Consider also referencing the views spec section: "View 04 — /admin/login" for the precise UI spec.
marcel commented 2026-05-05 11:28:20 +02:00
Author
Owner
Copy Link

👤 Tobias Wendt — DevOps & Platform Engineer

Observations

  • This issue creates no new infrastructure files. No changes to Dockerfile, docker-compose.yml, or Caddyfile are required for this task.
  • The admin session cookie is admin_session — its value (the admin username) is not sensitive enough to require encryption, but the env vars it derives from (ADMIN_MARCEL_PASSWORD_HASH, etc.) must be present. The docker-compose.yml template in the system design already includes these variables.
  • The acceptance criterion "Valid family_code cookie grants zero access to /admin/*" depends on the separation working correctly at runtime. This is a pure application-layer check — no infra involvement.
  • One infra-adjacent concern: the verifyAdminPassword function reads from process.env[ADMIN_${username.toUpperCase()}_PASSWORD_HASH]. If the env var is missing (misconfigured .env on the server), it silently returns undefined → bcrypt.compare short-circuits → login always fails. This is the correct secure behavior (fail closed), but the error is silent — the admin sees "Ungültige Zugangsdaten." with no log output indicating the real cause.

Recommendations

  • Add a startup check alongside SESSION_SECRET validation: log a warning (not an error — the app should still start) if any of the three ADMIN_*_PASSWORD_HASH env vars is missing. Example: 
    // In hooks.server.ts or lib/auth.ts startup check
for (const name of ['MARCEL', 'RENATE', 'BERIT']) {
  if (!process.env[`ADMIN_${name}_PASSWORD_HASH`]) {
    console.warn(`WARN: ADMIN_${name}_PASSWORD_HASH is not set — ${name} cannot log in`);
  }
}
    This makes misconfigured deployments diagnosable from logs without revealing hashes.
  • The /admin/logout POST action clears the admin_session cookie. Confirm the path: '/' option is set on the cookies.delete() call — without matching path, the browser won't honour the deletion, and the admin will appear to remain logged in.
  • The /health endpoint (referenced in the docker-compose.yml healthcheck) is not part of this issue but should exist before deployment. Flag this as a prerequisite for the first deploy, not this task.
  • The deploy checklist includes "Test login with each admin account" — this implies all three bcrypt hashes must be in the .env file before the first test. This is an ops concern to prepare before this task's acceptance testing.
## 👤 Tobias Wendt — DevOps & Platform Engineer ### Observations - This issue creates no new infrastructure files. No changes to `Dockerfile`, `docker-compose.yml`, or `Caddyfile` are required for this task. - The admin session cookie is `admin_session` — its value (the admin username) is not sensitive enough to require encryption, but the env vars it derives from (`ADMIN_MARCEL_PASSWORD_HASH`, etc.) must be present. The `docker-compose.yml` template in the system design already includes these variables. - The acceptance criterion "Valid `family_code` cookie grants zero access to `/admin/*`" depends on the separation working correctly at runtime. This is a pure application-layer check — no infra involvement. - One infra-adjacent concern: the `verifyAdminPassword` function reads from `process.env[`ADMIN_${username.toUpperCase()}_PASSWORD_HASH`]`. If the env var is missing (misconfigured `.env` on the server), it silently returns `undefined` → bcrypt.compare short-circuits → login always fails. This is the correct secure behavior (fail closed), but the error is silent — the admin sees "Ungültige Zugangsdaten." with no log output indicating the real cause. ### Recommendations - Add a startup check alongside `SESSION_SECRET` validation: log a warning (not an error — the app should still start) if any of the three `ADMIN_*_PASSWORD_HASH` env vars is missing. Example: ```typescript // In hooks.server.ts or lib/auth.ts startup check for (const name of ['MARCEL', 'RENATE', 'BERIT']) { if (!process.env[`ADMIN_${name}_PASSWORD_HASH`]) { console.warn(`WARN: ADMIN_${name}_PASSWORD_HASH is not set — ${name} cannot log in`); } } ``` This makes misconfigured deployments diagnosable from logs without revealing hashes. - The `/admin/logout` POST action clears the `admin_session` cookie. Confirm the `path: '/'` option is set on the `cookies.delete()` call — without matching `path`, the browser won't honour the deletion, and the admin will appear to remain logged in. - The `/health` endpoint (referenced in the `docker-compose.yml` healthcheck) is not part of this issue but should exist before deployment. Flag this as a prerequisite for the first deploy, not this task. - The deploy checklist includes "Test login with each admin account" — this implies all three bcrypt hashes must be in the `.env` file before the first test. This is an ops concern to prepare before this task's acceptance testing.
marcel commented 2026-05-05 11:28:39 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

4 decisions need your input before implementation starts.

Spec vs. Issue Consistency

  • Sidebar width: 155px (issue body) vs. 176px (views spec w-44). The two sources disagree on a measurable value. The views spec impl-ref table is the binding UI reference. Update the issue body to w-44 / 176px, or consciously override the spec with a documented reason. (Raised by: Markus Keller, Leonie Voss)

Design Tokens

  • Tailwind token name for admin background: admin or admin-bg. The views spec HTML uses bg-admin as the CSS class name; the implementation plan's tailwind.config.js defines the key as 'admin-bg' (producing bg-admin-bg). These are two different class names. This must be resolved before Task 1 (scaffold) locks in the Tailwind config, as it affects every admin page. (Raised by: Leonie Voss)

Security Policy

  • Brute-force rate limiting on the admin login endpoint. bcrypt WF12 is a speed brake (~250ms/attempt) but not a lockout. For this short-lived private app the risk is likely acceptable — but the decision should be explicit. Options: (a) accept the risk, document it, move on; (b) add a simple in-memory counter with a 15-minute lockout after 5 failed attempts (no database needed, resets on restart). (Raised by: Nora Steiner)

Testing Infrastructure

  • Test credentials for E2E admin login tests. The Playwright tests need a real bcrypt hash to log in as an admin. Options: (a) add a dedicated ADMIN_TEST_PASSWORD_HASH env var for test runs with a committed known-plaintext password; (b) add a TestAdmin admin account that only exists in the test environment. Real admin names (Marcel, Renate, Berit) must not appear in committed test files. Pick the approach before writing the E2E test setup. (Raised by: Sara Holt)
## 🗳️ Decision Queue — Action Required _4 decisions need your input before implementation starts._ ### Spec vs. Issue Consistency - **Sidebar width: 155px (issue body) vs. 176px (views spec `w-44`).** The two sources disagree on a measurable value. The views spec impl-ref table is the binding UI reference. Update the issue body to `w-44` / 176px, or consciously override the spec with a documented reason. _(Raised by: Markus Keller, Leonie Voss)_ ### Design Tokens - **Tailwind token name for admin background: `admin` or `admin-bg`.** The views spec HTML uses `bg-admin` as the CSS class name; the implementation plan's `tailwind.config.js` defines the key as `'admin-bg'` (producing `bg-admin-bg`). These are two different class names. This must be resolved before Task 1 (scaffold) locks in the Tailwind config, as it affects every admin page. _(Raised by: Leonie Voss)_ ### Security Policy - **Brute-force rate limiting on the admin login endpoint.** bcrypt WF12 is a speed brake (~250ms/attempt) but not a lockout. For this short-lived private app the risk is likely acceptable — but the decision should be explicit. Options: (a) accept the risk, document it, move on; (b) add a simple in-memory counter with a 15-minute lockout after 5 failed attempts (no database needed, resets on restart). _(Raised by: Nora Steiner)_ ### Testing Infrastructure - **Test credentials for E2E admin login tests.** The Playwright tests need a real bcrypt hash to log in as an admin. Options: (a) add a dedicated `ADMIN_TEST_PASSWORD_HASH` env var for test runs with a committed known-plaintext password; (b) add a `TestAdmin` admin account that only exists in the test environment. Real admin names (Marcel, Renate, Berit) must not appear in committed test files. Pick the approach before writing the E2E test setup. _(Raised by: Sara Holt)_
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
admin
Admin interface feature
 deployment
Docker and hosting configuration
 feature
New user-facing feature
 setup
Project scaffolding and infrastructure
No Label
Milestone
No items
No Milestone v1.0 — MVP
Projects
Clear projects
No project
Assignees
Clear assignees
marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/wannsee-kram#8
Delete Branch "%!s()"

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?