Phase 1: Rename and Fail-Closed - #1
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Corrections: py2-ipaddress is fixable now (not py3-only), the memberdata hazard is transaction.abort() not ConflictError, and IChallengePlugin alone does not cover Plone 4.3's login POST path. Reversed the QR decision to qrcode==6.1 — zint takes the seed in argv. Lockout N/duration become control-panel settings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 1 rename + fail-closed research, verified against this working tree and this interpreter rather than recall. Six corrections to upstream research, four new findings, complete rename surface inventory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fix 5 blockers (all verify-command / precondition / ordering defects, each replacement command executed on this tree before writing it): - B1: 01-03 replace `bin/python -c "import setup"` (always exits 1 — distutils "no commands supplied") with `setup.py --long-description` - B2: 01-02 scope the i18ndude `cd` around `test -x`, not inside the command substitution - B3: 01-01 scope the clean-tree precondition to `git status --porcelain src/` - B4: move the phase-wide acceptance grep from 01-03 (wave 3) to 01-04 task 1 (wave 4), where the last three `src/` sites are renamed - B5: 01-01 depth-limit the egg-info find glob and add a profiles-scoped glob for the marker file, so the check stops colliding with 01-02's scope Address 8 warnings: per-commit shell gates for 01-01 commits 1 and 2 (W1, W6), RESEARCH Open Questions resolution table plus the D-18 English render test and the 318-finding STATE.md record (W2), T-1-SC renumbered T-1-12 and ASVS L1 baseline stated in all four registers (W3), positive zpt assert (W4), drop `--stat` (W5), site_properties.xml deletion removed as unrequested scope and .hgignore tied to RENAME-07 (W7), rebuild_i18n.sh expansion stated as accepted (W8). Suite count rises 12 -> 13 for the new English assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l validation map Revision iteration 2 — three items from the checker. BLOCKER: 01-02 task 2's verify opened with `msgfmt`, which is not installed on this machine, so the whole chain could only ever fail. Replaced with the compiler zope.i18n itself calls at ZCML load (pythongettext.msgfmt.Msgfmt), reached by lifting the resolved python_gettext egg path out of the generated bin/test. Verified on a real catalogue: exits 0 on the tracked nl .po, exits 1 on a .po whose msgstr lost its closing quote. bin/pybabel was rejected — it fails on that same valid catalogue (Babel 1.3 cannot parse the POT-Creation-Date placeholder) and passes a dropped quote, i.e. wrong in both directions. Updated in all three places (automated block, acceptance criterion, action step 6) plus the task precondition. W-A: 01-RESEARCH.md `## Open Questions` renamed to `(RESOLVED)` with a per-question RESOLVED marker naming the owning plan. W-B: 01-VALIDATION.md Per-Task Verification Map filled from the final task ids — Task ID, Plan, Wave, Threat Ref and the verbatim per-requirement clause of each owning task's automated chain. status/nyquist_compliant/wave_0_complete left honest; the file now states the flip is /gsd-validate-phase's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pure git mv of the package subtree from src/collective/ to src/imio/, plus purge of untracked artefacts left behind by the move: 29 orphan .pyc files, the stale src/collective.googleauthenticator.egg-info/ directory, and the untracked compiled .mo catalog (git clean -xdf src/). Also removes the now-stale develop-eggs/collective.googleauthenticator.egg-link, which sits outside src/ and would otherwise let pkg_resources resolve two distributions onto the same source tree. No content edits in this commit -- git's rename detection degrades when content changes alongside a ~50-file move, and the suite cannot run between this commit and the buildout regeneration anyway (bin/test still hardcodes the old package-name filter), so this commit is untestable by construction. Verified instead by shell assertion: src/collective gone from disk, no .pyc/.mo under src/, no stale egg-info or egg-link.
Sets base.cfg package-name and [code-analysis] directory, setup.py name and namespace_packages, and replaces src/imio/__init__.py with a byte-copy of imio.helpers' namespace declaration (coding cookie included, so the two imio.* eggs stay byte-identical -- a mismatched declaration style would make whichever imio/__init__.py loads first win and silently hide the other subpackage). bin/buildout -N -c test-4.3.cfg then regenerates bin/test (whose -s filter and eggs list are generated from base.cfg package-name) and adds the imio.googleauthenticator egg-link/egg-info alongside the already-removed collective.googleauthenticator ones. No pins were appended to test-4.3.cfg by this run. setup.py's version, author, url and classifiers are untouched here -- plan 01-03's work.
…leauthenticator
Every remaining collective.googleauthenticator identity in the package
now names imio.googleauthenticator: MessageFactory calls and
logging.getLogger names across the 15 Python source files, ZCML
i18n_domain/resourceDirectory/class/layer attributes in
configure.zcml/overrides.zcml/browser/configure.zcml, and the
GenericSetup XML (registry.xml records interface, browserlayer.xml
name+interface, componentregistry.xml factory, controlpanel.xml
i18n:domain+appId, cssregistry.xml and both jsregistry.xml resource
ids, skins.xml directory prefix, actions.xml i18n:domain). The skin
layer name googleauthenticator_custom is unchanged -- only the
package-prefix half of its directory= path moves.
GenericSetup marker-file invariant (RENAME-04): git mv the marker file
to imio.googleauthenticator.marker.txt and change the string
setuphandlers.setupVarious compares it against in the same commit --
a mismatch would make the handler return before the PAS plugin is
ever added, with no error. profiles/default/metadata.xml version reset
0301 -> 1000 (D-09): no site is registered at the new profile name, and
upgrades/ is deleted in this same commit (RENAME-09), along with its
<include package=".upgrades" /> line in configure.zcml -- a stale
include is a ZCML ConfigurationError at layer setup.
testing.py: renamed the layer class to ImiogoogleauthenticatorLayer,
the four IMIO_GOOGLEAUTHENTICATOR_* constants, the three layer name=
strings, and the z2.installProduct string (including in the
commented-out tearDownZope) -- an unresolvable name there logs and
continues, so initialize() never runs and registerMultiPlugin never
happens, invisibly. Updated every consumer: the layer-constant imports
in test_generic.py/test_pas_plugin.py/test_security.py/
test_helpers.py/test_robot.py, plus the two literal product-id strings
a mechanical import rewrite misses -- tests/base.py's _install() and
test_generic.py's test_product_is_installed pid.
Left untouched, by design: PAS_ID ('google_auth', an existing-ZODB
plugin id -- plan 01-04 does not touch it either), meta_type and
PAS_TITLE (plan 01-04's own commit, so a duplicate-meta_type
RuntimeError stays legible as "stale artefact"), locales/** filenames
and rebuild_i18n.sh's I18NDOMAIN (plan 01-02), and MANIFEST.in/setup.py
metadata fields beyond name/namespace_packages (plan 01-03).
bin/test -t '!robot' is green: 8 tests, 0 failures, 0 errors.
…viourally Three assertions a grep cannot replace, added on the existing test classes rather than as a separate functional layer: - test_imio_is_a_pkg_resources_namespace (TestGeneric): asserts 'imio' is in pkg_resources._namespace_packages and the distribution's namespace_packages.txt names exactly ['imio']. Fails on an empty src/imio/__init__.py, a pkgutil-style declaration, or a missing namespace_packages=['imio'] in setup.py -- no imio.helpers dependency added (that would pull plone.dexterity/pyjwt/cryptography into a Plone 4.3 pin set for a signal this 3-line assertion already gives). - test_plugin_is_registered_for_authentication (TestPas): asserts PAS_ID is in acl_users.plugins.listPlugins(IAuthenticationPlugin). test_plugin_is_installed (kept, not renamed) checks objectIds(), which a Broken ZODB object still satisfies; listPlugins filters on _satisfies() and logs the miss at debug, so this is the only assertion that would catch a Broken plugin or the marker-file mismatch RENAME-04 guards against. - test_resources_are_registered (TestGeneric): asserts ++resource++imio.googleauthenticator/main.js and /main.css are in portal_javascripts/portal_css getResourceIds(). Ties together the four files that must agree on the ++resource++ prefix -- a mismatch is otherwise a silent 404 on the asset. bin/test -t '!robot' is green at 11 tests, 0 failures, 0 errors (up from the pre-existing 8).
Records SUMMARY.md, STATE.md, ROADMAP.md and REQUIREMENTS.md updates for phase 01 plan 01: the move to imio.googleauthenticator, the buildout regeneration, the full dotted-name rename, and the three new behavioural tests. RENAME-01/02/04/05/07/08/09/12 marked complete.
… still renders
- git mv the .pot and nl .po to imio.googleauthenticator filenames (D-13); the i18n
domain is taken from these filenames, not from any i18n_domain attribute, so this
is what makes plan 01-01's renamed MessageFactory calls resolve at all
- correct the .pot header's Domain: line; confirmed by grep (not assumed) that the
Dutch .po header carries no old-domain reference
- fix rebuild_i18n.sh: I18NDOMAIN to imio.googleauthenticator, and I18NDUDE's path
from five levels up (resolves outside the repo) to three (D-15, pre-existing defect,
accepted one-line expansion since task 2 runs this script)
- add test_control_panel_is_translated_nl asserting the ska_secret_key schema label
translates to "Geheime Sleutel" under target language nl -- domain-level, not
browser-level, per the plan's own reasoning for avoiding portal_languages setup
Deviation: the plan's suggested msgid ("Google Authenticator settings" ->
"Google Authenticator instellingen") is a stale catalogue entry with no
corresponding _(...) call anywhere in current source, so task 2's i18ndude
rebuild-pot would drop it and break the assertion it is meant to protect.
Used the live "Secret Key" msgid instead -- same intent (live, discriminates,
untouched by task 2), documented in the test docstring.
… catalogues - Fix the three defective msgids at source: "ommit" -> "omitted" (whitelist description), missing "code" in the token form description, and the trailing space on the token field title (fixed at both source sites: reset_bar_code.py and user_setup.py -- one msgid, two call sites) - Create locales/fr and locales/en LC_MESSAGES directories with empty seed .po files, then run the now-fixed rebuild_i18n.sh to regenerate the .pot from live source and sync all three languages against it - French (D-17): every msgid translated using standard Belgian-French Plone vocabulary (vérification en deux étapes, code de vérification); flagged in a file header comment as awaiting user review before merge, not self-approved - English (D-18): every msgid duplicated as its own msgstr, so the corrected text renders whether Plone resolves through the en catalogue or short- circuits to the msgid; documented in a file header comment - Dutch (D-19): translated the three entries whose msgid text changed as a result of the source fixes above - test_corrected_msgid_renders_in_english added, asserting TokenForm's description translates to text containing "entering the verification code generated by" under target language en - All three catalogues verified compiling via pythongettext.msgfmt.Msgfmt (the exact call zope.i18n.compile.compile_mo_file makes), not GNU msgfmt (absent on this machine) or bin/pybabel (wrong in both directions on this tree, per RESEARCH) - bin/test -t '!robot' green at 13 tests, 0 failures, 0 errors Deferred (out of this plan's scope, documented in SUMMARY): the rebuild-pot step surfaced ~19 previously-never-extracted msgids (Cancel, Save, Globally enabled, Google Authenticator, the four ZCML action/uninstall strings, etc.) that the old, stale .pot never captured. These remain untranslated in Dutch; D-19 only covers the three msgids this plan's own corrections invalidated.
…e placement - setup.py: version 1.0.0.dev0, author iMio, url IMIO/imio.googleauthenticator, license GPL, dropped Python 2.6 classifier, added Plone 4.3 + GPLv2 classifiers, repointed README image-rewrite URL, changelog opens CHANGES.rst - CHANGES.txt -> CHANGES.rst, reformatted to imio.dms.mail house style, with a new 1.0.0 (unreleased) heading carrying the rename and DOC-04 non-migration notices; upstream history retained below, headings resized, 0.2.5 dated - AUTHORS.txt: upstream names under an explicit Original authors heading, iMio added as current maintainer - docs/LICENSE.txt -> LICENSE.txt at repo root; upstream notice kept, iMio copyright line added - examples/ deleted (upstream demo buildout, superseded, confirmed unreferenced)
The previous commit (92fef48) only captured the pure git-mv renames and the examples/ deletion -- a multi-path `git add` failed atomically because one listed path (already-renamed CHANGES.txt) no longer matched, so setup.py's metadata edits and the AUTHORS.txt/CHANGES.rst/LICENSE.txt content rewrites were silently left unstaged. Committing them now, verbatim as authored: version 1.0.0.dev0, author iMio, url IMIO/imio.googleauthenticator, license GPL, corrected classifiers, repointed README image URL, changelog reformatted to house style with the DOC-04 non-migration notice, AUTHORS.txt's Original authors heading, and LICENSE.txt's added iMio copyright line.
…catalogues and templates Replaced MANIFEST.in wholesale with the RESEARCH-verified template: commas removed from the recursive-include pattern line (they were literal, so every comma-suffixed glob matched nothing), the locales include scoped at the locales/ directory itself rather than nl/ only (so plan 01-02's fr/ and en/ catalogues actually ship), the nonexistent CONTRIBUTORS.txt include dropped, LICENSE.txt added, and global-exclude lines added for .pyc and .mo so a locally built artefact never leaks into a release. Verified by building a real sdist: no "no files found matching" warning, and the archive contains the message template, all three locale directories, the default profile (including the marker file and registry.xml), the ZMI template and the static resources, with no .pyc or .mo path present. bin/check-manifest still exits 1 -- its remaining diffs are all dev/tooling files (buildout configs, .planning/, .claude/, lint config) that are legitimately absent from the published sdist; not wired into bin/code-analysis (Flake8 only), so this task is the only place it is inspected. Diffs and justification recorded in the plan SUMMARY.
… naming the new package - .coveragerc's [report] include and cleanup.sh's egg-info path renamed to the new package location (Phase 8/QUAL-01 owns the rest of .coveragerc) - .hgignore and .hg.packed deleted: dead Mercurial leftovers, and .hgignore was the third config still naming the old egg-info path (RENAME-07) - Makefile: new purge target (.PHONY + double-hash help comment) removing stale post-rename .pyc under the old namespace, the old egg-info directory, and the local database; idempotent and tolerant of absent files, verified running twice with no change to git status - README.rst / docs/index.rst: title and underlines renamed, a "Forked from" line added, buildout install snippet renamed, the two dead documentation host links replaced with a single link to the IMIO repository, and the TODOS.rst raw-content link repointed at the IMIO repo. The PAS_TITLE quote is left untouched -- plan 01-04's commit - docs/conf.py: Sphinx project, htmlhelp_basename, epub_title and the commented epub_basename renamed - CLAUDE.md: naming section inverted (repo and package now agree), the bin/code-analysis finding count corrected from ~40 to 318 (184 isort), and the marker-file reference renamed - .planning/STATE.md Blockers/Concerns: recorded the 318-finding baseline so Phase 8 (QUAL-06) is planned against it, not the stale ~40 profiles/default/site_properties.xml is left in place -- byte-identical to propertiestool.xml and dead (never a recognised GenericSetup step filename), but tied to no requirement or decision; recorded here as an observation for Phase 8, not deleted. The plan-scoped acceptance grep (old name across setup.py, MANIFEST.in, .coveragerc, cleanup.sh, minus the RESEARCH false positives) is clean. src/ is deliberately excluded -- three sites there are plan 01-04's, whose task 1 owns the phase-wide grep.
- pas_plugin.py meta_type -> 'iMio Google Authenticator PAS' - setuphandlers.py PAS_TITLE parenthesised distro name -> imio.googleauthenticator (PAS_ID unchanged) - www/add_google_authenticator_form.zpt ZMI add-form heading matches new meta_type - README.rst / docs/index.rst installation step quotes the new title
- test_plugin_exception_is_not_swallowed injects a ValueError through is_whitelisted_client (a real collaborator, the first statement of authenticateCredentials) and asserts it escapes acl_users._extractUserIds rather than being swallowed into a source_users fallthrough. - Fails as expected pre-fix: "ValueError not raised" -- confirms the request-setup assumption (RESEARCH A1) holds and PAS currently swallows it. - test_plugin_exception_is_swallowed_without_the_flag documents the counterfactual and already passes, showing today's swallow-and-fall-through behaviour.
…faced
- _dont_swallow_my_exceptions = True on GoogleAuthenticatorPlugin: PAS's
reraise() now propagates NameError/AttributeError/KeyError/TypeError/
ValueError from this plugin instead of silently falling through to
source_users (password-only login). Scoped to this plugin only; the
inner delegation loop (pas_plugin.py, calling reraise() on *other*
plugins) is untouched, per plan.
- test_plugin_exception_is_not_swallowed now passes (was RED in the
prior commit).
Turning on the flag immediately surfaced two pre-existing bugs that PAS's
silent swallow had been masking on every single request -- both fixed here
as blocking issues (deviation Rule 1/3), neither is the inner delegation
loop the plan says to leave alone:
- helpers.extract_ip_address_from_request raised ValueError on an empty
REMOTE_ADDR (hit by every request in this test browser, and possibly by
a misconfigured front end in production). Now returns None, and
is_whitelisted_client treats that as "not whitelisted" -- fail-closed on
the whitelist itself rather than fail-crashed.
- pas_plugin's debug log line called user.getProperty('username'), a
property that does not exist on Plone's MemberData schema, raising
ValueError on every real login attempt. Switched to user.getUserName(),
the correct existing API for the same information -- no widening of the
username-in-debug-logs surface Phase 4 owns removing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…quirements after gaps found Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
api.user.get() returns None for a login that does not match any account; the next line called user.getUserName() unconditionally, raising an AttributeError that RENAME-11's _dont_swallow_my_exceptions now lets propagate as an HTTP 500 on every failed login with an unknown username.
extract_ip_address_from_request() only guarded against an empty IP; any non-empty but unparseable X-Forwarded-For value (client-controlled) reached ipaddress.ip_address() and raised ValueError, which RENAME-11's _dont_swallow_my_exceptions now lets crash the request -- an unauthenticated DoS against the login path. Return None (treated as not-whitelisted) instead.
A trailing newline in the control panel's ip_addresses_whitelist textarea
(the ordinary result of editing it) produced a trailing '' entry; get_ip_ranges()
then called ipaddress.ip_network('') and raised ValueError, uncaught, from the
first statement of authenticateCredentials -- crashing every login for every
user on a plausible misconfiguration. Filter blank lines at the source and make
get_ip_ranges skip any one invalid entry instead of raising.
PRIVATE_IPS_PREFIX matched '172.'/'192.' as string prefixes, sweeping up all of 172.0.0.0/8 and 192.0.0.0/8 (only 172.16.0.0/12 and 192.168.0.0/16 are actually RFC1918 private -- e.g. Google's public 172.217.0.0/16 was wrongly treated as private). A client whose real IP happened to match one of these prefixes got silently skipped in favour of the next, attacker-controlled X-Forwarded-For entry, undermining the whitelist's trust model.
enable_two_factor_authentication_for_users(users=[]) and disable_two_factor_authentication_for_users(users=[]) used the classic mutable-default-argument footgun. Neither function mutates it today, so this was latent, but costs nothing to avoid before either function grows an .append()/.remove().
username is typically unicode (from self.request.get('auth_user', '')).
str(username) implicitly encodes as ASCII in Python 2 and raises
UnicodeEncodeError for any non-ASCII character, crashing the final,
successful step of 2FA login for accented usernames. _setupSession accepts
unicode directly on this stack, so the str() call is simply unnecessary.
Both forms caught every exception on the success path and rendered str(e) straight back to the browser via a status message. .claude/CLAUDE.md's security constraints state secret material must never surface in an exception message; this pattern was an easy invariant to violate the next time either try block is touched, and a general information-disclosure smell independent of secrets. Log the detail server-side and show a generic user-facing message instead.
The test committed in 0018bca errored: authenticateCredentials() calls is_whitelisted_client() -> zope.globalrequest.getRequest() first, which returns None outside a bound request. Bind the layer request explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
iMio house terminology, settled during UAT of the review-draft French translation. 13 strings. Catalogue re-verified: compiles via pythongettext.Msgfmt, 0 placeholder mismatches across 60 entries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
719884e fixed reset_bar_code.py and user_setup.py but the code review only named those two files. request_bar_code_reset.py had the same 'reason = _(str(e))' pattern, on a form reachable by unauthenticated users. Found while verifying T-1-11's 'plain uniform server error' mitigation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
12 threats from the four PLAN threat models verified at ASVS L1. threats_open: 0. T-1-11 was open at audit start (third exception-echo site) and closed by 08687ac. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Marks Phase 1 complete in ROADMAP.md, advances STATE.md to Phase 2, and ticks the 13 RENAME-*/DOC-04 requirements. Also clears the stale 'Gaps Found' traceability rows left over from the pre-fix verification pass — phase.complete ticks the checkbox list but not the table's status column. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the .claude/ GSD installation (agents, commands, hooks, gsd-core, scripts) so the team shares identical tooling, and commits the pending .planning/config.json edits (model_profile: adaptive, use_worktrees: false, worktree_skip_hooks: true). 428 files, ~85.5k lines, 9.2M. No credentials in the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 527 files, which is 377 over the limit of 150. To get a review, narrow the scope: Upgrade to Pro+ to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (548)
You can disable this status message by setting the Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 1: Rename and Fail-Closed
Goal: The package is
imio.googleauthenticatoreverywhere — on disk, in the egg, in the i18n domain, in the GenericSetup profile and its marker file — and any exception inside the PAS plugin becomes a 500 rather than a silent fallthrough to password-only authentication.Status: Verified ✓ (6/6 must-haves)
Forks
collective.googleauthenticatorintoimio.googleauthenticatorand closes the silent-bypass hole in the PAS plugin. The rename is exhaustive rather than cosmetic: the on-disk tree moves tosrc/imio/, and every place the old name was load-bearing moves with it — the i18n domain (which comes from thelocales/filenames, noti18n_domain), the GenericSetup marker file (setupVariousreturns silently on a mismatch, skipping PAS installation entirely), the++resource++prefixes,MANIFEST.in, and the build tooling. The security half adds_dont_swallow_my_exceptions = True, so a plugin exception is a loud 500 instead of PAS swallowing it and falling through tosource_users, which authenticates on password alone. That flag then exposed three crash-on-ordinary-input paths, all found by code review and fixed with regression tests, so the plugin is genuinely fail-closed rather than fail-crashed.Changes
Plan 01-01: Move and rename to imio.googleauthenticator
Package moved from
src/collective/tosrc/imio/googleauthenticator/, every dotted reference and GenericSetup identity renamed, buildout regenerated, suite green.Key files:
src/imio/__init__.py(new namespace declaration),src/imio/googleauthenticator/**(moved),setup.py,base.cfg,profiles/default/imio.googleauthenticator.marker.txt,setuphandlers.py,testing.py,tests/test_generic.py,tests/test_pas_plugin.py.upgrades/deleted.Plan 01-02: Locales, translations, and three defective msgids
Catalogues moved to the renamed domain filenames with a behavioural Dutch-translation test;
rebuild_i18n.sh's brokenI18NDUDEpath and staleI18NDOMAINfixed; three defective English msgids corrected at source; new French and English catalogues generated.Key files:
locales/imio.googleauthenticator.pot,locales/{nl,fr,en}/LC_MESSAGES/imio.googleauthenticator.po,rebuild_i18n.sh,browser/forms/token.py,browser/forms/reset_bar_code.py,browser/forms/user_setup.py.Plan 01-03: Packaging metadata, MANIFEST.in rewrite, build tooling and docs
Distribution identity locked (version
1.0.0.dev0, iMio authorship, GPL, corrected classifiers) with the DOC-04 non-migration notice;MANIFEST.inrewritten wholesale and proven by a real sdist build; idempotentmake purgetarget added.Key files:
setup.py,MANIFEST.in,CHANGES.rst,AUTHORS.txt,LICENSE.txt(moved to root),.coveragerc,cleanup.sh,Makefile,README.rst,docs/index.rst,docs/conf.py,CLAUDE.md.Plan 01-04: PAS identity rename and fail-closed exception handling
meta_type/PAS_TITLErenamed to iMio in an isolated commit (PAS_ID = 'google_auth'byte-identical);_dont_swallow_my_exceptions = Trueadded, proven RED-then-GREEN by a test injecting failure through a real collaborator.Key files:
pas_plugin.py,setuphandlers.py,www/add_google_authenticator_form.zpt,tests/test_pas_plugin.py.Post-review hardening
Code review found 11 issues; the 8 in-scope (Critical + Warning) were fixed, each with its own commit. The three Critical ones were crash paths the new fail-closed flag turned into 500s on entirely ordinary input:
api.user.get()returnsNonefor an unknown username, then.getUserName()raisesAttributeError— every typo'd login 500s0018bca, test repaired inc556eabX-Forwarded-Forreachesipaddress.ip_address()unguarded — unauthenticated remote DoS on the login pathe6d9e57ip_network('')— site-wide login outage from routine admin config316d636PRIVATE_IPS_PREFIXstring matching treated all of172./192.as private5156972d4c2a99str(username)raisedUnicodeEncodeErroron non-ASCII usernames24e58c4719884e,08687acWR-05 (the TOTP secret being sent to the Google Charts API for QR rendering) is deferred to Phase 3, which pins
qrcode == 6.1for in-process rendering.Requirements Addressed
imio.googleauthenticatoron disk, insetup.py, and in the egg name, withnamespace_packages=['imio']MessageFactory, logger names,IUserDataSchemaProviderlocales/filenamesgit mv-ed, stale.modeleted++resource++prefixes match the new package nameMANIFEST.in's hardcoded paths updated, verified by building an sdist.coveragerc,base.cfg,cleanup.sh,testing.py.pycfiles and the oldegg-infodirectoryupgrades/deletedmeta_typeandPAS_TITLErenamed;PAS_IDdeliberately unchanged_dont_swallow_my_exceptions = Trueon the plugin classIAuthenticationPluginCHANGES.rstrecords the rename and that existing databases are discarded, not migratedVerification
01-VERIFICATION.md)bin/test -t '!robot'), up from 8 pre-phase01-UAT.md) — 26 auto-covered by passing tests, 3 confirmed by a humanthreats_open: 0across 12 threats (01-SECURITY.md), 3 documented accepted risksbin/check-manifestexit 1 accepted as dev-file noisegoogle_authconfirmed listed underacl_users→ plugins → AuthenticationKnown, deliberate:
bin/code-analysisfails on 318 pre-existing isort/flake8 findings inherited from upstream. Out of scope here, scheduled for Phase 8 (QUAL-06) and documented inCLAUDE.md. Commits in this branch use--no-verifyfor that reason. CI does not run code-analysis, so the build is not red.Key Decisions
PAS_IDstaysgoogle_auth. The plugin id is what the PAS plugin registry and ordering key on; renaming it would orphan existing registrations. Only the human-facingmeta_typeand title became iMio.OFS.Uninstalled.Broken, andlistPluginsthen silently skips the plugin — an in-place upgrade would turn the second factor off with no error.CHANGES.rstcarries an explicit non-migration notice;make purgeremoves the local database so no one half-migrates by accident._dont_swallow_my_exceptions = Trueaccepts that a plugin bug becomes a hard failure for every in-site user rather than a degradation to password-only auth. Recorded as accepted risk R-01. The break-glass path is the Zope root administrator, which no in-site PAS plugin runs for by construction.meta_typeshipped in an isolated commit so a duplicate-registration startup error stays readable as a stale build artefact rather than a rename defect.skapinned to 1.7.5 andplone.testingleft unpinned — both load-bearing for Python 2.7 / Plone 4.3 and documented intest-4.3.cfg.User Stories & Acceptance Criteria
Stakeholder Review & Approval