Twisted Perspective Broker anonymous login support

[carlosedp]

Here follows the patch for pb.py, a testcase some changes to documentation and a usage example for anonymous login based on pbserver and pbclient.

[carlosedp]

Unified diff for the changes made to all files.

[glyph]

Not a requirement of the 2.5 release.

[exarkun]

review ticket -> highest priority

[glyph]

First off, I don't understand why this is necessary at all. You can already supply your own root object, and just start calling methods on it. That doesn't require integration with cred's anonymous logins.

However, I'm assuming you have some compelling use case, so I've also reviewed the patch. It needs quite a few fixes.

There is trailing whitespace all over this patch. (That is, whitespace at the end of a line that serves no purpose.) Please remove it.

Please format all new docstrings like this:

"""
Docstring
"""

In login's docstring, you say "L{twisted.cred.credentials.IAnonymous} are supported" immediately after the docstring says "only credentials implementing IUsernamePassword are supported. Please change that so that it makes sense, and uses full sentences.

Do not do isinstance checks in login. First of all, it's bad style, second, it makes the docstring incorrect. If you must, ask if IAnonymous.providedBy(credentials). Definitely don't do the second check at all; it should fail if given incorrect credentials, not return a Deferred which will hang forever.

remote_loginAnonymous requires documentation for its parameters and return type. A deeper explanation wouldn't hurt either.

_PortalAnonymousChallenger needs docstrings for its methods. It also shouldn't implement IAnonymous, that's a marker interface.

The tests need docstrings.

The test method name should be test_anonymousLogin, not testAnonymousLogin.

[exarkun]

A few more comments, but first, thanks for updating the documentation and adding test coverage for this feature. I wish all patches looked this good. :)

One thing I notice about the actual change that glyph didn't mention is that it seems to be doing more work than it needs to. I don't think you need _PortalAnonymousChallenger: remote_anonymousLogin can just do the portal.login call. This will also let you drop all of the username stuff from the anonymous login path, and it also reduces the number of network round-trips required for anonymous login.

[exarkun]

Just noticed this is essentially a duplicate of #439. Not going to close this since there's so much going on here, but when this ticket gets closed, that one should as well.

[carlosedp]

Made some changes to the patch, the _PortalAnonymousChallenger is not needed anymore as the already existent _PortalAuthChallenger can be used.

I could not figure out a way to put all portal.login stuff into remote_loginAnonymous because the class needs to implement IAnonymous so portal can do it´s providedBy call. It can be done if the class PortalWrapper can have a implements(IAnonymous) but I dont know if there are problems in this approach.

I fixed the documentation and docstrings.

Attached is the new patch from the source tree (not from my last patch).

[carlosedp]

After some extensive testing, I found that the last modification described added 26/12, inserted a problem when using servers with multiple checkers.

If I have a server that implements both password and anonymous checkers, sometimes the auth user gets anonymous perspective or is not allowed to login.

I made some changes and created a new patch pbAnonymous_patch3.diff that is attached.

Also I created a new testcase to address this issue.

[carlosedp]

Patch address Anonymous logins and handle problems described in last post.

[exarkun]

Code for this is in pb-login-2312 now. Working on cleaning it up some.

[exarkun]

Made some further changes, mostly to the documentation. Still a few changes necessary (eg docstring for remote_loginAnonymous). It might also make sense to generalize the dispatch of login via different credentials, but I'm not sure I'll actually bother doing that.

[exarkun]

Okay, perhaps this is completed. See pb-login-2312

[therve]

This looks great, awesome effort of documentation. I have only a few aesthetic remarks:

• Update the copyright notices
• there are a few new lines in pb.py over 80 characters. Don't bother to correct all the file, just don't add new ones :).
• typo in the pb-cred file: 'pbAnonServer.py is is'.
• make the new examples executable
• if possible, correct the shebangs of the pb examples to be identical

That's it!

[exarkun]

Fixed, except for the last point. Not sure what you mean, they're already the same as each other. They only differ from the other examples in whitespace. Is that it?

[therve]

Hum, you're right, I don't know what I've looked at. Please merge!

[exarkun]

Hmm, not merging for now due to security concerns.

[therve]

Is there some discussion around that? For the record, the security problem is that if you had a portal that allows anonymous login, it will being to work with the new code whereas it didn't work before. I don't really think that's a problem, but the feature looks good. Maybe we can add another flag somewhere to be sure anonymous has be setup explicitely ?

[exarkun]

Sorry for not responding sooner. Discussion was all offline, thanks for summarizing the issue.

I'm thinking about something like PBServerFactory(allowedCredentials=(IUsernamePassword, IAnonymous, ...)) now. Anyone have any thoughts on that? It seems as minimally invasive as we can do without completely papering over the security issues.

[therve]

Replying to exarkun:

I'm thinking about something like PBServerFactory(allowedCredentials=(IUsernamePassword, IAnonymous, ...)) now. Anyone have any thoughts on that? It seems as minimally invasive as we can do without completely papering over the security issues.

Pro: it does solve the issue, and it's simple, so probably easily testable

Cons: it sets a bad precedent (I think) of link between server factories and authentication.

If done, we could set a plan to deprecate this soon? That could look strange, but I don't have a better idea.

[therve]

While hanging around FTP code, I saw that FTPFactory has a flag allowAnonymous. Isn't exactly what we want?

[exarkun]

The branch bitrotted somewhat in the mean time. I've fixed various failing tests and improved documentation for the changes and new code.

I'm not really any closer to an idea for a good solution to the security problem mentioned above than when I last thought about this branch. I think I'd be fine merging the branch as-is now, without providing a way to restrict the interfaces which can be used.

There might be some more cred documentation to write, but I agree with your con above. The portal object is really the thing where this configuration belongs. We might want to emphasize that more. In practice, I don't think anyone is really going to be bitten by this.

[therve]

I was OK before, and I am still. My last comments:

• you renamed MyRealm to TestRealm, but kept MyPerspective and MyView.
• the error management in pbAnonClient.py is wrong (errback with 2 parameters).

[exarkun]

I fixed the example. I renamed MyRealm since I had to change it. Maybe "TestRealm" isn't much better than "MyRealm", but "My" gives me C++ flashbacks. ;)

Is this cool to merge now?

[therve]

Looks cool please merge.

[exarkun]

(In [21124]) Merge pb-login-2312-2

Author: carlosedp, exarkun Reviewer: glyph, exarkun, therve Fixes #439 Fixes #2312

Add support for IAnonymous credentials to PBServerFactory and PBClientFactory. It is now possible to attempt a cred login anonymously using these classes. The cred portal is ultimately responsible for determining if the login should succeed or fail, as is the general case.