diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..61502ef --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,2 @@ +reviews: + path_filters: ["!.planning/**"] diff --git a/.planning/PROJECT.md b/.planning/PROJECT.md index fdd8849..31a98cf 100644 --- a/.planning/PROJECT.md +++ b/.planning/PROJECT.md @@ -53,25 +53,40 @@ A second factor that actually holds for in-site users, and that can be deployed `X-Forwarded-For`, blank line in the IP whitelist) were found by code review and fixed with regression tests, so the plugin is fail-closed rather than fail-crashed +**Registry seeding** — *Validated in Phase 2: Registry Seeding and Import-Step Ordering (2026-07-29)* + +- ✓ `Interface ... IGoogleAuthenticatorSettings defines a field ska_secret_key, for which there + is no record` is fixed on new Plone site creation. `` + declared at `configure.zcml:50`, and the nested `runImportStepFromProfile` re-entry is gone + from `src/`. Confirmed against a real site-creation log (UAT 2026-07-29): zero `no record` + lines — REG-01, REG-02, REG-04 +- ✓ The ordering is asserted rather than accidental, and the assertion is a *genuine* control: + `test_import_step_declares_registry_dependency` asserts the pre-sort + `getImportStepMetadata(...)['dependencies']` and was reproduced failing when the `` + line is deleted. The older `test_import_step_ordering` is kept as the outcome check with a + docstring that admits it proves nothing alone — REG-03 +- ✓ `ska_secret_key` is seeded once at install time by `setuphandlers._setup_secret_key()`; + `get_ska_secret_key()` is a pure read that raises `ValueError` on an empty key (fail-closed, + no plaintext-equivalent fallback, no lazy mint on a `transaction.abort()` path). Re-applying + the profile leaves an existing key unchanged — REG-04, REG-05 +- ✓ Derived `ska` key components are separated by a length-prefixed netstring join, so two + component tuples that collide under bare concatenation now derive to different keys. + Asserted with an exact-string check on a provably-colliding fixture — BUG-04 + ### Active **Correctness** -- [ ] Fix `Interface ... IGoogleAuthenticatorSettings defines a field ska_secret_key, for which - there is no record` on new Plone site creation — via `` - and removing the nested `runImportStepFromProfile`, **not** via the rename. The root cause - is Python 2 `set` iteration order over import-step ids, so the rename changes a hash and - may make the error vanish without fixing it. An ordering assertion in the test suite is the - actual control -- [ ] `bin/code-analysis` exits 0 (~40 pre-existing findings; the buildout installs a - pre-commit hook that fails every commit until this is clean) +- [ ] `bin/code-analysis` exits 0. The corrected baseline is **318 pre-existing findings**, not + the ~40 previously recorded here — measured in plan 01-03 (RESEARCH C-6). 184 of the 318 + (58%) are `isort` findings, and the rename actively perturbs first-party import ordering, + so QUAL-06 must be planned against 318. The buildout installs a pre-commit hook that fails + every commit until this is clean (`--no-verify` in the meantime) - [ ] Fix open redirect: `next_url` accepted unvalidated at `token.py:112-113` - [ ] Fix `UnboundLocalError` on `redirect_url` at `user_setup.py:96` - [ ] Use a constant-time comparison for the reset token at `reset_bar_code.py:104` — encoding both sides first, because `hmac.compare_digest` raises `TypeError` across `str`/`unicode` and the stored and submitted values differ in type -- [ ] Separate the components of the derived `ska` key at `helpers.py:259` (currently bare - concatenation, collidable) - [ ] Swap `py2-ipaddress` for `ipaddress == 1.0.23` with `unicode` coercion at `helpers.py:459` and `:496`. Forced by adding `cryptography`, which pulls the `ipaddress` backport — both distributions install a top-level module of the same name, and the backport raises @@ -266,6 +281,9 @@ enumerates the bugs, security gaps, and test-coverage holes referenced above. | Recovery codes hashed with one salt per user, not per code | A per-code salt forces N hash runs per attempt (~1.1s for 10 codes) on a login-adjacent endpoint — a DoS lever. Per-user still defeats cross-user rainbow tables, which is all a salt does here | — Pending | | No upgrade steps for the rename; existing dev ZODBs discarded | No enrolled users to migrate, and pickled module paths make in-place migration far more work than recreating a dev database | — Pending | | Don't rename `PAS_ID` (`google_auth`) | Already namespace-neutral; renaming it would create a second plugin on any existing ZODB | — Pending | +| Seed `ska_secret_key` at install time in `setuphandlers._setup_secret_key()`, **not** lazily on first read | Reverses the 02-01 plan's D-04/D-05 lazy-mint design (CR-02). A mint inside `get_ska_secret_key()` is reachable from `authenticateCredentials()`, a path that ends in `transaction.abort()` on `Unauthorized` — it would discard the key *after* a signed URL using it was already handed to the browser. `get_ska_secret_key()` is now a pure read that raises `ValueError` on an empty key | ✓ Shipped Phase 2 | +| Derive the `ska` key with a length-prefixed netstring join, not bare concatenation | Bare concatenation of `(user_secret, browser_hash, ska_secret_key)` is collidable: a component-boundary shift yields the same key, so a signature minted in one context validates in another. Asserted with an exact-string check on a fixture that provably collides under the old scheme, so it cannot regress into a cosmetic reformat | ✓ Shipped Phase 2 | +| Assert the `` *declaration*, not just the resulting sorted order | The first ordering test was tautological — it stayed green with `` deleted, purely by CPython 2.7 string-hash coincidence. `test_import_step_declares_registry_dependency` asserts the pre-sort `getImportStepMetadata(...)['dependencies']` instead, and was reproduced failing on deletion. The outcome test is kept, with a docstring admitting it proves nothing alone | ✓ Shipped Phase 2 | ## Evolution @@ -285,4 +303,4 @@ This document evolves at phase transitions and milestone boundaries. 4. Update Context with current state --- -*Last updated: 2026-07-29 — Phase 1 complete (rename + fail-closed); rename requirements moved to Validated* +*Last updated: 2026-07-29 — Phase 2 complete (registry seeding + import-step ordering); registry-seeding and BUG-04 requirements moved to Validated, three Phase 2 decisions logged, the stale `~40 code-analysis findings` figure corrected to 318* diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index 5ef61f5..8a70621 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -26,11 +26,11 @@ ASVS V2, and to APIs executed against this repo's own Python 2.7.18 interpreter. ### Site creation and registry (REG) -- [ ] **REG-01**: Creating a new Plone site with the add-on selected completes without the `ska_secret_key ... no record` error -- [ ] **REG-02**: The `` declaration makes the import-step ordering explicit rather than dependent on Python 2 `set` iteration order -- [ ] **REG-03**: A test asserts `getSortedImportSteps()` places this package's step after `plone.app.registry` — the ordering assertion, not the rename, is the control -- [ ] **REG-04**: The nested `runImportStepFromProfile` call is gone; `ska_secret_key` is minted by a lazy accessor on first use -- [ ] **REG-05**: Re-applying the default profile leaves an existing `ska_secret_key` unchanged, so signed URLs in flight are not invalidated +- [x] **REG-01**: Creating a new Plone site with the add-on selected completes without the `ska_secret_key ... no record` error +- [x] **REG-02**: The `` declaration makes the import-step ordering explicit rather than dependent on Python 2 `set` iteration order +- [x] **REG-03**: A test asserts `getSortedImportSteps()` places this package's step after `plone.app.registry` — the ordering assertion, not the rename, is the control +- [x] **REG-04**: The nested `runImportStepFromProfile` call is gone; `ska_secret_key` is seeded reliably at install time (revised after CR-02 code review: a lazy-accessor mint on first use was tried first, but it wrote registry state from a request path that `transaction.abort()`s on `Unauthorized`, discarding the mint — see `02-01-SUMMARY.md` "Post-review revision") +- [x] **REG-05**: Re-applying the default profile leaves an existing `ska_secret_key` unchanged, so signed URLs in flight are not invalidated ### Secret handling (SEC) @@ -86,7 +86,7 @@ ASVS V2, and to APIs executed against this repo's own Python 2.7.18 interpreter. - [ ] **BUG-01**: `next_url` is validated against the portal URL before redirect; an off-site value is refused (`token.py:112-113`) - [ ] **BUG-02**: `redirect_url` is always bound on every code path through `user_setup.py` - [ ] **BUG-03**: The bar-code reset token comparison is constant-time, with both operands encoded first to avoid `TypeError` across `str`/`unicode` -- [ ] **BUG-04**: The derived `ska` key separates its components rather than concatenating them bare +- [x] **BUG-04**: The derived `ska` key separates its components rather than concatenating them bare - [ ] **BUG-05**: `py2-ipaddress` is replaced by `ipaddress == 1.0.23`, with `unicode` coercion at the two call sites, so adding `cryptography` cannot break every login through module shadowing - [ ] **BUG-06**: Query-string values are URL-encoded on the way in, resolving the `+`-escaping FIXME @@ -171,11 +171,11 @@ lists above is mechanical. Phase names are in `.planning/ROADMAP.md`. | RENAME-10 | Phase 1 | Complete | | RENAME-11 | Phase 1 | Complete | | RENAME-12 | Phase 1 | Complete | -| REG-01 | Phase 2 | Pending | -| REG-02 | Phase 2 | Pending | -| REG-03 | Phase 2 | Pending | -| REG-04 | Phase 2 | Pending | -| REG-05 | Phase 2 | Pending | +| REG-01 | Phase 2 | Complete | +| REG-02 | Phase 2 | Complete | +| REG-03 | Phase 2 | Complete | +| REG-04 | Phase 2 | Complete | +| REG-05 | Phase 2 | Complete | | SEC-01 | Phase 3 | Pending | | SEC-02 | Phase 3 | Pending | | SEC-03 | Phase 3 | Pending | @@ -216,7 +216,7 @@ lists above is mechanical. Phase names are in `.planning/ROADMAP.md`. | BUG-01 | Phase 7 | Pending | | BUG-02 | Phase 3 | Pending | | BUG-03 | Phase 3 | Pending | -| BUG-04 | Phase 2 | Pending | +| BUG-04 | Phase 2 | Complete | | BUG-05 | Phase 3 | Pending | | BUG-06 | Phase 7 | Pending | | QUAL-01 | Phase 8 | Pending | diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 8856ad7..8923ec8 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -31,7 +31,7 @@ failure mode has no error page and no log line. Decimal phases appear between their surrounding integers in numeric order. - [x] **Phase 1: Rename and Fail-Closed** - `imio.googleauthenticator` everywhere, and a plugin exception becomes a 500 instead of a password-only login (completed 2026-07-29) -- [ ] **Phase 2: Registry Seeding and Import-Step Ordering** - New Plone sites install cleanly, and the ordering that makes them clean is asserted rather than accidental +- [x] **Phase 2: Registry Seeding and Import-Step Ordering** - New Plone sites install cleanly, and the ordering that makes them clean is asserted rather than accidental (completed 2026-07-29) - [ ] **Phase 3: Encrypted Seeds and Local QR** - Seeds are Fernet-encrypted at rest, never sent to Google, and never fall back to plaintext - [ ] **Phase 4: PAS Boundary** - The second factor cannot be bypassed by any credentials extractor, and the refusal leaks nothing - [ ] **Phase 5: Drift, Replay and Lockout** - A replayed code fails, brute force stops at N attempts, and the counters actually persist @@ -90,12 +90,21 @@ Plans: **Success Criteria** (what must be TRUE): 1. Creating a new Plone site with the add-on selected completes with no `IGoogleAuthenticatorSettings defines a field ska_secret_key, for which there is no record` in `var/log/instance.log`. - 2. A test asserts `getSortedImportSteps()` places this package's step after `plone.app.registry`. **The assertion is the control, not the rename** — the rename changes the step id's hash and can make the error vanish without fixing anything, and it would return the first time any other add-on adds or removes an import step. - 3. `grep -r runImportStepFromProfile src/` returns nothing, and `ska_secret_key` is minted by a lazy accessor on first use rather than by a nested profile import. + 2. A test asserts the import step **declares** `plone.app.registry` as a dependency (via `getImportStepMetadata()['dependencies']`), and a second test asserts `getSortedImportSteps()` places this package's step after it. **The declaration assertion is the control, not the rename and not the sorted order** — verified in phase-2 verification: with the `` line deleted, the sorted-order assertion still passes by CPython 2.7 string-hash accident (index 51 vs 36 of 52), so order alone proves nothing and would flip the first time any other add-on adds or removes an import step. + 3. `grep -r runImportStepFromProfile src/` returns nothing, and `ska_secret_key` is minted without a nested profile import — seeded once at install time by `setuphandlers._setup_secret_key()`, with `get_ska_secret_key()` a pure read. *(Revised after code review CR-02: the original criterion said "minted by a lazy accessor on first use". A mint inside the accessor writes registry state from `authenticateCredentials()`, a path that ends in `transaction.abort()` on `Unauthorized`, discarding the key after a URL signed with it was already redirected to. See `02-REVIEW.md` CR-02 and `02-01-SUMMARY.md`.)* 4. A test applies the default profile **twice** and asserts `ska_secret_key` is unchanged, so signed URLs in flight are not invalidated by a reinstall. (A retained value that no longer validates is silently replaced by the default `u''`, with only an INFO log line.) 5. A test asserts the derived `ska` key separates its components: two different component tuples that share the same bare concatenation produce different keys. -**Plans**: TBD +**Plans**: 2/2 plans executed + +Plans: +**Wave 1** + +- [x] 02-01-PLAN.md — Declared ``, the nested profile re-entry deleted, `ska_secret_key` minted lazily in `get_ska_secret_key`, and one test asserting ordering, records, mint and profile-re-apply preservation + +**Wave 2** *(blocked on Wave 1 completion)* + +- [x] 02-02-PLAN.md — Length-prefixed `ska` key derivation with the collision it prevents asserted, plus the `get_browser_hash` empty-string regression guard and the changelog **Phase notes:** @@ -250,7 +259,7 @@ Phases execute in numeric order: 1 → 2 → 3 → 4 → 5 → 6 → 7 → 8 | Phase | Plans Complete | Status | Completed | |-------|----------------|--------|-----------| | 1. Rename and Fail-Closed | 4/4 | Complete | 2026-07-29 | -| 2. Registry Seeding and Import-Step Ordering | 0/TBD | Not started | - | +| 2. Registry Seeding and Import-Step Ordering | 2/2 | Complete | 2026-07-29 | | 3. Encrypted Seeds and Local QR | 0/TBD | Not started | - | | 4. PAS Boundary | 0/TBD | Not started | - | | 5. Drift, Replay and Lockout | 0/TBD | Not started | - | diff --git a/.planning/STATE.md b/.planning/STATE.md index 336d420..4e6185e 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,43 +2,43 @@ gsd_state_version: 1.0 milestone: v1.0 milestone_name: milestone -current_phase: 2 -current_phase_name: Registry Seeding and Import-Step Ordering -status: "Phase 01 shipped — PR #1" -stopped_at: Completed 01-04-PLAN.md -last_updated: "2026-07-29T09:54:11.248Z" +current_phase: 3 +current_phase_name: Encrypted Seeds and Local QR +status: "Phase 2 shipped — PR #2" +stopped_at: Completed 02-02-PLAN.md +last_updated: "2026-07-29T14:43:48.711Z" last_activity: 2026-07-29 progress: - total_phases: 1 - completed_phases: 1 - total_plans: 4 - completed_plans: 4 -last_activity_desc: Phase 01 complete, transitioned to Phase 2 + total_phases: 2 + completed_phases: 2 + total_plans: 6 + completed_plans: 6 +last_activity_desc: Phase 02 complete, transitioned to Phase 3 --- # Project State ## Project Reference -See: .planning/PROJECT.md (updated 2026-07-28) +See: .planning/PROJECT.md (updated 2026-07-29) **Core value:** A second factor that actually holds for in-site users, and that can be deployed alongside `imio.dms.mail` without colliding with it. -**Current focus:** Phase 01 — rename-and-fail-closed +**Current focus:** Phase 3 — Encrypted Seeds and Local QR ## Current Position -Phase: 2 — Registry Seeding and Import-Step Ordering +Phase: 3 — Encrypted Seeds and Local QR Plan: Not started -Status: Phase 01 shipped — PR #1 +Status: Phase 2 shipped — PR #2 Last activity: 2026-07-29 -Progress: [██████████] 100% +Progress: [████████████████████] 6/6 plans authored (100%) · 2 of 8 roadmap phases complete ## Performance Metrics **Velocity:** -- Total plans completed: 4 +- Total plans completed: 6 - Average duration: — - Total execution time: 0.0 hours @@ -47,6 +47,7 @@ Progress: [██████████] 100% | Phase | Plans | Total | Avg/Plan | |-------|-------|-------|----------| | 01 | 4 | - | - | +| 02 | 2 | - | - | **Recent Trend:** @@ -62,6 +63,8 @@ Progress: [██████████] 100% | Phase 01 P02 | 35min | 2 tasks | 9 files | | Phase 01 P03 | 30min | 3 tasks | 13 files | | Phase 01 P04 | 25min | 2 tasks | 7 files | +| Phase 02 P01 | 25min | 2 tasks | 4 files | +| Phase 02 P02 | 12min | 2 tasks | 3 files | ## Accumulated Context @@ -83,6 +86,10 @@ Recent decisions affecting current work: - [Phase ?]: 01-03: profiles/default/site_properties.xml left in place (dead per RESEARCH O-3) -- tied to no requirement, recorded as a Phase 8 observation. - [Phase ?]: 01-04: meta_type/PAS_TITLE renamed to iMio in an isolated commit; PAS_ID (google_auth) left untouched, per the roadmap's own commit-isolation requirement. - [Phase ?]: 01-04: _dont_swallow_my_exceptions = True surfaced two pre-existing bugs (is_whitelisted_client crashing on empty REMOTE_ADDR; a broken getProperty('username') debug line) that had likely been silently disabling the 2FA gate on every request in any deployment; both fixed as blocking Rule 1 auto-fixes. +- [Phase 02]: REG-01 was verified manually after all (UAT 2026-07-29) against a real site-creation log, not by the ordering assertion alone as D-01/D-02 planned. `var/log/instance.log` has zero `no record` / `defines a field ska_secret_key` lines. The 26 `Cannot find registry` INFO lines in that log are stock Plone noise from `plone.app.registry/exportimport/handler.py:67` and all precede our profile import — **do not treat that string as a regression signal in future phases.** +- [Phase 02]: **CORRECTION (supersedes the 02-01 plan's D-04/D-05):** `_setup_secret_key()` was NOT deleted and there is NO lazy mint. CR-02 reverted that design: `setuphandlers._setup_secret_key()` seeds `ska_secret_key` once at install time, and `get_ska_secret_key()` is a pure read that raises `ValueError` on an empty key (fail-closed). Phase 3 must build on the install-time seeding path, not a lazy accessor. +- [Phase ?]: 02-01: REG-05 double-apply test documented as a regression guard against a future schema tightening, not a fix for a currently-firing bug (D-13) +- [Phase ?]: 02-02: BUG-04 fixed via netstring-style length-prefixed join (D-08); test setUp needed a re-login after profile install because PLONE_FIXTURE's cached test-user property sheets predate the add-on's memberdata schema (own-test Rule 1 fix, no production change) ### Pending Todos @@ -95,6 +102,8 @@ None yet. [Issues that affect future work] - **External, Phase 3:** the encryption-key `concat::fragment` lives in the separate `industrialisation` repo. Not one of this roadmap's commits. Phase 3 code is testable without it; the feature is not deployable until it ships. +- **Phase 3 (from 02-SECURITY.md R-02-02):** T-02-09 was accepted on the grounds that every `get_ska_secret_key()` component is ASCII by construction. Phase 3 changes `user_secret` to `v1$` — base64, so still ASCII, but this assumption must be **re-checked, not re-assumed**, when that lands. +- **Phase 3 (from 02-SECURITY.md R-02-01):** `browser/controlpanel.py` renders `ska_secret_key` into a form field. Pre-existing and untouched by Phase 2; it is the recorded Phase 3 secret-hygiene deferred idea. - **Phases 1–7:** `bin/code-analysis` is not clean until Phase 8, so the buildout's pre-commit hook fails until then. Accepted; commits pass with `--no-verify`. - **Phase 8:** expect pre-existing test failures to surface when the test-layer isolation is fixed (`plone.testing 4.1.3` has no isolation guard; some tests currently pass *because* of a state leak). Real bugs revealed, not caused. - **Phase 8:** the post-fix coverage baseline is genuinely unknown and cannot be estimated before `[run] source` lands. The figure is expected to drop sharply; the drop is the truth. @@ -110,6 +119,6 @@ Items acknowledged and carried forward from previous milestone close: ## Session Continuity -Last session: 2026-07-29T07:58:18.330Z -Stopped at: Completed 01-04-PLAN.md +Last session: 2026-07-29T14:36:34Z +Stopped at: Phase 02 complete (UAT 1/1 passed, verification passed, threats_open 0), ready to plan Phase 3 Resume file: None diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md new file mode 100644 index 0000000..0ff6c07 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md @@ -0,0 +1,384 @@ +--- +phase: 02-registry-seeding-and-import-step-ordering +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: + - src/imio/googleauthenticator/configure.zcml + - src/imio/googleauthenticator/setuphandlers.py + - src/imio/googleauthenticator/helpers.py + - src/imio/googleauthenticator/tests/test_setuphandlers.py +autonomous: true +requirements: [REG-01, REG-02, REG-03, REG-04, REG-05] + +must_haves: + truths: + - "REG-03 / D-01: portal_setup.getSortedImportSteps() places 'imio.googleauthenticator' strictly after 'plone.app.registry', asserted in the suite rather than left to CPython 2.7 string-hash order over a Python 2 set — and this assertion is the sole mechanised proof of REG-01, with no automated second-site fixture and no manual site-creation run recorded as evidence" + - "REG-01/REG-02 outcome: after the default profile is applied, all three IGoogleAuthenticatorSettings records exist and get_app_settings() returns without raising KeyError" + - "REG-04: immediately after install, ska_secret_key equals the schema default u'' — no install-time seeding path remains anywhere in the package" + - "REG-04: the first call to get_ska_secret_key() mints and persists a non-empty ska_secret_key, and a second call returns that same key rather than minting again" + - "REG-04: `grep -r runImportStepFromProfile src/` returns nothing, compiled .pyc artefacts included" + - "REG-05: re-applying imio.googleauthenticator:default over an existing non-empty ska_secret_key leaves it equal to the value it had before the re-apply" + - statement: "D-02: Creating a new Plone site with the add-on selected completes with no 'IGoogleAuthenticatorSettings defines a field ska_secret_key, for which there is no record' in var/log/instance.log" + verification: backstop + prohibitions: + - statement: "MUST NOT silence the missing-record error with forInterface(check=False), with omit=(...), or by catching the KeyError from get_app_settings() — check=False converts a loud KeyError into an AttributeError deeper in the stack, and in an MFA package a swallowed plugin exception is a password-only login" + category: safety + requirement_id: REG-01 + - statement: "MUST NOT treat the disappearance of the 'no record' error after the Phase 1 rename as evidence the ordering bug is fixed — the rename changes the step id's string hash and can flip an unspecified ordering, which returns the first time any other add-on adds or removes an import step; the assertion is the control, not the rename" + category: transparency + requirement_id: REG-03 + - statement: "MUST NOT let a profile re-apply replace an existing ska_secret_key with the field default u'' — that silently invalidates every signed token URL in flight and every outstanding bar-code-reset link, and emits only an INFO log line" + category: safety + requirement_id: REG-05 + artifacts: + - path: "src/imio/googleauthenticator/configure.zcml" + provides: "Declared import-step ordering" + contains: 'depends name="plone.app.registry"' + - path: "src/imio/googleauthenticator/setuphandlers.py" + provides: "setupVarious reduced to the marker-file guard plus _add_plugin" + max_lines: 45 + - path: "src/imio/googleauthenticator/helpers.py" + provides: "get_ska_secret_key with the single lazy-mint branch" + contains: "if not ska_secret_key:" + - path: "src/imio/googleauthenticator/tests/test_setuphandlers.py" + provides: "TestSetupHandlers.test_setupVarious — ordering, records, mint and re-apply assertions" + min_lines: 40 + key_links: + - from: "src/imio/googleauthenticator/configure.zcml" + to: "Products.GenericSetup import-step topological sort" + via: " child element on the importStep directive" + pattern: "depends name=\"plone\\.app\\.registry\"" + - from: "src/imio/googleauthenticator/helpers.py" + to: "plone.registry IGoogleAuthenticatorSettings.ska_secret_key" + via: "get_ska_secret_key mints and writes settings.ska_secret_key on first use" + pattern: "settings\\.ska_secret_key = " +--- + + +Make a new Plone site install this add-on cleanly, and make the ordering that keeps it clean an +asserted property of the suite rather than an accident of CPython 2.7 string hashing. + +Three edits, one new test module: + +1. Declare `` on the package's import step (D-10, REG-02). +2. Delete `_setup_secret_key` outright — the nested profile re-entry *and* the seeding that + followed it — leaving `setupVarious` doing only the marker-file guard and `_add_plugin` + (D-04, REG-04). +3. Move the key's birthplace into `get_ska_secret_key()` as one unconditional + `if not ska_secret_key:` branch (D-05, REG-04). + +Purpose: `get_app_settings()`'s `KeyError` is one of PAS's swallowable exceptions, i.e. a 2FA +bypass, and Phase 3 reads the encryption key on a code path this bug destabilises. The bug must +die before encryption lands, and it must die provably — the roadmap says twice and the research +says three times that the error vanishing after Phase 1's rename is a changed string hash, not +a fix. + +Output: three edited source files, one new test module, and a single test method that fails if +anyone deletes the `` line, reintroduces install-time seeding, or tightens the schema +so a re-apply wipes the site signing key. + + + +@/srv/src/imio.googleauthenticator/.claude/gsd-core/workflows/execute-plan.md +@/srv/src/imio.googleauthenticator/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md +@.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md +@.planning/research/PITFALLS.md + + + + + + Task 1: End-to-end "installing the add-on yields a usable ska_secret_key" — one path only + + `bin/test` exists and runs (`make buildout` has been run against Plone 4.3; `.plone-version` records `4.3`). The suite is the only evidence this plan produces — if `bin/test` is absent, halt and run `make buildout` first. + + + src/imio/googleauthenticator/configure.zcml, + src/imio/googleauthenticator/setuphandlers.py, + src/imio/googleauthenticator/helpers.py, + src/imio/googleauthenticator/tests/test_setuphandlers.py + + + + - `src/imio/googleauthenticator/configure.zcml` — the `genericsetup:importStep` block at lines 44-49 is currently self-closing; adding a child element changes the tag shape, which is the one gotcha a text-only diff misses. + - `src/imio/googleauthenticator/setuphandlers.py` — the whole file (63 lines). Note the marker-file guard at line 53 and the two module imports that become dead. + - `src/imio/googleauthenticator/helpers.py` lines 1-42 (imports + `get_app_settings`) and lines 210-260 (`get_browser_hash`, `get_ska_secret_key`). `uuid4` is already imported at line 7 for `generate_secret` — reuse it, do not add an import. + - `src/imio/googleauthenticator/browser/controlpanel.py` lines 24-48 — `IGoogleAuthenticatorSettings`: three fields, `ska_secret_key` is `TextLine(required=False, default=u'')`. + - `src/imio/googleauthenticator/tests/test_generic.py` lines 1-35 — `TestGeneric.setUp`/`test_product_is_installed` is the closest analog: same layer, same `_install()` fixture. + - `src/imio/googleauthenticator/tests/base.py` — `BaseTest._install()` drives a real testbrowser through `prefs_install_products_form`; this is how the profile actually gets applied in this layer. + - `src/imio/googleauthenticator/tests/test_helpers.py` lines 1-12 — the single-import-per-line module header this package's test modules follow (`.isort.cfg`: `force_single_line`, `force_alphabetical_sort`, `line_length = 120`). + - `.planning/research/PITFALLS.md` §"Pitfall 6" (lines 278-347) — the root cause. **Read before touching `configure.zcml`.** `getSortedImportSteps()` builds a Python 2 `set` and `_computeTopologicalSort` inserts dependency-free steps in string-hash order. + - `.planning/research/PITFALLS.md` §"Pitfall 7" (lines 351-439) — the four things the nested profile re-entry does that nobody asked for, including the `queryUtility` path that logs "Cannot find registry" at INFO and creates no records. + - `.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md` — exact target shapes for all three edits. + - `/home/cadam/.claude/plugins/cache/imio-marketplace/imio-plone/1.2.0/skills/plone-write-tests/SKILL.md` — **required before creating the test module.** R5 (one test file per production file, one test method per tested function), R6 (all imports at module level, no exceptions), R1 (no mocking of Plone internals — use the real portal, the real registry, the real member). + + + + New `TestSetupHandlers.test_setupVarious` (integration layer, `_install()` in `setUp`), four + assertion groups in one method, each with a message naming its requirement id. Per D-03 the + mechanised control asserts **both halves** — ORDERING (tied to the `` mechanism, which + is what catches someone deleting the line) and OUTCOME (the property the original bug report was + about, which survives any future restructuring away from ``). Ordering-only and + outcome-only were both rejected: + + - ORDERING (REG-03): `steps = getToolByName(self.portal, 'portal_setup').getSortedImportSteps()` + then `assertGreater(steps.index('imio.googleauthenticator'), steps.index('plone.app.registry'))`. + - RECORDS (REG-01/REG-02 outcome): `get_app_settings()` returns without raising, and + `globally_enabled` and `ip_addresses_whitelist` are readable off the returned settings — + proving all three records exist, not just the one the bug names. + - NO INSTALL-TIME SEEDING (REG-04): immediately after `_install()`, + `assertEqual(u'', get_app_settings().ska_secret_key)`. + - LAZY MINT (REG-04): `first = get_ska_secret_key(request=self.request, user=api.user.get_current(), use_browser_hash=False)`; + assert `get_app_settings().ska_secret_key` is now truthy; call again and assert the stored + key is unchanged between the two calls (the branch is a one-shot mint, not a re-roll). + + + + Three production edits and one new test module. Do all four in one commit — the deletion in + `setuphandlers.py` removes the only thing that seeded the key today, so shipping it without + the mint branch leaves the package with no key at all. + + (a) `src/imio/googleauthenticator/configure.zcml` — give the `genericsetup:importStep` named + `imio.googleauthenticator` a child element ``. The + directive is currently self-closing; convert it to an open tag with a matching + `` close. Attribute form is `name="plone.app.registry"` — the step + id, not a profile id. There is no second `` in this file to copy; the shape is the + one `plone.app.registry` uses for its own three dependencies in + `/srv/cache/eggs/plone.app.registry-1.7.9-py2.7.egg/plone/app/registry/exportimport/configure.zcml:10-18`. + Do not touch `profiles/default/metadata.xml` — its + `profile-plone.app.registry:default` is a *profile* dependency and + orders nothing; per D-11 the `` stays even though (b) removes this phase's own + registry read from the import step, because REG-02/REG-03 mandate it and Phase 3 puts + registry reads back on install-adjacent paths. + + (b) `src/imio/googleauthenticator/setuphandlers.py` — delete the `_setup_secret_key` function + in full (the nested GenericSetup re-entry and both seeding lines that follow it) and delete + its single call site inside `setupVarious`. Keep the marker-file guard + (`context.readDataFile('imio.googleauthenticator.marker.txt') is None` → `return`) exactly as + it is, keep `portal = context.getSite()`, keep `pas = portal.acl_users` and `_add_plugin(pas)`. + Drop the now-dead module imports `from uuid import uuid4` and + `from imio.googleauthenticator.helpers import get_app_settings`; leave the + `GoogleAuthenticatorPlugin` and `MessageFactory` imports untouched. Per D-04, retain **no** + install-time seeding as a fallback — keeping any would put a registry read back inside the + import step, which is the exact coupling REG-04 exists to remove. Leave **no tombstone + comment in `src/` naming the deleted GenericSetup call**: the roadmap gate is a bare recursive + grep over `src/`, and a comment would keep it non-empty. Also delete the stale + `src/imio/googleauthenticator/setuphandlers.pyc` and its siblings (`find src -name '*.pyc' + -delete`) — they are git-ignored build artefacts from before Phase 1 set + `PYTHONDONTWRITEBYTECODE=1`, and the compiled copy still carries the deleted symbol. + + (c) `src/imio/googleauthenticator/helpers.py`, inside `get_ska_secret_key` only — after + `ska_secret_key = settings.ska_secret_key`, add one `if not ska_secret_key:` branch that + assigns `unicode(uuid4())` to a local, writes it back to `settings.ska_secret_key`, and falls + through to the existing return. Per D-05: one branch, one birthplace, no `create=` keyword + argument and no `get_or_create_ska_secret_key()` wrapper. Randomness stays `unicode(uuid4())`, + unchanged from the deleted seeding — raising the site key's entropy is Phase 3's `SEC-06` work + and is a recorded Deferred Idea, not this plan's business. `uuid4` is already imported at + `helpers.py:7`. Do **not** touch the return statement's formatting in this task — plan 02-02 + owns the derivation change (D-08). Do **not** wrap `get_app_settings()` in a try/except: per + D-07 a missing record must raise `KeyError` and propagate, which with + `_dont_swallow_my_exceptions = True` (Phase 1) surfaces as a 500, the correct fail-closed + behaviour for an MFA package. + + (d) New `src/imio/googleauthenticator/tests/test_setuphandlers.py` — one class + `TestSetupHandlers(unittest.TestCase, BaseTest)` on + `IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING`, `setUp` copied from + `TestGeneric.setUp` (`self.app`, `self.portal`, `self.request = self.layer['request']`, + `self.portal_url`, then `self._install()`), and one method `test_setupVarious` carrying the + four assertion groups from ``. All imports at module level, one name per line + (skill R6 + `.isort.cfg`). R5 note to record in the module docstring: the ordering assertion + tests a ZCML declaration and the mint assertion tests a `helpers.py` function, but both are + observable properties of *applying this profile*, whose handler is `setupVarious` — hence one + file, one class, one method, four assertion groups, rather than four methods. `IntegrationTesting` + aborts its transaction per test, so the mint write does not leak into sibling tests. + + While the ordering assertion is being written, print the `getSortedImportSteps()` tuple once + and paste it into the SUMMARY. That settles the roadmap's carried-forward MEDIUM (which of the + nested re-entry's four mechanisms fires on the site-creation path) by observation for free. + Per CONTEXT.md `` it is explicitly **not** worth a task of its own and is not a + blocker — if the print is not cheap, skip it and say so. + + + + find src -name '*.pyc' -delete; ! grep -r runImportStepFromProfile src/ && grep -q 'depends name="plone.app.registry"' src/imio/googleauthenticator/configure.zcml && bin/test -t test_setupVarious + + + + - `bin/test -t test_setupVarious` exits 0. + - `bin/test -t '!robot'` exits 0 — no pre-existing test regressed (`test_generic.py::test_product_is_installed` in particular still passes: the profile still installs). + - `grep -r runImportStepFromProfile src/` prints nothing and exits non-zero, with no `.pyc` under `src/` (ROADMAP success criterion 3, verbatim). + - `grep -c 'depends name="plone.app.registry"' src/imio/googleauthenticator/configure.zcml` returns 1. + - `grep -c '_setup_secret_key' src/imio/googleauthenticator/setuphandlers.py` returns 0. + - `grep -c 'from uuid import uuid4' src/imio/googleauthenticator/setuphandlers.py` returns 0. + - `grep -c 'get_app_settings' src/imio/googleauthenticator/setuphandlers.py` returns 0. + - `grep -c "readDataFile('imio.googleauthenticator.marker.txt')" src/imio/googleauthenticator/setuphandlers.py` returns 1 — the marker guard survived the shrink. + - `grep -c 'check=False' src/imio/googleauthenticator/helpers.py` returns 0. + - `grep -c 'if not ska_secret_key:' src/imio/googleauthenticator/helpers.py` returns 1. + - `python -c "import xml.dom.minidom; xml.dom.minidom.parse('src/imio/googleauthenticator/configure.zcml')"` exits 0 — the self-closing-to-open tag conversion is well-formed. + - The test module has zero `import`/`from` statements inside any method body (skill R6). + + + Applying the default profile leaves all three registry records present with `ska_secret_key == u''`; the first `get_ska_secret_key()` call mints and persists a key and the second returns it unchanged; `getSortedImportSteps()` puts this package's step after `plone.app.registry`; and all of that is asserted by one committed test. + + Deleting `_setup_secret_key` and adding a `` line are both one-commit reversions with no persisted consequence — an already-minted `ska_secret_key` is identical in shape to a seeded one, so a revert costs nothing. + + + + Task 2: REG-05 double-apply regression guard — a known value, asserted by equality + + src/imio/googleauthenticator/tests/test_setuphandlers.py + + + - `src/imio/googleauthenticator/tests/test_setuphandlers.py` — the module task 1 created; this task extends `test_setupVarious`, it does not add a second method. + - `.planning/research/PITFALLS.md` §"Pitfall 8" (lines 443-504) — hole 3 is what this guards: on re-import the existing value is retained, then `bound_field.validate(value)` runs inside a bare `except:` and on failure the value is **replaced by the field default**, with only an INFO log line. + - `src/imio/googleauthenticator/browser/controlpanel.py` lines 24-33 — `ska_secret_key = TextLine(required=False, default=u'')`. This is why hole 3 does not fire today. + - `/srv/cache/eggs/plone.app.testing-4.2.7-py2.7-linux-x86_64.egg/plone/app/testing/helpers.py:96` — `applyProfile(portal, profileName)` is the two-argument form on this Plone 4.3 pin. `portal_setup` has no `applyProfile` method; use the module function. + - `/home/cadam/.claude/plugins/cache/imio-marketplace/imio-plone/1.2.0/skills/plone-write-tests/SKILL.md` — R6 (the new `applyProfile` import goes at module level). + + + + Extend `TestSetupHandlers.test_setupVarious` with a fifth assertion group, REG-05, and a + docstring paragraph explaining why it exists. + + Add `from plone.app.testing import applyProfile` to the module-level import block (one name per + line). In the new assertion group: set `get_app_settings().ska_secret_key` to a distinctive + known literal, call `applyProfile(self.portal, 'imio.googleauthenticator:default')`, then + assert with `assertEqual` that `get_app_settings().ska_secret_key` still equals **that same + literal**. Per D-13, an assertion that the value is merely non-empty is worthless here — it + passes against a fresh re-mint and proves nothing. Note the profile id form: `applyProfile` + takes `imio.googleauthenticator:default` with no `profile-` prefix. + + Write into the method docstring, in words, that this is a **regression guard and not a live + bug fix** — `ska_secret_key` is `TextLine(required=False, default=u'')`, so an existing + non-empty unicode revalidates cleanly on re-import and Pitfall 8's hole 3 does not fire today. + It fires the day someone adds `required=True` or a constraint to that field. Say so explicitly + (D-13's own instruction), or a future reader will go hunting for a bug that is not there. + + + + bin/test -t test_setupVarious && bin/test -t '!robot' + + + + - `bin/test -t test_setupVarious` exits 0. + - `bin/test -t '!robot'` exits 0. + - `grep -c 'applyProfile' src/imio/googleauthenticator/tests/test_setuphandlers.py` returns 2 or more (the module-level import plus at least one call). + - The REG-05 assertion is an `assertEqual` against the same literal that was written before the re-apply — not `assertTrue`, not `assertNotEqual(u'')`, not a non-emptiness check. A reviewer can read the literal in both the set and the assert. + - `applyProfile` is imported at module level; zero `import`/`from` statements appear inside any method body (skill R6). + - The `test_setupVarious` docstring states in words that REG-05 is a regression guard against a future schema tightening, not a fix for a currently-firing bug. + + + Re-applying `imio.googleauthenticator:default` over a seeded, known `ska_secret_key` leaves it byte-identical, asserted by equality against that literal, and the test says out loud why it exists. + + Test-only. + + + + + + + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| GenericSetup profile import → `plone.registry` | A site admin (or the new-site wizard) runs an import step whose ordering relative to `plone.app.registry` is not declared; a step that runs early reads records that do not exist yet. | +| unauthenticated HTTP → `@@google-authenticator-token` / `@@reset-bar-code` → `validate_user_data` → `get_ska_secret_key` | An unauthenticated request can now reach the lazy-mint branch (D-06). | +| PAS `authenticateCredentials` → `sign_user_data` → `get_ska_secret_key` | A ZODB write on a request path that may end in `transaction.abort()` (`pas_plugin.py:160`). | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-02-01 | Elevation of Privilege | `helpers.get_app_settings()` — `forInterface` `KeyError` on a missing record | high | mitigate | Task 1(c): let the `KeyError` propagate. Never `forInterface(check=False)`, never `omit=(...)`, never a wrapping try/except (D-07). With `_dont_swallow_my_exceptions = True` from Phase 1 it renders a 500; without it, PAS swallows the exception and the login falls through to password-only. Enforced by the `check=False` grep in Task 1's acceptance criteria. | +| T-02-02 | Repudiation | `Products.GenericSetup.tool.getSortedImportSteps` — undeclared step order over a Python 2 `set` | medium | mitigate | Task 1(a): ``, plus Task 1(d)'s permanent ordering assertion. Without the assertion the failure has no error page and no log line — it re-flips silently when any other add-on registers a step, i.e. on first deployment next to `imio.dms.mail`. | +| T-02-03 | Tampering | `plone.app.registry` `` re-import replacing `ska_secret_key` with `u''` | medium | mitigate | Task 2's double-apply equality assertion (Pitfall 8 hole 3). A reset site key invalidates every in-flight signed URL and every outstanding bar-code-reset link, and logs only at INFO. | +| T-02-04 | Denial of Service | lazy mint reachable from an unauthenticated request (`token.py:87`, `reset_bar_code.py:150`) | low | accept | The write is idempotent, bounded to once per site, and reaches the state the first login attempt would have reached anyway. A concurrent mint across ZEO clients conflicts, ZODB retries, and the retry re-reads the committed value and skips the branch (D-06). | +| T-02-05 | Tampering | mint write discarded by `transaction.abort()` on the PAS plugin path (`pas_plugin.py:160`) | low | accept | Self-healing rather than silent: a lost mint means the URL just signed will not validate, the user retries, the next request mints again. Explicitly **not** the lockout-counter hazard PROJECT.md's constraint was written for — losing a counter increment means the lock never locks; losing a mint means one failed login (D-06). | +| T-02-06 | Information Disclosure | the minted `ska_secret_key` reaching a log line or exception message | low | accept | The mint branch adds no logging and no exception text. The pre-existing exposure — `browser/controlpanel.py` renders the key into a form field — is untouched here and is a recorded Phase 3 Deferred Idea (canon secret-hygiene, breadcrumbed to `/gsd-secure-phase`, not minted as a prohibition). | +| T-02-SC | Tampering | npm/pip/cargo installs | low | accept | This plan adds no package-manager install: `setup.py` `install_requires` and `test-4.3.cfg` `[versions]` are unchanged, so there is no `[ASSUMED]`/`[SUS]` package to gate and no legitimacy checkpoint is required. | + +ASVS level 1; blocking threshold `high`. Both `high` rows (T-02-01) and every `medium` row carry a +`mitigate` disposition wired to a named task and a named acceptance criterion. + + + +Five edge-probe rows came back `unclassified` and are carried here as explicit flagged assumptions +rather than silently dropped. Four belong to this plan; BUG-04's is carried in plan 02-02. + +| Requirement | Probe | Assumption taken | Consequence if wrong | +|---|---|---|---| +| REG-01 | `unclassified — review manually` | REG-01 is proven by the ordering assertion alone (D-01). No second-site fixture (~30-60 s on a suite Phase 8 must already repair) and no manual-run-and-paste evidence. The ROADMAP criterion-1 log check is therefore carried as a `verification: backstop` marker, deliberately the only must_have in this phase that is not machine-checkable. | At verify time the verifier abstains → `human_needed` (reason `insufficient_spec`) rather than silently passing. That abstention is the intended outcome, not a defect. The manual closing procedure, if anyone wants it, is recorded in 02-CONTEXT.md ``: `bin/instance fg`, create a site with the add-on ticked, then `grep -e "no record" -e "Cannot find registry" var/log/instance.log`. | +| REG-03 | `unclassified — review manually` | `getSortedImportSteps()` returns a tuple whose `.index()` is meaningful, and both `'imio.googleauthenticator'` and `'plone.app.registry'` are always present in the integration layer. | If either step id is absent, `.index()` raises `ValueError` and the test errors loudly — an acceptable failure mode, not a silent pass. | +| REG-04 | `unclassified — review manually` | "Lazy accessor" means exactly `get_ska_secret_key()` and nothing else; no other call site needs a mint, because all four consumers (`pas_plugin.py:160`, `token.py:87`, `reset_bar_code.py:150`, `request_bar_code_reset.py:66`) route through it or through `sign_user_data`/`validate_user_data`, which do. | If a fifth consumer reads `settings.ska_secret_key` directly, it can observe `u''`. `grep -rn 'ska_secret_key' src/` during execution confirms the four; report any fifth in the SUMMARY. | +| REG-05 | `unclassified — review manually` | `applyProfile(portal, 'imio.googleauthenticator:default')` in the integration layer exercises the same `` re-import path a real reinstall does. | If `applyProfile`'s purge semantics differ from QuickInstaller's reinstall, the guard tests a neighbouring path. Accepted: it still covers Pitfall 8's hole 3, which is a property of `registerInterface`, not of the caller. | + + + +New symbols and files created by this plan (excluded from drift verification — they do not exist +in the tree before execution): + +- `src/imio/googleauthenticator/tests/test_setuphandlers.py` — new file +- `TestSetupHandlers` — new test class +- `TestSetupHandlers.test_setupVarious` — new test method +- `` — new ZCML child element in `src/imio/googleauthenticator/configure.zcml` +- `if not ska_secret_key:` — new branch inside the existing `helpers.get_ska_secret_key` + +Deleted by this plan: + +- `setuphandlers._setup_secret_key` — function and its single call site +- `from uuid import uuid4` and `from imio.googleauthenticator.helpers import get_app_settings` in `setuphandlers.py` +- all `src/**/*.pyc` build artefacts + +**No new helper function** is introduced (D-05 rules out `get_or_create_ska_secret_key` and a +`create=` keyword argument). **`profiles/default/registry.xml` is untouched** (D-12): the bare +`` is the correct idiom for this schema — `TextLine`, `Bool` and `Text` all +have `IPersistentField` adapters and none is `readonly`, so neither of Pitfall 8's first two holes +applies and no `omit=(...)` or explicit `` node is warranted. **No profile version bump and +no upgrade step**: no profile *content* +changes here — `registry.xml`, `memberdata_properties.xml` and the rest are untouched, `` +is a ZCML step declaration, and `upgrades/` was deleted in Phase 1 (RENAME-09). Leave +`profiles/default/metadata.xml` at `1000`. + + + +- `bin/test -t '!robot'` exits 0 (whole suite, per `make test`). +- `bin/test -t test_setupVarious` exits 0. +- `grep -r runImportStepFromProfile src/` prints nothing (ROADMAP success criterion 3). +- `grep -q 'depends name="plone.app.registry"' src/imio/googleauthenticator/configure.zcml` exits 0. +- `bin/code-analysis` is **not** a gate for this plan — it fails on 318 pre-existing findings until + Phase 8 (QUAL-06). Commits need `--no-verify`; do not "fix" lint drive-by here. + + + +- REG-02: the import step declares its dependency on `plone.app.registry`. +- REG-03: a committed test asserts the resulting order via `getSortedImportSteps()`. +- REG-04: the nested profile re-entry is gone from `src/`, `.pyc` included, and the key is minted + by one branch inside `get_ska_secret_key()`. +- REG-05: a committed test proves a known `ska_secret_key` survives a profile re-apply, and says in + its docstring that it is a guard rather than a fix. +- REG-01: carried as a `verification: backstop` must_have; the ordering assertion is the mechanised + control (D-01/D-02). + + + +Create `.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md` when done. +Include the `getSortedImportSteps()` tuple if it was cheap to print (the carried-forward MEDIUM), +and the result of `grep -rn 'ska_secret_key' src/` (the REG-04 flagged assumption). + diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md new file mode 100644 index 0000000..3d51d3e --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md @@ -0,0 +1,228 @@ +--- +phase: 02-registry-seeding-and-import-step-ordering +plan: 01 +subsystem: auth +tags: [genericsetup, plone.registry, pas, ska, testing] + +requires: + - phase: 01-rename-and-fail-closed + provides: "_dont_swallow_my_exceptions = True on the PAS plugin, so a propagating KeyError from get_app_settings() surfaces as a 500 rather than a silent password-only fallthrough" +provides: + - "Declared import-step ordering: imio.googleauthenticator's GenericSetup import step depends on plone.app.registry" + - "A permanent test asserting that ordering via getSortedImportSteps(), rather than relying on it holding by accident" + - "ska_secret_key no longer seeded at install time; setupVarious does only the marker-file guard and _add_plugin" + - "ska_secret_key minted lazily, once, inside get_ska_secret_key() on first use" + - "A regression guard proving a profile re-apply does not reset an existing ska_secret_key" +affects: [02-02-ska-key-separation] + +tech-stack: + added: [] + patterns: + - "Import-step ordering declared via child element on genericsetup:importStep, asserted in the suite via portal_setup.getSortedImportSteps()" + - "Lazy-mint-on-first-use: a single 'if not :' branch inside the accessor is the sole birthplace of a registry-backed secret, no separate get_or_create wrapper" + +key-files: + created: + - src/imio/googleauthenticator/tests/test_setuphandlers.py + modified: + - src/imio/googleauthenticator/configure.zcml + - src/imio/googleauthenticator/setuphandlers.py + - src/imio/googleauthenticator/helpers.py + +key-decisions: + - "REG-01 is proven by the ordering assertion alone (D-01/D-02) — no second-site fixture, no manual site-creation run; the ROADMAP log-check criterion is a verification:backstop must_have that the verifier should abstain on, not fail" + - "_setup_secret_key deleted outright with no install-time seeding fallback retained (D-04)" + - "Mint lives inside get_ska_secret_key() as one unconditional 'if not ska_secret_key:' branch, no create= kwarg, no get_or_create_ska_secret_key() wrapper (D-05)" + - "REG-05's double-apply test documented in its own docstring as a regression guard against a future schema tightening, not a fix for a currently-firing bug (D-13)" + +requirements-completed: [REG-01, REG-02, REG-03, REG-04, REG-05] + +coverage: + - id: D1 + description: "imio.googleauthenticator's import step is declared to run after plone.app.registry ()" + requirement: "REG-02" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_setuphandlers.py#TestSetupHandlers.test_setupVarious (ORDERING group)" + status: pass + human_judgment: false + - id: D2 + description: "The resulting order is asserted via getSortedImportSteps(), not left to string-hash chance" + requirement: "REG-03" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_setuphandlers.py#TestSetupHandlers.test_setupVarious (ORDERING group)" + status: pass + human_judgment: false + - id: D3 + description: "After install, all three IGoogleAuthenticatorSettings records exist and get_app_settings() returns without raising (REG-01 outcome half)" + requirement: "REG-01" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_setuphandlers.py#TestSetupHandlers.test_setupVarious (RECORDS group)" + status: pass + human_judgment: true + rationale: "REG-01's ROADMAP criterion is a var/log/instance.log check from a real site-creation run (D-01/D-02), which this phase deliberately does not automate. The mechanised RECORDS/ORDERING assertions are the proven control; the log-based backstop must_have is out of reach for this test suite and is left to human verification per D-02." + - id: D4 + description: "No install-time seeding of ska_secret_key remains; _setup_secret_key and its runImportStepFromProfile re-entry are deleted from src/, .pyc included" + requirement: "REG-04" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_setuphandlers.py#TestSetupHandlers.test_setupVarious (NO INSTALL-TIME SEEDING group)" + status: pass + - kind: other + ref: "grep -r runImportStepFromProfile src/ (exits non-zero, no output)" + status: pass + human_judgment: false + - id: D5 + description: "get_ska_secret_key() mints and persists a non-empty key on first use, and does not re-mint on a second call" + requirement: "REG-04" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_setuphandlers.py#TestSetupHandlers.test_setupVarious (LAZY MINT group)" + status: pass + human_judgment: false + - id: D6 + description: "Re-applying imio.googleauthenticator:default over an existing known ska_secret_key leaves it byte-identical" + requirement: "REG-05" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_setuphandlers.py#TestSetupHandlers.test_setupVarious (REG-05 group)" + status: pass + human_judgment: false + +duration: 25min +completed: 2026-07-29 +status: complete +--- + +# Phase 2 Plan 1: Registry Seeding and Import-Step Ordering Summary + +**Declared `` on the import step, deleted the nested `runImportStepFromProfile` re-seeding, moved `ska_secret_key`'s mint into a single lazy branch inside `get_ska_secret_key()`, and asserted all of it — ordering, records, no-seeding, mint, and re-apply survival — in one committed test method.** + +## Performance + +- **Duration:** 25 min +- **Started:** 2026-07-29T12:43:04Z +- **Completed:** 2026-07-29 +- **Tasks:** 2 +- **Files modified:** 4 (3 modified, 1 created) + +## Accomplishments + +- `configure.zcml`'s `imio.googleauthenticator` import step now declares ``, converting the previously self-closing directive to an open tag with a matching close, so GenericSetup's topological sort orders this package's step after `plone.app.registry` deterministically instead of by CPython 2.7 string-hash chance. +- `setuphandlers.py`'s `_setup_secret_key` (the nested `portal_setup.runImportStepFromProfile('profile-imio.googleauthenticator:default', 'plone.app.registry')` re-entry plus its seeding) is deleted outright, along with its call site and the now-dead `from uuid import uuid4` / `from imio.googleauthenticator.helpers import get_app_settings` imports. `setupVarious` does only the marker-file guard and `_add_plugin`. +- `helpers.py`'s `get_ska_secret_key()` gained a single `if not ska_secret_key:` branch: mints `unicode(uuid4())` and writes it back to the registry on first use, then falls through to the existing return unchanged. No `create=` kwarg, no `get_or_create_ska_secret_key()` wrapper. +- New `tests/test_setuphandlers.py::TestSetupHandlers.test_setupVarious` — one integration-layer test method with five assertion groups: ORDERING (REG-03), RECORDS (REG-01/REG-02 outcome), NO INSTALL-TIME SEEDING (REG-04), LAZY MINT (REG-04), and REG-05's double-apply regression guard. +- All stale git-ignored `.pyc` build artefacts under `src/` were removed (28 files, including the one carrying the deleted `_setup_secret_key` symbol). + +## Task Commits + +Each task was committed atomically: + +1. **Task 1: End-to-end "installing the add-on yields a usable ska_secret_key"** - `5decb52` (feat) +2. **Task 2: REG-05 double-apply regression guard** - `ed59eae` (test) + +**Plan metadata:** _pending — final docs commit below_ + +_Note: Task 1 is `type="tracer"` — committed as a full working slice, then its `` was re-run end-to-end before Task 2 (auto mode active); it passed, so expansion proceeded without a checkpoint._ + +## Files Created/Modified + +- `src/imio/googleauthenticator/configure.zcml` - Added `` to the `imio.googleauthenticator` import step +- `src/imio/googleauthenticator/setuphandlers.py` - Deleted `_setup_secret_key` and its call site plus two dead imports; `setupVarious` now only guards the marker file and calls `_add_plugin` +- `src/imio/googleauthenticator/helpers.py` - Added the `if not ska_secret_key:` lazy-mint branch inside `get_ska_secret_key()` +- `src/imio/googleauthenticator/tests/test_setuphandlers.py` - New: `TestSetupHandlers.test_setupVarious`, five assertion groups + +## Decisions Made + +- Followed CONTEXT.md's D-01 through D-13 as written; no new decisions were required during execution — implementation matched the pattern map (`02-PATTERNS.md`) exactly, including the exact target shapes for all three production edits. +- Confirmed via `grep -rn 'ska_secret_key' src/imio/googleauthenticator/*.py src/imio/googleauthenticator/browser/*.py src/imio/googleauthenticator/browser/forms/*.py` that exactly four call sites route through `get_ska_secret_key`/`sign_user_data` (the flagged REG-04 assumption): `helpers.py` itself (definition + 2 internal calls) and `request_bar_code_reset.py:66`. No fifth consumer reads `settings.ska_secret_key` directly. +- Printed `getSortedImportSteps()` once while writing the ordering assertion (the carried-forward MEDIUM, settled by observation per CONTEXT.md ``): the sorted tuple places `u'plone.app.registry'` at index 34 and `u'imio.googleauthenticator'` at index 35, i.e. immediately after it — full tuple: + + ``` + (u'rolemap', u'sharing', u'plone-difftool', u'properties', u'toolset', u'cookie_authentication', + u'catalog', u'workflow', u'update-workflow-rolemap', u'uid_catalog', u'various', + u'reference_catalog', u'componentregistry', u'portal-transforms-various', u'skins', + u'cssregistry', u'jquerytools-various', u'jsregistry', u'actions', + u'plonetheme.sunburst-various', u'controlpanel', u'atcttool', u'tinymce_settings', + u'archetypes-various', u'archetypetool', u'difftool', u'memberdata-properties', u'plonepas', + u'plone_outputfilters_various', u'browserlayer', u'tinymce_various', u'mailhost', + u'content_type_registry', u'propertiestool', u'viewlets', u'mimetypes-registry-various', + u'plone.app.registry', u'imio.googleauthenticator', u'action-icons', u'languagetool', + u'typeinfo', u'factorytool', u'cmfeditions_various', u'repositorytool', u'content', + u'contentrules', u'portlets', u'plone-final', u'plone-content', u'plone.app.theming', + u'various-calendar', u'caching_policy_mgr', u'collective.z3cform.datetimewidget_various') + ``` + + This does not settle which of `runImportStepFromProfile`'s four mechanisms fired on this site's pre-fix path (that question is now moot — the mechanism is deleted), but it does confirm the `` declaration produces the intended adjacency on this suite's fixture. + +## Deviations from Plan + +None - plan executed exactly as written. Both production edits and the test module matched `02-PATTERNS.md`'s target shapes; no auto-fixes, no blocking issues, no architectural questions arose. + +## Issues Encountered + +None. The sandboxed `find`/`grep` wrappers in this environment don't support GNU-style `-delete`/`--include` flags used in the plan's literal verify command — worked around with `find ... | xargs rm -f` and separate per-directory `grep` invocations to the same effect; no functional difference in what was checked. + +## User Setup Required + +None - no external service configuration required. + +## Next Phase Readiness + +- `get_ska_secret_key()`'s bare `"{0}{1}{2}".format(...)` concatenation (BUG-04) is untouched here by design — plan 02-02 owns the netstring-style join (D-08) and the `get_browser_hash` `None`→`''` check (D-09, already satisfied in the installed tree per `02-PATTERNS.md`). +- REG-01's ROADMAP log-check criterion remains a `verification: backstop` must_have (D-02): the ordering/records assertions are the mechanised control; a manual `bin/instance fg` site-creation smoke run is documented in `02-CONTEXT.md ` if anyone wants to close it by hand, but is not required by this plan. +- Full suite (`bin/test -t '!robot'`) is green at 22 tests, 0 failures, 0 errors, after both task commits. + +--- +*Phase: 02-registry-seeding-and-import-step-ordering* +*Completed: 2026-07-29* + +## Self-Check: PASSED + +All created files and both task commit hashes verified present on disk / in git log. + +## Post-review revision + +Code review (`02-REVIEW.md`, CR-02) found that lazy-minting `ska_secret_key` inside +`get_ska_secret_key()` (D-05) — reached with no install-time seeding fallback (D-04) — +writes registry state from `sign_user_data()` inside +`GoogleAuthenticatorPlugin.authenticateCredentials()`, a request path that ends in +`transaction.abort()` on `Unauthorized`. That discards the mint after a signed URL +using it was already redirected to, leaving a 2FA-enabled user's first login stuck in +a permanently-invalid-signature loop with no self-recovery. This is exactly the class +of hazard `.claude/CLAUDE.md` names ("all state writes in the token form view"). + +**D-04 and D-05 are revised** (with the user's explicit authorisation to reopen locked +decisions): + +- Install-time seeding is restored in `setuphandlers._setup_secret_key()`, called from + `setupVarious`. D-04's actual intent — no nested `runImportStepFromProfile` re-entry — + is kept intact: the restored seeding is a direct `get_app_settings()` call, relying on + the REG-02 `` declaration to guarantee the registry + records already exist by the time `setupVarious` runs. +- `get_ska_secret_key()` is a pure read again: the `if not ska_secret_key:` mint branch + is removed and replaced with a fail-closed `raise ValueError(...)` if the key is + unexpectedly empty at read time (not caught anywhere — surfaces as a 500 via + RENAME-11's `_dont_swallow_my_exceptions = True`, never a silent password-only + fallthrough). + +Also fixed in the same review pass: CR-01 (a falsy/`None` +`two_factor_authentication_secret` crashed `get_ska_secret_key()` with `TypeError`; +coerced with `or ''`), and WR-03 (`test_setupVarious`'s five bundled assertion groups +split into five separately-named test methods). + +**Requirement status after revision:** + +- REG-01, REG-02, REG-03 — unaffected, still hold as originally verified (import-step + ordering and registry-records-exist are untouched by this revision). +- REG-04 — revised. "No install-time seeding path remains" no longer holds by design; + it is superseded by the fix for CR-02. The parts of REG-04 that still hold: no nested + `runImportStepFromProfile` re-entry exists anywhere in `src/`, and `ska_secret_key` is + guaranteed non-empty (now seeded at install rather than lazily minted). +- REG-05 — still holds; re-verified against the revised code (the double-apply guard + test is unchanged in intent, only relocated into its own method). +- BUG-04 — still holds; the netstring-style separation is untouched by this revision. + +See `02-REVIEW-FIX.md` for the fix-by-fix disposition and `bin/test` results. diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md new file mode 100644 index 0000000..228f8c9 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md @@ -0,0 +1,312 @@ +--- +phase: 02-registry-seeding-and-import-step-ordering +plan: 02 +type: execute +wave: 2 +depends_on: ["02-01"] +files_modified: + - src/imio/googleauthenticator/helpers.py + - src/imio/googleauthenticator/tests/test_helpers.py + - CHANGES.rst +autonomous: true +requirements: [BUG-04] + +must_haves: + truths: + - "BUG-04 / adjacency: two component tuples that share the same bare concatenation derive to different keys — (u'ab', u'', u'cd') and (u'a', u'', u'bcd') both concatenate to u'abcd' but derive to u'2:ab0:2:cd' and u'1:a0:3:bcd' (ROADMAP success criterion 5)" + - "BUG-04 / empty: an empty component is rendered rather than elided — a browser_hash of u'' contributes the literal u'0:' to the derived key, so 'no User-Agent' and 'a User-Agent hashing to nothing' remain distinguishable from a shifted component boundary" + - "BUG-04 / encoding: the derived key is a unicode value and each length prefix is len() over that component's own characters (Python 2 unicode code points, not bytes), asserted by exact-string equality against a known fixture rather than by a length count" + - "BUG-04 / ordering: component order is fixed as (user_secret, browser_hash, ska_secret_key), unchanged from the pre-existing concatenation, so framing is the only thing this change alters" + - "get_browser_hash returns u'' and never None when sha1() raises, so taking len() of it on a login path cannot raise TypeError" + - "The whole suite (bin/test -t '!robot') stays green: all four call sites of the derivation — pas_plugin.py:160, token.py:87, reset_bar_code.py:150, request_bar_code_reset.py:66 — are unmodified, because both sides of every signed URL derive the key through the same function" + prohibitions: + - statement: "MUST NOT change the ska key derivation again once the package is deployed or any user is enrolled, without an explicit migration — the derivation feeds sign_user_data, validate_user_data and request_bar_code_reset.py:66, so a later change invalidates every signed token URL in flight and every outstanding bar-code-reset email link, with the user seeing only a failed signature and no explanation" + category: safety + requirement_id: BUG-04 + artifacts: + - path: "src/imio/googleauthenticator/helpers.py" + provides: "Length-prefixed derivation in get_ska_secret_key" + contains: "u''.join" + - path: "src/imio/googleauthenticator/tests/test_helpers.py" + provides: "TestSkaSecretKey — derivation separation and browser-hash regression guard" + min_lines: 130 + key_links: + - from: "src/imio/googleauthenticator/helpers.py" + to: "src/imio/googleauthenticator/tests/test_helpers.py" + via: "TestSkaSecretKey.test_get_ska_secret_key asserts the exact derived string for a known (user_secret, browser_hash, ska_secret_key) fixture" + pattern: "get_ska_secret_key" + - from: "src/imio/googleauthenticator/helpers.py get_ska_secret_key" + to: "src/imio/googleauthenticator/helpers.py get_browser_hash" + via: "the derivation takes len() of get_browser_hash's return value, so a None return would raise TypeError on a login path" + pattern: "get_browser_hash\\(request=" +--- + + +Stop the derived `ska` signing key from being a bare concatenation of its three components. + +`helpers.py:259` renders `(user_secret, browser_hash, ska_secret_key)` into a single string with +no framing, so `(u'ab', u'', u'cd')` and `(u'a', u'', u'bcd')` produce the *same* signing key. Two +different users, or the same user on two different devices, can therefore end up signing and +validating under one key. Replace it with a length-prefixed join — each component rendered as +`:`, concatenated netstring-style (D-08). + +Purpose: this is a spoofing defect, not a strength defect, so the fix is framing rather than +hashing. It ships **now**, in Phase 2, precisely because it is cheap now and expensive later: +nothing is deployed and no user is enrolled, so no signed URL and no reset link is invalidated. It +is the last moment that is true. See `` on Task 1. + +Output: one changed return expression, one new test class with two methods, one changelog line. + + + +@/srv/src/imio.googleauthenticator/.claude/gsd-core/workflows/execute-plan.md +@/srv/src/imio.googleauthenticator/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md +@.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md +@.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md + + + + + + Task 1: Length-prefixed derivation in get_ska_secret_key, with the collision it prevents asserted + + + src/imio/googleauthenticator/helpers.py, + src/imio/googleauthenticator/tests/test_helpers.py + + + + - `src/imio/googleauthenticator/helpers.py` lines 228-260 — `get_ska_secret_key` as plan 02-01 left it: the lazy-mint branch is already in place; only the return expression changes here. + - `src/imio/googleauthenticator/helpers.py` lines 210-225 — `get_browser_hash`. Read it to confirm the `except` branch already returns `''`; Task 2 guards that, and this task depends on it being true. + - `src/imio/googleauthenticator/tests/test_helpers.py` — the whole file (92 lines). `TestIPWhitelisting` is the existing class; its module header at lines 1-11 is the single-import-per-line convention, and `test_extract_ip_address_from_request_ignores_malformed_ip` shows the plain-dict-as-request idiom this package already uses. + - `src/imio/googleauthenticator/profiles/default/memberdata_properties.xml` — `two_factor_authentication_secret` is a declared `string` property. Undeclared memberdata properties are silently popped by `MutablePropertySheet.setProperties`, so the test can only round-trip declared names. + - `src/imio/googleauthenticator/pas_plugin.py` around line 160, `src/imio/googleauthenticator/browser/forms/token.py` around line 87, `src/imio/googleauthenticator/browser/forms/reset_bar_code.py` around line 150, `src/imio/googleauthenticator/browser/forms/request_bar_code_reset.py` around line 66 — the four consumers. Read them to confirm none needs an edit: they all derive through this one function, so both sides of every signed URL move together. + - `.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md` §D-08 — the rationale, including why a single-delimiter join and an HMAC derivation were both rejected. + - `/home/cadam/.claude/plugins/cache/imio-marketplace/imio-plone/1.2.0/skills/plone-write-tests/SKILL.md` — R1 (use the real member and the real registry; do not fake a user object), R5 (one test method per tested function), R6 (module-level imports only). + + + + New `TestSkaSecretKey.test_get_ska_secret_key`, one method, integration layer, real objects + only — the real current member via `api.user.get_current()` and `setMemberProperties`, the real + registry via `get_app_settings()`. No fake user, no monkeypatch. + + - EXACT SHAPE: with `two_factor_authentication_secret` set to `'ab'`, `ska_secret_key` set to + `u'cd'`, and `use_browser_hash=False`, `get_ska_secret_key(...)` returns exactly + `u'2:ab0:2:cd'`. This one `assertEqual` pins the ordering (user secret first), the empty + component (`0:` for the blank browser hash), the delimiter and the length semantics at once. + - UNICODE: `assertIsInstance(result, unicode)`. + - THE COLLISION IT PREVENTS: assert first that the fixture really is a collision under the old + scheme — `assertEqual(u'ab' + u'' + u'cd', u'a' + u'' + u'bcd')` — then re-set the member + property to `'a'` and the registry value to `u'bcd'`, derive again, and + `assertNotEqual` the two derived keys. Without the first line the test looks arbitrary and a + future editor will "simplify" the fixture into one that no longer collides. + - MINT UNTOUCHED: with a non-empty `ska_secret_key` in place, the derivation does not re-mint — + the value read back from `get_app_settings()` after both calls is still `u'bcd'`. + + + + (a) `src/imio/googleauthenticator/helpers.py`, the return statement of `get_ska_secret_key` + only. Replace the three-placeholder string-format concatenation with a netstring-style + length-prefixed join: for each component in the order `(user_secret, browser_hash, + ska_secret_key)`, render `:` — the component's `len()`, a single ASCII colon, then + the component — and concatenate the three with no separator between them. Build it as a + `unicode` result (join onto `u''`, format with a `u'...'` template) so the derived key has one + type regardless of whether a component arrived as `str` or `unicode`; a generator expression + inside `u''.join(...)` matches this module's terseness. Keep the component order byte-identical + to the current one — reordering would change the key for no benefit. Touch nothing else in the + function: the lazy-mint branch from plan 02-01, the `request`/`user` defaulting, and the + `use_browser_hash` switch all stay as they are. Do not edit any of the four consumers; they + derive through this function on both the signing and the validating side, so they move + together by construction. + + Rejected alternatives, recorded so they are not re-litigated: a single-delimiter join + (`u"|".join`) is safe only while no component can contain the delimiter, and Phase 3 turns + `user_secret` into a `v1$` ciphertext, so that argument expires one phase from + now. An HMAC-based derivation is overkill — the stated defect is collidability, not weak + derivation, and hashing on the login path buys nothing here. Add **no** encode/decode coercion: + every component is ASCII today (a base32 seed, a hex sha1 or `u''`, and a uuid4 string) and + stays ASCII under Phase 3's base64 ciphertext, so a coercion would be untested defensive code + on a login path. + + (b) `src/imio/googleauthenticator/tests/test_helpers.py` — add a second class + `TestSkaSecretKey(unittest.TestCase, BaseTest)` on the same + `IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING` layer, with the `setUp` shape used in + `test_setuphandlers.py` (`self.app`, `self.portal`, `self.request = self.layer['request']`, + `self.portal_url`, `self._install()`), and one method `test_get_ska_secret_key` carrying the + `` assertions. Add `from imio.googleauthenticator.helpers import get_app_settings`, + `from imio.googleauthenticator.helpers import get_ska_secret_key` and `from plone import api` + to the module-level import block, one name per line. Set the member property with + `api.user.get_current().setMemberProperties(mapping={'two_factor_authentication_secret': 'ab'})` + and the site key by assigning to `get_app_settings().ska_secret_key`. `IntegrationTesting` + aborts its transaction per test, so neither write leaks into a sibling test. + + Skill-consistency note to record in the class docstring: R5 would put a `helpers.py` test in a + class named for the module, but this file already groups by concern (`TestIPWhitelisting`), so + a second concern-named class follows R7 rather than fighting it. + + + + bin/test -t test_get_ska_secret_key && bin/test -t '!robot' + + + + - `bin/test -t test_get_ska_secret_key` exits 0. + - `bin/test -t '!robot'` exits 0 — every pre-existing test, including the four consumers' browser tests in `test_generic.py`, still passes. + - The test asserts the exact string `u'2:ab0:2:cd'` by equality, not a substring, a length, or a regex. + - The test contains an assertion proving the fixture collides under bare concatenation, so the `assertNotEqual` that follows is non-vacuous. + - `grep -c "u''.join" src/imio/googleauthenticator/helpers.py` returns 1. + - `grep -c "'{0}{1}{2}'" src/imio/googleauthenticator/helpers.py` returns 0 and `grep -c '"{0}{1}{2}"' src/imio/googleauthenticator/helpers.py` returns 0 — the bare concatenation is gone in both quote styles. + - `git diff --stat src/imio/googleauthenticator/pas_plugin.py src/imio/googleauthenticator/browser/forms/token.py src/imio/googleauthenticator/browser/forms/reset_bar_code.py src/imio/googleauthenticator/browser/forms/request_bar_code_reset.py` shows no changes — the four consumers are untouched. + - `git diff src/imio/googleauthenticator/helpers.py` shows exactly one changed hunk, inside `get_ska_secret_key`, below the lazy-mint branch. + - Zero `import`/`from` statements inside any method body of `test_helpers.py` (skill R6). + + + `get_ska_secret_key` derives `u'2:ab0:2:cd'` for the known fixture, two component tuples with the same bare concatenation derive to different keys, and the whole suite is green with all four consumers unmodified. + + The derivation feeds `sign_user_data`, `validate_user_data` and `request_bar_code_reset.py:66`; changing it later invalidates every signed token URL in flight and every outstanding bar-code-reset email link. It is free *now* only because nothing is deployed and no user is enrolled — which is precisely why it sits in Phase 2 and not in a later one. No checkpoint is inserted (`costly`, not `one-way`), but a later reopening of this decision is not free and must carry a migration. + + + + Task 2: Regression guard for get_browser_hash's empty-string return, and the changelog line + + + src/imio/googleauthenticator/tests/test_helpers.py, + CHANGES.rst + + + + - `src/imio/googleauthenticator/helpers.py` lines 210-225 — `get_browser_hash`. **Read it before writing anything.** The `except` branch already reads `return ''`. Phase 1's fail-closed work fixed it. There is nothing to change in this function. + - `src/imio/googleauthenticator/tests/test_helpers.py` — the file as Task 1 left it; this task adds a second method to `TestSkaSecretKey`. + - `.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md` §D-09 — states that the function "falls off the end and returns `None`". That description is **stale** and does not match the tree; D-09's *intent* is what survives. + - `CHANGES.rst` lines 1-15 — the `1.0.0 (unreleased)` section and its bullet style (a sentence, then `[chris-adam]` on its own indented line). + + + + **Do not modify `src/imio/googleauthenticator/helpers.py` in this task.** Its `except` branch + already returns the empty string. 02-CONTEXT.md's D-09 says otherwise; D-09 is stale and Phase 1 + already fixed it. Verify by reading lines 210-225 before doing anything, and if the tree really + does differ from that expectation, stop and report rather than improvising. + + (a) Add a second method `test_get_browser_hash` to `TestSkaSecretKey` in + `src/imio/googleauthenticator/tests/test_helpers.py`. Call `get_browser_hash(request={})` — a + plain dict, the same request idiom `test_extract_ip_address_from_request_ignores_malformed_ip` + already uses — so the absent `HTTP_USER_AGENT` makes `sha1()` raise and the `except` branch + runs. Assert `assertEqual('', result)` **and** `assertIsNotNone(result)`; the second assertion + is the one that carries the intent, because the empty string and `None` are both falsy and a + truthiness check would not discriminate. Also assert the happy path in the same method (skill + R5, one method per tested function): with `request={'HTTP_USER_AGENT': 'Mozilla/5.0'}` the + result is a 40-character hex sha1 digest. + + Write into the method docstring **why this test exists**, because it guards nothing that is + broken today and will otherwise read as dead ceremony: Task 1's derivation takes `len()` of + this function's return value, and `len(None)` raises `TypeError` on a login path. So the moment + the length prefixing landed, this function's `except` branch stopped being cosmetic and became + a crash guard. This is a regression guard, exactly like plan 02-01's REG-05 test — nobody should + go looking for a live bug here. + + (b) `CHANGES.rst` — add bullets under `1.0.0 (unreleased)` in the existing style covering this + phase: the declared import-step dependency on `plone.app.registry`, the removal of the nested + profile import in favour of minting `ska_secret_key` on first use, and the length-prefixed + `ska` key derivation with a one-clause note that it invalidates any previously issued signed + URL. Attribution line matches the existing entries. + + + + bin/test -t test_get_browser_hash && bin/test -t '!robot' + + + + - `bin/test -t test_get_browser_hash` exits 0. + - `bin/test -t '!robot'` exits 0. + - `git diff --stat src/imio/googleauthenticator/helpers.py` shows **no** change from this task — the production function is untouched. + - The test asserts both `assertEqual('', ...)` and `assertIsNotNone(...)`; a truthiness-only assertion does not satisfy this criterion, because `''` and `None` are both falsy. + - The `test_get_browser_hash` docstring states in words that it is a regression guard protecting the length-prefixed derivation from a `TypeError`, not a fix for a currently-failing behaviour. + - `grep -c 'ska' CHANGES.rst` returns 1 or more, and the new bullets sit under the `1.0.0 (unreleased)` heading. + + + A committed test pins `get_browser_hash`'s empty-string return and says why; `helpers.py` is unchanged by this task; the changelog records the three user-visible changes of this phase. + + Test and documentation only. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| signed URL query string → `ska.validate_signed_request_data` | An attacker-supplied `auth_user` + signature is validated against a key derived from three components. If two component tuples can derive the same key, a signature minted for one context validates in another. | +| memberdata `two_factor_authentication_secret` → the derivation | A per-user value of attacker-influenced *length* (via enrolment) enters a string that is concatenated with a site-wide secret. | +| `HTTP_User-Agent` → `get_browser_hash` → the derivation | An absent or unhashable `User-Agent` on an unauthenticated login path reaches a `len()` call. | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-02-07 | Spoofing | `helpers.get_ska_secret_key` — unframed concatenation of `(user_secret, browser_hash, ska_secret_key)` | high | mitigate | Task 1(a): length-prefixed netstring join, so a component-boundary shift changes the derived key. Task 1(b) asserts the exact derived string for a fixture that provably collides under the old scheme, so the mitigation cannot silently regress into a cosmetic reformat. | +| T-02-08 | Denial of Service | `get_browser_hash` returning `None` under `len()` on the login path | medium | mitigate | Task 2(a)'s `assertIsNotNone` regression guard. The function is already correct in-tree; the guard is what stops a future edit from reintroducing a fall-off-the-end `None` now that a `TypeError` there would be an unauthenticated crash. | +| T-02-09 | Denial of Service | a non-ASCII `str` component reaching a `u'...'` format → `UnicodeDecodeError` on the login path | low | accept | Every component is ASCII by construction: a base32 seed, a hex sha1 or `u''`, and a `unicode(uuid4())`. Phase 3's `v1$` is base64, also ASCII. Coercion is deliberately not added — it would be untested defensive code on a login path guarding a state no code path can produce. Re-check when Phase 3 changes `user_secret`. | +| T-02-10 | Tampering | reopening the derivation after deployment | medium | accept | Recorded as the plan's one prohibition and as Task 1's `costly` reversibility rating. Accepted now because nothing is deployed and no user is enrolled; a later change requires an explicit migration, since it invalidates every signed URL in flight and every outstanding reset link. | +| T-02-SC | Tampering | npm/pip/cargo installs | low | accept | This plan adds no package-manager install: `setup.py` `install_requires` and `test-4.3.cfg` `[versions]` are unchanged, so there is no `[ASSUMED]`/`[SUS]` package to gate and no legitimacy checkpoint is required. | + +ASVS level 1; blocking threshold `high`. The single `high` row (T-02-07) is `mitigate`, wired to +Task 1 and to a named non-vacuous acceptance criterion. + + + +One edge-probe row came back `unclassified` for BUG-04 and is carried here rather than silently +dropped. (The other four rows of this phase's probe — REG-02's `adjacency`, `empty`, `encoding` and +`ordering` — were resolvable and are authored as plain `must_haves.truths` above. They are carried +in *this* plan rather than in 02-01 because all four are questions about D-08's netstring join, +even though REG-02's requirement itself, the `` declaration, ships in 02-01.) + +| Requirement | Probe | Assumption taken | Consequence if wrong | +|---|---|---|---| +| BUG-04 | `unclassified — review manually` | "Separates its components" is satisfied by unambiguous framing (a prefix-free encoding), not by a keyed derivation. The defect is collidability; length-prefixing makes the encoding injective, which is exactly and only what ROADMAP success criterion 5 asks for. | If the real intent were key *strength* rather than component separation, this fix would be insufficient. It is not: `unicode(uuid4())`'s ~122 bits of site-key entropy is a recorded Phase 3 Deferred Idea under `SEC-06`, tracked separately and explicitly out of scope here. | + + + +New symbols created by this plan (excluded from drift verification — they do not exist in the tree +before execution): + +- `TestSkaSecretKey` — new test class in `src/imio/googleauthenticator/tests/test_helpers.py` +- `TestSkaSecretKey.test_get_ska_secret_key` — new test method +- `TestSkaSecretKey.test_get_browser_hash` — new test method +- three new bullets under `1.0.0 (unreleased)` in `CHANGES.rst` + +Modified, not created: the return expression of the existing `helpers.get_ska_secret_key`. + +**No new helper function**, no new module, no change to `get_browser_hash`, no change to any of the +four derivation consumers, and no profile version bump. + + + +- `bin/test -t '!robot'` exits 0. +- `bin/test -t test_get_ska_secret_key` and `bin/test -t test_get_browser_hash` each exit 0. +- `git diff --stat` over `pas_plugin.py`, `browser/forms/token.py`, `browser/forms/reset_bar_code.py` + and `browser/forms/request_bar_code_reset.py` is empty across the whole plan. +- `bin/code-analysis` is **not** a gate — 318 pre-existing findings until Phase 8 (QUAL-06); commit + with `--no-verify`. + + + +- BUG-04: the derived `ska` key separates its components; two component tuples sharing a bare + concatenation produce different keys, asserted in the suite (ROADMAP success criterion 5). +- `get_browser_hash`'s empty-string return is pinned by a test that explains it is a guard for the + new length prefixing, not a fix for a live bug. +- The phase's user-visible changes are in `CHANGES.rst`. + + + +Create `.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md` when done. +Record the exact derived string the test pins, and confirm explicitly whether `helpers.py` lines +210-225 matched the expectation in Task 2's `read_first` (the stale-D-09 check). + diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md new file mode 100644 index 0000000..95e0aef --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md @@ -0,0 +1,229 @@ +--- +phase: 02-registry-seeding-and-import-step-ordering +plan: 02 +subsystem: auth +tags: [ska, helpers, testing, security] + +requires: + - phase: 02-registry-seeding-and-import-step-ordering + plan: 01 + provides: "get_ska_secret_key()'s lazy-mint branch (D-05), which this plan's return-expression edit sits directly below" +provides: + - "Length-prefixed (netstring-style) join in get_ska_secret_key(), replacing the bare '{0}{1}{2}'.format(...) concatenation" + - "TestSkaSecretKey.test_get_ska_secret_key -- pins the exact derived string for a known fixture, proves the fixture collides under the old scheme, and asserts the new scheme separates it" + - "TestSkaSecretKey.test_get_browser_hash -- regression guard pinning get_browser_hash's empty-string (not None) return" + - "CHANGES.rst bullets for this phase's three user-visible changes" +affects: [] + +tech-stack: + added: [] + patterns: + - "Netstring-style length-prefixed join (u'{0}:{1}'.format(len(part), part) per component, concatenated with no separator) for framing components that must not collide under concatenation" + - "Re-login (plone.app.testing.login) after installing a profile inside an integration-test setUp, when a test needs a memberdata property the profile just declared -- the fixture-login-cached user's property sheets predate the install" + +key-files: + created: [] + modified: + - src/imio/googleauthenticator/helpers.py + - src/imio/googleauthenticator/tests/test_helpers.py + - CHANGES.rst + +key-decisions: + - "BUG-04: netstring-style length-prefixed join, exact shape u'2:ab0:2:cd' for fixture (user_secret='ab', browser_hash='', ska_secret_key=u'cd'), per D-08" + - "get_browser_hash's except branch already returned '' before this plan (D-09 was stale); no production change, only a regression-guard test added, confirmed by reading helpers.py lines 210-225 before writing anything" + - "Test setUp re-logs in the fixture user (plone.app.testing.login) after _install() -- PLONE_FIXTURE's own login caches the test user's PAS property sheets before this profile's memberdata_properties.xml is applied, so a stale cached sheet silently drops setMemberProperties writes for the newly declared field (own test bug, Rule 1 auto-fix, not a plan deviation in production code)" + +requirements-completed: [BUG-04] + +coverage: + - id: D1 + description: "get_ska_secret_key() derives u'2:ab0:2:cd' for the known fixture (user_secret='ab', browser_hash='' via use_browser_hash=False, ska_secret_key=u'cd')" + requirement: "BUG-04" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_helpers.py#TestSkaSecretKey.test_get_ska_secret_key (EXACT SHAPE group)" + status: pass + human_judgment: false + - id: D2 + description: "Two component tuples sharing the same bare concatenation derive to different keys under the new scheme" + requirement: "BUG-04" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_helpers.py#TestSkaSecretKey.test_get_ska_secret_key (THE COLLISION IT PREVENTS group)" + status: pass + human_judgment: false + - id: D3 + description: "An already-non-empty ska_secret_key is not re-minted by the derivation" + requirement: "BUG-04" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_helpers.py#TestSkaSecretKey.test_get_ska_secret_key (MINT UNTOUCHED group)" + status: pass + human_judgment: false + - id: D4 + description: "get_browser_hash returns '' (not None) on a missing/unhashable User-Agent, and a real User-Agent still hashes to a 40-char hex digest" + requirement: "BUG-04" + verification: + - kind: unit + ref: "src/imio/googleauthenticator/tests/test_helpers.py#TestSkaSecretKey.test_get_browser_hash" + status: pass + human_judgment: false + - id: D5 + description: "The whole suite stays green with all four ska-derivation consumers unmodified" + requirement: "BUG-04" + verification: + - kind: unit + ref: "bin/test -t '!robot' (24 tests, 0 failures, 0 errors)" + status: pass + - kind: other + ref: "git diff --stat over pas_plugin.py, browser/forms/token.py, browser/forms/reset_bar_code.py, browser/forms/request_bar_code_reset.py -- empty" + status: pass + human_judgment: false + +duration: 12min +completed: 2026-07-29 +status: complete +--- + +# Phase 2 Plan 2: Registry Seeding and Import-Step Ordering (ska key separation) Summary + +**`get_ska_secret_key`'s bare `"{0}{1}{2}".format(...)` concatenation is replaced with a netstring-style length-prefixed join (`u'2:ab0:2:cd'` for the pinned fixture), with a test that proves the fixture collides under the old scheme and no longer does under the new one, plus a regression guard for `get_browser_hash`'s already-correct empty-string return.** + +## Performance + +- **Duration:** 12 min +- **Started:** 2026-07-29T12:54:16Z +- **Completed:** 2026-07-29T13:06:02Z +- **Tasks:** 2 +- **Files modified:** 3 (helpers.py, test_helpers.py, CHANGES.rst) + +## Accomplishments + +- `helpers.py`'s `get_ska_secret_key()` return statement now builds + `u''.join(u'{0}:{1}'.format(len(part), part) for part in (user_secret, browser_hash, ska_secret_key))` + instead of `"{0}{1}{2}".format(...)`. Component order (`user_secret`, `browser_hash`, + `ska_secret_key`) is unchanged; only the framing changed. Nothing else in the + function was touched -- the lazy-mint branch from plan 02-01 stays exactly as it was. +- New `TestSkaSecretKey` class in `test_helpers.py` (integration layer, real + `api.user.get_current()` member and real registry via `get_app_settings()`, + no fake user, no monkeypatch), with two methods: + - `test_get_ska_secret_key` -- asserts the exact derived string + `u'2:ab0:2:cd'` for the fixture (`user_secret='ab'`, `browser_hash=''`, + `ska_secret_key=u'cd'`), asserts the result is `unicode`, asserts the + fixture genuinely collides under bare concatenation + (`u'ab'+u''+u'cd' == u'a'+u''+u'bcd'`), then re-derives with the + second tuple and asserts the two derived keys are `assertNotEqual`, and + finally confirms the registry value is not re-minted. + - `test_get_browser_hash` -- asserts `get_browser_hash(request={})` returns + `''` (via `assertEqual` **and** `assertIsNotNone`, since both `''` and + `None` are falsy and a truthiness check would not discriminate), and + that a real `HTTP_USER_AGENT` still hashes to a 40-character hex digest. + Docstring states explicitly this is a regression guard, not a fix for a + live bug: `helpers.py` lines 210-225 already returned `''` from the + `except` branch before this plan touched anything, confirmed by reading + it first per the task's own instruction. Task 1's length-prefixed + derivation is what turns that `except` branch from cosmetic into a + `TypeError`-on-login-path crash guard (`len(None)` raises), so it is + worth pinning now even though it guards nothing broken today. +- `CHANGES.rst` gained four bullets under `1.0.0 (unreleased)`: the rename + (pre-existing), the import-step ordering dependency, the lazy + `ska_secret_key` mint, and the length-prefixed derivation with its + one-clause invalidation note. + +## Task Commits + +Each task was committed atomically: + +1. **Task 1: Length-prefixed derivation in get_ska_secret_key, with the collision it prevents asserted** - `80382a9` (feat) +2. **Task 2: Regression guard for get_browser_hash's empty-string return, and the changelog line** - `7fd95e3` (test) + +## Files Created/Modified + +- `src/imio/googleauthenticator/helpers.py` - `get_ska_secret_key`'s return statement replaced with the length-prefixed join (Task 1 only; untouched by Task 2) +- `src/imio/googleauthenticator/tests/test_helpers.py` - New `TestSkaSecretKey` class: `test_get_ska_secret_key` (Task 1), `test_get_browser_hash` (Task 2); module-level imports for `get_app_settings`, `get_browser_hash`, `get_ska_secret_key`, `plone.api`, `plone.app.testing.login`, `plone.app.testing.TEST_USER_NAME` +- `CHANGES.rst` - Three new bullets under `1.0.0 (unreleased)` (Task 2) + +## Stale-D-09 Check (Task 2's explicit output requirement) + +Confirmed explicitly: `helpers.py` lines 210-225 (`get_browser_hash`) **matched** +Task 2's `read_first` expectation exactly -- the `except` branch already reads +`return ''`, not a fall-off-the-end `None`. `02-CONTEXT.md`'s D-09 describing a +`None` return is stale (likely fixed incidentally during Phase 1's fail-closed +work per `02-01-SUMMARY.md`'s "Next Phase Readiness" note); no production +edit was made in Task 2, per its explicit instruction to stop and report if +the tree differed -- it did not differ, so only the regression-guard test was +added. + +## Exact Derived String Pinned (Task 2's explicit output requirement) + +For the fixture `(user_secret='ab', browser_hash='' [use_browser_hash=False], +ska_secret_key=u'cd')`, `get_ska_secret_key(...)` returns exactly +`u'2:ab0:2:cd'`, asserted by `assertEqual`, not substring/length/regex. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug in own test fixture] `setUp` re-logs in the test user after installing the add-on** + +- **Found during:** Task 1, writing `test_get_ska_secret_key` +- **Issue:** `setMemberProperties(mapping={'two_factor_authentication_secret': 'ab'})` + silently did not persist the value -- `get_ska_secret_key` derived + `u'0:0:2:cd'` instead of the expected `u'2:ab0:2:cd'`. Root cause traced via + direct inspection: `PLONE_FIXTURE`'s layer `testSetUp()` logs in + `TEST_USER_NAME` (`plone.testing.z2.login`) *before* this plan's + `TestSkaSecretKey.setUp()` runs and calls `self._install()`, which applies + `memberdata_properties.xml` for the first time. The login call constructs + and caches a PAS `PloneUser` whose `_propertysheets` (in + `Products.PlonePAS.plugins.ufactory.PloneUser`) are computed from + `portal_memberdata`'s schema *at that moment* -- before the add-on's three + new properties exist. `MemberData.getUser()` returns that same cached + object via acquisition, so `Products.PlonePAS.sheet.MutablePropertySheet.setProperties` + never finds `two_factor_authentication_secret` in the cached sheet's + `_properties` and silently drops it (exactly the class of hazard + `CLAUDE.md` documents for undeclared properties -- here the property *is* + declared, but the cached sheet predates the declaration). Confirmed by + printing `acl_users.getUserById(user.getId())`'s freshly-computed sheet, + which does have the property. +- **Fix:** `setUp()` calls `plone.app.testing.login(self.portal, TEST_USER_NAME)` + again immediately after `self._install()`, forcing a fresh `PloneUser` + (and fresh property sheets) to be built against the now-current + `portal_memberdata` schema. +- **Files modified:** `src/imio/googleauthenticator/tests/test_helpers.py` + (test fixture only; no production code touched) +- **Commit:** `80382a9` + +### None Other + +No further deviations. Task 1's production edit and Task 2's test-only +addition matched `02-PATTERNS.md`'s target shapes exactly; no blocking +issues, no architectural questions. + +## Issues Encountered + +The `setMemberProperties` staleness above was the only real obstacle; once +root-caused via direct inspection of `Products.PlonePAS` internals +(`ufactory.py`, `property.py`, `sheet.py`), the fix was a one-line addition +to the test fixture, not a production change. + +## User Setup Required + +None - no external service configuration required. + +## Next Phase Readiness + +- BUG-04, REG-01 through REG-05 are now all complete; Phase 2 (registry + seeding and import-step ordering) is fully executed across both plans. +- The `costly`-reversibility prohibition on this plan (Task 1) stands: the + derivation must not be reopened once any user is enrolled or any signed + URL is in flight, without an explicit migration. +- Full suite (`bin/test -t '!robot'`) is green at 24 tests, 0 failures, 0 + errors, after both task commits. + +--- +*Phase: 02-registry-seeding-and-import-step-ordering* +*Completed: 2026-07-29* + +## Self-Check: PASSED + +All created/modified files and both task commit hashes verified present on disk / in git log. diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md new file mode 100644 index 0000000..f6e0461 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md @@ -0,0 +1,261 @@ +# Phase 2: Registry Seeding and Import-Step Ordering - Context + +**Gathered:** 2026-07-29 +**Status:** Ready for planning + + +## Phase Boundary + +Installing this add-on seeds the `IGoogleAuthenticatorSettings` registry records reliably instead +of relying on CPython 2.7 string-hash order over GenericSetup step ids; the ordering that makes it +work is *declared* (``) and *asserted* in the suite; the nested +`runImportStepFromProfile` re-entry is gone, with `ska_secret_key` minted by a lazy accessor; and +the derived `ska` signing key stops being a bare concatenation of its three components. + +Requirements: REG-01 … REG-05, BUG-04. + +**Not this phase:** seed encryption and the `SKA_*`/Fernet key handling (Phase 3), the PAS +credentials-boundary rework (Phase 4), replay + lockout state (Phase 5), recovery codes (Phase 6), +override deletion and `id = 'login_form'` (Phase 7), lint debt and coverage (Phase 8). + + + + +## Implementation Decisions + +### REG-01 verification strategy — *discussed* + +- **D-01:** REG-01 is proven by the **ordering assertion only**. No manual site-creation run is + performed and recorded, and no automated second-site fixture + (`addPloneSite(app, ..., extension_ids=(...))`) is added. Rejected the automated route on cost: + a real second Plone site inside the layer is ~30–60s of suite time on a suite Phase 8 already + has to repair. Rejected the manual-run-and-paste route as evidence that decays the moment + anything else changes. + +- **D-02:** ROADMAP success criterion 1 ("Creating a new Plone site … completes with no + `IGoogleAuthenticatorSettings defines a field ska_secret_key, for which there is no record` in + `var/log/instance.log`") is carried in `must_haves` as a **structured backstop marker** — + `{ statement: , verification: backstop }` — not as a plain truth and not as a + parenthetical note. Consequence, and the point of choosing it: at verify time the verifier + cannot confirm it from any artifact, so it abstains to `human_needed` rather than silently + passing. This is the honest disposition given D-01, and it is the *only* must_have in this + phase that is not machine-checkable. + +- **D-03:** The mechanised control asserts **both halves**, as two assertions: + 1. *Ordering* — `steps = portal.portal_setup.getSortedImportSteps()`, then + `steps.index('imio.googleauthenticator') > steps.index('plone.app.registry')`. This is + REG-03 as written, and it is what catches someone deleting the `` line. + 2. *Outcome* — after the profile is applied, all three `IGoogleAuthenticatorSettings` records + exist and `get_app_settings()` returns without raising. This is the property the original + bug report was actually about, and it survives any future restructuring away from ``. + + Ordering-only was rejected because it is tied to one mechanism; outcome-only was rejected + because REG-03 names `getSortedImportSteps()` explicitly. + +### Claude's Discretion + +Three areas the user chose not to discuss. Decisions taken and recorded so downstream agents do +not re-open them. + +#### Lazy mint mechanics (REG-04) + +- **D-04:** `_setup_secret_key` is **deleted outright** — both the nested + `runImportStepFromProfile` and the seeding that followed it. `setupVarious` is left doing only + the marker-file guard and `_add_plugin`. No install-time seeding is retained as a fallback: + keeping it would put a registry read back inside the import step, which is the exact coupling + REG-04 exists to remove. + +- **D-05:** The mint lives **inside `get_ska_secret_key()`**, unconditionally, as a single + `if not ska_secret_key:` branch. No `create=` keyword argument and no separate + `get_or_create_ska_secret_key()` wrapper — one branch, one birthplace for the key. Randomness + stays `unicode(uuid4())`, unchanged from the deleted `_setup_secret_key`; raising the site key's + entropy is not asked for by any Phase 2 requirement and belongs with Phase 3's `SEC-06` work + (see Deferred Ideas). + +- **D-06:** Two consequences of D-05 are **accepted, with reasons recorded**, so they are not + rediscovered as bugs: + - *The mint can fire from the PAS plugin.* `pas_plugin.py:160` calls `sign_user_data` → + `get_ska_secret_key`, and PROJECT.md's constraint is that writes on that path are lost when the + request aborts. Accepted because the failure is self-healing rather than silent: a lost mint + means the URL just signed will not validate, the user retries, and the next request mints + again. It is not the lockout-counter hazard that constraint was written for — losing a counter + increment means the lock never locks, whereas losing a mint means one failed login. + - *The mint can fire from an unauthenticated request.* `validate_user_data` (`token.py:87`, + `reset_bar_code.py:150`) also routes through `get_ska_secret_key`. Accepted: the write is + idempotent, bounded to once per site, and reaches the same state the first login attempt would + have reached anyway. + - *Concurrent mints across ZEO clients resolve correctly.* Two clients writing the same registry + record conflict, ZODB retries the request, and the retry re-reads the now-committed non-empty + value and skips the branch. This is the same merge-and-retry reasoning PROJECT.md already + records for `OOBTree` storage. + +- **D-07:** When the record is **missing entirely** (not merely empty), `get_app_settings()`'s + `KeyError` **propagates**. It is not caught, and `forInterface(check=False)` is not used — + research names that as the escape hatch that converts a loud `KeyError` into an `AttributeError` + deeper in the stack. With `_dont_swallow_my_exceptions = True` live from Phase 1, that surfaces + as a 500, which is the correct fail-closed behaviour for an MFA package. + +#### `ska` key separation (BUG-04) + +- **D-08:** `helpers.py:259`'s `"{0}{1}{2}".format(user_secret, browser_hash, ska_secret_key)` is + replaced with a **length-prefixed join** — each component rendered as `:` and + concatenated, netstring-style. A single-delimiter scheme (`u"|".join(...)`) was rejected: it is + only safe while no component can contain the delimiter, and Phase 3 changes `user_secret` into a + `v1$` ciphertext, so the "this character can't appear" argument expires one phase + from now. An HMAC-based derivation was rejected as overkill — the stated defect is collidability, + not weak derivation, and hashing on the login path buys nothing here. + — **Reversibility:** costly — the derivation feeds `sign_user_data`, `validate_user_data` and + `request_bar_code_reset.py:66`; changing it later invalidates every signed token URL in flight + and every outstanding bar-code-reset email link. Free *now* only because nothing is deployed and + no users are enrolled, which is precisely why this sits in Phase 2 rather than later. + +- **D-09:** `get_browser_hash` (`helpers.py:210-224`) is fixed **in this phase** to + `return ''` from its `except` branch. Today it falls off the end and returns `None`, which + `format()` renders as the literal string `"None"`; under D-08's length-prefixing, `len(None)` + raises `TypeError` instead — so BUG-04 converts a latent wrong-key bug into a crash on a login + path, and the two changes must land together. `''` is the right value: it is exactly what + `use_browser_hash=False` already produces, and a client with no `User-Agent` gets no device + binding either way — the current `"None"` is a constant, so it binds nothing today. No security + regression, and it matches how Phase 1 fixed the three other crash-on-ordinary-input paths. + +#### Import-step guard scope (REG-02) + +- **D-10:** Keep the custom import step and add `` to it in + `configure.zcml:44-49`. `post_handler` (research's Pitfall 7 option 2) is **rejected**: it is not + run by `runImportStepFromProfile` nor by upgrade steps, and that caveat costs more than the + ordering guarantee is worth when one `` line achieves the same thing. + +- **D-11:** The `` is retained **even though D-04 leaves `setupVarious` no longer reading + the registry**, which makes it strictly belt-and-braces for this phase. Kept because REG-02 and + REG-03 mandate it, because Phase 3 puts registry reads back on install-adjacent paths, and + because it is one line guarded by a permanent assertion (D-03). + +- **D-12:** `profiles/default/registry.xml`'s bare `` stays as-is. It is + the correct idiom for this schema — `TextLine`, `Bool` and `Text` all have `IPersistentField` + adapters and none is `readonly`, so neither of Pitfall 8's first two holes applies. + +- **D-13:** REG-05's double-apply test is a **regression guard, not a live bug fix** — and the plan + must say so, or someone will go looking for a bug that isn't there. `ska_secret_key` is + `TextLine(required=False, default=u'')` (`browser/controlpanel.py:28-33`), so an existing + non-empty unicode revalidates cleanly on re-import and Pitfall 8's hole 3 does not fire today. + It fires the day someone adds `required=True` or a constraint. The test must set a **known** + value, re-apply the profile, and assert **equality with that value** — asserting merely + "non-empty" would pass against a fresh re-mint and prove nothing. + + + + +## Canonical References + +**Downstream agents MUST read these before planning or implementing.** + +### Project planning +- `.planning/PROJECT.md` — constraints (notably the `transaction.abort()` / state-writes rule that + D-06 reasons against), Key Decisions, Out of Scope +- `.planning/REQUIREMENTS.md` — REG-01…REG-05 and BUG-04 as written, plus the Open Decisions table + (none of the four land in this phase) +- `.planning/ROADMAP.md` §"Phase 2: Registry Seeding and Import-Step Ordering" — goal, the five + success criteria, and the phase notes carrying the site-creation-asymmetry MEDIUM +- `.planning/phases/01-rename-and-fail-closed/01-CONTEXT.md` — Phase 1's decisions, in particular + D-09 (profile version reset to `1000`) and the fail-closed break-glass reasoning + +### Research (primary-source, read against the installed eggs) +- `.planning/research/PITFALLS.md` §"Pitfall 6" (lines 278-347) — `getSortedImportSteps()` builds a + Python 2 `set`; `_computeTopologicalSort` inserts dependency-free steps in string-hash order. + **The root cause. Read before touching `configure.zcml`.** +- `.planning/research/PITFALLS.md` §"Pitfall 7" (lines 351-439) — the four things + `runImportStepFromProfile` does that nobody asked for, and the three fix options ranked +- `.planning/research/PITFALLS.md` §"Pitfall 8" (lines 443-504) — the three silent holes in + ``; hole 3 is what REG-05 guards against +- `.planning/research/SUMMARY.md` lines 48-50, 164, 249 — the reconciled account, including the + explicit warning that a post-rename disappearance of the error is **not** evidence of a fix +- `.planning/codebase/TESTING.md` — current layer setup and the known isolation problem +- `.planning/codebase/CONVENTIONS.md` — naming, import ordering, logging patterns to preserve + +### Source read during this discussion (file:line, verified) +- `src/imio/googleauthenticator/setuphandlers.py:33-44` — `_setup_secret_key`, the 3-line nested + import plus the seeding that D-04 deletes +- `src/imio/googleauthenticator/configure.zcml:44-49` — the dependency-free `importStep` + declaration that D-10 amends +- `src/imio/googleauthenticator/profiles/default/metadata.xml` — already declares + `profile-plone.app.registry:default`. **This is a *profile* dependency + and does not order import steps** — do not mistake it for the fix +- `src/imio/googleauthenticator/helpers.py:228-259` — `get_ska_secret_key`, the bare concat +- `src/imio/googleauthenticator/helpers.py:210-224` — `get_browser_hash` and its `None` return +- `src/imio/googleauthenticator/browser/controlpanel.py:24-48` — + `IGoogleAuthenticatorSettings`; three fields, all persistable, none readonly +- `/srv/cache/eggs/plone.app.registry-1.7.9-py2.7.egg/plone/app/registry/exportimport/configure.zcml:10-18` + — the step id is `plone.app.registry`, and it declares **three** dependencies: + `componentregistry`, `toolset`, `typeinfo`. PITFALLS.md line 304 lists only the first two — + minor correction, relevant if anyone reasons about the sort by hand + + + + +## Existing Code Insights + +### Reusable Assets +- **`tests/base.py` + `IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING`** — the existing layer already + applies the profile (`test_generic.py:_install()`), so both new tests hang off machinery that + exists. `IntegrationTesting` aborts its transaction per test, so a double-`applyProfile` (D-13) + does not leak into sibling tests. +- **`test_generic.py:test_product_is_installed`** — the closest analog for the new setup tests: + same layer, same `qi_tool` fixture, same shape. + +### Established Patterns +- `.isort.cfg`: `force_alphabetical_sort`, `force_single_line`, `line_length = 120`. New imports in + `helpers.py` and the test module follow it; the lint sweep itself is Phase 8. +- Helpers are thin, snake_case, `get_`/`is_`/`validate_`-prefixed, with reStructuredText + `:param Type name:` docstrings. D-05's mint branch lives inside an existing helper rather than + introducing a new public function. + +### Integration Points +- **`pas_plugin.py:160`** (`sign_user_data`) is the *only* caller on an abort-prone path. The two + `validate_user_data` callers — `token.py:87` and `reset_bar_code.py:150` — are views, which + commit. `request_bar_code_reset.py:66` calls `get_ska_secret_key` directly. Four call sites + total; all four are affected by D-08's derivation change. +- **The marker-file guard** — `setupVarious` returns silently unless + `context.readDataFile('imio.googleauthenticator.marker.txt')` is non-None. D-04 shrinks the + function but must not touch this guard. +- **`_dont_swallow_my_exceptions = True`** (Phase 1) is what makes D-07's propagating `KeyError` a + visible 500 rather than a fallthrough to password-only auth. + + + + +## Specific Ideas + +- **The assertion is the control, not the rename.** The roadmap says it twice and research says it + three times: if the `no record` error disappears after Phase 1's rename, that is a changed string + hash, not a fix. Any plan that treats "we can't reproduce it any more" as evidence is wrong. +- **Settle the carried-forward MEDIUM by observation, not argument.** Which of the four + `runImportStepFromProfile` mechanisms fires on the site-creation path is unresolved. Research is + explicit that the recommended fix does not depend on the answer, so it is **not** a blocker for + this phase — if it is cheap to print `getSortedImportSteps()` while writing D-03's test, record + what it shows; do not spend a task on it. + + + + +## Deferred Ideas + +- **Raise the `ska_secret_key` entropy** — `unicode(uuid4())` is ~122 bits for what is a signing + key. `binascii.hexlify(os.urandom(32))` is a one-line upgrade. Belongs with Phase 3, which + already touches secret entropy for `SEC-06` (the 160-bit TOTP seed) and can change both under one + rationale. Not in scope here: no Phase 2 requirement asks about entropy. +- **`ska_secret_key` is rendered into the control-panel HTML** — `browser/controlpanel.py` puts the + site signing key in a form field, so it is visible to anyone reaching the control panel and lands + in browser history/caches. Phase 3 is the secret-handling phase and the right place to decide + whether it becomes write-only or leaves the registry entirely (PROJECT.md's Pitfall 13 line + already leans that way for secret-adjacent values). +- **A manual site-creation smoke run** — explicitly not done (D-01). If REG-01's backstop truth + (D-02) reaches verify-phase and someone wants to close it by hand, this is the procedure: + `bin/instance fg`, create a site with the add-on ticked, then + `grep -e "no record" -e "Cannot find registry" var/log/instance.log`. + + + +--- + +*Phase: 2-Registry Seeding and Import-Step Ordering* +*Context gathered: 2026-07-29* diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-DISCUSSION-LOG.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-DISCUSSION-LOG.md new file mode 100644 index 0000000..7c63fd8 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-DISCUSSION-LOG.md @@ -0,0 +1,106 @@ +# Phase 2: Registry Seeding and Import-Step Ordering - Discussion Log + +> **Audit trail only.** Do not use as input to planning, research, or execution agents. +> Decisions are captured in CONTEXT.md — this log preserves the alternatives considered. + +**Date:** 2026-07-29 +**Phase:** 2-registry-seeding-and-import-step-ordering +**Areas discussed:** REG-01 verification strategy + +--- + +## Area selection + +Four gray areas were offered; one was selected. + +| Area | Description | Selected | +|------|-------------|----------| +| Lazy mint mechanics | REG-04 turns a read path into a write path — which request commits it, PAS-plugin abort risk, ZEO concurrency | | +| ska key separation (BUG-04) | Delimiter vs length-prefix vs HMAC; the `get_browser_hash` `None` path | | +| Import-step guard scope | Keep `` once the handler stops reading the registry, or use `post_handler` | | +| REG-01 verification strategy | How to prove a symptom that isn't unit-reproducible | ✓ | + +**Notes:** The three unselected areas were resolved under Claude's Discretion (D-04…D-13 in +CONTEXT.md) rather than left open. + +--- + +## REG-01 verification strategy + +### Q1 — How do we prove REG-01? + +| Option | Description | Selected | +|--------|-------------|----------| +| Manual run + ordering assertion | One recorded `bin/instance fg` site creation, grep `var/log/instance.log` for `no record` and `Cannot find registry`, paste the empty output into the phase SUMMARY; ordering assertion as the permanent control | | +| Add an automated site-creation test | `addPloneSite(app, 'reg01', extension_ids=('imio.googleauthenticator:default',))` in a fresh site, asserting no exception plus records present. Proves the literal REG-01 sentence forever, at ~30–60s of suite time | | +| Ordering assertion only | Treat REG-01 as satisfied by construction once the nested import is gone and the dependency is declared | ✓ | + +**User's choice:** Ordering assertion only. +**Notes:** Claude flagged before the question that the lazy accessor (REG-04) makes the original +`KeyError` unreachable on the install path regardless of step order, so the ordering assertion is +really guarding Phase 3+ rather than REG-01 itself. Consequence of this choice — that success +criterion 1 ends up with no evidence attached — was raised immediately as Q2 rather than left +implicit. + +### Q2 — How does the plan carry ROADMAP success criterion 1? + +| Option | Description | Selected | +|--------|-------------|----------| +| Backstop truth | `{ statement, verification: backstop }` in `must_haves`; the verifier abstains to `human_needed` rather than silently passing | ✓ | +| Narrow the criterion to what's asserted | Rewrite ROADMAP criterion 1 to claim only what the phase proves; nothing left unevidenced, but the roadmap stops claiming the user-visible symptom is verified | | +| Leave it and accept the flag | Write it as a plain truth and deal with the verifier's complaint in verify-phase | | + +**User's choice:** Backstop truth. +**Notes:** Recorded as D-02. This is the only must_have in the phase that is not machine-checkable; +CONTEXT.md says so explicitly so the planner does not try to invent a check for it. + +### Q3 — What does the mechanised control assert? + +| Option | Description | Selected | +|--------|-------------|----------| +| Both ordering and outcome | `steps.index(...)` comparison **and** post-`applyProfile` records-exist / `get_app_settings()` doesn't raise | ✓ | +| Ordering index only | Exactly REG-03 as worded; minimal and directly traceable, but tied to the `` mechanism | | +| Outcome only | Tests the property that matters and survives refactoring, but doesn't satisfy REG-03, which names `getSortedImportSteps()` explicitly | | + +**User's choice:** Both ordering and outcome. +**Notes:** Recorded as D-03. Before this question Claude verified against the installed egg that +the step id is `plone.app.registry` and that it declares three dependencies +(`componentregistry`, `toolset`, `typeinfo`) — PITFALLS.md line 304 lists only two. The correction +is carried into CONTEXT.md's canonical refs. + +### Q4 — Continue or write context? + +| Option | Description | Selected | +|--------|-------------|----------| +| I'm ready for context | Write CONTEXT.md; unpicked areas get Claude's decisions with rationale | ✓ | +| Explore more gray areas | Surface further areas — e.g. how the ska-collision test picks colliding component tuples, whether the REG-05 double-apply test pollutes the shared layer | | + +**User's choice:** I'm ready for context. + +--- + +## Claude's Discretion + +Three areas the user chose not to discuss, resolved with rationale in CONTEXT.md: + +- **Lazy mint mechanics (D-04…D-07)** — `_setup_secret_key` deleted outright with no install-time + fallback; the mint is one unconditional branch inside `get_ska_secret_key()`; three consequences + (PAS-plugin abort, unauthenticated trigger, ZEO concurrency) accepted with reasons; a missing + record propagates its `KeyError` rather than being papered over with `forInterface(check=False)`. +- **ska key separation (D-08, D-09)** — length-prefixed netstring join, chosen over a `|` delimiter + because Phase 3 turns `user_secret` into `v1$` and the "this character can't + appear" argument expires; `get_browser_hash`'s `None` return fixed to `''` in the same change, + because length-prefixing turns that latent wrong-key bug into a `TypeError` on a login path. +- **Import-step guard scope (D-10…D-13)** — `` on the custom step, `post_handler` + rejected for its `runImportStepFromProfile`/upgrade-step caveat; the `` kept even though + D-04 makes it belt-and-braces; `` left as-is; REG-05 documented as a + regression guard, with the test required to assert equality against a known value rather than + mere non-emptiness. + +## Deferred Ideas + +- Raise the `ska_secret_key` entropy from `uuid4()` (~122 bits) — Phase 3, alongside SEC-06. +- `ska_secret_key` is rendered into the control-panel HTML form — Phase 3 decides whether it + becomes write-only or leaves the registry. +- The manual site-creation smoke run, explicitly not done here — procedure recorded in CONTEXT.md + in case someone closes D-02's backstop by hand at verify time. diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md new file mode 100644 index 0000000..64070f6 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md @@ -0,0 +1,367 @@ +# Phase 2: Registry Seeding and Import-Step Ordering - Pattern Map + +**Mapped:** 2026-07-29 +**Files analyzed:** 3 modified source files + 1 modified test file (or new test module) +**Analogs found:** 4 / 4 — this phase edits existing files in place; every "analog" is the file's +own current content, since there is no comparable sibling elsewhere in the tree for a +GenericSetup import-step/registry-seeding fix. + +## File Classification + +| New/Modified File | Role | Data Flow | Closest Analog | Match Quality | +|-------------------|------|-----------|----------------|---------------| +| `src/imio/googleauthenticator/setuphandlers.py` | config/install handler | event-driven (GS install) | itself, `_setup_secret_key`/`setupVarious` (deleted/shrunk) | exact | +| `src/imio/googleauthenticator/configure.zcml` | config (ZCML) | n/a | itself, the `` block | exact | +| `src/imio/googleauthenticator/helpers.py` | utility | request-response (signing) | itself, `get_ska_secret_key` / `get_browser_hash` | exact | +| `src/imio/googleauthenticator/tests/test_generic.py` (or new `test_setuphandlers.py`) | test | request-response / CRUD (registry) | `test_generic.py::test_product_is_installed` + `tests/base.py` | exact | + +## Pattern Assignments + +### `src/imio/googleauthenticator/setuphandlers.py` (config/install handler, event-driven) + +**Analog:** itself, current lines 1-63 (read in full above). + +**Current state to delete (D-04):** +```python +from uuid import uuid4 +... +def _setup_secret_key(portal): + """ + Generate secret key + """ + portal.portal_setup.runImportStepFromProfile( + 'profile-imio.googleauthenticator:default', + 'plone.app.registry' + ) + + settings = get_app_settings() + if not settings.ska_secret_key: + settings.ska_secret_key = unicode(uuid4()) +``` +and its one call site inside `setupVarious`: +```python + portal = context.getSite() + + _setup_secret_key(portal) + + pas = portal.acl_users + _add_plugin(pas) +``` + +**Target shape** — `setupVarious` keeps only the marker guard and `_add_plugin`: +```python +def setupVarious(context): + """ + @param context: Products.GenericSetup.context.DirectoryImportContext instance + """ + + # We check from our GenericSetup context whether we are running + # add-on installation for your product or any other proudct + if context.readDataFile('imio.googleauthenticator.marker.txt') is None: + # Not your add-on + return + + portal = context.getSite() + + pas = portal.acl_users + _add_plugin(pas) +``` +Drop the now-unused `from uuid import uuid4` and `from imio.googleauthenticator.helpers import +get_app_settings` imports at the top of the file (both become dead once `_setup_secret_key` is +gone) — `.isort.cfg` `force_single_line`/`force_alphabetical_sort` conventions apply to whatever +import block remains; `_add_plugin` still needs its existing +`from imio.googleauthenticator.pas_plugin import GoogleAuthenticatorPlugin` and +`from zope.i18nmessageid import MessageFactory` lines untouched. + +**Error handling:** none added or removed here — `setupVarious` has never wrapped +`_add_plugin`/registry access in try/except, and D-07 explicitly wants `KeyError` to propagate +uncaught later in `get_ska_secret_key`/`get_app_settings`, not here. + +--- + +### `src/imio/googleauthenticator/configure.zcml` (ZCML config, n/a data flow) + +**Analog:** itself, lines 44-49, the existing dependency-free `importStep`: +```xml + +``` + +**Target shape (D-10)** — add a `` child element: +```xml + + + +``` +Note the closing-tag change (self-closing `/>` becomes `>...`) — this is +the one line-shape gotcha an executor doing a text-only diff can miss. No other element in this +file uses ``, so there is no second in-repo example to cross-check against; the shape is +`plone.app.registry`'s own step declaration +(`/srv/cache/eggs/plone.app.registry-1.7.9-py2.7.egg/plone/app/registry/exportimport/configure.zcml:10-18`, +cited in CONTEXT.md canonical refs) which uses the identical `` child-element +form for its own three dependencies (`componentregistry`, `toolset`, `typeinfo`). + +--- + +### `src/imio/googleauthenticator/helpers.py` (utility, request-response) + +**Analog:** itself, current `get_ska_secret_key` (lines 228-259) and `get_browser_hash` +(lines 210-225). + +**`get_browser_hash` — already matches D-09.** Current code already does: +```python +def get_browser_hash(request=None): + ... + try: + return sha1(request.get('HTTP_USER_AGENT')).hexdigest() + except Exception as e: + logger.debug(str(e)) + return '' +``` +This already returns `''` from the `except` branch, not `None` — CONTEXT.md's D-09 describes it as +falling through to `None`, but that no longer matches the installed tree (likely fixed incidentally +in Phase 1's fail-closed work). **No change needed here**; the planner/executor should verify this +during implementation and treat D-09 as already satisfied rather than re-doing it, but should still +add/keep a regression test asserting `get_browser_hash` returns `''` (not `None`) on a missing/bad +`HTTP_USER_AGENT`, since `test_helpers.py` today has no test for this function at all. + +**`get_ska_secret_key` — needs both D-05 (lazy mint) and D-08 (netstring join).** Current: +```python +def get_ska_secret_key(request=None, user=None, use_browser_hash=True): + """ + Gets the `secret_key` to be used in `ska` package. + ... + """ + if request is None: + request = getRequest() + + if user is None: + user = api.user.get_current() + + settings = get_app_settings() + + ska_secret_key = settings.ska_secret_key + + user_secret = user.getProperty('two_factor_authentication_secret') + + if use_browser_hash: + browser_hash = get_browser_hash(request=request) + else: + browser_hash = '' + + return "{0}{1}{2}".format(user_secret, browser_hash, ska_secret_key) +``` + +**Target shape** — single `if not ska_secret_key:` mint branch (D-05, randomness unchanged from the +deleted `_setup_secret_key`: `unicode(uuid4())`), plus a length-prefixed join replacing the bare +`.format()` concatenation (D-08). This requires `from uuid import uuid4` to move from +`setuphandlers.py` into `helpers.py` (it's already imported there at line 7 — reuse it, do not +re-import): +```python + settings = get_app_settings() + + ska_secret_key = settings.ska_secret_key + if not ska_secret_key: + ska_secret_key = unicode(uuid4()) + settings.ska_secret_key = ska_secret_key + + user_secret = user.getProperty('two_factor_authentication_secret') + + if use_browser_hash: + browser_hash = get_browser_hash(request=request) + else: + browser_hash = '' + + return u''.join( + u'{0}:{1}'.format(len(part), part) + for part in (user_secret, browser_hash, ska_secret_key) + ) +``` +(Exact netstring formatting — `len(part):part` per component, concatenated with no separator — is +D-08's own description; the executor should pick concrete syntax matching this module's existing +generator/comprehension style, e.g. `get_ip_ranges`'s list-comprehension-with-try style at lines +517-531 for comparison, though a plain generator expression as above is idiomatic enough and +matches `get_app_settings`'s and neighboring functions' terseness.) + +**Import ordering note:** `uuid4` is already imported at the top of `helpers.py` (line 7, +`from uuid import uuid4`) — used today by `generate_secret`. No new import line needed for the mint +branch. + +**Four call sites affected by the D-08 derivation change** (none need edits themselves — they all +call `get_ska_secret_key`/`sign_user_data`/`validate_user_data` and are correct as long as both +sides of every signed URL use the same derivation): +- `pas_plugin.py:160` — `sign_user_data(request=request, user=user, url=...)` +- `browser/forms/token.py:87` — `validate_user_data(request=..., user=..., use_browser_hash=...)` +- `browser/forms/reset_bar_code.py:150` — `validate_user_data(request=self.request, user=...)` +- `browser/forms/request_bar_code_reset.py:66` — `get_ska_secret_key(request=self.request, user=user)` +All four are call sites, not definition sites — verify each still reads the same way after the +edit; no diff is expected in these four files. + +--- + +### `src/imio/googleauthenticator/tests/test_generic.py` or new `test_setuphandlers.py` (test) + +**Analog:** `test_generic.py::test_product_is_installed` (lines 27-34) — same layer, same +`qi_tool`/`setUp` shape: +```python +class TestGeneric(unittest.TestCase, BaseTest): + + layer = IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING + + def setUp(self): + self.app = self.layer['app'] + self.portal = self.layer['portal'] + self.qi_tool = getToolByName(self.portal, 'portal_quickinstaller') + self.portal_url = api.portal.get().absolute_url() + self._install() + + def test_product_is_installed(self): + pid = 'imio.googleauthenticator' + installed = [p['id'] for p in self.qi_tool.listInstalledProducts()] + self.assertTrue(pid in installed, + u'package appears not to have been installed') +``` +`self._install()` (from `tests/base.py:6-28`) drives a real testbrowser through +`prefs_install_products_form` — this is how the profile actually gets applied in this layer +(`base.py:16-17` docstring: "generic setup profile is not applied correctly by plone.app.testing in +this testing layer"). **New D-03 tests can reuse this exact `setUp`/`_install()` pattern**; no new +fixture machinery is needed. + +**D-03 half 1 — ordering assertion.** No existing test calls `getSortedImportSteps()`; this is new +ground, but `self.portal.portal_setup` is accessed the same way other tests reach tools +(`getToolByName(self.portal, ...)` idiom used throughout `test_generic.py` and `test_helpers.py`): +```python + def test_import_step_runs_after_plone_app_registry(self): + portal_setup = getToolByName(self.portal, 'portal_setup') + steps = portal_setup.getSortedImportSteps() + self.assertGreater( + steps.index('imio.googleauthenticator'), + steps.index('plone.app.registry')) +``` + +**D-03 half 2 — outcome assertion**, chained onto the same test or a sibling, using +`get_app_settings()` per CONTEXT.md's D-07 (propagating `KeyError` if a record is missing): +```python + def test_registry_records_exist_after_install(self): + from imio.googleauthenticator.helpers import get_app_settings + settings = get_app_settings() # raises KeyError if any record is missing + self.assertIsNotNone(settings.ska_secret_key) +``` +Import placement: module-level, alongside the file's other `from imio.googleauthenticator...` +imports (single-import-per-line, per `.isort.cfg` `force_single_line` — see `test_helpers.py:7-11` +for the convention already followed in this package's test modules), not a function-local import +as shown inline above for brevity. + +**D-13 — double-apply value-preservation regression test.** No existing test calls `applyProfile` +directly (`_install()` goes through the QuickInstaller browser form instead), so this is the one +genuinely new pattern in this phase. Use `self.portal.portal_setup.applyProfile(...)` directly +(bypassing `_install()`'s browser dance, since the product is already installed by `setUp`) with a +**known** seeded value, not an assertion of mere non-emptiness: +```python + def test_ska_secret_key_survives_reapply(self): + from imio.googleauthenticator.helpers import get_app_settings + settings = get_app_settings() + settings.ska_secret_key = u'known-test-value' + self.portal.portal_setup.applyProfile('imio.googleauthenticator:default') + self.assertEqual(u'known-test-value', get_app_settings().ska_secret_key) +``` +`IntegrationTesting` aborts its transaction per test (Phase 1 PATTERNS.md, confirmed again here), +so this does not leak `known-test-value` into sibling tests. + +**BUG-04 component-separation test** — belongs in `test_helpers.py` (role/data-flow match: +request-response, secret derivation), next to the existing `TestIPWhitelisting` class or as a new +`TestSkaSecretKey` class using the same layer: +```python + def test_ska_key_components_do_not_collide(self): + """BUG-04 regression: '{0}{1}{2}'.format(a, b, c) let (user_secret='ab', browser_hash='', + ska='cd') collide with (user_secret='a', browser_hash='', ska='bcd'). The netstring-style + join must keep them distinct.""" + from imio.googleauthenticator.helpers import get_ska_secret_key + ... # construct two fake users/settings with colliding concatenations, + # assert get_ska_secret_key returns different strings for each +``` +No existing test in this package fakes `get_app_settings`/user objects for `get_ska_secret_key` +directly; the closest structural analog for "monkeypatch `helpers.get_app_settings`, restore in +`finally`" is `test_helpers.py:46-58` +(`test_get_ip_addresses_whitelist_drops_blank_lines`), which patches `helpers.get_app_settings` +with a `FakeSettings` class and restores the original in a `finally` block — copy that shape for +whatever fake settings/user objects the collision test needs. + +--- + +## Shared Patterns + +### Monkeypatching `helpers.get_app_settings` for a fake registry value +**Source:** `test_helpers.py:46-58` +**Apply to:** the BUG-04 collision test and D-13's known-value test if `get_app_settings` needs +faking rather than driving the real registry through `IRegistry`. +```python +from imio.googleauthenticator import helpers + +class FakeSettings(object): + ip_addresses_whitelist = '127.0.0.1\n192.168.0.0/16\n' + +original = helpers.get_app_settings +helpers.get_app_settings = lambda: FakeSettings() +try: + ... +finally: + helpers.get_app_settings = original +``` + +### Test module import block (single-import-per-line) +**Source:** `test_helpers.py:1-11` +**Apply to:** any new test file/class in this phase +```python +import unittest2 as unittest + +from imio.googleauthenticator.testing import \ + IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING +from imio.googleauthenticator.tests.base import BaseTest + +from imio.googleauthenticator.helpers import extract_ip_address_from_request +from imio.googleauthenticator.helpers import get_ip_addresses_whitelist +from imio.googleauthenticator.helpers import get_ip_ranges +``` +Note `test_generic.py`'s own import block (multi-name `from plone.app.testing import A, B, C, D`) +violates this and is **pre-existing debt, not a pattern to copy** — new imports in either file +should use the single-name-per-line form shown above; do not propagate the multi-name style further. + +### `getToolByName` for tool access +**Source:** used throughout `test_generic.py` and this phase's new tests (`portal_quickinstaller`, +`portal_setup`, `acl_users` in Phase 1's `test_pas_plugin.py`) +**Apply to:** `portal_setup` access for `getSortedImportSteps()` and `applyProfile()`. + +### `_dont_swallow_my_exceptions = True` (Phase 1, unrelated file but load-bearing here) +**Source:** PAS plugin class attribute set in Phase 1 +**Apply to:** why D-07's propagating `KeyError` from `get_app_settings()` surfaces as a 500 instead +of silently falling through — no code in this phase touches this attribute, but D-03/D-07's tests +rely on it having already landed. + +## No Analog Found + +| File | Role | Data Flow | Reason | +|------|------|-----------|--------| +| `` ZCML shape | config | n/a | No second `` usage exists in this package's own `configure.zcml`; shape sourced from the installed `plone.app.registry` egg's own `configure.zcml` (cited in CONTEXT.md canonical refs), not from an in-repo analog | +| Netstring-style join in `get_ska_secret_key` | utility | request-response | No existing helper in this codebase does length-prefixed joining; D-08's rationale (delimiter-collision safety ahead of Phase 3's Fernet-token `user_secret`) is itself the design source, not a copied pattern | +| `applyProfile()` double-apply test | test | CRUD (registry) | No existing test calls `applyProfile` directly (`_install()` goes through the QuickInstaller browser form); D-13's test is genuinely new machinery, built from `portal_setup` tool access already used elsewhere | + +## Metadata + +**Analog search scope:** `src/imio/googleauthenticator/setuphandlers.py`, +`src/imio/googleauthenticator/configure.zcml`, `src/imio/googleauthenticator/helpers.py`, +`src/imio/googleauthenticator/tests/{test_generic.py,test_helpers.py,base.py}`, +`src/imio/googleauthenticator/browser/forms/{token.py,reset_bar_code.py,request_bar_code_reset.py}`, +`src/imio/googleauthenticator/pas_plugin.py` (grep only, call sites), installed +`plone.app.registry` egg's `configure.zcml` (external reference cited by CONTEXT.md) +**Files read:** 7 (+1 grep-only for call-site confirmation) +**Pattern extraction date:** 2026-07-29 diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW-FIX.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW-FIX.md new file mode 100644 index 0000000..20d80e3 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW-FIX.md @@ -0,0 +1,157 @@ +--- +phase: 02-registry-seeding-and-import-step-ordering +fixed_at: 2026-07-29T13:53:31Z +review_path: .planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW.md +iteration: 1 +findings_in_scope: 5 +fixed: 5 +skipped: 0 +status: all_fixed +--- + +# Phase 02: Code Review Fix Report + +**Fixed at:** 2026-07-29T13:53:31Z +**Source review:** .planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope (critical + warning, per `fix_scope: critical_warning`): 5 (CR-01, CR-02, WR-01, WR-02, WR-03) +- Fixed: 5 +- Skipped: 0 + +Out of scope, untouched: IN-01, IN-02 (Info). + +## Fixed Issues + +### CR-02: Lazy-minting `ska_secret_key` inside a getter writes registry state from an abort-prone request path + +**Files modified:** `src/imio/googleauthenticator/setuphandlers.py`, `src/imio/googleauthenticator/helpers.py`, `src/imio/googleauthenticator/tests/test_setuphandlers.py` +**Commit:** `f9f72bc` +**Applied fix:** This is the important one, and it revises two of this phase's own locked +decisions (D-04, D-05) — done with the user's explicit authorisation, recorded in +`02-01-SUMMARY.md`'s new "Post-review revision" section. + +- Restored install-time seeding: `setuphandlers._setup_secret_key()` now calls + `get_app_settings()` directly and sets `ska_secret_key = unicode(uuid4())` if it is + empty, called from `setupVarious`. D-04's actual intent (no nested + `runImportStepFromProfile` re-entry) is kept intact — the restored seeding relies on + the REG-02 `` declaration to guarantee the + registry records already exist by the time `setupVarious` runs, so no nested profile + re-import is needed. +- Made `helpers.get_ska_secret_key()` a pure read again: removed the + `if not ska_secret_key: ... settings.ska_secret_key = ...` mint branch. In its place, + an empty `ska_secret_key` at read time now raises `ValueError` (uncaught, fail-closed + — surfaces as a 500 via `RENAME-11`'s `_dont_swallow_my_exceptions = True`, never a + silent password-only fallthrough). This satisfies the fix instruction's constraint #3: + no swallowable-`KeyError` bypass was reintroduced, and no weak/empty key is ever + silently produced. +- Rewrote `test_setupVarious`'s assertions to match the revised behaviour (install now + seeds a non-empty key; `get_ska_secret_key()` must NOT mutate the registry — this is + the "test that actually catches this class of bug" the fix instructions asked for, + since a happy-path-only test would not have caught CR-02) and split it into five + separately-named test methods (this also closes WR-03 — see below). + +### CR-01: `get_ska_secret_key()` raises unhandled `TypeError` on a falsy/`None` user secret + +**Files modified:** `src/imio/googleauthenticator/helpers.py`, `src/imio/googleauthenticator/tests/test_helpers.py` +**Commit:** `8d703e1` +**Applied fix:** `user_secret = user.getProperty('two_factor_authentication_secret')` +changed to `... or ''`, mirroring the existing guard in the sibling `get_secret()` +function, applied right where the review's fix suggestion placed it. Netstring framing +(BUG-04) is untouched — only the input is coerced before `len()` is taken, so the +separation property (and its existing collision test in `test_get_ska_secret_key`) is +unaffected. Added a regression test, +`TestSkaSecretKey.test_get_ska_secret_key_handles_missing_secret_property`, using a +`FakeUser` whose `getProperty` returns `None` regardless of the (absent) default +argument — this both closes CR-01 and satisfies WR-02's "add the falsy/None component +test" fix. Note: CR-01's line sits inside the same function CR-02 revised; both fixes +touch `get_ska_secret_key()`, but the two changes are on non-adjacent lines and were +verified/committed as two separate hunks (CR-02 first, CR-01 second, applied on top). + +### WR-01: `get_ska_secret_key()` is a mutating getter with an undocumented side effect + +**Files modified:** `src/imio/googleauthenticator/helpers.py` (via commit `f9f72bc`) +**Commit:** `f9f72bc` +**Applied fix:** Dissolved by the CR-02 fix, as the review itself anticipated +("WR-01 largely dissolves once CR-02 makes the getter read-only again"). Verified: after +the CR-02 revision, `get_ska_secret_key()` no longer writes to `plone.registry` under +any code path — it only reads `settings.ska_secret_key` and raises if empty. The +docstring was updated (as part of the CR-02 commit) to state plainly that this function +does NOT mint or persist the key and why, closing the "undocumented side effect" gap +without needing a separate `ensure_ska_secret_key()` split (no side effect remains to +document as a mutation — only the fail-closed raise, which the docstring now covers). +No side effects survive to document further. + +### WR-02: No test exercises `get_ska_secret_key()` with a falsy/`None` secret component + +**Files modified:** `src/imio/googleauthenticator/tests/test_helpers.py` +**Commit:** `8d703e1` (same commit as CR-01 — the fix instructions named this test as +part of CR-01's own regression coverage requirement) +**Applied fix:** Added `test_get_ska_secret_key_handles_missing_secret_property` +(see CR-01 above). Asserts the derivation returns a well-formed `unicode` result instead +of raising when the secret component is `None`. + +### WR-03: `test_setupVarious` bundles five independent assertion groups into one method + +**Files modified:** `src/imio/googleauthenticator/tests/test_setuphandlers.py` +**Commit:** `f9f72bc` (bundled with the CR-02 rewrite of this same file, since CR-02 +already required rewriting every assertion group's expected values) +**Applied fix:** Split into five methods, each named for its requirement and each with +its own docstring: `test_import_step_ordering` (REG-03), +`test_registry_records_exist_after_install` (REG-01/REG-02), `test_install_seeds_ska_secret_key` +(REG-04, revised), `test_get_ska_secret_key_does_not_mutate_registry` (CR-02 guard), +`test_reapply_profile_does_not_reset_ska_secret_key` (REG-05). A failure in one now +names exactly which requirement broke, per the review's fix suggestion. + +## Requirement / decision impact (per task instructions) + +- **D-04 and D-05 revised.** Recorded in `02-01-SUMMARY.md`'s new "Post-review revision" + section: D-04's seeding deletion and D-05's getter-mint are both reversed; D-04's + actual intent (no nested `runImportStepFromProfile`) is explicitly kept. +- **REG-01, REG-02, REG-03, BUG-04 — still hold**, unaffected by this revision (verified + by the still-passing `test_import_step_ordering`, `test_registry_records_exist_after_install`, + and the unchanged `test_get_ska_secret_key` collision test in `test_helpers.py`). +- **REG-05 — still holds**, re-verified against the revised code + (`test_reapply_profile_does_not_reset_ska_secret_key`). +- **REG-04 — revised, not broken.** The literal wording "`ska_secret_key` is minted by a + lazy accessor on first use" no longer describes the shipped code; the underlying goal + (no nested profile re-entry, `ska_secret_key` reliably non-empty) is still met, now via + install-time seeding instead. `REQUIREMENTS.md`'s REG-04 line was updated to describe + the actual mechanism rather than left silently mismatched against a `[x]` checkbox. + +## Verification + +`bin/test -t '!robot'` (full suite, run in the main repo after fast-forwarding the fix +commits from the isolated worktree): + +``` +Ran 29 tests with 0 failures and 0 errors in 4.309 seconds. +``` + +Targeted re-run of every new/renamed test named above (`test_import_step_ordering`, +`test_registry_records_exist_after_install`, `test_install_seeds_ska_secret_key`, +`test_get_ska_secret_key_does_not_mutate_registry`, `test_reapply_profile_does_not_reset_ska_secret_key`, +`test_get_ska_secret_key`, `test_get_ska_secret_key_handles_missing_secret_property`): + +``` +Ran 7 tests with 0 failures and 0 errors in 1.499 seconds. +``` + +`grep -rn runImportStepFromProfile src/*.py src/**/*.py` — the only match is inside a +docstring in `setuphandlers.py` explaining that the nested re-entry was NOT +reintroduced; no executable call site exists. `grep -c 'check=False' +src/imio/googleauthenticator/helpers.py` returns 0 (no swallowable-bypass reintroduced, +per the fix instructions' constraint #3). + +## Unresolved items + +None. All 5 in-scope findings (CR-01, CR-02, WR-01, WR-02, WR-03) are fixed and verified +by the full suite. IN-01 and IN-02 (Info) were left untouched per `fix_scope: +critical_warning`. + +--- + +_Fixed: 2026-07-29T13:53:31Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_ diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW.md new file mode 100644 index 0000000..e3adb36 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW.md @@ -0,0 +1,226 @@ +--- +phase: 02-registry-seeding-and-import-step-ordering +reviewed: 2026-07-29T00:00:00Z +depth: standard +files_reviewed: 5 +files_reviewed_list: + - src/imio/googleauthenticator/configure.zcml + - src/imio/googleauthenticator/helpers.py + - src/imio/googleauthenticator/setuphandlers.py + - src/imio/googleauthenticator/tests/test_helpers.py + - src/imio/googleauthenticator/tests/test_setuphandlers.py +findings: + critical: 2 + warning: 3 + info: 2 + total: 7 +status: issues_found +--- + +# Phase 02: Code Review Report + +**Reviewed:** 2026-07-29T00:00:00Z +**Depth:** standard +**Files Reviewed:** 5 +**Status:** issues_found + +## Summary + +Reviewed the import-step-ordering fix (`configure.zcml`), the removal of install-time +`ska_secret_key` seeding (`setuphandlers.py`), and the lazy-mint + netstring-style key +derivation in `helpers.get_ska_secret_key()`, plus the two new/expanded test files. The +`` fix and the netstring-style separation of the three +key components are both sound: the length-prefixed join is a textbook unambiguous +encoding and does close the collision the old bare concatenation had (confirmed correct by +tracing the decode invariant, not just trusting the test). + +However, moving the `ska_secret_key`-minting write out of the install step and into a bare +getter introduces two real regressions that the test suite does not catch, because every +new test drives `get_ska_secret_key()` directly with a hand-set, always-non-empty, +always-string `two_factor_authentication_secret` — never through the one call path +(`pas_plugin.authenticateCredentials` → `sign_user_data`) that the phase's own removed code +used to protect, and never with the falsy/`None` secret value the sibling `get_secret()` +function is defensively written to expect. + +## Critical Issues + +### CR-01: `get_ska_secret_key()` raises an unhandled `TypeError` if the user's secret property is falsy/`None`, where the old code silently tolerated it + +**File:** `src/imio/googleauthenticator/helpers.py:255-265` +**Issue:** +The old derivation was `"{0}{1}{2}".format(user_secret, browser_hash, ska_secret_key)` — +`str.format()` coerces `None` to the literal string `"None"`, so a missing/undeclared +`two_factor_authentication_secret` property never crashed this function (it just silently +produced a wrong-but-well-formed key). The new derivation: + +```python +return u''.join( + u'{0}:{1}'.format(len(part), part) + for part in (user_secret, browser_hash, ska_secret_key) +) +``` + +calls `len(part)` directly. If `user_secret` (`user.getProperty('two_factor_authentication_secret')`, +line 255, called with **no default argument**) is `None`, this raises +`TypeError: object of type 'NoneType' has no len()`, unhandled, inside a getter with no +`try`/`except`. + +This is not hypothetical: `.claude/CLAUDE.md` explicitly documents that "undeclared +memberdata properties are silently popped by `MutablePropertySheet.setProperties` with no +error" as a live hazard in this exact codebase — `getProperty(id)` with no default returns +`None` when a property isn't declared for a user's (possibly stale/cached) property sheet. +The sibling function `get_secret()` (lines 126-142, unchanged) already anticipates exactly +this by guarding `isinstance(secret, basestring) and secret` before use — `get_ska_secret_key()` +has no equivalent guard. + +Worse, the two callers are not equally protected: `sign_user_data()` (line 299) calls +`get_or_create_secret(user)` immediately before calling `get_ska_secret_key()`, which +guarantees the property is a set string. But `validate_user_data()` in the token form +(`browser/forms/token.py:87-88`) calls `get_ska_secret_key()` (via `validate_user_data`) +**without** that guarantee — so a user whose secret property is missing/dropped between the +initial redirect and following the signed link gets an unhandled 500 instead of the old +(silently-wrong-but-non-crashing) behavior. + +Since `pas_plugin.GoogleAuthenticatorPlugin._dont_swallow_my_exceptions = True` +(`pas_plugin.py:71`), an exception raised inside this plugin's own code is not swallowed +by PAS's `_SWALLOWABLE_PLUGIN_EXCEPTIONS` handling either — it propagates. + +**Fix:** +```python +user_secret = user.getProperty('two_factor_authentication_secret') or '' +``` +placed right after line 255, mirroring the defensive pattern already used in `get_secret()`. +Add a regression test calling `get_ska_secret_key()` with a user whose +`two_factor_authentication_secret` property is unset/`None` (e.g. a freshly created member +that never went through `get_or_create_secret`). + +### CR-02: Lazy-minting `ska_secret_key` inside a getter writes registry state from `authenticateCredentials`, not "the token form view" — violating the project's own documented `transaction.abort()` invariant + +**File:** `src/imio/googleauthenticator/helpers.py:250-253` (write), called from +`src/imio/googleauthenticator/pas_plugin.py:160` (via `sign_user_data`, not the token form) + +**Issue:** +`.claude/CLAUDE.md` states the architecture rule for exactly this class of hazard: + +> "The hazard to design against is not `ConflictError`... It is `transaction.abort()`: any +> request ending in an exception discards its writes, and `Unauthorized` is re-raised, so a +> counter written in the PAS plugin is a lockout that silently never locks. Hence: all state +> writes in the token form view" + +Before this phase, `ska_secret_key` was guaranteed non-empty by `_setup_secret_key()`, +which ran once, at install time, inside the GenericSetup import transaction (an +admin-triggered request that reliably commits). This phase deleted that seeding entirely +(confirmed via `git diff`: `setuphandlers.py` lost both the `uuid4`/`get_app_settings` +imports and the whole `_setup_secret_key` function) and replaced it with a lazy mint inside +`get_ska_secret_key()`: + +```python +ska_secret_key = settings.ska_secret_key +if not ska_secret_key: + ska_secret_key = unicode(uuid4()) + settings.ska_secret_key = ska_secret_key # <-- persistent write +``` + +This getter is called from `sign_user_data()`, which runs directly inside +`GoogleAuthenticatorPlugin.authenticateCredentials()` (`pas_plugin.py:160`) — i.e. exactly +the "written in the PAS plugin" location the CLAUDE.md passage calls out, not the token +form view. + +Concrete failure sequence: on the very first 2FA login after a fresh install (the case this +phase's own tests exercise for REG-04), any request that reaches this plugin's +`authenticateCredentials` while requesting a resource that needs more than Anonymous +permission (e.g. a 2FA-enabled user directly opening any member-only page — not an edge +case, this is exactly the situation that triggers 2FA in the first place) ends, after our +plugin empties `credentials` and returns `None`, in `Unauthorized` being raised for that +resource. Zope aborts the transaction on an unhandled exception, discarding the +just-written `ska_secret_key`. But `sign_user_data()` already computed and queued the +redirect (`response.redirect(signed_url, lock=1)`, `pas_plugin.py:169`) using the +in-memory (never-persisted) key value. When the user follows that link, the token form's +`validate_user_data()` calls `get_ska_secret_key()` again, finds the registry key **still +empty** (the mint was rolled back), and mints a **different** random key — so the +signature computed against the URL's key can never validate. The 2FA-enabled user is stuck +in a redirect loop / permanently invalid-signature error until some other request happens to +commit a mint, with no way for the user to recover on their own. + +This is a regression relative to the pre-phase behavior (seed always committed at install, +independent of any login request's fate), introduced specifically by moving the write out +of a reliably-committing context into a bare getter reachable from the PAS plugin. + +**Fix:** Do not perform registry writes from `get_ska_secret_key()`. Options, in order of +how little they cost given this package's stated 1-2 year lifespan: +- Re-add install-time seeding (without reintroducing the old nested + `runImportStepFromProfile` re-entry bug this phase fixed — just call + `get_app_settings()` + set the field directly in `setupVarious`, after the + `` guarantees registry records already exist). +- Or: keep the lazy mint, but perform the persisting write only from the token form view + (mirroring how replay/lockout counters are already scoped there), passing the freshly + minted value down to `sign_user_data()` instead of letting the getter mutate registry + state as a side effect. + +## Warnings + +### WR-01: `get_ska_secret_key()` is a mutating "getter" with an undocumented side effect + +**File:** `src/imio/googleauthenticator/helpers.py:228-265` +**Issue:** Per this codebase's own naming convention (`.claude/CLAUDE.md`: "Getter +functions prefix with `get_`"), a `get_` function is expected to be a pure accessor. This +one silently writes to `plone.registry` on first call (see CR-02) and the docstring +(lines 229-241) documents none of that — it still only describes the three input sources, +not the minting behavior or the new netstring-style output format. +**Fix:** Document the side effect explicitly in the docstring, or split into an explicit +`ensure_ska_secret_key()` write step called from a known-safe context (see CR-02 fix) plus +a genuinely pure `get_ska_secret_key()` read. + +### WR-02: No test exercises `get_ska_secret_key()` with a falsy/`None` secret component + +**File:** `src/imio/googleauthenticator/tests/test_helpers.py:126-160` +**Issue:** `test_get_ska_secret_key` only ever sets `two_factor_authentication_secret` to +non-empty string literals (`'ab'`, `'a'`). The most likely real-world failure mode for the +new `len(part)`-based derivation — a falsy/`None` component — has zero coverage, which is +exactly how CR-01 shipped uncaught despite the sibling `get_secret()` function's explicit +handling of the same scenario. +**Fix:** Add a case that leaves `two_factor_authentication_secret` unset (or explicitly +`None`) and asserts `get_ska_secret_key()` returns a well-formed key instead of raising. + +### WR-03: `test_setupVarious` bundles five independent assertion groups into one method + +**File:** `src/imio/googleauthenticator/tests/test_setuphandlers.py:36-105` +**Issue:** Ordering (REG-03), records-exist, no-install-seeding (REG-04), lazy-mint +(REG-04), and re-apply-guard (REG-05) are distinct requirements sharing one `test_` +method. Since `unittest` stops at the first failed assertion, a regression in an earlier +group (e.g. import-step ordering silently reverted) hides whether the later groups (lazy +mint, re-apply guard) still pass or fail. The docstring documents this as a deliberate +convention choice (R5), but it does reduce failure-localization precision for exactly the +kind of regression this phase is most at risk of (see CR-01/CR-02). +**Fix:** Not blocking given the documented convention, but consider splitting at least the +lazy-mint and re-apply-guard groups (the two REG-04/REG-05 behaviors most likely to +silently regress together) into their own test methods. + +## Info + +### IN-01: `get_ska_secret_key()` docstring not updated for the netstring-style format change + +**File:** `src/imio/googleauthenticator/helpers.py:228-241` +**Issue:** The docstring still describes only the three composite inputs; it doesn't +mention the `length:value` separation scheme or that a missing key is now minted on first +read. A future maintainer changing the format without reading the implementation could +reintroduce a collision. +**Fix:** Add a line noting the length-prefixed (netstring-style) separation and why it's +required (prevents the BUG-04 collision the tests pin). + +### IN-02: Implicit Python 2 `str`/`unicode` coercion in the netstring join is unguarded + +**File:** `src/imio/googleauthenticator/helpers.py:262-265` +**Issue:** `u'{0}:{1}'.format(len(part), part)` implicitly decodes a `str` `part` using the +ASCII codec in Python 2. Today this is safe only because every current producer of these +values is ASCII-only (`rebus.b32encode`, `sha1().hexdigest()`, `unicode(uuid4())`). If any +producer ever changes to allow non-ASCII bytes in a `str` (not `unicode`) value, this raises +`UnicodeDecodeError` inside the same unguarded getter as CR-01. +**Fix:** Low priority given current constraints; worth revisiting alongside the CR-01 fix +since both need the same kind of input-normalization guard. + +--- + +_Reviewed: 2026-07-29T00:00:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_ diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-SECURITY.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-SECURITY.md new file mode 100644 index 0000000..3eddbaf --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-SECURITY.md @@ -0,0 +1,102 @@ +--- +phase: 02 +slug: registry-seeding-and-import-step-ordering +status: verified +# threats_open = count of OPEN threats at or above workflow.security_block_on severity (the blocking gate) +threats_open: 0 +asvs_level: 1 +block_on: high +register_authored_at_plan_time: true +created: 2026-07-29 +--- + +# Phase 02 — Security + +> Per-phase security contract: threat register, accepted risks, and audit trail. + +Register origin: both `02-01-PLAN.md` and `02-02-PLAN.md` carry a `` block +authored at plan time, so this run **verified existing mitigations** rather than building a +retroactive STRIDE register. ASVS level 1, blocking threshold `high`; per the secure-phase +short-circuit rule (`threats_open: 0` + `register_authored_at_plan_time: true` + +`asvs_level == 1`), L1 grep-depth verification is sufficient and no deeper auditor pass was +required. + +--- + +## Trust Boundaries + +Merged from both plans' `` blocks. + +| Boundary | Description | Data Crossing | +|----------|-------------|---------------| +| GenericSetup profile import → `plone.registry` | An import step whose ordering relative to `plone.app.registry` is undeclared reads records that do not exist yet. | Registry records (`ska_secret_key`, `globally_enabled`, `ip_addresses_whitelist`) | +| unauthenticated HTTP → `@@google-authenticator-token` / `@@reset-bar-code` → `validate_user_data` → `get_ska_secret_key` | An unauthenticated request reaches the key derivation. The lazy-mint branch this boundary was written for **no longer exists** (CR-02); the function is now a pure read. | Signed-URL signature, site-wide secret | +| PAS `authenticateCredentials` → `sign_user_data` → `get_ska_secret_key` | A request path that ends in `transaction.abort()` on `Unauthorized` (`pas_plugin.py:160`). No ZODB write remains on this path. | Site-wide secret, per-user seed | +| signed URL query string → `ska.validate_signed_request_data` | Attacker-supplied `auth_user` + signature validated against a key derived from three components; a component-boundary collision would let a signature minted in one context validate in another. | Signature, `auth_user` | +| memberdata `two_factor_authentication_secret` → the derivation | A per-user value of attacker-influenced *length* (via enrolment) is joined with a site-wide secret. | Per-user TOTP seed | +| `HTTP_User-Agent` → `get_browser_hash` → the derivation | An absent or unhashable `User-Agent` on an unauthenticated login path reaches a `len()` call. | Device-binding hash | + +--- + +## Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation | Status | +|-----------|----------|-----------|----------|-------------|------------|--------| +| T-02-01 | Elevation of Privilege | `helpers.get_app_settings()` — `forInterface` `KeyError` on a missing record | high | mitigate | `helpers.py` `get_app_settings()` is a bare `registry.forInterface(IGoogleAuthenticatorSettings)` — no `check=False`, no `omit=`, no wrapping try/except, so the `KeyError` propagates. Verified by grep: `check=False\|omit=` over `helpers.py` returns no match. With Phase 1's `_dont_swallow_my_exceptions = True` this renders a 500 rather than falling through to password-only login. | closed | +| T-02-02 | Repudiation | `GenericSetup.tool.getSortedImportSteps` — undeclared step order over a Python 2 `set` | medium | mitigate | `configure.zcml:50` declares ``. Two assertions back it: `test_import_step_declares_registry_dependency` (`test_setuphandlers.py:44`, the real control — asserts the pre-sort `getImportStepMetadata(...)['dependencies']`) and `test_import_step_ordering` (`:71`, the outcome check, honestly self-limiting per its docstring). | closed | +| T-02-03 | Tampering | `plone.app.registry` `` re-import replacing `ska_secret_key` with `u''` | medium | mitigate | `test_reapply_profile_does_not_reset_ska_secret_key` (`test_setuphandlers.py:128`) sets a distinctive literal, re-applies the profile, asserts equality against that same literal. | closed | +| T-02-04 | Denial of Service | lazy mint reachable from an unauthenticated request | low | accept | **Moot — attack surface deleted.** CR-02 removed the mint branch entirely; `get_ska_secret_key` is a pure read that raises `ValueError` on an empty key (fail-closed). Seeding moved to `setuphandlers._setup_secret_key()` at install time. No unauthenticated write path remains. | closed | +| T-02-05 | Tampering | mint write discarded by `transaction.abort()` on the PAS plugin path | low | accept | **Moot — same removal.** There is no longer any ZODB write in `get_ska_secret_key`, so nothing on the `authenticateCredentials` path can be lost to `transaction.abort()`. Confirmed: no assignment to `settings.ska_secret_key` anywhere in `helpers.py`. | closed | +| T-02-06 | Information Disclosure | the `ska_secret_key` reaching a log line or exception message | low | accept | No mint branch, no logging of the key, and the fail-closed `ValueError` text names only the remedy (`'ska_secret_key is not set; (re)install imio.googleauthenticator'`), not the value. Verified by grep for `logger.*ska_secret_key` / `print.*ska_secret_key` across `helpers.py`, `browser/` and `browser/forms/` — no match. See Accepted Risks R-02-01 for the pre-existing control-panel exposure. | closed | +| T-02-07 | Spoofing | `helpers.get_ska_secret_key` — unframed concatenation of `(user_secret, browser_hash, ska_secret_key)` | high | mitigate | Length-prefixed netstring join: `u''.join(u'{0}:{1}'.format(len(part), part) for part in (user_secret, browser_hash, ska_secret_key))`. A component-boundary shift changes the derived key. `test_get_ska_secret_key` asserts a fixture that provably collides under bare concatenation derives to the exact string `u'2:ab0:2:cd'`, plus `assertNotEqual` against the colliding fixture — so the mitigation cannot regress into a cosmetic reformat. | closed | +| T-02-08 | Denial of Service | `get_browser_hash` returning `None` under `len()` on the login path | medium | mitigate | `helpers.py` `except` branch returns `''`; `test_get_browser_hash` asserts both `assertEqual('', result)` and `assertIsNotNone(result)` (`test_helpers.py:195`). The guard is what stops a future edit reintroducing a fall-off-the-end `None`, which on this path would be an unauthenticated `TypeError`. | closed | +| T-02-09 | Denial of Service | a non-ASCII `str` component reaching a `u'...'` format → `UnicodeDecodeError` on the login path | low | accept | Accepted — see R-02-02. Every component is ASCII by construction (base32 seed, hex sha1 or `u''`, `unicode(uuid4())`). | closed | +| T-02-10 | Tampering | reopening the derivation after deployment | medium | accept | Accepted — see R-02-03. Recorded as the plan's one prohibition and as Task 1's `costly` reversibility rating. | closed | +| T-02-SC | Tampering | npm/pip/cargo installs (supply chain) | low | accept | No package-manager install added by either plan. Verified: `git diff master...HEAD --stat -- setup.py test-4.3.cfg requirements-4.3.txt` is empty, so there is no `[ASSUMED]`/`[SUS]` package to gate and no legitimacy checkpoint is required. Declared identically in both plans (duplicate ID, recorded once). | closed | + +*Status: open · closed · open — below high threshold (non-blocking)* +*Severity: critical > high > medium > low — only open threats at or above `high` count toward `threats_open`* +*Disposition: mitigate (implementation required) · accept (documented risk) · transfer (third-party)* + +**Blocking tally:** 2 `high` threats (T-02-01, T-02-07), both `mitigate`, both verified closed → +`threats_open: 0`. + +--- + +## Accepted Risks Log + +| Risk ID | Threat Ref | Rationale | Accepted By | Date | +|---------|------------|-----------|-------------|------| +| R-02-01 | T-02-06 | `browser/controlpanel.py` renders `ska_secret_key` into a form field. **Pre-existing**, untouched by this phase, and already a recorded Phase 3 Deferred Idea (canon secret-hygiene, breadcrumbed to `/gsd-secure-phase`). Not minted as a prohibition here. | Chris (plan D-06) | 2026-07-29 | +| R-02-02 | T-02-09 | Every derivation component is ASCII by construction: a base32 seed, a hex sha1 or `u''`, and a `unicode(uuid4())`. Phase 3's planned `v1$` is base64, also ASCII. Coercion deliberately not added — untested defensive code on a login path guarding a state no code path can produce. **Re-check when Phase 3 changes `user_secret`.** | Chris (02-02-PLAN) | 2026-07-29 | +| R-02-03 | T-02-10 | Reopening the `ska` derivation after deployment invalidates every signed URL in flight and every outstanding bar-code-reset link. Accepted now because nothing is deployed and no user is enrolled; a later change requires an explicit migration. Recorded as the plan's one prohibition. | Chris (02-02-PLAN) | 2026-07-29 | +| R-02-04 | T-02-04, T-02-05 | Both were accepted at plan time against the lazy-mint design. CR-02 subsequently **deleted** that branch, so the accepted risk is moot rather than live — recorded here only so the two IDs do not resurface as unexplained closures in a future audit. | Chris (CR-02) | 2026-07-29 | + +--- + +## Security Audit Trail + +| Audit Date | Threats Total | Closed | Open | Run By | +|------------|---------------|--------|------|--------| +| 2026-07-29 | 11 | 11 | 0 | Claude (`/gsd-secure-phase 02`, L1 short-circuit — no auditor spawn) | + +**Evidence run in this audit (not taken on trust from VERIFICATION.md):** + +- `bin/test -t '!robot'` after a `.pyc` purge → `Ran 30 tests with 0 failures and 0 errors` +- `grep -n "check=False\|omit=" src/imio/googleauthenticator/helpers.py` → no match (T-02-01) +- `get_ska_secret_key` body read directly; netstring join present, no write branch, fail-closed `ValueError` (T-02-07, T-02-04/05) +- `grep -n "logger.*ska_secret_key\|print.*ska_secret_key"` over `helpers.py`, `browser/`, `browser/forms/` → no match (T-02-06) +- `configure.zcml:50` `` present; both backing tests present at `test_setuphandlers.py:44` and `:71` (T-02-02) +- `git diff master...HEAD --stat` over `setup.py`, `test-4.3.cfg`, `requirements-4.3.txt` → empty (T-02-SC) +- `git diff master...HEAD --stat` over the four `ska`-derivation consumers (`pas_plugin.py`, `browser/forms/token.py`, `reset_bar_code.py`, `request_bar_code_reset.py`) → empty + +--- + +## Sign-Off + +- [x] All threats have a disposition (mitigate / accept / transfer) +- [x] Accepted risks documented in Accepted Risks Log +- [x] `threats_open: 0` confirmed +- [x] `status: verified` set in frontmatter + +**Approval:** verified 2026-07-29 diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-UAT.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-UAT.md new file mode 100644 index 0000000..fee653c --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-UAT.md @@ -0,0 +1,79 @@ +--- +status: complete +phase: 02-registry-seeding-and-import-step-ordering +source: [02-VERIFICATION.md] +started: 2026-07-29T16:45:00Z +updated: 2026-07-29T16:50:00Z +--- + +## Current Test + +[testing complete] + +## Tests + +### 1. REG-01 / SC-1 — creating a new Plone site with the add-on selected completes with no `ska_secret_key ... no record` error in `var/log/instance.log` + +expected: `bin/instance fg`, create a new Plone site with `imio.googleauthenticator` selected, then `grep -e "no record" -e "Cannot find registry" var/log/instance.log` finds no matches. +result: pass +reported: "It appeared many times but I found no proof it comes from this package" — 26 `Cannot find registry` lines, no `no record` lines. +evidence: | + Site creation ran twice (11:21 and 16:27 on 2026-07-29); `var/log/instance.log` + from the real run was inspected directly. + + DECISIVE CRITERION — CLEAN. `grep -e "no record" -e "defines a field"` over + `var/log/instance.log` and `var/log/instance-Z2.log`: zero matches. No + `IGoogleAuthenticatorSettings defines a field ska_secret_key, for which there is + no record`. No ERROR, WARNING or Traceback anywhere in the 16:27 run. + + SECOND GREP PATTERN IS A FALSE POSITIVE. The 26 `Cannot find registry` hits are + emitted by `plone.app.registry-1.2.5/plone/app/registry/exportimport/handler.py:67` + — a `logger.info` in the `queryUtility(IRegistry) is None` early-return branch. It + fires for any profile whose `registry.xml` step runs before the `IRegistry` local + utility exists, which during Plone site creation is every registry step queued + ahead of `plone.app.registry`'s own profile. On the success path that handler logs + nothing, so absence of a line for our profile is the success signal. + + NOT ATTRIBUTABLE TO THIS PACKAGE — timeline proof: + - 11:21 run: last `Cannot find registry` 11:21:34 (line 2708); + `Applying main profile profile-imio.googleauthenticator:default` 11:21:35 (line 2892). + - 16:27 run: last `Cannot find registry` 16:27:42 (line 3206); + `Applying main profile profile-imio.googleauthenticator:default` 16:27:43 (line 3390). + All 26 occurrences precede our profile's import; zero occur inside its block. + Immediately before our profile: `No upgrades available for profile + profile-plone.app.registry:default` — the registry dependency resolving ahead of + us, i.e. the REG-02 `` ordering doing its job. + +test_definition_defect: | + The `-e "Cannot find registry"` half of this grep is over-broad — it matches + stock Plone 4.3 site-creation noise in every build and cannot distinguish our + package. A future re-run should grep only for the attributable string: + grep -e "no record" -e "defines a field ska_secret_key" var/log/instance.log + Corrected in `reproduce` below. Not a code defect; no gap raised. + +why_human: Deliberately not automated per D-01/D-02 — this is a declared +`verification: backstop` must_have. No second-site fixture exists (by design), and no +`var/log/instance.log` from a real site-creation run is available to the verifier. The +mechanised substitute controls all pass: the RECORDS assertion +(`test_registry_records_exist_after_install`), the DECLARATION assertion +(`test_import_step_declares_registry_dependency` — proven to fail when the `` +line is deleted), and the ORDERING assertion (`test_import_step_ordering`). + +reproduce: | + bin/instance fg + # In another shell: browse to http://localhost:8080, add a Plone site, + # tick "Google Authenticator plugin (imio.googleauthenticator)" in the + # add-ons list on the site-creation form. + # Then, back in the repo — attributable strings only, NOT "Cannot find registry": + grep -e "no record" -e "defines a field ska_secret_key" var/log/instance.log + +## Summary + +total: 1 +passed: 1 +issues: 0 +pending: 0 +skipped: 0 +blocked: 0 + +## Gaps diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/02-VERIFICATION.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-VERIFICATION.md new file mode 100644 index 0000000..d3b3271 --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/02-VERIFICATION.md @@ -0,0 +1,221 @@ +--- +phase: 02-registry-seeding-and-import-step-ordering +verified: 2026-07-29T16:30:00Z +status: passed +score: 9/9 must-haves verified +behavior_unverified: 0 +overrides_applied: 0 +re_verification: + previous_status: gaps_found + previous_score: 8/9 + gaps_closed: + + - "REG-03 / ROADMAP Success Criterion 2: the getSortedImportSteps() ordering assertion is the mechanised control for the declaration and must fail if the line is deleted" + gaps_remaining: [] + regressions: [] +human_verification: + + - test: "REG-01 / ROADMAP Success Criterion 1 -- real site-creation smoke check" + expected: "bin/instance fg, create a new Plone site with imio.googleauthenticator selected in the add-ons list, then grep -e \"no record\" -e \"Cannot find registry\" var/log/instance.log finds no matches" + why_human: "Deliberately not automated per D-01/D-02 (verification: backstop must_have) -- no second-site fixture exists and no var/log/instance.log from a real site-creation run is available to this verifier. The RECORDS + ORDERING + DECLARATION assertions are the mechanised substitute control and all now pass." +--- + +# Phase 2: Registry Seeding and Import-Step Ordering Verification Report + +**Phase Goal:** Creating a new Plone site with the add-on selected completes without the +`ska_secret_key ... no record` error, and the import-step ordering that makes it complete is +asserted in the suite rather than left to CPython 2.7 string-hash order. +**Verified:** 2026-07-29T16:30:00Z +**Status:** human_needed +**Re-verification:** Yes — after gap closure (commits `30c755b`, `f9ed419`) + +## Goal Achievement + +### What changed since the prior verification + +The prior report (`gaps_found`, 8/9) found one blocking gap: `test_import_step_ordering` was +tautological — it stayed green with `` deleted from +`configure.zcml`, purely by CPython 2.7 string-hash coincidence among the now dependency-free +import steps. That is the exact failure mode the phase's own goal statement (and D-01/D-03) name +as unacceptable. + +The fix (commit `30c755b`) adds `test_import_step_declares_registry_dependency`, which asserts +`portal_setup.getImportStepMetadata('imio.googleauthenticator')['dependencies']` contains +`'plone.app.registry'` — the pre-sort declaration GenericSetup parsed from ZCML, not the post-sort +flattened tuple. `test_import_step_ordering` is kept, re-docstringed as the outcome check ("on its +own it proves nothing... the two together assert both the declaration and its effect"), rather than +deleted or left silently mischaracterized. + +Commit `f9ed419` updates the stale ROADMAP SC-2/SC-3 wording and the stale `CHANGES.rst` bullet +flagged in the prior report. + +### Independent re-verification of the fix (not taken on trust) + +Ran the exact experiment myself, from a clean tree, rather than accepting the SUMMARY/orchestrator's +account: + +1. `find . -name '*.pyc' -delete` (avoids the stale-`.pyc` false-positive noted in the prior report). +2. `bin/test -t '!robot'` on the unmodified tree: **`Ran 30 tests with 0 failures and 0 errors`** + (was 29 at the prior verification — the one new test). + +3. Deleted `` from `configure.zcml` (converted the + `genericsetup:importStep` block back to self-closing), confirmed the edit is still well-formed + XML (`xml.dom.minidom.parse` exits 0). + +4. `bin/test -t test_import_step_declares_registry_dependency` on the mutated tree: + **1 failure** — `AssertionError: 'plone.app.registry' not found in ()`. This is the new control + catching the exact regression it exists to catch. + +5. `bin/test -t test_import_step_ordering` on the same mutated tree: **0 failures** — confirms the + kept outcome-check test is *still* tautological on its own in this fixture (as its docstring now + says explicitly), which is exactly why the declaration test above is the one that must exist. + +6. Restored `configure.zcml` from a pre-edit backup, confirmed `git status` is clean and + `git diff --stat` is empty, re-ran `bin/test -t '!robot'`: back to 30/0/0. + +This closes the gap: a future deletion of the `` line is now caught by +`test_import_step_declares_registry_dependency`, independent of whether the topological sort's +emergent order happens to still agree with it. + +### Observable Truths + +| # | Truth | Status | Evidence | +|---|-------|--------|----------| +| 1 | SC1 / REG-01 (`verification: backstop`, D-01/D-02): creating a new Plone site with the add-on selected completes with no `ska_secret_key ... no record` in `var/log/instance.log` | ⚠️ Abstain (`insufficient_spec`) | No automated second-site fixture exists (deliberately, per D-01) and no `var/log/instance.log` from a real site-creation run is available to this verifier. Routed to Human Verification below, per D-02's own design. Unchanged from prior verification. | +| 2 | SC2 / REG-03: the ordering assertion is a genuine mechanised control for the `` declaration, not tautological hash-order luck | ✓ **VERIFIED (gap closed)** | `test_import_step_declares_registry_dependency` asserts `getImportStepMetadata(...)['dependencies']` contains `'plone.app.registry'`. Independently reproduced: deleting the `` line makes this test fail (`'plone.app.registry' not found in ()`), while `test_import_step_ordering` alone still passes on the same mutated tree — confirming the declaration test, not the ordering test, is the real control. Both tests are retained, and `test_import_step_ordering`'s docstring now says in words that it proves nothing on its own. | +| 3 | REG-02: import step declares `` | ✓ VERIFIED | `src/imio/googleauthenticator/configure.zcml:50` — present, well-formed (`xml.dom.minidom.parse` exits 0). | +| 4 | SC3 / REG-04: nested `runImportStepFromProfile` re-entry is gone from `src/`; `ska_secret_key` is reliably non-empty after install | ✓ VERIFIED | `grep -rn runImportStepFromProfile src/` (after a fresh `.pyc` purge) returns nothing, exit 1. `setuphandlers.setupVarious` calls `_setup_secret_key()`, a direct `get_app_settings()` read + conditional assignment — no nested profile import. `test_install_seeds_ska_secret_key` passes. ROADMAP SC3 wording now matches this implementation exactly (commit `f9ed419`): "seeded once at install time by `setuphandlers._setup_secret_key()`, with `get_ska_secret_key()` a pure read" — the stale "lazy accessor" phrasing flagged in the prior report is gone. | +| 5 | SC4 / REG-05: re-applying the default profile leaves a known `ska_secret_key` unchanged | ✓ VERIFIED | `test_reapply_profile_does_not_reset_ska_secret_key` sets a distinctive literal, calls `applyProfile`, asserts `assertEqual` against that same literal. Passes in the full suite run. | +| 6 | SC5 / BUG-04: derived `ska` key separates its components — two tuples sharing a bare concatenation derive to different keys | ✓ VERIFIED | `test_get_ska_secret_key`: fixture collision asserted explicitly, then exact-string equality (`u'2:ab0:2:cd'`) and `assertNotEqual` against the second fixture's derivation. Passes. | +| 7 | `get_browser_hash` returns `u''`, never `None`, so `len()` on a login path cannot raise `TypeError` | ✓ VERIFIED | `helpers.py:221-225` `except` branch returns `''`, unchanged. `test_get_browser_hash` asserts `assertEqual('', result)` and `assertIsNotNone(result)`. Passes. | +| 8 | Whole suite green; all four `ska` derivation consumers unmodified | ✓ VERIFIED | `bin/test -t '!robot'` run live by this verifier: **30 tests, 0 failures, 0 errors**. `git diff --stat` over `pas_plugin.py`, `browser/forms/token.py`, `browser/forms/reset_bar_code.py`, `browser/forms/request_bar_code_reset.py` across the whole phase's commit range is empty — no changes. | +| 9 | CR-01 fix: `get_ska_secret_key()` does not raise `TypeError` on a falsy/`None` user secret | ✓ VERIFIED | `helpers.py:275`: `user.getProperty(...) or ''`. `test_get_ska_secret_key_handles_missing_secret_property` passes. | +| 10 | CR-02 fix: `get_ska_secret_key()` is a pure read and does not mutate the registry | ✓ VERIFIED | `helpers.py:250-269`: no write branch; empty key raises `ValueError` (fail-closed). `test_get_ska_secret_key_does_not_mutate_registry` passes. | + +**Score:** 9/9 machine-checkable truths verified (the REG-03 gap from the prior verification is +closed). 1 additional truth (REG-01/SC1) is a declared `verification: backstop` and is excluded +from the denominator per its own design (D-02) — routed to Human Verification instead, unchanged +from the prior report. + +### Regression Check on Previously-Passed Truths + +The gap-closure commits touched `test_setuphandlers.py`, `CHANGES.rst`, and `ROADMAP.md` — files +that back several of the 8 previously-verified truths. Re-checked each: + +- **REG-02 / configure.zcml**: unchanged by the gap-closure commits; still present and well-formed. +- **REG-04 / setuphandlers.py, helpers.py**: unchanged by the gap-closure commits (only + `test_setuphandlers.py` was touched, adding a new test method and re-docstringing an existing + one — no assertion in the five pre-existing methods was weakened or removed). + +- **REG-05, BUG-04, CR-01, CR-02**: their backing test methods (`test_reapply_profile_does_not_reset_ska_secret_key`, + `test_get_ska_secret_key`, `test_get_ska_secret_key_handles_missing_secret_property`, + `test_get_ska_secret_key_does_not_mutate_registry`) are byte-for-byte unchanged; confirmed by + reading the current file and comparing against the prior verification's evidence. + +- **Whole-suite regression**: `bin/test -t '!robot'` — 30/0/0, no new failures, one new test + (the count previously was 29). + +No regressions found. + +### Required Artifacts + +| Artifact | Expected | Status | Details | +|----------|----------|--------|---------| +| `src/imio/googleauthenticator/configure.zcml` | Declared import-step ordering | ✓ VERIFIED | `` present at line 50, well-formed XML. Unchanged since prior verification. | +| `src/imio/googleauthenticator/setuphandlers.py` | Install-time seeding, no nested re-entry | ✓ VERIFIED | 71 lines. `_setup_secret_key()` — direct `get_app_settings()` call + conditional assignment; no `runImportStepFromProfile`. Unchanged. | +| `src/imio/googleauthenticator/helpers.py` | `get_ska_secret_key` — pure read, netstring derivation | ✓ VERIFIED | No mint branch; fail-closed `raise ValueError`; `u''.join(...)` derivation present. Unchanged. | +| `src/imio/googleauthenticator/tests/test_setuphandlers.py` | Ordering/declaration/records/seed/mutation/reapply assertions | ✓ VERIFIED | 143 lines, 6 well-named methods (was 5). New `test_import_step_declares_registry_dependency` is the closed gap; `test_import_step_ordering` retained with an honest, self-limiting docstring. | +| `src/imio/googleauthenticator/tests/test_helpers.py` | `TestSkaSecretKey` — derivation, collision, browser-hash guard | ✓ VERIFIED | 200 lines, unchanged since prior verification. | + +### Key Link Verification + +| From | To | Via | Status | Details | +|------|----|----|--------|---------| +| `configure.zcml` | GenericSetup import-step topological sort | `` | ✓ WIRED (gap closed) | Declaration present, well-formed, and now proven to be the actual control: `test_import_step_declares_registry_dependency` fails when the line is removed, independent of the emergent hash-order coincidence that used to mask its absence. | +| `helpers.py get_ska_secret_key` | `plone.registry IGoogleAuthenticatorSettings.ska_secret_key` | Pure read, `ValueError` if empty | ✓ WIRED | Unchanged; no write path remains. | +| `setuphandlers.setupVarious` | `helpers.get_app_settings` | `_setup_secret_key()` seeds `ska_secret_key` at install | ✓ WIRED | Unchanged. | + +### Behavioral Spot-Checks / Probe Execution + +| Behavior | Command | Result | Status | +|----------|---------|--------|--------| +| Full suite green | `bin/test -t '!robot'` (run live by this verifier, `.pyc` purged first) | `Ran 30 tests with 0 failures and 0 errors` | ✓ PASS | +| No nested re-entry in source | `grep -rn runImportStepFromProfile src/` | no output, exit 1 | ✓ PASS | +| Declaration control genuinely catches removal | Deleted `` line, reran `bin/test -t test_import_step_declares_registry_dependency` on the mutated tree | 1 failure — `'plone.app.registry' not found in ()` | ✓ PASS — the gap is closed | +| Kept outcome-check is honestly self-limiting | Same mutated tree, reran `bin/test -t test_import_step_ordering` | 0 failures — confirms this test alone still cannot detect the removal, matching its own updated docstring | ✓ PASS (documents the limitation rather than hiding it) | +| Reverted mutation leaves a clean tree | `git status`, `git diff --stat` after restoring `configure.zcml` | clean tree, empty diff | ✓ PASS | +| No `check=False` bypass reintroduced | `grep -n check=False src/imio/googleauthenticator/helpers.py` | no match | ✓ PASS | +| Four `ska`-derivation consumers unmodified across the whole phase | `git diff --stat` over `pas_plugin.py`, `browser/forms/token.py`, `browser/forms/reset_bar_code.py`, `browser/forms/request_bar_code_reset.py` (full phase range) | empty | ✓ PASS | +| No project-declared `scripts/*/tests/probe-*.sh` | `find scripts -path '*/tests/probe-*.sh'` | none found; phase is not migration/probe-based | N/A — SKIPPED | + +All experimental file edits made during this re-verification (`configure.zcml`) were reverted from +a pre-edit backup; `git status` confirms a clean working tree and the full suite is green (30/0/0) +at time of writing this report. + +### Requirements Coverage + +| Requirement | Source Plan | Description | Status | Evidence | +|-------------|-------------|--------------|--------|----------| +| REG-01 | 02-01 | Site install completes without the `ska_secret_key ... no record` error | ⚠️ NEEDS HUMAN | `verification: backstop` per D-02; DECLARATION/ORDERING/RECORDS assertions are the mechanised proxy — all now pass, including the previously-flagged gap. | +| REG-02 | 02-01 | `` declared | ✓ SATISFIED | `configure.zcml:50` | +| REG-03 | 02-01 | Ordering asserted via a genuine mechanised control, not string-hash chance | ✓ **SATISFIED (was BLOCKED)** | `test_import_step_declares_registry_dependency` — independently reproduced to fail on `` removal | +| REG-04 | 02-01 | No nested re-entry; `ska_secret_key` reliably non-empty | ✓ SATISFIED | `setuphandlers._setup_secret_key`, `test_install_seeds_ska_secret_key`; ROADMAP/REQUIREMENTS wording now matches the shipped mechanism | +| REG-05 | 02-01 | Re-apply does not reset `ska_secret_key` | ✓ SATISFIED | `test_reapply_profile_does_not_reset_ska_secret_key` | +| BUG-04 | 02-02 | Derived key separates its components | ✓ SATISFIED | `test_get_ska_secret_key` | + +No orphaned requirements — all 6 IDs mapped to this phase in `REQUIREMENTS.md` (lines 174-178, 219) +appear in a plan's `requirements` frontmatter and are accounted for above. + +### Anti-Patterns Found + +| File | Line | Pattern | Severity | Impact | +|------|------|---------|----------|--------| +| `src/imio/googleauthenticator/helpers.py` | 336, 364 | Pre-existing `FIXME` markers | ℹ️ Info | Predate this phase (Lukas Graf, 2015); not introduced or touched by Phase 2's commits, including the gap-closure commits. Not this phase's debt. | + +The previously-flagged stale `CHANGES.rst` bullet is fixed (commit `f9ed419`) and no longer an +anti-pattern. No blocking debt markers (`TBD`/`FIXME`/`XXX` without a tracked-issue reference) were +introduced by this phase's own commits, including the two gap-closure commits reviewed here. + +### Human Verification Required + +### 1. REG-01 / ROADMAP Success Criterion 1 — real site-creation smoke check + +**Test:** `bin/instance fg`, create a new Plone site with `imio.googleauthenticator` selected in the +add-ons list, then `grep -e "no record" -e "Cannot find registry" var/log/instance.log`. +**Expected:** No matches — the site installs cleanly with no `IGoogleAuthenticatorSettings defines a +field ska_secret_key, for which there is no record` line. +**Why human:** Deliberately not automated per D-01 (cost of a second-site fixture judged not worth +it; the DECLARATION/RECORDS/ORDERING mechanised assertions are the substitute control, and all three +now pass, including the previously-flagged gap). This verifier has no `var/log/instance.log` from a +real site-creation run to inspect, so it abstains (`insufficient_spec`) rather than guessing, per +D-02's own explicit design. Unchanged from the prior verification — this item was never part of the +gap, and closing the gap does not remove the need for this one backstop check. + +### Gaps Summary + +**No gaps remain.** The single blocking gap from the prior verification — the REG-03 ordering +assertion being tautological and not an actual control for the `` declaration — is closed. +This was independently re-verified in this session (not taken on trust from the SUMMARY or the +orchestrator's account): deleting `` now makes +`test_import_step_declares_registry_dependency` fail with the exact error the gap-closure commit +claims (`'plone.app.registry' not found in ()`), while the old `test_import_step_ordering` test +predictably still passes on its own — confirming the new test, not the old one, is what actually +guards the requirement, and that the old test's docstring is now honest about its own limits. + +All 8 previously-verified truths were re-checked for regression given that the gap-closure commits +touched `test_setuphandlers.py`, `CHANGES.rst`, and `ROADMAP.md` — none regressed; the five +pre-existing test methods in `test_setuphandlers.py` are byte-identical to what was verified before. + +The stale `CHANGES.rst` bullet and stale ROADMAP SC3 wording flagged as non-blocking documentation +issues in the prior report are also both fixed (commit `f9ed419`), confirmed by direct diff +inspection. + +The one remaining item — REG-01's `var/log/instance.log` smoke check — is an intentionally +un-automated `verification: backstop` must_have (D-01/D-02), not a gap. It routes this phase to +`human_needed` rather than `passed`, exactly as it did in the prior verification; nothing about +closing the REG-03 gap changes that routing. + +--- + +_Verified: 2026-07-29T16:30:00Z_ +_Verifier: Claude (gsd-verifier)_ diff --git a/.planning/phases/02-registry-seeding-and-import-step-ordering/COVERAGE.md b/.planning/phases/02-registry-seeding-and-import-step-ordering/COVERAGE.md new file mode 100644 index 0000000..6545d0f --- /dev/null +++ b/.planning/phases/02-registry-seeding-and-import-step-ordering/COVERAGE.md @@ -0,0 +1,3 @@ +No external API integration: this phase edits GenericSetup wiring (`configure.zcml`, `setuphandlers.py`), one local helper (`helpers.get_ska_secret_key`) and tests — no external API or SDK is called. + +Detector result for the phase scope (ROADMAP §Phase 2 + 02-CONTEXT.md): `{"detected":false,"signals":[]}`. diff --git a/CHANGES.rst b/CHANGES.rst index b1d76b8..5c87e81 100755 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -14,6 +14,24 @@ Changelog - Previously-issued signed token URLs keep validating: the rename changes none of the three inputs to the ``ska`` signing key. [chris-adam] +- The ``imio.googleauthenticator`` GenericSetup import step now declares + ````, so the registry records it needs + are seeded deterministically instead of by CPython 2.7 string-hash chance. + [chris-adam] +- ``ska_secret_key`` is seeded once at install time, without the nested + ``runImportStepFromProfile`` re-entry the ```` declaration above + makes unnecessary. ``get_ska_secret_key()`` is a pure read and raises if the + key is missing: minting inside it would write registry state from the PAS + ``authenticateCredentials()`` path, which ends in ``transaction.abort()`` on + ``Unauthorized`` and would discard the key after a URL signed with it had + already been sent to the browser. + [chris-adam] +- The ``ska`` signing key derivation now length-prefixes its three + components instead of bare-concatenating them, so two different + component boundaries can no longer collide on the same key. This + invalidates any previously issued signed URL -- harmless before any site + is deployed and any user is enrolled, which is why it ships now. + [chris-adam] 0.3.0 (unreleased) ------------------ diff --git a/src/imio/googleauthenticator/configure.zcml b/src/imio/googleauthenticator/configure.zcml index b2d50cb..d602e91 100755 --- a/src/imio/googleauthenticator/configure.zcml +++ b/src/imio/googleauthenticator/configure.zcml @@ -46,7 +46,9 @@ title="imio.googleauthenticator install steps" description="" handler="imio.googleauthenticator.setuphandlers.setupVarious" - /> + > + + diff --git a/src/imio/googleauthenticator/helpers.py b/src/imio/googleauthenticator/helpers.py index 9241287..b1d5501 100755 --- a/src/imio/googleauthenticator/helpers.py +++ b/src/imio/googleauthenticator/helpers.py @@ -227,7 +227,15 @@ def get_browser_hash(request=None): def get_ska_secret_key(request=None, user=None, use_browser_hash=True): """ - Gets the `secret_key` to be used in `ska` package. + Gets the `secret_key` to be used in `ska` package. A pure read -- this + function does NOT mint or persist `ska_secret_key` (CR-02): seeding + happens once, reliably, at install time + (`setuphandlers._setup_secret_key`), because a write performed here would + be reachable from `sign_user_data()` inside + `GoogleAuthenticatorPlugin.authenticateCredentials()`, a request path + that ends in `transaction.abort()` on `Unauthorized` and would discard + the mint after a signed URL using it had already been handed to the + browser. - Value of the ``two_factor_authentication_secret`` (from users' profile). - Browser info (hash of) @@ -248,15 +256,33 @@ def get_ska_secret_key(request=None, user=None, use_browser_hash=True): settings = get_app_settings() ska_secret_key = settings.ska_secret_key - - user_secret = user.getProperty('two_factor_authentication_secret') + if not ska_secret_key: + # Fail closed (CR-02): install-time seeding should already guarantee + # a non-empty key. An empty value here means installation was + # skipped or the registry record was cleared out-of-band -- signing + # with an empty/weak key would silently degrade the 2FA guarantee, + # so raise instead of minting one. Not caught anywhere: RENAME-11's + # _dont_swallow_my_exceptions = True turns this into a 500 on the + # PAS plugin path rather than a swallowed exception falling through + # to password-only login. + raise ValueError( + 'ska_secret_key is not set; (re)install imio.googleauthenticator') + + # CR-01: getProperty() with no default returns None for an + # undeclared/stale-cached property sheet (documented hazard, see + # CLAUDE.md); len(None) would raise TypeError. Coerce to '' like the + # sibling get_secret()/get_browser_hash() already do. + user_secret = user.getProperty('two_factor_authentication_secret') or '' if use_browser_hash: browser_hash = get_browser_hash(request=request) else: browser_hash = '' - return "{0}{1}{2}".format(user_secret, browser_hash, ska_secret_key) + return u''.join( + u'{0}:{1}'.format(len(part), part) + for part in (user_secret, browser_hash, ska_secret_key) + ) def is_two_factor_authentication_globally_enabled(): diff --git a/src/imio/googleauthenticator/setuphandlers.py b/src/imio/googleauthenticator/setuphandlers.py index 8682cba..4aa8803 100755 --- a/src/imio/googleauthenticator/setuphandlers.py +++ b/src/imio/googleauthenticator/setuphandlers.py @@ -10,6 +10,26 @@ PAS_TITLE = 'Google Authenticator plugin (imio.googleauthenticator)' PAS_ID = 'google_auth' +def _setup_secret_key(): + """ + Seed ska_secret_key at install time, if it is not already set. + + Post-review revision (CR-02): this seeding was deleted by this phase's + original plan (D-04) in favour of a lazy mint inside + get_ska_secret_key(), which turned out to write registry state from a + request path (PAS authenticateCredentials -> sign_user_data) that ends + in transaction.abort() on Unauthorized, discarding the mint after a + signed URL using it was already redirected to. Restoring seeding here + fixes that. D-04's actual intent is kept intact: no nested profile + import-step re-entry -- the + declaration added by REG-02 already guarantees the registry records + exist by the time setupVarious runs, so a direct get_app_settings() + call is enough. + """ + settings = get_app_settings() + if not settings.ska_secret_key: + settings.ska_secret_key = unicode(uuid4()) + def _add_plugin(pas, pluginid=PAS_ID): """ Install and activate imio.googleauthenticator PAS plugin @@ -30,19 +50,6 @@ def _add_plugin(pas, pluginid=PAS_ID): [x[0] for x in pas.plugins.listPlugins(interface)[:-1]], ) -def _setup_secret_key(portal): - """ - Generate secret key - """ - portal.portal_setup.runImportStepFromProfile( - 'profile-imio.googleauthenticator:default', - 'plone.app.registry' - ) - - settings = get_app_settings() - if not settings.ska_secret_key: - settings.ska_secret_key = unicode(uuid4()) - def setupVarious(context): """ @param context: Products.GenericSetup.context.DirectoryImportContext instance @@ -56,7 +63,7 @@ def setupVarious(context): portal = context.getSite() - _setup_secret_key(portal) + _setup_secret_key() pas = portal.acl_users _add_plugin(pas) diff --git a/src/imio/googleauthenticator/tests/test_helpers.py b/src/imio/googleauthenticator/tests/test_helpers.py index e0f610c..73aed62 100755 --- a/src/imio/googleauthenticator/tests/test_helpers.py +++ b/src/imio/googleauthenticator/tests/test_helpers.py @@ -1,12 +1,19 @@ import unittest2 as unittest +from plone import api +from plone.app.testing import login +from plone.app.testing import TEST_USER_NAME + from imio.googleauthenticator.testing import \ IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING from imio.googleauthenticator.tests.base import BaseTest from imio.googleauthenticator.helpers import extract_ip_address_from_request +from imio.googleauthenticator.helpers import get_app_settings +from imio.googleauthenticator.helpers import get_browser_hash from imio.googleauthenticator.helpers import get_ip_addresses_whitelist from imio.googleauthenticator.helpers import get_ip_ranges +from imio.googleauthenticator.helpers import get_ska_secret_key from ipaddress import IPv4Network from ipaddress import IPv4Address @@ -89,3 +96,104 @@ def test_extract_ip_address_still_strips_real_private_hops(self): self.assertEqual( IPv4Address(u'8.8.8.8'), extract_ip_address_from_request(request=request)) + + +class TestSkaSecretKey(unittest.TestCase, BaseTest): + """Concern-named class, like TestIPWhitelisting above: this file already + groups by concern rather than by module (R7), so the ska key derivation + and its browser-hash guard live in one class named for what they protect + rather than a second class named for test_helpers.py itself. + """ + + layer = IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING + + def setUp(self): + self.app = self.layer['app'] + self.portal = self.layer['portal'] + self.request = self.layer['request'] + self.portal_url = api.portal.get().absolute_url() + self._install() + # PLONE_FIXTURE logs the test user in (and caches its property + # sheets) before this class's own setUp installs the add-on's + # memberdata_properties.xml. Re-login so the cached user is rebuilt + # against the now-current portal_memberdata schema; otherwise + # setMemberProperties silently drops 'two_factor_authentication_secret' + # per MutablePropertySheet.setProperties (CLAUDE.md's documented + # "undeclared properties are popped" hazard -- here the property IS + # declared, but the cached sheet predates the declaration). + login(self.portal, TEST_USER_NAME) + + def test_get_ska_secret_key(self): + """BUG-04: the derivation must separate its three components instead + of bare-concatenating them, so two component tuples sharing the same + concatenation derive to different keys. + """ + user = api.user.get_current() + + # EXACT SHAPE: pins ordering (user secret first), the empty + # component (browser hash), the delimiter and the length semantics, + # all in one assertion. + user.setMemberProperties( + mapping={'two_factor_authentication_secret': 'ab'}) + get_app_settings().ska_secret_key = u'cd' + result = get_ska_secret_key( + request=self.request, user=user, use_browser_hash=False) + self.assertEqual(u'2:ab0:2:cd', result) + self.assertIsInstance(result, unicode) + + # THE COLLISION IT PREVENTS: the fixture above really does collide + # under the old bare-concatenation scheme. Without this line the + # fixture below looks arbitrary and a future editor could + # "simplify" it into one that no longer collides. + self.assertEqual(u'ab' + u'' + u'cd', u'a' + u'' + u'bcd') + + user.setMemberProperties( + mapping={'two_factor_authentication_secret': 'a'}) + get_app_settings().ska_secret_key = u'bcd' + second_result = get_ska_secret_key( + request=self.request, user=user, use_browser_hash=False) + self.assertNotEqual(result, second_result) + + # MINT UNTOUCHED: a non-empty ska_secret_key already in place is not + # re-minted by the derivation -- the registry value read back after + # both derive calls above is still the one explicitly set here. + self.assertEqual(u'bcd', get_app_settings().ska_secret_key) + + def test_get_ska_secret_key_handles_missing_secret_property(self): + """CR-01/WR-02 regression: a user whose + two_factor_authentication_secret property is unset/None (e.g. a + freshly created member that never went through + get_or_create_secret, or a cached property sheet that predates the + memberdata_properties.xml declaration -- see this class's setUp + docstring for why that can happen) must not crash + get_ska_secret_key() with 'TypeError: object of type NoneType has + no len()'. The old bare "{0}{1}{2}".format(...) concatenation + coerced None to the literal string "None" and never crashed; the + netstring-style len()-based derivation (BUG-04) must keep that same + crash-safety by coercing a falsy/None component to '' first, like + the sibling get_secret() already does. + """ + class FakeUser(object): + def getProperty(self, name, default=None): + return None + + result = get_ska_secret_key( + request=self.request, user=FakeUser(), use_browser_hash=False) + self.assertIsInstance(result, unicode) + + def test_get_browser_hash(self): + """Regression guard, not a fix for a live bug: get_browser_hash's + `except` branch already returns '' today (not None). It stopped + being merely cosmetic the moment Task 1's length-prefixed derivation + landed -- that derivation takes len() of this return value, and + len(None) raises TypeError on a login path. Nobody should go looking + for a currently-firing bug here; this pins the guard against a + future edit reintroducing a fall-off-the-end None. + """ + result = get_browser_hash(request={}) + self.assertEqual('', result) + self.assertIsNotNone(result) + + happy_result = get_browser_hash( + request={'HTTP_USER_AGENT': 'Mozilla/5.0'}) + self.assertEqual(40, len(happy_result)) diff --git a/src/imio/googleauthenticator/tests/test_setuphandlers.py b/src/imio/googleauthenticator/tests/test_setuphandlers.py new file mode 100644 index 0000000..799804d --- /dev/null +++ b/src/imio/googleauthenticator/tests/test_setuphandlers.py @@ -0,0 +1,142 @@ +import unittest2 as unittest + +from Products.CMFCore.utils import getToolByName + +from plone import api +from plone.app.testing import applyProfile + +from imio.googleauthenticator.helpers import get_app_settings +from imio.googleauthenticator.helpers import get_ska_secret_key +from imio.googleauthenticator.testing import \ + IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING +from imio.googleauthenticator.tests.base import BaseTest + + +class TestSetupHandlers(unittest.TestCase, BaseTest): + """Integration-layer assertions for setupVarious and the import-step + ordering it depends on. + + WR-03: one test method per requirement rather than one method bundling + all assertion groups, so a failure in an earlier group (e.g. import-step + ordering silently reverted) does not hide whether the later groups + (seeding, re-apply guard) still pass or fail. + + Post-review revision (CR-02): install-time seeding of ska_secret_key was + restored after code review found the lazy-mint-inside-a-getter design + (this phase's original D-04/D-05) writes registry state from + authenticateCredentials() -> sign_user_data(), a request path that ends + in transaction.abort() on Unauthorized and silently discards the mint -- + see 02-01-SUMMARY.md "Post-review revision" for the full account. The + tests below assert the current (post-revision) behaviour: install seeds + a non-empty key, and get_ska_secret_key() is a pure read that must not + mutate the registry. + """ + + layer = IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING + + def setUp(self): + self.app = self.layer['app'] + self.portal = self.layer['portal'] + self.request = self.layer['request'] + self.portal_url = api.portal.get().absolute_url() + self._install() + + def test_import_step_declares_registry_dependency(self): + """REG-03: the declaration is + recorded on our import step. + + This -- not the sorted order below -- is the control. Asserting only + the post-sort position of getSortedImportSteps() is a tautology in + the current fixture: with the line deleted, + 'imio.googleauthenticator' still lands after 'plone.app.registry' + (index 51 vs 36 of 52) purely by CPython 2.7 string-hash order, so + that assertion passes either way and would not catch the deletion. + Verified empirically during phase-2 verification. Asserting the + recorded dependency instead fails the moment the declaration goes. + """ + portal_setup = getToolByName(self.portal, 'portal_setup') + metadata = portal_setup.getImportStepMetadata( + 'imio.googleauthenticator') + self.assertIsNotNone( + metadata, + 'REG-03: imio.googleauthenticator import step must be registered') + self.assertIn( + 'plone.app.registry', + metadata['dependencies'], + 'REG-03: the import step must declare -- without it the registry records ' + 'may not exist when setupVarious runs, and the resulting ' + 'get_app_settings() KeyError is a swallowable PAS exception') + + def test_import_step_ordering(self): + """REG-03: imio.googleauthenticator's import step sorts after + plone.app.registry. + + Kept as the outcome check that the declared dependency above is + actually honoured by GenericSetup's topological sort. On its own it + proves nothing (see + test_import_step_declares_registry_dependency) -- the two together + assert both the declaration and its effect. + """ + portal_setup = getToolByName(self.portal, 'portal_setup') + steps = portal_setup.getSortedImportSteps() + self.assertGreater( + steps.index('imio.googleauthenticator'), + steps.index('plone.app.registry'), + 'REG-03: imio.googleauthenticator must sort after plone.app.registry') + + def test_registry_records_exist_after_install(self): + """REG-01/REG-02 outcome: after the default profile is applied, all + three IGoogleAuthenticatorSettings records exist and + get_app_settings() returns without raising. + """ + settings = get_app_settings() + self.assertIsNotNone( + settings.globally_enabled, + 'REG-01/REG-02: globally_enabled record must exist after install') + self.assertIsNotNone( + settings.ip_addresses_whitelist, + 'REG-01/REG-02: ip_addresses_whitelist record must exist after install') + + def test_install_seeds_ska_secret_key(self): + """REG-04, post-CR-02 revision: setupVarious seeds ska_secret_key at + install time (setuphandlers._setup_secret_key), so it is non-empty + immediately after install -- no PAS-plugin-path mint is required + for even the very first login. + """ + self.assertTrue( + get_app_settings().ska_secret_key, + 'REG-04: install must seed a non-empty ska_secret_key') + + def test_get_ska_secret_key_does_not_mutate_registry(self): + """CR-02 regression guard: get_ska_secret_key() must be a pure read + and must NOT write settings.ska_secret_key as a side effect. That + write, when it happened from sign_user_data() inside + authenticateCredentials(), was silently discarded by + transaction.abort() on the Unauthorized login path -- a test that + only checks the happy path (a getter returning a well-formed key) + does not catch a reintroduced mint; this asserts the registry value + is unchanged across the call. + """ + before = get_app_settings().ska_secret_key + get_ska_secret_key( + request=self.request, user=api.user.get_current(), use_browser_hash=False) + self.assertEqual( + before, get_app_settings().ska_secret_key, + 'CR-02: get_ska_secret_key() must not mutate ska_secret_key') + + def test_reapply_profile_does_not_reset_ska_secret_key(self): + """REG-05 regression guard, not a live bug fix: ska_secret_key is + TextLine(required=False, default=u''), so an existing non-empty + unicode value revalidates cleanly on profile re-import today, and + the "bare replaces the value with the field + default on re-import" hole does not fire. It would fire the day + someone adds required=True or a constraint to that field, which is + what this test guards against. + """ + known_value = u'known-test-value-for-reg-05' + get_app_settings().ska_secret_key = known_value + applyProfile(self.portal, 'imio.googleauthenticator:default') + self.assertEqual( + known_value, get_app_settings().ska_secret_key, + 'REG-05: re-applying the default profile must not reset ska_secret_key')