<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/&lt;pluginName&gt;/&lt;pluginSourcePage&gt;.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;">&nbsp;&nbsp;"test": {</span><br style="font-family: courier new,courier,monaco,monospace,sans-serif;"><span style="font-family: courier new,courier,monaco,monospace,sans-serif;">&nbsp;&nbsp; &nbsp;"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;">&nbsp;&nbsp; &nbsp;"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;">&nbsp;&nbsp; &nbsp;"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;">&nbsp;&nbsp; &nbsp;"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;">&nbsp;&nbsp; &nbsp;"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;">&nbsp;&nbsp;}</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;">&nbsp;&nbsp;"test": "/webadmin/webadmin/plugin/test/foo.html" // Simple pluginName -&gt; 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;">&nbsp;&nbsp;// ... 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;">&nbsp;&nbsp;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;">&nbsp;&nbsp;{"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;">&lt;pluginName&gt;-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 -&gt; pluginSourcePageUrl</span>, to <span style="font-style: italic;">pluginName -&gt; 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>&nbsp;&nbsp;* 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 -&gt; pluginDefObject</span> mapping<br>&nbsp;&nbsp;* 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;">&nbsp;&nbsp;"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;">&nbsp;&nbsp;"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;">&nbsp;&nbsp;"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;">&nbsp;&nbsp;"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;">&nbsp;&nbsp;"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;">&nbsp;&nbsp;"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 -&gt; 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 -&gt; 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 -&gt; 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>&gt; 1. &nbsp;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>&gt; 2. &nbsp;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>&gt; 3. &nbsp;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" &lt;Chris.Frantz@hp.com&gt;<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. &nbsp;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>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;"name": "test",<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;"version": "1.0",<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;"url": "/webadmin/webadmin/plugin/test/foo.html",<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;"path": "/tmp",<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;"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. &nbsp;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. &nbsp;However, a plugin may want to store static resources at "path" and have the engine serve those resources. &nbsp;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. &nbsp;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. &nbsp;I suspect the way I've modified PluginSourcePage makes it vulnerable to directory climbing attacks.<br><br>3. &nbsp;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>