T10.5 closed, not built - the required group stays an env var
Proposed, examined, rejected. The wave file now records why at the task, so
nobody reads the original and assumes it was merely skipped.
Three honest points, in order of weight:
The console requirement was invented in the wave file, not asked for. D13
criterion 3 says "An AD group is configured" - not "configurable from the
console". LDAP_REQUIRED_GROUP satisfies the criterion as written.
The validate-on-save guard existed only to defend against a risk the console
itself introduced. A feature whose complexity exists to defend against itself
is usually the wrong feature.
And the lockout it defended against is already handled. Verified rather than
asserted: a group that does not resolve raises LookupError in member_of, which
verify() maps to GROUP_NOT_FOUND, which is_config_problem classifies as ours -
so login() answers 503 and the log reads "required group 'X' does not resolve
in DC=prime,DC=local - refusing the sign-in. This is a configuration fault, not
a bad password." A genuine non-member still gets 401. Already distinguishable.
Accepted losses, both recorded: an app admin cannot see the required group
without Portainer or shell access (a read-only diagnostic line was offered and
declined as unnecessary), and nothing validates the group until the first
sign-in attempt - which is unfixable, since resolving a group needs an
authenticated search, anonymous bind is disabled here, and there is no service
account by design.
Also logged two full-suite failures as backlog rather than letting them read as
D13 fallout. Both proven pre-existing: git diff main...HEAD shows neither file
touched by this branch.
BL-028 assets_check asserts "no MICRON_DB_URL" but start_server passes the
ambient environment through, so it fails on any machine whose .env
sets it. Same class of bug T10.7 fixed for LDAP_REQUIRED_GROUP.
BL-029 generalinfo_check flags rgba(20,30,50,.18) in the creator stylesheet,
last touched by 8efe624 (F6). Needs C4's rgba-alpha exception
interpreted before anyone decides whether the literal or the check is
the wrong one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -590,3 +590,36 @@ deliberately deferred.
|
||||
`CLAUDE.md` forbids. Recording it is not the same as reopening it.
|
||||
- **Suggested wave or follow-up:** its own item and its own decision, with Nick and
|
||||
whoever administers the Okta tenant. Not a widening of D13.
|
||||
|
||||
### BL-028 — `assets_check` fails on any machine that has `MICRON_DB_URL` set
|
||||
|
||||
- **Found during:** `T10.7` (D13), running the full suite
|
||||
- **Where:** `tests/assets_check.py`, the "no `MICRON_DB_URL`" case
|
||||
- **What:** the check asserts `/api/assets` answers `configured:false` when the catalog
|
||||
is not configured, but `start_server` passes the ambient environment through. On a
|
||||
developer machine whose `.env` sets `MICRON_DB_URL` — which is the normal state for
|
||||
anyone who has ever used the asset picker — the API is genuinely configured, returns
|
||||
real Micron tags, and three checks fail. Nothing is wrong with the app; the test's
|
||||
premise is violated by the environment it runs in.
|
||||
- **Fix:** pop `MICRON_DB_URL` from the env for that server, exactly as `start_server`
|
||||
now pops `LDAP_REQUIRED_GROUP` for the same reason (`T10.7`).
|
||||
- **Why not now:** it is not this wave's defect and the fix belongs with whoever owns
|
||||
the asset picker's tests. Recorded so the failure is not mistaken for D13 fallout.
|
||||
- **Suggested wave or follow-up:** next housekeeping pass.
|
||||
|
||||
### BL-029 — `generalinfo_check` flags a pre-existing `rgba()` in the creator stylesheet
|
||||
|
||||
- **Found during:** `T10.7` (D13), running the full suite
|
||||
- **Where:** `html/wp-creation-styles.css:929` — `box-shadow:0 8px 24px rgba(20,30,50,.18)`
|
||||
- **What:** `generalinfo_check`'s token-rule check reports "no colour literal was added
|
||||
to the creator's stylesheet" and fails on `rgba(`. The literal predates this wave —
|
||||
last touched by `8efe624` (F6) — and `git diff main...HEAD` shows the file untouched
|
||||
by the LDAPS branch.
|
||||
- **The real question is which is wrong.** `C4`'s recorded exception allows rgba
|
||||
**alphas** as opacity recipes, which is arguably what a shadow is; if so the check is
|
||||
too strict and should match a colour literal rather than the `rgba(` token. If not,
|
||||
the shadow needs a token. Either way it is a one-line change plus a decision, and
|
||||
the decision is not this wave's to make.
|
||||
- **Why not now:** drive-by fixes to the token system are what `CLAUDE.md` forbids, and
|
||||
this one needs the C4 exception interpreted rather than guessed.
|
||||
- **Suggested wave or follow-up:** next housekeeping pass, with `C4` re-read first.
|
||||
|
||||
@@ -188,38 +188,48 @@ creating it is exactly the kind of event that record exists for.
|
||||
|
||||
---
|
||||
|
||||
### T10.5 — The required group is configurable
|
||||
### T10.5 — CLOSED, NOT BUILT (Aug 24 2026): the required group stays an env var
|
||||
|
||||
- **Items:** `D13` (3)
|
||||
- **Depends on:** T10.1
|
||||
- **Blocks:** T10.8
|
||||
- **Surface:** `server/` + `html/`
|
||||
- **Files:** `server/notify.py` (`DEFAULTS`), `server/app.py` (`SettingsIn`, `/api/settings`),
|
||||
`html/admin.js`
|
||||
- **Status:** **won't build.** `LDAP_REQUIRED_GROUP` in the environment is the answer.
|
||||
|
||||
**Problem:** The group has to be settable without a redeploy, but it gates login — a typo
|
||||
in a text box locks the whole company out, including whoever typed it.
|
||||
**Do not build this later by reading the original task and assuming it was skipped.**
|
||||
It was proposed, examined and rejected on purpose, and the reasoning is below.
|
||||
|
||||
**Do:** Add `ldap_required_group` to `notify.DEFAULTS` and `SettingsIn`, and a field in the
|
||||
Admin console's settings card next to the notification block. `LDAP_REQUIRED_GROUP` from the
|
||||
environment is the initial value and the fallback when the setting is empty.
|
||||
**What it was going to be:** the required group moved out of the environment into an
|
||||
Admin console setting, with a validate-on-save guard that resolved the group in the
|
||||
directory and confirmed the saving admin was a member.
|
||||
|
||||
**Validate on save, not on login.** Before persisting, resolve the group in the directory and
|
||||
confirm the saving admin is themselves a member. Refuse the save with a specific message if
|
||||
either check fails. That single guard is what stops the lockout scenario, so it is not
|
||||
optional. Do not add it to `notify.PUBLIC_KEYS` — an unauthenticated caller must not be able
|
||||
to read the group name off the login page.
|
||||
**Why it is not being built:**
|
||||
|
||||
**Done when:**
|
||||
1. **The console requirement was invented here, not asked for.** D13 criterion 3 says
|
||||
*"An AD group is configured"* — not "configurable from the console". The env var
|
||||
satisfies the criterion as written.
|
||||
2. **The validate-on-save guard existed only to defend against a risk the console
|
||||
itself introduced.** A feature whose complexity exists to defend against itself is
|
||||
usually the wrong feature.
|
||||
3. **The lockout scenario it defended against is already handled.** A group that does
|
||||
not resolve raises `LookupError` in `member_of`, which `verify()` maps to
|
||||
`GROUP_NOT_FOUND`, which `is_config_problem` classifies as ours — so `login()`
|
||||
answers **503**, not 401, and the log says *"required group 'X' does not resolve in
|
||||
DC=prime,DC=local — refusing the sign-in. This is a configuration fault, not a bad
|
||||
password."* A genuine non-member still gets 401. The two are already distinguishable
|
||||
in both the log and the response.
|
||||
4. **A redeploy is deliberate and reviewable; a text box is not.** The group is set
|
||||
once and effectively never changes — it is not SMTP configuration.
|
||||
5. **The console version creates a circular failure.** Fixing a lockout would require
|
||||
the console the lockout prevents you from reaching. Editing the env var does not.
|
||||
|
||||
- [ ] the field appears in the Admin console and persists across a restart
|
||||
- [ ] saving a group that does not exist is refused, with a message naming the group
|
||||
- [ ] saving a group the current admin is not in is refused
|
||||
- [ ] an empty setting falls back to `LDAP_REQUIRED_GROUP`, and empty-with-no-env means no group gate (documented, logged at startup)
|
||||
- [ ] the group name is absent from `/api/settings` for a non-admin and from any pre-login response
|
||||
- [ ] the field is reachable by keyboard, labelled, and announces its save state through the existing `aria-live` region (`C1`)
|
||||
**What is genuinely lost, and accepted:**
|
||||
|
||||
---
|
||||
- **Discoverability.** An app admin cannot see which group is required without
|
||||
Portainer or shell access. A read-only line in the Admin console diagnostics would
|
||||
give the useful half without the dangerous half; it was offered and declined on
|
||||
Aug 24 as not needed.
|
||||
- **Deploy-time validation.** Nothing confirms the group resolves until the first
|
||||
sign-in attempt. This is not fixable: resolving a group needs an authenticated
|
||||
search, anonymous bind is disabled on this estate, and there is no service account
|
||||
by design. The first-attempt 503 is the earliest possible detection.
|
||||
|
||||
### T10.6 — Strip the password UI
|
||||
|
||||
|
||||
Reference in New Issue
Block a user