[Bug 32125] Should support auth using captcha

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=32125

--- Comment #7 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-01-30 04:41:26 PST ---
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

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.