[pve-devel] [PATCH-SERIES v5 manager/common/storage] fix #1710: add download from url button

[pve-devel] [PATCH-SERIES v5 manager/common/storage] fix #1710: add download from url button

[Lorenz Stechauner]

changes to v4:
reworked check_file_hash
small fixes: typos, default values, etc.

patches:
1 - manager: add query_url_metadata method
2 - common: add download_file_from_url
3 - manager: refactor aplinfo to use common download function
4 - storage: add download_url method
5 - manager: add HashAlgorithmSelector
6 - manager: change download task format
7 - manager: fix #1710: add download from url button


pve-manager:
Lorenz Stechauner (5):
  api: nodes: add query_url_metadata method
  api: nodes: refactor aplinfo to use common download function
  ui: add HashAlgorithmSelector
  ui: Utils: change download task format
  fix #1710: ui: storage: add download from url button

 PVE/API2/Nodes.pm                          | 159 +++++++-----
 www/manager6/Makefile                      |   1 +
 www/manager6/Utils.js                      |   2 +-
 www/manager6/form/HashAlgorithmSelector.js |  16 ++
 www/manager6/storage/Browser.js            |   8 +
 www/manager6/storage/ContentView.js        | 282 +++++++++++++++++++--
 6 files changed, 376 insertions(+), 92 deletions(-)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js


pve-common:
Lorenz Stechauner (1):
  tools: add download_file_from_url

 src/PVE/Tools.pm | 124 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)


pve-storage
Lorenz Stechauner (1):
  status: add download_url method

 PVE/API2/Storage/Status.pm | 118 +++++++++++++++++++++++++++++++++++--
 PVE/Storage.pm             |  10 ++++
 2 files changed, 123 insertions(+), 5 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v5 manager 1/7] api: nodes: add query_url_metadata method

[Lorenz Stechauner]

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 e58d9c10..a289ef8b 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;
@@ -238,6 +239,7 @@ __PACKAGE__->register_method ({
 	    { name => 'netstat' },
 	    { name => 'network' },
 	    { name => 'qemu' },
+	    { name => 'query-url-metadata' },
 	    { name => 'replication' },
 	    { name => 'report' },
 	    { name => 'rrd' }, # fixme: remove?
@@ -1595,6 +1597,100 @@ __PACKAGE__->register_method({
 	return $rpcenv->fork_worker('download', undef, $user, $worker);
     }});
 
+__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=(.+)/) {
+	    $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.20.1

[pve-devel] [PATCH v5 common 2/7] tools: add download_file_from_url

[Lorenz Stechauner]

code is based on
manager:PVE/API2/Nodes.pm:aplinfo

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

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 16ae3d2..7b82e00 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1829,4 +1829,128 @@ sub safe_compare {
     return $cmp->($left, $right);
 }
 
+
+# opts
+#  -> hash_required
+#       if 1, at least one checksum has to be specified otherwise an error will be thrown
+#  -> http_proxy
+#  -> https_proxy
+#  -> verify_certificates
+#  -> sha(1|224|256|384|512)sum
+#  -> md5sum
+sub download_file_from_url {
+    my ($dest, $url, $opts) = @_;
+
+    my $tmpdest = "$dest.tmp.$$";
+
+    my $algorithm;
+    my $expected;
+
+    for ('sha512', 'sha384', 'sha256', 'sha224', 'sha1', 'md5') {
+	if (defined($opts->{"${_}sum"})) {
+	    $algorithm = $_;
+	    $expected = $opts->{"${_}sum"};
+	    last;
+	}
+    }
+
+    die "checksum required but not specified\n" if ($opts->{hash_required} && !$algorithm);
+
+    my $worker = sub  {
+	my $upid = shift;
+
+	print "downloading $url to $dest\n";
+
+	eval {
+	    if (-f $dest && $algorithm) {
+		print "calculating checksum of existing file...\n";
+		my $correct = check_file_hash($algorithm, $expected, $dest);
+
+		if ($correct) {
+		    print "file already exists, no need to download\n";
+		    return;
+		} else {
+		    print "mismatch, downloading\n";
+		}
+	    }
+
+	    my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $url);
+
+	    local %ENV;
+	    if ($opts->{http_proxy}) {
+		$ENV{http_proxy} = $opts->{http_proxy};
+	    }
+	    if ($opts->{https_proxy}) {
+		$ENV{https_proxy} = $opts->{https_proxy};
+	    }
+
+	    my $verify = $opts->{verify_certificates} // 1;
+	    if (!$verify) {
+		push @cmd, '--no-check-certificate';
+	    }
+
+	    if (run_command([[@cmd]]) != 0) {
+		die "download failed: $!\n";
+	    }
+
+	    if ($algorithm) {
+		print "calculating checksum...\n";
+
+		my $correct = check_file_hash($algorithm, $expected, $tmpdest);
+
+		if ($correct) {
+		    print "checksum verified\n";
+		} else {
+		    die "checksum mismatch\n";
+		}
+	    } else {
+		print "no checksum for verification specified\n";
+	    }
+
+	    if (!rename($tmpdest, $dest)) {
+		die "unable to save file: $!\n";
+	    }
+	};
+	my $err = $@;
+
+	unlink $tmpdest;
+
+	if ($err) {
+	    print "\n";
+	    die $err;
+	}
+
+	print "download finished\n";
+    };
+
+    my $rpcenv = PVE::RPCEnvironment::get();
+    my $user = $rpcenv->get_user();
+
+    (my $filename = $dest) =~ s!.*/([^/]*)$!$1!;
+
+    return $rpcenv->fork_worker('download', $filename, $user, $worker);
+}
+
+sub check_file_hash {
+    my ($algorithm, $expected, $filename) = @_;
+
+    my $algorithm_map = {
+	'md5' => sub { Digest::MD5->new },
+	'sha1' => sub { Digest::SHA->new(1) },
+	'sha224' => sub { Digest::SHA->new(224) },
+	'sha256' => sub { Digest::SHA->new(256) },
+	'sha384' => sub { Digest::SHA->new(384) },
+	'sha512' => sub { Digest::SHA->new(512) },
+    };
+
+    my $digester = $algorithm_map->{$algorithm}->() or die "unknown algorithm '$algorithm'\n";
+
+    open(my $fh, '<', $filename) or die "unable to open '$filename': $!\n";
+    binmode($fh);
+
+    my $digest = $digester->addfile($fh)->hexdigest;
+
+    return lc($digest) eq lc($expected);
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v5 manager 3/7] api: nodes: refactor aplinfo to use common download function

[Lorenz Stechauner]

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

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index a289ef8b..16b10925 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1513,88 +1513,19 @@ __PACKAGE__->register_method({
 	my $src = $pd->{location};
 	my $tmpldir = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
 	my $dest = "$tmpldir/$template";
-	my $tmpdest = "$tmpldir/${template}.tmp.$$";
 
-	my $worker = sub  {
-	    my $upid = shift;
-
-	    print "starting template download from: $src\n";
-	    print "target file: $dest\n";
-
-	    my $check_hash = sub {
-		my ($template_info, $filename, $noerr) = @_;
-
-		my $digest;
-		my $expected;
-
-		eval {
-		    open(my $fh, '<', $filename) or die "Can't open '$filename': $!";
-		    binmode($fh);
-		    if (defined($template_info->{sha512sum})) {
-			$expected = $template_info->{sha512sum};
-			$digest = Digest::SHA->new(512)->addfile($fh)->hexdigest;
-		    } elsif (defined($template_info->{md5sum})) {
-			#fallback to MD5
-			$expected = $template_info->{md5sum};
-			$digest = Digest::MD5->new->addfile($fh)->hexdigest;
-		    } else {
-			die "no expected checksum defined";
-		    }
-		    close($fh);
-		};
-
-		die "checking hash failed - $@\n" if $@ && !$noerr;
-
-		return ($digest, $digest ? lc($digest) eq lc($expected) : 0);
-	    };
-
-	    eval {
-		if (-f $dest) {
-		    my ($hash, $correct) = &$check_hash($pd, $dest, 1);
-
-		    if ($hash && $correct) {
-			print "file already exists $hash - no need to download\n";
-			return;
-		    }
-		}
-
-		local %ENV;
-		my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
-		if ($dccfg->{http_proxy}) {
-		    $ENV{http_proxy} = $dccfg->{http_proxy};
-		}
-
-		my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $src);
-		if (system (@cmd) != 0) {
-		    die "download failed - $!\n";
-		}
-
-		my ($hash, $correct) = &$check_hash($pd, $tmpdest);
-
-		die "could not calculate checksum\n" if !$hash;
-
-		if (!$correct) {
-		    my $expected = $pd->{sha512sum} // $pd->{md5sum};
-		    die "wrong checksum: $hash != $expected\n";
-		}
-
-		if (!rename($tmpdest, $dest)) {
-		    die "unable to save file - $!\n";
-		}
-	    };
-	    my $err = $@;
-
-	    unlink $tmpdest;
-
-	    if ($err) {
-		print "\n";
-		die $err if $err;
-	    }
-
-	    print "download finished\n";
+	my $opts = {
+	    hash_required => 1,
+	    sha512sum => $pd->{sha512sum},
+	    md5sum => $pd->{md5sum},
 	};
 
-	return $rpcenv->fork_worker('download', undef, $user, $worker);
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	if ($dccfg->{http_proxy}) {
+	    $opts->{http_proxy} = $dccfg->{http_proxy};
+	}
+
+	return PVE::Tools::download_file_from_url($dest, $src, $opts);
     }});
 
 __PACKAGE__->register_method({
-- 
2.20.1

[pve-devel] [PATCH v5 storage 4/7] status: add download_url method

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Storage/Status.pm | 118 +++++++++++++++++++++++++++++++++++--
 PVE/Storage.pm             |  10 ++++
 2 files changed, 123 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 897b4a7..4c1d1fc 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -412,11 +412,7 @@ __PACKAGE__->register_method ({
 	my $size = -s $tmpfilename;
 	die "temporary file '$tmpfilename' does not exist\n" if !defined($size);
 
-	my $filename = $param->{filename};
-
-	chomp $filename;
-	$filename =~ s/^.*[\/\\]//;
-	$filename =~ s/[^-a-zA-Z0-9_.]/_/g;
+	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
 
 	my $path;
 
@@ -497,4 +493,116 @@ __PACKAGE__->register_method ({
 	return $upid;
    }});
 
+__PACKAGE__->register_method({
+    name => 'download_url',
+    path => '{storage}/download-url',
+    method => 'POST',
+    description => "Download templates and ISO images by using an URL.",
+    proxyto => 'node',
+    permissions => {
+	check => [ 'and',
+	    ['perm', '/storage/{storage}', [ 'Datastore.AllocateTemplate' ]],
+	    ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+	],
+    },
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    storage => get_standard_option('pve-storage-id'),
+	    url => {
+		description => "The URL to download the file from.",
+		type => 'string',
+		pattern => 'https?://.*',
+	    },
+	    content => {
+		description => "Content type.",
+		type => 'string', format => 'pve-storage-content',
+	    },
+	    filename => {
+		description => "The name of the file to create.",
+		type => 'string',
+	    },
+	    checksum => {
+		description => "The expected checksum of the file.",
+		type => 'string',
+		requires => 'checksum-algorithm',
+		optional => 1,
+	    },
+	    'checksum-algorithm' => {
+		description => "The algorithm to calculate the checksum of the file.",
+		type => 'string',
+		enum => ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'],
+		requires => 'checksum',
+		optional => 1,
+	    },
+	    'verify-certificates' => {
+		description => "If false, no SSL/TLS certificates will be verified.",
+		type => 'boolean',
+		optional => 1,
+		default => 1,
+	    }
+	},
+    },
+    returns => {
+	type => "string"
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $cfg = PVE::Storage::config();
+
+	my ($node, $storage) = $param->@{'node', 'storage'};
+	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
+
+	die "can't upload to storage type '$scfg->{type}, not a file based storage!'\n" if !defined($scfg->{path});
+
+	my ($content, $url) = $param->@{'content', 'url'};
+
+	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
+	my $path;
+
+	# MIME type is checked in front end only
+	# this check is omitted here intentionally and replaced by file extension check
+	if ($content eq 'iso') {
+	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
+		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
+	    }
+	    $path = PVE::Storage::get_iso_dir($cfg, $storage);
+	} elsif ($content eq 'vztmpl') {
+	    if ($filename !~ m![^/]+\.tar\.[gx]z$!) {
+		raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
+	    }
+	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
+	} else {
+	    raise_param_exc({ content => "upload content type '$content' not allowed" });
+	}
+
+	die "storage '$storage' does not support '$content' content\n" if !$scfg->{content}->{$content};
+
+	PVE::Storage::activate_storage($cfg, $storage);
+	File::Path::make_path($path);
+
+	my $dest = "$path/$filename";
+
+	my $opts = {
+	    hash_required => 0,
+	    verify_certificates => $param->{'verify-certificates'} // 1,
+	};
+
+	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm'};
+	if ($checksum) {
+	    $opts->{"${checksum_algorithm}sum"} = $checksum;
+	    $opts->{hash_required} = 1;
+	}
+
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	if ($dccfg->{http_proxy}) {
+	    $opts->{http_proxy} = $dccfg->{http_proxy};
+	}
+
+	return PVE::Tools::download_file_from_url($dest, $url, $opts);
+    }});
+
 1;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 122c3e9..d57fd43 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1931,4 +1931,14 @@ sub assert_sid_unused {
     return undef;
 }
 
+sub normalize_content_filename {
+    my ($filename) = @_;
+
+    chomp $filename;
+    $filename =~ s/^.*[\/\\]//;
+    $filename =~ s/[^-a-zA-Z0-9_.]/_/g;
+
+    return $filename;
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v5 manager 5/7] ui: add HashAlgorithmSelector

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/Makefile                      |  1 +
 www/manager6/form/HashAlgorithmSelector.js | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js

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/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'],
+    ],
+});
-- 
2.20.1

[pve-devel] [PATCH v5 manager 6/7] 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 581d2040..38306c11 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1819,7 +1819,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.20.1

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

[Lorenz Stechauner]

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

diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index 5fee94c7..c0d647d3 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 = !!(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: enableUpload && 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: enableUpload && enableDownloadUrl,
+		    useUploadButton: true,
 		});
 	    }
 	    if (contents.includes('snippets')) {
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index dd6df4b1..9125c0bb 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -191,6 +191,214 @@ Ext.define('PVE.storage.Upload', {
     },
 });
 
+Ext.define('PVE.storage.DownloadUrl', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveStorageDownloadUrl',
+
+    isCreate: true,
+
+    showTaskViewer: true,
+
+    title: gettext('Download from URL'),
+    submitText: gettext('Download'),
+
+    id: 'download-url',
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	urlChange: function(field) {
+	    let me = Ext.getCmp('download-url');
+	    field = me.down('[name=url]');
+	    field.setValidation("Waiting for response...");
+	    field.validate();
+	    me.setValues({size: ""});
+	    Proxmox.Utils.API2Request({
+		url: `/nodes/${me.nodename}/query-url-metadata`,
+		method: 'GET',
+		params: {
+		    url: field.getValue(),
+		    'verify-certificates': me.getValues()['verify-certificates'],
+		},
+		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('downloadUrlChecksum');
+	    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('download-url');
+	    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: 'downloadUrlChecksum',
+		},
+		{
+		    xtype: 'pveHashAlgorithmSelector',
+		    name: 'checksum-algorithm',
+		    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: 'verify-certificates',
+		    fieldLabel: gettext('Verify certificates'),
+		    uncheckedValue: 0,
+		    checked: true,
+		    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}/download-url`;
+	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 +457,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('Download from URL'),
+		    disabled: !me.enableDownloadUrlButton,
+		    handler: function() {
+			Ext.create('PVE.storage.DownloadUrl', {
+			    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(
 	    '->',
-- 
2.20.1

Re: [pve-devel] [PATCH-SERIES v5 manager/common/storage] fix #1710: add download from url button

[Dominik Csapak]

api side LGTM and it works like advertised, some coding issues on
the last patch, i'll answer there separately

On 5/12/21 10:22, Lorenz Stechauner wrote:
> changes to v4:
> reworked check_file_hash
> small fixes: typos, default values, etc.
> 
> patches:
> 1 - manager: add query_url_metadata method
> 2 - common: add download_file_from_url
> 3 - manager: refactor aplinfo to use common download function
> 4 - storage: add download_url method
> 5 - manager: add HashAlgorithmSelector
> 6 - manager: change download task format
> 7 - manager: fix #1710: add download from url button
> 
> 
> pve-manager:
> Lorenz Stechauner (5):
>    api: nodes: add query_url_metadata method
>    api: nodes: refactor aplinfo to use common download function
>    ui: add HashAlgorithmSelector
>    ui: Utils: change download task format
>    fix #1710: ui: storage: add download from url button
> 
>   PVE/API2/Nodes.pm                          | 159 +++++++-----
>   www/manager6/Makefile                      |   1 +
>   www/manager6/Utils.js                      |   2 +-
>   www/manager6/form/HashAlgorithmSelector.js |  16 ++
>   www/manager6/storage/Browser.js            |   8 +
>   www/manager6/storage/ContentView.js        | 282 +++++++++++++++++++--
>   6 files changed, 376 insertions(+), 92 deletions(-)
>   create mode 100644 www/manager6/form/HashAlgorithmSelector.js
> 
> 
> pve-common:
> Lorenz Stechauner (1):
>    tools: add download_file_from_url
> 
>   src/PVE/Tools.pm | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 124 insertions(+)
> 
> 
> pve-storage
> Lorenz Stechauner (1):
>    status: add download_url method
> 
>   PVE/API2/Storage/Status.pm | 118 +++++++++++++++++++++++++++++++++++--
>   PVE/Storage.pm             |  10 ++++
>   2 files changed, 123 insertions(+), 5 deletions(-)
> 

Re: [pve-devel] [PATCH v5 manager 7/7] fix #1710: ui: storage: add download from url button

[Dominik Csapak]

comments inline,
some things would have been caught with 'eslint',
so i would recommend running it on the files you edit/add,
sans the errors from existing code

(most errors can even be auto-fixed with '--fix')


On 5/12/21 10:22, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   www/manager6/storage/Browser.js     |   8 +
>   www/manager6/storage/ContentView.js | 282 +++++++++++++++++++++++++---
>   2 files changed, 265 insertions(+), 25 deletions(-)
> 
> diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
> index 5fee94c7..c0d647d3 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 = !!(caps.nodes['Sys.Audit'] && caps.nodes['Sys.Modify']);

you use this always with 'enableUpload' anyway so you could add
it here already

let enableDownloadUrl = enableUpload && ...;

> +
>   	    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: enableUpload && 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: enableUpload && enableDownloadUrl,
> +		    useUploadButton: true,
>   		});
>   	    }
>   	    if (contents.includes('snippets')) {
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index dd6df4b1..9125c0bb 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -191,6 +191,214 @@ Ext.define('PVE.storage.Upload', {
>       },
>   });
>   
> +Ext.define('PVE.storage.DownloadUrl', {
> +    extend: 'Proxmox.window.Edit',
> +    alias: 'widget.pveStorageDownloadUrl',
> +
> +    isCreate: true,
> +
> +    showTaskViewer: true,
> +
> +    title: gettext('Download from URL'),
> +    submitText: gettext('Download'),
> +
> +    id: 'download-url',
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	urlChange: function(field) {
> +	    let me = Ext.getCmp('download-url');
> +	    field = me.down('[name=url]');

there is a better way to get the 'view' and fields when you have
a controller

instead of giving the view an id and do a getCmp, you can
simply do a

let me = this; // this is the controller here
let view = me.getView(); // this is the view (== window)

simply use view then instead of me

also if you add a 'reference' on the field,
you can simply do a

let field = me.lookup('fieldreference'); // works on both controller and 
view

> +	    field.setValidation("Waiting for response...");
> +	    field.validate();
> +	    me.setValues({size: ""});
> +	    Proxmox.Utils.API2Request({
> +		url: `/nodes/${me.nodename}/query-url-metadata`,
> +		method: 'GET',
> +		params: {
> +		    url: field.getValue(),
> +		    'verify-certificates': me.getValues()['verify-certificates'],
> +		},
> +		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) || "",

this throws an eslint warning:

no-mixed-operators - Unexpected mix of '&&' and '||'.

better use braces

> +			mimetype: data.mimetype || "",
> +		    }); > +		},
> +	    });
> +	},
> +
> +	hashChange: function(field) {
> +	    let cecksum = Ext.getCmp('downloadUrlChecksum');

nitpick: typo s/cecksum/checksum/

> +	    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('download-url');

same here:

let me = this;
let view = me.getView();

> +	    let content = me.getValues()['content'];

this throws eslint error:

dot-notation - ["content"] is better written in dot notation.

> +	    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: 'downloadUrlChecksum',
> +		},
> +		{
> +		    xtype: 'pveHashAlgorithmSelector',
> +		    name: 'checksum-algorithm',
> +		    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: 'verify-certificates',
> +		    fieldLabel: gettext('Verify certificates'),
> +		    uncheckedValue: 0,
> +		    checked: true,
> +		    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}/download-url`;
> +	me.method = 'POST';

this could go statically into the class on top:

method: 'POST',



> +
> +	let contentTypeSel = me.items[0].column1[0];
> +	contentTypeSel.cts = me.contents;
> +	contentTypeSel.value = me.contents[0] || '';

this and the url could go into our 'cbind' mixin
(check for example panel/GuestStatusView.js)

> +
> +        me.callParent();
> +    },
> +});
> +
>   Ext.define('PVE.storage.ContentView', {
>       extend: 'Ext.grid.GridPanel',
>   
> @@ -249,36 +457,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(),
> +			    }

the reload makes more sense on the 'taskDone' callback
since that really only triggers when the download is finished,
instead of when the window is closed (which may not have
changed the content at all)

> +			});
> +		    },
> +		},
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Download from URL'),
> +		    disabled: !me.enableDownloadUrlButton,
> +		    handler: function() {
> +			Ext.create('PVE.storage.DownloadUrl', {
> +			    nodename: nodename,
> +			    storage: storage,
> +			    contents: [content],
> +			    autoShow: true,
> +			    listeners: {
> +				destroy: () => reload(),
> +			    },

same here

> +			});
> +		    },
> +		},
> +		'-',
> +	    );
>   	}
> -	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());

could we not simply 'unshift' our upload/download button instead?
that way, this code would not have to change and we would not
move other stuff around...

> +	} else {
> +	    me.tbar.push({
> +		xtype: 'proxmoxStdRemoveButton',
> +		selModel: sm,
> +		delay: 5,
> +		callback: function() {
> +		    reload();
> +		},
> +		baseurl: baseurl + '/',
> +	    });
>   	}
>   	me.tbar.push(
>   	    '->',
>