Bug 34416 - Add support for Account.I.ExternalPasswordStorage
Summary: Add support for Account.I.ExternalPasswordStorage
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: mission-control (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/da...
Whiteboard:
Keywords: patch
Depends on: 33485 34586
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-17 15:42 UTC by Jonathon Jongsma
Modified: 2011-02-23 15:30 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Jonathon Jongsma 2011-02-17 15:42:47 UTC
See bug #33485 for proposed specification to support CMs that can store passwords internall.  To make things work smoothly, mission-control should implement the Account.I.ExternalPasswordStorage interface for accounts on CMs that implement the CM.I.AccountStorage interface.
Comment 1 Danielle Madeley 2011-02-21 20:50:45 UTC
http://git.collabora.co.uk/?p=user/danni/telepathy-mission-control.git;a=shortlog;h=refs/heads/external-password-storage-34416

Here is an incomplete branch to implement this (it doesn't implement the PasswordSaved property). Incomplete because I've run into spec issues I've raised in #33485.

This branch also depends on the following tp-glib trivia: http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=commitdiff;h=aae116f232fa266eb194b8021eaca0e47004085b
Comment 2 Will Thompson 2011-02-22 03:02:12 UTC
(In reply to comment #1)
> This branch also depends on the following tp-glib trivia:
> http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=commitdiff;h=aae116f232fa266eb194b8021eaca0e47004085b

r+, please merge!
Comment 3 Danielle Madeley 2011-02-22 21:49:58 UTC
Ok. This is now ready for review.

The only thing I don't like about this approach is how often it calls IdentifyAccount(), but it seems like it's not correct to cache that value?
Comment 4 Will Thompson 2011-02-23 12:26:13 UTC
+
+        /* determine if we support Acct.I.ExternalPasswordStorage */
+        if (tp_proxy_has_interface_by_id (mcd_manager_get_tp_proxy (manager),
+                MC_IFACE_QUARK_CONNECTION_MANAGER_INTERFACE_ACCOUNT_STORAGE))
+        {
+            DEBUG ("CM %s has CM.I.AccountStorage iface",
+                   mcd_manager_get_name (manager));
+            mcd_dbus_activate_optional_interface (
+                TP_SVC_DBUS_PROPERTIES (account),
+                MC_TYPE_SVC_ACCOUNT_INTERFACE_EXTERNAL_PASSWORD_STORAGE);
+        }

Does this work if the TpConnectionManager's been constructed from the .manager file, without actually service-activating the CM? I suspect that it doesn't, unfortunately … I will check telepathy-glib.

+        tp_cli_protocol_call_identify_account (protocol, -1, params,
+            account_delete_identify_account_cb,
+            NULL, NULL, G_OBJECT (account));

I wonder if there's a chance that the account could be unreferenced, stopping the callback from being called, and hence stopping the password from being purged? I think taking a reference explicitly would be safest.

(In reply to comment #3)
> The only thing I don't like about this approach is how often it calls
> IdentifyAccount(), but it seems like it's not correct to cache that value?

On the contrary. MC should not only cache the value, but it should name accounts using it. Of course, because we have to support accounts whose name was chosen by MC forever, we'll need to have a separate field in the account to cache this value.

I think we could file a separate bug for that: it might be hairy, and what you have here works.
Comment 5 Will Thompson 2011-02-23 14:57:40 UTC
(In reply to comment #4)
> +
> +        /* determine if we support Acct.I.ExternalPasswordStorage */
> +        if (tp_proxy_has_interface_by_id (mcd_manager_get_tp_proxy (manager),
> +                MC_IFACE_QUARK_CONNECTION_MANAGER_INTERFACE_ACCOUNT_STORAGE))
> +        {
> +            DEBUG ("CM %s has CM.I.AccountStorage iface",
> +                   mcd_manager_get_name (manager));
> +            mcd_dbus_activate_optional_interface (
> +                TP_SVC_DBUS_PROPERTIES (account),
> +                MC_TYPE_SVC_ACCOUNT_INTERFACE_EXTERNAL_PASSWORD_STORAGE);
> +        }
> 
> Does this work if the TpConnectionManager's been constructed from the .manager
> file, without actually service-activating the CM? I suspect that it doesn't,
> unfortunately … I will check telepathy-glib.

It does! Hooray.

> +        tp_cli_protocol_call_identify_account (protocol, -1, params,
> +            account_delete_identify_account_cb,
> +            NULL, NULL, G_OBJECT (account));
> 
> I wonder if there's a chance that the account could be unreferenced, stopping
> the callback from being called, and hence stopping the password from being
> purged? I think taking a reference explicitly would be safest.

http://git.collabora.co.uk/?p=user/wjt/telepathy-mission-control-wjt.git;a=commitdiff;h=44cf2efe29e430c5794216c3e440a4d265dcec61

> (In reply to comment #3)
> > The only thing I don't like about this approach is how often it calls
> > IdentifyAccount(), but it seems like it's not correct to cache that value?
> 
> On the contrary. MC should not only cache the value, but it should name
> accounts using it. Of course, because we have to support accounts whose name
> was chosen by MC forever, we'll need to have a separate field in the account to
> cache this value.
> 
> I think we could file a separate bug for that: it might be hairy, and what you
> have here works.

This is bug 24640.
Comment 6 Will Thompson 2011-02-23 15:17:42 UTC
(In reply to comment #5)
> http://git.collabora.co.uk/?p=user/wjt/telepathy-mission-control-wjt.git;a=commitdiff;h=44cf2efe29e430c5794216c3e440a4d265dcec61

There are now a couple of patches at <http://git.collabora.co.uk/?p=user/wjt/telepathy-mission-control-wjt.git;a=shortlog;h=refs/heads/external-password-storage-34416>: one fixing this nit, and one to depend on telepathy-glib 0.13.14.

> This is bug 24640.

Actually, it's bug 34640. Whoops.
Comment 7 Will Thompson 2011-02-23 15:30:33 UTC
Woop woop. Will be in 5.7.4.


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.