XmlStream should be convenient to use in a server context where there should be one authenticator per connection

[ralphm]

As the Jabber implementation has been mostly focussed on client side code (Jabber clients and Jabber components). The concept of authenticators has been used to initialize an XmlStream to the point that the connection can be used to exchange so-called XML stanzas. However, up to now, an already instantiated authenticator was passed to XmlStreamFactory, which in turn passes it to the XmlStream on instantiating in buildProtocol. This complicates using the same concept for server-side use, regarding #2320 and #3407.

This ticket attempts to change how authenticators get instantiated, by passing the authenticator class and its parameters to the factory, leaving it to the XmlStream instance to instantiate the authenticator. This makes adding a factory for accepting XML streams trivial.

[ralphm]

(In [24846]) Branching to 'jabber-authenticators-3435'

[ralphm]

This branch splits out the work that was incorporated the work attached to #3407. Please review.

[exarkun]

Two major points:

• There are a lot of changes which are not backward compatible.
• The new class, XmlStreamServerFactory, is untested.

If the change is made in a backward compatible way, then most of the patch would disappear (or could at least be relegated to an "update the test suite to the new, superior API" ticket if we wanted).

It'd be great to have some explanation of either the goal of these API changes or some documentation about the new API (basically, something that explains why - the how is clear enough in this patch, but it's hard to understand by itself, since nothing makes use of it in a way that's not a trivial adjustment of the old API).

[ralphm]

I am not sure if we need to preserve backwards compatibility for this API. The assumption here is that authenticators are really only instantiated by helper functions like t.w.p.j.client.basicClientFactory. I changed all those to match.

That said, of course XmlStream could check if the passed (first) argument is an instance or a class, and keep the old behaviour for the former.

I thought the intention of this patch was explained in this ticket's description, but maybe that is not clear enough?

As for tests for XmlStreamServerFactory, would just testing for its bases and the value of protocol be sufficient?

[itamar]

Why require passing in a class? Why not just a (factory) callable?

[exarkun]

Replying to ralphm:

I am not sure if we need to preserve backwards compatibility for this API. The assumption here is that authenticators are really only instantiated by helper functions like t.w.p.j.client.basicClientFactory. I changed all those to match.

The names are all public (don't start with underscores), though, so the usual compatibility guarantees apply. The signature of XmlStream.__init__ is part of this, but also keep in mind that subclasses might be calling up to associateWithStream or overriding that method and expecting it to be called.

That said, of course XmlStream could check if the passed (first) argument is an instance or a class, and keep the old behaviour for the former.

That may be fine; it depends on what exactly worked before and what is intended to work now. If the two can be distinguished unambiguously, great, otherwise a new API might be in order.

I thought the intention of this patch was explained in this ticket's description, but maybe that is not clear enough?

Part of what's not clear is why associateWithStream is being removed. The rest is about what kind of factory interface this code will benefit the most from. It would help if the events which can happen which involve these objects were documented along with all the information these events carry (for example, TCP connections have a server factory with doStart, doStop, and buildProtocol methods - doStart is called when the factory is used with a port which starts listening, doStop when that port is no longer listening, and buildProtocol when a connection is established - and the remote address of the connection is passed as an argument to buildProtocol. What can happen to an authenticator?). It might be sufficient to explain that stuff here so people not familiar with all the details of XMPP can more easily review the branch, but it may also be useful to document this either in docstrings or in a howto-style document that deals with writing XMPP servers with Twisted, since ideally it wouldn't be necessary to be an XMPP expert in order to use these APIs.

As for tests for XmlStreamServerFactory, would just testing for its bases and the value of protocol be sufficient?

Tests for its behavior would be better. Assertions about its __bases__ and protocol attribute are really just duplications of its implementation. The docstrings says "reconnecting client". Okay - test that it reconnects (except it doesn't seem to). It's protocol attribute is bound to XmlStream - okay, then perhaps its buildProtocol method should return an XmlStream instance?

[ralphm]

Ok, I refactored the authenticator changes so that it uses a factory function as itamar suggested and also is backwards compatible. I.e. XmlStream tries to call the passed argument, and if that fails uses the passed argument as the authenticator.

I'll address the server factory tests next.

As for documentation: yes I should make a document on that.

[ralphm]

Tests for XmlStreamServerFactory added. Please review.

[colin]

XMPPClientFactory is currently a proxy method that instantiates and returns a different object. This could lead to, among numerous other things, breaking inheritance (where the trend in Twisted for Factories is adaptable objects), as well as the instance not being the object you called. Ie: isinstance(myXMPPFactory, XMPPClientFactory) will break.

This should be something like...

class XMPPClientFactory(xmlstream.XmlStreamFactory):
    """
    Client factory for XMPP 1.0 (only).

    This returns a L{xmlstream.XmlStreamFactory} with an L{XMPPAuthenticator}
    object to perform the stream initialization steps (such as authentication).

    @see: The notes at L{XMPPAuthenticator} describe how the L{jid} and
    L{password} parameters are to be used.

    @param jid: Jabber ID to connect with.
    @type jid: L{jid.JID}
    @param password: password to authenticate with.
    @type password: L{unicode}
    @return: XML stream factory.
    @rtype: L{xmlstream.XmlStreamFactory}
    """

    def __init__(self, jid, password):
        xmlstream.XmlStreamFactory.__init__(self, XMPPAuthenticator(jid, password))

[ralphm]

Replying to colin:

XMPPClientFactory is currently a proxy method that instantiates and returns a different object. This could lead to, among numerous other things, breaking inheritance (where the trend in Twisted for Factories is adaptable objects), as well as the instance not being the object you called. Ie: isinstance(myXMPPFactory, XMPPClientFactory) will break.

This was raised on #twisted, too. But I don't see what this has to do with this particular ticket. That function has been in there for ages, the branch associated with this ticket doesn't touch it (anymore) and no one has complained about basicClientFactory either, which is like 5 years old now. If you still think it is a concern, file another ticket.

[glyph]

Trying to adjust ticket summaries to be actionable / clear statements of what's happening rather than just general notes about the area that a particular ticket addresses. Please feel free to correct if I have misunderstood.

[glyph]

The current check for the authenticator's authenticator-ness looks pretty bad for me. Consider:

>>> x()()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'x' object is not callable
>>> y()()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: y instance has no __call__ method

Introducing dependencies on the distinctions between new- and old-style classes seems ... inadvisable, at this moment in history.

There are trivially more portable/consistent ways to perform this test:

>>> callable(x())
False
>>> callable(y())
False
>>> callable(x)
True
>>> callable(y)
True

but I would prefer something much more specific and intent-revealing:

>>> from zope.interface import Interface, implements
>>> class IAuthenticator(Interface): ""
... 
>>> class Authenticator: implements(IAuthenticator)
... 
>>> IAuthenticator.providedBy(Authenticator)
False
>>> IAuthenticator.providedBy(Authenticator())
True

In general, in a case like this, I'd like to say that one style of calling or the other is preferred and the other is slated to be deprecated. Do you think it would be a good idea to deprecate passing the authenticator itself rather than the factory? I'd certainly prefer to have there only be one good way to do it, and passing a factory is more flexible.

However, it also seems to me that perhaps passing the authenticator itself is just fine, and it should be the server factory's job to accept the factory and construct it in buildProtocol. Right now, XmlStreamFactory has basically no functionality; it's not clear to me why it exists at all. Furthermore, there's no reasonable way for a user to intuit the way that they're required to construct an XmlStreamServerFactory. They have to look up its inheritance hierarchy, and then read the code or your mind: the args and kwargs attributes / arguments are not documented, and even if they were, you need to follow the API documentation through the protocol attribute to discover that you can pass an authenticator or a factory... and which one are you supposed to use for a server? Why?

If XmlStream remained unmodified, and the server factory were added with a clear signature (pass an authenticator factory, and only a factory, no checking required, to construct the authenticator for its connection) it seems like this would be a pretty straightforward (and even smaller than it already is) change.

[ralphm]

I agree that the detection of what the passed object is, is kind of ugly, and it would be better to use an interface for that. I kind of wonder why there is no IAuthenticator, now I think about it.

From within this branch there are three different factories: t.w.x.xmlstream.XmlStreamFactory, t.w.p.j.xmlstream.XmlStreamFactory and the new t.w.p.j.xmlstream.XmlStreamServerFactory. The first one is a reconnecting client that uses xish' XmlStream as the (default) protocol and most of the real functionality, adding bootstrap observers, is in XmlStreamFactoryMixin.

The second just assigns a different protocol, but only accepts one parameter, the authenticator, and stores it. I don't think the storage of the authenticator in the factory is actually used, so one might argue that one could easily use xish' XmlStreamFactory.

Finally, XmlStreamServerFactory is not a (reconnecting) client, so it derives from XmlStreamFactoryMixin, just like xish XmlStreamFactory`. Maybe it should move there, really.

On the other hand, your suggestion of making the factory also instantiate the authenticator crossed my mind earlier, too. I'll take your suggestion and see how that looks.

[ralphm]

This approach works well, although given the way xish' XmlStreamFactory works, with the args and kwargs, it might not look the prettiest. I didn't think it was a good thing to replicate the adding of bootstrap observers in this factory, though. Changing those mechanics should be probably be another ticket.

Please review.

[therve]

That looks ok. Can you just document the args and kwargs attributes of XmlStreamFactoryMixin, so that it's more clear what's going on? Then you can merge.

[ralphm]

(In [24961]) Add server factory for Jabber XML Streams.

Author: ralphm. Reviewer: exarkun, glyph, therve. Fixes #3435.

This server factory takes an authenticator factory and instantiates a new authenticator for every incoming connection, in buildProtocol.