Skip to content

GitLab

Closed
Opened by alant@alantDeveloper1 of 1 task completed1/1 task

Fix non-blocking issues identified during ASP code review

Originally created by @alant on #15838 (Redmine)

My first batch of remarks:

  • 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.

configuration-window.ui:

  • Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway

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:

  • 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
  • it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager
  • both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?
  • as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True

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

  • message> is not translatable

persistence.py:

  • get_persistence_path(): only ever called with search_new_persistence=True, return_nonexistent=False
  • has_unlocked_persistence(): only called with search_new_persistence=True

Feature Branch: bugfix/15838-asp-fix-non-blocking-issues

Subtasks

Related issues

Edited by alant
To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information