← Watch feed
wallet: enable/disable_keystore: rm functionality from 2fa wallets
Public commit record
65/100 · Adequate
What the developer wrote
Authored by SomberNight
wallet: enable/disable_keystore: rm functionality from 2fa wallets
was already not working, but does not even really make sense without larger changes
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides an explanatory body
What changed, and why it matters
This commit removes the ability to enable or disable individual keystores in Electrum's two-factor authentication (2FA) wallets. The feature was already broken for these wallets, and the change hides the buttons in the user interface and makes the underlying methods raise errors if called. It is a hardening/cleanup change rather than a fix for an active security vulnerability.
Recommended action
No urgent action required. Reviewers may want to confirm that the new guard and assertions prevent any inconsistent wallet states, and that the removed 2FA functionality is not needed by users.
Security signals we found
01
Feature removal for a wallet type where it was non-functional
02
Addition of guard method to prevent unsupported keystore state changes
03
Addition of assertions validating keystore membership and absence of Lightning channels before disabling
04
UI now conditionally hides keystore enable/disable buttons based on wallet capability
Evidence from the diff
The patch adds a can_enable_disable_keystore() guard to Abstract_Wallet (default False), overrides it to True for Deterministic_Wallet, and overrides it to False for Wallet_2fa in the trustedcoin plugin. The Qt wallet info dialog now only shows ‘Disable/Enable keystore’ buttons when the guard returns True. The 2FA wallet’s enable_keystore() and disable_keystore() methods now raise exceptions. Additional assertions in Deterministic_Wallet check that the keystore belongs to the wallet and that no Lightning channels exist before disabling. Tests are updated to assert the new behavior.
Changed components
electrum/wallet.pyelectrum/plugins/trustedcoin/trustedcoin.pyelectrum/gui/qt/wallet_info_dialog.pytests/test_wizard.pyInspect captured patch +51 / −12
diff --git a/electrum/gui/qt/wallet_info_dialog.py b/electrum/gui/qt/wallet_info_dialog.py
index 9ab87a5..b194f07 100644
--- a/electrum/gui/qt/wallet_info_dialog.py
+++ b/electrum/gui/qt/wallet_info_dialog.py
@@ -150,16 +150,17 @@ class WalletInfoDialog(WindowModalDialog):
bip32fp_hbox.addWidget(bip32fp_text)
bip32fp_hbox.addStretch()
ks_vbox.addLayout(bip32fp_hbox)
- ks_buttons = []
- if not ks.is_watching_only():
- rm_keystore_button = QPushButton('Disable keystore')
- rm_keystore_button.clicked.connect(partial(self.disable_keystore, ks))
- ks_buttons.insert(0, rm_keystore_button)
- else:
- add_keystore_button = QPushButton('Enable Keystore')
- add_keystore_button.clicked.connect(self.enable_keystore)
- ks_buttons.insert(0, add_keystore_button)
- ks_vbox.addLayout(Buttons(*ks_buttons))
+ if wallet.can_enable_disable_keystore(ks):
+ ks_buttons = []
+ if not ks.is_watching_only():
+ rm_keystore_button = QPushButton('Disable keystore')
+ rm_keystore_button.clicked.connect(partial(self.disable_keystore, ks))
+ ks_buttons.insert(0, rm_keystore_button)
+ else:
+ add_keystore_button = QPushButton('Enable Keystore')
+ add_keystore_button.clicked.connect(self.enable_keystore)
+ ks_buttons.insert(0, add_keystore_button)
+ ks_vbox.addLayout(Buttons(*ks_buttons))
tab_label = _("Cosigner") + f' {idx+1}' if len(keystores) > 1 else _("Keystore")
index = self.keystore_tabs.addTab(ks_w, tab_label)
if not ks.is_watching_only():
diff --git a/electrum/plugins/trustedcoin/trustedcoin.py b/electrum/plugins/trustedcoin/trustedcoin.py
index db1605c..41e90c9 100644
--- a/electrum/plugins/trustedcoin/trustedcoin.py
+++ b/electrum/plugins/trustedcoin/trustedcoin.py
@@ -45,6 +45,7 @@ from electrum.plugin import BasePlugin, hook
from electrum.util import NotEnoughFunds, UserFacingException, error_text_str_to_safe_str
from electrum.network import Network
from electrum.logging import Logger
+from electrum.keystore import KeyStore
if TYPE_CHECKING:
from electrum.wizard import NewWalletWizard
@@ -382,6 +383,15 @@ class Wallet_2fa(Multisig_Wallet):
def is_billing_address(self, addr: str) -> bool:
return addr in self._billing_addresses_set
+ def can_enable_disable_keystore(self, ks: KeyStore) -> bool:
+ return False
+
+ def enable_keystore(self, keystore, is_hardware_keystore, password):
+ raise Exception("2fa wallet cannot enable keystore")
+
+ def disable_keystore(self, keystore):
+ raise Exception("2fa wallet cannot disable keystore")
+
# Utility functions
diff --git a/electrum/wallet.py b/electrum/wallet.py
index fac15d6..9a03dc6 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -3261,6 +3261,12 @@ class Abstract_Wallet(ABC, Logger, EventListener):
def save_keystore(self):
pass
+ def can_enable_disable_keystore(self, ks: KeyStore) -> bool:
+ """Whether the wallet is capable of disabling/enabling the given keystore.
+ This is a necessary but not sufficient check: e.g. if wallet has LN channels, we should not allow disabling.
+ """
+ return False
+
def enable_keystore(self, keystore: KeyStore, is_hardware_keystore: bool, password) -> None:
raise NotImplementedError()
@@ -4039,14 +4045,20 @@ class Deterministic_Wallet(Abstract_Wallet):
def get_txin_type(self, address=None):
return self.txin_type
+ def can_enable_disable_keystore(self, ks: KeyStore) -> bool:
+ return True
+
def enable_keystore(self, keystore: KeyStore, is_hardware_keystore: bool, password) -> None:
+ assert self.can_enable_disable_keystore(keystore)
if not is_hardware_keystore and self.storage.is_encrypted_with_user_pw():
keystore.update_password(None, password)
self.db.put('use_encryption', True)
self._update_keystore(keystore)
def disable_keystore(self, keystore: KeyStore) -> None:
+ assert self.can_enable_disable_keystore(keystore)
assert not self.has_channels()
+ assert keystore in self.get_keystores()
if hasattr(keystore, 'thread') and keystore.thread:
keystore.thread.stop()
if self.storage.is_encrypted_with_hw_device():
diff --git a/tests/test_wizard.py b/tests/test_wizard.py
index 96e9d66..6ceb1df 100644
--- a/tests/test_wizard.py
+++ b/tests/test_wizard.py
@@ -126,7 +126,6 @@ class ServerConnectWizardTestCase(WizardTestCase):
class KeystoreWizardTestCase(WizardTestCase):
# TODO add test cases for:
# - multisig
- # - 2fa
class TKeystoreWizard(KeystoreWizard):
def is_single_password(self):
@@ -194,6 +193,7 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet = self._create_xpub_keystore_wallet(xpub=myxpub)
self.assertTrue(wallet.get_keystore().is_watching_only())
+ self.assertTrue(wallet.can_enable_disable_keystore(ks))
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
self.assertEqual(myseed, wallet.get_keystore().get_seed(None))
@@ -223,6 +223,7 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet = self._create_xpub_keystore_wallet(xpub=myxpub)
self.assertTrue(wallet.get_keystore().is_watching_only())
+ self.assertTrue(wallet.can_enable_disable_keystore(ks))
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
self.assertEqual(myseed, wallet.get_keystore().get_seed(None))
@@ -249,6 +250,7 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet = self._create_xpub_keystore_wallet(xpub=myxpub)
self.assertTrue(wallet.get_keystore().is_watching_only())
+ self.assertTrue(wallet.can_enable_disable_keystore(ks))
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
self.assertEqual(myseed, wallet.get_keystore().get_seed(None))
@@ -275,6 +277,7 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet = self._create_xpub_keystore_wallet(xpub=myxpub)
self.assertTrue(wallet.get_keystore().is_watching_only())
+ self.assertTrue(wallet.can_enable_disable_keystore(ks))
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
@@ -303,6 +306,7 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet = self._create_xpub_keystore_wallet(xpub=myxpub)
self.assertTrue(wallet.get_keystore().is_watching_only())
+ self.assertTrue(wallet.can_enable_disable_keystore(ks))
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
@@ -342,6 +346,7 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet = self._create_xpub_keystore_wallet(xpub=myxpub)
self.assertTrue(wallet.get_keystore().is_watching_only())
+ self.assertTrue(wallet.can_enable_disable_keystore(ks))
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
self.assertTrue(isinstance(wallet.get_keystore(), Hardware_KeyStore))
@@ -701,7 +706,17 @@ class WalletWizardTestCase(WizardTestCase):
v = w.resolve_next(v.view, d)
self.assertEqual('trustedcoin_show_confirm_otp', v.view)
v = w.resolve_next(v.view, d)
- self._set_password_and_check_address(v=v, w=w, recv_addr="bc1qnf5qafvpx0afk47433j3tt30pqkxp5wa263m77wt0pvyqq67rmfs522m94")
+ wallet = self._set_password_and_check_address(v=v, w=w, recv_addr="bc1qnf5qafvpx0afk47433j3tt30pqkxp5wa263m77wt0pvyqq67rmfs522m94")
+
+ with self.subTest(msg="2fa wallet cannot enable/disable keystore"):
+ for ks in wallet.get_keystores():
+ self.assertFalse(wallet.can_enable_disable_keystore(ks))
+ with self.assertRaises(Exception) as ctx:
+ wallet.enable_keystore(ks, False, None)
+ self.assertTrue("2fa wallet cannot" in ctx.exception.args[0])
+ with self.assertRaises(Exception) as ctx:
+ wallet.enable_keystore(ks, False, None)
+ self.assertTrue("2fa wallet cannot" in ctx.exception.args[0])
async def test_2fa_haveseed_keep2FAenabled(self):
self.assertTrue(self.config.get('enable_plugin_trustedcoin'))
@@ -1054,3 +1069,4 @@ class WalletWizardTestCase(WizardTestCase):
set(wallet.get_receiving_addresses()),
{"bc1qq2tmmcngng78nllq2pvrkchcdukemtj56uyue0", "1LNvv5h6QHoYv1nJcqrp13T2TBkD2sUGn1", "1FJEEB8ihPMbzs2SkLmr37dHyRFzakqUmo"},
)
+ self.assertFalse(wallet.can_enable_disable_keystore(wallet.keystore))
Risk score
Our methodology →Why this scored 27/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.