This reverts commit aadd033597.
Commit "Change SASL handler computation type" introduced a deadlock
when sasl handlers tried to take the TMVar that was already taken by xmppSasl
The SaslM computation type _has_ to be a State transformer rather than working on the TMVar directly because otherwise we would either have to acquire the same lock twice (resulting in a deadlock; situation before this patch) or release the lock prematurly, which would allow the authentication to be preempted (introducing a race condition)
Conflicts:
source/Network/Xmpp/Sasl.hs
As mentioned in a previous patch, the `AuthFailure' type signals a
(non-fatal) SASL error condition. This is now reflected in the
documentation.
I went through the different constructors for the type, looking at how
they were produced (thrown) and whether or not that information were
useful for the application using Pontarius XMPP.
To begin, I conclude that `AuthStreamFailure' is only used internally.
It will probably be removed when the internal type signatures of the
Sasl package are changed to conform with the rest of the `Error'
computations of Pontarius XMPP.
`AuthFailure' is not thrown as far as I can see, but is only used for
the Error instance.
`AuthNoAcceptableMechanism' is thrown by `xmppSasl' when none of the
mechanisms offered by the server is specified as acceptable by the
client. It wraps the mechanisms offered. I consider this information
useful for client developers, and will therefor keep this constructor.
`AuthSaslFailure' wraps a `SaslFailure' (from Types.hs) and is only
thrown when `pullSaslElement' unpickles a SASL failure. This, together
with `AuthNoAcceptableMechanism' above, could be considered the
`normal' ways of which SASL might be failing.
`AuthStringPrepFailure' is thrown if `prepCredentials' fails to
stringprep-verify the credentials. This might be interesting for the
client developer. As I think that `AuthIllegalCredentials' is more
understandable, I have changed the name to that.
`AuthNoStream' is thrown by `xmppSasl' when the stream state is
`Closed'. This is the result of a client program error/bug. This patch
removes this constructor and modifies the behaviour of xmppSasl to
throw an `XmppFailure' instead.
`AuthChallengeFailure' is thrown if `fromPairs' fails (in Scram.hs),
if a challenge element could not be pulled (in Common.hs), by
`saslFromJust' if a `Nothing' value is encountered (in Common.hs), in
`pullFinalMessage' (`decode') if the success payload could not be
decoded (in Common.hs), or if `toPairs' (in Common.hs) can not extract
the pairs. Furthermore, `AuthServerAuthFailure' is thrown if there is
no `v' value in the final message of the SCRAM handler. Finally,
`AuthXmlFailure' is thrown when `pullSuccess' find something other
than a success element (and, I'm guessing, a `SaslFailure' element).
This can only happen if there is a bug in Pontarius XMPP or the
server. The way I see it, all these failures are abnormal and are of
no interest from the client application itself. I suggest that these
events are logged instead, and that we signal any of these conditions
with a new `AuthOtherFailure' constructor.
I suggest that we remove the `AuthFailure' constructor, and use the
`AuthOtherFailure' for the `Error' instance.
The `AuthFailure' type and all its constructors are now documented.
I also made some minor documentation enhancements to the `XmppFailure'
type.
This patch changes the SASL handler computation type from `ErrorT
AuthFailure (StateT Stream IO) ()' to `TMVar Stream -> IO (Either
XmppFailure (Maybe AuthFailure))' to better conform with the rest of
the API.
While short, I do believe that `SaslM' type makes the code
significantly less understandable. This is at least the case for me.
This patch removes it and changes the types to read the full `ErrorT
AuthFailure (StateT Stream IO) a' type instead.
The `Stream' and the `Connection' modules/concepts has been quite
similar and, in some ways, overlapping. The `Stream' and `Connection'
modules allow for direct access to the XML stream. The `Stream' module
exported an API for opening, restarting, and closing the stream. The
`Connection' module, on the other hand, allowed for pushing and
pulling elements and stanzas to and from the stream. Furthermore, the
`Connection' type was used in both the `Connection' module and the
higher-level `Stream' module.
This patch joins the two modules into one `Stream' module, and renames
the `Connection' type to `Stream'. It also renames most other
connection-related functions and types. Also, `connect' is renamed
`openStream' and `closeStreams' is renamed `closeStream' (the stream
is `bidirectional' in RFC 6120 terminology).
We can treat all functions related to SASL negotiation as a submodule
to Pontarius XMPP if there are no dependencies from the internal
Network.Xmpp modules to the SASL functionality. Because of this,
`auth' and `authSimple' were moved from Session.hs to Sasl.hs. As the
bind and the `{urn:ietf:params:xml:ns:xmpp-session}session'
functionality are related only to the SASL negotation functionality,
these functions has been moved to the SASL submodule as well.
As these changes only leaves `connect' in the Session module, it seems
fitting to move `connect' to Network.Xmpp.Stream (not
Network.Xmpp.Connection, as `connect' depends on `startStream').
The internal Network.Xmpp modules (Connection.hs) no longer depend on
the Concurrent submodule. This will decrease the coupling between
Network.Xmpp and the concurrent implementation, making it easier for
developers to replace the concurrent implementation if they wanted to.
As Network.Xmpp.Connection is really a module that breaks the
encapsulation that is Network.Xmpp and the concurrent interface, I
have renamed it Network.Xmpp.Internal. As this frees up the
Network.Xmpp.Connection name, Network.Xmpp.Connection_ can reclaim it.
The high-level "utility" functions of Network.Xmpp.Utilities,
Network.Xmpp.Presence, and Network.Xmpp.Message has been moved to
Network.Xmpp.Utilities. This module contains functions that at most
only depend on the internal Network.Xmpp.Types module, and doesn't
belong in any other module.
The functionality of Jid.hs was moved to Types.hs.
Moved some of the functions of Network.Xmpp.Pickle to
Network.Xmpp.Marshal, and removed the Network.Xmpp.Pickle module.
A module imports diagram corresponding to the one of my last patch
shows the new module structure. I also include a diagram showing
the `Sasl' and `Concurrent' module imports.
I'm assuming and defining the following:
1. XMPP failures (which can occur at the TCP, TLS, and XML/XMPP
layers (as a stream error or forbidden input)) are fatal; they will
distrupt the XMPP session.
2. All fatal failures should be thrown (or similar) by `session', or
any other function that might produce them.
3. Authentication failures that are not "XMPP failures" are not fatal.
They do not necessarily terminate the stream. For example, the
developer should be able to make another authentication attempt.
The `Session' object returned by `session' might be useful even if
the authentication fails.
4. We can (and should) use one single data type for fatal failures.
(Previously, both StreamFailure and TlsFailure was used.)
5. We can catch and rethrow/wrap IO exceptions in the context of the
Pontarius XMPP error system that we decide to use, making the error
system more intuitive, Haskell-like, and more straight-forward to
implement. Calling `error' may only be done in the case of a
program error (a bug).
6. A logging system will remove the need for many of the error types.
Only exceptions that seem likely to affect the flow of client
applications should be defined.
7. The authentication functions are prone to fatal XMPP failures in
addition to non-fatal authentication conditions. (Previously,
`AuthStreamFailure' was used to wrap these errors.)
I'm hereby suggesting (and implementing) the following:
`StreamFailure' and `TlsFailure' should be joined into `XmppFailure'.
`pullStanza' and the other Connection functions used to throw
`IOException', `StreamFailure' and `TlsFailure' exceptions. With this
patch, they have been converted to `StateT Connection IO (Either
XmppFailure a)' computations. They also catch (some) IOException
errors and wrap them in the new `XmppIOException' constructor.
`newSession' is now `IO (Either XmppFailure Session)' as well (being
capable of throwing IO exceptions).
Whether or not to continue to a) wrap `XmppFailure' failures in an
`AuthStreamFailure' equivalent, or, b) treat the authentication
functions just like the other functions that may result in failure
(Either XmppFailure a), depends on how Network.Xmpp.Connection.auth
will be used. Since the latter will make `auth' more consistent, as
well as remove the need for a wrapped (and special-case) "AuthFailure"
type, I have decided to give the "b" approach a try. (The drawback
being, of course, that authentication errors can not be accessed
through the use of ErrorT. Whether or not this might be a problem, I
don't really know at this point.) As the SASL code (and SaslM)
depended on `AuthStreamFailure', it remains for internal use, at least
for the time-being. `session' is now an ErrorT computation as well.
Some functions have been updated as hacks, but this will be changed if
we decide to move forward with this approach.
As mentioned in #pontarius, `Context' is simply a bunch of thread
management features, and users that want that can build their own on
top of the `Connection' layer. The benefit of hiding `Context' is that
it makes the API clearer, and significantly decreases the complexity
of the library.
As the `Basic' module is simply an interface to `Connection', it was
renamed to `Connection'. The old `Connection' module was moved to
`Connection_'.
Exported the types of the fields of `Connection' (such as
`ConnectionState' and `ConnectionHandle' (previously `HandleLike').
Jon Kristensen
Philipp Balzarek