Branch implementing proposed changes here: http://git.collabora.co.uk/?p=user/jonathon/telepathy-spec;a=shortlog;h=refs/heads/account-storage

+    <method name="ForgetPassword" tp:name-for-bindings="Forget_Password">
+      <tp:docstring>
+        <p>Clears any saved password associated with this account.</p>

Either tp:docstring requires namespacing, or you should leave out the HTML here (also happens a few other times).

+      <arg direction="in" name="Account_id"

Account_Id

+      <arg direction="in" name="Account_id"

Here too.

Also you should include rationales to explain why these things are required (they also tend to help explain when you should use them in your own CM and when you should not).

If you mean SHOULD, SHOULD NOT, MUST, MAY, etc. in the RFC sense, you should capitalise them.

Looks functionally good. Various formatting/documentation nitpicks, basically:

+        <p>A list of the Accounts stored in this connection manager, and flags
+          indicating their status.</p>

an a{sv} is not what i'd normally describe as a list. maybe set?

+    <method name="RemoveAccount" tp:name-for-bindings="Remove_Account">
+      <tp:docstring>
+        <p>Removes an account from the connection manager's internal
+          storage.</p>
+      </tp:docstring>

I guess this includes removing its credentials? I think it'd be good to spell out exactly how ForgetCredentials and RemoveAccount differ.

+    <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
+      <p>An interface for Accounts whose passwords are stored externally and
+        should not be stored by the client.</p>
+    </tp:docstring>

Which client? I think it's worth saying explicitly that neither the Account Manager, nor any ServerAuthentication handler, should store the credentials.

+        <p>If this property is false, the client should not attempt to cache the
+          authentication response in its own keyring.</p>

<code>False</code>

+
+        <p>If this property is not specified, it should be treated as if it were
+          true.</p>

<code>True</code>

+        more complex than a simple password prompt (e.g.  X-TELEPATHY-PASSWORD
+        or PLAIN).</p>

<code> or <tt> around the mechanisms plz.

+    <method name="StoreCredentials" tp:name-for-bindings="Store_Credentials">
+      <arg direction="in" name="Store" type="b">
+        <tp:docstring>
+          Whether to store the authentication credentials.
+        </tp:docstring>
+      </arg>
+      <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
+        <p>This method tells the connection manager whether to store the
+          authentication response in order to allow the connection manager to
+          sign-on automatically in the future.</p>

When should the handler call this method? Should it call it before or after providing the password? Just before calling Close? (Allowing it to be called after authentication has succeeded would allow the “remember your password?” prompt only to be shown when the password's known to be correct.)

One new commit has been added to the branch that fixes the review findings above.

Looks good to me, please merge as a DRAFT

Merged as draft

Do we need to keep this bug to track the spec's eventual undrafting?

Since I am now attempting to implement the Mission Control parts of this, I am reopening the bug to point out issues in the draft.

 - It is not specified whether Acct.I.EPS should be present on all accounts or just ones who's CM implements CM.I.AS. I have assumed the latter case, because this allows us to do a tp_proxy_has_interface() to discover whether the functionality is available.

 - CM.I.AS.Accounts has no change notification. As a result, it's not possible to implement the Acct.I.EPS.PasswordSaved property, since we can't make async method calls from a property getter function, and we can't request and monitor the value without any change notification.

 - Similarly. Acct.I.EPS provides no change notification: AccountPropertyChanged cannot be used here:

"""
The values of one or more properties on this interface (that do not specify that this signal does not apply to them) may have changed. This does not cover properties of other interfaces, which must provide their own change notification if appropriate.
"""

(In reply to comment #8)
> Since I am now attempting to implement the Mission Control parts of this, I am
> reopening the bug to point out issues in the draft.
> 
>  - It is not specified whether Acct.I.EPS should be present on all accounts or
> just ones who's CM implements CM.I.AS. I have assumed the latter case, because
> this allows us to do a tp_proxy_has_interface() to discover whether the
> functionality is available.

This sounds right to me.

>  - CM.I.AS.Accounts has no change notification. As a result, it's not possible
> to implement the Acct.I.EPS.PasswordSaved property, since we can't make async
> method calls from a property getter function, and we can't request and monitor
> the value without any change notification.

The AM could cache the value … and if someone removes the saved password from the CM behind the AM's back, it will get out of synch, but hey.

>  - Similarly. Acct.I.EPS provides no change notification:
> AccountPropertyChanged cannot be used here:
> 
> """
> The values of one or more properties on this interface (that do not specify
> that this signal does not apply to them) may have changed. This does not cover
> properties of other interfaces, which must provide their own change
> notification if appropriate.
> """

AccountPropertyChanged is a bit upsetting.

You could push the boat out and become the first user, in Telepathy, of PropertiesChanged <http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties>.

http://git.collabora.co.uk/?p=user/danni/telepathy-spec.git;a=shortlog;h=refs/heads/fixes-33485

I've taken your advice, it makes sense to start using PropertiesChanged.

Added a depends on #34586 -- support for DBus.Properties.PropertiesChanged in tp-glib.

Note to self: Accounts need to document the case where we don't know if a password has been saved or even what accounts are known. I'm proposing: if an account_id is not listed in the map, the value is unknown and MAY be assumed to have its credentials stored. ForgetCredentials() will not fail for an account_id returned by IdentifyAccount(), even if it does not appear in the map.

(In reply to comment #11)
> Note to self: Accounts need to document the case where we don't know if a
> password has been saved or even what accounts are known. I'm proposing: if an
> account_id is not listed in the map, the value is unknown and MAY be assumed to
> have its credentials stored. ForgetCredentials() will not fail for an
> account_id returned by IdentifyAccount(), even if it does not appear in the
> map.

Sjoerd is of the opinion that the unknown case isn't real, because a CM can always keep a config file of what accounts it thinks it has saved passwords for. I think he's right, so we can ignore this.

The changes on this spec branch look fine!

merged

(In reply to comment #7)
> Do we need to keep this bug to track the spec's eventual undrafting?

The answer should have been "yes".

Is there an implementation?

(Context: for Telepathy 1.0, I'm deciding between "delete these interfaces" and "undraft these interfaces".)

I'm assuming there must be some non-public connection manager somewhere that implements the ConnectionManager and Channel side of this? It isn't applicable to the usual suspects, which all follow the "CMs are stateless" design principle. It could be reasonable for Haze to implement this, though, so it can cache stuff in the equivalent of ~/.pidgin? (To fit into the Telepathy model, it would have to disable each libpurple account on startup, and enable them as their corresponding connections get connected.)

MC has an implementation of the AccountManager and Account side, but MC can crash if there's no TpProtocol on which to call IdentifyAccount (Bug #44512), and there are no regression tests.

Is there a client-side in Empathy or something?

I'm tempted to back this out from Telepathy 1.0, and we can bring it back if/when it's necessary, with tests this time.

(hey!) I always hated this spec. Seemed to me like a good candidate for being made private to its single user.

Thanks, I'll delete this from the Telepathy 1.0 branch at some point.

So we should remove:
- Account.Interface.ExternalPasswordStorage1
- ConnectionManager.Interface.AccountStorage1
- Channel.Interface.CredentialsStorage1

Created attachment 93050 [details] [review]
remove CM side password storage API

(In reply to comment #20)
> Created attachment 93050 [details] [review] [review]
> remove CM side password storage API

Ship it. Please also chop these interfaces out from MC master and next.

> - <xi:include href="Connection_Manager_Interface_Account_Storage1.xml"/>

We might well want to bring back this part ("CMs being notified when accounts are deleted") eventually, for:

* CM-side roster caches
* CM-side alias caches
* CM-maintained avatar caches (if the cache ever gets refcounting)
* Haze maintaining a persistent ~/.pidgin equivalent per account

but let's cross that bridge when we come to it.

(In reply to comment #21)
> Please also chop these interfaces out from MC master and next.

Actually, I'll do that bit - it will conflict with the next thing I want to do.

Spec merged to next; closing this bug.

(In reply to comment #22)
> Actually, I'll do that bit - it will conflict with the next thing I want to
> do.

https://bugs.freedesktop.org/show_bug.cgi?id=69446#c3
https://bugs.freedesktop.org/show_bug.cgi?id=69600