Fixes #4708, adds sdconfig prompt for v2/v3 services #4710

kushaldas · 2019-08-27T19:03:24Z

Now it asks if the admin wants to enable v2 or v3 services.
It will not let you to disable v3 if you already disabled v2.

Status

Work in progress

Description of Changes

Fixes #4708

./securedrop-admin sdconfig now asks for admin input to set to enable v2 and v3 onion services.

Testing

A few scenarios.

Already installed system, enable v2 and answer no for v3_onion_services
Already installed system, try to answer no to both v2, and v3. It will not let do v3 as no if you already disabled v2
Fresh system (by just deleting/moving install_files/ansible-base/group_vars/all/site-specific file), by default v2_onion_services should ask as no and v3_onion_services should give yes as default value.

Note: I could not write/upgrade integration tests for this yet.

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make -C securedrop test) pass in the development container

If you made changes to `securedrop-admin`:

Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

Configuration tests pass

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

If you made changes to documentation:

Doc linting (make docs-lint) passed locally

codecov-io · 2019-08-28T19:44:47Z

Codecov Report

Merging #4710 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #4710   +/-   ##
========================================
  Coverage    81.61%   81.61%           
========================================
  Files           49       49           
  Lines         3416     3416           
  Branches       391      391           
========================================
  Hits          2788     2788           
  Misses         535      535           
  Partials        93       93

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3d3ab7...36ea430. Read the comment docs.

redshiftzero

looking good - a few comments/questions inline

redshiftzero · 2019-08-28T22:20:41Z

admin/securedrop_admin/__init__.py

+             lambda x: x.lower() == 'yes',
+             lambda config: True],
+            ['v3_onion_services', self.check_for_v3_onion, bool,
+             u'Do you want to enable new v3 onion services?',


Do you want to enable new v3 onion services? -> Do you want to enable v3 onion services (recommended)?

redshiftzero · 2019-08-28T22:23:10Z

admin/securedrop_admin/__init__.py

@@ -403,6 +422,16 @@ def __init__(self, args):
             SiteConfig.ValidateLocales(self.args.app_path),
             string.split,
             lambda config: True],
+            ['v2_onion_services', self.check_for_v2_onion(), bool,
+             u'Do you want to enable v2 onion services?',


Do you want to enable v2 onion services? -> Do you want to enable v2 onion services (recommended only for SecureDrop instances installed before 1.0.0)?

redshiftzero · 2019-08-28T22:24:05Z

admin/securedrop_admin/__init__.py

+            # is already disabled.
+            if text == 'no' and \
+                    not self.caller._config.get("v2_onion_services"):
+                raise ValidationError(message="Must be yes as you disabled v2")


Must be yes as you disabled v2 -> Since you disabled v2 onion services, you must enable v3 onion services.

redshiftzero · 2019-08-28T22:25:14Z

admin/securedrop_admin/__init__.py


    def user_prompt_config(self):
-        config = {}
+        self._config = {}
        for desc in self.desc:
            (var, default, type, prompt, validator, transform,


I know this is not in the diff but the variable names here are uninformative (since not in the diff, not requesting changing, just noting), e.g. var, desc, condition in particular

That is the old code :) I am trying not to modify anything which does not require for this issue. We can do code cleanup, but, in a future PR.

redshiftzero · 2019-08-28T22:26:09Z

admin/securedrop_admin/__init__.py

+             SiteConfig.ValidateYesNo(),
+             lambda x: x.lower() == 'yes',
+             lambda config: True],
+            ['v3_onion_services', self.check_for_v3_onion, bool,


Hmm do we need the check_for_v3_onion method? It seems like we could remove the method and just have this be True?

@redshiftzero This dynamic function checks if the user input for v2_onion_services was no, and then v3_onion_services is must to yes. But, if v2_onion_servies was set to yes, and previously the admin set v3_onion_service to no, then it will show no as default value.

conorsch · 2019-08-29T01:51:01Z

Have not given a full review here yet, but I see @redshiftzero was kind enough to stop by with a few requests. Please address @redshiftzero's comments when you log in, @kushaldas, and I can follow up with another round when you're satisfied.

redshiftzero · 2019-08-29T18:56:26Z

admin/securedrop_admin/__init__.py

@@ -261,6 +278,8 @@ def validate(self, document):
    def __init__(self, args):
        self.args = args
        self.config = {}
+        # This is to hold runtime configuration before save
+        self._config = {}


we should rename this before merge to something more expressive

runtime_config ?

Reasonable; I was going to suggest _config_in_progress.

Just pick one and change every where (including tests). I don't want to do it at 1:30AM :)

conorsch · 2019-08-29T20:34:31Z

Functionally, works as described. Will push up a small change to make variable names more explicit, as discussed above.

conorsch

Approving for merge. Confirmed the following:

running sdconfig with no site-specific config (i.e. fresh install) defaults to no-v2, yes-v3
running sdconfig with existing site-specific config (i.e. existing install) defaults to yes-v2, yes-v2
running sdconfig and setting "no" to both v2 & v3 raises a blocking error, preventing broken config

Added a single dev-oriented commit, fine to merge after tests pass.

conorsch · 2019-08-29T21:43:53Z

Seeing a rather perplexing CI error in the lint step: https://circleci.com/gh/freedomofpress/securedrop/32939 Will try to reproduce locally.

Now it asks if the admin wants to enable v2 or v3 services. It will not let you to disable v3 if you already disabled v2.

We now have two more integration tests to verify that we can have "enabled v2 and disabled v3" and both versions enabled at the same time.

conorsch · 2019-08-29T22:37:21Z

Rebased on top of latest develop (d3d3ab7) to resolve CI woes. Hat tip to @rmol for pointing out the drift.

There's already a `.config` attribute in the sdconfig flow, so let's rename `._config` -> `._config_in_progress` to make the intended use case explicit to future maintainers. The contents of the _config_in_progress dict are consulted during validation steps as part of `sdconfig`.

@redshiftzero

Changes requested by @redshiftzero have been implemented

conorsch

Reapproving now that CI is passing. 😎

kushaldas force-pushed the ask_all_questions branch 2 times, most recently from 392b413 to e68c745 Compare August 28, 2019 16:52

kushaldas marked this pull request as ready for review August 28, 2019 17:02

kushaldas requested review from emkll, heartsucker, msheiny and redshiftzero as code owners August 28, 2019 17:02

kushaldas force-pushed the ask_all_questions branch 3 times, most recently from c52d918 to eebe892 Compare August 28, 2019 18:56

redshiftzero previously requested changes Aug 28, 2019

View reviewed changes

kushaldas mentioned this pull request Aug 29, 2019

Fixes #4677 warns admin if v3 and https both are enabled #4720

Merged

5 tasks

redshiftzero reviewed Aug 29, 2019

View reviewed changes

conorsch previously approved these changes Aug 29, 2019

View reviewed changes

kushaldas added 3 commits August 29, 2019 15:36

Fixes #4708, adds sdconfig prompt for v2/v3 services

c2210c3

Now it asks if the admin wants to enable v2 or v3 services. It will not let you to disable v3 if you already disabled v2.

Adds test cases and related change for v2/v3 user input

d12f971

We now have two more integration tests to verify that we can have "enabled v2 and disabled v3" and both versions enabled at the same time.

Updates based on feedback of strings

f1b4e48

conorsch dismissed their stale review via 968ee87 August 29, 2019 22:36

conorsch force-pushed the ask_all_questions branch from f46ebda to 968ee87 Compare August 29, 2019 22:36

conorsch force-pushed the ask_all_questions branch from 968ee87 to 36ea430 Compare August 29, 2019 23:34

conorsch self-requested a review August 30, 2019 00:30

conorsch approved these changes Aug 30, 2019

View reviewed changes

conorsch merged commit 549a056 into develop Aug 30, 2019

conorsch deleted the ask_all_questions branch August 30, 2019 00:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #4708, adds sdconfig prompt for v2/v3 services #4710

Fixes #4708, adds sdconfig prompt for v2/v3 services #4710

kushaldas commented Aug 27, 2019 •

edited

Loading

codecov-io commented Aug 28, 2019 •

edited

Loading

redshiftzero left a comment

redshiftzero Aug 28, 2019

redshiftzero Aug 28, 2019

redshiftzero Aug 28, 2019

redshiftzero Aug 28, 2019

kushaldas Aug 29, 2019

redshiftzero Aug 28, 2019

kushaldas Aug 29, 2019

conorsch commented Aug 29, 2019

redshiftzero Aug 29, 2019 •

edited

Loading

kushaldas Aug 29, 2019

conorsch Aug 29, 2019

kushaldas Aug 29, 2019

conorsch commented Aug 29, 2019

conorsch left a comment

conorsch commented Aug 29, 2019

conorsch commented Aug 29, 2019

conorsch left a comment

Fixes #4708, adds sdconfig prompt for v2/v3 services #4710

Fixes #4708, adds sdconfig prompt for v2/v3 services #4710

Conversation

kushaldas commented Aug 27, 2019 • edited Loading

Status

Description of Changes

Testing

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you made non-trivial code changes:

If you made changes to documentation:

codecov-io commented Aug 28, 2019 • edited Loading

Codecov Report

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Aug 29, 2019

redshiftzero Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Aug 29, 2019

conorsch left a comment

Choose a reason for hiding this comment

conorsch commented Aug 29, 2019

conorsch commented Aug 29, 2019

conorsch left a comment

Choose a reason for hiding this comment

kushaldas commented Aug 27, 2019 •

edited

Loading

If you made changes to `securedrop-admin`:

codecov-io commented Aug 28, 2019 •

edited

Loading

redshiftzero Aug 29, 2019 •

edited

Loading