From 4af5f6d8e0c6ea739097da4e2b374aefb6215cce Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Mon, 16 Nov 2020 15:09:28 -0800
Subject: [PATCH 1/2] Do handling of placeholders after _URL variables parsed

b3bc8ebf caused some problems by moving the parsing of `_URL`
settings to after the call to `JobSettings::generate_settings`,
which is where handling of placeholders (%FOO%), usually from the
templates, is done. This broke the case where e.g. a template
uses `%ISO%` in a value, and the job is posted with `ISO_URL` but
no explicit `ISO` set. In that case the derived `ISO` setting
should be used in the placeholder expansion, but this was broken.

This fixes the problems by splitting up the "parse _URL settings"
and "create download list" functions of `create_downloads_list`,
and moving the parsing part into `generate_settings`, just before
placeholder expansion happens. This is after we pull in settings
from the templates, so the bug b3bc8ebf was trying to fix (POO
62159) should still be fixed. Download list creation should still
happen the same as currently, so POO #70687 should still be fixed.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 lib/OpenQA/JobSettings.pm | 39 +++++++++++++++++++++++++++
 lib/OpenQA/Utils.pm       | 56 +++++++++++++++++----------------------
 t/api/02-iso-download.t   | 13 +++++++++
 3 files changed, 77 insertions(+), 31 deletions(-)

diff --git a/lib/OpenQA/JobSettings.pm b/lib/OpenQA/JobSettings.pm
index 0d6c11811..b0e364c8c 100644
--- a/lib/OpenQA/JobSettings.pm
+++ b/lib/OpenQA/JobSettings.pm
@@ -17,6 +17,12 @@ package OpenQA::JobSettings;
 use strict;
 use warnings;
 
+use File::Basename;
+use Mojo::URL;
+use Mojo::Util 'url_unescape';
+use OpenQA::Log 'log_debug';
+use OpenQA::Utils 'get_url_short';
+
 sub generate_settings {
     my ($params) = @_;
     my $settings = $params->{settings};
@@ -53,6 +59,7 @@ sub generate_settings {
         $settings->{JOB_DESCRIPTION} = $job_template->description if length $job_template->description;
     }
 
+    parse_url_settings($settings);
     handle_plus_in_settings($settings);
     return expand_placeholders($settings);
 }
@@ -104,4 +111,36 @@ sub handle_plus_in_settings {
     }
 }
 
+# Given a hashref of settings, parse any whose names end in _URL
+# to the short name, then if there is not already a setting with
+# the short name, set it to the filename from the URL (with the
+# compression extension removed in the case of _DECOMPRESS_URL).
+# This has to happen *before* generate_jobs
+sub parse_url_settings {
+    my ($settings) = @_;
+    for my $setting (keys %$settings) {
+        my ($short, $do_extract) = get_url_short($setting);
+        next unless ($short);
+        next if ($settings->{$short});
+        # As this comes in from an API call, URL will be URI-encoded
+        # This obviously creates a vuln if untrusted users can POST
+        $settings->{$setting} = url_unescape($settings->{$setting});
+        my $url = $settings->{$setting};
+        my $filename;
+        $filename = Mojo::URL->new($url)->path->parts->[-1];
+        if ($do_extract) {
+            # if user wants to extract downloaded file, final filename
+            # will have last extension removed
+            $filename = fileparse($filename, qr/\.[^.]*/);
+        }
+        $settings->{$short} = $filename;
+        if (!$settings->{$short}) {
+            log_debug("Unable to get filename from $url. Ignoring $setting");
+            delete $settings->{$short} unless $settings->{$short};
+            next;
+        }
+    }
+    return undef;
+}
+
 1;
diff --git a/lib/OpenQA/Utils.pm b/lib/OpenQA/Utils.pm
index 4db65eaeb..40dc928b0 100644
--- a/lib/OpenQA/Utils.pm
+++ b/lib/OpenQA/Utils.pm
@@ -21,7 +21,6 @@ use Carp;
 use Cwd 'abs_path';
 use IPC::Run();
 use Mojo::URL;
-use Mojo::Util 'url_unescape';
 use Regexp::Common 'URI';
 use Try::Tiny;
 use Mojo::File 'path';
@@ -62,6 +61,7 @@ our @EXPORT  = qw(
   asset_type_from_setting
   check_download_url
   check_download_passlist
+  get_url_short
   create_downloads_list
   human_readable_size
   locate_asset
@@ -499,42 +499,36 @@ sub check_download_passlist {
     return ();
 }
 
+sub get_url_short {
+    # Given a setting name, if it ends with _URL or _DECOMPRESS_URL
+    # return the name with that string stripped, and a flag indicating
+    # whether decompression will be needed. If it doesn't, returns
+    # empty string and 0.
+    my ($arg) = @_;
+    return ('', 0) unless ($arg =~ /_URL$/);
+    my $short;
+    my $do_extract = 0;
+    if ($arg =~ /_DECOMPRESS_URL$/) {
+        $short      = substr($arg, 0, -15);
+        $do_extract = 1;
+    }
+    else {
+        $short = substr($arg, 0, -4);
+    }
+    return ($short, $do_extract);
+}
+
 sub create_downloads_list {
     my ($args) = @_;
     my %downloads = ();
     for my $arg (keys %$args) {
-        next unless ($arg =~ /_URL$/);
-        # As this comes in from an API call, URL will be URI-encoded
-        # This obviously creates a vuln if untrusted users can POST
-        $args->{$arg} = url_unescape($args->{$arg});
-        my $url        = $args->{$arg};
-        my $do_extract = 0;
-        my $short;
+        my $url = $args->{$arg};
         my $filename;
-        # if $args{FOO_URL} or $args{FOO_DECOMPRESS_URL} is set but $args{FOO}
-        # is not, we will set $args{FOO} (the filename of the downloaded asset)
-        # to the URL filename. This has to happen *before*
-        # generate_jobs so the jobs have FOO set
-        if ($arg =~ /_DECOMPRESS_URL$/) {
-            $do_extract = 1;
-            $short      = substr($arg, 0, -15);    # remove whole _DECOMPRESS_URL substring
-        }
-        else {
-            $short = substr($arg, 0, -4);          # remove _URL substring
-        }
+        my ($short, $do_extract) = get_url_short($arg);
+        next unless ($short);
         if (!$args->{$short}) {
-            $filename = Mojo::URL->new($url)->path->parts->[-1];
-            if ($do_extract) {
-                # if user wants to extract downloaded file, final filename
-                # will have last extension removed
-                $filename = fileparse($filename, qr/\.[^.]*/);
-            }
-            $args->{$short} = $filename;
-            if (!$args->{$short}) {
-                log_debug("Unable to get filename from $url. Ignoring $arg");
-                delete $args->{$short} unless $args->{$short};
-                next;
-            }
+            log_debug("No target filename set for $url. Ignoring $arg");
+            next;
         }
         else {
             $filename = $args->{$short};
diff --git a/t/api/02-iso-download.t b/t/api/02-iso-download.t
index fcb3744f0..53b67ebf5 100644
--- a/t/api/02-iso-download.t
+++ b/t/api/02-iso-download.t
@@ -298,4 +298,17 @@ subtest 'download task only blocks the related job when test suites have differe
     is scalar(@{$gru_dep_tasks->{$gru_task_ids[0]}}), 3, 'one download task was created and it blocked 3 jobs';
 };
 
+subtest 'placeholder expansions work with _URL-derived settings' => sub {
+    $test_suites->find({name => 'kde'})->settings->create({key => 'FOOBAR', value => '%ISO%'});
+    my $new_params = {%params, ISO_URL => 'http://localhost/openSUSE-13.1-DVD-i586-Build0091-Media.iso', TEST => 'kde'};
+    $rsp = schedule_iso($new_params, 200);
+    is $rsp->json->{count}, 1, 'one job was scheduled';
+    my $expanderjob = get_job($rsp->json->{ids}->[0]);
+    is(
+        $expanderjob->{settings}->{FOOBAR},
+        'openSUSE-13.1-DVD-i586-Build0091-Media.iso',
+        '%ISO% in template is expanded by posted ISO_URL'
+    );
+};
+
 done_testing();
-- 
2.29.2