Bug 66459 - Integrate new accounts-sso support into mission-control
Summary: Integrate new accounts-sso support into mission-control
Status: RESOLVED MOVED
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:
Whiteboard:
Keywords:
: 62814 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-07-01 14:23 UTC by Alex Fiestas
Modified: 2019-12-03 20:12 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
UOA patch (48.96 KB, patch)
2013-07-01 14:23 UTC, Alex Fiestas
Details | Splinter Review

Description Alex Fiestas 2013-07-01 14:23:52 UTC
Created attachment 81807 [details] [review]
UOA patch

We have split the  "UOA" support from emapthy into a patch that can be applied against mission-control, hoping that it will help on integrating UOA support into mainline mission-control.

The patch is called UOA, but it replaces the old accounts-sso support that comes from MeeGo with new code written by xclaesse.

The patch is still full with references from empathy, but maybe it is just good enough for you to pick it up, clean it and merge it.

As a final note, the support should be called "Accounts-SSO" or something similar, since it will add support not only for Ubuntu, but also for KDE and Jolla.

Thanks !
Comment 1 John Brooks 2013-07-02 07:26:17 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.
Comment 2 Xavier Claessens 2013-07-02 09:22:04 UTC
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.
Comment 3 Simon McVittie 2013-08-28 13:20:57 UTC
*** Bug 62814 has been marked as a duplicate of this bug. ***
Comment 4 Simon McVittie 2013-11-04 18:53:42 UTC
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 5 Simon McVittie 2013-11-04 19:01:40 UTC
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.
Comment 6 GitLab Migration User 2019-12-03 20:12:58 UTC
-- 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.