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-20130219
         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
-------------------------------------+-------------------------------------

Comment (by 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.

 '''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.
 - `_have_users` lacks documentation.
 - 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.

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

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

 - 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'))
 }}}

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