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):

 Replying to [comment:9 jelte]:

 > > - maybe we need a changelog entry for this task.
 >
 > Ack.
 >
 > [func]* jelte
 > The b10-cmdctl-usermgr has been updated; it now defaults to the same
 accounts file as b10-cmdctl defaults to. It can now be used to remove
 users from the accounts file as well, and it now accepts command-line
 arguments to specify the username and password to add or remove, in which
 case it will not prompt for them.

 This looks okay, but we might want to note that it's not recommenced
 to specify the password in command line here, too.

 > > '''gen_password_hash'''
 > >
 > > - what's the rationale of the magic numbers?
 >
 > 33 and 127 seemed quite obvious to me, but taking a quick look at
 randint() i do believe it should have been 126 (proving your point i guess
 :))
 >
 > changed to use ord.
 >
 > The character set was chosen to be printable afaict (though really, i am
 not sure why this is not done through hex or base64)
 >
 > The size was, as far as i know, arbitrary.
 >
 > We could change either of these, esp. input set. But that I do consider
 out of scope here, and it is a potentially problematic backwards
 incompatible change that should be handled in a separate ticket imo.

 When it's related to security I don't like to do something arbitrary
 that may be seemingly secure, but in this case I'm okay with excluding
 it from this ticket.  Also, this specific case wouldn't be worse than
 sufficiently randomized human-created password, so it's probably not
 really bad.

 > > '''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.
 >
 > it's not (though it initially was a potentially tricky when there was no
 class yet), but as i already said, the behaviour of the private methods
 here doesn't matter, as they are only for this one small tool anyway. I
 guess this also comes down to the question whether you should extensively
 test private methods or not. For the issue below I did make exceptions, I
 guess we can discuss whether or not all the other ones need the same
 treatment :)

 I generally prefer tests with simpler setups (and for dynamic
 languages like Python it's moot whether it's a good idea to test
 private methods anyway; in some sense nothing is private).  But in
 this specific case I'm okay with the current approach.

 As for the naming issue, I don't think it's a blocking issue for this
 ticket.  If it's worth a discussion, keep it and maybe create a ticket
 later.  If the result is to not change it, that's fine too.

 Comments on the revised branch: all minor, but there are some non
 trivial number of them, so we'll probably need another round of cycle.

 '''b10-cmdctl-usermgr_test'''

 - I'd make new_getpass "private"
 {{{#!python
         self.new_getpass = new_getpass
 }}}
   same `PrintCatcher.orig_stdout_write` for attributes for
   `OverrideInput`, `TestUserMgr.usermgr_module`,
   `TestUserMgr.usermgr` etc.


 - `TestUserMgr.run_check`: parameter stdin isn't documented.

 - test_default_file: I don't understand this comment:
 {{{
         Check that the prompting methods do verification
 }}}

 - test_prompt_for_password_different: maybe a matter of taste, but you
   can avoid `nonlocal` if you define getpass_different_called as
   object's attribute (self.getpass_different_called) (and in that case
   I'd also make it "private").
 - test_prompt_for_password_different: also a matter of taste, you can
   use '+= 1'  here:
 {{{#!python
             getpass_different_called = getpass_different_called + 1
 }}}
 - test_prompt_for_password_empty: same comments apply.
 - test_prompt_for_password_empty: you could avoid variables options
   and args.
 - test_prompt_for_user: same comments about nonlocal apply.

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