Summary: | Integrate new accounts-sso support into mission-control | ||
---|---|---|---|
Product: | Telepathy | Reporter: | Alex Fiestas <afiestas> |
Component: | mission-control | Assignee: | Telepathy bugs list <telepathy-bugs> |
Status: | RESOLVED MOVED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
Severity: | normal | ||
Priority: | medium | CC: | xclaesse |
Version: | git master | ||
Hardware: | Other | ||
OS: | All | ||
Whiteboard: | |||
i915 platform: | i915 features: | ||
Attachments: | UOA patch |
Description
Alex Fiestas
2013-07-01 14:23:52 UTC
Yes, this would be useful. Jolla (and an opensource mobile platform, Nemo) are currently using a light fork of the same plugin from Empathy. We also use a fork of the SaslSignonAuth service from Empathy as a ServerAuthentication channel handler. The code for both is at https://github.com/nemomobile/telepathy-accounts-signon/commits/master, if it's of any interest. We would definitely move to an upstream plugin if possible. The patch does not apply on MC master here. You should use "git format-patch" or link to the branch here (you can host on github for example). From a quick read of the patch: 1) you don't need signon dependency 2) Would be nice to remove "uoa", "ubuntu" and "empathy" words from the code. Previous plugin seems to be called "libaccounts-sso", I think "sso" does not mean much, so I would call the new plugin just "libaccounts". 3) EmpathyWebcredentialsMonitor is problematic, it uses an ubuntu specific dbus service. As I understand, its only purpose is to tell MC to retry connecting when a previous auth failure got resolved (like user re-typing his pwd, or grant permission on google app, etc). I have to admit I'm not sure how to fix this, and it seems GOA plugin does not do anything similar (so I suppose it just doesn't work there). I see various solutions, but none satisfy me completely: a) keep it out of scope from MC plugin, "something" else will have to tell MC to reconnect. b) standardize a freedesktop spec for that iface. c) Add a "auth-failure" boolean property on AgAccount and make change notification through account's db. *** Bug 62814 has been marked as a duplicate of this bug. *** See also Bug #71230, which deletes our old Maemo libaccounts (pseudo-)plugin. It might be instructive to compare the UOA plugin with the one deleted there, although libaccounts has probably moved on since that plugin last worked, and it hasn't been tested (or testable) since Maemo Harmattan. Comment on attachment 81807 [details] [review] UOA patch Review of attachment 81807 [details] [review]: ----------------------------------------------------------------- I haven't really reviewed the code here, only the build system... ::: configure.ac @@ +211,5 @@ > AC_DEFINE([ENABLE_GNOME_KEYRING], [0], [Define whether gnome-keyring support is enabled]) > fi > > +# ---------------------------------------------------------------- > +# dnl libaccounts-glib SSO Please don't commit commented-out code without a very good reason. If the current libaccounts-sso plugin is terrible (which I can well believe) then we should just delete it (Bug #71230). @@ +289,5 @@ > +# ----------------------------------------------------------- > +AC_ARG_ENABLE(ubuntu-online-accounts, > + AS_HELP_STRING([--enable-ubuntu-online-accounts=@<:@no/yes/auto@:>@], > + [build Ubuntu Online Accounts plugins]), , > + enable_ubuntu_online_accounts=auto) If you use correct quoting (one level of [] per level of ()) then you should never need @<:@. AC_ARG_ENABLE([ubuntu-online-accounts], [AS_HELP_STRING([--enable-...=[no/yes/auto]], [build Ubuntu Online Accounts plugins]), [], [enable_ubuntu_online_accounts=auto]) @@ +291,5 @@ > + AS_HELP_STRING([--enable-ubuntu-online-accounts=@<:@no/yes/auto@:>@], > + [build Ubuntu Online Accounts plugins]), , > + enable_ubuntu_online_accounts=auto) > + > +if test "x$enable_ubuntu_online_accounts" != "xno"; then AS_IF, please. See https://bugs.freedesktop.org/show_bug.cgi?id=53445 @@ +300,5 @@ > + ], have_uoa="yes", have_uoa="no") > + > + # provider plugin dir > +# AC_MSG_CHECKING([Accounts provider plugin dir]) > +# ACCOUNTS_PROVIDER_PLUGIN_DIR=`pkg-config --variable=provider_plugindir account-plugin` Again, no commented-out code, please. ::: plugins/Makefile.am @@ +26,5 @@ > + $(ERROR_CFLAGS) > + > +pluginsdir = $(libdir) > +plugins_LTLIBRARIES = \ > + mcp-account-manager-uoa.la This should be conditionally-compiled. ::: plugins/empathy-webcredentials-monitor.c @@ +1,1 @@ > +#include "config.h" Licensing header? Implementation not reviewed; it would be good to have some sort of agreement on how this is meant to work in a "generic libaccounts" world. Either it's necessary, in which case libaccounts should have a way to do it, or it isn't. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-mission-control/issues/72. |
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.