Project

General

Profile

Feature #5623

Tails Installer should refuse upgrading a device that hasn't Tails installed

Added by Tails about 6 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
Installation
Target version:
Start date:
Due date:
% Done:

100%

Feature Branch:
kurono:feature/5623-Installer-should-refuse-empty-device
Type of work:
Code
Blueprint:
Starter:
Yes
Affected tool:
Installer

Description

Apparently, some users try to use the "Upgrade from ISO" option to install Tails on a blank USB stick partition, and are surprised the resulting stick does not boot. Similarly, Tails Installer doesn't prevent attempting "Clone & Upgrade" on a non-Tails device (at least if it has a GPT and a VFAT-formatted partition, and possibly in more cases).

Note that we want to add an "Install from ISO" option (#8865). But regardless, Tails Installer should prevent users from trying non-working "upgrade" scenarios.

The list of potential target devices should be filtered to only display those with an existing Tails installation, when the user has chosen an upgrade scenario. It's probably acceptable to only check for GPT + existence of a VFAT filesystem on a partition whose GPT label is "Tails", so we don't have to mount anything to check its content.

usb_install.png View (17.7 KB) kytv, 05/19/2015 10:56 AM


Related issues

Related to Tails - Bug #7659: "Upgrade from ISO" doesn't error on fresh disk Duplicate 07/25/2014
Related to Tails - Feature #8865: Implement "Install from ISO" in Tails Installer Duplicate 02/04/2015
Related to Tails - Bug #10539: "Clone and upgrade" on Jessie pretends my Tails was not installed with our Installer Resolved 11/12/2015
Duplicated by Tails - Bug #9275: Can "Clone & Upgrade" to a small device which does not have Tails installed Duplicate 04/23/2015
Duplicated by Tails - Feature #6054: Tails Installer should hide the USB stick that contains the ISO Duplicate

Associated revisions

Revision 3da0bfde (diff)
Added by intrigeri over 4 years ago

Add automated tests for trying to upgrade an empty device.

Refs: #5623

Revision 7543c7d6 (diff)
Added by intrigeri over 4 years ago

Adjust test suite wrt. changes in GUI behaviour when a device cannot be upgraded.

Refs: #5623

Revision d5c6915d (diff)
Added by intrigeri about 4 years ago

Add the feature-installer-for-1.5 APT overlay.

Fix-committed: #5623
Fix-committed: #9886

Revision c2fbb02b
Added by intrigeri about 4 years ago

Merge branch 'feature/installer-for-1.5' into devel

Fix-committed: #5623
Fix-committed: #9886

History

#1 Updated by intrigeri about 6 years ago

  • Starter set to Yes

#2 Updated by intrigeri about 6 years ago

  • Category set to Installation

#3 Updated by BitingBird over 5 years ago

  • Subject changed from liveusb-creator vs. empty partition upgrade to Liveusb-creator vs. empty partition upgrade

#4 Updated by BitingBird over 5 years ago

  • Subject changed from Liveusb-creator vs. empty partition upgrade to liveusb-creator vs. empty partition upgrade

#5 Updated by intrigeri almost 5 years ago

  • Related to Bug #7659: "Upgrade from ISO" doesn't error on fresh disk added

#6 Updated by BitingBird almost 5 years ago

  • Affected tool set to Installer

#7 Updated by intrigeri over 4 years ago

  • Subject changed from liveusb-creator vs. empty partition upgrade to Tails Installer should refuse upgrading a device that hasn't Tails installed
  • Description updated (diff)

#8 Updated by intrigeri over 4 years ago

  • Related to Feature #8865: Implement "Install from ISO" in Tails Installer added

#9 Updated by intrigeri over 4 years ago

  • Description updated (diff)

#10 Updated by intrigeri over 4 years ago

  • Assignee set to kurono

kurono, I've updated the description to match my current understanding of the problem and what should be done to address it. Do you want to take it, e.g. for 1.4.1 or 1.5?

#11 Updated by BitingBird over 4 years ago

  • Duplicated by Bug #9275: Can "Clone & Upgrade" to a small device which does not have Tails installed added

#12 Updated by kurono over 4 years ago

  • Target version set to Tails_1.4.1

#13 Updated by kurono over 4 years ago

  • Assignee deleted (kurono)
  • QA Check set to Ready for QA
  • Feature Branch set to feature/5623-Installer-should-refuse-empty-device

#14 Updated by intrigeri over 4 years ago

  • Assignee set to intrigeri

#15 Updated by intrigeri over 4 years ago

  • Feature Branch changed from feature/5623-Installer-should-refuse-empty-device to kurono:feature/5623-Installer-should-refuse-empty-device

#16 Updated by intrigeri over 4 years ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 20

#17 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to kurono
  • QA Check changed from Ready for QA to Dev Needed

Thanks! I didn't test it, but the code looks good.

Two suggestions:

  • I think the "GPT + vfat + partition label" test should live in a dedicated function. This would make the code more readable, and I bet we'll need to reuse it elsewhere when porting Tails Installer to regular Debian/Ubuntu.
  • I'm pretty sure that the "The selected device does not have a Tails installation, it is required for the upgrade process." new string is not up to our GUI standards => please ask sajolida or tails-ux@ for a better phrasing (you'll want to point them at this ticket, so that they understand in which case the string is shown, even if they don't read/understand the code).

#18 Updated by kurono over 4 years ago

Ok, I sent it to tails-ux@.

#19 Updated by kurono over 4 years ago

  • Assignee deleted (kurono)
  • QA Check changed from Dev Needed to Ready for QA

#20 Updated by intrigeri over 4 years ago

  • Assignee set to intrigeri

#21 Updated by intrigeri over 4 years ago

  • Feature Branch changed from kurono:feature/5623-Installer-should-refuse-empty-device to feature/5623-Installer-should-refuse-empty-device

Build and uploaded a package, the topic branch should have it now.

#22 Updated by intrigeri over 4 years ago

  • % Done changed from 20 to 30

... and code review looks as good as one can expect on this codebase.

#23 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to kytv
  • % Done changed from 30 to 50

Wrote a couple automated tests to ensure this works as designed. Kill Your TV, may you please run usb_install.feature (and all other features that are somehow affected by usb.rb) and review my tests? Then, once happy, please reassign to me for merging.

#24 Updated by kytv over 4 years ago

  • Assignee changed from kytv to intrigeri
  • QA Check changed from Ready for QA to Dev Needed

The tests look good (of course :), but due to what may be described as a regression in Tails Installer the following scenario fails:

  @keep_volumes
  Scenario: Try upgrading but end up installing Tails to a USB drive containing a Tails isohybrid installation # features/usb_install.feature:320
    Given a computer                                                                                           # features/step_definitions/common_steps.rb:66
    And I start Tails from DVD with network unplugged and I login                                              # features/step_definitions/common_steps.rb:154
    And I plug USB drive "isohybrid"                                                                           # features/step_definitions/common_steps.rb:92
    And I try a "Clone & Upgrade" Tails to USB drive "isohybrid"                                               # features/step_definitions/usb.rb:153
    But I am suggested to do a "Clone & Install"                                                               # features/step_definitions/usb.rb:179
      FindFailed: can not find USBSuggestsInstall.png on the screen.
      Line ?, in File ? (RuntimeError)
      features/usb_install.feature:325:in `But I am suggested to do a "Clone & Install"'
    And I kill the process "liveusb-creator"                                                                   # features/step_definitions/common_steps.rb:499
    And I "Clone & Install" Tails to USB drive "isohybrid"                                                     # features/step_definitions/usb.rb:141
    Then the running Tails is installed on USB drive "isohybrid"                                               # features/step_definitions/usb.rb:284
    But there is no persistence partition on USB drive "isohybrid"                                             # features/step_definitions/usb.rb:300
    And I unplug USB drive "isohybrid" 

Instead of the string that was previously displayed (Unsupported filesystem iso9660..), on the screen is (The destination device is not a Tails device...).

#25 Updated by kytv over 4 years ago

#26 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to kurono

Instead of the string that was previously displayed (Unsupported filesystem iso9660..), [...]

Kill Your TV: good catch, thanks! I should really have run the entire usb_install.feature before sneaking this ticket onto your plate. Sorry.

kurono: so, this means that the proposed branch currently introduces a regression when attempting to upgrade a Tails device that was created with hybrid ISO + cat/dd -- it will be very confusing to users to be told that their Tails device "is not a Tails device". However, I think we can easily work this around by making the error message we display a bit more generic, so that it takes the hybrid ISO + cat/dd use case into account; e.g. in Tails Upgrader, we say "was not created using Tails Installer" => please propose an alternate wording (ideally, we should just reuse the Upgrader's one) and have it reviewed by -ux@. Note, though, that the message we currently (Tails 1.4) display when attempting to upgrade a device created with hybrid ISO + cat/dd helps a bit the user decide what to do, and we should IMO not lose that.

Also, I believe we can simply kill the code that handles iso9660 as a special case, since it is not reachable anymore, right?

On second thought, one concern I have with the current implementation is that it only works in GUI mode, right? (e.g. the iso9660 special handling looks like it should work in command-line mode as well) That's a concern since we'll pretty soon want to polish the user story we provide for command-line users (#8861). Do you think this can be fixed by some light refactoring of the code you added?

#27 Updated by intrigeri over 4 years ago

  • Duplicated by Feature #6054: Tails Installer should hide the USB stick that contains the ISO added

#28 Updated by kurono over 4 years ago

  • Assignee deleted (kurono)
  • QA Check changed from Dev Needed to Ready for QA

#29 Updated by intrigeri over 4 years ago

  • Assignee set to intrigeri

#30 Updated by intrigeri over 4 years ago

  • Target version changed from Tails_1.4.1 to Tails_1.5

Too late => postponing.

#31 Updated by intrigeri over 4 years ago

Uploaded an updated package to the corresponding APT suite. Next step is to update the test suite accordingly.

#32 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to kurono
  • QA Check changed from Ready for QA to Dev Needed

kurono: I don't understand why new code was added to the _refresh_overlay_slider method, which seems to be totally unrelated to the problem at hand. Isn't there a better place to put it?

#33 Updated by intrigeri over 4 years ago

I've added some more automated tests for these changes in feature/5623-Installer-should-refuse-empty-device, so it'll be easy to validate the proposed changes once the code (that seems to work fine) is moved to a more suitable place :)

#34 Updated by kurono over 4 years ago

Ok, I decide to put the code in the same place as in https://labs.riseup.net/code/issues/9130, so the devices are skipped if they are not valid for installation nor upgrade. New iteration in https://git-tails.immerda.ch/kurono/liveusb-creator/log/?h=feature/5623-Installer-should-refuse-empty-device

#35 Updated by kurono over 4 years ago

  • Assignee deleted (kurono)
  • QA Check changed from Dev Needed to Ready for QA

#36 Updated by intrigeri over 4 years ago

  • Assignee set to intrigeri

#37 Updated by intrigeri over 4 years ago

OK, code looks good! I've pushed a few minor improvements on top in feature/5623-Installer-should-refuse-empty-device => kurono, please review them.

Then I've updated and pushed the Debian package to the corresponding APT suite. Will now run the test suite (including the new tests I've added specifically for this feature).

#38 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to kurono
  • QA Check changed from Ready for QA to Info Needed

Updated test suite, it's all green.

However: is it on purpose that "USB device found" is displayed even when no suitable device is listed (in my case, the only device is empty and thus cannot be upgraded)? I find this a little bit confusing.

#39 Updated by kurono over 4 years ago

  • Assignee deleted (kurono)
  • QA Check changed from Info Needed to Ready for QA

#40 Updated by intrigeri over 4 years ago

  • Assignee set to intrigeri

#41 Updated by intrigeri about 4 years ago

  • Assignee changed from intrigeri to kurono
  • % Done changed from 50 to 60
  • QA Check changed from Ready for QA to Info Needed

Cool, thanks. Another (and hopefully last) question: on #9734 you wrote " I decided to remove the exception since an user can have several devices conected and it does not make sense to completely stop the installer only because one of them is small". I think it totally makes sense => why not do the same on this branch as well?

#42 Updated by kurono about 4 years ago

  • Assignee deleted (kurono)
  • QA Check changed from Info Needed to Ready for QA

Ok, this is ready.

#43 Updated by intrigeri about 4 years ago

  • Assignee set to anonym

(Assigning to the RM to make it clear that I won't take care of this one.)

#44 Updated by intrigeri about 4 years ago

  • Assignee changed from anonym to intrigeri

#45 Updated by intrigeri about 4 years ago

  • Assignee changed from intrigeri to kurono
  • QA Check changed from Ready for QA to Dev Needed
  • Feature Branch changed from feature/5623-Installer-should-refuse-empty-device to kurono:feature/5623-Installer-should-refuse-empty-device

Thanks! However, this doesn't merge cleanly into current master => please merge master into your topic branch, resolve conflicts, test, and resubmit for QA.

#46 Updated by kurono about 4 years ago

  • Assignee changed from kurono to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

#47 Updated by intrigeri about 4 years ago

  • % Done changed from 60 to 70

Code review passes, yay!

Still, two comments that will become important since you intend to extend the scope of your contributions to Tails, possibly to areas where we care a bit more about having a nice and clean Git history:

  • commit 848824f: fixing merge conflicts should be done in the merge commit itself, not in a follow-up one; git commit --amend may help :)
  • commit e43af9e: "small change to message" is not a terribly useful commit message; in this case, I would prefer something like "don't localize error message a second time: this was done when defining the 'message' string already"; see what I mean?

Next step for me is to build an ISO with these changes in, and to run (+ adjust if needed) the tests I've written weeks ago.

#48 Updated by intrigeri about 4 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 70 to 100

#50 Updated by intrigeri about 4 years ago

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

Note that I've dared merging my test suite changes corresponding to this branch, since kytv already reviewed and tested them earlier.

#51 Updated by BitingBird about 4 years ago

  • Status changed from Fix committed to Resolved

#52 Updated by intrigeri almost 4 years ago

  • Related to Bug #10539: "Clone and upgrade" on Jessie pretends my Tails was not installed with our Installer added

Also available in: Atom PDF