Opened 7 years ago

Closed 7 years ago

#2713 closed defect (fixed)

b10-cmdctl-usermgr's prompts should be revisited

Reported by: muks Owned by: jelte
Priority: medium Milestone: Sprint-20130305
Component: ~cmd-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 3.8 Internal?: no

Description

The following is a session with b10-cmdctl-usermgr:

[muks@totoro bind10]$ sbin/b10-cmdctl-usermgr 
Desired Login Name:muks
Choose a password:
Re-enter password:

 create new account successfully! 

continue to create new account by input 'y' or 'Y':
[muks@totoro bind10]$ 

Try using it. This is less than ideal. The prompts and messages have to be rewritten. There should also be a way to pass these using the command-line.

Help looks like this:

[muks@totoro bind10]$ sbin/b10-cmdctl-usermgr -h
Usage: usermgr [options]
           -h, --help 	 Show this help message and exit
           -f, --file 	 Specify the file to append user name and password
           -v, --version	 Get version number
           
[muks@totoro bind10]$ 

(Note the inconsistency in program name.)

These should be addressed quickly as running b10-cmdctl-usermgr is now mandatory after #2641.

Subtickets

Change History (17)

comment:1 Changed 7 years ago by muks

Here is an example with better messages:

Desired username:<space><wait><echo-username>
Desired password:<space><wait><don't-echo-password>
Account was successfully created.
Account could not be created: <error>.

There's no need for "continue to create new account". If the user wants multiple accounts, they can run the tool again.

Options:

Usage: b10-cmdctl-usermgr [options] <path-to-accounts-file>
           -h, --help 	 Show this help message and exit
           -v, --version	 Get version number
           -c, --create          Create a new file (default is to update existing one)
           -u, --username=<username>
           -p, --password=<password>

If the user already exists, the user's password is updated. We could improve the options to also allow for deleting users.

This is just one example. Others can review it and suggest changes.

comment:2 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130305

comment:3 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:4 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

It's ready for review.

I don't think I left much of the original code intact, so it might be better to review the code as if it was new.

I added unit tests (there were none), however, they are really more 'system-level', as in they don't care about the actual internal methods (and don't call any of them directly), just the behaviour of the tool, and therefore only call it as an external program (with one exception). For such a simple tool (and methods that aren't called from other sources) I think that makes more sense.

I did change the command-line arguments from the proposal, instead of a number of options I added three arguments, two of which are optional (username and password).

While I was rewriting it anyway it was just as easy to add in a few more options (some of these are other tickets):

  • you can now delete users as well as add them
  • there is a -q option that suppresses output (except when it prompts for data)
  • it defaults to sysconfdir/package/cmdctl-accounts.csv instead of cwd

It also has better error checking and more graceful error handling than the original.

comment:5 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 follow-up: Changed 7 years ago 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:
        #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):
                    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.
    

comment:7 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:8 in reply to: ↑ 6 Changed 7 years ago by jreed

Replying to jinmei:

  • 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)?

I agree we should discourage this. I should have mentioned earlier but maybe we shouldn't have added that feature that way in the first place. A common way is to refer to some named pipe, or file descriptor, or a file that contains the passphrase, etc.

comment:9 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to 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:
        #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

added

  • the body of with could be simplified as follows (probably a matter of taste though):
                    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

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jinmei

Replying to 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"
            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:
                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.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

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"
            self.new_getpass = new_getpass
    
    same PrintCatcher.orig_stdout_write for attributes for OverrideInput, TestUserMgr.usermgr_module, TestUserMgr.usermgr etc.


done

  • TestUserMgr.run_check: parameter stdin isn't documented.

actually I though I was going to use that functionality, but didn't end up doing it, so I removed the parameter again

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

that is right, because it makes no sense :) removed, and updated the docstrings of the two next ones

  • 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").

TBH i prefer the explicit nonlocal in this case (and keep scope to this specific function)
i guess making it a dict would work as well (called = { "times": 0 }) but then we'd really be getting dirty :p

  • test_prompt_for_password_different: also a matter of taste, you can use '+= 1' here:

doh of course

  • test_prompt_for_password_empty: same comments apply.
  • test_prompt_for_password_empty: you could avoid variables options and args.

ahyes sorry, forgot that one

  • test_prompt_for_user: same comments about nonlocal apply.

and the same answer does as well ;)

comment:13 in reply to: ↑ 12 Changed 7 years ago by jinmei

Replying to jelte:

docstring for test_prompt_for_password_empty is a copy of
test_prompt_for_password_different, which should be correct.

With fixing it I think the branch can be merged (I don't see the need
for checking the change).

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:15 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 3.8

comment:16 Changed 7 years ago by shane

Note that when this branch is merged, perhaps we can change b10-cmdctl-usermgr from yellow to green on the ComponentStatus page?

comment:17 Changed 7 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

thanks! merged, closing ticket.

Note: See TracTickets for help on using tickets.