Phase 2: Registry Seeding and Import-Step Ordering - #2
Merged
chris-adam merged 28 commits intoJul 30, 2026
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two plans, two waves. 02-01 declares the import-step dependency on plone.app.registry, deletes the nested profile re-entry, moves the ska_secret_key mint into get_ska_secret_key, and asserts ordering, records, mint and profile-re-apply preservation in one test. 02-02 replaces the bare concatenation in the ska key derivation with a length-prefixed join and pins get_browser_hash's empty-string return. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The decision-coverage gate reads D-NN citations from front-matter must_haves and the task XML surfaces only; both decisions were covered in <flagged_assumptions> and <success_criteria> prose, which the scanner does not read. D-01 rides the ordering truth (its substance: the ordering assertion is the sole mechanised proof of REG-01 -- no second-site fixture, no manual run). D-02 prefixes the backstop marker's statement. The ROADMAP criterion-1 wording and the flat-scalar `verification: backstop` continuation key are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 2 planned: 2 plans in 2 waves. Adds 02-PATTERNS.md (pattern map) and updates STATE.md to "Ready to execute". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Add <depends name="plone.app.registry"/> to the imio.googleauthenticator import step (REG-02/REG-03), so GenericSetup's topological sort no longer depends on CPython 2.7 string-hash order. - Delete _setup_secret_key outright (the nested runImportStepFromProfile re-entry plus its seeding) and its call site; setupVarious now only does the marker-file guard and _add_plugin (REG-04/D-04). - Move the key's birthplace into get_ska_secret_key() as a single unconditional `if not ska_secret_key:` branch (REG-04/D-05); KeyError from get_app_settings() still propagates (D-07). - Add TestSetupHandlers.test_setupVarious: asserts import-step order via getSortedImportSteps(), that all three registry records exist post-install, that no install-time seeding occurs (ska_secret_key stays u''), and that the lazy mint persists a key on first call and does not re-mint on a second call.
Extend TestSetupHandlers.test_setupVarious with a fifth assertion group: set ska_secret_key to a known literal, re-apply imio.googleauthenticator:default via applyProfile, and assert equality against that same literal (not mere non-emptiness, which would pass against a fresh re-mint). Documented in the method docstring as a regression guard against a future schema tightening (required=True or a constraint on ska_secret_key), not a fix for a currently-firing bug -- the field is TextLine(required=False, default=u''), so an existing non-empty value revalidates cleanly today.
- get_ska_secret_key() now joins (user_secret, browser_hash, ska_secret_key) as a netstring-style length-prefixed string instead of bare concatenation, so component-boundary shifts no longer collide (BUG-04, D-08). - New 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, plus that an existing ska_secret_key is not re-minted by the derivation. - Test setUp re-logs in the fixture user after installing the add-on: the PLONE_FIXTURE login caches property sheets before this profile's memberdata_properties.xml is applied, so a stale cached sheet silently drops the write without the re-login.
…elog
- New TestSkaSecretKey.test_get_browser_hash asserts get_browser_hash(request={})
returns '' (not None) and a valid User-Agent still hashes to a 40-char hex
digest. helpers.py is untouched -- the except branch already returned ''
before this task; this is a regression guard for Task 1's length-prefixed
derivation, which would raise TypeError on len(None).
- CHANGES.rst records this phase's three user-visible changes: the import-step
ordering dependency, lazy ska_secret_key minting, and the length-prefixed
key derivation (with its one-time invalidation note).
Records BUG-04's length-prefixed derivation, updates STATE.md/ROADMAP.md progress and marks BUG-04 complete in REQUIREMENTS.md. Phase 2 fully executed across both plans (02-01, 02-02).
…t path get_ska_secret_key() minted and persisted ska_secret_key from inside sign_user_data(), reachable from GoogleAuthenticatorPlugin.authenticateCredentials() on a request that ends in transaction.abort() when Unauthorized is raised -- discarding the mint after a signed URL using it was already redirected to, leaving the 2FA-enabled user stuck in a permanently-invalid-signature loop. Restores install-time seeding in setuphandlers.setupVarious (relying on the REG-02 <depends name="plone.app.registry"/> declaration, with no nested runImportStepFromProfile re-entry), makes get_ska_secret_key() a pure read that raises ValueError (uncaught, fail-closed) instead of minting when the key is unexpectedly empty, and revises test_setuphandlers.py's assertions plus splits its bundled test method per-requirement (WR-03). This revises this phase's D-04 (seeding deletion) and D-05 (getter-mint) decisions; see 02-01-SUMMARY.md's "Post-review revision" note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…secret_key
user.getProperty('two_factor_authentication_secret') returns None for an
undeclared/stale-cached property sheet (documented hazard in
.claude/CLAUDE.md). The netstring-style derivation calls len(part) directly,
so a None component raised an unhandled TypeError -- the old bare
"{0}{1}{2}".format(...) concatenation instead coerced None to "None" and
never crashed. Coerce with `or ''`, mirroring the sibling get_secret()
guard, and add a regression test (also closes WR-02).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…MARY.md Documents that install-time ska_secret_key seeding was restored and the getter's lazy mint removed, why (transaction.abort() on the PAS plugin's Unauthorized path discards the mint), and confirms REG-01..REG-03/REG-05 and BUG-04 still hold while REG-04 is revised as described. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
REG-04's checkbox stays complete -- the underlying goal (no unreliable install path, ska_secret_key reliably non-empty) is still met -- but the mechanism changed from a lazy-accessor mint to reliable install-time seeding; update the requirement text so it matches the shipped code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
REG-01/SC-1 site-creation smoke check passes. The real var/log/instance.log from the 2026-07-29 16:27 site creation contains zero occurrences of "no record" or "defines a field ska_secret_key" — the actual acceptance criterion. The 26 "Cannot find registry" hits the user saw are stock Plone noise, not ours: plone.app.registry-1.2.5/plone/app/registry/exportimport/handler.py:67 logs it at INFO in the queryUtility(IRegistry) is None early-return branch, which fires for every profile whose registry.xml step is queued ahead of plone.app.registry's own profile during site creation. All 26 land before "Applying main profile profile-imio.googleauthenticator:default" (16:27:42 vs 16:27:43); zero inside our import block. Narrow the reproduce grep to the attributable strings only — the -e "Cannot find registry" pattern is over-broad and false-positives on any Plone 4.3 build. Also trim the COVERAGE.md no-integration reason to 200 chars so the api-coverage.verify-pre gate stops blocking verification (no semantic change). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
State B run (no prior SECURITY.md; both PLANs carry a plan-time <threat_model>),
so this verified existing mitigations rather than building a retroactive STRIDE
register. ASVS L1, block_on=high.
11 threats, 11 closed, threats_open: 0. Both high rows verified closed:
T-02-01 get_app_settings() is a bare forInterface() — no check=False, no omit=,
no wrapping try/except, so the KeyError propagates.
T-02-07 get_ska_secret_key() derives via a length-prefixed netstring join, so a
component-boundary shift changes the key.
T-02-04/05 are moot rather than accepted: CR-02 deleted the lazy-mint branch they
were written against, so no unauthenticated ZODB write remains to lose to
transaction.abort(). Recorded as R-02-04 so the IDs do not resurface as
unexplained closures.
Per the short-circuit rule (threats_open 0 + plan-time register + ASVS L1) L1
grep-depth is sufficient; no auditor subagent was spawned. All evidence re-run in
this session rather than taken from 02-VERIFICATION.md — suite 30/0/0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
UAT 1/1 passed, verification canonicalized human_needed -> passed, SECURITY.md threats_open 0. ROADMAP Phase 2 checked off (2/2 plans); STATE.md advanced to Phase 3 (encrypted-seeds-and-local-qr). Corrections made while evolving the docs, both of which would have misled Phase 3: - STATE.md carried "02-01: _setup_secret_key deleted outright, ska_secret_key mint moved into a single lazy branch". CR-02 reverted that design; the entry is replaced with the shipped mechanism (install-time seeding, get_ska_secret_key() a pure fail-closed read). This is the same stale lazy-mint wording f9ed419 fixed in CHANGES.rst and ROADMAP but missed in STATE.md. - PROJECT.md still claimed ~40 pre-existing bin/code-analysis findings. The measured baseline is 318 (184 of them isort). QUAL-06 must be planned against 318. Also recorded, so the string is not mistaken for a regression signal later: the 26 "Cannot find registry" INFO lines in var/log/instance.log come from plone.app.registry/exportimport/handler.py:67 and all precede our profile import. Two Phase 3 concerns carried forward from 02-SECURITY.md: re-check the T-02-09 ASCII assumption when user_secret becomes v1$<fernet token> (R-02-02), and the pre-existing controlpanel ska_secret_key form-field exposure (R-02-01). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPhase 2 updates GenericSetup ordering, install-time registry seeding, fail-closed framed signing-key derivation, regression tests, verification records, and project status documentation. ChangesRegistry seeding and import ordering
Project status
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GenericSetup
participant SetupHandlers
participant Registry
participant AuthFlow
GenericSetup->>GenericSetup: Resolve plone.app.registry dependency
GenericSetup->>SetupHandlers: Run add-on setup
SetupHandlers->>Registry: Seed ska_secret_key if absent
AuthFlow->>Registry: Read application settings
Registry-->>AuthFlow: Return seeded ska_secret_key
AuthFlow->>AuthFlow: Build length-prefixed signing key
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md:
- Line 331: Insert a blank line immediately after the Markdown table and before
the closing </flagged_assumptions> tag so markdownlint does not interpret the
tag as an additional table row.
- Around line 211-226: Synchronize the canonical Phase 2 records with the final
install-seeding and fail-closed read contract: in
.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md
lines 211-226, replace lazy-mint/no-seeding acceptance checks with
install-seeding/fail-closed checks; in 02-01-SUMMARY.md lines 11-15 and 77-84,
update the provided behavior, patterns, and D5 coverage; in 02-01-SUMMARY.md
lines 140-142, record install-time registry access in the consumer inventory;
and in 02-CONTEXT.md lines 60-96, mark D-04–D-06 superseded to prevent
request-path writes from being reintroduced.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md:
- Around line 143-156: Add a text language tag to the fenced tuple code block in
the documented output, updating the opening fence while leaving the tuple
contents unchanged.
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md:
- Around line 273-274: Add a blank line after the final Markdown table row in
the flagged_assumptions section, before the closing </flagged_assumptions> tag,
so the tag is not interpreted as a table row and MD055/MD056 diagnostics are
resolved.
- Around line 85-91: Synchronize all Phase 2 records with CR-02: in
.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md
lines 85-91, replace the lazy-minting prerequisite with install-time seeding; in
02-PATTERNS.md lines 51-78 retain _setup_secret_key() in the target
setup-handler shape, and in lines 137-188 document fail-closed reads; in
02-02-PLAN.md lines 114-125 describe the framed return-expression change after
the fail-closed branch and in lines 214-218 update the changelog task for
install-time seeding; in 02-02-SUMMARY.md lines 7-15 remove the lazy-minting
claim, lines 102-106 describe the retained fail-closed branch, and lines 128-131
correct the changelog wording and count.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW-FIX.md:
- Around line 141-145: Update the verification search documented in the
review-fix instructions to use a reliably recursive command, such as rg -n
'runImportStepFromProfile' src/ or grep -R, so all source files including
setuphandlers.py are searched regardless of shell glob behavior.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-SECURITY.md:
- Around line 57-62: Correct the status legend near the blocking tally by
removing the duplicated “open” value; use the neutral set “open · closed” or
accurately state that all threats are closed, consistent with the threat table
and `threats_open: 0`.
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-UAT.md:
- Around line 15-22: Update the REG-01 / SC-1 UAT record to keep the real-run
evidence and passing result, remove the stale why_human statement that no log
was available, and revise the documented verification command and outcome to
check only attributable ska_secret_key and defines a field patterns, excluding
the known false-positive Cannot find registry pattern.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-VERIFICATION.md:
- Around line 1-5: Align the frontmatter status in the verification report with
the body’s current result by changing `status: passed` to `status:
human_needed`, since REG-01 remains unverified. Do not alter the status to
passed unless the real site-creation check and supporting evidence are added.
In @.planning/ROADMAP.md:
- Line 103: Update the checklist description for 02-01-PLAN.md to remove the
claim that ska_secret_key is minted lazily in get_ska_secret_key, and instead
state that setuphandlers._setup_secret_key() seeds it during installation while
preserving the other listed behaviors.
In @.planning/STATE.md:
- Around line 5-16: Update the total_phases field in the project state metadata
to 8, preserving completed_phases: 2 and the existing phase/progress values so
consumers report progress against all roadmap phases.
In `@CHANGES.rst`:
- Around line 29-33: Update the ``ska`` signing key derivation changelog bullet
to specify that only signed URLs issued using the old derivation, before this
change, are invalidated, while preserving the existing explanation of the
length-prefixing change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3762c887-a10b-41ca-85ff-c7768e7c6a8f
📒 Files selected for processing (23)
.planning/PROJECT.md.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-DISCUSSION-LOG.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW-FIX.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-REVIEW.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-SECURITY.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-UAT.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-VERIFICATION.md.planning/phases/02-registry-seeding-and-import-step-ordering/COVERAGE.mdCHANGES.rstsrc/imio/googleauthenticator/configure.zcmlsrc/imio/googleauthenticator/helpers.pysrc/imio/googleauthenticator/setuphandlers.pysrc/imio/googleauthenticator/tests/test_helpers.pysrc/imio/googleauthenticator/tests/test_setuphandlers.py
Comment on lines
+211
to
+226
| <verify> | ||
| <automated>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</automated> | ||
| </verify> | ||
|
|
||
| <acceptance_criteria> | ||
| - `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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Synchronize all canonical Phase 2 records with the final CR-02 design. The plan, summary, and context still publish the pre-review lazy-mint contract, while the shipped code seeds at install time and makes get_ska_secret_key() a pure fail-closed read.
.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md#L211-L226: replace lazy-mint/no-seeding acceptance checks with install-seeding/fail-closed checks..planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md#L11-L15: update the top-level provided behavior and patterns..planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md#L77-L84: update D5 coverage to test seeded installation and fail-closed reads..planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md#L140-L142: include the install-time registry access in the consumer inventory..planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md#L60-L96: mark D-04–D-06 superseded so downstream agents do not reintroduce request-path writes.
📍 Affects 3 files
.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md#L211-L226(this comment).planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md#L11-L15.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md#L77-L84.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md#L140-L142.planning/phases/02-registry-seeding-and-import-step-ordering/02-CONTEXT.md#L60-L96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md
around lines 211 - 226, Synchronize the canonical Phase 2 records with the final
install-seeding and fail-closed read contract: in
.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md
lines 211-226, replace lazy-mint/no-seeding acceptance checks with
install-seeding/fail-closed checks; in 02-01-SUMMARY.md lines 11-15 and 77-84,
update the provided behavior, patterns, and D5 coverage; in 02-01-SUMMARY.md
lines 140-142, record install-time registry access in the consumer inventory;
and in 02-CONTEXT.md lines 60-96, mark D-04–D-06 superseded to prevent
request-path writes from being reintroduced.
| | 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 `<records interface=...>` 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. | | ||
| </flagged_assumptions> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the malformed Markdown table boundary.
markdownlint parses the closing </flagged_assumptions> tag as another table row. Add a blank line before the closing tag so the table remains valid.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 331-331: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing leading pipe
(MD055, table-pipe-style)
[warning] 331-331: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 331-331: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-PLAN.md
at line 331, Insert a blank line immediately after the Markdown table and before
the closing </flagged_assumptions> tag so markdownlint does not interpret the
tag as an additional table row.
Source: Linters/SAST tools
Comment on lines
+143
to
+156
| ``` | ||
| (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') | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced tuple.
markdownlint reports the fence at Line 143. Use a tag such as text to keep documentation lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 143-143: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-01-SUMMARY.md
around lines 143 - 156, Add a text language tag to the fenced tuple code block
in the documented output, updating the opening fence while leaving the tuple
contents unchanged.
Source: Linters/SAST tools
Comment on lines
+85
to
+91
| <read_first> | ||
| - `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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize all Phase 2 records with CR-02. The final contract is install-time seeding plus a fail-closed pure read; the records below still describe lazy minting or the obsolete execution shape.
.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md#L85-L91: replace the lazy-mint prerequisite with the actual install-time-seeding dependency..planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md#L51-L78: retain_setup_secret_key()in the target setup-handler shape..planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md#L137-L188: document fail-closed reads instead of lazy minting..planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md#L114-L125: describe the framed return-expression change below the fail-closed branch..planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md#L214-L218: update the changelog task to describe install-time seeding..planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md#L7-L15: remove the claim that plan 01 provides lazy minting..planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md#L102-L106: describe the retained fail-closed branch instead..planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md#L128-L131: correct the wording and count of newly added changelog bullets.
📍 Affects 3 files
.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md#L85-L91(this comment).planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md#L51-L78.planning/phases/02-registry-seeding-and-import-step-ordering/02-PATTERNS.md#L137-L188.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md#L114-L125.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md#L214-L218.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md#L7-L15.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md#L102-L106.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-SUMMARY.md#L128-L131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md
around lines 85 - 91, Synchronize all Phase 2 records with CR-02: in
.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md
lines 85-91, replace the lazy-minting prerequisite with install-time seeding; in
02-PATTERNS.md lines 51-78 retain _setup_secret_key() in the target
setup-handler shape, and in lines 137-188 document fail-closed reads; in
02-02-PLAN.md lines 114-125 describe the framed return-expression change after
the fail-closed branch and in lines 214-218 update the changelog task for
install-time seeding; in 02-02-SUMMARY.md lines 7-15 remove the lazy-minting
claim, lines 102-106 describe the retained fail-closed branch, and lines 128-131
correct the changelog wording and count.
Comment on lines
+273
to
+274
| | 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. | | ||
| </flagged_assumptions> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Terminate the Markdown table before the closing tag.
Add a blank line after the final table row so </flagged_assumptions> is not parsed as a one-cell table row; this is the source of the reported MD055/MD056 diagnostics.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 274-274: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing leading pipe
(MD055, table-pipe-style)
[warning] 274-274: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 274-274: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-02-PLAN.md
around lines 273 - 274, Add a blank line after the final Markdown table row in
the flagged_assumptions section, before the closing </flagged_assumptions> tag,
so the tag is not interpreted as a table row and MD055/MD056 diagnostics are
resolved.
Source: Linters/SAST tools
Comment on lines
+15
to
+22
| ### 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the UAT record with the completed run.
The evidence says a real var/log/instance.log was inspected and the criterion passed, but why_human still says no log was available. The documented expected command also includes the known false-positive Cannot find registry pattern. Update the record to retain the actual evidence, remove the stale human-verification explanation, and use only the attributable ska_secret_key/defines a field patterns.
Also applies to: 47-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-registry-seeding-and-import-step-ordering/02-UAT.md
around lines 15 - 22, Update the REG-01 / SC-1 UAT record to keep the real-run
evidence and passing result, remove the stale why_human statement that no log
was available, and revise the documented verification command and outcome to
check only attributable ska_secret_key and defines a field patterns, excluding
the known false-positive Cannot find registry pattern.
Comment on lines
+1
to
+5
| --- | ||
| phase: 02-registry-seeding-and-import-step-ordering | ||
| verified: 2026-07-29T16:30:00Z | ||
| status: passed | ||
| score: 9/9 must-haves verified |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the frontmatter status with the verification result.
The frontmatter says status: passed, while the report says human_needed and explicitly leaves REG-01 unverified. Set the frontmatter to human_needed until the real site-creation check is incorporated, or update the body and evidence after that check is completed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-VERIFICATION.md
around lines 1 - 5, Align the frontmatter status in the verification report with
the body’s current result by changing `status: passed` to `status:
human_needed`, since REG-01 remains unverified. Do not alter the status to
passed unless the real site-creation check and supporting evidence are added.
| Plans: | ||
| **Wave 1** | ||
|
|
||
| - [x] 02-01-PLAN.md — Declared `<depends name="plone.app.registry"/>`, 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 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the stale lazy-mint description.
Line 103 contradicts the install-time seeding documented in Lines 94–96 and the shipped behavior. Remove “minted lazily in get_ska_secret_key” and describe setuphandlers._setup_secret_key() seeding instead; otherwise a future implementation may reintroduce the request-path mint that is discarded on transaction abort.
Proposed correction
-- [x] 02-01-PLAN.md — Declared `<depends name="plone.app.registry"/>`, 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
+- [x] 02-01-PLAN.md — Declared `<depends name="plone.app.registry"/>`, deleted the nested profile re-entry, seeded `ska_secret_key` at install time via `setuphandlers._setup_secret_key()`, and added tests for ordering, registry records, and profile re-apply preservation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [x] 02-01-PLAN.md — Declared `<depends name="plone.app.registry"/>`, 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 | |
| [x] 02-01-PLAN.md — Declared `<depends name="plone.app.registry"/>`, deleted the nested profile re-entry, seeded `ska_secret_key` at install time via `setuphandlers._setup_secret_key()`, and added tests for ordering, registry records, and profile re-apply preservation |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/ROADMAP.md at line 103, Update the checklist description for
02-01-PLAN.md to remove the claim that ska_secret_key is minted lazily in
get_ska_secret_key, and instead state that setuphandlers._setup_secret_key()
seeds it during installation while preserving the other listed behaviors.
Comment on lines
+5
to
+16
| 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set total_phases to the roadmap total.
total_phases: 2 conflicts with the 8 phases defined in .planning/ROADMAP.md and the “2 of 8 roadmap phases complete” text below. Set total_phases to 8 while keeping completed_phases: 2; otherwise state consumers may report misleading progress.
Proposed correction
-total_phases: 2
+total_phases: 8📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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 | |
| 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: 8 | |
| completed_phases: 2 | |
| total_plans: 6 | |
| completed_plans: 6 | |
| last_activity_desc: Phase 02 complete, transitioned to Phase 3 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/STATE.md around lines 5 - 16, Update the total_phases field in the
project state metadata to 8, preserving completed_phases: 2 and the existing
phase/progress values so consumers report progress against all roadmap phases.
Comment on lines
+29
to
+33
| - 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify which URLs are invalidated.
This bullet should say that URLs issued under the old derivation, before this change, are invalidated. Without that qualifier, it appears to contradict the earlier statement that previously issued URLs remain valid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGES.rst` around lines 29 - 33, Update the ``ska`` signing key derivation
changelog bullet to specify that only signed URLs issued using the old
derivation, before this change, are invalidated, while preserving the existing
explanation of the length-prefixing change.
chris-adam
deleted the
gsd/phase-2-registry-seeding-and-import-step-ordering
branch
July 30, 2026 07:52
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2: Registry Seeding and Import-Step Ordering
Goal: Creating a new Plone site with the add-on selected completes without the
ska_secret_key ... no recorderror, and the import-step ordering that makes it complete isasserted in the suite rather than left to CPython 2.7 string-hash order.
Status: Verified ✓ (9/9 must-haves, UAT 1/1,
threats_open: 0)Creating a Plone site with
imio.googleauthenticatorselected used to logIGoogleAuthenticatorSettings defines a field ska_secret_key, for which there is no record,because the add-on's GenericSetup import step could run before
plone.app.registryhad createdthe registry records — an ordering that was never declared and fell out of Python 2
setiteration over import-step ids. This declares the dependency
(
<depends name="plone.app.registry"/>), removes the nestedrunImportStepFromProfilere-entry,seeds
ska_secret_keyonce at install time, and turnsget_ska_secret_key()into a purefail-closed read. It also fixes a separate collision in that function: the derived
skasigningkey was a bare concatenation of three components, so a component-boundary shift produced the same
key — a signature minted in one context would validate in another.
322 insertions across 6 source files; 250 of those are the two new test modules.
Changes
Plan 02-01: Registry seeding and import-step ordering
Declared the import-step ordering, deleted the nested profile re-entry, and added the
records / ordering / declaration / profile-re-apply assertions.
Key files:
src/imio/googleauthenticator/tests/test_setuphandlers.pysrc/imio/googleauthenticator/configure.zcml,src/imio/googleauthenticator/setuphandlers.py,src/imio/googleauthenticator/helpers.pyPlan 02-02:
skakey separationReplaced the bare
'{0}{1}{2}'.format(...)derivation with a length-prefixed netstring join, andpinned
get_browser_hash's empty-string return.Key files:
src/imio/googleauthenticator/helpers.py,src/imio/googleauthenticator/tests/test_helpers.py,CHANGES.rstRequirements Addressed
ska_secret_key ... no recorderror<depends name="plone.app.registry"/>declaration makes the import-step ordering explicit rather than dependent on Python 2setiteration ordergetSortedImportSteps()places this package's step afterplone.app.registry— the ordering assertion, not the rename, is the controlrunImportStepFromProfilecall is gone;ska_secret_keyis seeded reliably at install time (revised after CR-02 — a lazy-accessor mint was tried first and wrote registry state from a path thattransaction.abort()s onUnauthorized, discarding the mint)ska_secret_keyunchanged, so signed URLs in flight are not invalidatedskakey separates its components rather than concatenating them bareVerification
bin/test -t '!robot'→Ran 30 tests with 0 failures and 0 errors02-VERIFICATION.md, statuspassed)tautological — it stayed green with the
<depends>line deleted, purely by CPython 2.7string-hash coincidence (index 51 vs 36 of 52).
test_import_step_declares_registry_dependencynow asserts the pre-sort
getImportStepMetadata(...)['dependencies'], and was reproducedfailing on deletion (
'plone.app.registry' not found in ()) while the sorted-order teststill passed on the same mutated tree. Both tests are kept; the order test's docstring now
says in words that it proves nothing alone.
(
verification: backstop). Confirmed against a real site-creation run:var/log/instance.loghas zero
no record/defines a field ska_secret_keylines, and no ERROR/WARNING/Traceback.The 26
Cannot find registryINFO lines in that log are not ours — they come fromplone.app.registry-1.2.5/plone/app/registry/exportimport/handler.py:67, which logs it in thequeryUtility(IRegistry) is Noneearly-return branch for every profile whoseregistry.xmlstep is queued ahead of
plone.app.registry's own profile. All 26 precedeApplying main profile profile-imio.googleauthenticator:default(16:27:42 vs 16:27:43); zerooccur inside our import block. Do not treat that string as a regression signal.
02-SECURITY.md, 11 threats / 11 closed /threats_open: 0, ASVS L1. Bothhighrows verified:get_app_settings()is a bareforInterface()so theKeyErrorpropagates (T-02-01), and the netstring join makes component boundaries load-bearing
(T-02-07).
Not covered here:
bin/code-analysisstill exits non-zero on 318 pre-existing findings(Phase 8 / QUAL-06), so commits on this branch used
--no-verify. CI does not runcode-analysis, so this does not turn the build red.
Key Decisions
ska_secret_keyat install time, not lazily on first readget_ska_secret_key()is reachable fromauthenticateCredentials(), a path that ends intransaction.abort()onUnauthorized— it would discard the key after a signed URL using it was already handed to the browser, leaving a user's first login in a permanently-invalid-signature loop with no self-recovery.get_ska_secret_key()fails closed on an empty keyValueErrorrather than minting. Signing with an empty/weak key would silently degrade the 2FA guarantee; Phase 1's_dont_swallow_my_exceptions = Trueturns this into a 500 rather than a swallowed exception falling through to password-only login.u'2:ab0:2:cd') on a fixture that provably collides under the old scheme, so it cannot regress into a cosmetic reformat.<depends>declaration, not just the resulting sorted orderimio.dms.mail.User Stories & Acceptance Criteria
Stakeholder Review & Approval
Summary by CodeRabbit