From eb221a9db3eba43fda322631ad09638e4c2efb0e Mon Sep 17 00:00:00 2001 From: bifrost0x Date: Tue, 25 Aug 2026 17:11:53 +0200 Subject: [PATCH 1/2] Fix file workspace and MFA state handling --- app/account_step_up_routes.py | 12 +++- static/js/i18n.js | 12 ++++ static/js/sftp-file-manager.js | 2 +- static/js/webauthn.js | 4 +- templates/admin.html | 2 +- templates/change_password.html | 2 +- templates/index.html | 2 +- templates/login.html | 84 ++++++++++++++++++--------- templates/register.html | 2 +- templates/security.html | 6 +- tests/e2e/mfa.spec.js | 41 +++++++++++++ tests/e2e/run_app.py | 57 ++++++++++++++---- tests/js/sftp-transfer-queue.test.js | 33 +++++++++++ tests/test_factor_step_up_contract.py | 44 ++++++++++---- tests/test_key_management_ui.py | 2 +- tests/test_profile_launcher_ui.py | 2 +- tests/test_totp_routes.py | 24 +++++++- tests/test_webssh2_shell.py | 4 +- 18 files changed, 271 insertions(+), 64 deletions(-) diff --git a/app/account_step_up_routes.py b/app/account_step_up_routes.py index 897b167..9f58725 100644 --- a/app/account_step_up_routes.py +++ b/app/account_step_up_routes.py @@ -30,7 +30,14 @@ current_authentication_session, ) from .ldap_service import LDAPDirectory, LDAPLookupRejected, LDAPUnavailable -from .models import LDAPIdentity, OIDCIdentity, User, WebAuthnCredential, db +from .models import ( + LDAPIdentity, + OIDCIdentity, + TOTPAuthenticator, + User, + WebAuthnCredential, + db, +) from .oidc_service import OIDCStateError from .security_features import feature_is_active from .step_up import ( @@ -549,6 +556,9 @@ def disable_account_mfa(): ) except StepUpError: return _error("step_up_required", 403) + TOTPAuthenticator.query.filter_by( + user_id=user.id, + ).delete(synchronize_session=False) user.mfa_enabled = False db.session.commit() log_security_event( diff --git a/static/js/i18n.js b/static/js/i18n.js index 9d281aa..78e5195 100644 --- a/static/js/i18n.js +++ b/static/js/i18n.js @@ -467,6 +467,8 @@ const translations = { 'security.methodTotp': 'Authenticator app', 'security.confirmSecurityAction': 'Confirm security action', 'security.confirmDisableMfa': 'Disable every MFA factor?', + 'security.confirmDisableMfaAndRemoveTotp': 'Disable MFA and remove all authenticator apps? This cannot be undone.', + 'security.disableMfaAndRemoveTotp': 'Disable MFA and remove authenticators', 'security.invalidTotpCode': 'Enter a valid six-digit authenticator code.', 'security.mfaDisabled': 'MFA disabled', 'security.mfaEnabled': 'MFA enabled', @@ -1457,6 +1459,8 @@ const translations = { 'security.methodTotp': 'Ứng dụng xác thực', 'security.confirmSecurityAction': 'Xác nhận thao tác bảo mật', 'security.confirmDisableMfa': 'Tắt tất cả các yếu tố MFA?', + 'security.confirmDisableMfaAndRemoveTotp': 'T\u1eaft MFA v\u00e0 x\u00f3a t\u1ea5t c\u1ea3 \u1ee9ng d\u1ee5ng x\u00e1c th\u1ef1c? Kh\u00f4ng th\u1ec3 ho\u00e0n t\u00e1c thao t\u00e1c n\u00e0y.', + 'security.disableMfaAndRemoveTotp': 'T\u1eaft MFA v\u00e0 x\u00f3a tr\u00ecnh x\u00e1c th\u1ef1c', 'security.invalidTotpCode': 'Nhập mã xác thực gồm sáu chữ số hợp lệ.', 'security.mfaDisabled': 'Đã tắt MFA', 'security.mfaEnabled': 'Đã bật MFA', @@ -2520,6 +2524,8 @@ const translations = { 'security.methodTotp': 'Authenticator-App', 'security.confirmSecurityAction': 'Sicherheitsaktion bestätigen', 'security.confirmDisableMfa': 'Alle MFA-Faktoren deaktivieren?', + 'security.confirmDisableMfaAndRemoveTotp': 'MFA deaktivieren und alle Authenticator-Apps entfernen? Dies kann nicht r\u00fcckg\u00e4ngig gemacht werden.', + 'security.disableMfaAndRemoveTotp': 'MFA deaktivieren und Authenticator-Apps entfernen', 'security.invalidTotpCode': 'Gib einen gültigen sechsstelligen Authenticator-Code ein.', 'security.mfaDisabled': 'MFA deaktiviert', 'security.mfaEnabled': 'MFA aktiviert', @@ -3508,6 +3514,8 @@ const translations = { 'security.methodTotp': "Application d’authentification", 'security.confirmSecurityAction': 'Confirmer l’action de sécurité', 'security.confirmDisableMfa': 'Désactiver tous les facteurs MFA ?', + 'security.confirmDisableMfaAndRemoveTotp': 'D\u00e9sactiver l\u2019AMF et supprimer toutes les applications d\u2019authentification\u00a0? Cette action est irr\u00e9versible.', + 'security.disableMfaAndRemoveTotp': 'D\u00e9sactiver l\u2019AMF et supprimer les authentificateurs', 'security.invalidTotpCode': 'Saisissez un code d’authentification valide à six chiffres.', 'security.mfaDisabled': 'MFA désactivée', 'security.mfaEnabled': 'MFA activée', @@ -4533,6 +4541,8 @@ const translations = { 'security.methodTotp': 'Aplicación de autenticación', 'security.confirmSecurityAction': 'Confirmar acción de seguridad', 'security.confirmDisableMfa': '¿Desactivar todos los factores MFA?', + 'security.confirmDisableMfaAndRemoveTotp': '\u00bfDesactivar MFA y eliminar todas las aplicaciones de autenticaci\u00f3n? Esta acci\u00f3n no se puede deshacer.', + 'security.disableMfaAndRemoveTotp': 'Desactivar MFA y eliminar autenticadores', 'security.invalidTotpCode': 'Introduce un código de autenticación válido de seis dígitos.', 'security.mfaDisabled': 'MFA desactivado', 'security.mfaEnabled': 'MFA activado', @@ -5558,6 +5568,8 @@ const translations = { 'security.methodTotp': '身份验证器应用', 'security.confirmSecurityAction': '确认安全操作', 'security.confirmDisableMfa': '停用所有 MFA 因素？', + 'security.confirmDisableMfaAndRemoveTotp': '\u7981\u7528 MFA \u5e76\u79fb\u9664\u6240\u6709\u8eab\u4efd\u9a8c\u8bc1\u5668\u5e94\u7528\uff1f\u6b64\u64cd\u4f5c\u65e0\u6cd5\u64a4\u9500\u3002', + 'security.disableMfaAndRemoveTotp': '\u7981\u7528 MFA \u5e76\u79fb\u9664\u8eab\u4efd\u9a8c\u8bc1\u5668', 'security.invalidTotpCode': '请输入有效的六位身份验证器代码。', 'security.mfaDisabled': 'MFA 已停用', 'security.mfaEnabled': 'MFA 已启用', diff --git a/static/js/sftp-file-manager.js b/static/js/sftp-file-manager.js index cf45037..04003d2 100644 --- a/static/js/sftp-file-manager.js +++ b/static/js/sftp-file-manager.js @@ -151,7 +151,7 @@ class SFTPFileManager { left: this.createEmptyPaneState(), right: this.createEmptyPaneState(), }; - this.panes = this.workspaceEmptyPanes; + this.panes = { ...this.workspaceEmptyPanes }; this.standalonePanes = this.panes; this.embeddedPanes = null; this.activePane = 'left'; diff --git a/static/js/webauthn.js b/static/js/webauthn.js index 5de2d73..377e829 100644 --- a/static/js/webauthn.js +++ b/static/js/webauthn.js @@ -496,8 +496,8 @@ async function disableTotpMfa() { if (!window.confirm(t( - 'security.confirmDisableMfa', - 'Disable the MFA requirement for future sign-ins? Enrolled factors remain stored.' + 'security.confirmDisableMfaAndRemoveTotp', + 'Disable MFA and remove all authenticator apps? This cannot be undone.' ))) { return; } const headers = await stepUpHeaders('mfa.disable'); if (headers === null) { return; } diff --git a/templates/admin.html b/templates/admin.html index 615249d..d375960 100644 --- a/templates/admin.html +++ b/templates/admin.html @@ -328,7 +328,7 @@

Final destru
- + diff --git a/templates/change_password.html b/templates/change_password.html index 24918d2..2fed4e7 100644 --- a/templates/change_password.html +++ b/templates/change_password.html @@ -119,7 +119,7 @@

Change Password

- + diff --git a/templates/index.html b/templates/index.html index f7e4b8a..7701c64 100644 --- a/templates/index.html +++ b/templates/index.html @@ -1391,7 +1391,7 @@

File Preview

- + diff --git a/templates/login.html b/templates/login.html index fae5654..d7c12a2 100644 --- a/templates/login.html +++ b/templates/login.html @@ -96,9 +96,15 @@

The control center for your servers.
+ {% if mfa_required %} + Confirm sign-in +

Two-factor authentication

+

Use one of your available factors for this second step. A recovery code opens a restricted session where you renew your account protection.

+ {% else %} Sign in

Access your SSH workspace

Use the identity source configured for this WebSSH instance.

+ {% endif %}
{% with messages = get_flashed_messages(with_categories=true) %} @@ -113,31 +119,31 @@

Access your SSH workspace

{% set selected_auth_source = auth_source if auth_source in ('local', 'ldap') else ('ldap' if ldap_enabled else 'local') %} - {% if mfa_required and 'totp' in mfa_methods %} - - {% endif %} - {% if mfa_required and 'recovery' in mfa_methods %} - + {% if mfa_required %} + {% set default_mfa_method = 'totp' if 'totp' in mfa_methods else ('passkey' if 'passkey' in mfa_methods else 'recovery') %} + {% if mfa_methods|length > 1 %} +
+ {% if 'totp' in mfa_methods %} + + {% endif %} + {% if 'passkey' in mfa_methods %} + + {% endif %} + {% if 'recovery' in mfa_methods %} + + {% endif %} +
{% endif %} - {% if not mfa_required %} + {% else %}
+ + {% endif %} + {% if mfa_required and 'recovery' in mfa_methods %} + + {% endif %} {% if webauthn_enabled and (not mfa_required or 'passkey' in mfa_methods) %} -
- + - + diff --git a/templates/register.html b/templates/register.html index 6a18229..4c03e63 100644 --- a/templates/register.html +++ b/templates/register.html @@ -136,7 +136,7 @@

Create a local WebSSH account

- + diff --git a/templates/security.html b/templates/security.html index 6f720ab..a974860 100644 --- a/templates/security.html +++ b/templates/security.html @@ -151,7 +151,7 @@

Authenticator app (TOTP)

- +
@@ -217,8 +217,8 @@

Co
- + - + diff --git a/tests/e2e/mfa.spec.js b/tests/e2e/mfa.spec.js index 8ecfc78..5238fac 100644 --- a/tests/e2e/mfa.spec.js +++ b/tests/e2e/mfa.spec.js @@ -90,8 +90,49 @@ test('enrolls optional TOTP and completes password plus MFA login', async ({ await page.locator('#password').fill('browser-password'); await page.locator('form button[type="submit"]').click(); await expect(page.locator('#totpMfaPanel')).toBeVisible(); + await expect(page.locator('#authMfaMethodSwitcher')).toBeVisible(); + await expect(page.locator('#recoveryMfaPanel')).toBeHidden(); await expect(page.locator('#passwordAuthenticationForms')).toHaveClass(/hidden/); + await page.locator('[data-auth-mode="recovery"]').click(); + await expect(page.locator('#totpMfaPanel')).toBeHidden(); + await expect(page.locator('#recoveryMfaPanel')).toBeVisible(); + await page.locator('[data-auth-mode="totp"]').click(); + await expect(page.locator('#totpMfaPanel')).toBeVisible(); + await expect(page.locator('#recoveryMfaPanel')).toBeHidden(); await page.locator('#totpMfaCode').fill(totp(secret, Date.now())); await page.locator('#submitTotpMfa').click(); await expect(page).toHaveURL(/\/$/); + + await page.goto('/security'); + await expect(page.locator('#totpList')).toContainText('E2E authenticator'); + const dialogPromise = page.waitForEvent('dialog'); + const disableClick = page.locator('#totpDisableBtn').click(); + const dialog = await dialogPromise; + expect(dialog.message()).toContain('remove all authenticator apps'); + await dialog.accept(); + await disableClick; + await expect(page.locator('#totpList')).toContainText( + 'No authenticator app is enrolled.', + ); + await expect(page.locator('#totpDisableBtn')).toBeHidden(); +}); + +test('shows mixed MFA methods one at a time', async ({ page }) => { + await page.goto('/login'); + await page.locator('#username').fill('e2e_mixed_mfa'); + await page.locator('#password').fill('browser-password'); + await page.locator('form button[type="submit"]').click(); + + await expect(page.locator('#authMfaMethodSwitcher')).toBeVisible(); + await expect(page.locator('.auth-header')).toContainText('Two-factor authentication'); + await expect( + page.locator('[data-auth-mode-panel]:visible'), + ).toHaveCount(1); + + await page.locator('[data-auth-mode="passkey"]').click(); + await expect(page.locator('#totpMfaPanel')).toBeHidden(); + await expect(page.locator('#passkeyLoginMode')).toBeVisible(); + await expect( + page.locator('[data-auth-mode-panel]:visible'), + ).toHaveCount(1); }); diff --git a/tests/e2e/run_app.py b/tests/e2e/run_app.py index 770f563..7ad1b98 100644 --- a/tests/e2e/run_app.py +++ b/tests/e2e/run_app.py @@ -302,7 +302,13 @@ def main(): app = create_app() with app.app_context(): - from app.models import OIDCIdentity, SecurityFeatureState, db + from app.models import ( + OIDCIdentity, + SecurityFeatureState, + TOTPAuthenticator, + WebAuthnCredential, + db, + ) admin, error = register_user('e2e_admin', 'browser-password') if error: @@ -314,17 +320,46 @@ def main(): mfa_user, error = register_user('e2e_mfa', 'browser-password') if error: raise RuntimeError(error) - if not admin.is_admin or user.is_admin or mfa_user.is_admin: + mixed_mfa_user, error = register_user('e2e_mixed_mfa', 'browser-password') + if error: + raise RuntimeError(error) + if ( + not admin.is_admin + or user.is_admin + or mfa_user.is_admin + or mixed_mfa_user.is_admin + ): raise RuntimeError('E2E user roles were not seeded deterministically') - db.session.add(OIDCIdentity( - user_id=user.id, - issuer='https://issuer.example', - subject='existing-e2e-subject', - )) - db.session.add(SecurityFeatureState( - feature='totp', - enabled=True, - updated_by=admin.id, + mixed_mfa_user.mfa_enabled = True + db.session.add_all(( + OIDCIdentity( + user_id=user.id, + issuer='https://issuer.example', + subject='existing-e2e-subject', + ), + SecurityFeatureState( + feature='totp', + enabled=True, + updated_by=admin.id, + ), + SecurityFeatureState( + feature='passkey', + enabled=True, + updated_by=admin.id, + ), + TOTPAuthenticator( + user_id=mixed_mfa_user.id, + encrypted_secret=b'e2e-only-mixed-totp', + label='E2E authenticator', + active=True, + ), + WebAuthnCredential( + user_id=mixed_mfa_user.id, + credential_id=b'e2e-mixed-passkey', + public_key=b'e2e-only-public-key', + transports='[]', + name='E2E passkey', + ), )) db.session.commit() key = _seed_launcher_profiles(admin, user) diff --git a/tests/js/sftp-transfer-queue.test.js b/tests/js/sftp-transfer-queue.test.js index d21ae9c..4d8bd6e 100644 --- a/tests/js/sftp-transfer-queue.test.js +++ b/tests/js/sftp-transfer-queue.test.js @@ -74,6 +74,39 @@ test('standalone workspace starts in one pane with independent empty states', () assert.equal(manager.workspace.getActiveTab('right'), null); }); +test('closing the final tab restores an independent empty pane state', () => { + for (const pane of ['left', 'right']) { + const manager = Object.create(SFTPFileManager.prototype); + manager.initializeWorkspaceState(); + const populatedState = filePane(manager, `sftp-session:${pane}`, { + path: `/srv/${pane}`, + files: [{ name: 'stale.txt', is_dir: false }], + }); + const tab = manager.workspace.openTab( + pane, + fileSource(`sftp-session:${pane}`), + populatedState, + ); + manager.syncPaneFromWorkspace(pane); + Object.assign(manager, { + displayMode: 'modal', + updatePathInput() {}, + updatePaneBadge() {}, + renderPane() {}, + renderWorkspaceChrome() {}, + openSourceLauncher() {}, + }); + + manager.closeSourceTab(pane, tab.id); + + assert.equal(manager.workspace.getActiveTab(pane), null); + assert.notEqual(manager.panes[pane], populatedState); + assert.equal(manager.panes[pane].source, null); + assert.equal(manager.panes[pane].path, '/'); + assert.deepEqual(manager.panes[pane].files, []); + } +}); + test('pane state has one canonical source and no legacy identity fields', () => { const manager = Object.create(SFTPFileManager.prototype); diff --git a/tests/test_factor_step_up_contract.py b/tests/test_factor_step_up_contract.py index 09d99d3..4133dc3 100644 --- a/tests/test_factor_step_up_contract.py +++ b/tests/test_factor_step_up_contract.py @@ -199,13 +199,19 @@ def test_last_local_factor_cannot_be_deleted_while_mfa_is_enabled( assert db.session.get(WebAuthnCredential, credential_id) is not None -def test_mfa_disable_requires_strong_grant_and_preserves_enrolled_factors( +def test_mfa_disable_requires_strong_grant_and_removes_all_totp_authenticators( app, client, monkeypatch, ): import config - from app.models import SecurityFeatureState, TOTPAuthenticator, User, db + from app.models import ( + SecurityFeatureState, + TOTPAuthenticator, + User, + WebAuthnCredential, + db, + ) monkeypatch.setattr(config, "TOTP_ENABLED", True) app.extensions["security_feature_readiness"]["totp"] = (True, None) @@ -214,14 +220,28 @@ def test_mfa_disable_requires_strong_grant_and_preserves_enrolled_factors( user = db.session.get(User, user_id) user.mfa_enabled = True db.session.add(SecurityFeatureState(feature="totp", enabled=True)) - factor = TOTPAuthenticator( - user_id=user_id, - encrypted_secret=b"encrypted-secret", - active=True, - ) - db.session.add(factor) + db.session.add_all(( + TOTPAuthenticator( + user_id=user_id, + encrypted_secret=b"encrypted-secret-a", + label="Phone", + active=True, + ), + TOTPAuthenticator( + user_id=user_id, + encrypted_secret=b"encrypted-secret-b", + label="Tablet", + active=True, + ), + WebAuthnCredential( + user_id=user_id, + credential_id=b"unrelated-passkey", + public_key=b"public-key", + sign_count=0, + transports="[]", + ), + )) db.session.commit() - factor_id = factor.id headers = _strong_account_headers( app, client, @@ -238,4 +258,8 @@ def test_mfa_disable_requires_strong_grant_and_preserves_enrolled_factors( assert response.status_code == 200 with app.app_context(): assert db.session.get(User, user_id).mfa_enabled is False - assert db.session.get(TOTPAuthenticator, factor_id).active is True + assert TOTPAuthenticator.query.filter_by(user_id=user_id).count() == 0 + assert WebAuthnCredential.query.filter_by( + user_id=user_id, + credential_id=b"unrelated-passkey", + ).count() == 1 diff --git a/tests/test_key_management_ui.py b/tests/test_key_management_ui.py index 41e7113..694c9e4 100644 --- a/tests/test_key_management_ui.py +++ b/tests/test_key_management_ui.py @@ -102,7 +102,7 @@ def test_key_replacement_event_updates_ui_and_asset_version(): assert "socket.on('key_replaced'" in APP assert 'ProfileManager.upsertKeySummary(data.key)' in APP assert "filename='js/profile-manager.js') }}?v=13" in TEMPLATE - assert "filename='js/i18n.js') }}?v=24" in TEMPLATE + assert "filename='js/i18n.js') }}?v=25" in TEMPLATE assert "filename='js/app.js') }}?v=22" in TEMPLATE assert "filename='css/style.css') }}?v=23" in TEMPLATE diff --git a/tests/test_profile_launcher_ui.py b/tests/test_profile_launcher_ui.py index 266521e..6215588 100644 --- a/tests/test_profile_launcher_ui.py +++ b/tests/test_profile_launcher_ui.py @@ -34,7 +34,7 @@ def test_merged_profile_frontend_assets_have_distinct_cache_versions(): expected_versions = { "filename='css/style.css'": '?v=23', "filename='css/sftp-file-manager.css'": '?v=12', - "filename='js/i18n.js'": '?v=24', + "filename='js/i18n.js'": '?v=25', "filename='js/command-workspace.js'": '?v=2', "filename='js/command-palette-utils.js'": '?v=1', "filename='js/profile-launcher-utils.js'": '?v=5', diff --git a/tests/test_totp_routes.py b/tests/test_totp_routes.py index ae7b57a..1a680b7 100644 --- a/tests/test_totp_routes.py +++ b/tests/test_totp_routes.py @@ -180,7 +180,13 @@ def test_passkey_remains_visible_as_an_alternative_mfa_method( monkeypatch, ): import config - from app.models import SecurityFeatureState, User, WebAuthnCredential, db + from app.models import ( + SecurityFeatureState, + TOTPAuthenticator, + User, + WebAuthnCredential, + db, + ) user_id = _create_user(app, "mixed_factor_user") _activate_totp_feature(app, monkeypatch) @@ -189,6 +195,11 @@ def test_passkey_remains_visible_as_an_alternative_mfa_method( db.session.merge(SecurityFeatureState(feature="passkey", enabled=True)) user = db.session.get(User, user_id) user.mfa_enabled = True + db.session.add(TOTPAuthenticator( + user_id=user_id, + encrypted_secret=b"encrypted-mixed-factor-secret", + active=True, + )) db.session.add(WebAuthnCredential( user_id=user_id, credential_id=b"mixed-factor-credential", @@ -207,6 +218,17 @@ def test_passkey_remains_visible_as_an_alternative_mfa_method( assert response.status_code == 200 assert 'id="passkeyLoginBtn"' in html assert 'id="passwordAuthenticationForms" class="hidden"' in html + assert 'id="authMfaMethodSwitcher"' in html + assert 'data-auth-mode="totp" aria-selected="true"' in html + assert 'data-auth-mode="passkey" aria-selected="false"' in html + assert ( + 'id="totpMfaPanel" class="login-mode" ' + 'data-auth-mode-panel="totp" aria-hidden="false"' + ) in html + assert ( + 'id="passkeyLoginMode" class="login-mode auth-provider-mode hidden" ' + 'data-auth-mode-panel="passkey" aria-hidden="true"' + ) in html def test_mfa_disable_is_explicit_and_recently_reauthenticated( diff --git a/tests/test_webssh2_shell.py b/tests/test_webssh2_shell.py index 61fad44..c0fa2b3 100644 --- a/tests/test_webssh2_shell.py +++ b/tests/test_webssh2_shell.py @@ -114,7 +114,7 @@ def test_every_user_facing_page_uses_current_shared_asset_versions(app, client): assert response.status_code == 200 assert b'css/style.css?v=23' in response.data assert b'css/webssh-2.css?v=15' in response.data - assert b'js/i18n.js?v=24' in response.data + assert b'js/i18n.js?v=25' in response.data client.post("/logout") for path in ("/login", "/register"): @@ -122,7 +122,7 @@ def test_every_user_facing_page_uses_current_shared_asset_versions(app, client): assert response.status_code == 200 assert b'css/style.css?v=23' in response.data assert b'css/webssh-2.css?v=15' in response.data - assert b'js/i18n.js?v=24' in response.data + assert b'js/i18n.js?v=25' in response.data def test_authentication_pages_use_the_shared_professional_auth_shell(): From 51305762378ff843a7a361ddbc51c3429472399f Mon Sep 17 00:00:00 2001 From: bifrost0x Date: Tue, 25 Aug 2026 17:44:13 +0200 Subject: [PATCH 2/2] Delete TOTP factors on every MFA disable path --- app/account_step_up_routes.py | 8 ++------ app/auth_assurance.py | 3 ++- app/totp_routes.py | 3 ++- app/totp_service.py | 8 ++++++++ tests/test_recovery_service.py | 19 ++++++++++++++++++- tests/test_totp_routes.py | 11 ++++++++++- 6 files changed, 42 insertions(+), 10 deletions(-) diff --git a/app/account_step_up_routes.py b/app/account_step_up_routes.py index 9f58725..32cc70a 100644 --- a/app/account_step_up_routes.py +++ b/app/account_step_up_routes.py @@ -33,7 +33,6 @@ from .models import ( LDAPIdentity, OIDCIdentity, - TOTPAuthenticator, User, WebAuthnCredential, db, @@ -52,7 +51,7 @@ consume_account_step_up_grant, recent_strong_assurance, ) -from .totp_service import verify_totp +from .totp_service import disable_totp_mfa, verify_totp from .webauthn_service import ChallengeError, consume_challenge, create_challenge @@ -556,10 +555,7 @@ def disable_account_mfa(): ) except StepUpError: return _error("step_up_required", 403) - TOTPAuthenticator.query.filter_by( - user_id=user.id, - ).delete(synchronize_session=False) - user.mfa_enabled = False + disable_totp_mfa(user) db.session.commit() log_security_event( "ACCOUNT_STEP_UP_CONSUMED", diff --git a/app/auth_assurance.py b/app/auth_assurance.py index 798b212..db3ae0b 100644 --- a/app/auth_assurance.py +++ b/app/auth_assurance.py @@ -25,6 +25,7 @@ db, ) from .session_epoch import current_epoch +from .totp_service import disable_totp_mfa _PENDING_TTL = timedelta(minutes=5) @@ -497,7 +498,7 @@ def clear_recovery_restriction(*, replacement_factor=None, disable_mfa=False): if user is None: raise AuthenticationFinalizationError("account is no longer eligible") if disable_mfa: - user.mfa_enabled = False + disable_totp_mfa(user) row.assurance = AssuranceLevel.BASIC.value db.session.commit() log_security_event( diff --git a/app/totp_routes.py b/app/totp_routes.py index 7035321..240ed81 100644 --- a/app/totp_routes.py +++ b/app/totp_routes.py @@ -24,6 +24,7 @@ TOTPEnrollmentError, activate_totp_enrollment, begin_totp_enrollment, + disable_totp_mfa, verify_totp, ) from .step_up import ( @@ -228,7 +229,7 @@ def disable_mfa(): if grant_error is not None: return grant_error user = db.session.get(User, current_user.id) - user.mfa_enabled = False + disable_totp_mfa(user) db.session.commit() log_security_event("MFA_DISABLED", user=user.username) return jsonify({"ok": True}) diff --git a/app/totp_service.py b/app/totp_service.py index 7c5aae8..48f33a2 100644 --- a/app/totp_service.py +++ b/app/totp_service.py @@ -31,6 +31,14 @@ class TOTPEnrollmentError(RuntimeError): """The enrollment token, binding, or first code was invalid.""" +def disable_totp_mfa(user): + """Remove every TOTP factor and disable MFA without committing.""" + TOTPAuthenticator.query.filter_by( + user_id=user.id, + ).delete(synchronize_session=False) + user.mfa_enabled = False + + @dataclass(frozen=True) class EnrollmentView: token: str diff --git a/tests/test_recovery_service.py b/tests/test_recovery_service.py index 3750bd2..bd9a584 100644 --- a/tests/test_recovery_service.py +++ b/tests/test_recovery_service.py @@ -191,10 +191,26 @@ def test_recovery_code_cannot_be_replayed_after_restricted_logout(app, client): def test_explicit_mfa_disable_releases_restricted_session(app, client): - from app.models import User, db + from app.models import TOTPAuthenticator, User, db user_id = _create_user(app, "disable_recovered_mfa") code = _enable_recovery_mfa(app, user_id, count=1)[0] + with app.app_context(): + db.session.add_all(( + TOTPAuthenticator( + user_id=user_id, + encrypted_secret=b"active-recovery-totp-secret", + label="Phone", + active=True, + ), + TOTPAuthenticator( + user_id=user_id, + encrypted_secret=b"inactive-recovery-totp-secret", + label="Retired phone", + active=False, + ), + )) + db.session.commit() _login(client, "disable_recovered_mfa", expected=200) assert client.post("/api/auth/recovery", json={"code": code}).status_code == 200 @@ -212,6 +228,7 @@ def test_explicit_mfa_disable_releases_restricted_session(app, client): assert client.get("/").status_code == 200 with app.app_context(): assert db.session.get(User, user_id).mfa_enabled is False + assert TOTPAuthenticator.query.filter_by(user_id=user_id).count() == 0 def test_verified_totp_replacement_releases_restricted_session( diff --git a/tests/test_totp_routes.py b/tests/test_totp_routes.py index 1a680b7..0597419 100644 --- a/tests/test_totp_routes.py +++ b/tests/test_totp_routes.py @@ -236,7 +236,7 @@ def test_mfa_disable_is_explicit_and_recently_reauthenticated( client, monkeypatch, ): - from app.models import User, db + from app.models import TOTPAuthenticator, User, db user_id = _create_user(app, "disable_totp_user") _activate_totp_feature(app, monkeypatch) @@ -259,6 +259,14 @@ def test_mfa_disable_is_explicit_and_recently_reauthenticated( "confirm_enable_mfa": True, }, ).status_code == 200 + with app.app_context(): + db.session.add(TOTPAuthenticator( + user_id=user_id, + encrypted_secret=b"inactive-totp-secret", + label="Retired phone", + active=False, + )) + db.session.commit() rejected = client.post( "/api/totp/disable", @@ -288,6 +296,7 @@ def test_mfa_disable_is_explicit_and_recently_reauthenticated( assert accepted.status_code == 200 with app.app_context(): assert db.session.get(User, user_id).mfa_enabled is False + assert TOTPAuthenticator.query.filter_by(user_id=user_id).count() == 0 def test_totp_enrollment_verification_is_rate_limited_before_code_check(