GNOME Bugzilla – Bug 789852
Ensure extensions parts of a mode are from system, report back correct status for g-s-extension-prefs
Last modified: 2021-07-05 14:04:36 UTC
Created attachment 362886 [details] [review] [PATCH 1/6] Tag extensions as part of current mode As discussed on https://mail.gnome.org/archives/gnome-shell-list/2017-October/msg00034.html, enhance feedback on various tooling and secure extensions being part of a mode. Doing it with minimizing API changes: G-S: * Add a new "MODE" type for any extensions being part of current mode. This mode will only load system version of the extension (as mode can only be from the system anyway). * Prevent updating mode extensions installing in user's directory and report error accordingly (dbus and direct shell calls). Users can still go on another session (where this extension isn't a mode one) to be prompted. However, when switching back to session having this extension as a mode, the system one will be loaded. Same for uninstalling. * Note that we do pass the extension mode list as a parameter to not have misc/*.js depending on ui/main.js, which creates more issues in prefs when loading it. * Move as well the ExtensionState for the same reason. G-S-extension-prefs * Ensure g-s-extension-prefs reports correct status for MODE extensions. Previously, they were seen as disabled as g-s-e-p was relying on the gsettings enabled-key (from which mode extensions aren't listed). Implement and report via G-S dbus API the correct status (type and state of a mode). * Make switch activation (enabling/disabling an extension) dependant if an extension is of mode type. Note that the "MODE" type makes the system version loading mandatory and ENABLED status will be reported correctly. We can make that optional if desired for distribution to control this, however, it means exposing another "can_activate" or so activation status to control if the user can disable an extension (which they can't anyway for extension mode). Finally, there are some additional cleanups in the last patches (method renaming and such), which are completely optional. I'll, once those patches are accepted, modify Tweaks and the extension to honor enable/disable status properly and activation state. Do not hesitate if anything isn't clear or can be improved, attaching now the set of patches.
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?
Created attachment 367297 [details] [review] ExtensionSystem: enable removing session mode extension Add a 'disabled-extensions' to list session mode extension that shouldn't be loaded. This key is taking precedence of 'enabled-extensions'. Ensure depending in the case (user/system manually added extensions or extension enabled by a mode) that we update the correct key.
Created attachment 367298 [details] [review] Extension*: refactor adding utility functions Simplify some code by adding mode detection and getting session mode extensions helper.
Created attachment 367299 [details] [review] ShellDbus: API for enabling/disabling extensions Add an API for enabling or disabling extensions, ensuring we update the correct gsettings key to avoid duplication logic. Note that this API was already listed in the gsettings schema, but not implemented.
Created attachment 367300 [details] [review] prefs: Switch to dbus API to get extension live state As we don't want to rely in the gsettings key (not reflecting Mode extension real status), list extensions from the Shell dbus API. - mode extension will be reported as such, without needing to know which current mode session we are in. - state will be real state (mode extension weren't in the gsettings key previously, and so, weren't considered enabled) - update of status can be reflected via reloading from the Shell API. - we install the extension objects with same structure than the one in the Shell as well ExtensionUtils.extensions as preferences from some extension rely on this.
Created attachment 367301 [details] [review] prefs: base enablement and active status on extension object * Use the Shell reported state and type to determine if an extension is enabled or not. * Use D-Bus API for enablement/disablement. * Return OUT OF DATE status based on real extension state.
Created attachment 367302 [details] [review] prefs: Update state row when extension changes As we do not rely on the gsettings key anymore, update the state via the ExtensionStatusChanged signal. Update lightely the extension object state (do not reload all extensions as it was done previously).
Created attachment 367303 [details] [review] prefs: Show error if can't connect to the Shell Display an error if we can't reach GNOME Shell via D-BUS.
Created attachment 367304 [details] [review] prefs: Rename prefs detection method name to match what it does This method didn't check for extension avaibility but rather detects if extension has changed.
Created attachment 367305 [details] [review] extensionSystem: canEnable property on extensions As extensions can be enabled/disabled via gsettings key or be forced diabled with DISABLE_USER_EXTENSIONS_KEY, ensure that writable key status or any other changes reflects properly in the canEnable status. This will enable us to reflect that in 3rd party extensions UIs.
Created attachment 367306 [details] [review] prefs/dbus API: reflect canEnable extension status Reflect canEnable extension status in the Prefs UI. Related gsettings key writable state or DISABLE_USER_EXTENSIONS_KEY impacts extensions that you can enable/disable. Also, ensure the correct update signal is sent in case of changes. Finally, change the dbus API ExtensionStatusChanged to add this canEnable information. Extensions prefs UI needs to be updated for this API breakage as signature changes.
Here are the patchs on what was agreed upon: 1. enable disabling session mode extension, adding the 'disabled-extensions' gsettings key 2. ensure we report the correct status for session mode extension, not relying on the gsettings keys, but via the dbus API. I factored out the logic from -prefs to the shell itself, so that 3rd parties like Tweaks or chromium extensions report the same logic. 3. disable the extension in the various UI if the gsettings keys aren't writable (or user extension is disabled). Report it in the API without leaking the SESSION_MODE type. (just a new canEnable property). However, I still think as I made the case above that there ought to be an option to prevent loading user extensions matching system extensions being part of a mode. I would really like some feedbacks on the QA/Security case I presented above. But that doesn't prevent to get the review on this first patch set which isn't impacted by this.
One thing that always strikes me as odd in the Linux communities is why you can't just enhance the upstream version? Why always make a fork and then add to the confusion of the end users? Surly the better approach would be to apply the canonical resources to improving the original extension?
Paul, you are completely out of context here. The patches here are filed since November against the upstream version, to get that merged into the upstream source code itself. So basically, this is exactly what you encourage to do. Before submitting the patches, there was a conversation on the upstream GNOME Shell mailing list: https://mail.gnome.org/archives/gnome-shell-list/2017-October/msg00034.html in October. Unfortunately, no upstream answer. Similarly, the same patches have been resubmitted after the move to gitlab: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/1. Unfortunately, no upstream answer again. The only answer we got is from Florian here, then, you can read my answer asking for more details, and then, no answer back. You will thus realize that your comment here is completely unfair and out of context. Maybe you can get upstream to finally review and comment on those patches?
My Apologies Didier, I seem to have confused a paragraph from the mailing list email with this bug thread! My bad. I was referring to "ubuntu-dock" and "ubuntu-appindicator". I shall try and split my comments more clearly between this thread and that one.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.