skillbert / alt1-electron

Experimantal electron implementation of Alt1 Toolkit
GNU General Public License v3.0
57 stars 15 forks source link

Add app settings #20

Open mimvdb opened 1 year ago

mimvdb commented 1 year ago

Add ability to add and remove apps from the settings.

The capture settings from the previous settings are in a separate tab, but don't work (they didn't work before either)

This uses the same ipc mechanism as the alt1 API, for security it checks the webcontent id against a list of allowed admins when doing privileges actions like updating the settings.

When #18 is merged, a line should be added to the end of the uninstallApp function: updateTray();

image

mimvdb commented 1 year ago

Thanks for the review.

I'm worried that this change is a little too far ahead of the underlying infrastructure. All config changes should trigger an event in the main process that gets forwarded to all admin windows, which should in turn be the trigger to update ui state. Something similar to RsInstance.emitAppEvent, but not specific to an rs window.

Yeah makes sense, a lot less state to worry about that way.

Also a proper concept of what an "admin" app means exactly should be discussed.

Besides admin just being the first word that came to mind when punching a hole through the sandbox, the following is what I think of when I say admin window:

Some window using the web UI capabilities of electron, but one that should not be restricted in its usage of higher privilege functions. It should therefore not be an app (something I associate with anything 3rd party, like the alt1 apps). Any admin window should be a locally developed UI, and not load any foreign assets to prevent privilege escalation.

I don't think the admin part should be more tied down than that. For example, while I think for some admin windows it might be useful to be attached to a specific RS instance (e.g. a toolbar), it isn't the case for all (like the settings).

@skillbert Any comments?

skillbert commented 1 year ago

What i was talking about is whether it could be a subclass of ManagedWindow (aka: app window). However it doesn't look like there is quite enough overlap and would cause more trouble than it solves.

I looked at it some more and i think keeping a list of trusted frames makes sense (like you did with admins). That list can then also be used to deliver events.

mimvdb commented 1 year ago

A lot more changes this time because all non-readonly accesses to settings must change to somehow trigger the event. At first I was planning on encapsulating everything in ManagedSettings but sometimes there is just a simple toggle like app.wasOpen = true that I just added the emit() to.

I made a seperate AppConfig because the files were already split, but it might be more convenient to combine it with ManagedSettings. Now ManagedSettings just subscribes to the changed event from AppConfig and then emits its own changed.

@skillbert ready for review