[pve-devel] [PATCH v11 manager 0/3] fix#1710: add download from url button

[pve-devel] [PATCH v11 manager 0/3] fix#1710: add download from url button

[Lorenz Stechauner]

changes to v10:
* dropped already applied patches
* added "check" button - the gui now does not automatically send the metadata request anymore
* removed (visible) content type selector, because there was only one hard-coded option every time
* added loading mask while the metadata check is in progress

Lorenz Stechauner (3):
  api: nodes: add query_url_metadata method
  ui: Utils: change download task format
  fix #1710: ui: storage: add download from url button

 PVE/API2/Nodes.pm                   |  96 ++++++++++
 www/manager6/Utils.js               |   2 +-
 www/manager6/storage/Browser.js     |   8 +
 www/manager6/storage/ContentView.js | 262 +++++++++++++++++++++++++---
 4 files changed, 343 insertions(+), 25 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v11 manager 1/3] api: nodes: add query_url_metadata method

[Lorenz Stechauner]

metadata is gained using a HEAD request.

Due to the ability of this api endpoint to request files on internal
networks (which would not be visible/accessible from outside) it is
restricted to users with permissions `Sys.Audit` and `Sys.Modify` on
`/`. Users with these permissions are able to alter node (network)
config anyway, so this should not create any further security risk.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Nodes.pm | 96 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 76b0d85a..565cbccc 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -11,6 +11,7 @@ use JSON;
 use POSIX qw(LONG_MAX);
 use Time::Local qw(timegm_nocheck);
 use Socket;
+use IO::Socket::SSL;
 
 use PVE::API2Tools;
 use PVE::APLInfo;
@@ -231,6 +232,7 @@ __PACKAGE__->register_method ({
 	    { name => 'netstat' },
 	    { name => 'network' },
 	    { name => 'qemu' },
+	    { name => 'query-url-metadata' },
 	    { name => 'replication' },
 	    { name => 'report' },
 	    { name => 'rrd' }, # fixme: remove?
@@ -1493,6 +1495,100 @@ __PACKAGE__->register_method({
 	return $upid;
     }});
 
+__PACKAGE__->register_method({
+    name => 'query_url_metadata',
+    path => 'query-url-metadata',
+    method => 'GET',
+    description => "Query metadata of an URL: file size, file name and mime type.",
+    proxyto => 'node',
+    permissions => {
+	check => ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    url => {
+		description => "The URL to query the metadata from.",
+		type => 'string',
+		pattern => 'https?://.*',
+	    },
+	    'verify-certificates' => {
+		description => "If false, no SSL/TLS certificates will be verified.",
+		type => 'boolean',
+		optional => 1,
+		default => 1,
+	    }
+	},
+    },
+    returns => {
+	type => "object",
+	properties => {
+	    filename => {
+		type => 'string',
+		optional => 1,
+	    },
+	    size => {
+		type => 'integer',
+		renderer => 'bytes',
+		optional => 1,
+	    },
+	    mimetype => {
+		type => 'string',
+		optional => 1,
+	    },
+	},
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $url = $param->{url};
+
+	my $ua = LWP::UserAgent->new();
+
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	if ($dccfg->{http_proxy}) {
+	    $ua->proxy('http', $dccfg->{http_proxy});
+	}
+
+	my $verify = $param->{'verify-certificates'} // 1;
+	if (!$verify) {
+	    $ua->ssl_opts(
+		verify_hostname => 0,
+		SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE,
+	    );
+	}
+
+	my $req = HTTP::Request->new(HEAD => $url);
+	my $res = $ua->request($req);
+
+	die "invalid server response: '" . $res->status_line() . "'\n" if ($res->code() != 200);
+
+	my $size = $res->header("Content-Length");
+	my $disposition = $res->header("Content-Disposition");
+	my $type = $res->header("Content-Type");
+
+	my $filename;
+
+	if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) {
+	    $filename = $1;
+	} elsif ($url =~ m!^[^?]+/([^?/]*)(?:\?.*)?$!) {
+	    $filename = $1;
+	}
+
+	# Content-Type: text/html; charset=utf-8
+	if ($type && $type =~ m/^([^;]+);/) {
+	    $type = $1;
+	}
+
+	my $ret = {};
+	$ret->{filename} = $filename if $filename;
+	$ret->{size} = $size + 0 if $size;
+	$ret->{mimetype} = $type if $type;
+
+	return $ret;
+    }});
+
 __PACKAGE__->register_method({
     name => 'report',
     path => 'report',
-- 
2.30.2

[pve-devel] [PATCH v11 manager 2/3] ui: Utils: change download task format

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/Utils.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 3fdf37d3..c78467e7 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1782,7 +1782,7 @@ Ext.define('PVE.Utils', {
 	    clusterjoin: ['', gettext('Join Cluster')],
 	    dircreate: [gettext('Directory Storage'), gettext('Create')],
 	    dirremove: [gettext('Directory'), gettext('Remove')],
-	    download: ['', gettext('Download')],
+	    download: [gettext('File'), gettext('Download')],
 	    hamigrate: ['HA', gettext('Migrate')],
 	    hashutdown: ['HA', gettext('Shutdown')],
 	    hastart: ['HA', gettext('Start')],
-- 
2.30.2

[pve-devel] [PATCH v11 manager 3/3] fix #1710: ui: storage: add download from url button

[Lorenz Stechauner]

uses the common function PVE::Tools::download_file_from_url to
download a iso image or container template.

note: Only users with permissions `Sys.Audit` and `Sys.Modify` on
`/` are permitted to use the api endpoints due to security reasons.
(it is possible to download files from internal networks which would
be not visible/accessible from outside)

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/storage/Browser.js     |   8 +
 www/manager6/storage/ContentView.js | 262 +++++++++++++++++++++++++---
 2 files changed, 246 insertions(+), 24 deletions(-)

diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index 5fee94c7..fe5df3e2 100644
--- a/www/manager6/storage/Browser.js
+++ b/www/manager6/storage/Browser.js
@@ -53,6 +53,9 @@ Ext.define('PVE.storage.Browser', {
 	    let plugin = res.plugintype;
 	    let contents = res.content.split(',');
 
+	    let enableUpload = !!caps.storage['Datastore.AllocateTemplate'];
+	    let enableDownloadUrl = enableUpload && !!(caps.nodes['Sys.Audit'] && caps.nodes['Sys.Modify']);
+
 	    if (contents.includes('backup')) {
 		me.items.push({
 		    xtype: 'pveStorageBackupView',
@@ -91,6 +94,8 @@ Ext.define('PVE.storage.Browser', {
 		    itemId: 'contentIso',
 		    content: 'iso',
 		    pluginType: plugin,
+		    enableUploadButton: enableUpload,
+		    enableDownloadUrlButton: enableDownloadUrl,
 		    useUploadButton: true,
 		});
 	    }
@@ -101,6 +106,9 @@ Ext.define('PVE.storage.Browser', {
 		    iconCls: 'fa fa-file-o lxc',
 		    itemId: 'contentVztmpl',
 		    pluginType: plugin,
+		    enableUploadButton: enableUpload,
+		    enableDownloadUrlButton: enableDownloadUrl,
+		    useUploadButton: true,
 		});
 	    }
 	    if (contents.includes('snippets')) {
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index dd6df4b1..4dc394a0 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -191,6 +191,206 @@ Ext.define('PVE.storage.Upload', {
     },
 });
 
+Ext.define('PVE.storage.DownloadUrl', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveStorageDownloadUrl',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    isCreate: true,
+
+    method: 'POST',
+
+    showTaskViewer: true,
+
+    title: gettext('Download from URL'),
+    submitText: gettext('Download'),
+
+    cbindData: function(initialConfig) {
+	var me = this;
+	return {
+	    nodename: me.nodename,
+	    storage: me.storage,
+	    content: me.content,
+	};
+    },
+
+    cbind: {
+	url: '/nodes/{nodename}/storage/{storage}/download-url',
+    },
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	urlChange: function(field) {
+	    let me = this;
+	    let view = me.getView();
+	    field = view.down('[name=url]');
+	    field.setValidation(gettext("Please check URL"));
+	    field.validate();
+	    view.setValues({
+		size: gettext("unknown"),
+		mimetype: gettext("unknown"),
+	    });
+	},
+
+	urlCheck: function(field) {
+	    let me = this;
+	    let view = me.getView();
+	    field = view.down('[name=url]');
+	    view.setValues({
+		size: gettext("unknown"),
+		mimetype: gettext("unknown"),
+	    });
+	    Proxmox.Utils.API2Request({
+		url: `/nodes/${view.nodename}/query-url-metadata`,
+		method: 'GET',
+		params: {
+		    url: field.getValue(),
+		    'verify-certificates': view.getValues()['verify-certificates'],
+		},
+		waitMsgTarget: view,
+		failure: function(res, opt) {
+		    field.setValidation(res.result.message);
+		    field.validate();
+		},
+		success: function(res, opt) {
+		    field.setValidation();
+		    field.validate();
+
+		    let data = res.result.data;
+		    view.setValues({
+			filename: data.filename || "",
+			size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("unknown"),
+			mimetype: data.mimetype || gettext("unknown"),
+		    });
+		},
+	    });
+	},
+
+	hashChange: function(field) {
+	    let checksum = Ext.getCmp('downloadUrlChecksum');
+	    if (field.getValue() === '__default__') {
+		checksum.setDisabled(true);
+		checksum.setValue("");
+		checksum.allowBlank = true;
+	    } else {
+		checksum.setDisabled(false);
+		checksum.allowBlank = false;
+	    }
+	},
+    },
+
+    items: [
+	{
+	    xtype: 'inputpanel',
+	    border: false,
+	    columnT: [
+		{
+		    xtype: 'fieldcontainer',
+		    layout: 'hbox',
+		    fieldLabel: gettext('URL'),
+		    items: [
+			{
+			    xtype: 'textfield',
+			    name: 'url',
+			    allowBlank: false,
+			    flex: 1,
+			    listeners: {
+				change: 'urlChange',
+			    },
+			},
+			{
+			    xtype: 'button',
+			    name: 'check',
+			    text: gettext('Check'),
+			    margin: '0 0 0 5',
+			    listeners: {
+				click: 'urlCheck',
+			    },
+			},
+		    ],
+		},
+		{
+		    xtype: 'textfield',
+		    name: 'filename',
+		    allowBlank: false,
+		    fieldLabel: gettext('File name'),
+		},
+	    ],
+	    column1: [
+		{
+		    xtype: 'displayfield',
+		    name: 'size',
+		    fieldLabel: gettext('File size'),
+		    value: gettext('unknown'),
+		},
+	    ],
+	    column2: [
+		{
+		    xtype: 'displayfield',
+		    name: 'mimetype',
+		    fieldLabel: gettext('MIME type'),
+		    value: gettext('unknown'),
+		},
+	    ],
+	    advancedColumn1: [
+		{
+		    xtype: 'pveHashAlgorithmSelector',
+		    name: 'checksum-algorithm',
+		    fieldLabel: gettext('Hash algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		    listeners: {
+			change: 'hashChange',
+		    },
+		},
+		{
+		    xtype: 'textfield',
+		    name: 'checksum',
+		    fieldLabel: gettext('Checksum'),
+		    allowBlank: true,
+		    disabled: true,
+		    emptyText: gettext('none'),
+		    id: 'downloadUrlChecksum',
+		},
+	    ],
+	    advancedColumn2: [
+		{
+		    xtype: 'proxmoxcheckbox',
+		    name: 'verify-certificates',
+		    fieldLabel: gettext('Verify certificates'),
+		    uncheckedValue: 0,
+		    checked: true,
+		    listeners: {
+			change: 'urlChange',
+		    },
+		},
+	    ],
+	},
+	{
+	    xtype: 'hiddenfield',
+	    name: 'content',
+	    cbind: {
+		value: '{content}',
+	    },
+	},
+    ],
+
+    initComponent: function() {
+        var me = this;
+
+	if (!me.nodename) {
+	    throw "no node name specified";
+	}
+	if (!me.storage) {
+	    throw "no storage ID specified";
+	}
+
+        me.callParent();
+    },
+});
+
 Ext.define('PVE.storage.ContentView', {
     extend: 'Ext.grid.GridPanel',
 
@@ -249,36 +449,50 @@ Ext.define('PVE.storage.ContentView', {
 
 	Proxmox.Utils.monStoreErrors(me, store);
 
-	let uploadButton = Ext.create('Proxmox.button.Button', {
-	    text: gettext('Upload'),
-	    handler: function() {
-		let win = Ext.create('PVE.storage.Upload', {
-		    nodename: nodename,
-		    storage: storage,
-		    contents: [content],
-		});
-		win.show();
-		win.on('destroy', reload);
-	    },
-	});
-
-	let removeButton = Ext.create('Proxmox.button.StdRemoveButton', {
-	    selModel: sm,
-	    delay: 5,
-	    callback: function() {
-		reload();
-	    },
-	    baseurl: baseurl + '/',
-	});
-
 	if (!me.tbar) {
 	    me.tbar = [];
 	}
 	if (me.useUploadButton) {
-	    me.tbar.push(uploadButton);
+	    me.tbar.unshift(
+		{
+		    xtype: 'button',
+		    text: gettext('Upload'),
+		    disabled: !me.enableUploadButton,
+		    handler: function() {
+			Ext.create('PVE.storage.Upload', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    taskDone: () => reload(),
+			});
+		    },
+		},
+		{
+		    xtype: 'button',
+		    text: gettext('Download from URL'),
+		    disabled: !me.enableDownloadUrlButton,
+		    handler: function() {
+			Ext.create('PVE.storage.DownloadUrl', {
+			    nodename: nodename,
+			    storage: storage,
+			    content: content,
+			    autoShow: true,
+			    taskDone: () => reload(),
+			});
+		    },
+		},
+		'-',
+	    );
 	}
 	if (!me.useCustomRemoveButton) {
-	    me.tbar.push(removeButton);
+	    me.tbar.push({
+		xtype: 'proxmoxStdRemoveButton',
+		selModel: sm,
+		delay: 5,
+		callback: () => reload(),
+		baseurl: baseurl + '/',
+	    });
 	}
 	me.tbar.push(
 	    '->',
-- 
2.30.2

Re: [pve-devel] [PATCH v11 manager 0/3] fix#1710: add download from url button

[Dominik Csapak]

works, patches look good

Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>

On 7/1/21 10:50, Lorenz Stechauner wrote:
> changes to v10:
> * dropped already applied patches
> * added "check" button - the gui now does not automatically send the metadata request anymore
> * removed (visible) content type selector, because there was only one hard-coded option every time
> * added loading mask while the metadata check is in progress
> 
> Lorenz Stechauner (3):
>    api: nodes: add query_url_metadata method
>    ui: Utils: change download task format
>    fix #1710: ui: storage: add download from url button
> 
>   PVE/API2/Nodes.pm                   |  96 ++++++++++
>   www/manager6/Utils.js               |   2 +-
>   www/manager6/storage/Browser.js     |   8 +
>   www/manager6/storage/ContentView.js | 262 +++++++++++++++++++++++++---
>   4 files changed, 343 insertions(+), 25 deletions(-)
> 

[pve-devel] applied: [PATCH v11 manager 0/3] fix#1710: add download from url button

[Thomas Lamprecht]

On 01.07.21 10:50, Lorenz Stechauner wrote:
> changes to v10:
> * dropped already applied patches
> * added "check" button - the gui now does not automatically send the metadata request anymore
> * removed (visible) content type selector, because there was only one hard-coded option every time
> * added loading mask while the metadata check is in progress
> 
> Lorenz Stechauner (3):
>   api: nodes: add query_url_metadata method
>   ui: Utils: change download task format
>   fix #1710: ui: storage: add download from url button
> 
>  PVE/API2/Nodes.pm                   |  96 ++++++++++
>  www/manager6/Utils.js               |   2 +-
>  www/manager6/storage/Browser.js     |   8 +
>  www/manager6/storage/ContentView.js | 262 +++++++++++++++++++++++++---
>  4 files changed, 343 insertions(+), 25 deletions(-)
> 


applied series, thanks! Did some followups though: 

- I moved out the download window into its own file, was a bit out of place in the storage
  content view and adding 200+ lines code is just better down in a separate file, if not
  really general code to the existing content view components
- I found the event logic a bit hard to grasp, reworked it with a view model
- Renamed `Check` to `Query URL`, as when trying to think as user I had no idea what
  `Check` would actually do.
- disabled the `Query URL` button when done so successfully, any change in URL or verify
  state re-enables it, it stays also enabled if the query itself fails, as that could 
  have been a (network) hiccup, where allowing one to requery makes sense from UX POV.
- do not invalidate size/mime on verify cert check toggle, makes no sense to me, the
  last OK queried info will still be valid.

Above could have been noticed earlier on review, sorry for that, but as I was pretty
swamped to take a closer look and there are others which can review such things I'm
so free to not take the whole blame ;-) 

In general I wonder if we could do the metadata query also client side, we could
just do a HEAD request from there which would be much nicer, but there are also
cases where an admin may enter an internal URL or the DNS the browsers queries is
another than the PVE hosts have, so while I think it would cover most of the
actual cases in practice (as even with internal URLs, the admin often access PVE
also from an internal network), it's still not always doable. On the other hand,
it would drop any remaining concern about doing some http request from the PVE
side.

just noting for the record, we can always switch to that in the future and sunset
or further-restrict the backend querier if we want...