Merge pull request #3615 from freedomofpress/remove-todos

removed TODOs and documented them in ticket tracker

docs/backup_and_restore.rst

@@ -153,8 +153,6 @@ Once the restore is done, the Application Server will use the original Source an
 *Journalist Interface* Onion URLs. You will need to update the corresponding
 files on the Admin Workstation:
 
-.. todo:: We really should automate this process for Admins.
-
 * ``app-source-ths``
 * ``app-journalist-aths``
 * ``app-ssh-aths``

docs/create_admin_account.rst

@@ -50,9 +50,6 @@ output like this:
 Passphrases include the spaces between the words, but not leading or trailing
 whitespace. Be sure to save this passphrase in the appropriate KeePassX database.
 
-.. todo:: Clarify how to set up TOTP/HOTP through ``./manage.py
-          add-admin``.
-
 Once that's done, you should open the Tor Browser |TorBrowser| and
 navigate to the *Journalist Interface*'s .onion address. Verify that you
 can log in to the *Journalist Interface* with the admin account you just

docs/development/setup_development.rst

@@ -164,15 +164,6 @@ from the `Vagrant Downloads page`_ and then install it.
             instructions in Vagrantfile that would enable vagrant-cachier are
             currently commented out.
 
-.. todo:: This warning is here because a common refrain during hackathons for
-          SecureDrop a while back was "setting up VMs is too slow, you should
-          use vagrant-cachier". We tried it and it had some nasty interactions
-          with Ansible, so we dropped it, and added this note to prevent other
-          people from making the same suggestion. Eventually, we should: (i)
-          Build our own base boxes to dramatically cut down on provisioning
-          times (ii) Remove this note as well as the commented vagrant-cachier
-          lines from the Vagrantfile
-
 VirtualBox should be at least version 5.x. See `GitHub #1381`_ for documentation
 of incompatibility with the older VirtualBox 4.x release series.
 

docs/journalist.rst

@@ -40,13 +40,6 @@ key, used for decryption, stays on the *Journalist Workstation*. The
 public key, used for encryption, is copied to the *Secure Viewing
 Station*.
 
-.. todo:: This document recommends transferring documents from the
-          *SVS* to the *Journalist Workstation*, without any
-          discussion of the potential risks or mitigations that should
-          be taken when doing so. A section needs to be added on why
-          doing this could be risky, and what can be done to make this
-          situation better.
-
 If you do not yet have a GPG key, follow the instructions for your
 operating system to set one up:
 

docs/onboarding.rst

@@ -41,9 +41,6 @@ you have a lot of journalists who wish to access SecureDrop
 concurrently, you will need to provision multiple *Secure Viewing
 Stations*.
 
-.. todo:: Describe best practices for provisioning multiple Secure
-          Viewing Stations.
-
 Create a Journalist Tails USB
 -----------------------------
 

docs/overview.rst

@@ -58,9 +58,6 @@ the sources, and the journalists.
 
 |SecureDrop architecture overview diagram|
 
-.. todo:: A picture of an actual physical setup (e.g. the office
-          setup) with the components labeled would also be good here.
-
 Servers
 ~~~~~~~
 

docs/source.rst

@@ -1,12 +1,6 @@
 Source Guide
 ============
 
-.. todo:: There's a lot more to it than this, but then we begin to
-          duplicate content from individual organization's landing
-          pages and Micah's Intercept article. For example: what
-          computer should you use? what network should you be on? etc.
-
-
 Choosing the Right Location
 ---------------------------
 

docs/terminology.rst

@@ -6,8 +6,6 @@ diagram <https://docs.securedrop.org/en/latest/overview.html#infrastructure>`__,
 are specific to SecureDrop. The list below attempts to enumerate and
 define these terms.
 
-.. todo:: Pictures would be good for many of the objects defined here
-
 Source
 ------
 

install_files/ansible-base/inventory

@@ -1,7 +1,6 @@
 #
 # Connection information so Ansible can access the app and monitor servers.
 # Defaults are from the documentation, adjust to your configuration.
-# TODO: configure networking with Ansible
 #
 # NOTE: after the install is complete, you can re-provision with Ansible but you will need to:
 # 1. Set up HidServAuth in your torrc with the values from app-ssh-aths and mon-ssh-aths

install_files/ansible-base/prod-specific.yml

@@ -4,7 +4,6 @@ ssh_users: ""
 dns_server: "8.8.8.8"
 daily_reboot_time: 4 # An integer between 0 and 23
 
-# TODO Should use ansible to gather this info
 monitor_ip: ""
 monitor_hostname: ""
 app_hostname: ""

install_files/ansible-base/roles/app-test/tasks/staging_wsgi_files.yml

@@ -5,7 +5,7 @@
 # logging for the source interface is useful for debugging,
 # so we enable it here.
 
-# TODO: This staging-only template clobbers the production template
+# This staging-only template clobbers the production template
 # installed by the `app` role. That means the app-staging host
 # will always mark the task as changed. It would be cleaner to merge
 # the templates and use group_vars to conditionally enable the logging

install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml

@@ -56,14 +56,6 @@
     - permissions
     - securedrop_config
 
-# TODO: config.py.example is already written using Jinja2 format, and should be
-# easy to template-ize. However, we cannot do this because when Ansible writes
-# a template, it re-does the entire thing. This would cause a problem in the
-# case where we want to re-provision a machine with an updated value (e.g. the
-# application key fingerprint). Blindly overwriting the entire file would cause
-# major problems. It would be nice to use templates here, assuming we find a
-# way to selectively update the file.
-
 # Note: we can also use register with with_items to cut down on repetition
 # here. See
 # http://docs.ansible.com/playbooks_loops.html#using-register-with-a-loop

install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/tasks/sass.yml

@@ -2,7 +2,7 @@
 - name: Install sass Ruby gem
   gem:
     name: sass
-    # TODO: This version lock can be removed when if/when we can get Ruby>2
+    # This version lock can be removed when if/when we can get Ruby>2
     # installed on the machine that runs this task (see
     # https://github.com/freedomofpress/securedrop/issues/1594).
     version: "3.4.23"

install_files/ansible-base/roles/common/tasks/create_users.yml

@@ -34,7 +34,7 @@
     - users
     - environment
 
-# TODO: Backwards-compatibility. Previously, the SecureDrop bashrc additions
+# Backwards-compatibility. Previously, the SecureDrop bashrc additions
 # for forcing a terminal multiplexer during interactive login sessions were
 # added to ~/.bashrc for each admin user account. It's cleaner to add the
 # config lines to /etc/profile.d, so all accounts get them by default.

install_files/ansible-base/roles/postfix/tasks/install_postfix.yml

@@ -57,5 +57,3 @@
     name: postfix
     state: "{{ 'started' if postfix_enable_service else 'stopped' }}"
     enabled: "{{ postfix_enable_service }}"
-
-# TODO - name: configure postfix proxy

install_files/ansible-base/roles/restrict-direct-access/templates/rules_v4

@@ -20,8 +20,6 @@
 {% endif %}
 
 # Load before tor user drop rules
-# TODO: use ansible facts to populate the in use interface to further restrict
-# the rules.
 -A OUTPUT -p tcp -m owner --uid-owner debian-tor -m state --state NEW,ESTABLISHED,RELATED -j ACCEPT -m comment --comment "Allow tor outbound"
 -A INPUT -p tcp -m state --state ESTABLISHED,RELATED -j ACCEPT -m comment --comment "Allow traffic back for tor"
 
@@ -69,13 +67,11 @@
 
 {% if 'securedrop_application_server' in group_names %}
 # OSSEC server-agent rules
-# TODO add owner to OUTPUT rule
 -A OUTPUT -d {{ monitor_hostname }} -p udp --dport 1514 -m state --state NEW,ESTABLISHED,RELATED -j ACCEPT -m comment --comment "OSSEC server agent"
 -A INPUT -s {{ monitor_hostname }} -p udp --sport 1514 -m state --state ESTABLISHED,RELATED -j ACCEPT -m comment --comment "OSSEC server agent"
 
 {% elif 'securedrop_monitor_server' in group_names %}
 # OSSEC server-agent rules
-# TODO add owner to OUTPUT rule
 -A INPUT -s {{ app_hostname }} -p udp --dport 1514 -m state --state NEW,ESTABLISHED,RELATED -j ACCEPT -m comment --comment "Allow OSSEC agent to monitor"
 -A OUTPUT -d {{ app_hostname }} -p udp --sport 1514 -m state --state ESTABLISHED,RELATED -j ACCEPT -m comment --comment "Allow OSSEC agent to monitor"
 

securedrop/config.py.example

@@ -8,7 +8,6 @@ class FlaskConfig(object):
 
 # Use different secret keys for the two different applications so their
 # sessions can't be confused.
-# TODO consider using salts instead: http://pythonhosted.org//itsdangerous/#the-salt
 class SourceInterfaceFlaskConfig(FlaskConfig):
     SECRET_KEY = '{{ source_secret_key.stdout }}'
     SESSION_COOKIE_NAME = "ss"
@@ -60,7 +59,6 @@ elif env == 'test':
     FlaskConfig.TESTING = True
     # Disable CSRF checks to make writing tests easier
     FlaskConfig.WTF_CSRF_ENABLED = False
-    # TODO use a unique temporary directory for each test so we can parallelize them
     SECUREDROP_DATA_ROOT = '/tmp/securedrop'
 
 # The following configuration is dependent on SECUREDROP_DATA_ROOT
@@ -78,9 +76,6 @@ GPG_KEY_DIR=os.path.join(SECUREDROP_DATA_ROOT, 'keys')
 TEMP_DIR = os.path.join(SECUREDROP_DATA_ROOT, "tmp")
 
 # Database configuration
-# TODO we currently use sqlite in production since it is sufficient and simple,
-# but in the future may want to be able to choose a different database
-# depending on the environment
 DATABASE_ENGINE = 'sqlite'
 DATABASE_FILE = os.path.join(SECUREDROP_DATA_ROOT, 'db.sqlite')
 

securedrop/crypto_util.py

@@ -81,7 +81,6 @@ def __init__(self,
             self.adjectives = f.read().splitlines()
 
     # Make sure these pass before the app can run
-    # TODO: Add more tests
     def do_runtime_tests(self):
         if self.scrypt_id_pepper == self.scrypt_gpg_pepper:
             raise AssertionError('scrypt_id_pepper == scrypt_gpg_pepper')
@@ -184,7 +183,6 @@ def delete_reply_keypair(self, source_filesystem_id):
         # deleted. http://pythonhosted.org/python-gnupg/#deleting-keys
         self.gpg.delete_keys(key, True)  # private key
         self.gpg.delete_keys(key)  # public key
-        # TODO: srm?
 
     def getkey(self, name):
         for key in self.gpg.list_keys():

securedrop/journalist_app/decorators.py

@@ -12,7 +12,6 @@ def admin_required(func):
     def wrapper(*args, **kwargs):
         if logged_in() and g.user.is_admin:
             return func(*args, **kwargs)
-        # TODO: sometimes this gets flashed 2x (Chrome only?)
         flash(gettext("Only administrators can access this page."),
               "notification")
         return redirect(url_for('main.index'))

securedrop/manage.py

@@ -42,7 +42,7 @@ def reset(args):
     """
     # Erase the development db file
     if not hasattr(config, 'DATABASE_FILE'):
-        raise Exception("TODO: ./manage.py doesn't know how to clear the db "
+        raise Exception("./manage.py doesn't know how to clear the db "
                         'if the backend is not sqlite')
 
     # we need to save some data about the old DB file so we can recreate it

securedrop/management/run.py

@@ -88,16 +88,6 @@ def __init__(self, proc_funcs):
 
     def monitor(self):
         while True:
-            # TODO: we currently don't handle input, which makes using an
-            # interactive debugger like pdb impossible. Since Flask provides
-            # a featureful in-browser debugger, I'll accept that pdb is
-            # broken for now. If someone really wants it, they should be
-            # able to change this function to make it work (although I'm not
-            # sure how hard that would be).
-            #
-            # If you really want to use pdb, you can just run the
-            # application scripts individually (`python source.py` or
-            # `python journalist.py`).
             rprocs, _, _ = select.select(self.procs, [], [])
 
             for proc in rprocs:

securedrop/models.py

@@ -26,9 +26,6 @@
 
 
 LOGIN_HARDENING = True
-# Unfortunately, the login hardening measures mess with the tests in
-# non-deterministic ways.  TODO rewrite the tests so we can more
-# precisely control which code paths are exercised.
 if os.environ.get('SECUREDROP_ENV') == 'test':
     LOGIN_HARDENING = False
 

securedrop/tests/conftest.py

@@ -22,7 +22,7 @@
 from source_app import create_app as create_source_app
 import utils
 
-# TODO: the PID file for the redis worker is hard-coded below.
+# The PID file for the redis worker is hard-coded below.
 # Ideally this constant would be provided by a test harness.
 # It has been intentionally omitted from `config.py.example`
 # in order to isolate the test vars from prod vars.

securedrop/tests/functional/journalist_navigation_steps.py

@@ -234,8 +234,7 @@ def _add_user(self, username, is_admin=False, hotp=None):
             hotp_secret.send_keys(hotp)
 
         if is_admin:
-            # TODO implement (checkbox is unchecked by default)
-            pass
+            raise NotImplementedError("Admin's can't be added yet.")
 
         submit_button = self.driver.find_element_by_css_selector(
             'button[type=submit]')

securedrop/tests/test_journalist.py

@@ -161,7 +161,6 @@ def test_unauthorized_access_redirects_to_login(journalist_app):
 
 def test_login_throttle(journalist_app, test_journo):
     # Overwrite the default value used during testing
-    # TODO this may break other tests during parallel testing
     models.LOGIN_HARDENING = True
     try:
         with journalist_app.test_client() as app:
@@ -1130,9 +1129,6 @@ def tearDown(self):
     # making a point of this, we hope to avoid the introduction of new tests,
     # that do not truly prove their result because of this disconnect between
     # request context in Flask Testing and production.
-    #
-    # TODO: either ditch Flask Testing or subclass it as discussed in the
-    # aforementioned issue to fix the described problem.
     def _login_admin(self):
         self._ctx.g.user = self.admin
 

securedrop/tests/utils/env.py

@@ -16,7 +16,7 @@
 
 FILES_DIR = abspath(join(dirname(realpath(__file__)), '..', 'files'))
 
-# TODO: the PID file for the redis worker is hard-coded below.  Ideally this
+# The PID file for the redis worker is hard-coded below.  Ideally this
 # constant would be provided by a test harness.  It has been intentionally
 # omitted from `config.py.example` in order to isolate the test vars from prod
 # vars.  When refactoring the test suite, the test_worker_pidfile

testinfra/app/test_tor_hidden_services.py

@@ -13,7 +13,6 @@ def test_tor_service_directories(File, Sudo, tor_service):
     with Sudo():
         f = File("/var/lib/tor/services/{}".format(tor_service['name']))
         assert f.is_directory
-        # TODO: tor might mark these dirs as setgid
         assert oct(f.mode) == "0700"
         assert f.user == "debian-tor"
         assert f.group == "debian-tor"