Solves #9674 #643
Solves #9674 #643
Conversation
| self.fire_event_for_permission() | ||
|
|
||
| denied = self.dest.devices_denied | ||
| return "\n".join(map(repr, DeviceInterface.from_str_bulk(denied))) |
There was a problem hiding this comment.
Maybe assert that there are no newlines in the repr output?
qubes/device_protocol.py
Outdated
| @staticmethod | ||
| def from_str_bulk(interfaces: Optional[str]) -> List["DeviceInterface"]: | ||
| interfaces = interfaces or [] | ||
| return [ | ||
| DeviceInterface(interfaces[i: i + 7]) | ||
| for i in range(0, len(interfaces), 7) | ||
| ] |
There was a problem hiding this comment.
I suggest raising a QubesValueError if the size is not a multiple of 8 or if there are invalid characters in the interface name.
qubes/device_protocol.py
Outdated
| @@ -733,6 +733,14 @@ def unknown(cls) -> "DeviceInterface": | |||
| """Value for unknown device interface.""" | |||
| return cls("?******") | |||
|
|
|||
| @staticmethod | |||
| def from_str_bulk(interfaces: Optional[str]) -> List["DeviceInterface"]: | |||
| interfaces = interfaces or [] | |||
There was a problem hiding this comment.
| interfaces = interfaces or [] | |
| interfaces = interfaces or "" |
Fix typing error.
qubes/vm/qubesvm.py
Outdated
| self, prop, value, | ||
| "Interface code list contains duplicates.") | ||
| # block, usb, mic, pci, * | ||
| pattern = r"^([bump\*][0123456789\*]{6})*$" |
There was a problem hiding this comment.
What about hex numbers in interfaces?
qubes/api/admin.py
Outdated
| Payload: | ||
| Encoded device interface (can be repeated without any separator). | ||
| If payload is empty, all interfaces are removed. |
There was a problem hiding this comment.
This invites mistakes... maybe better some magic value like all?
| Add device interface(s) to the denied list for the VM. | ||
| Payload: | ||
| Encoded device interface (can be repeated without any separator). |
There was a problem hiding this comment.
What is intended behavior on duplicates? IIUC right now if at least one interface you try to add is already on deny list, the whole operation will fail. Is that intentional? IMHO better would be to ignore duplicates (as in - adding interface that is already denied is no-op, but others non-duplicated added at the same time are still added). Excluding duplicates at the client side is racy (somebody can modify the value between client reads it (to exclude duplicates) and writes it back).
There was a problem hiding this comment.
Yes, this was intentional; I followed the "fail fast" principle. However, now I understand that in the mentioned example, admin methods should be idempotent.
To keep consistency, in the case of removing an interface that is not currently on the list nothing should be done, am I right?
+ tests for empty payloads
qubes/device_protocol.py
Outdated
| def from_str_bulk(interfaces: Optional[str]) -> List["DeviceInterface"]: | ||
| interfaces = interfaces or "" | ||
| if len(interfaces) % 7 != 0: | ||
| QubesValueError( |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #643 +/- ##
==========================================
+ Coverage 69.50% 69.71% +0.20%
==========================================
Files 58 58
Lines 12468 12498 +30
==========================================
+ Hits 8666 8713 +47
+ Misses 3802 3785 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
As previously - can you fold fixes (especially the exception one) into relevant commits? |
Delete the file and drop-in folder for the denied device interfaces list and move the list to
qvm-prefs.Solves QubesOS/qubes-issues/issues/9674