Re: [pve-devel] [PATCH v3 manager 2/2] fix #1710: add retrieve from url button for storage

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 04.05.21 10:57, Lorenz Stechauner wrote:
> Add PVE.storage.Retrieve window and PVE.form.hashAlgorithmSelector.
> Users are now able to download/retrieve any .iso/... file onto their
> storages and verify file integrity with checksums.
> 
> Add new method: GET /nodes/{node}/urlmeta - returns url metadata

I'd separate backend and gui into at least two patches, e.g., the hash algorithm
widget has nothing directly to do with querying metadata from an URL.

Also, you do not handle the datacenter.cfg http_proxy setting, neither here
nor in the storage patch.

> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Nodes.pm                          |  97 +++++++
>  www/manager6/Makefile                      |   1 +
>  www/manager6/Utils.js                      |   1 +
>  www/manager6/form/HashAlgorithmSelector.js |  16 ++
>  www/manager6/storage/Browser.js            |   8 +
>  www/manager6/storage/ContentView.js        | 281 +++++++++++++++++++--
>  6 files changed, 379 insertions(+), 25 deletions(-)
>  create mode 100644 www/manager6/form/HashAlgorithmSelector.js
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index ba6621c6..c2407d99 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;
> @@ -254,6 +255,7 @@ __PACKAGE__->register_method ({
>  	    { name => 'tasks' },
>  	    { name => 'termproxy' },
>  	    { name => 'time' },
> +	    { name => 'urlmeta' },
>  	    { name => 'version' },
>  	    { name => 'vncshell' },
>  	    { name => 'vzdump' },
> @@ -1596,6 +1598,101 @@ __PACKAGE__->register_method({
>  	return $rpcenv->fork_worker('download', undef, $user, $worker);
>      }});
>  
> +
> +__PACKAGE__->register_method({
> +    name => 'urlmeta',
> +    path => 'urlmeta',

If one would just read this when doing a pvesh ls /nodes/foo they'd be probably
have no idea what this would possibly do...

Rather use something like "retrieve-url-metadata" or "query-url-metadata".

> +    method => 'GET',
> +    description => "Download templates and ISO images by using an URL.",

But, this does not actually downloads any template, just queries some
information about an url?

> +    proxyto => 'node',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
> +    },
> +    protected => 1,

why is this protected? This can run as unprivileged www user just fine, or?

> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    url => {
> +		description => "The URL to retrieve the file from.",
> +		type => 'string',
> +	    },
> +	    insecure => {
> +		description => "Allow TLS certificates to be invalid.",
> +		type => 'boolean',
> +		optional => 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};
> +
> +	die "invalid https or http url"
> +	    if $url !~ qr!^https?://!;

this could get handled directly in the schema as regex

> +
> +	my $ua = LWP::UserAgent->new();
> +	$ua->ssl_opts(
> +	    verify_hostname => 0,
> +	    SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE,
> +	) if $param->{insecure};

that post-if is easily overlooked and goes against our style guide
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If

Use a "normal" if () { here

> +
> +	my $req = HTTP::Request->new(HEAD => $url);
> +	my $res = $ua->request($req);
> +
> +	die "invalid server response: '" . $res->status_line() . "'"
> +	    if ($res->code() != 200);
> +
> +	my $size = $res->header("Content-Length");
> +	my $dispo = $res->header("Content-Disposition");

dispo? please avoid truncating names if not really a common abbreviation where
the code would actually benefit from the shorter version.

> +	my $type = $res->header("Content-Type");
> +
> +	my $filename;
> +
> +	if ($dispo && $dispo =~ 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;

if would fit in same line as expression

> +
> +	$ret->{size} = $size + 0
> +	    if $size;

if would fit in same line as expression, also you may want to check for definendess
here.

> +
> +	$ret->{mimetype} = $type
> +	    if $type;

if would fit in same line as expression

> +
> +	return $ret;
> +    }});
> +
>  __PACKAGE__->register_method({
>      name => 'report',
>      path => 'report',
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index afed3283..8e6557d8 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -38,6 +38,7 @@ JSSRC= 							\
>  	form/GlobalSearchField.js			\
>  	form/GroupSelector.js				\
>  	form/GuestIDSelector.js				\
> +	form/HashAlgorithmSelector.js			\
>  	form/HotplugFeatureSelector.js			\
>  	form/IPProtocolSelector.js			\
>  	form/IPRefSelector.js				\
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 581d2040..b7479e7a 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1826,6 +1826,7 @@ Ext.define('PVE.Utils', {
>  	    hastop: ['HA', gettext('Stop')],
>  	    imgcopy: ['', gettext('Copy data')],
>  	    imgdel: ['', gettext('Erase data')],
> +	    imgdownload: [gettext('Storage'), gettext('Download data')],
>  	    lvmcreate: [gettext('LVM Storage'), gettext('Create')],
>  	    lvmthincreate: [gettext('LVM-Thin Storage'), gettext('Create')],
>  	    migrateall: ['', gettext('Migrate all VMs and Containers')],
> diff --git a/www/manager6/form/HashAlgorithmSelector.js b/www/manager6/form/HashAlgorithmSelector.js
> new file mode 100644
> index 00000000..5ae7a08b
> --- /dev/null
> +++ b/www/manager6/form/HashAlgorithmSelector.js
> @@ -0,0 +1,16 @@
> +Ext.define('PVE.form.hashAlgorithmSelector', {
> +    extend: 'Proxmox.form.KVComboBox',
> +    alias: ['widget.pveHashAlgorithmSelector'],
> +    config: {
> +	deleteEmpty: false,
> +    },
> +    comboItems: [
> +	['__default__', 'None'],
> +	['md5', 'MD5'],
> +	['sha1', 'SHA-1'],
> +	['sha224', 'SHA-224'],
> +	['sha256', 'SHA-256'],
> +	['sha384', 'SHA-384'],
> +	['sha512', 'SHA-512'],
> +    ],
> +});
> diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
> index 5fee94c7..da3e66c8 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 enableRetrieve = !!(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,
> +		    enableRetrieveButton: enableUpload && enableRetrieve,
>  		    useUploadButton: true,
>  		});
>  	    }
> @@ -101,6 +106,9 @@ Ext.define('PVE.storage.Browser', {
>  		    iconCls: 'fa fa-file-o lxc',
>  		    itemId: 'contentVztmpl',
>  		    pluginType: plugin,
> +		    enableUploadButton: enableUpload,
> +		    enableRetrieveButton: enableUpload && enableRetrieve,
> +		    useUploadButton: true,
>  		});
>  	    }
>  	    if (contents.includes('snippets')) {
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index dd6df4b1..d01526e1 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -191,6 +191,213 @@ Ext.define('PVE.storage.Upload', {
>      },
>  });
>  
> +Ext.define('PVE.storage.Retrieve', {
> +    extend: 'Proxmox.window.Edit',
> +    alias: 'widget.pveStorageRetrieve',
> +
> +    isCreate: true,
> +
> +    showTaskViewer: true,
> +
> +    title: gettext('Retrieve from URL'),
> +    submitText: gettext('Download'),
> +
> +    id: 'retrieve',
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	urlChange: function(field) {
> +	    let me = Ext.getCmp('retrieve');
> +	    field = me.down('[name=url]');
> +	    field.setValidation("Waiting for response...");
> +	    field.validate();
> +	    me.setValues({size: ""});
> +	    Proxmox.Utils.API2Request({
> +		url: `/nodes/${me.nodename}/urlmeta`,
> +		method: 'GET',
> +		params: {
> +		    url: field.getValue(),
> +		    insecure: me.getValues()['insecure'],
> +		},
> +		failure: function(res, opt) {
> +		    field.setValidation(res.result.message);
> +		    field.validate();
> +		    me.setValues({
> +			size: "",
> +			mimetype: "",
> +		    });
> +		},
> +		success: function(res, opt) {
> +		    field.setValidation();
> +		    field.validate();
> +
> +		    let data = res.result.data;
> +		    me.setValues({
> +			filename: data.filename || "",
> +			size: data.size && Proxmox.Utils.format_size(data.size) || "",
> +			mimetype: data.mimetype || "",
> +		    });
> +		},
> +	    });
> +	},
> +
> +	hashChange: function(field) {
> +	    let cecksum = Ext.getCmp('retrieveChecksum');
> +	    if (field.getValue() === '__default__') {
> +		cecksum.setDisabled(true);
> +		cecksum.setValue("");
> +		cecksum.allowBlank = true;
> +	    } else {
> +		cecksum.setDisabled(false);
> +		cecksum.allowBlank = false;
> +	    }
> +	},
> +
> +	typeChange: function(field) {
> +	    let me = Ext.getCmp('retrieve');
> +	    let content = me.getValues()['content'];
> +	    let type = field.getValue();
> +
> +	    const types = {
> +		iso: [
> +		    'application/octet-stream',
> +		    'application/x-iso9660-image',
> +		    'application/x-ima',
> +		],
> +		vztmpl: [
> +		    'application/octet-stream',
> +		    'application/gzip',
> +		    'application/tar',
> +		    'application/tar+gzip',
> +		    'application/x-gzip',
> +		    'application/x-gtar',
> +		    'application/x-tgz',
> +		    'application/x-tar',
> +		],
> +	    };
> +
> +	    if (type === "" || (types[content] && types[content].includes(type))) {
> +		field.setValidation();
> +		field.setDisabled(true);
> +	    } else {
> +		field.setDisabled(false);
> +		field.setValidation("Invalid type");
> +	    }
> +	    field.validate();
> +	},
> +    },
> +
> +    items: [
> +	{
> +	    xtype: 'inputpanel',
> +	    waitMsgTarget: true,
> +	    border: false,
> +	    columnT: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'url',
> +		    allowBlank: false,
> +		    fieldLabel: gettext('URL'),
> +		    listeners: {
> +			change: {
> +			    buffer: 500,
> +			    fn: 'urlChange',
> +			},
> +		    },
> +		},
> +		{
> +		    xtype: 'textfield',
> +		    name: 'filename',
> +		    allowBlank: false,
> +		    fieldLabel: gettext('File name'),
> +		},
> +	    ],
> +	    column1: [
> +		{
> +		    xtype: 'pveContentTypeSelector',
> +		    fieldLabel: gettext('Content'),
> +		    name: 'content',
> +		    allowBlank: false,
> +		},
> +	    ],
> +	    column2: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'size',
> +		    disabled: true,
> +		    fieldLabel: gettext('File size'),
> +		    emptyText: gettext('unknown'),
> +		},
> +	    ],
> +	    advancedColumn1: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'checksum',
> +		    fieldLabel: gettext('Checksum'),
> +		    allowBlank: true,
> +		    disabled: true,
> +		    emptyText: gettext('none'),
> +		    id: 'retrieveChecksum',
> +		},
> +		{
> +		    xtype: 'pveHashAlgorithmSelector',
> +		    name: 'checksumalg',
> +		    fieldLabel: gettext('Hash algorithm'),
> +		    allowBlank: true,
> +		    hasNoneOption: true,
> +		    value: '__default__',
> +		    listeners: {
> +			change: 'hashChange',
> +		    },
> +		},
> +	    ],
> +	    advancedColumn2: [
> +		{
> +		    xtype: 'textfield',
> +		    fieldLabel: gettext('MIME type'),
> +		    name: 'mimetype',
> +		    disabled: true,
> +		    editable: false,
> +		    emptyText: gettext('unknown'),
> +		    listeners: {
> +			change: 'typeChange',
> +		    },
> +		},
> +		{
> +		    xtype: 'proxmoxcheckbox',
> +		    name: 'insecure',
> +		    fieldLabel: gettext('Trust invalid certificates'),
> +		    uncheckedValue: 0,
> +		    listeners: {
> +			change: 'urlChange',
> +		    },
> +		},
> +	    ],
> +	},
> +    ],
> +
> +    initComponent: function() {
> +        var me = this;
> +
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +	if (!me.storage) {
> +	    throw "no storage ID specified";
> +	}
> +
> +	me.url = `/nodes/${me.nodename}/storage/${me.storage}/retrieve`;
> +	me.method = 'POST';
> +
> +	let contentTypeSel = me.items[0].column1[0];
> +	contentTypeSel.cts = me.contents;
> +	contentTypeSel.value = me.contents[0] || '';
> +
> +        me.callParent();
> +    },
> +});
> +
>  Ext.define('PVE.storage.ContentView', {
>      extend: 'Ext.grid.GridPanel',
>  
> @@ -249,36 +456,60 @@ 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.push(
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Upload'),
> +		    disabled: !me.enableUploadButton,
> +		    handler: function() {
> +			Ext.create('PVE.storage.Upload', {
> +			    nodename: nodename,
> +			    storage: storage,
> +			    contents: [content],
> +			    autoShow: true,
> +			    listeners:{
> +				destroy: () => reload(),
> +			    }
> +			});
> +		    },
> +		},
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Retrieve from URL'),
> +		    disabled: !me.enableRetrieveButton,
> +		    handler: function() {
> +			Ext.create('PVE.storage.Retrieve', {
> +			    nodename: nodename,
> +			    storage: storage,
> +			    contents: [content],
> +			    autoShow: true,
> +			    listeners: {
> +				destroy: () => reload(),
> +			    },
> +			});
> +		    },
> +		},
> +		'-',
> +	    );
>  	}
> -	if (!me.useCustomRemoveButton) {
> -	    me.tbar.push(removeButton);
> +	if (me.useCustomRemoveButton) {
> +	    // custom remove button was inserted as first element
> +	    // -> place it at the end of tbar
> +	    me.tbar.push(me.tbar.shift());
> +	} else {
> +	    me.tbar.push({
> +		xtype: 'proxmoxStdRemoveButton',
> +		selModel: sm,
> +		delay: 5,
> +		callback: function() {
> +		    reload();
> +		},
> +		baseurl: baseurl + '/',
> +	    });
>  	}
>  	me.tbar.push(
>  	    '->',
>