Add support for custom user passwords

[robvdl]

Adds support for user passwords, passwords must be encrypted using crypt, for example:

mkpasswd --method=sha-512 'password'

Includes tests and documentation update.

[codecov-io]

Codecov Report

Merging #60 into master will increase coverage by 0.56%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   90.58%   91.15%   +0.56%     
==========================================
  Files          41       41              
  Lines         935      938       +3     
==========================================
+ Hits          847      855       +8     
+ Misses         88       83       -5

Impacted Files	Coverage Δ
lxdock/conf/schema.py	100% <ø> (ø)	⬆️
lxdock/guests/base.py	100% <100%> (ø)	⬆️
lxdock/container.py	80.68% <100%> (+2.23%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faeb780...90707bd. Read the comment docs.

[ghost]

I'm a little late to the party regarding this, but I don't understand the use case for requiring passwords to be hashed. That's not very convenient and I don't see in what cases such "security" would be required. I'm guessing that the vast majority of users will end up adding a comment over the hash with the cleartext password.

[ghost]

Otherwise, this code looks good to me. 3 integration tests seem a little excessive though. These tests are expensive, computing-wise and in that specific case, they don't really test that users were properly created (with the proper home and password). They only test that containers are able to spawn without error with this config. I can't blame you for that because we don't have proper tooling right now to conveniently write tests that would tests interesting things, such as "was my user created with the proper password?" and I wouldn't be sure where to start to write such as test.

But anyway, because we already have that kind of tests at https://github.com/lxdock/lxdock/blob/master/tests/integration/test_container.py#L86 , those 3 tests I think could be combined into one. You don't even need to run shell() because that's not testing anything that hasn't been tested already.

[robvdl]

@hsoft the reason passwords require to be hashed is simple, this is what the useradd command uses in the first place for the -p parameter (see man useradd), the useradd command doesn't allow plaintext passwords and otherwise if lxdock did have them in plaintext then it would need to encrypt them before calling useradd anyway.

Regarding the 3 integration tests, I actually didn't write any integration tests at first, but I couldn't get the commit to pass codecov as the coverage had reduced 0.63% that was enough for it to fail codecov which was a little irritating (and why I was complaining a little about the stringent codecov requirements).

I actually started them off in a single integration test anyway, then split them, but I will do as you ask and put them back into one test again and remove the shell part.

[robvdl]

Essentially all that I was trying to test in this integration test, was to test the container._setup_users() method because that was never tested in the first place so lacked coverage, and by me adding those few lines to this method is why codecov failed.

Even if I was to write a test for container._setup_users(), be it an integration or unit test, all it would be testing is if container._setup_users() was invoking self._guest.create_user() correctly.

There isn't a huge amount of value in that either other than just to add coverage for container._setup_users(). As there is already a unit test for guest.create_user(), I didn't really want to repeat that as another unit test for container that does the same thing just to get it past coverage.

[ghost]

I don't think we need to care much about the code coverage figure. It's often a bad representation of the quality of tests, especially if we do contrived testing for the sole purpose of raising that coverage figure.

[ellmetha]

This looks good. @robvdl regarding the test coverage requirement, 'tis not a big deal. I think that we could've been fine without an integration test here, but since you've added three of them I think too that you could combine them into one test.

[robvdl]

Changed to one test and moved to test_container.py, changed what is being tested a little

[ghost]

Good work on this change to your test. This looks good to me.

As for the password vs. password hash thing, I understand that it's more work to support plaintext passwords out of the box. I'm fine with merging this PR as-is and delay plaintext support.

Thanks again for this PR @robvdl