Ticket #2312 (closed enhancement: fixed)

Opened 4 years ago

Last modified 3 years ago

Twisted Perspective Broker anonymous login support

Reported by: carlosedp Owned by: exarkun
Priority: highest Milestone:
Component: pb Keywords:
Cc: exarkun, therve Branch:
Author: Launchpad Bug:

Description

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.

Attachments

twisted_PBanonymous.diff Download (8.1 KB) - added by carlosedp 4 years ago.
Unified diff for the changes made to all files.
pbAnonymous_patch2.diff Download (10.0 KB) - added by carlosedp 4 years ago.
pbAnonymous_patch3.diff Download (11.3 KB) - added by carlosedp 4 years ago.
Patch address Anonymous logins and handle problems described in last post.

Change History

Changed 4 years ago by carlosedp

Unified diff for the changes made to all files.

  Changed 4 years ago by carlosedp

  • keywords review added; pb anonymous spread removed

  Changed 4 years ago by glyph

  • milestone Twisted-2.5 deleted

Not a requirement of the 2.5 release.

  Changed 4 years ago by exarkun

  • priority changed from normal to highest

review ticket -> highest priority

  Changed 4 years ago by glyph

  • keywords review removed
  • priority changed from highest to normal
  • owner changed from glyph to carlosedp

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.

  Changed 4 years ago by 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.

  Changed 4 years ago by exarkun

  • cc exarkun added
  • component changed from core to pb

  Changed 4 years ago by 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.

  Changed 4 years ago by 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).

Changed 4 years ago by carlosedp

  Changed 4 years ago by carlosedp

  • keywords review added

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.

Changed 4 years ago by carlosedp

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

  Changed 4 years ago by carlosedp

  • owner changed from carlosedp to glyph
  • priority changed from normal to high

  Changed 4 years ago by exarkun

  • owner changed from glyph to exarkun
  • keywords review removed

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

  Changed 3 years ago by 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.

  Changed 3 years ago by exarkun

  • keywords review added
  • owner exarkun deleted

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

  Changed 3 years ago by therve

  • cc therve added
  • owner set to exarkun
  • keywords review removed

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!

  Changed 3 years ago by exarkun

  • priority changed from high to highest
  • keywords review added
  • owner changed from exarkun to therve

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?

  Changed 3 years ago by therve

  • owner changed from therve to exarkun
  • keywords review removed

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

  Changed 3 years ago by exarkun

Hmm, not merging for now due to security concerns.

  Changed 3 years ago by 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 ?

follow-up: ↓ 20   Changed 3 years ago by 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.

in reply to: ↑ 19   Changed 3 years ago by 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.

  Changed 3 years ago by therve

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

  Changed 3 years ago by exarkun

  • owner exarkun deleted
  • keywords review added

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.

  Changed 3 years ago by therve

  • keywords review removed
  • owner set to exarkun

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

  Changed 3 years ago by exarkun

  • keywords revew added
  • owner changed from exarkun to therve

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?

  Changed 3 years ago by exarkun

  • keywords review added; revew removed

  Changed 3 years ago by therve

  • owner changed from therve to exarkun
  • keywords review removed

Looks cool please merge.

  Changed 3 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(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.

Note: See TracTickets for help on using tickets.