BIND 10 #2641: Disable default account, require authentication setup during initialization

[BIND 10 Development]

#2641: Disable default account, require authentication setup during initialization
-------------------------------------+-------------------------------------
            Reporter:  shane         |                        Owner:
                Type:  enhancement   |  jinmei
            Priority:  very high     |                       Status:
           Component:  bind-ctl      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130305
         Sub-Project:  Core          |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:13 jinmei]:
 > '''general'''
 >
 > Most of bindcmd and cmdctl changes are not tested.  I'm not sure
 > if "tests missing for cmdctl HTTP API calls" meant this, but in any
 > case I don't think we can allow us to use it as an excuse of skipping
 > tests that "the amount of work would exceed the estimation".  If it
 > would be beyond the estimation, we should defer the whole development
 > to a later task with re-evaluation, not allowing to merge the
 > implementation only.  Besides, I don't necessarily think testing these
 > requires an excessively large amount of work; from a quick look many
 > things can be tested without requiring actual network I/O or SSL
 > related things (if that's the reason why you thought it was big).
 >
 > If we really think adding tests is huge addition, my suggestion is:
 > - if removing the default account is deemed to be important, introduce
 >   only that change, and defer all other implementation changes to a
 >   later task.
 > - if we can also defer removing the default account, simply defer the
 >   entire task to post-release, re-estimating it if necessary.

 You have merged a partial implementation to `master` branch to disable
 the default accounts. The rest of the work is now in the `trac2641_3`
 branch, which is revised on top of that work.

 > '''bindcmd.py'''
 >
 > - `_have_users` repeats many things of `_try_login`.  Besides, if I
 >   understand it correctly, the introduction of `_have_users` would
 >   make the checks in `_try_login` unnecessary.  These should be
 >   clarified, and the code needs to be cleaned up accordingly.

 I don't understand how the checks are unnecessary. If these failures
 occur, they should still cause `FailToLogin` to be raised. But maybe I
 don't follow properly.

 > - `_have_users` lacks documentation.

 This has been added.

 > - send_POST: I'd revise it to:
 > {{{
 >         if post_param is not None and len(post_param) != 0:
 >             param = json.dumps(post_param)
 > }}}
 >   to clarify it's about comparison with `None`, not just whether it's
 >   evaluated to be true/false as boolean.  I'd also remove the
 >   unnecessary parentheses.

 Nod. I had this version first, and shortened it. :)

 > '''cmdctl'''
 >
 > - why was cmdctl-accounts.csv moved to tests/testdata?  Also, is the
 >   now-empty csv file under cmdctl/ used?  And, in any event, the
 >   changes seem to introduce several non trivial implications.  First,
 >   how should this work if we run bind10 from source using
 >   run_bind10.sh?  Using the one in testdata or the empty one?  If it's
 >   the latter, we can override the file for local experiments/tests and
 >   can accidentally commit it to the repository, which is not good.
 >   Also, I personally would like to avoid having hardcoded
 >   "tests/testdata" in the production version of script; even though
 >   things like B10_FROM_BUILD or B10_FROM_SOURCE is already a hack,
 >   embedded string of "test" makes it look even uglier.

 B10_FROM_SOURCE and B10_FROM_BUILD hotwire stuff for the tests to work.
 I don't think making use of `tests/testdata/` in this context is too
 bad, considering the contents of the `.csv` file *are* test data and it
 belongs in the `tests/testdata/` directory.

 I think you have made it not install the empty one in your branch, so
 there is no longer any empty file in the tree.

 > - get_num_users: it's not clear why such a very short method only used
 >   by one other method is defined as a separate public method.  If the
 >   intent is to make it testable separately, that should be documented
 >   (but it seems we can easily cover that part of test by testing
 >   `_handle_users_exist`).  If the intent is something else, I don't
 >   see the reason.  And, in any case, this is essentially a private
 >   method, so unless it's needed for tests, it should be prefixed with
 >   `__`; if it's needed to be tested directly, I'd make it "protected"
 >   with a single underscore with documenting the intent.

 `get_num_users()` belongs to a different class (`SecureHTTPServer`) and
 access member data inside that class. It is called from
 `SecureHTTPRequestHandler` class. So for this reason, it is public. It
 is also easy to test it this way.

 > - test_get_num_users: although not necessarily harmful, I don't see
 >   the point of testing the result of get_user_info in this case:
 > {{{#!python
 >         self.assertIn('6f0c73bd33101a5ec0294b3ca39fec90ef4717fe',
 >                       self.server.get_user_info('root'))
 > }}}

 I have moved this to a unittest for `get_user_info()`. I hope this is a
 more appropriate place for it.

 In response to your other comments about missing tests, I have covered
 the API with tests now. I have also added tests for some related code in
 cmdctl and bindctl which were missing tests. Please check these. I think
 these should (reasonably) be enough tests for the API.

 I should have assigned this ticket back earlier today, but I found a
 regression triggered during lettuce testing that had to be fixed first.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2641#comment:16>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development