BIND 10 #2713: b10-cmdctl-usermgr's prompts should be revisited

[BIND 10 Development]

#2713: b10-cmdctl-usermgr's prompts should be revisited
-------------------------------------+-------------------------------------
            Reporter:  muks          |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  cmd-ctl       |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130305
         Sub-Project:  DNS           |                   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):

 '''others/general'''

 - maybe we need a changelog entry for this task.
 - the "run" script doesn't work:
 {{{
 % ./run_b10-cmdctl-usermgr.sh
 Traceback (most recent call last):
   File "b10-cmdctl-usermgr", line 22, in <module>
     from bind10_config import SYSCONFPATH
 ImportError: No module named bind10_config
 }}}
   I've committed a suggested fix: 5836dc5
 - you can avoid the need for the variable result:
 {{{#!python
     #result = main()
     #sys.exit(result)
     sys.exit(main())
 }}}
 - should we care about broken csv files (missing field, etc)?

 - maybe we should discourage typing in password as a command-line
   argument (because in theory it could be visible to others via ps(1)
   etc)?

 - probably not a big deal or matter of taste for the expected scale of
   the account file, but we might use a dictionary from username to
   other account info for the internal `self.user_info.  It would make
   other operations (especially find and delete) simpler.

 - also maybe a bikeshed matter, but I wonder whether xxxmgr is a good
   name for this type of tool, especially when we use the word 'mgr'
   for daemon-like programs, too.  Maybe something like b10-passwd?

 - in general (both for the main and test .py), I'd like to clarify
   which methods are inherently public, which are intended to be
   definitely private, and which are basically private and only
   directly accessible by tests with the `_` and `__` prefixes.

 '''gen_password_hash'''

 - what's the rationale of the magic numbers?  I actually see 33 and
   127, but I'd avoid the raw numbers here.  Is there any cryptographic
   consideration needed for the limited character set and the size of
   output?

 '''read_user_info'''

 - according to python doc the CSV file must be opened with
   `newline=''`:
 http://docs.python.org/release/3.2.3/library/csv.html#csv.reader
   same for save_userinfo and tests.
 - the body of `with` could be simplified as follows (probably a matter
   of taste though):
 {{{#!python
                 self.user_info = [row for row in csv.reader(csvfile)]
 }}}
   Same for check_output_file() in tests.

 '''save_userinfo'''

 - maybe a matter of taste, but temporary 'writer' variable can be
   omitted.

 '''b10-cmdctl-usermgr.xml'''

 {{{
         <term><option>-f
 <replaceable>filename</replaceable></option></term>
         <term><option>--file
 <replaceable>filename</replaceable></option></term>
         <listitem><para>
           Define the filename to append the account to. The default is
           <filename>cmdctl-accounts.csv</filename> in the system config
           directory.
         </para></listitem>
 }}}

 This is not only for "appending" the account any more.

 '''tests/Makefile.am'''

 - I think this should be removed:
 {{{
 # If necessary (rare cases), explicitly specify paths to dynamic libraries
 # required by loadable python modules.
 #LIBRARY_PATH_PLACEHOLDER =
 #if SET_ENV_LIBRARY_PATH
 #LIBRARY_PATH_PLACEHOLDER +=
 $(ENV_LIBRARY_PATH)=$(abs_top_builddir)/src/lib/cryptolink/.libs:$(abs_top_builddir)/src/lib/dns/.libs:$(abs_top_builddir)/src/lib/dns/python/.libs:$(abs_top_builddir)/src/lib/cc/.libs:$(abs_top_builddir)/src/lib/config/.libs:$(abs_top_builddir)/src/lib/log/.libs:$(abs_top_builddir)/src/lib/util/.libs:$(abs_top_builddir)/src/lib/exceptions/.libs:$(abs_top_builddir)/src/lib/util/io/.libs:$(abs_top_builddir)/src/lib/datasrc/.libs:$$$(ENV_LIBRARY_PATH)
 #endif
 }}}

 - and maybe this one too:
 {{{
 #CLEANFILES = test-keyfile.pem test-certfile.pem
 }}}

 '''b10-cmdctl-usermgr_test.py'''

 - general comment: isn't it that hard to instantiate `UserManager` in
   the test and directly check its methods, i.e., without the help of
   other process?  In this case the overhead of invoking processes may
   not be severe, but it's generally better to avoid such indirect
   helper in unittests.
 - I guess prompt_xxx aren't tested probably because it requires UI,
   but it seems to be at least possible to test post-input validation
   by mocking getpass.getpass() and input().

 '''bind10_config.py.in'''

 - Maybe we should describe SYSCONFPATH around here:
 {{{
     # LIBEXECPATH: Paths to programs invoked by the b10-init process
 ...
     #  tree the programs in the tree (not installed ones) will be used.
 }}}

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