BIND 10 #2713: b10-cmdctl-usermgr's prompts should be revisited
BIND 10 Development
do-not-reply at isc.org
Tue Feb 26 21:55:08 UTC 2013
#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
-------------------------------------+-------------------------------------
Changes (by jelte):
* owner: jelte => jinmei
Comment:
Replying to [comment:6 jinmei]:
> '''others/general'''
>
> - 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.
> - the "run" script doesn't work:
> I've committed a suggested fix: 5836dc5
Thanks
> - you can avoid the need for the variable result:
> {{{#!python
> #result = main()
> #sys.exit(result)
> sys.exit(main())
> }}}
tbh I did that on purpose to make it less side-effect-y, but ok
> - should we care about broken csv files (missing field, etc)?
>
for this specific tool, i don't think it needs to fail on missing fields,
but i do make it fail gracefully if it cannot even parse the csv; Added
tests and one fix
> - 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)?
>
OK, added something to help and manpage
> - 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.
>
OK, used an ordereddict though, to preserve order of additions
> - 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?
>
I am not opposed, and really have no strong opinion of it, but I sent a
message to -dev, since if we're renaming, we might as well rename to
something that has a bit of consensus :p
> - 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.
>
all of them except run, inherently since this is not a file to be
imported. But added __.
> '''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?
>
this is like the one line i did not touch :p
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.
> '''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.
added
> - 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.
>
not anymore since user_info is now a dict :)
updated test
> '''save_userinfo'''
>
> - maybe a matter of taste, but temporary 'writer' variable can be
> omitted.
>
also moot now
> '''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.
>
i knew i had to miss one :p updated
> '''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
> }}}
>
ack
> - and maybe this one too:
> {{{
> #CLEANFILES = test-keyfile.pem test-certfile.pem
> }}}
>
ack
> '''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 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().
>
i briefly experimented with writing to subprocess.stdin but getpass()
really doesn't like not having control over the tty :) Added tests that do
override methods (and classes to do so)
> '''bind10_config.py.in'''
>
> - Maybe we should describe SYSCONFPATH around here:
added a short description
--
Ticket URL: <http://bind10.isc.org/ticket/2713#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list