Phase 5 - #4
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four plans across two waves for the PAS boundary phase.
Planning re-read the installed eggs and found three mechanical errors in
04-RESEARCH.md, each now recorded with file:line evidence:
- response.setBody('') is a no-op (HTTPResponse.py:459)
- the challenge redirect needs lock=1 (HTTPResponse.py:799-803)
- request.get('_2fa_pending') is attacker-settable (HTTPRequest.py:1250-1255)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Left untracked by the pattern-mapper run; 04-01/04-02/04-03 all cite it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 plans in 2 waves, plan-checker passed on first iteration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…foreCommit
One extractor vetoed end to end: authenticateCredentials becomes decide-only
(stashes a pending signal via _mark_2fa_pending, touches no RESPONSE, no
ZODB write beyond the pre-existing get_secret/get_or_create_secret path),
and a new redirect_pending_2fa IPubBeforeCommit subscriber drives the
actual redirect via the shared send_2fa_redirect(request, response) in
pas_plugin.py.
- MFA-02: response.body is cleared with a plain attribute assignment (not
setBody(''), which is a no-op per HTTPResponse.py:453-460) and then
locked with setBody('', lock=1) so plone.transformchain's own
IPubBeforeCommit subscriber cannot refill it.
- COEX-08 (login-POST half): the pending signal travels only through
request.other (never request.get, which falls through to form data and
cookies), written by _mark_2fa_pending and read by redirect_pending_2fa,
both keyed off shared REQUEST_KEY_PENDING/REQUEST_KEY_USER_ID constants.
- SEC-03 preserved: get_secret(user) -- a pure read, no ZODB write -- forces
the seed-decrypt check synchronously inside authenticateCredentials, so a
broken encryption key still raises out of _extractUserIds on this same
request, matching the pre-restructure guarantee.
- tests/test_challenge.py: TestPubBeforeCommitRedirect proves the body-clear
and its lock against a real HTTPResponse, the end-to-end login-POST
redirect to @@google-authenticator-token, and that the pending signal
cannot be forged via a query string.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Belt-and-braces MFA-02 control alongside the direct-call HTTPResponse assertion from Task 1: test_no_body_leak_over_http drives the login-POST through zope.testbrowser 3.11.1 / mechanize 0.2.5 and asserts the raw 302's own body is empty, without following the redirect. set_handle_redirect(False) and raiseHttpErrors=False alone are not enough for a clicked form control: Browser.getControl(...).click() -> _clickSubmit() re-raises any mechanize.HTTPError unconditionally and never consults raiseHttpErrors. Submitting the encoded POST directly through Browser.open() instead routes through the code path that actually honours the switch, so the test survives rather than needing to be dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ted on reinstall - Replace movePluginsDown(iface, listPlugins(iface)[:-1]) (accidental index-0 via most-recently-activated entry) with an explicit movePluginsTop call - Narrow _add_plugin's early-return guard to object creation only, so activation and ordering are re-asserted on every profile application, not only the first -- reinstalling the profile is now a real recovery for a plugin displaced by a third-party add-on - Guard activatePlugin with listPluginIds to avoid KeyError: Duplicate plugin id on reinstall, since movePluginsTop itself is already idempotent - Add test_plugin_is_first_authenticator (MFA-03 security control), test_reapply_profile_keeps_plugin_first_and_unique (proves the displace-then-recover path, red against the pre-restructure guard), and test_plugin_declares_no_challenge_protocol (Open Question 3)
Checkpoint (Task 2) answered by human operator on 2026-07-31: keep credentials_basic_auth active rather than deactivate it. Recorded as a comment block above _add_plugin in setuphandlers.py naming the date, the three repositories the evidence covered (imio.dms.mail, server.dmsmail, industrialisation), the veto path (test_pas_plugin.py::test_basic_auth_veto, plan 04-03), and the load-bearing consequence: test_plugin_is_first_authenticator is the only thing standing between a future plugin reorder and a Basic Auth bypass. No _deactivate_basic_auth code added -- the "keep" branch writes no deactivation, only makes the decision visible in the repository.
Records the checkpoint decision (keep credentials_basic_auth active), updates STATE.md position/decisions, ROADMAP.md plan progress, and marks MFA-03 complete in REQUIREMENTS.md.
GoogleAuthenticatorPlugin now also implements IChallengePlugin: challenge() returns True and redirects to @@google-authenticator-token exactly when this request's authenticateCredentials marked it pending, reusing the same send_2fa_redirect builder the login-POST subscriber uses so the two entry points cannot drift. Write-free by construction (the transaction is already aborted by the time this runs) and status-locked (send_2fa_redirect's redirect(..., lock=1) survives HTTPResponse.exception's later 401). Open Question 3 resolved empirically as "not needed": once classImplements declares IChallengePlugin, 04-02's existing movePluginsTop loop (already unconditional over every interface the plugin provides) puts google_auth first among IChallengePlugin with no new code -- confirmed by tests/test_challenge.py::test_challenge_fires_on_unauthorized landing on the token form rather than credentials_cookie_auth's login_form, and by a one-off listPlugins(IChallengePlugin) inspection during development showing google_auth first. - test_challenge_declines_without_the_flag, test_challenge_writes_nothing, test_challenge_fires_on_unauthorized added to test_challenge.py - Updated the _dont_swallow_my_exceptions comment now that Phase 4's boundary rework is complete (04-01's decide-only split plus this plan's challenge())
…tion path
Five methods added to TestPas, each proven load-bearing by a recorded
mutation check (04-03-SUMMARY.md) rather than by a green run alone:
- test_form_post_veto (MFA-04) and test_basic_auth_veto (MFA-01), each with
a disabled-2FA non-vacuity control run first, so a pass cannot be
explained by a wrong password in the fixture. test_basic_auth_veto
asserts through the normal _extractUserIds path per 04-02-SUMMARY.md's
guidance (credentials_basic_auth stays active).
- test_both_extractors_at_once_grant_no_session (MFA-04 adjacency probe):
one request carrying both form credentials and an Authorization: Basic
header for the same 2FA-enabled user -- both extractor passes must
independently grant nothing.
- test_empty_credentials_do_not_raise (MFA-04 empty probe):
authenticateCredentials({}) and the '' / None login variants return None
and raise nothing.
- test_exception_path_still_wipes_credentials (ROADMAP success criterion
5): injects via pas_plugin._mark_2fa_pending (the module-level seam
04-01 introduced) and asserts the shared dict is still empty on the
exception exit.
Commenting out the credentials-wipe loop turns the first three red;
moving the wipe below _mark_2fa_pending turns the fifth red (with the
wipe temporarily relocated to prove the reordering itself is load-bearing,
not just its presence) -- both mutations reverted after confirming.
Records the IChallengePlugin.challenge() addition, the five per-extractor veto tests, Open Question 3's empirical resolution, and the accepted HTTP-Basic-Auth-loops-forever finding. Updates STATE.md, ROADMAP.md, and REQUIREMENTS.md (MFA-01, MFA-04, COEX-08).
…DOC-01, DOC-02) Adds two README.rst sections an operator needs and no test could previously enforce: the Zope-root/emergency-user boundary this in-site PAS plugin cannot reach (DOC-01), and the settled `credentials_basic_auth` "keep active" decision with its WebDAV/FTP/XML-RPC consequence and the service-account + IP-whitelist alternative (DOC-02). Reconciles the "ZMI -> acl_users" section to read as a verification/recovery step now that movePluginsTop is profile-authoritative (04-02), rather than a manual install instruction. Both facts get a CI assertion on load-bearing identifiers rather than prose, per 04-VALIDATION.md's override of 04-RESEARCH.md's manual-only classification -- test_readme_documents_zope_root_limitation and test_readme_documents_basic_auth_consequence, each proven load-bearing by a delete-the-section mutation check run during execution (recorded in the plan summary) rather than trusted on a green run alone.
Records the phase as it actually shipped, sourced from 04-01/04-02/04-03's summaries rather than the plans: the response-body leak fix, the redirect's move to IPubBeforeCommit + IChallengePlugin, movePluginsTop re-asserted on every profile application, the credentials wipe running before delegation, the credentials_basic_auth "keep active" decision, and the new README sections documenting both.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… as UAT Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Operator confirmed no external consumer authenticates against this site's acl_users over HTTP Basic Auth, closing the manual-only verification that plan 04-02's keep-credentials_basic_auth decision left open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
24 threats from the four plan threat models verified against the implementation. All closed, threats_open 0. Five accepted risks recorded, including a note that Phase 5 must not attach lockout state to the send_2fa_redirect call chain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Marks Phase 4 complete in ROADMAP.md and moves STATE.md to Phase 5. Canonicalizes 04-VERIFICATION.md to status passed now that its one human verification item has passed UAT. Evolves PROJECT.md: moves Phase 4's requirements (MFA-01..04, COEX-08, DOC-01, DOC-02) to Validated and logs four Phase 4 decisions. Also moves Phase 3's requirements (SEC-01..08, BUG-02, BUG-03, BUG-05, DOC-03), which had been left in Active because PROJECT.md was last evolved after Phase 2. Records two Phase 5 constraints in STATE.md: lockout state must not be attached to the send_2fa_redirect call chain, and the seed-key failure mode for never-enrolled accounts needs a synchronous check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audits 04-VALIDATION.md, the record of which phase behaviours have automated test coverage, against the executed codebase. All 8 audited behaviours are covered: each named test command was run individually and returned 1 test, 0 failures. No tests needed writing. Fills the Plan, Wave and Threat Ref columns that were left TBD at seed time, adds test file and line references, and adds the non-vacuity evidence showing each test fails when its guarded behaviour is removed. Corrects three stale entries: the quick-run command omitted test_challenge (7 of this phase's tests); the runtime figures were phase-3 estimates (~11s/48 tests) rather than measured values (32.3s full suite, 24.4s quick run); and all three research open questions were still listed as unresolved though the execution settled each one. Records one target miss honestly rather than ticking it: the full suite takes 32.3s against a 30s feedback-latency target. The quick run meets it at 24.4s. Layer setup dominates both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sion Seeds 05-VALIDATION.md from the research validation architecture, keyed by requirement (phase 4's shape) with real test-function names rather than placeholders. ROADMAP.md Phase 5 changes: - **UI hint**: no — the UI safety gate word-matches the phase section and hits 'form', 'view' and 'layout' in Plone class names (token form view, layout.wrap_form, RegistryEditForm), blocking for a missing UI-SPEC on a phase whose only UI is two integer control-panel fields. - Success criterion 4 corrected: _is_possible_token is private to the pinned onetimepass==0.2.2 egg, not in this package, so it cannot be patched. The exact-6-digit gate must be new code in helpers.py. - Records the lockout scope decision: the counter and lock cover both browser/forms/token.py and browser/forms/reset_bar_code.py. reset-bar-code is anonymously reachable and validates the token before the reset signature, so leaving it unmetered keeps an anonymous TOTP guessing oracle open.
Three plans for Phase 5 (Drift, Replay and Lockout), covering MFA-05..13: tracer-first lockout on the token form, drift+replay+format gate in helpers.validate_token in one commit, and the same counter on @@reset-bar-code per the operator's scope override. --no-verify: bin/code-analysis fails on 318 pre-existing findings until Phase 8 (QUAL-06), the documented accepted state in CLAUDE.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
05-PATTERNS.md was left untracked by the pattern-mapper step; all three Phase 5 plans reference it from their <read_first> blocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
STATE.md status set to ready-to-execute with 3 plans. ROADMAP.md Phase 5 plan list filled in with the two waves and their blocking relationship.
…en.py Task 1 of plan 05-01: three int memberdata properties (two_factor_authentication_failed_attempts/_locked_until/_last_interval) declared in userdataschema.py and memberdata_properties.xml, two control-panel settings (max_failed_attempts=5, lockout_duration=900) on IGoogleAuthenticatorSettings, and is_account_locked/register_failed_second_factor/ reset_failed_second_factor in helpers.py. TokenForm.handleSubmit checks the lock before validate_user_data/validate_token and reuses the existing generic error message, and registers/resets the counter on failure/success. New tests/test_token.py::test_lockout_after_five_failures proves the fifth wrong code locks the account while the fourth does not. Non-vacuity: removing register_failed_second_factor's call site from the failure branch turned test_lockout_after_five_failures red (asserted 0 not greater than a future epoch); restored byte-identical and the full suite re-run green (68 tests, 0 failures).
… effect Task 2 of plan 05-01: test_new_memberdata_properties_round_trip (tests/test_helpers.py, new TestDriftAndReplay class) writes and reads back all three new int properties, asserts isinstance(value, int), asserts a float write raises PropertyValueError, and asserts the idempotent-reset edge. test_memberdata_properties_import_declares_expected_types (tests/test_setuphandlers.py) reads portal_memberdata's own property-map API after a real profile install rather than parsing the XML, with the two pre-existing properties as a non-vacuity control. test_control_panel_has_lockout_fields (tests/test_generic.py) asserts both new Int fields, their fieldset membership, and get_app_settings() readback after install. Non-vacuity: deleting the two_factor_authentication_locked_until line from memberdata_properties.xml turned both the round-trip test and the import test red; file restored byte-identical and the full suite re-run green (71 tests, 0 failures).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five sequential plans for Phase 8 (Coverage Instrument and Test Layers): - 08-01 (tracer, QUAL-01/02/03): corrected .coveragerc, set -e in the [test-coverage] template, buildout parts + coverage == 5.5, proven with a real red build. One commit, per ROADMAP same-commit requirement. - 08-02 (QUAL-05/07): profile install moves to setUpPloneSite; the Browser-driven quickinstaller helper and its 21 call sites deleted; installedness asserted via plugin registration, registry records and browser layer. - 08-03 (QUAL-05): every test class to the ZSERVER-free FunctionalTesting layer, integration layer retired, revealed failures fixed at their cause. - 08-04 (QUAL-04): branch coverage above 90% with real tests for the four weakest modules, CI pointed at bin/test-coverage. - 08-05 (QUAL-06): all lint findings fixed so bin/code-analysis exits 0 and the pre-commit hook works; stale figures in CLAUDE.md and codebase/TESTING.md corrected. Committed with --no-verify: the buildout pre-commit hook runs bin/code-analysis, which exits 1 until plan 08-05 lands (CONTEXT.md D-06). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- .coveragerc: [run] source/omit/branch, drop [report] include (QUAL-01) - base.cfg: enable [coverage]/[test-coverage] parts, drop createcoverage, add set -e to the test-coverage template (QUAL-02, QUAL-03) - test-4.3.cfg: pin coverage = 5.5, drop createcoverage = 1.5 pin (QUAL-03) Re-measured baseline under coverage 5.5: 1048 stmts / 131 missed, 286 branches / 60 partial, TOTAL 84% -- unchanged from the 4.2 reference, confirming the corrected instrument reproduces the known-good figure.
…rom functional layer - Add ImiogoogleauthenticatorLayer.setUpPloneSite(portal), applying the imio.googleauthenticator:default profile once per layer instead of once per test class via the old Browser-driven quickinstaller round trip. - Drop z2.ZSERVER_FIXTURE from IMIO_GOOGLEAUTHENTICATOR_FUNCTIONAL_TESTING's bases; the robot layer keeps its own.
…dness assertion - tests/base.py: delete BaseTest._install() (the Browser-driven prefs_install_products_form round trip) and its SITE_OWNER_NAME/ SITE_OWNER_PASSWORD import; _get_browser/_login_browser kept verbatim. - Delete all 16 measured call sites of _install() across 12 test files (plan estimated 21; actual measured lower in test_helpers.py, test_pas_plugin.py, test_request_bar_code_reset.py, test_setuphandlers.py and test_user_setup.py -- see SUMMARY reconciliation). - Delete all 8 self.qi_tool = getToolByName(..., 'portal_quickinstaller') assignments; drop the now-unused getToolByName import in test_controlpanel.py, test_request_bar_code_reset.py and test_security.py. - Delete the three unused `from plone.app.testing import quickInstallProduct` imports (test_generic.py, test_pas_plugin.py, test_security.py). - test_generic.py::test_product_is_installed rewritten (QUAL-07): installedness now asserted via PAS plugin registration for IAuthenticationPlugin, IGoogleAuthenticatorSettings registry-record presence (registry.forInterface, no hardcoded count), and browser layer registration -- not portal_quickinstaller. Added self.pas to TestGeneric.setUp. Deleted the commented-out test_disable_view block. - Updated three stale in-repo comments that named the deleted BaseTest._install() method as the reason for cross-test memberdata leakage, without changing the leakage-mitigation code they describe.
…e integration layer - Renamed IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING to IMIO_GOOGLEAUTHENTICATOR_FUNCTIONAL_TESTING at all 16 layer attributes and matching imports across the 12 test files that used it - Deleted the IMIO_GOOGLEAUTHENTICATOR_INTEGRATION_TESTING layer definition and the now-unused IntegrationTesting import from testing.py - Reworded a stale helpers.py comment naming IntegrationTesting so it does not reference an identifier that no longer exists in this package - bin/test -t '!robot': 111 tests, 0 failures, 0 errors (run twice, stable) -- the isolation change revealed no failures
- QUAL-05 marked Complete: every test class runs on the ZSERVER-free functional layer, integration layer retired - Isolation change revealed zero pre-existing failures; post-layer-change coverage baseline recorded for plan 08-04 (TOTAL 1048/131/286/61, 84%)
- New test_disable_two_factor_authentication.py: anonymous guard (401 + no property mutation), the property-mutation branch (all three properties cleared), and the status message + redirect branch. - Each test proven non-vacuous by temporarily breaking its branch, confirming red, then restoring byte-identical (verified: git diff on browser/disable_two_factor_authentication.py is empty). - Module coverage: 40% -> 100% (18 stmts, 2 branches). TOTAL branch coverage: 84% -> 85%.
… branches - Bulk disable view (@@google-authenticator-disable-for-all-users): asserts an 'info' message and that a previously-enabled user is actually disabled afterward. - handleSave's globally-disabled branch: asserts changes are applied and no user is disabled (the bulk-disable call in that branch stays commented out, unchanged). - handleSave's neither-true-nor-false branch (globally_enabled absent from extracted data entirely): asserts changes are applied and no user is enabled. - Each test proven non-vacuous by temporarily breaking its branch, confirming red, then restoring byte-identical (verified: git diff on both browser/*.py files touched during investigation is empty). - Module coverage: disable_two_factor_authentication_for_all_users.py 56% -> 100%; browser/controlpanel.py 68% -> 86%. TOTAL branch coverage: 85% -> 87%.
- test_reset_bar_code.py: 8 new methods closing reset_bar_code.py's remaining branches -- extraction errors, unknown username, non-site- local user, the two updateFields QR-embed branches (success and stale token), the handleSubmit success path, and the bare except Exception arm. Module coverage: 76% -> 99% (one branch, 200->215, left undrivable -- barcode_field is always truthy in this schema). - Discovered and worked around a process-wide shared-mutable-state hazard in the same module: updateFields() rewrites the qr_code schema field's *class-level* description in place, with no per-request copy, so a successful reset in one test leaked its QR-code HTML into every later test's assertions regardless of run order. Reset in setUp() to the captured schema default; no production code changed. - test_controlpanel.py: 3 further methods needed because closing reset_bar_code.py alone landed at 89.73% TOTAL, short of the plan's 90% truth criterion -- outside Task 3's originally declared file list, documented as a deviation in the SUMMARY. Covers handleSave's globally-enabled-True success and exception-arm branches (both previously unreached: the existing test_helpers.py test with the same intent returns early on an unrelated RequiredMissing extraction error and its assertion passes only by reading a leftover message from an earlier call in the same test), plus handleCancel. - .github/workflows/package-test.yml: test_command now runs bin/test-coverage instead of bin/test, so the 90% threshold gates every push. Nothing else in the file changed. - Each of the 11 new test methods proven non-vacuous by temporarily breaking its branch, confirming red, then restoring byte-identical. - TOTAL branch coverage: 87% -> 90% (90.03% precise). bin/test-coverage -t '!robot' exits 0.
Task 1 of 08-05: run the repository's own isort config (force_single_line, force_alphabetical_sort, line_length=120) across every .py file under src/imio/googleauthenticator/. Clears all three import-ordering codes (I001 259->0, I003 23->0, I004 77->0) plus incidental W292/W293/E271 findings that happened to live at import-block boundaries. No other code's count increased. base.cfg [code-analysis] untouched. Suite: 128 tests, 0 failures, 0 errors. Coverage: 90% TOTAL, bin/test-coverage exits 0. --no-verify: bin/code-analysis still reports E251/E302/F401/etc, cleared in Task 2.
Task 2 of 08-05: closed the 148 findings the mechanical sweep left behind across three groups. Group 1 (keyword spacing, E251): 100 findings in five files, almost all zope.schema field declarations with spaces around keyword-argument `=`. Fixed by removing them, matching the already-conforming browser/forms/reset_bar_code.py exemplar. Group 2 (real defects): deleted 9 genuinely-unused F401 imports (disable_two_factor_authentication_for_users in controlpanel.py -- only its enable counterpart is called, the disable path stays commented out; plone.testing.z2.Browser in three test files; a local `import os` in test_generic.py; four unused plone.app.testing constants in the placeholder test_security.py) and 2 F841 unused locals (`changes = self.applyChanges(data)` -> bare call; `except ... as e` -> bare except, since only a fresh message is raised). None was a re-export or registration side effect -- confirmed against the live tabulation, not the stale pre-phase audit. Group 3 (whitespace, 39 E302/E265/E261/E231/W291 findings): two blank lines before top-level defs, `# ` block-comment spacing, two spaces before inline comments, whitespace after `,` (trailing commas before a closing bracket removed), and trailing whitespace on three docstring example lines in adapter.py's CameFromAdapter :example: block -- documentation-only content, not a translated message or template string, so trimming it carries no behaviour risk. bin/code-analysis exits 0. Suite: 128 tests, 0 failures, 0 errors. Coverage: 90% TOTAL, bin/test-coverage exits 0. base.cfg [code-analysis] flake8-ignore/directory/flake8-extensions/pre-commit-hook byte-identical to their pre-plan values. No suppression added anywhere. This commit is made WITHOUT --no-verify: the buildout's pre-commit hook runs bin/code-analysis on staged content and this is the first commit since Phase 1 where it passes on its own.
…te (D-19)
Task 3 of 08-05. Both documents were made wrong by this same plan's own
Tasks 1-2, so D-19 corrects them here rather than deferring to a later
docs pass.
CLAUDE.md: replaced the lint-debt paragraph's live claim ("318 unfixed
findings ... commits need --no-verify until ... Phase 8") with what is
true now that bin/code-analysis exits 0 and a normal commit passes.
Kept the 318 (measured 01-03) and 500 (re-measured 2026-08-05 after
Phase 7) figures as dated historical context, not live counts. Updated
the Commands section's inline comment to match.
.planning/codebase/TESTING.md (generated 2026-07-28, predates the
buildout migration and the rename): corrected the four things D-19
names -- coverage section (bin/test-coverage -t '!robot' replaces the
removed unscoped coverage command and the corrected .coveragerc scope),
test-layer section (one IMIO_GOOGLEAUTHENTICATOR_FUNCTIONAL_TESTING
layer + the robot layer, applied once via the layer's own
setUpPloneSite hook; the old per-test-class Browser-driven install
round trip and the integration layer it ran on are both gone), every
collective.googleauthenticator/COLLECTIVE_GOOGLEAUTHENTICATOR_* naming
reference, and the test count (128, up from 8). No other file under
.planning/codebase/ touched.
bin/code-analysis still exits 0; bin/test-coverage -t '!robot' still
exits 0 at 90% TOTAL.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (85)
📒 Files selected for processing (59)
📝 WalkthroughWalkthroughThe change adds TOTP replay prevention, recovery codes, configurable lockouts, deferred 2FA redirects, safer credential handling, resource ownership cleanup, functional test coverage, and updated coverage tooling and documentation. ChangesMFA security and authentication flow
Deployment, resources, and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PASPlugin
participant BeforeCommitSubscriber
participant TokenForm
participant Helpers
Client->>PASPlugin: submit first-factor credentials
PASPlugin->>PASPlugin: clear credentials and mark pending 2FA
PASPlugin->>BeforeCommitSubscriber: publish pre-commit event
BeforeCommitSubscriber->>PASPlugin: build signed token redirect
PASPlugin-->>Client: redirect to TokenForm
Client->>TokenForm: submit TOTP or recovery code
TokenForm->>Helpers: validate_second_factor
Helpers-->>TokenForm: accept or reject and update state
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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 by CodeRabbit