diff --git a/.env.example b/.env.example index 15e672f..d8a7952 100644 --- a/.env.example +++ b/.env.example @@ -1 +1,5 @@ -DNS_DOMAIN=localhost \ No newline at end of file +DNS_DOMAIN=localhost +# Keycloak client for the /appeal login. Must match the client TripBot redeems +# the code with (KEYCLOAK_BAN_APPEALS_CLIENT_ID). Read when `next dev` starts or +# `next build` runs, not by a running production container. +NEXT_PUBLIC_KEYCLOAK_CLIENT_ID= diff --git a/Dockerfile b/Dockerfile index d5aa146..0c263bc 100644 --- a/Dockerfile +++ b/Dockerfile @@ -54,6 +54,18 @@ COPY --chown=node:node --from=development /usr/src/app/node_modules ./node_modul # Copy over the existing source code COPY --chown=node:node . . +# Next.js writes `process.env.NEXT_PUBLIC_*` into the browser bundle as literal +# values during `next build`, so they must be set here, at build time. Setting +# them on the running container has no effect on client code. Pass them with +# `docker build --build-arg` or `build.args` in compose. An unset arg becomes an +# empty string, and the code then uses its fallback (see src/utils/keycloak.ts). +ARG NEXT_PUBLIC_DNS_DOMAIN +ARG NEXT_PUBLIC_KEYCLOAK_URL +ARG NEXT_PUBLIC_KEYCLOAK_CLIENT_ID +ENV NEXT_PUBLIC_DNS_DOMAIN=$NEXT_PUBLIC_DNS_DOMAIN \ + NEXT_PUBLIC_KEYCLOAK_URL=$NEXT_PUBLIC_KEYCLOAK_URL \ + NEXT_PUBLIC_KEYCLOAK_CLIENT_ID=$NEXT_PUBLIC_KEYCLOAK_CLIENT_ID + RUN npx next build ################### diff --git a/REVIEW-ISSUES.md b/REVIEW-ISSUES.md index 2413ad3..3619f1b 100644 --- a/REVIEW-ISSUES.md +++ b/REVIEW-ISSUES.md @@ -59,6 +59,7 @@ Priorities: **P0** exploitable or critical, fix now · **P1** real bug or exposu | WEB-028 | P2 | deploy | The shared deployer gives up after ~60 s when another deploy is running | | WEB-029 | P3 | bug | Factsheets pager shows the full count ("1-10 of 555") after filtering | | WEB-030 | P2 | perf | Expanding a factsheet row keeps the browser busy for tens of seconds | +| WEB-031 | P1 | auth, build | Ban-appeal login fails: the Keycloak client ID never reaches `next build` | ### Overlap groups (same files; take together or in sequence) @@ -67,11 +68,11 @@ Priorities: **P0** exploitable or critical, fix now · **P1** real bug or exposu - **Template scripts:** WEB-008, WEB-009, WEB-022 (`_document.tsx`, `_app.tsx`, `public/assets/vendor`). - **Drug data:** WEB-007, WEB-010, WEB-029, WEB-030 (`_app.tsx`, `Factsheets.tsx`, `factsheets/[drug].tsx`, `DrugInfo.tsx`). -- **Auth:** WEB-005, WEB-006, WEB-025 (`keycloak.ts`, `ProfileButton.tsx`, `appeal.tsx`). +- **Auth:** WEB-005, WEB-006, WEB-025, WEB-031 (`keycloak.ts`, `ProfileButton.tsx`, `appeal.tsx`). - **Homepage:** WEB-003, WEB-020, WEB-021 (`pages/index.tsx`, `Counter.tsx`, `Ghost.tsx`). - **Workflows:** WEB-012, WEB-013, WEB-015, WEB-017 (`.github/workflows/`). -- **Docker/Drone:** WEB-014, WEB-016, WEB-018, WEB-027, WEB-028 (`.drone.yml`, `Dockerfile`, - `.dockerignore`, Drone and deployer settings on the servers). +- **Docker/Drone:** WEB-014, WEB-016, WEB-018, WEB-027, WEB-028, WEB-031 (`.drone.yml`, + `Dockerfile`, `.dockerignore`, Drone and deployer settings on the servers). --- @@ -568,3 +569,25 @@ drug, and the table is wrapped in several providers and animations. **Fix.** Profile an expansion in Chrome DevTools (Performance panel) to find what keeps the main thread or compositor busy, then fix that. + +### WEB-031 — Ban-appeal login fails: the Keycloak client ID never reaches `next build` + +- Priority: P1 · Area: auth, build +- Status: fixed — fix/WEB-031-keycloak-build-args (#266) +- Notes: Also needs `build.args` for the three `NEXT_PUBLIC_*` variables in the compose files + that build the image: `docker-compose.website-prod.yml` on the UAT server (WEB-014) and + `docker-compose.website.yml` in the production server's repo. Production builds from `main`. +- Files: `Dockerfile`, `src/utils/keycloak.ts` + +**Problem.** Logging in at `/appeal` fails after the user signs in. TripBot logs "Keycloak +token error" and `POST /api/v2/keycloak/token` returns 500. Keycloak binds an auth code to +the client that requested it, and TripBot redeems it as the ban-appeals client +(`KEYCLOAK_BAN_APPEALS_CLIENT_ID`). The browser asked as `website`. Next.js writes +`NEXT_PUBLIC_*` values into the browser bundle during `next build`, but the Dockerfile never +declared them, so the build saw none. The live bundle still has +`process.env.NEXT_PUBLIC_KEYCLOAK_CLIENT_ID||"website"` and always uses the fallback. + +**Fix.** Declare the three variables as `ARG`/`ENV` in the Dockerfile's build stage and pass +them as compose `build.args`. `keycloak.ts` no longer falls back to `website`: a missing +client ID throws outside production and is logged in production, where Keycloak then rejects +the request before the user signs in. diff --git a/src/utils/__tests__/keycloak.test.ts b/src/utils/__tests__/keycloak.test.ts new file mode 100644 index 0000000..60976a6 --- /dev/null +++ b/src/utils/__tests__/keycloak.test.ts @@ -0,0 +1,66 @@ +import { getLoginUrl } from "../keycloak"; + +// NODE_ENV is typed read-only; tests need to switch it. +const env = process.env as Record; +const saved = { + NODE_ENV: env.NODE_ENV, + NEXT_PUBLIC_KEYCLOAK_URL: env.NEXT_PUBLIC_KEYCLOAK_URL, + NEXT_PUBLIC_KEYCLOAK_CLIENT_ID: env.NEXT_PUBLIC_KEYCLOAK_CLIENT_ID, +}; + +function restore(name: keyof typeof saved) { + if (saved[name] === undefined) delete env[name]; + else env[name] = saved[name]; +} + +beforeEach(() => { + delete env.NEXT_PUBLIC_KEYCLOAK_URL; + delete env.NEXT_PUBLIC_KEYCLOAK_CLIENT_ID; +}); + +afterEach(() => { + restore("NODE_ENV"); + restore("NEXT_PUBLIC_KEYCLOAK_URL"); + restore("NEXT_PUBLIC_KEYCLOAK_CLIENT_ID"); + jest.restoreAllMocks(); +}); + +describe("getLoginUrl", () => { + it("asks Keycloak for a code as the configured client", () => { + env.NEXT_PUBLIC_KEYCLOAK_CLIENT_ID = "ban-appeals"; + const url = new URL(getLoginUrl()); + + expect(url.origin + url.pathname).toBe( + "https://auth.tripsit.me/realms/TripSit/protocol/openid-connect/auth", + ); + expect(url.searchParams.get("client_id")).toBe("ban-appeals"); + expect(url.searchParams.get("redirect_uri")).toBe( + `${window.location.origin}/appeal`, + ); + expect(url.searchParams.get("response_type")).toBe("code"); + }); + + it("uses NEXT_PUBLIC_KEYCLOAK_URL when it is set", () => { + env.NEXT_PUBLIC_KEYCLOAK_CLIENT_ID = "ban-appeals"; + env.NEXT_PUBLIC_KEYCLOAK_URL = "https://auth.example.test"; + + expect(getLoginUrl()).toMatch( + /^https:\/\/auth\.example\.test\/realms\/TripSit\//, + ); + }); + + it("throws outside production when the client ID is missing", () => { + expect(() => getLoginUrl()).toThrow(/NEXT_PUBLIC_KEYCLOAK_CLIENT_ID/); + }); + + it("never falls back to the old 'website' client in production", () => { + env.NODE_ENV = "production"; + const error = jest.spyOn(console, "error").mockImplementation(() => {}); + const url = new URL(getLoginUrl()); + + expect(url.searchParams.get("client_id")).toBe(""); + expect(error).toHaveBeenCalledWith( + expect.stringContaining("NEXT_PUBLIC_KEYCLOAK_CLIENT_ID"), + ); + }); +}); diff --git a/src/utils/keycloak.ts b/src/utils/keycloak.ts index 7d325da..55db931 100644 --- a/src/utils/keycloak.ts +++ b/src/utils/keycloak.ts @@ -1,22 +1,47 @@ // src/utils/keycloak.ts -// Simple Keycloak helper functions (placeholder values) +// Builds the Keycloak login and logout URLs for the ban appeal flow. +// +// Next.js replaces each `process.env.NEXT_PUBLIC_*` expression with its value +// when `next build` runs, so in the browser these are fixed at build time. +// Changing them on a running container does nothing; they must be passed as +// build args (see the Dockerfile). Next.js only replaces the full +// `process.env.NAME` expression, so don't destructure or index process.env here. + +const CLIENT_ID_MISSING = + "NEXT_PUBLIC_KEYCLOAK_CLIENT_ID was not set when this bundle was built. " + + "Set it to the ban-appeals Keycloak client that TripBot redeems the code " + + "with (KEYCLOAK_BAN_APPEALS_CLIENT_ID); otherwise the token exchange fails."; -// Helper function to get config at runtime instead of build time function getKeycloakConfig() { return { realm: "TripSit", url: process.env.NEXT_PUBLIC_KEYCLOAK_URL || "https://auth.tripsit.me", - clientId: process.env.NEXT_PUBLIC_KEYCLOAK_CLIENT_ID || "website", - redirectUri: - typeof window !== "undefined" ? `${window.location.origin}/appeal` : "", }; } +// Keycloak binds an auth code to the client that asked for it, and TripBot's +// /api/v2/keycloak/token redeems it as the ban-appeals client. Any other client +// ID here makes every login fail after the user signs in, so there is no +// default. Outside production a missing ID throws; in production it is logged, +// and Keycloak rejects the login request straight away. +function getClientId(): string { + const clientId = process.env.NEXT_PUBLIC_KEYCLOAK_CLIENT_ID; + if (!clientId) { + if (process.env.NODE_ENV !== "production") { + throw new Error(CLIENT_ID_MISSING); + } + console.error(CLIENT_ID_MISSING); + return ""; + } + return clientId; +} + export function getLoginUrl(): string { const config = getKeycloakConfig(); const params = new URLSearchParams({ - client_id: config.clientId, - redirect_uri: config.redirectUri, + client_id: getClientId(), + redirect_uri: + typeof window !== "undefined" ? `${window.location.origin}/appeal` : "", response_type: "code", scope: "openid profile email", });