GNOME Bugzilla – Bug 729173
Offer a way for admins to selectively disable providers
Last modified: 2014-06-05 14:17:22 UTC
Some site administrators of large GNOME deployments have asked for a way to selectively disable providers. Often, this is because of corporate policies that restrict employees from using certain online service providers at work. I can think of multiple ways to do this. One option is to have a white or black list in the form of a GSettings key that is locked down by the site administrator. The other would be to split the providers out into loadable modules that can be put into separate sub-packages by the distributor, and the site administrator can choose which ones to install. I am currently leaning in favour of the second option because it goes well with the fact that the providers are implemented as GIOExtensions. As long as we build the modules from the same tree as the rest of the code base, we would not have to deal with problems (eg., stable plugin interface) posed by 3rd party plugins.
I have split up the providers into loadable modules in the wip/modules branch.
We should teach the gnome-control-center UI to adapt itself when certain classes of providers are not installed.
So I had a quick look over your branch. - I do think making these extensions into loadable modules has the potential for causing headaches. These plugins are going to be loaded by users of gnome-online-accounts, not just goa-daemon, right? At a minimum, the extensions should be marked un-unloadable since they use types and libraries that are not unloadable. - Even if you do decide to make the extensions into loaded modules, it might make sense to be consistent with gnome-settings-daemon and provide a whitelisted-plugins gsettings key (that defaults to the magic value "['all']"). The login screen could then set it to "[]" in case goa-daemon gets accidentally activated (see bug 707486) - One small thing I noticed is you load the no-longer-built-in extensions in a function called ensure_builtins_loaded. I think it would make sense to rename the function to ensure_extensions_loaded. - As an aside, glib now has a function g_type_ensure() that would be a little cleaner way of achieving what ensure_builtins_loaded used to do. I guess that moot now, though, since it's going away. - From a cursor-y look, the changes seem alright, in general. - I think only starting the identity service if the kerberos provider is available makes sense.
(In reply to comment #3) > - I think only starting the identity service if the kerberos provider is > available makes sense. Isn't that already the case: https://git.gnome.org/browse/gnome-online-accounts/commit/?h=wip/modules&id=fe21ad712118880884beb6e58fadb83c6eb8c90b ?
yes, i was saying it makes sense. It's the main difference between this provider and the other providers. You specifically asked me to evaluate this provider because it was a little different than others, so I was just affirming that I think what you're doing is right.
Created attachment 276790 [details] [review] build: Clean up uses of _CFLAGS
Created attachment 276791 [details] [review] telepathy: Silence -Wunused-but-set-variable
As an alternative to the modules there is a whitelist implementation in the wip/whitelist branch.
Created attachment 277534 [details] [review] provider: Use g_type_ensure for registering types
Created attachment 277535 [details] [review] Consolidate the provider types and extension point names
Created attachment 277536 [details] [review] Add a whitelist for providers
Review of attachment 277534 [details] [review]: Committed.
Review of attachment 277535 [details] [review]: seems like a reasonable clean up
Created attachment 277740 [details] [review] Add a whitelist for providers
Review of attachment 277740 [details] [review]: cool. looks pretty good to me. ::: configure.ac @@ +425,3 @@ # +IT_PROG_INTLTOOL([0.50.1]) why this change? ::: src/goabackend/goaprovider.c @@ -764,3 @@ - * important because it affects the order in which they are - * returned by goa_provider_get_all. - */ maybe preserve this comment? @@ +793,3 @@ #endif + {NULL, NULL} +}; spaces in braces @@ +832,3 @@ + /* Otherwise try them one by one. */ + g_debug ("Loading whitelisted providers: "); + for (i = 0; !all && ordered_builtins_map[i].name != NULL; i++) maybe: if (all) goto cleanup; or so before the loop, so you don't check if all is set every itereation of the loop. Compiler probably optimizes it out anyway, though, so probably doesn't matter.
(In reply to comment #15) > Review of attachment 277740 [details] [review]: > ::: configure.ac > @@ +425,3 @@ > # > > +IT_PROG_INTLTOOL([0.50.1]) > > why this change? We are relying on intltool's ability to extract translatable strings from GSettings schema files without the use of underscore prefixed nodes. Otherwise we will have to use an extra level of .in with '_summary' and '_description'. This is only possibly with intltool >= 0.50.1.
Created attachment 277789 [details] [review] Add a whitelist for providers
(In reply to comment #16) > We are relying on intltool's ability to extract translatable strings from > GSettings schema files without the use of underscore prefixed nodes. Otherwise > we will have to use an extra level of .in with '_summary' and '_description'. > This is only possibly with intltool >= 0.50.1. Alright, it's a a little confusing because $INTLTOOL_REQUIRED was used before but not defined. My first thought was there was some INTLTOOL_REQUIRED= line being left around. It might make sense to do this part as its own commit so you can highlight that you're also mopping up an undefined variable.
Created attachment 277859 [details] [review] build: Remove unused variable
Created attachment 277860 [details] [review] Add a whitelist for providers
Review of attachment 277859 [details] [review]: ++
Review of attachment 277860 [details] [review]: Looks fine, though I would say "modeled off gnome-settings-daemon whitelist" instead of "Based on ideas from Ray Strode" since casual reader might want to see the g-s-d code if they knew about it.
(In reply to comment #22) > Review of attachment 277860 [details] [review]: > > Looks fine, though I would say "modeled off gnome-settings-daemon whitelist" > instead of "Based on ideas from Ray Strode" since casual reader might want to > see the g-s-d code if they knew about it. Done.