all: implement code review comments

core/src/apps/management/apply_settings.py

@@ -77,7 +77,7 @@ async def require_confirm_change_passphrase_source(
     text = Text("Passphrase source", ui.ICON_CONFIG)
     if passphrase_always_on_device:
         text.normal(
-            "Do you really want to", "entry passphrase always", "on the device?"
+            "Do you really want to", "enter passphrase always", "on the device?"
         )
     else:
         text.normal("Do you want to revoke", "the passphrase on device", "setting?")

core/src/storage/device.py

@@ -144,16 +144,13 @@ def no_backup() -> bool:
 
 
 def get_passphrase_always_on_device() -> bool:
-    b = common.get(_NAMESPACE, _PASSPHRASE_ALWAYS_ON_DEVICE)
+    """
-    # backwards compatible for _PASSPHRASE_SOURCE.HOST = 2
+    This is backwards compatible with _PASSPHRASE_SOURCE:
-    if b == b"\x02":
+    - If ASK(0) => returns False, the check against b"\x01" in get_bool fails.
-        return False
+    - If DEVICE(1) => returns True, the check against b"\x01" in get_bool succeeds.
-    # backwards compatible for _PASSPHRASE_SOURCE.DEVICE = 1
+    - If HOST(2) => returns False, the check against b"\x01" in get_bool fails.
-    # and also \x01 is TRUE_BYTE so it is future compatible as well
+    """
-    elif b == b"\x01":
+    return common.get_bool(_NAMESPACE, _PASSPHRASE_ALWAYS_ON_DEVICE)
-        return True
-    else:
-        return False
 
 
 def load_settings(

python/src/trezorlib/cli/settings.py

@@ -139,23 +139,12 @@ def passphrase():
 
 
 @passphrase.command(name="enabled")
-@click.option("-f", "--force-on-device", is_flag=True)
+@click.option("-f/-F", "--force-on-device/--no-force-on-device", default=None)
-@click.option("-F", "--no-force-on-device", is_flag=True)
 @click.pass_obj
-def passphrase_enable(connect, force_on_device: bool, no_force_on_device: bool):
+def passphrase_enable(connect, force_on_device: bool):
     """Enable passphrase."""
-    if force_on_device and no_force_on_device:
-        raise ValueError(
-            "Only one option of --force-on-device/-no-force-on-device makes sense."
-        )
-    on_device = None
-    if force_on_device:
-        on_device = True
-    if no_force_on_device:
-        on_device = False
-
     return device.apply_settings(
-        connect(), use_passphrase=True, passphrase_always_on_device=on_device
+        connect(), use_passphrase=True, passphrase_always_on_device=force_on_device
     )
 
 

python/src/trezorlib/cli/trezorctl.py

@@ -141,11 +141,7 @@ def configure_logging(verbose: int):
     "-j", "--json", "is_json", is_flag=True, help="Print result as JSON object"
 )
 @click.option(
-    "-P",
+    "-P", "--passphrase-on-host", is_flag=True, help="Enter passphrase on host.",
-    "--passphrase-on-host",
-    "passphrase_on_host",
-    is_flag=True,
-    help="Enter passphrase on host.",
 )
 @click.version_option()
 @click.pass_context

tests/device_tests/test_protection_levels.py

@@ -63,7 +63,6 @@ class TestProtectionLevels:
             )
             device.change_pin(client)
 
-    @pytest.mark.setup_client()
     def test_ping(self, client):
         with client:
             client.set_expected_responses([proto.ButtonRequest(), proto.Success()])

tests/device_tests/test_session_id_and_passphrase.py

@@ -17,10 +17,19 @@
 import pytest
 
 from trezorlib import messages
+from trezorlib.messages import FailureType
 from trezorlib.tools import parse_path
 
+XPUB_PASSPHRASE_A = "xpub6CekxGcnqnJ6osfY4Rrq7W5ogFtR54KUvz4H16XzaQuukMFZCGebEpVznfq4yFcKEmYyShwj2UKjL7CazuNSuhdkofF4mHabHkLxCMVvsqG"
+XPUB_PASSPHRASE_NONE = "xpub6BiVtCpG9fQPxnPmHXG8PhtzQdWC2Su4qWu6XW9tpWFYhxydCLJGrWBJZ5H6qTAHdPQ7pQhtpjiYZVZARo14qHiay2fvrX996oEP42u8wZy"
+
 
 def _get_xpub(client, passphrase):
+    """
+    Are calls are intentionally "raw", i.e. we are not using trezorlib
+    to avoid its implicit session management
+    (clearing session after settings, hidden handling of session ids, etc.).
+    """
     response = client.call_raw(
         messages.GetPublicKey(address_n=parse_path("44'/0'/0'"), coin_name="Bitcoin")
     )
@@ -40,7 +49,6 @@ def _enable_passphrase(client):
 
 
 @pytest.mark.skip_ui
-@pytest.mark.setup_client()
 def test_session_with_passphrase(client):
     # Turn on passphrase.
     _enable_passphrase(client)
@@ -54,10 +62,7 @@ def test_session_with_passphrase(client):
     # GetPublicKey requires passphrase and since it is not cached,
     # Trezor will prompt for it.
     xpub = _get_xpub(client, passphrase="A")
-    assert (
+    assert xpub == XPUB_PASSPHRASE_A
-        xpub
-        == "xpub6CekxGcnqnJ6osfY4Rrq7W5ogFtR54KUvz4H16XzaQuukMFZCGebEpVznfq4yFcKEmYyShwj2UKjL7CazuNSuhdkofF4mHabHkLxCMVvsqG"
-    )
 
     # Call Initialize again, this time with the received session id and then call
     # GetPublicKey. The passphrase should be cached now so Trezor must
@@ -65,33 +70,23 @@ def test_session_with_passphrase(client):
     response = client.call_raw(messages.Initialize(session_id=session_id))
     assert isinstance(response, messages.Features)
     xpub = _get_xpub(client, passphrase=None)
-    assert (
+    assert xpub == XPUB_PASSPHRASE_A
-        xpub
-        == "xpub6CekxGcnqnJ6osfY4Rrq7W5ogFtR54KUvz4H16XzaQuukMFZCGebEpVznfq4yFcKEmYyShwj2UKjL7CazuNSuhdkofF4mHabHkLxCMVvsqG"
-    )
 
     # If we set session id in Initialize to None, the cache will be cleared
     # and Trezor will ask for the passphrase again.
     response = client.call_raw(messages.Initialize(session_id=None))
     assert isinstance(response, messages.Features)
     xpub = _get_xpub(client, passphrase="A")
-    assert (
+    assert xpub == XPUB_PASSPHRASE_A
-        xpub
-        == "xpub6CekxGcnqnJ6osfY4Rrq7W5ogFtR54KUvz4H16XzaQuukMFZCGebEpVznfq4yFcKEmYyShwj2UKjL7CazuNSuhdkofF4mHabHkLxCMVvsqG"
-    )
 
     # Unknown session id is the same as setting it to None.
     response = client.call_raw(messages.Initialize(session_id=b"X" * 32))
     assert isinstance(response, messages.Features)
     xpub = _get_xpub(client, passphrase="A")
-    assert (
+    assert xpub == XPUB_PASSPHRASE_A
-        xpub
-        == "xpub6CekxGcnqnJ6osfY4Rrq7W5ogFtR54KUvz4H16XzaQuukMFZCGebEpVznfq4yFcKEmYyShwj2UKjL7CazuNSuhdkofF4mHabHkLxCMVvsqG"
-    )
 
 
 @pytest.mark.skip_ui
-@pytest.mark.setup_client()
 def test_session_enable_passphrase(client):
     # Let's start the communication by calling Initialize.
     response = client.call_raw(messages.Initialize())
@@ -101,10 +96,7 @@ def test_session_enable_passphrase(client):
 
     # Trezor will not prompt for passphrase because it is turned off.
     xpub = _get_xpub(client, passphrase=None)
-    assert (
+    assert xpub == XPUB_PASSPHRASE_NONE
-        xpub
-        == "xpub6BiVtCpG9fQPxnPmHXG8PhtzQdWC2Su4qWu6XW9tpWFYhxydCLJGrWBJZ5H6qTAHdPQ7pQhtpjiYZVZARo14qHiay2fvrX996oEP42u8wZy"
-    )
 
     # Turn on passphrase.
     _enable_passphrase(client)
@@ -114,25 +106,18 @@ def test_session_enable_passphrase(client):
     xpub = _get_xpub(client, passphrase=None)
     assert isinstance(response, messages.Features)
     assert session_id == response.session_id
-    assert (
+    assert xpub == XPUB_PASSPHRASE_NONE
-        xpub
-        == "xpub6BiVtCpG9fQPxnPmHXG8PhtzQdWC2Su4qWu6XW9tpWFYhxydCLJGrWBJZ5H6qTAHdPQ7pQhtpjiYZVZARo14qHiay2fvrX996oEP42u8wZy"
-    )
 
     # We clear the session id now, so the passphrase should be asked.
     response = client.call_raw(messages.Initialize())
     xpub = _get_xpub(client, passphrase="A")
     assert isinstance(response, messages.Features)
     assert session_id != response.session_id
-    assert (
+    assert xpub == XPUB_PASSPHRASE_A
-        xpub
-        == "xpub6CekxGcnqnJ6osfY4Rrq7W5ogFtR54KUvz4H16XzaQuukMFZCGebEpVznfq4yFcKEmYyShwj2UKjL7CazuNSuhdkofF4mHabHkLxCMVvsqG"
-    )
 
 
 @pytest.mark.skip_ui
 @pytest.mark.skip_t1
-@pytest.mark.setup_client()
 def test_passphrase_always_on_device(client):
     # Let's start the communication by calling Initialize.
     response = client.call_raw(messages.Initialize())
@@ -158,10 +143,7 @@ def test_passphrase_always_on_device(client):
     client.debug.input("")  # Input empty passphrase.
     response = client.call_raw(messages.ButtonAck())
     assert isinstance(response, messages.PublicKey)
-    assert (
+    assert response.xpub == XPUB_PASSPHRASE_NONE
-        response.xpub
-        == "xpub6BiVtCpG9fQPxnPmHXG8PhtzQdWC2Su4qWu6XW9tpWFYhxydCLJGrWBJZ5H6qTAHdPQ7pQhtpjiYZVZARo14qHiay2fvrX996oEP42u8wZy"
-    )
 
     # Passphrase will not be prompted. The session id stays the same and the passphrase is cached.
     response = client.call_raw(messages.Initialize(session_id=session_id))
@@ -170,10 +152,7 @@ def test_passphrase_always_on_device(client):
         messages.GetPublicKey(address_n=parse_path("44'/0'/0'"), coin_name="Bitcoin")
     )
     assert isinstance(response, messages.PublicKey)
-    assert (
+    assert response.xpub == XPUB_PASSPHRASE_NONE
-        response.xpub
-        == "xpub6BiVtCpG9fQPxnPmHXG8PhtzQdWC2Su4qWu6XW9tpWFYhxydCLJGrWBJZ5H6qTAHdPQ7pQhtpjiYZVZARo14qHiay2fvrX996oEP42u8wZy"
-    )
 
     # In case we want to add a new passphrase we need to send session_id = None.
     response = client.call_raw(messages.Initialize(session_id=None))
@@ -185,15 +164,11 @@ def test_passphrase_always_on_device(client):
     client.debug.input("A")  # Input empty passphrase.
     response = client.call_raw(messages.ButtonAck())
     assert isinstance(response, messages.PublicKey)
-    assert (
+    assert response.xpub == XPUB_PASSPHRASE_A
-        response.xpub
-        == "xpub6CekxGcnqnJ6osfY4Rrq7W5ogFtR54KUvz4H16XzaQuukMFZCGebEpVznfq4yFcKEmYyShwj2UKjL7CazuNSuhdkofF4mHabHkLxCMVvsqG"
-    )
 
 
 @pytest.mark.skip_ui
 @pytest.mark.skip_t2
-@pytest.mark.setup_client()
 def test_passphrase_on_device_not_possible_on_t1(client):
     # Let's start the communication by calling Initialize.
     response = client.call_raw(messages.Initialize())
@@ -206,7 +181,7 @@ def test_passphrase_on_device_not_possible_on_t1(client):
     response = client.call_raw(messages.ApplySettings(passphrase_always_on_device=True))
 
     assert isinstance(response, messages.Failure)
-    assert response.code == 3  # DataError
+    assert response.code == FailureType.DataError
 
     response = client.call_raw(
         messages.GetPublicKey(address_n=parse_path("44'/0'/0'"), coin_name="Bitcoin")
@@ -214,7 +189,7 @@ def test_passphrase_on_device_not_possible_on_t1(client):
     assert isinstance(response, messages.PassphraseRequest)
     response = client.call_raw(messages.PassphraseAck(on_device=True))
     assert isinstance(response, messages.Failure)
-    assert response.code == 3  # DataError
+    assert response.code == FailureType.DataError
 
 
 @pytest.mark.skip_ui
@@ -226,7 +201,7 @@ def test_passphrase_ack_mismatch(client):
     assert isinstance(response, messages.PassphraseRequest)
     response = client.call_raw(messages.PassphraseAck(passphrase="A", on_device=True))
     assert isinstance(response, messages.Failure)
-    assert response.code == 3  # DataError
+    assert response.code == FailureType.DataError
 
 
 @pytest.mark.skip_ui
@@ -238,4 +213,12 @@ def test_passphrase_missing(client):
     assert isinstance(response, messages.PassphraseRequest)
     response = client.call_raw(messages.PassphraseAck(passphrase=None))
     assert isinstance(response, messages.Failure)
-    assert response.code == 3  # DataError
+    assert response.code == FailureType.DataError
+
+    response = client.call_raw(
+        messages.GetPublicKey(address_n=parse_path("44'/0'/0'"), coin_name="Bitcoin")
+    )
+    assert isinstance(response, messages.PassphraseRequest)
+    response = client.call_raw(messages.PassphraseAck(passphrase=None, on_device=False))
+    assert isinstance(response, messages.Failure)
+    assert response.code == FailureType.DataError