use python 2.5 'spwd' module instead of z3p secret 'shadow' module when available

[glyph]

It might even make sense to get rid of the 'shadow' module and just include a 2.3 backport of spwd within Twisted, but I'll leave it up to you to make another ticket. However, given that this functionality is actually maintained somewhere, let's use the maintained version...

[exarkun]

I would expect the PAM functionality to almost entirely obsolete any direct support of /etc/passwd or /etc/shadow. Is this something we actually need to continue to maintain?

[exarkun]

Also, whatever third-party module is used should be documented as a dependency. (See #1633)

[jkk]

Replying to exarkun:

I would expect the PAM functionality to almost entirely obsolete any direct support of /etc/passwd or /etc/shadow. Is this something we actually need to continue to maintain?

It will happen eventually, but currently PyPAM is in Alpha stage and it doesn't work under python 2.5 .I wouldn't yet entirely rely on it as the only authentication module supported.

[jkk]

Replying to jkk:

Replying to exarkun:

I would expect the PAM functionality to almost entirely obsolete any direct support of /etc/passwd or /etc/shadow. Is this something we actually need to continue to maintain?

It will happen eventually, but currently PyPAM is in Alpha stage and it doesn't work under python 2.5 .I wouldn't yet entirely rely on it as the only authentication module supported.

Ok, so it will work on python 2.5.I've just sent a necessary fix to PyPAM maintainer.

[z3p]

(In [26271]) Branching to 'spwd-3242'

[z3p]

Ready for review, I guess.

[glyph]

You should write some tests that mock the appropriate functions (seteuid, setegid, getspwnam) here. Ideally you could break it up so that the front-end/back-end separation with UNIXPasswordDatabase is done via an attribute that is an (optional, for compatibility purposes) argument to its constructor or something.

[z3p]

I refactored that class to take an option argument of getpwnam functions, and some tests for that class and the new functions.

[glyph]

Erm... I don't actually see any of that in the branch. I only see changes to twisted/conch/checkers.py, as a matter of fact. Is there another branch with the wrong ticket number on it somewhere? If so, want to merge forward with a correct number so the 'branch' field picks it up?

[z3p]

Oh, looks like the commit didn't actually make it into SVN. Should be there now.

[glyph]

Conflicts in both files when merged to trunk. Looks like you need that merge forward anyway :).

[z3p]

(In [26921]) Branching to 'spwd-3242-2'

[z3p]

This time for sure!

[glyph]

Definitely an improvement. I like the large number of duplicated seteuid lines which are deleted ;-).

1. getpwnam_functions
   1. its name violates the coding standard.
   2. it isn't documented. The class needs a docstring which has an @ivar.
   3. why have the 'None' condition at all? is the list intended to be mutable per-instance? wouldn't it be simpler to have it be a tuple, and have the signature be def __init__(self, getpwnamFunctions=(getpwnam_passwd, getpwnam_shadow))?
2. getpwnam_passwd and getpwnam_shadow
   1. names violate the coding standard
   2. 3 blank lines between toplevel functions, please
   3. neither one has a docstring
3. test_checkers
   1. We've been moving towards a more declarative, active-voice style of writing docstrings, one which is both briefer and more amenable to documentation generation. For example, instead of "When foo is true, bar() should baz without raising an exception.", "bar() bazzes when foo.". Avoid pronouns, except where the antecedent is clearly visible in the docstring itself. Avoid words like "should" and "verifies" and so on. Just say what the expected behavior is, not that the test verifies it or that it should be so.
   2. test_verifyCryptedPassword and test_verifyCryptedPassword_md5 have blank docstrings.
   3. test_getpwnam_passwd, test_getpwnam_shadow, test_default_checkers, test_pass_in_checkers, test_verify_password, test_fail_on_KeyError, test_fail_on_bad_password, and test_loop_through_functions all have names which violate the coding standard.
   4. assertLoggedIn and assertUnauthorizedLogin are a bit vague. "is a valid login"? What does that mean? What constitutes validity? I see a @type, but what about a @param? The bit where you need to return its result or it might not actually do anything seems pretty important, too.
   5. rather than touching a bunch of unrelated lines by changing the import from importing classes to importing modules, why not just `from twisted.conch.checkers import pwd as pwdcheckers, shadow as spwdcheckers?
   6. If you import individual modules, patch() calls can be less invasive. Since spwd and pwd aren't particularly widely used, this isn't of terribly much practical concern in this case, but it's a good habit to get into: patch as narrowly as possible. In this particular case, getpwnam and getspwnam are the only functions that are used in checkers, so you could import them directly.
   7. there are a couple of >80 character lines.
   8. please put 2 blank lines between methods and 3 between classes.
   9. test_loop_through_functions' docstring is a bit vague. Phrase it a bit more positively, i.e. "UNIXPasswordDatabase.requestAvatarId loops through each getpwnam function associated with it and returns a Deferred which fires with the result of the first one which returns a value rather than raising exception Foo."

[z3p]

Other than 3.5 and 3.6, the other comments are addressed in the branch.

3.5: I switched to importing the module because otherwise I'm importing a ton of individual things and that seemed more cluttered. If it's really a problem I can do it that way, though.

3.6: I'm not sure what this means; I'm only patching individual functions; I'm not sure how to patch more narrowly than that. Perhaps an example would help.

I'm going to put this back up for review, but if those comments need to be addressed, feel free to bounce it back.

[therve]

*

     import Crypto.Cipher.DES3
+    Crypto
 except ImportError:

Please don't do that. I know that pyflakes complain, but I'd rather see it fixed in pyflakes.

*

+        credentials = SSHPrivateKey('test', 'ssh-rsa',
+                                    keydata.publicRSA_openssh, 'foo',
+                                    keys.Key.fromString(
+                keydata.privateRSA_openssh).sign('foo'))

Indentation is broken. Maybe something like that:

        credentials = SSHPrivateKey(
            'test', 'ssh-rsa', keydata.publicRSA_openssh, 'foo',
            keys.Key.fromString(keydata.privateRSA_openssh).sign('foo'))

It's visible in a couple of place: the second indentation should always be 4 spaces, not 8.

*

+        skip = 'cannot run without crypt module'

Do you mean pwd here?

*

+    elif not getattr(os, 'O_NOCTTY', None):
+        skip = 'cannot run without os.O_NOCTTY'

Please check for is None explicitely.

*

+    @param username: the username of the user to return the passwd database
+    information for.

The second line should be indented (and the following as well).

*

+    A checker which validates users out of the UNIX password databases, or a
+    databases a compatible format.

"a database in a compatible format" ?

This is a pretty nice enhancement. I suppose it depends on #3822 before being merged?

Thanks!

[z3p]

(In [27081]) Branching to 'spwd-3242-3'

[z3p]

It didn't actually depend on #3832; that's why I had to have the os.O_NOCTTY check. Now that it's merged, I can remove that check.

Other comments are addressed, I believe.

[glyph]

Regarding my point 3.5, never mind. An example regarding point 3.6 is forthcoming.

[glyph]

Okay. So, regarding patching more narrowly - reading my comment right now, I can see it doesn't make much sense, so here's a much-expanded version.

The tests are currently passing os, checkers.shadow (i.e. "spwd"), and pwd as arguments to "patch". This means that any module that does import os (or import spwd or import pwd) will be using the replaced functions for the duration of the test.

As I said, the functions being patched in this case are unlikely to be used by trial, but, "unlikely" is the not same as "certainly not". Certainly other functions in the os module are.

This is a pre-existing problem in this test module, so I don't think it's necessarily the responsibility of this branch to fix. However, as a matter of habit, patching should always be done so that it affects as few modules as possible, to minimize the effect of future features that trial might have. (Imagine a tool which let you run every test as a different effective user-ID. I don't know why you'd want such a thing... but you could have it.)

Taking the specific example of test_getpwnamShadow, instead of this:

self.patch(checkers.shadow, 'getspnam', lambda u: (0, 1, 2, 3))
self.patch(os, "geteuid", self.mockos.geteuid)
self.patch(os, "getegid", self.mockos.getegid)
# ... et cetera ...

you could do this:

self.patch(checkers, "os", self.mockos)
class MockShadow(object):
    def getspnam(self, u):
        return (0, 1, 2, 3)
self.patch(checkers, "shadow", MockShadow())

(of course you could put MockShadow at module level)

The second example there only patches the names actually used in the checkers module, so other modules which import the same names are not affected.

[mwh]

I'm not sure what the status of this branch is. Was glyph's comment intended as a review?

[glyph]

My comment was an explanation of a point in an earlier review, which z3p said he didn't understand in comment 15.

[exarkun]

Hopefully z3p can respond to glyph's point 3.6 in comment 15 now.

[exarkun]

(In [29696]) Branching to 'spwd-3242-4'

[exarkun]

(In [29711]) Branching to 'spwd-3242-5'

[exarkun]

(In [29836]) Branching to 'spwd-3242-6'

[z3p]

(In [33143]) Branching to 'swpd-3242-7'

[z3p]

(In [33144]) Branching to 'spwd-3242-7'

[z3p]

(In [33147]) address comment 3.6

Refs #3242

[z3p]

Ready for review.

Buildbox results will be at  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/spwd-3242-7

[glyph]

Reviewing.

[glyph]

WELCOME BACK TO THE STAGE OF HISTORY

1. Looks like  the API documentation builder found some problems; you should fix those.
2. assertUnauthorizedLogin incorrectly describes its @rtype as Deferred; its return type is actually NoneType. (Which is good, because new tests should not spin the reactor.)
3. This code still uses patch an awful lot. I can see you responded to my previous review, and the patches are all much less aggressive, but in the future you may want to consider attributes looked up on instances in more places than temporarily-replaced attributes of modules or classes. (Nothing to be done for this ticket, just be aware of that in the future.)

Otherwise this looks pretty good, thanks: if you can fix points 1 and 2, I think it's ready to land!

[thijs]

ShadowDatabase in t.p.fakepwd also needs an @since marker.

[exarkun]

Looks like the API documentation builder found some problems; you should fix those.

Dang. I'd really like to teach pydoctor about the standard library. :/

[z3p]

I've addressed point 2, and added the @since marker.

I'm not sure what the right answer is for point 1. Should I untag the module in the docstring, or is there some other fix to make?

[z3p]

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/spwd-3242-7

They're green, including the API builder (thanks to exarkun for the pointer).

[jesstess]

The API changes look good, the rtype change looks good, @since looks good, and I think this is ready to merge!

[z3p]

(In [33231]) Merge branch spwd-3242-7: use python 2.5 'spwd' module instead of z3p secret 'shadow' module when available

Author: exarkun, z3p Reviewer: glyph, therve, thijs, jesstess Fixes: #3242

Python 2.5 includes a standard library module to interact with the /etc/shadow password database. This updates Twisted to that module, rather than the module z3p hacked together many, many years ago.