Created attachment 362887 [details] [review]
[PATCH 2/6] Don't allow installing/updating/removing extension mode

Created attachment 362888 [details] [review]
[PATCH 3/6] Switch to dbus API in -prefs to get extension live state

Created attachment 362889 [details] [review]
[PATCH 4/6] Base enablement and active status on extension object

Created attachment 362890 [details] [review]
[PATCH 5/6] Update state row when extension changes

Created attachment 362891 [details] [review]
[PATCH 6/6] Rename prefs detection method name to match what it does

I've been busy with downstream work recently, so this went on the todo-pile. Most of this will require some design discussion - see bug 689781 for an old unresolved one - to figure out the desired behavior. That is, whether session mode extensions should:
 - not show up at all
 - show up disabled
 - a mix of both (don't show it if it is only installed system-wide,
   show it disabled if the user also installed it explicitly)

I'll bring this up at the UX hackfest.

Review of attachment 362888 [details] [review]:

If you want a port to the D-Bus extension API, then please do it properly - shoehorning this into ExtensionFinder is a terrible hack, don't do that.

Also: In a follow-up patch, add an empty state for the case where the tool is not run under gnome-shell

Review of attachment 362891 [details] [review]:

Yeah, the variable name is a left-over from when the tool only allowed interacting with extensions with preferences. Good to commit with the commit message fixed:
 "but rather detects if extension has changed" => "but rather detects whether the extension has preferences"

(In reply to Florian Müllner from comment #7)
> Review of attachment 362888 [details] [review] [review]:
> 
> If you want a port to the D-Bus extension API, then please do it properly -
> shoehorning this into ExtensionFinder is a terrible hack, don't do that.
> 

That was my first approach, however, when loading the .prefs from multiple extensions, I saw that some of them are relying on ExtensionUtils.extensions array to provide all existing extensions. Which is why I went to this approach from loading from the disk, and get additional updated metatada from dbus. Also, the object on dbus hasn't the same layout than the one in the "extensions" variable.

What do you think should be done? I'm happy to load the list and state from dbus, rebuilding extension objects with the expected structure (with the .meta properties), but it means as well to not break extensions I have in -prefs to override ExtensionUtils.extensions? Or I can do this still in ExtensionFinder, but only relying on dbus data and not loading from disk?

Tell me what you think is the best approach for this.

Created attachment 363491 [details] [review]
Switch to dbus API in -prefs to get extension live state

Here is an approach I tried to take (refreshing patch 3 & 4): getting all informations frol dbus, but registering in the global ExtensionUtils.extensions still.

The gotcha is that there isn't shell-version or some metadata which aren't serialized from the dbus API, even if I whitelist them, their type is object or undefined.

Tell me what you think about those changes.

Created attachment 363492 [details] [review]
[PATCH 4/6] Base enablement and active status on extension object

Created attachment 363516 [details] [review]
[PATCH] Show error if can't connect to the Shell

Review of attachment 362886 [details] [review]:

::: js/ui/extensionDownloader.js
@@ +14,3 @@
 const ExtensionSystem = imports.ui.extensionSystem;
 const FileUtils = imports.misc.fileUtils;
+const Main = imports.ui.main;

This should be probably part of the patch 2 instead?

As mentioned, we briefly discussed this at the UX hackfest. We started out with the following options:

 1. show session-enabled extensions enabled but insensitive

 2. hide session-enabled extensions

 3. a mix of 1+2: show session-enabled extensions as insensitive if
    the user also has it installed in their home, hide it otherwise

The arguments for options 1. and 2. are essentially the discussion from bug 689781 - on the one hand, that a session is partially implemented via extensions should be an implementation detail, but on the other hand, the user may still want to access the settings.

The 3. option tries to compromise between the other two options, but in the end we weren't really happy with either approach:
Presenting the user with settings that they cannot change is somewhat rude, while showing a different list of extensions depending on how the user logged in is confusing.

So we ended up with a 4. option that's quite different:

 4. make the switch work

This clearly departs from the notion that session-enabled extensions are an implementation detail, but then all the options "leak" that information some way or another. It's still slightly odd to present users with enabled extensions that they did not actually enable themselves, but that's really inherit to the concept of session-mode extensions. But at least from then on, everything would work just as users expect - enabled extensions show up as enabled, and turning them off disables them.

I'm aware that your QA will probably not be overly happy with that solution (I *suspect* that RHEL QA will mind less, at least that's what I hope), but then you cannot really guarantee any extension state unless you seriously lock down the system:

https://extensions.gnome.org/extension/1290/disable-ubuntu-dock/

(I don't know how useful that link is, as the extension is still stuck in review for using the Ubuntu trademark in its name - since its first version it has been updated to a generic "turn-off-session-mode-extensions"-extension)

Now, assuming that we'd implement the behavior with a complimentary 'disabled-extensions' GSetting:
If you really must, we could add a SESSION_MODE type and make those switches insensitive if the 'disabled-extensions' setting isn't writable, so you could still replicate that behavior from your patches by locking down the key.

I'm less sold on forcing mode extensions to load from the system directories, but not outright opposed. However I definitively don't like throwing obscure errors at the user - having the session determine what users can or cannot install is a seriously weird experience. I also doubt that this is too much of a problem in practice, as the user already has the functionality available via the system extension, so I only suspect a small subset to care enough about some specific functionality to want to 'reinstall' a newer version from e.g.o - and I suspect that a large subset of that subset can figure out ways of working around any restrictions ;-) (like disabling the session-mode extension via an extension, then change the UUID of the 'upstream' extension - or simply brute-force their way by downloading the extension zip and overwrite the files from the extension package).

FWIW, we would actually like to phase out the browser plugin in favor of the extension support in Software. As part of that, my stance on extension updates is that they should be made part of offline updates - the unfinished update mechanism we have now dates back to a time where gjs was much less powerful, but nowadays extensions may register custom GTypes (by subclassing a GObject class) which cannot simply be reregistered with an updated version. That's clearly out of scope for this bug, but I'd rather see work towards that than throwing in some quick "sorry Dave, I cannot let you do that" errors ;-)

Thanks for your detailed answer!

I would maybe surprise you, but I don't care that much for people to be able to remove extensions being part of a mode. Indeed, we are not in a lock down system or kiosk mode, people can change their default file manager, package manager and even Shell! I just thought that GNOME Shell didn't allow disabling extension mode was a design decision and just not an implementation oversight. So I started from this wrong precept :) I don't think our QA team will be against this as well for the same reasons. In addition to this, that would enable us to remove our hack in our dock extension to let people loading dash to dock, for instance (or any other 3rd party extension), so that's a benefit.

Consequently, I'm happy to implement this design if desired with the additional 'disabled-extensions' GSettings key. I think the goal then, is to keep the whole logic in the Shell itelf (removing the extension from enabled-extensions vs adding it to disabled-extensions, and same for enabling it) and move both -prefs and Tweaks to only rely on the D-BUS API Shell API, correct?
However, imagine a distribution is locking down 'disabled-extensions" or "enabled-extensions" keys (and I don't think Ubuntu will), we need then a way to communicate that to those third-parties apps, no? So we will still need a can_enable_disable (needing a better name!) properties on the extensions as part of the API, or am I missing something as they don't know at all about the gsettings key? Also, the Shell needs to track mode extension to know if they need to update disabled-extensions, or do you want to always add to it, unconditionnally, which doesn't seem optimal?

On upgrade and loading system version however, that's the part I, and our QA team, are more assertive of. Let me try to contextualize the issue. Of course, we don't (and can't even if we wanted to, which isn't the case) prevent people who like to play with their computers to dive into their system and workaround/change anything (like the extension to disable it or people changing the UIID). We are not on a kiosk system again, and that would be silly. But as well, we don't want totally legit user to inadvertly end up in an unsupported state without knowing it.

Everything that we install by default are coming from the ubuntu archive, which is QA tested. Any updates thus need as well to come from the same source repository. We have a SRU process involving each bug fix upload to be confirmed and a staging period of an entire week to spot potential regressions. Also, for a given release, no new features or behavior changes are allowed. Having Ubuntu Dock or Ubuntu appindicator extensions coming from the archive, installed by default, and then, getting updates directly from extensions.gnome.org circumvent this whole process.

What does it means?
* it means either we never upload Ubuntu Dock or Ubuntu Appindicator to e.g.o. However, that leave a security gap, where if someone with slippery fingers approves by mistake a malicious extension with those names on the website, then, the extension malicious author can own any ubuntu desktop system. Not sure we can live happily with this.
* another possibility is to push an "empty" extension that we never update on e.g.o just to own the name, which is what is waiting on review. I don't really like that approach which is a hack.
* we can also only push the first version of the extension on e.g.o, and never update it (as we don't want behavior changes and we support multiple releases in parallel). Not really a satisfactory solution either. Also note that I got requests from users wanting to get our Ubuntu changes to dash to dock published on the extension website.

This also forces us to keep our Ubuntu appindicator fork, which only changes the UUID for that reason, where we wanted to reuse the KStatutsNofier one directly available.

Hence the "for this set of extension, please don't rely on the website" request. Even if the web chrome/firefox extension is going to be retired, it's the most popular solution right now to install an extension and will be available by the time 18.04 LTS is released. We don't want people enabling this and clicking on the innocent "an update is available for…" to get into that situation. This was why there was the proposal of "don't allow updates" + "always load system version if current mode enables it". Indeed, the issue is that if we only block current session for updates, people switch to another session, they will be prompted to upgrade this extension, they will upgrade to it. Let's say for whatever reason they switch back to the original session, the user is back in an unsupported state without being aware of it (as he got the prompt and thought this was ok).

Do you see any way we can solve this in any manner if preventing updates + loading system version isn't possible?