Bug 32125

Maiku and I have been looking at this a bit recently. In principle it shouldn't be rocket science: we can have a CaptchaAuthentication interface which looks a lot like SASLAuthentication. In practice, it gets a bit more complicated.

Background
----------

Known users of captchas include:

* MXit, when you connect (possibly just the first time) -
  this can just be ServerAuthentication

* XMPP, whenever it wants to (but most popularly, when you message someone
  or join a MUC) - this would need a new channel type

MXit just sends a simple captcha image (the common "enter the text you see" sort). This is easy to represent on D-Bus, but impossible to answer if you're blind or partially sighted; but that's MXit's problem, not ours.

In the XMPP world, captchas are XEP-0158, which is... elaborate. I think it's safe to say that it's a superset of anything any other protocol will do. The server can ask you to solve any of:

* ocr: the common "enter the text" sort
* qa: answer an arbitrary ASCII question
* audio_recog: identify a sound
* picture_q: answer the question in an image
* picture_recog: identify an image
* speech_q: answer the question in a sound
* speech_recog: enter the words in a sound
* video_q: answer the question in a video
* video_recog: enter the words in a video
* SHA-256: HashCash (carry out some computation)

Further, it can require that you solve at least n of those, and it can make some of them mandatory.

SHA-256 is easy: Gabble can do it on your behalf, remove it from the list and decrement the number remaining to solve. The rest (if any) would need to be passed to a UI to be answered.

All except "SHA-256" and "qa" come with one or more alternative versions of a data payload (e.g. audio_recog might be offered as both audio/x-wav and audio/speex), which can be in-band via XEP-0014, or out-of-band via any URI (typically HTTP).

As far as I've been able to tell, real implementations of XEP-0158 (in ejabberd and an unofficial prosody plugin) only use "ocr", so they're just as easy to spec (and just as bad for blind/partially-sighted users!) as MXit.

Implementations of captchas on the web tend to consist of either OCR only (the same as MXit), or OCR + one of the audio-based things, where you can solve either.

Spec
----

There are basically two and a half ways to spec this.

One way is to design an interface modelling what MXit does: one "ocr" puzzle, which is mandatory. On the one hand, this is easy to spec (I believe Maiku has a sketch of a spec already), and supports the most common form of captcha. On the other hand, it can only support protocols which are inaccessible to partially sighted users, even if those protocols later implement better captchas, which seems pretty rubbish.

A second way is to design an interface which is basically XEP-0158, and assume that all practical protocols are going to be some subset of that.

The "and a half" is to present the "ocr" puzzle, but have an extension point for a UI to request further options: a simple UI which doesn't care about partially sighted users could just present the "ocr" puzzle. This assumes that nobody is ever going to deploy a captcha-style system where you have to solve more than one captcha (which XEP-0158 allows for!), or a system where none of the puzzles are "ocr".

Implementing for MXit
---------------------

Having a spec won't be sufficient to implement this in Haze for MXit: we'll also need to enhance libpurple so it can present the captcha in a machine-readable way (probably something remarkably similar to the Telepathy API), and only if the UI doesn't handle that, fall back to the current "pop up a dialog" (which Telepathy doesn't and can't implement, because the UI and the CM are separate).

Sketch of an ocr-only interface for this:

property CanRefreshCaptcha: b, readable, immutable

property CaptchaStatus: u, readable, change notification
    enum { Local_Pending, Remote_Pending, Succeeded, Try_Again, Failed }

property CaptchaError: s, DBus_Error_Name
    "" in all states except Try_Again and Failed.
    Typical values: "", Cancelled, AuthenticationFailed, NotAvailable.

property CaptchaErrorDetails: a{sv}
    Just like for Connection.ConnectionError.

GetCaptcha() -> ay, s
    Callable in state Local_Pending or Try_Again only. Return a captcha.
    State changes to Local_Pending, error changes to nothing.
    If called repeatedly, fails with NotAvailable unless CanRefreshCaptcha.

    [Rationale for not being a property: not immutable, because
    some captcha implementations let you try another if you can't
    read the first.]

AnswerCaptcha(s) -> nothing
    Callable in state Local_Pending only. State changes to Remote_Pending.

CancelCaptcha(u: enum { User_Cancelled, Not_Supported }) -> nothing
    Cancel. State changes to Failed with error NotAvailable or Cancelled
    if it isn't already Failed. All you can do now is to close the channel.

Channel.Close() does the equivalent of CancelCaptcha(Not_Supported) if not already Failed, then closes the channel.

Sketch of an interface modelling XEP-0158 for this: as above, except:

GetCaptchas() -> a(u: ID, s: Type, s: Label, u: Flags),
                 u: Number_Required,
                 s: Language

    ID is unique within this channel instance only.

    One flag defined: Required. Most captchas will have no flags.

    Type is as defined in XEP-0158.

    Label is as defined in XEP-0158. In particular, for "qa" it's the
    question.

    Language is the language of each Label as an RFC 5646 language tag,
    e.g. "en_US", or "" if unknown.

GetCaptchaData(u: ID, as: Mime_Types) -> ay

    Fetch and return the captcha data, giving highest priority to MIME
    types earlier in the list if possible. The CM is expected to implement
    HTTP, for instance, if captchas in this protocol generally require it.

    Returns an empty array if the type was "qa".

    [Rationale: If audio-based and image-based captchas are both available,
    we don't want to waste time downloading the audio until/unless the
    user asks to hear it. The extra D-Bus round-trips are not a problem,
    since they are expected to be quick compared with the time taken for
    the user to solve the captcha.]

AnswerCaptchas(a{u: ID => s: Answer}) -> nothing

    (As in the single-captcha case, but answer many captchas simultaneously.)

Only Handlers should interact with this interface (like SASLAuthentication).

Because of that, I think it's safe to say that they MAY assume the state is initially Local_Pending. It's not as if they could do anything about it apart from close the channel, if it wasn't.

(In reply to comment #1)
> Implementations of captchas on the web tend to consist of either OCR only (the
> same as MXit), or OCR + one of the audio-based things, where you can solve
> either.

... or answering a text-only question, like XMPP's "qa" (highly visible example for Telepathy developers: the "TextCha" to edit the freedesktop.org wiki).

> ... assumes that
> nobody is ever going to deploy a captcha-style system where ... none of
> the puzzles are "ocr".

Trivially not true, TextCha exists (although I haven't seen it used on XMPP).

Regarding error conditions, in XMPP we have:

- if client gives up or doesn't understand, it sends <not-acceptable/>
  (so in XMPP, User_Cancelled and Not_Supported would both map to
  the same thing - but in other protocols they might be different)

- if client gives the wrong answer, server sends <not-acceptable/>,
  which could map to AuthenticationFailed (stretching the meaning of
  "authentication" a bit)

- if client replies much too slowly, or twice, or otherwise gets the
  protocol wrong, the server sends <service-unavailable/>, which
  could map to NotAvailable

Reviewing commit 41651d8c9a5 in Maiku's branch.

In this comment I've used _foo_ as pseudo-markup for an appropriate hyperlink.

Incompatible changes
--------------------

I'd be tempted to rename CanRefreshCaptcha to CanRetryCaptcha, now that there's no Refresh method.

All mutable properties should explicitly document their change-notification (this is a general rule of Telepathy spec design). In this interface, it's currently via CaptchaStatusChanged, but we could in fact remove CaptchaStatusChanged and mark them as emitting o.fd.DBus.Properties.PropertiesChanged instead (see the Subject2 interface for how you do that).

This interface isn't fundamental to Telepathy; our new rule is that such interfaces should be version-numbered, like CaptchaAuthentication1 (again, see Subject2 for the details of how this is done).

Compatible/wording changes
--------------------------

A lot of the wording comes straight from my sketch here, which was quite brief and not very "spec-like"... but we can fix that any time I suppose.

> + <p>The ID with which to reference this captcha method

"... when answering it" maybe?

> + <p>Type as defined in XEP-0158.</p>

In the introduction to the interface, please include some wording about this interface's relationship to XEP-0158, something like:

The most commonly used form of captcha challenge is OCR (recognition of distorted letters or words in an image), but for accessibility reasons, this interface also allows various other types of challenge, such as plain-text questions or recognition of words in audio. Its structure is modelled on XMPP's _XEP-0158_, but can be used with other protocols by mapping their semantics into those used in XMPP.

<tp:rationale>
  It is important to support multiple types of captcha challenge to avoid discriminating against certain users; for instance, blind or partially-sighted users cannot be expected to answer an OCR challenge.

  XEP-0158 supports a superset of all other known protocols' captcha interfaces, and is sufficiently elaborate that we expect it will continue to do so.
</tp:rationale>

> + <p>Type as defined in XEP-0158.</p>

Please hyperlink to http://xmpp.org/extensions/xep-0158.html#challenge and list at least the most common type:

"The type of challenge _as defined by XEP-0158_. For instance, the commonly-used "type the letters/words you see in this image" challenge is represented by <code>ocr</code>."

> + <p>There can only be one Handler, which is a good fit for captcha's
> 1-1 conversation between a client and a server.</p>

To be grammatically correct this would be "... for a captcha's 1-1...", but tbh better phrasing would be something like "... a good fit for the question/answer model implied by captchas".

> + <p>Label is as defined in XEP-0158. In particular for "qa"
> + it's the question.</p>

A human-readable label for the challenge, as defined in XEP-0158. In particular, when Type = <code>qa</code>, this is a plain-text question for the user to answer.

> + The number of captcha methods required to be answered
> + in order to successfully complete this captcha challenge.

... challenge (most frequently 1, but XMPP allows servers to demand that more than one captcha is answered).

> + The language of each label in Captcha_Info if available,
> + or "" if unknown.

"... of each Label in ..." to indicate the cross-reference.

Please add a named "s" type, Language_Tag, to generic-types.xml, referencing IETF BCP 47 <https://www.rfc-editor.org/rfc/bcp/bcp47.txt>, and declare this to be of that type. (That's the familiar language tag from e.g. XML, like en_US.)

It might be worth saying "... if available, for instance en_US, or "" if..." as well, just to clarify.

> + <p>Gets information regarding each of the captcha methods
> + available and which and how many need to be successfully answered</p>

+ "To call this method successfully, the state must be Local_Pending or Try_Again. If it is Local_Pending, it remains Local_Pending. If called more than once while in Local_Pending state, or if the state is Try_Again, this method fetches a new set of captcha challenges, if possible, and the state returns to Local_Pending." (or similar wording)

<tp:rationale>: For instance, you could call GetCaptchas again from Local_Pending state if the user indicates that they can't understand the initially-offered captcha.

<tp:rationale> for GetCaptchas: This is a method, not a property, so that it can be used to fetch more than one set of captcha challenges, and so that change notification is not required. Only the Handler should call this method, and calling GetAll would not reduce round-trips, so the usual reasons to prefer a property do not apply here.

In CaptchaState please say: Because only the Handler should call methods on this interface, the Handler MAY reduce round-trips by not fetching the initial value of this property, and instead assuming that it is initially Local_Pending.

<tp:rationale>: This assumption normally avoids the need to call GetAll(), since the values of CaptchaError and CaptchaErrorDetails are also implied by this assumption, and the only other property is CanRetryCaptcha, which is immutable.

> + Either the state is not Local_Pending 

"not suitable" (it can either be Local_Pending or Try_Again)

<tp:rationale> for the list of MIME types in GetCaptchaData: XEP-0158 allows the same captcha to be made available in multiple formats, for instance the same spoken question as audio/x-wav, application/ogg and audio/speex.

> The CM is
> + expected to implement HTTP, for instance, if captchas in this
> + protocol generally require it.</p>

This looks more like an implementation note than normative spec text, but you could say something like "In protocols where captchas are downloaded out-of-band (for instance via HTTP), the connection manager is expected to do so."

> - Reason for abort.
> + Reason for cancelling.
> - Debug message for abort.
> + Debug message to describe reason for cancelling.

Should specify what these are for. I'd say:

reason: Reason for cancelling. This MAY be used to choose an error response to the remote server, and SHOULD also be reflected in the _CaptchaError_.

debug message: A textual description of the reason for cancelling, supplied by the Handler. This message SHOULD NOT be sent to the remote server, but SHOULD be copied into the 'debug-message' field of the _CaptchaErrorDetails_ and _ConnectionError_.

> + <tp:enumvalue suffix="User_Cancelled" value="0">

Should say: If this is used, the _CaptchaError_ SHOULD be set to _Cancelled_.

> + <tp:enumvalue suffix="Not_Supported" value="1">

If this is used, the _CaptchaError_ SHOULD be set to _NotImplemented_. (Unless you have a better idea?)

> + <tp:enumvalue suffix="Try_Again" value="3">
> + Call <tp:member-ref>GetCaptchas</tp:member-ref> again to get
> + a new captcha (if possible), or

Would there ever be any reason to set this state if you can't actually retry? (My guess is: no.)

> + useful to do in this state except to close the channel with
> + <tp:dbus-ref namespace="org.freedesktop.Telepathy.Channel">Close</tp:dbus-ref>
> + to close the channel.

At least one instance of "close the channel" is redundant :-P

        <p>Typical values: "", Cancelled, AuthenticationFailed, NotAvailable</p>

I realise I may have contradicted myself. Does Not_Supported from the cancellation enum map to the error NotAvailable, NotImplemented or a new CaptchaNotSupported?

I think NotAvailable should be ruled out, because that's what we'd get (as noted in Comment #6) if the server thinks we took too long; a new CaptchaNotSupported (meaning "either we don't have a UI to present captchas, or we do but it wasn't able to answer any of the captchas we were given") is probably the best option.

        <p>In particular, an ordinary authentication failure (as would
          be produced for an incorrect answer) SHOULD be represented by
          <tp:error-ref>AuthenticationFailed</tp:error-ref>,
          cancellation by the user's request SHOULD be represented
          by <tp:error-ref>Cancelled</tp:error-ref>, and
          cancellation by a local process due to inconsistent or invalid
          challenges from the server SHOULD be represented by
          <tp:error-ref>ServiceConfused</tp:error-ref>.</p>

This should mention the Not_Supported case, whatever that ends up being.

If we want ServiceConfused to be possible here, then we should have a Service_Confused cancellation reason in the enum, so the UI can tell us that it thinks the server is giving it gibberish.

Updated per your comments. Can once again be found here: http://cgit.collabora.com/git/user/maiku/telepathy-spec.git/log/?h=captcha

> + <tp:added version="0.21.5">(version 1)</tp:added>

This is a lie. Say 0.UNRELEASED, and whoever releases the spec will correct it to the about-to-be-released version just before they release it.

> + <annotation name="org.freedesktop.DBus.Property.EmitsChangedSignal"
> + value="true"/>

Does our toolchain support this appearing on the whole <interface>, rather than on each <property>? (I hadn't realised it *could* go on the <interface> until I looked it up in the D-Bus Specification...)

> <tp:enumvalue suffix="Not_Supported" value="1">

Please document that this is also what should happen if you Close the channel without first calling CancelCaptcha. (Rationale: if no Handler supports captcha channels, the ChannelDispatcher will just call Close, because it has no knowledge of specific channel types.)

Once again updated per your comments.

After talking with Thiago, we definitely need to add a signal for Status, as QtDBus does not (want) to support property changes over the bus. We can work this around in Telepathy-Qt4 and I'll probably do that, but still we need that signal for compatibility with previous releases. So please restore the StatusChanged signal which was in the first revision of the spec, and I'll take care to sync tp-qt's next versions with the new approach.

And another thing we discussed with Simon: would be nice to have an immutable property defining which mimetypes (and eventually which challenge types) the connection manager makes available, sort of SupportedMimeTypes and SupportedChallengeTypes.

(In reply to comment #12)
> After talking with Thiago, we definitely need to add a signal for Status, as
> QtDBus does not (want) to support property changes over the bus.

So there are two possibilities here:

If QtDBus doesn't provide high-level convenience API to emit or receive PropertiesChanged automatically (much like the way dbus-python doesn't handle Properties at all), but can emit/receive it like any other signal via explicit code in services/clients, then we can deal with that. If this is the case, IMO the interface should still use the standard PropertiesChanged signal, and we should put glue in telepathy-qt to make it easier to emit and/or receive if necessary, perhaps something like:

    SomeHelperClass::emitPropertiesChanged(QDBusConnection *,
        const QString &objectPath,
        const QString &iface,
        const QVariantMap &valuesProvided,
        const QStringList &invalidated);

on the emitting side, and

     signal Tp::Proxy::propertiesChanged(QString iface,
         QVariantMap valuesProvided,
         QStringList invalidated);

on the receiving side.

(For those who didn't follow the design of that signal: "valuesProvided" is a map from property names to their new values, whereas "invalidated" is a list of properties whose values have changed, but which the service has declined to include in valuesProvided, for instance for performance or secrecy reasons. Clients may retrieve them via GetAll() or something if desired.)

On the other hand, if QtDBus is actively obstructive and prevents us from emitting and/or receiving PropertiesChanged even via generic "any signal" stuff, then we need to know about that so we can avoid relying on PropertiesChanged *anywhere*. That'd be a problem - we already have interfaces that rely on it, like Chan.I.Subject2. To work around this, we'd either have to add our own PropertiesChanged to each interfaces (like the current Account.AccountPropertiesChanged, or any oFono interface), or add a new interface with one signal with semantics identical to those of PropertiesChanged and use that to work around Qt.

Which is it?

This wouldn't be the first time we'd worked around a Properties implementation - telepathy-glib has never used dbus-glib's implementation of Properties, because I spotted a serious mis-design in the way it dealt with properties, and worked around it by using TpDBusPropertiesMixin instead. (What I didn't realise at the time was that part of that mis-design was in fact a security bug, CVE-2010-1172...)

(In reply to comment #14) 
> Which is it?

The first - of course we can attach manually to PropertiesChanged and listen to it from within QtDBus. QtDBus always allows to connect to arbitrary signals on any interface without limitations, provided there's a way to demarshall the types. My main concern here was about the fact that in one specific situation where we want to deploy a handler we might not be able to rely on an updated version of tp-qt. However, for the time being I can maybe work this around through an extension. I'll try working on that in the next days, and will report back.

Mike, what do you think about my other proposals about the Supported* properties?

Let me try to address some of the comments:

1) any application in QtDBus can emit any signal. All it needs to do is call QDBusMessage::createSignal and QDBusConnection::send. The library will not block any sends.

2) any application can receive any signal, from any interface, provided that the bus allows it (non-eavesdropping). The introspection is not required if using QDBusConnection::connect.

3) the <interface> entry for org.freedesktop.DBus.Properties in the introspection is added by QtDBus automatically and it will not include the PropertiesChanged signal. As long as the receiver side does not require the introspection, it should work.

4) QtDBus has so far not chosen to implement the signal due to the lack of a way to be told when a receiver is listening for the signal. I refuse to implement something that will broadcast useless data on the bus into /dev/null.

(In reply to comment #17) 
> 2) any application can receive any signal, from any interface, provided that
> the bus allows it (non-eavesdropping). The introspection is not required if
> using QDBusConnection::connect.
> 
> 3) the <interface> entry for org.freedesktop.DBus.Properties in the
> introspection is added by QtDBus automatically and it will not include the
> PropertiesChanged signal. As long as the receiver side does not require the
> introspection, it should work.

I guess this simply means using QDBusConnection::connect to listen to PropertiesChanged on any object is the way to go? That was pretty much the solution I was leaning towards.

(In reply to comment #13)
> And another thing we discussed with Simon: would be nice to have an immutable
> property defining which mimetypes (and eventually which challenge types) the
> connection manager makes available, sort of SupportedMimeTypes and
> SupportedChallengeTypes.

and

(In reply to comment #16)
> Mike, what do you think about my other proposals about the Supported*
> properties?

I'm afraid I fail to see how that would be an improvement. Such as for XMPP, the server can send you any type imaginable. Yes, it would be kind of odd for when requesting new captchas for the server to change the types in the middle, but still. It seems like having such immutable properties would make it less flexible.

(In reply to comment #19)
> (In reply to comment #13)
> > And another thing we discussed with Simon: would be nice to have an immutable
> > property defining which mimetypes (and eventually which challenge types) the
> > connection manager makes available, sort of SupportedMimeTypes and
> > SupportedChallengeTypes.
> 
> I'm afraid I fail to see how that would be an improvement. Such as for XMPP,
> the server can send you any type imaginable. Yes, it would be kind of odd for
> when requesting new captchas for the server to change the types in the middle,
> but still.

Yes; in XMPP the server can even invent previously unknown types of captcha (not on the list in the XEP) whenever it wants to.

If you want to go as far as possible towards this approach, you could put an array of MIME types in the result of GetCaptcha, and instead of passing an array of MIME types to GetCaptchaData, pass a single MIME type chosen from that array.

(In reply to comment #17)
> 1) any application in QtDBus can emit any signal. All it needs to do is call
> QDBusMessage::createSignal and QDBusConnection::send. The library will not
> block any sends.

Good, this is what I hoped. We can emit the signal whenever an interesting property changes, and make our own decision between using the 'a{sv}' and the 'as', then. (In the case of this interface, all properties are small, so we'd use the a{sv}.)

Dario/Mike: if you aren't able to rely on an updated telepathy-qt for this, just implement a helper function in the connection manager that implements this interface, and put the same helper function in future telepathy-qt versions. It should only need to be a couple of lines.

> 2) any application can receive any signal, from any interface, provided that
> the bus allows it (non-eavesdropping). The introspection is not required if
> using QDBusConnection::connect.

Great. Dario/Mike: again, I'd say implement this in the client (if you can't rely on an updated telepathy-qt or telepathy-glib), and put an identical implementation in our client libraries for the future. It'll probably end up being a one-line wrapper around QDBC::connect. Make sure someone knowledgeable about D-Bus reviews it (perhaps cc me and/or Olli) to double-check that you're using a narrow enough signal match rule.

> 3) the <interface> entry for org.freedesktop.DBus.Properties in the
> introspection is added by QtDBus automatically and it will not include the
> PropertiesChanged signal. As long as the receiver side does not require the
> introspection, it should work.

I don't think any currently-existing dynamic bindings require signals to appear in introspection: dbus-python requires methods in introspection, but that's all.

It'd be nice if QtDBus could include this signal in introspection - it's a standard part of Properties now, even though it's not guaranteed to be emitted.

> 4) QtDBus has so far not chosen to implement the signal due to the lack of a
> way to be told when a receiver is listening for the signal.

Sure, it's the same as any other signal really. IMO the only sensible way to implement PropertiesChanged is to let interface-specific code (i.e. Telepathy or the application, here) decide whether it wants each changed property to get a property/value pair in the 'a{sv}', or a property name in the 'as' of invalidated properties, or no notification at all; and the easiest way to do that is to make the domain-specific layer or application entirely responsible for emitting PropertiesChanged, analogous to the way GObject objects are responsible for calling g_object_notify() (to emit notify::p) whenever the value of a property p changes.

Created attachment 56744 [details] [review]
Patch for PropertiesChanged on tp-qt

Attached comes a patch to tp-qt to support PropertiesChanged. Thiago, Simon, can you please have a look? It's trivial, but would appreciate your ack.

(In reply to comment #21)
> Created attachment 56744 [details] [review] [review]
> Patch for PropertiesChanged on tp-qt
> 
> Attached comes a patch to tp-qt to support PropertiesChanged. Thiago, Simon,
> can you please have a look? It's trivial, but would appreciate your ack.

Ok, some comments about the patch:

If I understood correctly (please correct me if I am wrong) PropertiesChanged is not interface specific, so adding it to AbstractInterface would mean that we would need to filter for interface name before emitting AbstractInterface::propertiesChanged. It also means we would need to connect to the same signal multiple times without any need if we want to handle propertiesChanged in multiple interfaces.

The other possible place we could put this is directly into Tp::DBusProxy. 

I see 2 problems with that:
- Tp::DBusProxy would gain a (public) signal emitting D-Bus PropertiesChanged directly. DBusProxy is inherited by most all high-level class that should not expose D-Bus specific data directly to the user. The main use case for this signal is internal, where we want to handle the property changed signal and act accordingly, not for users of our high-level classes.
- Tp::DBusProxy inherits Tp::Object that has a notify/propertyChanged mechanism (for Qt properties). So if adding to DBusProxy, we should probably name it something like dbusPropertiesChanged instead of propertiesChanged.

Another possible solution that I prefer would be to have a separate object (I would name it DBusPropertiesInterface) and add a protected accessor in DBusProxy to retrieve this object.
DBusPropertiesInterface would have the methods like in this patch. It would take a DBusProxy in the constructor and connect to the signal if monitorConnections is set to true, relaying it to the user as propertiesChanged.

What do you think?

(In reply to comment #22)
> DBusPropertiesInterface would have the methods like in this patch. It would
> take a DBusProxy in the constructor and connect to the signal if
> monitorConnections is set to true, relaying it to the user as
> propertiesChanged.
Meaning monitorProperties is set to true.

Looks like I missed possible-errors for GetCaptchaData. I was thinking NotAvailable when not in LocalPending or if GetCaptchas wasn't called and therefore the handler has no ID, InvalidArgument if the ID doesn't match any returned by the latest call to GetCaptchas, but I'm not certain what would be best for if the Handler's list of MIME types doesn't contain one the CM has. Would it be CaptchaNotSupported? Would it instead be better to list the possible MIME types in GetCaptchas as suggested above?

(In reply to comment #19)
> (In reply to comment #16)
> > Mike, what do you think about my other proposals about the Supported*
> > properties?
> 
> I'm afraid I fail to see how that would be an improvement. Such as for XMPP,
> the server can send you any type imaginable. Yes, it would be kind of odd for
> when requesting new captchas for the server to change the types in the middle,
> but still. It seems like having such immutable properties would make it less
> flexible.

Not really - I know the spec allows any possible mimetype, but at the same time it's likely a CM will support just a limited subset (for example, one might just support png). Of course the property would be CM-specific, and might be useful to find out which mimetypes would it support.

Though, I see a potential improvement in the type of the captcha - in that case, it would be nice to have an enum with the supported types instead of a string (which would be better from a client POV), and would be nice to advertise which type(s) of captcha the CM is going to send to the client. Or am I missing something here?


Re: Andre's comment
> What do you think?

As we already discussed the rationale on IRC, we need to wait for Simon or Thiago to give out an opinion.

If you don't like the way the current spec deals with MIME types, here is my new proposal, as described in Comment #20:

GetCaptchas() -> a(u: ID, s: Type, s: Label, u: Flags,
                   as: Available_MIME_Types),
                 u: Number_Required,
                 s: Language

GetCaptchaData(u: ID, s: MIME_Type) -> ay: Bytes

I can see that this is probably better than what I said previously.

(In reply to comment #25)
> I know the spec allows any possible mimetype, but at the same time
> it's likely a CM will support just a limited subset (for example, one might
> just support png).

For some protocols (mainly proprietary ones), sure, there might be a list, most likely of length 1. But for XMPP *there is no list*. The server doesn't pre-announce which MIME types it might send us (and it wouldn't be useful for it to do so, either): it just gives us some options (at the same time it gives us the list of captchas) and the CM passes them through.

What the CM supports is irrelevant: to it, the captcha is just a blob of bytes. What matters is that the Handler supports what the server gives us. I don't think having the CM try to transcode between image formats makes sense, unless the protocol's format is extremely strange, in which case it might make sense to offer it as image/png or image/jpeg or something.

Perhaps it would be helpful to have a list of MIME types that are mandatory-to-implement for the Handler, so that if the CM *is* transcoding, it can make sure to offer one of those. If we follow XMPP's example, which seems sensible, then that would be:

* image/jpeg if still images are supported
* audio/x-wav if sounds are supported
* video/mpeg if videos are supported

> Of course the property would be CM-specific, and might be
> useful to find out which mimetypes would it support.

Why? I don't see anything useful that the Handler could do with that information. Either the Handler supports showing at least one of the formats in which the captcha is offered, or it doesn't. If it supports one, great, if not, it can't possibly solve that captcha. Nothing the Handler does can change that.

The only thing I can see that you could possibly do with an immutable property listing MIME types would be to not even bother calling GetCaptcha() if you weren't going to be able to solve any of them... but that doesn't improve anything in the successful case, and all it does in the failure case is to save one D-Bus round trip. Why would you want to optimize for failure?

Having a property listing possible MIME types does raise the possibility of pre-emptively failing when you could, in fact, have succeeded, if the property's value was just a guess (and on XMPP, it can only ever be a guess). In this case, having the property doesn't just not help, it actively makes things worse!

> Though, I see a potential improvement in the type of the captcha - in that
> case, it would be nice to have an enum with the supported types instead of a
> string (which would be better from a client POV)

An enum isn't sufficient to represent XMPP: the server can offer us literally any format of its choice (image/x-ascii-art? :-). Given that there's a widely-implemented Internet standard for file types, we might as well use it...

(In reply to comment #24)
> Looks like I missed possible-errors for GetCaptchaData. I was thinking
> NotAvailable when not in LocalPending or if GetCaptchas wasn't called and
> therefore the handler has no ID, InvalidArgument if the ID doesn't match any
> returned by the latest call to GetCaptchas, but I'm not certain what would be
> best for if the Handler's list of MIME types doesn't contain one the CM has.
> Would it be CaptchaNotSupported? Would it instead be better to list the
> possible MIME types in GetCaptchas as suggested above?

In the API described in Comment #3, I'd intended for the CM to return an arbitrary MIME type of its choice if there was no intersection at all between the MIME types the Handler prefers, and the MIME types the CM has. (Rationale: it might as well return *something*, the Handler is going to fail anyway.) CaptchaNotSupported would also be reasonable, though.

If the API changes to what's described in Comment #26 (which it probably should), then the Handler is expected to know what's available (because you just told it), so the error could be InvalidArgument - in this API it doesn't matter that the Handler can't distinguish which argument was invalid, because a correct Handler wouldn't get it wrong, and if it does, it's a programming error rather than something the user can solve.

(In reply to comment #26)
> If you don't like the way the current spec deals with MIME types, here is my
> new proposal, as described in Comment #20:
> 
> GetCaptchas() -> a(u: ID, s: Type, s: Label, u: Flags,
>                    as: Available_MIME_Types),
>                  u: Number_Required,
>                  s: Language
> 
> GetCaptchaData(u: ID, s: MIME_Type) -> ay: Bytes
> 
> I can see that this is probably better than what I said previously.

I +1 that, and reckon everything you said below, you are definitely right.


> > Though, I see a potential improvement in the type of the captcha - in that
> > case, it would be nice to have an enum with the supported types instead of a
> > string (which would be better from a client POV)
> 
> An enum isn't sufficient to represent XMPP: the server can offer us literally
> any format of its choice (image/x-ascii-art? :-). Given that there's a
> widely-implemented Internet standard for file types, we might as well use it...

I wasn't referring to mimetypes here, but to the "Type" parameter. XMPP
actually does define a set of possible types of challenge (
http://xmpp.org/extensions/xep-0158.html#challenge ), and in this case I really
think we'd be better off with an enum

(In reply to comment #26)
> GetCaptchaData(u: ID, s: MIME_Type) -> ay: Bytes

Just a question about this specific bit which came to my mind right now: what should the CM do if MIME_Type was an empty string? I'm starting to lean towards the possibility of allowing the handler not to specify any set of supported mimetypes unless it has specific needs, especially if this bit:

> Perhaps it would be helpful to have a list of MIME types that are
> mandatory-to-implement for the Handler

came true.

(In reply to comment #21)
> Attached comes a patch to tp-qt to support PropertiesChanged.

I don't think this blocks an initial implementation of captchas: at the spec design stage we only need to know that it's possible, we don't necessarily need to get it into a library and update that library. (A first Qt client implementation of captchas could just use QDBusConnection::connect directly, in the Handler - that's not a high-level API or anything, but it'd work.)

(In reply to comment #22)
> If I understood correctly (please correct me if I am wrong) PropertiesChanged
> is not interface specific, so adding it to AbstractInterface would mean that we
> would need to filter for interface name before emitting
> AbstractInterface::propertiesChanged. It also means we would need to connect to
> the same signal multiple times without any need if we want to handle
> propertiesChanged in multiple interfaces.

PropertiesChanged is emitted from the Properties interface, regardless of the interface whose properties are changing, but the intention in its design was that you use a D-Bus match rule containing arg0="org.freedesktop.Telepathy.Channel.Interface.CaptchaAuthentication" (or whatever) to listen for it, which makes it interface-specific again.

Unfortunately, QDBusConnection::connect doesn't seem to support arg0-matching in match rules yet... so unless/until it does, yes, you'll have to filter client-side.

Connecting to the signal multiple times isn't really going to be significantly more expensive than connecting once, unless QtDBus' data structures are less efficient than I thought: the underlying D-Bus message is only delivered once, and QtDBus will call all the slots to which it's relevant.

If QtDBus is ever going to support arg0 match rules, we should be careful that the telepathy-qt API we provide for this doesn't rule out making use of them to avoid unnecessary message delivery (so the API should look like "subscribe to PropertiesChanged for interface X on this Tp::DBusProxy", not "subscribe to PropertiesChanged on this whole Tp::DBusProxy").

> - Tp::DBusProxy inherits Tp::Object that has a notify/propertyChanged mechanism
> (for Qt properties). So if adding to DBusProxy, we should probably name it
> something like dbusPropertiesChanged instead of propertiesChanged.

That's a good point though, perhaps you should do that rename anyway.

(In reply to comment #28)
> > > Though, I see a potential improvement in the type of the captcha - in that
> > > case, it would be nice to have an enum with the supported types instead of a
> > > string (which would be better from a client POV)
> 
> I wasn't referring to mimetypes here, but to the "Type" parameter. XMPP
> actually does define a set of possible types of challenge (
> http://xmpp.org/extensions/xep-0158.html#challenge ), and in this case I really
> think we'd be better off with an enum

Ah, right. This is less clear-cut - in theory the set can be extended in later versions of XEP-0158 if someone thinks of more challenges that ought to be supported, and we might have to deal with non-XMPP protocols with challenge types that aren't even in the XEP (in which case we could use Type = "x-turing-test" or whatever), but extension here seems less likely than with MIME types.

The rule of thumb that I tend to use is that if proprietary or obscure implementations will need to extend something, then it has to be a string set; but if extensions will only ever be made via the Telepathy spec process, then it can be an enum. I must admit that in this case, I was partly being lazy by letting xmpp.org maintain the registry of possible challenge types so we don't have to :-) I expect that proprietary protocols will rarely support more than a couple of challenge types, which they can just hard-code.

If we use an enum, then the Handler can switch() over well-known values instead of using an if/else if... chain, which is slightly simpler... but then Gabble would need to do a remarkably similar if/else if... chain to convert into our enum, rather than just passing them through without understanding, which seems non-ideal (Gabble doesn't otherwise need to understand anything about specific challenge types).

As far as I can see, the Handler is going to need specific code for each challenge type (or broad category of challenge types: image/video/audio/text) anyway, so the simplification from using an enum seems pretty small?

Feel free to write an enum for the spec if you're unconvinced, though; I could go either way.

(In reply to comment #30)
> (In reply to comment #21)
> > Attached comes a patch to tp-qt to support PropertiesChanged.
> 
> I don't think this blocks an initial implementation of captchas: at the spec
> design stage we only need to know that it's possible, we don't necessarily need
> to get it into a library and update that library. (A first Qt client
> implementation of captchas could just use QDBusConnection::connect directly, in
> the Handler - that's not a high-level API or anything, but it'd work.)

Indeed not - actually, I already fixed my code using an old tp-qt to support
that in a different way.

> 
> (In reply to comment #22)
> > If I understood correctly (please correct me if I am wrong) PropertiesChanged
> > is not interface specific, so adding it to AbstractInterface would mean that we
> > would need to filter for interface name before emitting
> > AbstractInterface::propertiesChanged. It also means we would need to connect to
> > the same signal multiple times without any need if we want to handle
> > propertiesChanged in multiple interfaces.
> 
> PropertiesChanged is emitted from the Properties interface, regardless of the
> interface whose properties are changing, but the intention in its design was
> that you use a D-Bus match rule containing
> arg0="org.freedesktop.Telepathy.Channel.Interface.CaptchaAuthentication" (or
> whatever) to listen for it, which makes it interface-specific again.
> 
> Unfortunately, QDBusConnection::connect doesn't seem to support arg0-matching
> in match rules yet... so unless/until it does, yes, you'll have to filter
> client-side.
> 
> Connecting to the signal multiple times isn't really going to be significantly
> more expensive than connecting once, unless QtDBus' data structures are less
> efficient than I thought: the underlying D-Bus message is only delivered once,
> and QtDBus will call all the slots to which it's relevant.

All the structures are implicitly-shared, so the overhead should be
trascurable. I am also quite sure you are right about QDBusConnection::connect

> 
> If QtDBus is ever going to support arg0 match rules, we should be careful that
> the telepathy-qt API we provide for this doesn't rule out making use of them to
> avoid unnecessary message delivery (so the API should look like "subscribe to
> PropertiesChanged for interface X on this Tp::DBusProxy", not "subscribe to
> PropertiesChanged on this whole Tp::DBusProxy").

Just to make it clear, in the end I suppose you favorable towards keeping this
in AbstractInterface (+ filtering based on the interface, and eventually using
arg0 in the future if ever), or did I misunderstand you and you prefer Andre's
approach?

(In reply to comment #31)
> (In reply to comment #28)
> > > > Though, I see a potential improvement in the type of the captcha - in that
> > > > case, it would be nice to have an enum with the supported types instead of a
> > > > string (which would be better from a client POV)
> > 
> > I wasn't referring to mimetypes here, but to the "Type" parameter. XMPP
> > actually does define a set of possible types of challenge (
> > http://xmpp.org/extensions/xep-0158.html#challenge ), and in this case I really
> > think we'd be better off with an enum
> 
> [snip]
> 
> If we use an enum, then the Handler can switch() over well-known values instead
> of using an if/else if... chain, which is slightly simpler... but then Gabble
> would need to do a remarkably similar if/else if... chain to convert into our
> enum, rather than just passing them through without understanding, which seems
> non-ideal (Gabble doesn't otherwise need to understand anything about specific
> challenge types).
> 
> As far as I can see, the Handler is going to need specific code for each
> challenge type (or broad category of challenge types: image/video/audio/text)
> anyway, so the simplification from using an enum seems pretty small?
> 

You are right indeed. Actually, it might be better to do what I am already doing, and leave the parameter as a string in the spec, and expose an enum in the client, just to make it easier for people implementing handlers who won't have to parse XMPP spec this way. This should make everyone's life easier :)

(In reply to comment #29)
> (In reply to comment #26)
> > GetCaptchaData(u: ID, s: MIME_Type) -> ay: Bytes
> 
> Just a question about this specific bit which came to my mind right now: what
> should the CM do if MIME_Type was an empty string?

In the Comment #26 API, why would you ever do that? If the Handler thinks it can display every image format ever invented, it could just as easily have a rule like "always pick the first type the CM offered".

You could make "" equivalent to the first thing in the CM's list if you wanted to encourage that behaviour, but I expect that real Handlers would want a rule more like this:

    if (supports audio/speex)
      that's the smallest, use it
    else if (supports audio/ogg)
      that's pretty small, use it
    else if (supports audio/x-wav)
      oh well, that's not so bad
    else
      either
        no particular preference, let's try the first one?
      or
        there's nothing here that I know I can handle, fail

(Using audio here because there isn't such an obvious order of precedence for images, but for images you probably want JPEG > PNG > GIF > BMP > other, or something.)

(In reply to comment #34)
> (In reply to comment #29)
> > (In reply to comment #26)
> > > GetCaptchaData(u: ID, s: MIME_Type) -> ay: Bytes
> > 
> > Just a question about this specific bit which came to my mind right now: what
> > should the CM do if MIME_Type was an empty string?
> 
> In the Comment #26 API, why would you ever do that? If the Handler thinks it
> can display every image format ever invented, it could just as easily have a
> rule like "always pick the first type the CM offered".
> 

True, makes total sense.

Last version of the spec pushed to Maiku's clone looks very good to me and I have no comments/wishes left. Thanks :)

I have a branch with a trivial change (fixing the copyright date) in cgit at <http://cgit.freedesktop.org/~smcv/telepathy-spec/?h=captcha>. git remote:

    ssh://people.freedesktop.org/~smcv/telepathy-spec captcha

Jeremy, Mike, are you confident that this is ready? If so, I'm happy to merge this to spec master. Already Reviewed-by: me and Dario.

For ease of review, here is HTML uploaded via `make upload-branch`:

http://people.freedesktop.org/~smcv/telepathy-spec-captcha/spec/

At the moment, Label and Language are mandatory. This comes from me having an XMPP mindset, but is wrong.

Consider this situation:

* the protocol is not XMPP, and is hard-coded to always have an "ocr"
  challenge

* because of that, the server does not send a label for the challenge:
  UIs for this protocol are expected to label it appropriately

* the user's locale is (say) German

We don't want the CM to generate a label corresponding to "ocr", because CMs are locale-neutral (not localized), so that label would have to be in the C locale (American/programmer English). The UI would display it, because it doesn't know it didn't come from the server - so the user gets instructions in English, not German.

I think we want these semantics:

* The Label for any captcha except "qa" may be ''. If so, the UI SHOULD
  generate a label in the user's locale; it MAY use the strings
  suggested in XEP-0158. Captchas of type "qa" with an empty Label
  are meaningless, and SHOULD NOT be generated or displayed.

* The Language may be '', which indicates that the language of the labels
  (if any) is unknown.

(In reply to comment #38)
> I think we want these semantics:
> 
> * The Label for any captcha except "qa" may be ''. If so, the UI SHOULD
>   generate a label in the user's locale; it MAY use the strings
>   suggested in XEP-0158. Captchas of type "qa" with an empty Label
>   are meaningless, and SHOULD NOT be generated or displayed.
> 
> * The Language may be '', which indicates that the language of the labels
>   (if any) is unknown.

I agree. Incidentally, I was already considering this behavior when designing my handler, probably due to my TL;DR on the overall documentation of the structure. So it's a full ++ for me on having these semantics.

(In reply to comment #38)
> * The Label for any captcha except "qa" may be ''.

I've added this to my branch and updated the HTML. Review, please?

> * The Language may be '', which indicates that the language of the labels
>   (if any) is unknown.

This was already allowed, actually.

(In reply to comment #40)
> (In reply to comment #38)
> > * The Label for any captcha except "qa" may be ''.
> 
> I've added this to my branch and updated the HTML. Review, please?

+            other than <code>qa</code>, connection managers SHOULD set Label
+            to an empty string instead of generating their own text.

What if protocol specifies the label anyway even if the challenge is, say, OCR? And most of all, what if a friendly proprietary protocol implements the captcha type "lotsofcrap" which actually requires a label? I'd rephrase that part so that label should be set to an empty string if the protocol does not supply one.

Or actually, if you really intend to always provide the label from the handler unless it's part of the challenge (which makes sense), wouldn't it make sense to ditch the label property in the structure, and provide the label in the payload instead? Although this is not matching the XMPP spec 1:1 it seems to me the most flexible approach for handlers. In this case, qa will simply provide a payload consisting of plain text.

Now that I wrote this down in my stream of consciousness, I think I am strongly favorable towards ditching the label. After all, it doesn't make much sense to have a field just for the sake of matching the spec (also, mainly for a captcha type we're not likely to be ever handling). Also, providing the label in the payload prevents confusion from the handler side, which is much better off in providing a "Please answer this question: <payload>". We can advertise the mimetype to be plain text in that case, and it will all quite fit together.

What do you reckon?

(In reply to comment #41)
> What if protocol specifies the label anyway even if the challenge is,
> say, OCR?

(XMPP does.)

Then we give the Handler as much information as possible, and let it make its own decision depending on UI policy and the value of Language: it can either display what the server gave it, or its own translated string; or even something like displaying the server's string if its Language matches the current locale, or both strings if not.

> And most of all, what if a friendly proprietary protocol implements
> the captcha type "lotsofcrap" which actually requires a label? I'd
> rephrase that part so that label should be set to an empty string if the
> protocol does not supply one.

I don't think the current spec rules out that behaviour. It does say CMs SHOULD NOT emit an empty label for "qa", but only because that's meaningless and unsolvable.

If a Handler implements the lotsofcrap challenge type, it needs to know enough about it to display it usefully, in which case it could easily extend the "ignore (CM or server is broken) if no label" rule to it.

Or please propose wording (as a spec patch) if you have a better version?

> Or actually, if you really intend to always provide the label from the
> handler unless it's part of the challenge (which makes sense),

I intend to *allow* Handlers to have that behaviour, but not *require* it. As a general design principle, telepathy-spec doesn't make UI decisions.

> wouldn't it
> make sense to
> ditch the label property in the structure, and provide the label in the
> payload instead?

I did consider putting the "qa" question in as a "text/plain" payload, but there doesn't seem a whole lot of point in special-casing it, if labels exist at all.

One difference between "qa" and the others is that for any other captcha type, you're expected to display the Label (or your own localized version of it) *and* the payload, whereas for "qa" (afaics) you're expected to display the Label (question) only:

  /-[Captcha]---------------\           /-[Captcha]---------\
  | Type the words you see: |           | What is 2+2?      |
  | /------------------\    |      vs.  |                   |
  | | spleen Belgium   |    |           | [_______________] |
  | \------------------/    |           \-------------------/
  | [_____________________] |
  \-------------------------/

(In reply to comment #42)
> (In reply to comment #41)
> > What if protocol specifies the label anyway even if the challenge is,
> > say, OCR?
> 
> (XMPP does.)
> 
> Then we give the Handler as much information as possible, and let it make its own decision depending on UI policy and the value of Language: it can either
> display what the server gave it, or its own translated string; or even something like displaying the server's string if its Language matches the current
> locale, or both strings if not.

I see your point

> 
> > wouldn't it
> > make sense to
> > ditch the label property in the structure, and provide the label in the
> > payload instead?
> 
> I did consider putting the "qa" question in as a "text/plain" payload, but there doesn't seem a whole lot of point in special-casing it, if labels exist at
> all.
> 
> One difference between "qa" and the others is that for any other captcha type, you're expected to display the Label (or your own localized version of it) *and*
> the payload, whereas for "qa" (afaics) you're expected to display the Label (question) only:
> 
>   /-[Captcha]---------------\           /-[Captcha]---------\
>   | Type the words you see: |           | What is 2+2?      |
>   | /------------------\    |      vs.  |                   |
>   | | spleen Belgium   |    |           | [_______________] |
>   | \------------------/    |           \-------------------/
>   | [_____________________] |
>   \-------------------------/

I don't see this as the main issue, as generally a handler receiving a payload with text/plain should figure out it's suppose to display that text only. However, in the end I think the spec is ok this way due to what you said about XMPP before and the fact that we might be hiding information if we went down the payload-and-no-label way.

++ to Simon's patches in his branch, btw.

(In reply to comment #44)
> ++ to Simon's patches in his branch, btw.

Thanks, Captcha1 merged to the spec for 0.25.2.