sdw-last-updated not created/updated after update that requires reboot

[eloquence]

Steps to reproduce

1. Remove ~/.securedrop_launcher/sdw-last-updated if it exists, or note its contents
2. Perform an update using the preflight updater that includes fedora updates and requires a reboot
3. Inspect sdw-last-updated (you don't have to actually reboot, but you're welcome to)

Expected behavior

sdw-last-updated is up-to-date

Actual behavior

sdw-last-updated is not written / stale, causing sdw-notify to fire again. Due to the skip logic introduced in #402, sdw-last-updated will not be written in the immediately subsequent run, either.

Can I have some logs?

Yes.

[eloquence]

There appear to be two related issues with the current updater logic:

• When we check for updates in the Fedora VM (in an UpdateThread), we don't interpret the exit code -- we just treat "0" as "all good" and everything else as "problem or update required". As a consequence, we don't determine whether a reboot is required until updates are successfully applied. This is the check_call logic here. Note how we don't resolve the reboot requirement yet.

• As of Skip VM updates if last check was performed <X hours ago #402, we write the sdw-last-updated file to disk if a reboot is required, but only during the "check for updates" stage. This is the UpdateThread logic here. Note that there is no corresponding check for REBOOT_REQUIRED in the UpgradeThread logic here.

As a consequence of this logic, we never create or update the sdw-last-updated file in the "reboot required" case. This means sdw-notify will fire again even after a reboot, as it queries that file for the last successful update.

I think the easy fix is to move the REBOOT_REQUIRED check to UpgradeThread. Even if it was working there, it should arguably not run in UpdateThread, as that only tells us if updates are available, or that a reboot will eventually be required. We want to write sdw-last-updated in exactly two situations:

• we have just confirmed that the the system is up-to-date
• updates have been downloaded and applied (reboot may still be required).

That means UPDATES_OK in UpdateThread, and UPDATES_OK or REBOOT_REQUIRED in UpgradeThread. Right now the logic is the other way around.

@zenmonkeykstop @emkll If that above reasoning makes sense to you, I can put in a small PR just for the UpdateThread -> UpgradeThread change and test it.

[eloquence]

Tracking as release blocker since it causes incorrect security notifications to be displayed to the user, which prevent them from focusing on their work at hand.

[zenmonkeykstop]

With a bit more explanation from @eloquence this makes sense, 👍 from me.

[eloquence]

@emkll Am I correct that this code path will never be entered?

https://github.com/freedomofpress/securedrop-workstation/blob/master/launcher/sdw_updater_gui/UpdaterApp.py#L93-L116

Near as I can tell, none of the UpdateThreads (as opposed to UpgradeThreads) will ever return REBOOT_REQUIRED. _check_updates_dom0 and _check_updates_fedora (the ones that will trigger the reboot requirement) will return UPDATE_REQUIRED instead. Only overall_update_status can return this result, if a reboot requirement from a prior run has not been met yet.

If that's correct, I would suggest we remove this dead code path as part of the PR that resolves this issue.

I can also see if we can move some more business logic into Updater.py. The 0% test coverage on UpdaterApp.py really worries me for complex cases like this.

[eloquence]

If that's correct

It's not - that code path exists to handle the UI updates needed when the user restarts the updater when a reboot is still required. It gets its result from the VM status aggregation logic in overall_update_status, which tests whether the machine has been rebooted or not; it was added in #436. We may be able to remove that logic if we implement freedomofpress/securedrop-updater#17.

Therefore nothing else to do here right now, marking #465 ready for review. Thanks to @emkll for the explanation via Slack.