docs(specs): plan admin gate for RemoveAllResolvedAlerts (#68)

[SoulKyu]

Planning spec for #68.

Decision

RemoveAllResolvedAlerts currently authorizes any valid session to hard-delete the team's whole resolved-alert history. The spec gates it on a config-driven admin list:

• Authority: new AdminConfig.Users + IsAdmin(usernameOrEmail), fed by NOTIFICATOR_ADMIN_USERS. Matches the existing config pattern, works for local and OAuth accounts, no DB schema change.
• Impersonation list: not silently reused — kept only as an explicit, documented fallback when admin.users is unset, because the WebUI already gates this exact button on canImpersonate. Both empty ⇒ fail-closed.
• OAuth administrator role: rejected as authority — the string exists only in default GroupMapping tables, no Go code reads it and no role is persisted on User.

Scope

Backend gate (services.go:1197+) + config plumbing + WebUI UX check alignment + env/chart docs. Session errors from #65 untouched.

Closes #68

[SoulKyu]

Solid spec: the authority decision is argued with evidence, the fail-closed default is deliberate, and the acceptance-criteria mapping lines up with issue #68. I verified the code claims locally — the admin config struct at config/config.go:30-42 matches the description, RemoveAllResolvedAlerts at internal/backend/services/services.go:1197+ really has no authorization check, and NewAlertServiceGorm has exactly the 2 call sites the risk table lists.

Three gaps below, all about the config/UX surface rather than the gate itself. They are written as top-level feedback because the outbound content-safety gate rejected inline anchors on this PR's spec filename; each finding names its section and line range in the spec document added by this PR (the only changed file, under specs/).

---

1. Approach, step 1 (spec lines 76-83) — state that the env var is the only working config source

Problem: step 1 prescribes both a manual NOTIFICATOR_ADMIN_USERS parse and a viper default plus env binding on the admin.users key, and gives the new field a json:"users" tag. That reads as "you can also set admin.users in the config file". You cannot: LoadConfigWithViper calls viper.Unmarshal(cfg) at config/config.go:263 with no decoder option, so mapstructure's default tag applies — and the config structs carry only json tags. A snake_case viper key never matches the Go field name.

Evidence: I probed the existing sibling field. Feeding viper a config containing an admin block with an impersonation_allowed_users list and unmarshalling into Config gives viper.IsSet(...) == true and viper.GetStringSlice(...) returning the list, but the struct field cfg.Admin.ImpersonationAllowedUsers stays nil. The only thing that populates that field today is the manual os.Getenv block at config/config.go:326-336 — the exact block this spec mirrors.

Why it matters: combined with the deliberate fail-closed default (spec lines 68-72), an operator who configures the admin list in config.json gets a silently dead feature: no error, no admin, removal denied for everyone. That failure mode is indistinguishable from "I forgot to configure it", and the spec's "config-driven admin list" framing plus the documented json tag point operators straight at the broken path. The spec rubric wants config precedence explicit; right now it is implied and wrong.

Suggested change: add a short Config sources note under step 1, e.g. "The environment variable is the only supported source. The admin.users key is not readable from the config file: viper.Unmarshal at config/config.go:263 uses mapstructure tags and the config structs only carry json tags, so admin.* keys never bind — the default/bind lines exist for parity with the impersonation key, not as a functioning file source." Then either keep that limitation and document it in ENVIRONMENT_VARIABLES.md next to the fail-closed semantics, or widen the scope to add mapstructure tags to both admin fields — and if you take that second option, add a Validation bullet asserting that a config-file-only admin list produces a working admin.

---

2. Approach, step 4 / WebUI (spec lines 106-109) — the denial reaches users as an HTTP 500

Problem: step 4 keeps the WebUI check as UX only and says it "avoids a confusing 500-ish round trip", but never says what happens when the round trip does occur. It will occur: backend and webui are separate processes each loading their own config (cmd/backend.go:43 and internal/webui/router.go:25), and step 5 asks operators to set the variable in two separate chart env blocks. Drift between them is a normal misconfiguration, not an edge case.

Evidence: on the denial path, BackendClient.RemoveAllResolvedAlerts collapses an unsuccessful response into a generic error at internal/webui/client/backend_client.go:579-581, and the handler turns any error into http.StatusInternalServerError at internal/webui/handlers/dashboard_handlers.go:1891-1894. So a clean authorization denial surfaces as an HTTP 500 whose text reads "Failed to remove resolved alerts: failed to remove all resolved alerts: Admin rights required". Contrast the local gate two lines above it, which correctly returns http.StatusForbidden.

Why it matters: issue #68's first acceptance criterion is an explicit admin-required denial. Delivered as a 500 it looks like a backend outage — operators will debug the gRPC service instead of their admin list, and monitoring will count authorization denials as server errors.

Suggested change: add a sub-bullet to step 4: "BackendClient.RemoveAllResolvedAlerts must distinguish denial from failure — return a sentinel error such as ErrAdminRequired when the backend denies, and have the dashboard handler return http.StatusForbidden with the backend message for that case, keeping 500 for genuine transport or database failures." Then add a Validation bullet: "With the admin list set on the backend but not on the webui process (and the reverse), the request returns 403 with the admin-required message, not 500."

---

3. Admin authority section, IsAdmin signature block (spec lines 59-63) — define the empty-input contract

Problem: the IsAdmin contract covers the fallback and case-insensitivity but says nothing about empty input or empty list entries. Step 2 (spec line 93) calls it as a match on the username or the email, and the email is routinely empty — nothing in the auth path requires one. If EqualFold is applied naively, as CanImpersonate does at config/config.go:35-42, and the list ever contains an empty entry, then IsAdmin on an empty string returns true and every user without an email becomes an administrator of a destructive, irreversible endpoint.

Evidence: the env parser at config/config.go:330-333 trims and drops empty segments, so the env path is safe today — but that safety is a property of the parser, not of IsAdmin, and it disappears the moment the list arrives from anywhere else (a config file, a Helm value rendering an empty string, or a future loader). The spec explicitly says to mirror CanImpersonate, which has no such guard.

Why it matters: this is the one fail-open path in an otherwise carefully fail-closed design, and it is invisible in review because the guard lives in a different function than the one being reviewed.

Suggested change: extend the doc comment on the signature to state the contract — "Returns false for an empty username or email. Empty or whitespace-only list entries are ignored." — implement that guard inside IsAdmin itself rather than relying on the parser, and add a case to the config unit test listed in Validation (spec lines 138-139): an empty identity is never admin, even when the admin list contains an empty entry.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🧪 QA Report

Environment: make test (docker-compose, backend :50051/:8080 healthy, webui :8081 healthy, postgres + fake alertmanager). Criteria taken from issue #68, which this PR body closes via Closes #68.

This PR's diff is one file — specs/2026-07-25-68-...md, 151 added lines, zero code. So the functional criteria of #68 were exercised against the running app at this head SHA and are, with one exception, unmet. Details below are empirical, not a code read.

Criterion	Result	Evidence
A valid non-admin session calling RemoveAllResolvedAlerts is denied (no deletion, explicit "admin required" message)	❌	Registered qa_nonadmin via notificator.auth.AuthService/Register, logged in, seeded 3 resolved alerts (GetResolvedAlerts → totalCount: 3), then called notificator.alert.AlertService/RemoveAllResolvedAlerts over gRPC with that session → {"success": true, "removedCount": 3, "message": "Successfully removed 3 resolved alerts"}, totalCount after = 0. Wide open.
A valid admin session succeeds and deletes as before	❌	Cannot be exercised: no admin notion exists to grant. config.AdminConfig (config/config.go:30-32) has only ImpersonationAllowedUsers; there is no Users field and no IsAdmin. Every session is effectively admin, so the criterion is unverifiable rather than satisfied.
Admin determination reuses an existing/config-driven mechanism, documented	❌	Booted a second backend with both NOTIFICATOR_ADMIN_USERS=only_the_real_admin and NOTIFICATOR_ADMIN_IMPERSONATION_ALLOWED_USERS=only_the_real_admin. Seeded 2 resolved alerts, called the RPC with the session of qa_nonadmin — a user on neither list → {"success": true, "removedCount": 2}, count after = 0. Neither variable has any effect on this endpoint.
Env var / config surface for the admin list documented	❌	ENVIRONMENT_VARIABLES.md: 0 matches for ADMIN. .env.example: only NOTIFICATOR_SESSION_SECRET. charts/: 0 matches for ADMIN.
Existing session-validation behaviour from #65 preserved	✅	session_id: "" → "Session ID is required"; session_id: "deadbeefnotasession" → "Invalid session". Both intact, no deletion.

Exploratory: No adjacent breakage — expected for a docs-only diff, and confirmed live. Logged into the WebUI in a real browser as qa_nonadmin: /dashboard and /silences both render 200 with alerts listed; the WebUI UX gate still holds, DELETE /api/v1/dashboard/remove-resolved-alerts → 403 {"success":false,"error":"You are not allowed to remove resolved alerts"} via canImpersonate. That 403 is exactly the point — the gate exists only in the WebUI, and the gRPC backend behind it accepts the same user without question.

Verdict: FAIL

Note for whoever picks this up

The failure is a scope/linkage issue, not a defect in the spec's content. The spec itself is coherent and reviewable; nothing in it was contradicted by the running app — I confirmed its problem statement empirically (services.go:1197+ really does authorize any session, and the OAuth administrator role really is inert). But the PR body says Closes #68, so merging it auto-closes the issue while a valid non-admin session can still irreversibly wipe the team's entire resolved-alert history. That is the exact vulnerability #68 was filed for.

Two ways out, both cheap:

1. Change Closes #68 to Refs #68 / Part of #68 in the PR body, so the issue stays open for the implementation PR. Then this spec merges on its review merits alone.
2. Implement the gate in this PR (steps 1-5 of the spec), at which point the criteria above become testable.

Also worth folding in before implementation: the reviewer's three findings above (config-file source is dead because viper.Unmarshal needs mapstructure tags, denial surfacing as HTTP 500 instead of 403, and the IsAdmin empty-input contract) — I did not re-verify those, they are orthogonal to this run.

Fixer summoned via looper fix SoulKyu/notificator#87.

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

Re-review at the rebased head. The spec file itself is byte-identical to the version I reviewed in the previous round — the only delta in this range is unrelated devtools/factory-tui work pulled in by the rebase — so the three findings from that round are still open, and I re-verified the spec's code claims against the current tree (they all hold: AdminConfig at config/config.go:30-32 has only ImpersonationAllowedUsers, RemoveAllResolvedAlerts at internal/backend/services/services.go:1197-1232 has no authorization check between GetUserBySession and the delete, and NewAlertServiceGorm has exactly the 2 call sites the risk table names).

This review is blocking — it is posted as a COMMENT only because GitHub refuses a changes-requested review on a self-authored PR.

Two blocking findings below. Both are top-level rather than inline because the outbound content-safety gate rejects inline anchors on this PR's spec filename; each names its section and line range in the spec document added by this PR, the only changed file in scope.

---

Blocking — PR description, the issue-closing keyword

Problem: the PR body ends with a Closes keyword pointing at issue #68, but this PR adds only the single spec document under specs/ — 151 lines of plan, zero lines of code. Merging it makes GitHub auto-close issue #68 while the vulnerability described in the spec's own Problem section is still live.

Evidence: the QA run on this PR exercised the endpoint against the running stack at this head: a freshly registered non-admin session called notificator.alert.AlertService/RemoveAllResolvedAlerts over gRPC and got success: true with removedCount: 3, and the resolved-alert count went to zero. It also booted a backend with both admin-related environment variables pointed at a different user and got the same unconditional success — neither variable is read by any code today.

Why it matters: an auto-closed issue drops out of the backlog and out of Looper's implementation queue. The plan would merge, the issue would read as fixed, and any authenticated account would still be able to irreversibly wipe the team's resolved-alert history — which is exactly the failure #68 was filed for. This is a lifecycle bug in the PR, not a defect in the spec's content.

Suggested change: change the closing keyword to a non-closing reference — Refs #68 or Part of #68 — in the PR description, so the issue stays open for the implementation PR that carries steps 1-5. If you would rather land plan and code together, fold the implementation into this PR instead and keep the closing keyword.

---

Blocking — WebUI gate, Approach step 4

Fallback location: the spec document added by this PR under specs/, Approach step 4, spec lines 106-109 (the inline anchor was rejected by the outbound content-safety gate on this PR's spec filename).

Blocking — step 4 misses the client-side half of the WebUI gate, so the new admin list makes the button disappear for admins.

Problem: step 4 names a single site, dashboard_handlers.go:1878, and says to switch it from canImpersonate(c) to an AdminConfig.IsAdmin check. But that handler only guards the POST. Whether the button is rendered at all is decided client-side by a completely separate flag that step 4 never mentions.

Evidence: the "Remove All Resolved Alerts" button lives at internal/webui/templates/components/modal_components.templ:148-160, wrapped in <template x-if="canAdmin">, and canAdmin is assigned from window.impersonationState.canImpersonate at line 150 of that file. That global is populated in internal/webui/templates/layouts/Base.templ:115-125 from the can_impersonate field of the impersonation status response, which internal/webui/handlers/impersonation_handlers.go:184 computes with canImpersonate(c) — i.e. purely from ImpersonationAllowedUsers. NewDashboard.templ:223 gates on the same canAdmin.

Why it matters: follow step 4 literally and an operator who puts a user on the new admin list but not on the impersonation list gets a backend that authorizes that user and a UI that never shows them the button. The feature becomes unreachable through the WebUI for exactly the configuration this spec tells operators to adopt, which directly contradicts the Risks table (spec line 123): "one env var restores it" would not be true. The mirror case is worse for review confidence — someone on the impersonation list but absent from the admin list still sees the button and gets a hard denial from the backend, which is the confusing round trip step 4 says it wants to avoid.

Suggested change: replace step 4 with a two-part instruction. (a) Add an is_admin field to the impersonation status payload at impersonation_handlers.go:184, computed with appConfig.Admin.IsAdmin(user.Username) || appConfig.Admin.IsAdmin(user.Email), carry it through Base.templ as a distinct window.impersonationState.isAdmin, and rebind canAdmin in modal_components.templ:150 and NewDashboard.templ:223 to that new flag — leaving canImpersonate to keep meaning impersonation only. (b) Switch dashboard_handlers.go:1878 to the same IsAdmin check. Then add a Validation bullet under the existing list (spec lines 128-141): "With the admin environment variable naming a user who is absent from the impersonation list, that user sees the Remove All Resolved Alerts button and the request succeeds; a user on the impersonation list only does not see the button."

---

Still open from the previous round (non-blocking, unchanged text in the spec)

These were raised in the earlier review and none of the referenced spec lines moved, so they carry over rather than being restated in full here:

1. Approach step 1 (spec lines 76-83) — the admin.users config-file path is dead. LoadConfigWithViper calls viper.Unmarshal(cfg) at config/config.go:263 with no decoder option, so mapstructure's default tag applies, and the config structs carry only json tags. Either document that the environment variable is the only supported source, or add mapstructure tags to both admin fields plus a Validation bullet proving a file-only admin list works.
2. Approach step 4 (spec lines 106-109) — a clean authorization denial reaches the browser as HTTP 500, because BackendClient.RemoveAllResolvedAlerts collapses it into a generic error (internal/webui/client/backend_client.go:579-581) and the handler maps any error to http.StatusInternalServerError (internal/webui/handlers/dashboard_handlers.go:1891-1894). Specify a sentinel error and a 403 on the denial path.
3. IsAdmin signature block (spec lines 59-63) — define the empty-input contract, so an empty user.Email combined with an empty list entry can never match.

The authority decision itself remains well argued and I would not change it: the dedicated admin list, the explicit-not-silent impersonation fallback, and the fail-closed default are all the right calls for a destructive endpoint.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🧪 QA Report

Environment: make test (docker-compose rebuild; backend :50051/:8080 healthy, webui :8081 healthy, postgres + fake alertmanager). Criteria taken from issue #68, which this PR body closes via Closes #68.

Diff at this head is still one file — specs/2026-07-25-68-...md, 151 added lines, zero code. Compared to the previously-QA'd head 543af2a, the only change is devtools/factory-tui/factory-tui.py (rebase pickup); the spec file and the PR body are byte-identical. Criteria were nonetheless re-exercised live at this SHA — everything below is empirical, not carried over.

Criterion	Result	Evidence
A valid non-admin session calling RemoveAllResolvedAlerts is denied (no deletion, explicit "admin required" message)	❌	Registered qa_nonadmin2 via notificator.auth.AuthService/Register, logged in, seeded 3 resolved alerts (GetResolvedAlerts → totalCount: 3), then called notificator.alert.AlertService/RemoveAllResolvedAlerts over gRPC with that session → {"success": true, "removedCount": 3, "message": "Successfully removed 3 resolved alerts"}; totalCount after = 0. Still wide open.
A valid admin session succeeds and deletes as before	❌	Cannot be exercised: no admin notion exists to grant. config.AdminConfig (config/config.go:30-32) still has only ImpersonationAllowedUsers — no Users field, no IsAdmin. Every session is effectively admin, so the criterion is unverifiable rather than satisfied.
Admin determination reuses an existing/config-driven mechanism, documented	❌	Booted a second backend (:50052) with both NOTIFICATOR_ADMIN_USERS=only_the_real_admin and NOTIFICATOR_ADMIN_IMPERSONATION_ALLOWED_USERS=only_the_real_admin. Seeded 2 resolved alerts, called the RPC with the session of qa_nonadmin2 — a user on neither list → {"success": true, "removedCount": 2}, count after = 0. Neither variable has any effect on this endpoint.
Env var / config surface for the admin list documented	❌	ENVIRONMENT_VARIABLES.md: 0 matches for ADMIN. .env.example: only NOTIFICATOR_SESSION_SECRET. charts/: the sole admin hits are inside a Google OAuth scope URL (admin.directory.group.readonly), not an admin list.
Existing session-validation behaviour from #65 preserved	✅	session_id: "" → {"message": "Session ID is required"}; session_id: "deadbeefnotasession" → {"message": "Invalid session"}. Both intact, no deletion in either case.

Exploratory: No adjacent breakage — expected for a docs-only diff, and confirmed live in a real browser (Playwright/Chromium). Logged in as qa_nonadmin2: login redirects to /dashboard, which renders 13 alerts (Critical 6 / Warning 5 / Info 2) with filters and table intact; /silences returns 200. The WebUI UX gate still holds — DELETE /api/v1/dashboard/remove-resolved-alerts from that logged-in session → 403 {"success":false,"error":"You are not allowed to remove resolved alerts"} via canImpersonate. That 403 is exactly the point: the gate exists only in the WebUI, and the gRPC backend behind it accepted the very same user's session without question (row 1).

Verdict: FAIL

Note for whoever picks this up

Unchanged from the previous run, because the substance of the PR is unchanged. The failure is a scope/linkage issue, not a defect in the spec's content. The spec itself is coherent and reviewable, and nothing in it was contradicted by the running app — I re-confirmed its problem statement empirically (services.go:1198+ really does authorize any session, and the OAuth administrator role really is inert). But the PR body says Closes #68, so merging it auto-closes the issue while a valid non-admin session can still irreversibly wipe the team's entire resolved-alert history. That is the exact vulnerability #68 was filed for.

Two ways out, both cheap:

1. Change Closes #68 to Refs #68 / Part of #68 in the PR body, so the issue stays open for the implementation PR. Then this spec merges on its review merits alone.
2. Implement the gate in this PR (steps 1-5 of the spec), at which point the criteria above become testable.

Fixer summoned via looper fix SoulKyu/notificator#87.

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

Re-review at the new head. The spec document added by this PR under specs/ is byte-identical to the version I reviewed in the previous round — the only delta in this range comes from a rebase that pulled in unrelated internal/alertmanager and internal/webui/router.go work — so every finding from that round is still open. I re-verified all of the spec's code claims against the current tree and they all hold: AdminConfig at config/config.go:30-32 still carries only ImpersonationAllowedUsers, RemoveAllResolvedAlerts at internal/backend/services/services.go:1197-1232 still runs GetUserBySession straight into the delete with no authorization check, and NewAlertServiceGorm still has exactly the 2 call sites the risk table names.

This review reads as blocking. It is submitted with a COMMENT state only because GitHub refuses a changes-requested review on a self-authored pull request.

Two blocking findings below, followed by three non-blocking carry-overs. All of them are top-level rather than inline: the outbound content-safety gate rejects inline anchors on this PR's spec filename, so each finding states its section and line range instead.

---

Blocking 1 — PR description, the issue-closing keyword

Fallback location: the pull request description, final line.

Problem: the PR body ends with a closing keyword pointing at issue #68, but this PR adds only the single spec document under specs/ — 151 lines of plan, zero lines of code. Merging it makes GitHub auto-close issue #68 while the vulnerability described in the spec's own Problem section is still live in main.

Evidence: the QA runs on this PR exercised the endpoint against the running stack. A freshly registered non-admin session called the alert service's remove-all-resolved-alerts RPC over gRPC and got success: true with a non-zero removed count, and the resolved-alert count went to zero. A backend booted with both admin-related environment variables pointed at a different user returned the same unconditional success, because no code reads either variable today. I confirmed the code side of that above: there is no authorization branch between GetUserBySession and s.db.RemoveAllResolvedAlerts().

Why it matters: an auto-closed issue drops out of the backlog and out of the implementation queue. The plan would merge, the issue would read as fixed, and any authenticated account would still be able to irreversibly wipe the team's resolved-alert history — exactly the failure #68 was filed for.

Suggested change: replace the closing keyword with a non-closing reference such as "Refs #68" or "Part of #68" in the PR description, so the issue stays open for the implementation PR that carries steps 1-5. If you would rather land plan and code together, fold the implementation into this PR and keep the closing keyword.

---

Blocking 2 — Approach step 4 misses the client-side half of the WebUI gate

Fallback location: the spec document added by this PR under specs/, Approach step 4, spec lines 106-109.

Problem: step 4 names a single site, dashboard_handlers.go:1878, and says to switch it from canImpersonate(c) to an AdminConfig.IsAdmin check. That handler only guards the POST. Whether the button is rendered at all is decided client-side by a separate flag the step never mentions.

Evidence: the "Remove All Resolved Alerts" button lives in internal/webui/templates/components/modal_components.templ, wrapped in <template x-if="canAdmin"> at line 151, and canAdmin is assigned from window.impersonationState.canImpersonate at line 150. The same assignment appears in internal/webui/templates/layouts/Base.templ:258, and NewDashboard.templ:223 gates on the same canAdmin. That global is populated from the can_impersonate field of the impersonation status response, which internal/webui/handlers/impersonation_handlers.go:184 computes with canImpersonate(c) — purely from ImpersonationAllowedUsers.

Why it matters: follow step 4 literally and an operator who puts a user on the new admin list but not on the impersonation list gets a backend that authorizes that user and a UI that never shows them the button. The feature becomes unreachable through the WebUI for exactly the configuration this spec tells operators to adopt, which contradicts the Risks table at spec line 123 ("one env var restores it"). The mirror case is worse: someone on the impersonation list but absent from the admin list still sees the button and gets a hard denial from the backend — the confusing round trip step 4 says it wants to avoid.

Suggested change: replace step 4 with a two-part instruction. (a) Add an is_admin field to the impersonation status payload at impersonation_handlers.go:184, computed as appConfig.Admin.IsAdmin(user.Username) || appConfig.Admin.IsAdmin(user.Email), carry it through Base.templ as a distinct window.impersonationState.isAdmin, and rebind canAdmin in modal_components.templ:150, Base.templ:258 and NewDashboard.templ:223 to that new flag — leaving canImpersonate to keep meaning impersonation only. (b) Switch dashboard_handlers.go:1878 to the same IsAdmin check. Then add a Validation bullet under the existing list at spec lines 128-141: "With the admin environment variable naming a user who is absent from the impersonation list, that user sees the Remove All Resolved Alerts button and the request succeeds; a user on the impersonation list only does not see the button."

---

Non-blocking carry-overs (unchanged spec text, referenced lines have not moved)

1. Approach step 1, spec lines 76-83 — the config-file path for admin.users is dead. Step 1 prescribes a viper default plus env binding on the admin.users key and gives the new field a json:"users" tag, which reads as "you can also set this in the config file". You cannot: LoadConfigWithViper calls viper.Unmarshal(cfg) at config/config.go:263 with no decoder option, so mapstructure's default tag applies and the config structs carry only json tags — a snake_case viper key never matches the Go field name. The only thing populating the sibling field today is the manual environment block at config/config.go:326-336, which I re-read at this head. Combined with the deliberate fail-closed default at spec lines 68-72, an operator who configures the list in config.json gets a silently dead feature. Fix: add a short "Config sources" note under step 1 stating that the environment variable is the only supported source and why, and document that in ENVIRONMENT_VARIABLES.md next to the fail-closed semantics — or widen scope to add mapstructure tags to both admin fields plus a Validation bullet proving a file-only admin list produces a working admin.

2. Approach step 4, spec lines 106-109 — a clean denial reaches the browser as HTTP 500. BackendClient.RemoveAllResolvedAlerts collapses an unsuccessful response into a generic error at internal/webui/client/backend_client.go:579-581, and the handler maps any error to http.StatusInternalServerError at internal/webui/handlers/dashboard_handlers.go:1891-1894 — while the local gate a few lines above correctly returns http.StatusForbidden. Backend and webui load their own config in separate processes, and step 5 asks operators to set the variable in two chart env blocks, so drift is a normal misconfiguration. Issue #68's first criterion is an explicit denial; delivered as a 500 it looks like an outage. Fix: specify a sentinel error such as ErrAdminRequired on the client, map it to 403 with the backend message in the handler, keep 500 for genuine transport or database failures, and add a Validation bullet covering the backend-set/webui-unset configuration in both directions.

3. IsAdmin signature block, spec lines 59-63 — define the empty-input contract. Step 2 at spec line 93 calls IsAdmin on the username or the email, and the email is routinely empty. The spec says to mirror CanImpersonate, which at config/config.go:34-42 applies strings.EqualFold with no guard — so an empty list entry would make every user without an email an administrator of a destructive endpoint. The environment parser trims and drops empty segments, but that safety belongs to the parser, not to IsAdmin, and it disappears the moment the list arrives from anywhere else. Fix: extend the doc comment to state "Returns false for an empty username or email. Empty or whitespace-only list entries are ignored", implement that guard inside IsAdmin itself, and add a case to the config unit test in Validation asserting an empty identity is never admin even when the list contains an empty entry.

---

The authority decision itself remains well argued and I would not change it: the dedicated admin list, the explicit-not-silent impersonation fallback, and the fail-closed default are all the right calls for a destructive, irreversible endpoint.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🧪 QA Report

Environment: make test (docker-compose rebuild; backend :50051/:8080 healthy, webui :8081 healthy, postgres + fake alertmanager). Criteria taken from issue #68, which this PR body closes via Closes #68.

Third run, post-rebase head 06e0d12. The diff is still one file — specs/2026-07-25-68-fix-backend-restrict-removeallresolvedalerts.md, 151 added lines, zero code. Against the previously-QA'd head f5710ce, the spec file and the PR body are byte-identical; only the base moved. Every criterion below was nonetheless re-exercised live at this SHA — all evidence is empirical, none carried over.

Criterion	Result	Evidence
A valid non-admin session calling RemoveAllResolvedAlerts is denied (no deletion, explicit "admin required" message)	❌	Registered qa_nonadmin3 via notificator.auth.AuthService/Register, logged in, seeded 3 resolved alerts (GetResolvedAlerts → totalCount: 3), then called notificator.alert.AlertService/RemoveAllResolvedAlerts over gRPC with that session → {"success": true, "removedCount": 3, "message": "Successfully removed 3 resolved alerts"}; count after = Found 0 resolved alerts. Still wide open.
A valid admin session succeeds and deletes as before	❌	Cannot be exercised: no admin notion exists to grant. config.AdminConfig (config/config.go:30-32) still has only ImpersonationAllowedUsers — no Users field, no IsAdmin anywhere in the tree. Every session is effectively admin, so the criterion is unverifiable rather than satisfied.
Admin determination reuses an existing/config-driven mechanism, documented	❌	Booted a second backend (:50052, same DB) with both NOTIFICATOR_ADMIN_USERS=only_the_real_admin and NOTIFICATOR_ADMIN_IMPERSONATION_ALLOWED_USERS=only_the_real_admin. Seeded 2 resolved alerts, called the RPC with the session of qa_nonadmin3 — a user on neither list → {"success": true, "removedCount": 2}, count after = 0. Neither variable has any effect on this endpoint.
Env var / config surface for the admin list documented	❌	ENVIRONMENT_VARIABLES.md: 0 case-insensitive matches for ADMIN. .env.example: only NOTIFICATOR_SESSION_SECRET. charts/: the sole admin hits are inside a Google OAuth scope URL (values.yaml:89, README.md:208 — admin.directory.group.readonly), not an admin list.
Existing session-validation behaviour from #65 preserved	✅	session_id: "" → {"message": "Session ID is required"}; session_id: "deadbeefnotasession" → {"message": "Invalid session"}. Both intact, no deletion in either case.

Exploratory: No adjacent breakage — expected for a docs-only diff, confirmed live in a real browser (Playwright/Chromium, chrome-devtools MCP unavailable in this run). Logged in as qa_nonadmin3: login redirects to /dashboard, which renders 16 alert rows with search/filter bar and table intact; /silences → 200. The WebUI UX gate still holds — DELETE /api/v1/dashboard/remove-resolved-alerts from that logged-in session → 403 {"success":false,"error":"You are not allowed to remove resolved alerts"} via canImpersonate. That 403 is exactly the point: the gate exists only in the WebUI, and the gRPC backend behind it accepted the very same user's session without question (row 1).

Verdict: FAIL

Note for whoever picks this up

Unchanged from the two previous runs, because the substance of the PR is unchanged — the rebase moved the base, not the content. The failure is a scope/linkage issue, not a defect in the spec's content. The spec itself is coherent and reviewable, and nothing in it was contradicted by the running app — I re-confirmed its problem statement empirically (services.go:1198+ really does authorize any valid session, and the OAuth administrator role really is inert). But the PR body says Closes #68, so merging it auto-closes the issue while a valid non-admin session can still irreversibly wipe the team's entire resolved-alert history. That is the exact vulnerability #68 was filed for.

Two ways out, both cheap:

1. Change Closes #68 to Refs #68 / Part of #68 in the PR body, so the issue stays open for the implementation PR. Then this spec merges on its review merits alone.
2. Implement the gate in this PR (steps 1-5 of the spec), at which point the criteria above become testable.

Re-running QA a fourth time on an unchanged spec will produce this same table; the ball is on the linkage decision.

Fixer summoned via looper fix SoulKyu/notificator#87 (run 39b46fd6). Note: this is the third summon on identical spec content — if the fixer cannot act, the linkage decision above needs a human.

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

Re-review at head 2ae142d. The rebase collapsed this branch to a single commit and the diff is now exactly one file — the spec document added under specs/, 151 added lines, zero code. That document is byte-identical to the version reviewed in the previous rounds, so all five findings carry over. I re-verified every code claim the spec makes against the current tree, and they all hold:

• AdminConfig at config/config.go:30-32 still carries only ImpersonationAllowedUsers; CanImpersonate at config/config.go:35-42 is a bare strings.EqualFold loop with no empty-input guard.
• RemoveAllResolvedAlerts at internal/backend/services/services.go:1198-1232 still runs GetUserBySession straight into s.db.RemoveAllResolvedAlerts() with no authorization branch.
• NewAlertServiceGorm has exactly the 2 call sites the risk table names (internal/backend/server.go:127, internal/backend/services/remove_resolved_alerts_test.go:28).
• Steps 3 and 4 are feasible as written: Server.config exists at internal/backend/server.go:32, and the WebUI has a package-level appConfig set through SetAppConfig at internal/webui/handlers/impersonation_handlers.go:18.

This review reads as blocking. It is submitted with a COMMENT state only because GitHub refuses a changes-requested review on a self-authored pull request.

Two blocking findings, then three non-blocking carry-overs. All of them are top-level rather than inline: the outbound content-safety gate rejects inline anchors on this PR's only changed path, so each finding states its section and line range instead.

---

Blocking 1 — PR description, final line: the issue-closing keyword

Fallback location: the pull request description, last line.

Problem: the PR body ends with a closing keyword pointing at issue #68, but this PR ships only the plan. Merging it makes GitHub auto-close #68 while the vulnerability described in the spec's own Problem section is still live in main.

Evidence: the QA runs on this branch exercised the endpoint against the running stack — a freshly registered non-admin session called the alert service's remove-all-resolved-alerts RPC over gRPC and got success: true with a non-zero removed count, and the resolved-alert count went to zero. A backend booted with both admin-related environment variables pointed at a different user returned the same unconditional success, because no code reads either variable today. I confirmed the code side above: there is no authorization branch between GetUserBySession and s.db.RemoveAllResolvedAlerts() at internal/backend/services/services.go:1205-1217, and NOTIFICATOR_ADMIN_USERS appears nowhere in the tree.

Why it matters: an auto-closed issue drops out of the backlog and out of the implementation queue. The plan would merge, the issue would read as fixed, and any authenticated account would still be able to irreversibly wipe the team's resolved-alert history — exactly the failure #68 was filed for.

Suggested change: replace the closing keyword with a non-closing reference such as "Refs #68" or "Part of #68", so the issue stays open for the implementation PR that carries steps 1-5. If you would rather land plan and code together, fold the implementation into this PR and keep the closing keyword.

---

Blocking 2 — Approach step 4 (spec lines 106-109) misses the client-side half of the WebUI gate

Fallback location: the spec document added by this PR under specs/, Approach step 4, spec lines 106-109.

Problem: step 4 names a single site, dashboard_handlers.go:1878, and says to switch it from canImpersonate(c) to an AdminConfig.IsAdmin check. That handler only guards the POST. Whether the button is rendered at all is decided client-side by a separate flag the step never mentions.

Evidence: the "Remove All Resolved Alerts" button lives in internal/webui/templates/components/modal_components.templ, wrapped in <template x-if="canAdmin"> at line 151, and canAdmin is assigned from window.impersonationState.canImpersonate at line 150. The same assignment appears at internal/webui/templates/layouts/Base.templ:258 (initialized false at line 248), and internal/webui/templates/pages/NewDashboard.templ:223 gates on the same canAdmin. That global is populated from the can_impersonate field of the impersonation status response, which internal/webui/handlers/impersonation_handlers.go:184 computes with canImpersonate(c) — purely from ImpersonationAllowedUsers (impersonation_handlers.go:23-35).

Why it matters: follow step 4 literally and an operator who puts a user on the new admin list but not on the impersonation list gets a backend that authorizes that user and a UI that never shows them the button. The feature becomes unreachable through the WebUI for exactly the configuration this spec tells operators to adopt, which contradicts the Risks table at spec line 123 ("one env var restores it"). The mirror case is worse for review confidence: someone on the impersonation list but absent from the admin list still sees the button and gets a hard denial from the backend — the confusing round trip step 4 says it wants to avoid.

Suggested change: replace step 4 with a two-part instruction. (a) Add an is_admin field to the impersonation status payload at impersonation_handlers.go:184, computed as appConfig.Admin.IsAdmin(user.Username) || appConfig.Admin.IsAdmin(user.Email), carry it through Base.templ as a distinct window.impersonationState.isAdmin, and rebind canAdmin at modal_components.templ:150, Base.templ:258 and NewDashboard.templ:223 to that new flag — leaving canImpersonate to keep meaning impersonation only. (b) Switch dashboard_handlers.go:1878 to the same IsAdmin check. Then add two Validation bullets under the existing list at spec lines 128-141: "With the admin environment variable naming a user who is absent from the impersonation list, that user sees the Remove All Resolved Alerts button and the request succeeds; a user on the impersonation list only does not see the button", and — since (a) edits .templ sources — "make webui-templates regenerated and the generated files committed", which the current Validation list does not mention at all.

---

Non-blocking carry-overs (unchanged spec text, referenced lines have not moved)

1. Approach step 1, spec lines 76-83 — the config-file path for admin.users is dead. Step 1 prescribes a viper default plus env binding on the admin.users key and gives the new field a json:"users" tag, which reads as "you can also set this in the config file". You cannot: LoadConfigWithViper calls viper.Unmarshal(cfg) at config/config.go:263 with no decoder option, so mapstructure's default tag applies and the config structs carry only json tags — a snake_case viper key never matches the Go field name. The proof is in the sibling key: admin.impersonation_allowed_users already has both a default (config/config.go:564) and an env binding (config/config.go:567), yet the only thing that actually populates cfg.Admin.ImpersonationAllowedUsers is the manual environment block at config/config.go:326-336. Combined with the deliberate fail-closed default at spec lines 68-72, an operator who configures the list in config.json gets a silently dead feature — no error, no admin, removal denied for everyone, indistinguishable from "I forgot to configure it". Fix: add a short "Config sources" note under step 1 stating that the environment variable is the only supported source and why, and document that in ENVIRONMENT_VARIABLES.md next to the fail-closed semantics — or widen scope to add mapstructure tags to both admin fields plus a Validation bullet proving a file-only admin list produces a working admin.

2. Approach step 4, spec lines 106-109 — a clean denial reaches the browser as HTTP 500. BackendClient.RemoveAllResolvedAlerts collapses an unsuccessful response into a generic error at internal/webui/client/backend_client.go:579-581, and the handler maps any error to http.StatusInternalServerError at internal/webui/handlers/dashboard_handlers.go:1891-1894 — while the local gate a few lines above at dashboard_handlers.go:1878-1881 correctly returns http.StatusForbidden. So a clean authorization denial surfaces as a 500 reading "Failed to remove resolved alerts: failed to remove all resolved alerts: Admin rights required". Backend and webui load their own config in separate processes, and step 5 asks operators to set the variable in two chart env blocks, so drift is a normal misconfiguration rather than an edge case. Issue #68's first criterion is an explicit denial; delivered as a 500 it looks like an outage, so operators debug the gRPC service instead of their admin list and monitoring counts denials as server errors. Fix: specify a sentinel error such as ErrAdminRequired on the client, map it to 403 with the backend message in the handler, keep 500 for genuine transport or database failures, and add a Validation bullet covering the backend-set/webui-unset configuration in both directions.

3. IsAdmin signature block, spec lines 59-63 — define the empty-input contract. Step 2 at spec line 93 calls IsAdmin on the username or the email, and the email is routinely empty for local accounts. The spec says to mirror CanImpersonate, which at config/config.go:35-42 applies strings.EqualFold with no guard on either side — so a single empty list entry would make every user without an email an administrator of a destructive endpoint. The environment parser at config/config.go:329-334 trims and drops empty segments, but that safety belongs to the parser, not to IsAdmin, and it disappears the moment the list arrives from anywhere else (a config file, a Helm value rendering an empty string, a future loader). This is the one fail-open path in an otherwise carefully fail-closed design, and it is invisible in review because the guard lives in a different function than the one being reviewed. Fix: extend the doc comment to state "Returns false for an empty username or email. Empty or whitespace-only list entries are ignored", implement that guard inside IsAdmin itself, and add a case to the config unit test at spec lines 138-139 asserting an empty identity is never admin even when the list contains an empty entry.

---

The authority decision itself remains well argued and I would not change it: the dedicated admin list, the explicit-not-silent impersonation fallback, and the fail-closed default are all the right calls for a destructive, irreversible endpoint. The candidate-rejection table and the acceptance-criteria mapping are genuinely good spec work — the two blockers are about lifecycle and about one missing surface, not about the design.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

Re-review at head 4ddb581. The diff in scope is exactly one file — the planning document added under specs/, 151 added lines, zero code — and that document is byte-identical to the version reviewed in the previous rounds (every line reference below still lands on the text it describes). The PR body is also unchanged, including its final line. So all five findings from the previous round carry over, and I re-verified each code claim the plan makes against the current tree rather than trusting the earlier pass:

• AdminConfig (config/config.go:30-32) still carries only ImpersonationAllowedUsers, and CanImpersonate (config/config.go:35-42) is still a bare strings.EqualFold loop with no guard on either side.
• RemoveAllResolvedAlerts (internal/backend/services/services.go:1198-1233) still goes straight from GetUserBySession to s.db.RemoveAllResolvedAlerts() with no authorization branch.
• NewAlertServiceGorm has exactly the two call sites the risk table names (internal/backend/server.go:127, internal/backend/services/remove_resolved_alerts_test.go:28).
• Steps 3 and 4 remain feasible as written: the WebUI has a package-level appConfig installed via SetAppConfig (internal/webui/handlers/impersonation_handlers.go:14-20), and canImpersonate there reads only the impersonation list.
• The new admin environment variable name appears nowhere in the tree outside this plan document.

This review reads as blocking. It is submitted with a COMMENT state only because GitHub refuses a changes-requested review on a self-authored pull request.

Two blocking findings, then three non-blocking carry-overs. All are top-level rather than inline: the outbound content-safety gate rejects inline anchors on this PR's only changed path, so each finding names its section and line range instead.

---

Blocking 1 — PR description, final line: the issue-closing keyword

Fallback location: the pull request description, last line.

Problem: the PR body ends with a closing keyword pointing at issue #68, but this PR ships only the plan. Merging it makes GitHub auto-close #68 while the vulnerability described in the plan's own Problem section is still live in main.

Evidence: three QA runs on this branch exercised the endpoint against the running stack — a freshly registered non-admin session called the alert service's remove-all-resolved-alerts RPC over gRPC and got success: true with a non-zero removed count, and the resolved-alert count then read zero. A backend booted with the admin-related environment variables pointed at a different user returned the same unconditional success, because no code reads either variable today. The code side is confirmed above: no authorization branch exists between GetUserBySession and s.db.RemoveAllResolvedAlerts() in internal/backend/services/services.go, and the new variable name is absent from the tree.

Why it matters: an auto-closed issue drops out of the backlog and out of the implementation queue. The plan would merge, the issue would read as fixed, and any authenticated account would still be able to irreversibly wipe the team's resolved-alert history — exactly the failure #68 was filed for.

Suggested change: replace the closing keyword with a non-closing reference such as "Refs #68" or "Part of #68", so the issue stays open for the implementation PR that carries steps 1-5. If you would rather land plan and code together, fold the implementation into this PR and keep the closing keyword.

---

Blocking 2 — Approach step 4 (spec lines 106-109) misses the client-side half of the WebUI gate

Fallback location: the planning document added by this PR under specs/, Approach step 4, spec lines 106-109.

Problem: step 4 names a single site, the dashboard handler at line 1878, and says to switch it from canImpersonate(c) to an AdminConfig.IsAdmin check. That handler only guards the POST. Whether the button is rendered at all is decided client-side by a separate flag the step never mentions.

Evidence: the "Remove All Resolved Alerts" button lives in internal/webui/templates/components/modal_components.templ, wrapped in <template x-if="canAdmin"> at line 151, and canAdmin is assigned from window.impersonationState.canImpersonate at line 150. The same assignment appears at internal/webui/templates/layouts/Base.templ:258 (initialized false at line 248), and internal/webui/templates/pages/NewDashboard.templ:223 gates on the same canAdmin. That global is populated from the can_impersonate field of the impersonation status response, which internal/webui/handlers/impersonation_handlers.go:184 computes with canImpersonate(c) — purely from ImpersonationAllowedUsers (impersonation_handlers.go:22-35).

Why it matters: follow step 4 literally and an operator who puts a user on the new admin list but not on the impersonation list gets a backend that authorizes that user and a UI that never shows them the button. The feature becomes unreachable through the WebUI for exactly the configuration this plan tells operators to adopt, which contradicts the Risks table at spec line 123 ("one env var restores it"). The mirror case is worse for review confidence: someone on the impersonation list but absent from the admin list still sees the button and gets a hard denial from the backend — the confusing round trip step 4 says it wants to avoid.

Suggested change: replace step 4 with a two-part instruction. (a) Add an is_admin field to the impersonation status payload at impersonation_handlers.go:184, computed as appConfig.Admin.IsAdmin(user.Username) || appConfig.Admin.IsAdmin(user.Email), carry it through Base.templ as a distinct window.impersonationState.isAdmin, and rebind canAdmin at modal_components.templ:150, Base.templ:258 and NewDashboard.templ:223 to that new flag — leaving canImpersonate to keep meaning impersonation only. (b) Switch the dashboard handler at line 1878 to the same IsAdmin check. Then add two Validation bullets under the existing list at spec lines 128-141: "With the admin environment variable naming a user who is absent from the impersonation list, that user sees the Remove All Resolved Alerts button and the request succeeds; a user on the impersonation list only does not see the button", and — since (a) edits .templ sources — "make webui-templates regenerated and the generated files committed", which the current Validation list does not mention at all.

---

Non-blocking carry-overs (unchanged plan text, referenced lines have not moved)

1. Approach step 1, spec lines 76-83 — the config-file path for the admin list is dead. Step 1 prescribes a viper default plus env binding on the admin.users key and gives the new field a json:"users" tag, which reads as "you can also set this in the config file". You cannot: LoadConfigWithViper calls viper.Unmarshal(cfg) at config/config.go:263 with no decoder option, so mapstructure's default tag applies and the config structs carry only json tags — a snake_case viper key never matches the Go field name. The proof is in the sibling key: admin.impersonation_allowed_users already has both a default (config/config.go:564) and an env binding (config/config.go:567), yet the only thing that actually populates cfg.Admin.ImpersonationAllowedUsers is the manual environment block at config/config.go:326-336. Combined with the deliberate fail-closed default at spec lines 68-72, an operator who configures the list in config.json gets a silently dead feature — no error, no admin, removal denied for everyone, indistinguishable from "I forgot to configure it". Fix: add a short "Config sources" note under step 1 stating that the environment variable is the only supported source and why, and document that in ENVIRONMENT_VARIABLES.md next to the fail-closed semantics — or widen scope to add mapstructure tags to both admin fields plus a Validation bullet proving a file-only admin list produces a working admin.

2. Approach step 4, spec lines 106-109 — a clean denial reaches the browser as HTTP 500. BackendClient.RemoveAllResolvedAlerts collapses an unsuccessful response into a generic error at internal/webui/client/backend_client.go:579-581, and the handler maps any error to http.StatusInternalServerError at internal/webui/handlers/dashboard_handlers.go:1891-1894 — while the local gate a few lines above at dashboard_handlers.go:1878-1881 correctly returns http.StatusForbidden. So a clean authorization denial surfaces as a 500 reading "Failed to remove resolved alerts: failed to remove all resolved alerts: Admin rights required". Backend and webui load their own config in separate processes, and step 5 asks operators to set the variable in two chart env blocks, so drift is a normal misconfiguration rather than an edge case. Issue #68's first criterion is an explicit denial; delivered as a 500 it looks like an outage, so operators debug the gRPC service instead of their admin list and monitoring counts denials as server errors. Fix: specify a sentinel error such as ErrAdminRequired on the client, map it to 403 with the backend message in the handler, keep 500 for genuine transport or database failures, and add a Validation bullet covering the backend-set/webui-unset configuration in both directions.

3. IsAdmin signature block, spec lines 59-63 — define the empty-input contract. Step 2 at spec line 93 calls IsAdmin on the username or the email, and the email is routinely empty for local accounts. The plan says to mirror CanImpersonate, which at config/config.go:35-42 applies strings.EqualFold with no guard on either side — so a single empty list entry would make every user without an email an administrator of a destructive endpoint. The environment parser at config/config.go:326-336 trims and drops empty segments, but that safety belongs to the parser, not to IsAdmin, and it disappears the moment the list arrives from anywhere else (a config file, a Helm value rendering an empty string, a future loader). This is the one fail-open path in an otherwise carefully fail-closed design, and it is invisible in review because the guard lives in a different function than the one being reviewed. Fix: extend the doc comment to state "Returns false for an empty username or email. Empty or whitespace-only list entries are ignored", implement that guard inside IsAdmin itself, and add a case to the config unit test at spec lines 138-139 asserting an empty identity is never admin even when the list contains an empty entry.

---

The authority decision itself remains well argued and I would not change it: the dedicated admin list, the explicit-not-silent impersonation fallback, and the fail-closed default are all the right calls for a destructive, irreversible endpoint. The candidate-rejection table and the acceptance-criteria mapping are genuinely good planning work — the two blockers are about lifecycle and about one missing surface, not about the design.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🧪 QA Report

Tested against a live make test stack (postgres + backend + webui + fake alertmanager), built from this branch. The diff contains exactly one file — specs/2026-07-25-68-fix-backend-restrict-removeallresolvedalerts.md — and no Go code, yet the PR body states Closes #68. Criteria below are the acceptance criteria of #68, exercised against the running app.

Criterion	Result	Evidence
A valid non-admin session calling RemoveAllResolvedAlerts is denied (no deletion, explicit 'admin required' message)	❌	Registered qanonadmin via notificator.auth.AuthService/Register, logged in, seeded 2 resolved alerts (CreateResolvedAlert, GetResolvedAlerts → totalCount: 2). grpcurl -plaintext -d '{"session_id":"<non-admin session>"}' localhost:50051 notificator.alert.AlertService/RemoveAllResolvedAlerts → {"success": true, "removedCount": 2, "message": "Successfully removed 2 resolved alerts"}; recount → Found 0 resolved alerts. The whole history was wiped by a plain user.
A valid admin session succeeds and deletes as before	❌	Cannot be exercised: no admin notion exists for this RPC at this HEAD. config.AdminConfig still has only ImpersonationAllowedUsers (config/config.go:30-33) — there is no Users field and no IsAdmin. Every valid session is treated identically (see row above).
Admin determination reuses an existing/config-driven mechanism, documented	❌	Decided in the spec (dedicated AdminConfig.Users + IsAdmin, impersonation list as an explicit documented fallback — a sound and well-argued choice), but not implemented. grep -rn "IsAdmin" --include=*.go returns nothing; NewAlertServiceGorm (services.go:437) takes no config.
Env var / config surface for the admin list documented	❌	grep -i ADMIN finds 0 matches in ENVIRONMENT_VARIABLES.md, .env.example, and charts/notificator-app/values.yaml (only an unrelated Google OAuth scope comment). NOTIFICATOR_ADMIN_USERS does not exist.
Existing session-validation behaviour from #65 is preserved	✅	session_id: "" → {"message": "Session ID is required"}; session_id: "totally-bogus-session" → {"message": "Invalid session"}. Both still deny, no deletion. go test ./internal/backend/services/... → ok.

Exploratory: adjacent flows are intact (nothing can regress — zero code changed): webui login via /api/v1/auth/login → 200, and the WebUI UX gate still refuses non-admins (DELETE /api/v1/dashboard/remove-resolved-alerts → 403 "You are not allowed to remove resolved alerts" via canImpersonate). That 403 is exactly the false sense of safety #68 describes — the gRPC trust boundary underneath it is wide open, as demonstrated above.

On the spec itself: it is accurate and implementable — I verified its code references (services.go:1198 / GetUserBySession at 1206, config/config.go:30-41, impersonation_handlers.go:34, dashboard_handlers.go:1877-1878, exactly 2 NewAlertServiceGorm call sites, existing RejectsMissingSession / RejectsInvalidSession / ValidSessionDeletes tests). The problem is not the plan's quality, it is that Closes #68 on a plan-only PR will auto-close the issue and leave the destructive endpoint unguarded in main.

Suggested resolution: either implement the spec's steps 1-5 in this PR, or change Closes #68 to Refs #68 so the issue stays open for the implementation PR.

Verdict: FAIL

Summoning the fixer loop to pick this up.

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

🧪 QA Report

Re-test after the rebase (head ebb297a). Stack rebuilt from this branch with make test
(postgres + backend + webui + fake alertmanager, all healthy). The diff versus main is still
exactly one file — specs/2026-07-25-68-fix-backend-restrict-removeallresolvedalerts.md,
151 added lines, zero Go/templ/docs/chart changes — while the PR body says Closes #68.
Criteria below are the acceptance criteria of #68, exercised against the running app.

Criterion	Result	Evidence
A valid non-admin session calling RemoveAllResolvedAlerts is denied (no deletion, explicit 'admin required' message)	❌	Registered qanonadmin (notificator.auth.AuthService/Register + Login), seeded 2 resolved alerts (CreateResolvedAlert; GetResolvedAlerts → Found 2 resolved alerts). grpcurl -plaintext -d '{"session_id":"<non-admin session>"}' localhost:50051 notificator.alert.AlertService/RemoveAllResolvedAlerts → {"success": true, "removedCount": 2, "message": "Successfully removed 2 resolved alerts"}; recount → Found 0 resolved alerts. A plain user wiped the whole team history.
A valid admin session succeeds and deletes as before	❌	Not exercisable — no admin notion exists for this RPC at this head. services.go:1202-1237 goes straight from GetUserBySession to s.db.RemoveAllResolvedAlerts(); AdminConfig still has only ImpersonationAllowedUsers (config/config.go:30-32), no Users, no IsAdmin. Proof the knob is inert: booted a second backend from the same image with NOTIFICATOR_ADMIN_USERS=qanonadmin (docker compose run -e … -p 50052:50051 backend), then used a different user (qaother, absent from that list) → {"success": true, "removedCount": 1}. Admin and non-admin sessions are indistinguishable.
Admin determination reuses an existing/config-driven mechanism, documented	❌	Decided in the spec (dedicated AdminConfig.Users + IsAdmin, impersonation list as an explicit fallback — sound reasoning), not implemented. rg 'IsAdmin|NOTIFICATOR_ADMIN_USERS' outside specs/ → 0 hits; NewAlertServiceGorm still takes no config.
Env var / config surface for the admin list documented	❌	0 admin matches in ENVIRONMENT_VARIABLES.md and .env.example; the only hits in charts/notificator-app/values.yaml:89 / README.md:208 are the unrelated Google OAuth admin.directory.group.readonly scope. NOTIFICATOR_ADMIN_USERS is undocumented and unread.
Existing session-validation behaviour from #65 is preserved	✅	session_id: "" → {"message": "Session ID is required"}; session_id: "totally-bogus-session" → {"message": "Invalid session"}; neither deletes. go test ./internal/backend/services/... ./config/... → 10 passed.

Exploratory: no regression is structurally possible (zero code changed), and the adjacent flows
confirm it live — Playwright login as qanonadmin reaches /dashboard (14 alerts rendered), the
Resolved tab loads (Showing 1 of 1 resolved alerts), and the Remove All Resolved button is
correctly absent for this user (window.impersonationState.canImpersonate === false); the HTTP
route refuses it too (DELETE /api/v1/dashboard/remove-resolved-alerts → 403 "You are not allowed to remove resolved alerts", resolved count unchanged). That WebUI 403 is exactly the false sense of
safety #68 describes: the gRPC trust boundary underneath is wide open, as the first row shows.
Two pre-existing 401s at dashboard load (/api/v1/profile/timezone, /api/impersonate/status) are
unrelated to this PR.

On the spec itself: still accurate and implementable — its code references check out
(services.go:1202 / GetUserBySession at 1210, config/config.go:30-41, dashboard_handlers.go
canImpersonate gate, 2 NewAlertServiceGorm call sites) and the authority decision is well
argued. The QA problem is unchanged by the rebase: Closes #68 on a plan-only PR auto-closes the
issue and leaves the destructive endpoint unguarded on main.

Suggested resolution: implement steps 1-5 in this PR, or change Closes #68 → Refs #68 so the
issue stays open for the implementation PR.

Verdict: FAIL

Summoning the fixer loop to pick this up.

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

🧪 QA Report

Environment: make test (full docker-compose rebuild — postgres + fake alertmanager + backend :50051/:8080 + webui :8081, all healthy), booted with NOTIFICATOR_ADMIN_USERS=qa_admin on the backend. Criteria taken from issue #68 (Closes #68).

What changed since the last QA run (f5710ce): the PR was rebased and this head now contains 23693e7 fix(backend): restrict RemoveAllResolvedAlerts to admins (#113). The gate the spec plans is present in the tree under test — so the criteria that were previously unverifiable are now exercisable, and all of them were exercised live at this SHA.

Criterion	Result	Evidence
A valid non-admin session calling RemoveAllResolvedAlerts is denied (no deletion, explicit "admin required" message)	✅	Registered + logged in qa_nonadmin, seeded 3 resolved alerts (GetResolvedAlerts → totalCount: 3), called notificator.alert.AlertService/RemoveAllResolvedAlerts over gRPC → {"message":"Admin rights required"}, success absent/false. Count after = 3, untouched.
A valid admin session succeeds and deletes as before	✅	Same 3 seeded alerts, same RPC with qa_admin's session (the user listed in NOTIFICATOR_ADMIN_USERS) → {"success":true,"removedCount":3,"message":"Successfully removed 3 resolved alerts"}; count after = 0.
Admin determination reuses an existing/config-driven mechanism (documented), impersonation list not silently reused	✅	Three live configurations against the same postgres: (a) NOTIFICATOR_ADMIN_USERS unset + NOTIFICATOR_ADMIN_IMPERSONATION_ALLOWED_USERS=qa_nonadmin → that user still gets Admin rights required, 2 seeded alerts untouched ⇒ impersonation grants nothing, fail-closed. (b) NOTIFICATOR_ADMIN_USERS=QA_NONADMIN@EXAMPLE.COM + impersonation list =qa_admin → qa_admin (impersonation only) denied, count still 2; qa_nonadmin (uppercase email match) allowed, removedCount: 2. Confirms the authority is admin.users alone, matched case-insensitively on username or email. (c) baseline stack, NOTIFICATOR_ADMIN_USERS=qa_admin, as rows 1-2.
Env var / config surface for the admin list is documented	✅	ENVIRONMENT_VARIABLES.md:56-64 — new "Admin Configuration" section documenting NOTIFICATOR_ADMIN_USERS and the previously-undocumented NOTIFICATOR_ADMIN_IMPERSONATION_ALLOWED_USERS, with the "empty ⇒ nobody" semantics and the distinction between the two. .env.example:11-13. charts/notificator-app/values.yaml:65. The documented semantics match the observed runtime behaviour exactly.
Existing session-validation behaviour from #65 preserved	✅	session_id: "" → {"message":"Session ID is required"}; session_id: "deadbeefnotasession" → {"message":"Invalid session"}. Resolved-alert count after both = 3, unchanged. Denial ordering is correct: session errors still fire before the admin check.

Exploratory: No adjacent breakage. Drove a real Chromium (Playwright) through the full login flow for both users — POST /api/v1/auth/login → 200 → redirect to /dashboard, which renders the alert table, severity counters (Critical/Warning/Info/Total) and all six filter dropdowns intact for non-admin and admin alike; Silences/Statistics/Resolved tabs present, no new console errors beyond the pre-login 401s. Seeding via CreateResolvedAlert and reading via GetResolvedAlerts are unaffected by the gate — only the destructive RPC is gated. One pre-existing mismatch worth noting (from #113, not introduced by this diff): the WebUI handler internal/webui/handlers/dashboard_handlers.go:1914 still gates on canImpersonate(c), so qa_admin — a backend admin via NOTIFICATOR_ADMIN_USERS — gets 403 {"success":false,"error":"You are not allowed to remove resolved alerts"} from the browser and window.impersonationState.canImpersonate === false. An operator who sets only NOTIFICATOR_ADMIN_USERS gets a working gRPC path but a UI button that refuses. This is exactly step 4 of the spec under review ("switch the WebUI gate from canImpersonate to AdminConfig.IsAdmin"), which is not implemented. The trust boundary itself is correct — the backend denies independently — so this is UX, not a security hole, and it is outside issue #68's criteria.

Verdict: PASS

Two notes on the spec's accuracy vs. what shipped

Not criterion failures — flagging so the merged spec isn't read as a description of current behaviour:

1. The documented IsAdmin fallback does not exist. Spec lines 59-62 and 68-72 state that IsAdmin falls back to ImpersonationAllowedUsers when admin.users is empty. config/config.go:49-56 has no fallback, and configuration (a) above proves it empirically: impersonation-listed user, empty admin list, still denied. ENVIRONMENT_VARIABLES.md agrees with the code, not with the spec. The shipped behaviour is the stricter and better one — the spec paragraph is just stale.
2. Step 4 (WebUI alignment) is unimplemented, per the exploratory finding above.

Neither blocks this PR: #68 is already closed and the implementation landed in #113, so merging this planning spec changes no runtime behaviour. A one-line edit to the fallback paragraph would make the merged spec match reality.

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

🧪 QA Report

Environment: make test at this head (full docker-compose rebuild; postgres + fake alertmanager + backend :50051/:8080 + webui :8081, all healthy). Criteria from issue #68 (Closes #68).

What changed since the last two QA runs (543af2a, f5710ce): the rebase moved this branch onto a main that now contains 23693e7 fix(backend): restrict RemoveAllResolvedAlerts to admins (#113). The diff of this PR is still one file (specs/2026-07-25-68-...md, 151 lines, zero code), but the gate it plans is now live in the base, so #68's criteria are finally exercisable — and they pass. #68 itself is already CLOSED, so the earlier Closes #68 scope objection is moot.

Criterion	Result	Evidence
A valid non-admin session is denied (no deletion, explicit "admin required" message)	✅	Backend booted with NOTIFICATOR_ADMIN_USERS=qa_admin87. Registered + logged in qa_nonadmin87, seeded 3 resolved alerts (GetResolvedAlerts → totalCount: 3), called notificator.alert.AlertService/RemoveAllResolvedAlerts with that session → {"message": "Admin rights required"}, no success. Count after = 3, nothing deleted. Same through the real UI (Playwright/Chromium, settings modal → "remove all resolved alerts" → confirm dialog accepted): DELETE /api/v1/dashboard/remove-resolved-alerts → 500 {"success":false,"error":"...: Admin rights required"}, count 4 → 4.
A valid admin session succeeds and deletes as before	✅	Same run, qa_admin87 session → {"success": true, "removedCount": 3, "message": "Successfully removed 3 resolved alerts"}, count 3 → 0. Real browser as qa_admin87: same button + confirm → 200 {"success":true,...,"All resolved alerts have been removed successfully"}, count 4 → 0.
Admin determination reuses an existing/config-driven mechanism, documented; impersonation list not silently reused	✅	Authority is AdminConfig.Users ← NOTIFICATOR_ADMIN_USERS, decision written up in the PR body. Proved not overloaded: qa_nonadmin87 was in NOTIFICATOR_ADMIN_IMPERSONATION_ALLOWED_USERS=qa_admin87,qa_nonadmin87 and was still denied (row 1). Restarted with NOTIFICATOR_ADMIN_USERS unset and both users impersonation-listed → both got Admin rights required, count 3 → 3 (fail-closed). Restarted with NOTIFICATOR_ADMIN_USERS=QA_NONADMIN87@EXAMPLE.COM → that user deleted 3 (email match + case-insensitive both work).
Env var / config surface documented	✅	.env.example:13 (NOTIFICATOR_ADMIN_USERS=, commented semantics), ENVIRONMENT_VARIABLES.md:58 (new "Admin Configuration" section, plus the previously-undocumented impersonation var), charts/notificator-app/values.yaml:65 (commented example).
Existing session-validation behaviour from #65 preserved	✅	session_id: "" → {"message": "Session ID is required"}; session_id: "deadbeefnotasession" → {"message": "Invalid session"}. Count unchanged at 3 in both cases.

Exploratory: No adjacent breakage — reads are untouched (GetResolvedAlerts → success: true, totalCount: 2 and GetResolvedAlert → "Resolved alert found" for a non-admin session), login/dashboard/settings modal all render fine for both users. Two gaps found in the merged implementation (main, via #113), not in this PR's diff: (1) with NOTIFICATOR_ADMIN_IMPERSONATION_ALLOWED_USERS empty and NOTIFICATOR_ADMIN_USERS=qa_admin87, the button is not rendered at all for the configured admin — the WebUI still gates on canImpersonate (dashboard_handlers.go:1914), so the documented env var alone gives no UI path (the same admin still deleted 2 over gRPC); (2) the backend denial surfaces as HTTP 500 with no visible error banner (actionError stayed null) instead of a 403 with a message.

Verdict: PASS

Notes for the record

The five acceptance criteria all pass live, so this is a PASS. Two spec-vs-shipped divergences are worth folding into a follow-up rather than blocking a docs-only merge — the plan doc being merged now describes behaviour the code does not have:

1. Spec §"Admin authority": "if admin.users is empty, IsAdmin consults ImpersonationAllowedUsers". Not implemented — IsAdmin (config/config.go) only reads Users. Verified empirically above: impersonation-listed users are denied when admin.users is unset. Only the fail-closed half of that paragraph is true.
2. Spec approach step 4: "switch [the WebUI gate] from canImpersonate(c) to an admin check based on AdminConfig.IsAdmin". Not done — hence exploratory finding (1). Operators must set both env vars for the feature to be reachable from the UI, which nothing documents.

Either amend those two paragraphs to match what shipped, or open a follow-up issue for the WebUI alignment + 403 mapping.

Env note for the next agent: make test fails with templ: No such file or directory (Error 127, Makefile:43) unless templ is on PATH — go install github.com/a-h/templ/cmd/templ@latest first, and note make clean deletes the generated *_templ.go files (38 tracked) before regenerating them.