Tails

#1 Updated by intrigeri 2018-08-26 06:09:11

• Deliverable for set to 299

#2 Updated by alant 2018-08-26 14:22:30

• related to Feature #14598: Code review for Additional Software packages GUI added

#3 Updated by alant 2018-08-26 16:40:08

• Assignee changed from alant to segfault
• % Done changed from 0 to 30
• QA Check set to Info Needed

> * I’d suggest you set `PERSISTENCE_SETUP_USERNAME = “tails-persistence-setup”` in tailslib/init.py and use it in the two occurrances where this string is currently used.
>
Done

> configuration-window.ui:
>
> * Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway
>
That would make the UI file less understandable for no benefit that I can see.

> tails-additional-software:
>
> * Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred

Done

> * Use logging.warning instead of deprecated logging.warn

Done

> * _notify_failure():
> * incomplete sentence in docstring: “The user has the option to edit the configuration of to view the system log”

Fixed

> * second “details =” line uses .format() but the string doesn’t contain any replacement fields

Fixed

> * apt_hook_pre(): you shouldn’t use `assert` to check for conditions that could happen, because it is only evaluated if debug is True (see https://docs.python.org/3/reference/simple_stmts.html)

Fixed

> * main(): What do we need the syslog handler for? Wouldn’t it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.

Fixed
>
> config.py:
>
Renamed to additionalsoftware.py so that we don’t have an empty package.

> * there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).
> This way you could also get rid of the python3-atomicwrites package

As far as I understand, such lock are released on close, so I don’t see how it would actually lock the config file (see https://linux.die.net/man/2/flock).
The way the code works now, the file is opened for reading, then closed, then opened again for writing, closed, and chmod.
We don’t even have any guarantee we actually write the the same path we read, because get_packages_list_path is called again each time.
Am I wrong?
Do you think we should ensure the file is only opened once? Or at least that it is the same file the gets written that the one we read?

> * it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager

I added a try...finally block. I don’t see why a context manager used only once would be better.

> * both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?

You’re right, it was meant for providing the same beahavior as before when I added the argument, but it’s now useless for these functions.

> * as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True
>
No, see config/chroot_local-includes/usr/local/sbin/tails-additional-software:516:

@ packages = get_additional_packages()@

> persistence.py:
>
> * get_persistence_path(): only ever called with search_new_persistence=True, return_nonexistent=False

No, see above note on search_new_persistence.

> * has_unlocked_persistence(): only called with search_new_persistence=True

Right.

I’m not sure wether it’s better to remove the argument for the function where it’s always called with True or to keep it so that they all behave the same way. I’m tempted to say the 2nd is more readeable, but that may be lazyness speaking. What do you think?

#4 Updated by alant 2018-08-26 22:28:40

• Status changed from Confirmed to In Progress

Applied in changeset commit:8194bb557f97bbf1bb6431c21d206e86718bc740.

#5 Updated by alant 2018-08-31 14:39:23

• Feature Branch set to feature/14594-asp-gui

#6 Updated by alant 2018-08-31 18:06:29

• blocked by Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies added

#7 Updated by intrigeri 2018-09-01 07:46:13

• blocks deleted (Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies)

#9 Updated by segfault 2018-09-01 19:27:06

• Assignee changed from segfault to alant
• QA Check changed from Info Needed to Dev Needed

alant wrote:
> > configuration-window.ui:
> >
> > * Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway
> >
> That would make the UI file less understandable for no benefit that I can see.

I see these disadvantages of keeping the string:

• Can easily lead to confusion when someone wants to change that string
• Will lead to unnecessary work for translators in case the string is changed in only one place

> > config.py:
> >
> Renamed to additionalsoftware.py so that we don’t have an empty package.
>
> > * there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).
> > This way you could also get rid of the python3-atomicwrites package
>
> As far as I understand, such lock are released on close, so I don’t see how it would actually lock the config file (see https://linux.die.net/man/2/flock).
> The way the code works now, the file is opened for reading, then closed, then opened again for writing, closed, and chmod.
> We don’t even have any guarantee we actually write the the same path we read, because get_packages_list_path is called again each time.
> Am I wrong?
> Do you think we should ensure the file is only opened once?

Yes, that’s what I meant and what the flock can be used for.

> > * it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager
>
> I added a try...finally block. I don’t see why a context manager used only once would be better.

I generally prefer them for readability, but I’m also OK with the way you did it.

> > * both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?
>
> You’re right, it was meant for providing the same beahavior as before when I added the argument, but it’s now useless for these functions.
>
> > * as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True
> >
> No, see config/chroot_local-includes/usr/local/sbin/tails-additional-software:516:
>
> @ packages = get_additional_packages()@

> I’m not sure wether it’s better to remove the argument for the function where it’s always called with True or to keep it so that they all behave the same way. I’m tempted to say the 2nd is more readeable, but that may be lazyness speaking. What do you think?

I think it would make the code more readable if you would just remove this argument from all functions, and always check if there is a new persistence. Even in this one line you quoted above, it only does not check for a new persistence because you expect that there is none anyway, because it’s called right after boot, right? So it wouldn’t be an issue if it would still check for a (non-existing) new persistence.

#13 Updated by Anonymous 2018-09-03 08:09:33

• related to Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies added

#19 Updated by intrigeri 2018-09-04 10:57:56

• related to deleted (Feature #14598: Code review for Additional Software packages GUI)

#20 Updated by intrigeri 2018-09-04 10:58:04

• blocks Feature #14598: Code review for Additional Software packages GUI added

#21 Updated by intrigeri 2018-09-04 13:15:42

• related to deleted (Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies)

#22 Updated by alant 2018-10-15 12:42:37

• Description updated

#23 Updated by Anonymous 2018-10-16 13:40:58

• related to Bug #16060: Improve ASP code: configuration window added

#24 Updated by Anonymous 2018-10-16 13:44:13

• related to Bug #16061: Improve ASP code: get rid of search_new_persistence argument added

#25 Updated by Anonymous 2018-10-16 13:45:40

• related to Bug #16062: Improve ASP code: config.py added

#26 Updated by Anonymous 2018-10-16 13:58:56

• Status changed from In Progress to Confirmed
• QA Check deleted (Dev Needed)

@alant: you can close this ticket if all remaining tasks have been correctly reported elsewhere.

#27 Updated by alant 2018-10-18 17:53:31

• related to Bug #16034: ASP: Fix race condition when writing to packages file added

#29 Updated by alant 2018-10-18 18:12:21

• Assignee changed from alant to segfault
• QA Check set to Ready for QA

This ticket is supposed to solve:

• I’d suggest you set `PERSISTENCE_SETUP_USERNAME = “tails-persistence-setup”` in tailslib/init.py and use it in the two occurrances where this string is currently used.

tails-additional-software:

• Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred
• Use logging.warning instead of deprecated logging.warn
• _notify_failure(): * incomplete sentence in docstring: “The user has the option to edit the configuration of to view the system log” * second “details =” line uses .format() but the string doesn’t contain any replacement fields
• show_system_log(): the log file is not readable by amnesia, so gedit is not able to show it
• apt_hook_pre(): you shouldn’t use `assert` to check for conditions that could happen, because it is only evaluated if debug is True (see https://docs.python.org/3/reference/simple_stmts.html)
• main(): What do we need the syslog handler for? Wouldn’t it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.

config.py:

• it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager

org.boum.tails.additional-software.policy:

• is not translatable

#30 Updated by segfault 2018-10-20 11:52:58

• QA Check changed from Ready for QA to Pass

alant wrote:
> This ticket is supposed to solve:
>
> * I’d suggest you set `PERSISTENCE_SETUP_USERNAME = “tails-persistence-setup”` in tailslib/init.py and use it in the two occurrances where this string is currently used.
>
> tails-additional-software:
>
> * Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred
> * Use logging.warning instead of deprecated logging.warn
> * _notify_failure(): * incomplete sentence in docstring: “The user has the option to edit the configuration of to view the system log” * second “details =” line uses .format() but the string doesn’t contain any replacement fields
> * show_system_log(): the log file is not readable by amnesia, so gedit is not able to show it
> * apt_hook_pre(): you shouldn’t use `assert` to check for conditions that could happen, because it is only evaluated if debug is True (see https://docs.python.org/3/reference/simple_stmts.html)
> * main(): What do we need the syslog handler for? Wouldn’t it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.
>
> config.py:
>
> * it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager

LGTM

> org.boum.tails.additional-software.policy:
>
> * is not translatable

The fix for this was already merged in stable

I’m still reviewing Bug #15983, which was done on the same branch, so I will not pass this to intrigeri’s plate until I’m done with that review.

#31 Updated by segfault 2018-10-20 12:39:35

• Assignee changed from segfault to intrigeri

> I’m still reviewing Bug #15983, which was done on the same branch, so I will not pass this to intrigeri’s plate until I’m done with that review.

That’s done

#32 Updated by intrigeri 2018-10-21 09:04:35

• Feature Branch changed from feature/14594-asp-gui to bugfix/15838-asp-fix-non-blocking-issues

feature/14594-asp-gui does not exist anymore so given segfault’s comment, I guess the branch is bugfix/15838-asp-fix-non-blocking-issues. Alan, in the future please keep such metadata up-to-date :)

#33 Updated by intrigeri 2018-10-21 09:17:43

• Status changed from Confirmed to In Progress
• Assignee changed from intrigeri to alant
• Target version changed from Tails_3.10.1 to Tails_3.11
• QA Check deleted (Pass)

Same as Bug #15983#note-7: merged locally, will push soon. Reassigning to Alan for the next steps (according to the ticket description, there’s still issues left to be fixed).

#34 Updated by alant 2018-10-21 10:58:43

• Assignee changed from alant to intrigeri
• Target version changed from Tails_3.11 to Tails_3.10.1
• QA Check set to Pass

intrigeri wrote:
> feature/14594-asp-gui does not exist anymore so given segfault’s comment, I guess the branch is bugfix/15838-asp-fix-non-blocking-issues. Alan, in the future please keep such metadata up-to-date :)

Right, sorry.

> Same as Bug #15983#note-7: merged locally, will push soon. Reassigning to Alan for the next steps (according to the ticket description, there’s still issues left to be fixed).

No I have moved them because it started to be very messy. See https://labs.riseup.net/code/issues/15838#note-28.

#35 Updated by intrigeri 2018-10-21 18:47:52

• Assignee changed from intrigeri to alant
• QA Check changed from Pass to Info Needed

OK. So is there anything I am supposed to do? (You’ve reassigned to me.) If not, please set status = Fix committed and boom :)

#36 Updated by alant 2018-10-23 17:38:18

• Status changed from In Progress to Fix committed
• Assignee deleted (alant)
• QA Check changed from Info Needed to Pass

#37 Updated by CyrilBrulebois 2018-10-24 11:19:46

• Status changed from Fix committed to Resolved

#38 Updated by bertagaz 2018-11-08 16:28:09

• related to Bug #16110: Button to remove package in ASP GUI has a wrong label added