<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: times new roman,new york,times,serif; font-size: 12pt; color: #000000'>Hi Chris,<br><br>thanks for taking the time to make this patch, these are some excellent ideas! (CC'ing engine-devel so that we can discuss this with other guys as well)<br><br>First of all, I really like the way you designed plugin source page URLs (going through <em>PluginSourcePageServlet</em>), e.g. "/webadmin/webadmin/plugin/<pluginName>/<pluginSourcePage>.html", plus the concept of "path" JSON attribute.<br><br><span style="font-style: italic;">WebadminDynamicHostingServlet</span> loads and caches all plugin definitions (<span style="font-style: italic;">*.json</span> files), and directly embeds them into WebAdmin host page as <span style="font-style: italic;">pluginDefinitions</span> JavaScript object. I'm assuming that <span style="font-style: italic;">pluginDefinitions</span> object will now look like this:<br><br><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">var pluginDefinitions = {</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "test": {</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "name": "test",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "version": "1.0",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "url": "/webadmin/webadmin/plugin/test/foo.html",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "path": "/tmp",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "config": {"a":1, "b":2, "c":3}</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> }</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">}</span><br><br>Originally, the <span style="font-style: italic;">pluginDefinitions</span> object looked like this:<br><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">var pluginDefinitions = {</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "test": "/webadmin/webadmin/plugin/test/foo.html" // Simple pluginName -> pluginSourcePageUrl mappings</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">}</span><br><br>This is because PluginManager (WebAdmin) only needs <span style="font-style: italic;">pluginName</span> ("name") and <span style="font-style: italic;">pluginSourcePageUrl</span> ("url") during startup, when creating plugin iframe. But this can be changed :)<br><br>Plugin "version" makes sense, plus the plugin configuration object ("config") can be useful directly on the client. Let me explain:<br><br>Originally, plugin configuration was supposed to be passed to actual plugin code (through immediately-invoked-function-expression, or IIFE), just like this:<br><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">(function (pluginApi, pluginConfig) { // JavaScript IIFE</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> // ... actual plugin code ...</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">})(</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> parent.pluginApi, /* reference to global pluginApi object */</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> {"a":1, "b":2, "c":3} /* embedded plugin configuration as JavaScript object */</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">);</span><br><br>The whole purpose of <span style="font-style: italic;">PluginSourcePageServlet</span> was to "wrap" actual plugin code into HTML, so that users don't need to write HTML pages for their plugins manually. <span style="font-style: italic;">PluginSourcePageServlet</span> would handle any plugin dependencies (placed into HTML head), with actual plugin code being wrapped into IIFE, as shown above. Plugin configuration was meant to be stored in a separate file, e.g. <span style="font-style: italic;"><pluginName>-config.json</span>, so that users could change the default plugin configuration to suit their needs.<br><br>Inspired by your patch, rather than reading/embedding plugin configuration when serving plugin HTML page (<span style="font-style: italic;">PluginSourcePageServlet</span>), it's even better to have the plugin configuration embedded directly into WebAdmin host page, along with introducing new <span style="font-style: italic;">pluginApi</span> function to retrieve the plugin configuration object.<br><br>Based on this, I suggest following modifications to the original concept:<br><br>- modify original <span style="font-style: italic;">pluginDefinitions</span> structure, from <span style="font-style: italic;">pluginName -> pluginSourcePageUrl</span>, to <span style="font-style: italic;">pluginName -> pluginDefObject</span><br>- <span style="font-style: italic;">pluginDefObject</span> is basically a subset of physical plugin definition (<span style="font-style: italic;">test.json</span>, see below), suitable for use on the client<br>- add following attributes to <span style="font-style: italic;">pluginDefObject</span>: <span style="font-style: italic;">version</span>, <span style="font-style: italic;">url</span>, <span style="font-style: italic;">config</span><br> * note #1: <span style="font-style: italic;">name</span> is not needed, since it's already the key of <span style="font-style: italic;">pluginName -> pluginDefObject</span> mapping<br> * note #2: <span style="font-style: italic;">path</span> is not needed on the client (more on this below)<br>- introduce <span style="font-style: italic;">pluginApi.config(pluginName)</span> function for plugins to retrieve their configuration object, and remove <span style="font-style: italic;">pluginConfig</span> parameter from main IIFE (as shown above)<br><br>[a] Physical plugin definition file (JSON) might be located at oVirt "DataDir", e.g. <span style="font-style: italic;">/usr/share/ovirt-engine/ui-plugins/test.json</span>, for example:<br><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">{</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "name": "test",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "version": "1.0",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "url": "/webadmin/webadmin/plugin/test/start.html",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "path": "/tmp",</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "config": "test-config.json"</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">}</span><br><br>[b] Plugin configuration file (JSON) might be located at oVirt "ConfigDir", e.g. <span style="font-style: italic;">/etc/ovirt-engine/ui-plugins/test-config.json</span>, for example:<br><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">{</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;"> "a":1, "b":2, "c":3</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">}</span><br><br>[c] Finally, plugin static resources (plugin source page, actual plugin code, plugin dependencies, CSS/images, etc.) would be located at <span style="font-style: italic;">/tmp</span> (as shown in [a]), for example:<br><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">/tmp/start.html -> plugin source page, used to load actual plugin code</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">/tmp/test.js -> actual plugin code</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">/tmp/deps/jquery-min.js -> simulate 3rd party plugin dependency</span><br><br>For example:<br>"/webadmin/webadmin/plugin/test/start.html" will be mapped to <span style="font-style: italic;">/tmp/start.html</span><br>"/webadmin/webadmin/plugin/test/deps/jquery-min.js" will be mapped to <span style="font-style: italic;">/tmp/deps/jquery-min.js</span><br><br>This approach has some pros and cons:<br>(+) plugin static resources can be served through <span style="font-style: italic;">PluginSourcePageServlet</span> (pretty much like oVirt documentation resources, served through oVirt Engine root war's <span style="font-style: italic;">FileServlet</span>)<br>(+) plugin author has complete control over plugin source page<br>(-) plugin author actually needs to write plugin source page<br><br>Overall, I think this approach is better than the previous one (where <span style="font-style: italic;">PluginSourcePageServlet</span> took care of rendering plugin source page, but sacrificed some flexibility).<br><br>By the way, here's what would happen behind the scenes:<br><ol><li>user requests WebAdmin host page, <span style="font-style: italic;">WebadminDynamicHostingServlet</span> loads and caches all plugin definitions [a] + plugin configurations [b] and constructs/embeds appropriate <span style="font-style: italic;">pluginDefinitions</span> JavaScript object<br><br></li><li>during WebAdmin startup, <span style="font-style: italic;">PluginManager</span> registers the plugin (name/version/url/config), and creates/attaches the iframe to fetch plugin source page ansynchronously<br><br></li><li><span style="font-style: italic;">PluginSourcePageServlet</span> handles plugin source page request, resolves the correct path [c] and just streams the file content back to client<br><br></li></ol>> 1. The plugin configuration files should probably have an "enabled"
field and an "apiVersion" field that should be examined to determine
whether or not to use the plugin.<br><br>Sounds good, we can implement these later on :)<br><br>> 2. I suspect the way I've modified PluginSourcePage makes it vulnerable to directory climbing attacks.<br><br>Yes, but we can defend against these, restricting access only to plugin's "path" and its sub-directories.<br><br>> 3. Is /usr/share/ovirt-engine the right place for the plugin config files?<br><br>I suppose you mean plugin definition files [a], cannot tell for sure, but we can change this anytime :)<br><br><br>Chris, please let me know what you think, and again - many thanks for sending the patch!<br><br><br>Regards,<br>Vojtech<br><br><br><hr id="zwchr"><br>From: "Chris Frantz" <Chris.Frantz@hp.com><br>To: vszocs@redhat.com<br>Sent: Wednesday, August 22, 2012 7:56:45 PM<br>Subject: UI Plugins configuration<br><br>Vojtech,<br><br>I decided to work on making the plugin patch a bit more configurable, following some of the ideas expressed by Itamar and others in the meeting yesterday. The attached patch is a simple first-attempt.<br><br>Plugin configurations are stored in /usr/share/ovirt-engine/ui-plugins/*.json.<br><br>Example:<br>{<br> "name": "test",<br> "version": "1.0",<br> "url": "/webadmin/webadmin/plugin/test/foo.html",<br> "path": "/tmp",<br> "config": {"a":1, "b":2, "c": 3}<br>}<br><br>The engine reads all of the *.json files in that directory to build the list of known plugins and gives that list to the webadmin.<br><br>When webadmin loads a plugin, it requests the URL given in the plugin config file. The "plugin" URL is mapped to PluginSourcePage, which will translate the first part of the path ("test") into whatever path is stored in pluginConfig ("/tmp") in this case, and then serve the static file (e.g. "/tmp/foo.html").<br><br>I didn't use the renderPluginSourcePage() method in favor of just serving a static file, but I have no strong opinion on the matter. However, a plugin may want to store static resources at "path" and have the engine serve those resources. By just serving files through PluginSourcePage, we don't need any other servlets to provide those resources.<br><br>There is still a bit of work to do:<br><br>1. The plugin configuration files should probably have an "enabled" field and an "apiVersion" field that should be examined to determine whether or not to use the plugin.<br><br>2. I suspect the way I've modified PluginSourcePage makes it vulnerable to directory climbing attacks.<br><br>3. Is /usr/share/ovirt-engine the right place for the plugin config files?<br><br>Let me know what you think,<br>--Chris<br><br></div></body></html>