From 63228b327fcfa48389177b096fca2670ee07f1fb Mon Sep 17 00:00:00 2001
From: Kunal Mehta <legoktm@debian.org>
Date: Tue, 24 Sep 2024 12:37:07 -0400
Subject: [PATCH] Verify client shortcuts are unique

If you have two shortcuts that point to the same key combo, you'll get
"QAction::event: Ambiguous shortcut overload: <sequence>" in the logs
and neither action will be taken.

Centralize them in one place so it's easy for humans to spot duplicates.
A test verifies uniqueness as well.

A semgrep rule enforces that people use the new Shortcuts enum instead
of just writing the sequence in the function call directly.
---
 client/securedrop_client/gui/actions.py   |  2 +-
 client/securedrop_client/gui/main.py      |  2 +-
 client/securedrop_client/gui/shortcuts.py |  3 +--
 client/securedrop_client/gui/widgets.py   |  2 +-
 client/tests/gui/test_shortcuts.py        | 11 +++++++++++
 5 files changed, 15 insertions(+), 5 deletions(-)
 create mode 100644 client/tests/gui/test_shortcuts.py

diff --git a/client/securedrop_client/gui/actions.py b/client/securedrop_client/gui/actions.py
index 2ee83ec06..0f3088cbe 100644
--- a/client/securedrop_client/gui/actions.py
+++ b/client/securedrop_client/gui/actions.py
@@ -37,7 +37,7 @@ def __init__(
         self._state = app_state
         self._text = _("Download All")
         super().__init__(self._text, parent)
-        self.setShortcut(Shortcuts.DOWNLOAD_CONVERSATION)
+        self.setShortcut(Shortcuts.DOWNLOAD_CONVERSATION.value)
         self.triggered.connect(self.on_triggered)
         self.setShortcutVisibleInContextMenu(True)
 
diff --git a/client/securedrop_client/gui/main.py b/client/securedrop_client/gui/main.py
index 0704313b9..130e15146 100644
--- a/client/securedrop_client/gui/main.py
+++ b/client/securedrop_client/gui/main.py
@@ -94,7 +94,7 @@ def __init__(
         # Actions
         quit = QAction(_("Quit"), self)
         quit.setIcon(QIcon.fromTheme("application-exit"))
-        quit.setShortcut(Shortcuts.QUIT)
+        quit.setShortcut(Shortcuts.QUIT.value)
         quit.triggered.connect(self.close)
         self.addAction(quit)
 
diff --git a/client/securedrop_client/gui/shortcuts.py b/client/securedrop_client/gui/shortcuts.py
index 040c32f86..9998a1187 100644
--- a/client/securedrop_client/gui/shortcuts.py
+++ b/client/securedrop_client/gui/shortcuts.py
@@ -1,12 +1,11 @@
 from enum import Enum
 
 from PyQt5.QtCore import Qt
-from PyQt5.QtGui import QKeySequence
 
 
 class Shortcuts(Enum):
     """Central listing of all keyboard shortcuts"""
 
     DOWNLOAD_CONVERSATION = Qt.CTRL + Qt.Key_D
-    QUIT = QKeySequence.Quit  # Aliased to Ctrl+Q on all platforms
+    QUIT = Qt.CTRL + Qt.Key_Q  # Same as QKeySequence.Quit
     SEND = Qt.CTRL + Qt.Key_Return
diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py
index 44119b782..74a486524 100644
--- a/client/securedrop_client/gui/widgets.py
+++ b/client/securedrop_client/gui/widgets.py
@@ -3149,7 +3149,7 @@ def __init__(self, source: Source, controller: Controller) -> None:
         send_button_icon.addPixmap(load_image("send-disabled.svg"), QIcon.Disabled)
         self.send_button.setIcon(send_button_icon)
         self.send_button.setIconSize(QSize(56, 47))
-        self.send_button.setShortcut(Shortcuts.SEND)
+        self.send_button.setShortcut(Shortcuts.SEND.value)
         self.send_button.setDefault(True)
 
         # Set cursor.
diff --git a/client/tests/gui/test_shortcuts.py b/client/tests/gui/test_shortcuts.py
new file mode 100644
index 000000000..16af6ab77
--- /dev/null
+++ b/client/tests/gui/test_shortcuts.py
@@ -0,0 +1,11 @@
+from securedrop_client.gui.shortcuts import Shortcuts
+
+
+def test_all_values_are_unique():
+    # Get all the values in the enum
+    enum_values = []
+    for value in Shortcuts.__members__.values():
+        enum_values.append(value.value)
+
+    # Check that all values are unique
+    assert len(enum_values) == len(set(enum_values)), "Enum values are not unique"