Bug 21735 - Map all Wocky errors to an appropriate TpError
Summary: Map all Wocky errors to an appropriate TpError
Status: NEW
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
: 19785 23372 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-05-14 05:09 UTC by Will Thompson
Modified: 2016-02-21 19:08 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Will Thompson 2009-05-14 05:09:40 UTC
Connection_Status_Reason is not expressive enough to encode the errors that might occur when registering an account.

For example, I tried to register two accounts in quick succession on jabster.pl, and for the second one it said:

<iq from='jabster.pl' id='788035814819' type='error'><query xmlns='jabber:iq:register'> <username>jkjkjkjkdjdj</username>
 <password>redacted</password>
</query><error code='500' type='wait'><resource-constraint xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'/><text xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'>Users are not allowed to register accounts so quickly</text></error></iq>

Gabble exposed that as Authentication_Failed, which is obviously wrong. It should emit ConnectionError with error NotAvailable.
Comment 1 Will Thompson 2009-07-21 04:29:47 UTC
ConnectionError would also let us emit a better error when we get the <hostname-unknown/> stream error. Currently, we emit AuthenticationFailed, because the most likely scenario is using a non-Google Talk JID with the connect server set to talk.google.com.
Comment 2 Simon McVittie 2010-06-23 06:55:26 UTC
Here's some infrastructural work for this, which doesn't improve on the current error mappings much, but does at least implement ConnectionError.

The next step would be to extend the error mapping functions to cover all Wocky errors; we can also add any missing errors to tp-spec.

(In reply to comment #0)
> </query><error code='500' type='wait'><resource-constraint
> xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'/><text
> xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'>Users are not allowed to register
> accounts so quickly</text></error></iq>

Perhaps resource-constraint should even be Service_Busy? Gabble with this branch would map it like that if it's a WockyXmppError, or to something else if it's a WockyConnectorError.

(In reply to comment #1)
> ConnectionError would also let us emit a better error when we get the
> <hostname-unknown/> stream error. Currently, we emit AuthenticationFailed,
> because the most likely scenario is using a non-Google Talk JID with the
> connect server set to talk.google.com.

This is still Authentication_Failed at the moment; we could add a Wrong_Server or Hostname_Unknown error, perhaps?

Ideally, Wrong_Server would put extra keys in @details to indicate what's wrong, something like this:

ConnectionError(Wrong_Server, { "debug-message": "host unknown",
    "connected-to": "talk.google.com",
    "requested-hostname": "collabora.co.uk" })
Comment 3 Will Thompson 2010-08-03 06:31:09 UTC
In 34cda73:

+    case WOCKY_XMPP_STREAM_ERROR_CONFLICT:
+      /* Assume we got this while already signed-in, rather than while
+       * connecting (which would be ALREADY_CONNECTED). */
+      return set_conn_reason (conn_reason,
+          TP_CONNECTION_STATUS_REASON_NAME_IN_USE,
+          TP_ERROR_CONNECTION_REPLACED);
+

Why is this assumption safe?

Otherwise, looks fine.
Comment 4 Simon McVittie 2010-08-03 06:39:22 UTC
(In reply to comment #3)
> In 34cda73:
> 
> +    case WOCKY_XMPP_STREAM_ERROR_CONFLICT:
> +      /* Assume we got this while already signed-in, rather than while
> +       * connecting (which would be ALREADY_CONNECTED). */
> +      return set_conn_reason (conn_reason,
> +          TP_CONNECTION_STATUS_REASON_NAME_IN_USE,
> +          TP_ERROR_CONNECTION_REPLACED);
> +
> 
> Why is this assumption safe?
> 
> Otherwise, looks fine.

I believe the while-connecting case comes out of Wocky as WOCKY_CONNECTOR_ERROR_BIND_CONFLICT instead. I should probably add it to connect/test-fail.py to make sure that's the case.
Comment 5 Simon McVittie 2010-08-03 07:51:43 UTC
(In reply to comment #4)
> I believe the while-connecting case comes out of Wocky as
> WOCKY_CONNECTOR_ERROR_BIND_CONFLICT instead.

It doesn't. See updated branch, which expects ConnectionError every time the tests expect a non-REQUESTED DISCONNECTED.
Comment 6 Will Thompson 2010-08-03 07:56:47 UTC
                                  _|_|_
                                ^/ . ..\^
                            ___[=========]___
                 ___-==++""" .  /. . .  \ .  """++==-___
           __-+"" __\   .. . .  | ..  . |  . .  .   /__ ""+-__
          /\__+-""   `-----=====\_ <O> _/=====-----'   ""-+__/\
        _/_/                      ""=""                      \_\_
       /_/                          |                          \_\
      //                            |                            \\
     /")                          \ | /                          ("\
     \o\                           \*/                           /o/
      \_)                       --**O**--                       (_/
                                   /*\
                                  / | \
                                    |
Comment 7 Simon McVittie 2010-08-03 08:02:04 UTC
*** Bug 23372 has been marked as a duplicate of this bug. ***
Comment 8 Simon McVittie 2010-08-03 08:03:35 UTC
Merged for 0.9.16. Back to NEW, because we still need to cover all the Wocky errors, and perhaps add more errors to the spec; in particular, the case you first thought of isn't covered.
Comment 9 Simon McVittie 2010-08-03 08:03:48 UTC
*** Bug 19785 has been marked as a duplicate of this bug. ***
Comment 10 Simon McVittie 2010-08-11 09:44:23 UTC
WOCKY_TLS_CERT_ERROR
====================

Easy to map into Telepathy, particularly with the new errors from spec 0.19.11.  We currently don't distinguish between SIGNER_UNKNOWN and SIGNER_UNAUTHORISED (which are both Cert.Untrusted), but I think that's fine.

WOCKY_TLS_CERT_OK maps to Cert.OtherError, but it's an internal error in either Wocky or Gabble if we see that as an error code.

WOCKY_TLS_CERT_INTERNAL_ERROR maps to Cert.OtherError at the moment: we could introduce an InternalError, perhaps?
Comment 11 Simon McVittie 2010-08-11 09:49:29 UTC
WOCKY_XMPP_STREAM_ERROR
=======================

This is the urn:ietf:params:xml:ns:xmpp-streams vocabulary from RFC 3920 (XMPP Core). We don't cover it very well at the moment.

We currently map <bad-format/> and all the other "your XML is bad" errors (<bad-namespace-prefix/>, <invalid-xml/>, <restricted-xml/>, <unsupported-encoding/>, and <xml-not-well-formed/>) to NetworkError. I think it's reasonable to conflate them all into one code, but we could perhaps introduce ServiceDenied or something for these "the server says we were wrong" errors?

<conflict/> is handled already.

<connection-timeout/> deserves a ServiceTimeout error ("dropped by the server for being idle"), I think.

<host-unknown/> is currently AuthenticationFailed (because in practice it probably means we're logging in to the wrong server), but it probably deserves an error code, HostUnknown or WrongServer or something. <host-gone/> could be the same or distinct.

<improper-addressing/> only affects s2s AFAICS, so we don't need to care.

<internal-server-error/> deserves a code, perhaps ServiceInternalError. <remote-connection-failed/> is probably the same (it's the server's problem, not ours).

<invalid-from/>, <invalid-id/>, <invalid-namespace/>, <not-authorized/>, <unsupported-stanza-type/>, <unsupported-version/> are all the same as <bad-format/> from our point of view. (Note that <not-authorized/> is *not* PermissionDenied - it's complaining about Gabble's behaviour, not about user behaviour.)

<policy-violation/> might deserve its own error or be PermissionDenied.

<resource-constraint/> isn't currently mapped, but should be ServiceBusy.

<see-other-host/> has special semantics - it has XML cdata which is the hostname or IP address. We need to get that into the ConnectionError details somehow, which probably means GabbleConnection handling it specially.

<system-shutdown/> deserves its own error code, perhaps ServiceShutdown.

<undefined-condition/> is anyone's guess. Perhaps we should follow D-Bus' example and introduce Failed, which explicitly means "we just don't know", or perhaps we should use NotAvailable.
Comment 12 Simon McVittie 2010-08-11 10:11:02 UTC
WOCKY_CONNECTOR_ERROR (first half)
==================================

UNKNOWN currently secretly means internal error in Wocky.

IN_PROGRESS would indicate an internal error in Gabble, if reached.

BAD_JID shouldn't be possible if Gabble has validated the 'account' parameter correctly; it's not much of a stretch to use InvalidHandle (it's an invalid thing-that-will-be-a-handle).

NON_XMPP_V1_SERVER isn't relevant to Gabble, which enables legacy Jabber auth, but it could reasonably be NotImplemented ("your server isn't good enough").

BAD_FEATURES means the server got confused or something. ServiceConfused?

TLS_UNAVAILABLE and TLS_REFUSED should probably both be EncryptionNotAvailable?

TLS_SESSION_FAILED should be EncryptionError.

BIND_UNAVAILABLE is "your server isn't good enough" like NON_XMPP_V1_SERVER.

BIND_FAILED I have no idea; how can it fail?

BIND_INVALID is WOCKY_XMPP_ERROR_BAD_REQUEST in response to binding; see also.

BIND_DENIED is WOCKY_XMPP_ERROR_NOT_ALLOWED in response to binding; see also.

BIND_CONFLICT is handled already.
Comment 13 Simon McVittie 2010-08-13 05:28:44 UTC
WOCKY_CONNECTOR_ERROR (second half)
===================================

BIND_REJECTED is any unrecognised WOCKY_XMPP_ERROR in reply to resource binding. In the regression tests, this is tested via the well-known <badger-badger-badger-mushroom/> error :-) This is a job for NotAvailable or a new Failed, perhaps.

SESSION_FAILED is a nonsensical reply or internal server error while establishing the session. NotAvailable? NetworkError? ServiceInternalError?

SESSION_DENIED is <forbidden/>. PermissionDenied? CSR_Authentication_Failed?

SESSION_CONFLICT is <conflict/> in reply to session establishment; it's not currently handled in Gabble, afaics, but should be mapped to AlreadyConnected.

The JABBER_AUTH sub-family of errors don't actually appear to be used anywhere.

INSECURE is an attempt to do XEP-0077 registration over an unencrypted connection when this was disallowed, so, EncryptionNotAvailable.

REGISTRATION_FAILED is a nonsensical reply, or any reply other than <service-unavailable>, to the registration form request, or any error stanza when submitting the form that doesn't have a more specific meaning. I don't think there's anything very useful we can do with this?

REGISTRATION_UNAVAILABLE is <service-unavailable> on the registration form request: clearly ServiceBusy.

REGISTRATION_CONFLICT is <conflict/> on the registration form submission, and is already mapped to RegistrationExists.

REGISTRATION_REJECTED is <not-acceptable/> on the registration form submission, or a registration form that requires email when we didn't supply one. We currently map this to PermissionDenied and CSR_Authentication_Failed, but that's a bit rubbish, perhaps?

REGISTRATION_UNSUPPORTED is a registration form that contains fields other than username, password and email. We currently use NotAvailable and CSR_Authentication_Failed.

REGISTRATION_EMPTY is a registration form with no fields, which makes no sense. We don't map this; it could be ServiceInternalError?

UNREGISTER_DENIED is <forbidden/> or <not-allowed/>. UNREGISTER_FAILED is any other failure to unregister. We don't support unregistration in Gabble anyway, so these don't really need mapping yet.
Comment 14 Simon McVittie 2010-08-13 05:46:56 UTC
WOCKY_AUTH_ERROR
================

INIT_FAILED doesn't actually ever seem to happen.

NOT_SUPPORTED is <service-unavailable/> during Jabber auth, which means the server doesn't support Jabber auth; it can also mean there are no supported SASL mechanisms, in SASL auth. Should this be AuthenticationFailed, or NotImplemented?

NO_SUPPORTED_MECHANISMS is somewhat similar.

NETWORK is already NetworkError.

INVALID_REPLY is an internal server error or nonsensical reply; see other similar errors above.

NO_CREDENTIALS means we weren't given the username/password. Can this happen in Gabble? AuthenticationFailed seems a fair mapping.

FAILURE is a catch-all for misc failures, so it should probably be AuthenticationFailed.

CONNRESET is already ConnectionLost.

STREAM is any stream error (see above) during SASL or Jabber auth. We currently guess NetworkError; information is lost inside Wocky.

RESOURCE_CONFLICT is <conflict/> for Jabber auth, so it should be AlreadyConnected.

NOT_AUTHORIZED is <not-authorized/>, which normally means the server believes our code is wrong, but in Jabber auth it means incorrect credentials, so, AuthenticationFailed.

NO_CREDENTIALS is <not-acceptable/>, which means we failed to send some info the server requires; probably our fault? AuthenticationFailed seems fair.
Comment 15 Simon McVittie 2010-08-13 05:48:05 UTC
> REGISTRATION_UNAVAILABLE is <service-unavailable> on the registration form
> request: clearly ServiceBusy.

... but that's not what <service-unavailable/> means. This should actually be NotImplemented.
Comment 16 Simon McVittie 2010-08-13 07:44:51 UTC
WOCKY_XMPP_ERROR
================

These are RFC 3920 stanza errors.

bad-request and unexpected-request are probably internal problems in Gabble or Wocky? We currently map them to NotAvailable.

conflict is currently mapped to CSR_Name_In_Use, NotAvailable, which is the best we can do in general; bits of Gabble are expected to special-case it, and indeed they do.

feature-not-implemented and service-unavailable are NotImplemented, although NotCapable would also be a reasonable guess.

forbidden, payment-required and not-authorized are CSR_Authentication_Failed, PermissionDenied.

gone is DoesNotExist (a reasonable guess, I think).

internal-server-error needs a better mapping, as mentioned above.

item-not-found is DoesNotExist.

jid-malformed is Invalid[thing that would be a]Handle.

recipient-unavailable is Offline.

redirect needs extra info in ConnectionError anyway. We currently guess wildly and say DoesNotExist.

remote-server-not-found is currently DoesNotExist, with a FIXME comment suggesting that it should perhaps be NetworkError; I could go either way.

remote-server-timeout is currently NetworkError.

resource-constraint from the server is exactly ServiceBusy. We can't distinguish between this and the remote UI being busy without further context, unfortunately.

not-allowed, not-acceptable, registration-required and subscription-required are all currently mapped to CSR_Authentication_Failed, PermissionDenied.

undefined-condition is anyone's guess; we use NotAvailable.
Comment 17 Simon McVittie 2010-11-23 08:40:40 UTC
(In reply to comment #14)
> WOCKY_AUTH_ERROR

Dealt with as part of redoing SASL support, since it's relevant there. The rest of this bug remains.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.