UI freeze exception for usb-creator 0.2.7

Adding PK support: that sounds like a very intrusive change, since that usually means to split the entire program into a d-bus backend (running as root), and a GUI, communicating over d-bus and PK-protected functions to the backend.

Or does it just call existing d-bus backends like devicekit-disks? Then you shouldn't need any client-side policykit code?

How intrusive is this change?

The rest looks fine, I also subscribed ubuntu-docs.

It creates a small helper process (bin/usb-creator-helper in lp:usb-creator) to do the work. It's just under 150 lines without comments or blank lines.

As the code was already split into separate backends and frontends (usb-creator has a windows backend, and windows, kde, and gtk frontends), this was just a matter of moving the relevant code into a separate process and communicating with that over dbus.

I'm reviewing

  http://bazaar.launchpad.net/%7Eusb-creator-hackers/usb-creator/trunk/revision/201

for this.

The default PK policy for the "bootloader", "format", etc. methods is "yes", i. e. it will silently do that without authentication dialogs. The D-BUS methods allow you to specify an arbitrary device, and call syslinux/devicekit without further checks. Since the usb-creator backend runs as root, this circumvents the existing dk-disks checks. Apps shouldn't maliciously or accidentally be able to reformat/change internal disks.

Either you need to call the dk-disks stuff as normal user from the client side, and thus get the existing DK-disks policy checks (which might be a bit tricky since you also need to call syslinux etc.), or you need to mirror DK-disks' checks for removable devices, or you need to change the default policy to auth_admin_keep.

Otherwise the changes are primarily moving existing code around into the new d-bus wrapper.

If the policy checks get fixed, this looks fine for me, since it also helps to get rid of gksu.

Martin,

First, thanks for this review.

The bootloader, format, and image methods first call into a check_system_internal function, which checks the device-is-system-internal property in dk-disks to make sure that the device in question is *not* a system-internal device (bzr rev 170.1.2, part of the r201 merge). If it is system internal, it raises an exception.

So I believe the code already addresses the concern you raise. Do you disagree?

Ah, I missed that. Looks fine then, please go ahead.

This bug was fixed in the package usb-creator - 0.2.7

---------------
usb-creator (0.2.7) karmic; urgency=low

  [ Evan Dandrea ]
  * Add PolicyKit support (LP: #273483).
  * Move logging back to the home directory, now that usb-creator is run as a
    regular user (LP: #431266).
  * Use GIO instead of gnomevfs. Only lookup GNOME device names and
    icons as needed.
  * Remove the device in the DeviceKit-disks backend when it's removed
    from the system.
  * Only set the non-size columns to expand to fill available space in the GTK+
    frontend. Set a minimum width of 75px for all columns.
  * Add the missing retry dialog to the GTK+ frontend.
  * Fix a deadlock when the failed dialog runs.
  * Explicitly depend on mtools, just in case someone removes it and
    expects usb-creator to still work (LP: #295212).
  * Re-enable the format button now that Devicekit-disks 007 has been
    released.
  * Depend on DeviceKit-disks >= 007.
  * Freeze exception (LP: #432542).
  * Update translations from Launchpad.

  [ Roderick B. Greening ]
  * Bump version in setup.py, kde_about.py, usb-creator-gtk, and man to 0.2.7
  * Remove completed TODO items
  * Update some message strings for translations
  * Add the missing retry dialog to the KDE frontend.
  * Update man pages to reflect new log file location
  * In devicekit backend, ensure mount is empty string '' rather than empty
    dbus.Array to prevent crashes in os.statvfs from misc.py

 -- Evan Dandrea <email address hidden> Thu, 24 Sep 2009 10:02:28 -0700