From 0b2bb60efb0292764efe3c00ae19743b5165c5d8 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Tue, 17 Nov 2020 10:16:51 -0800
Subject: [PATCH 2/4] Don't set FOO from FOO_URL if FOO is defined but false

If someone has explicitly set e.g. ISO to "" for some scenario,
that's a clear signal they want the test in that scenario to run
without an ISO attached, even if it's posted with ISO_URL set.
However, when we decide whether to generate FOO from FOO_URL,
the check we use is not whether FOO is already *defined* in the
settings hash, but whether it *evaluates true*. That seems wrong.

We ran into this is in real life after b3bc8eb changed the order
in which we do _URL parsing and +-overrides. There is a Fedora
test suite with `+ISO` set to `''`, the intent being that the
'ISO' value derived from the posted `ISO_URL` should be
overridden to `''` when running that test (so it runs with no
disc attached, and boots from the network). This worked so long
as we handled the +-overrides after we parsed the _URL settings,
but b3bc8eb changed things so we do the +-overrides before we
parse the _URL settings, and caused that case to trigger this.

The previous commit to this one changes the ordering back to fix
a similar issue with placeholder expansion, and so also happens
to hide this bug again in that specific real life case, but this
seems like the 'correct' fix. Consider for instance if the test
suite just set `ISO` (not `+ISO`) to `''`; it seems like even
then we ought not to override that intent by parsing `ISO_URL`,
it should not actually be necessary for the test suite to use
the +-override mechanism.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 lib/OpenQA/JobSettings.pm | 2 +-
 t/api/02-iso-download.t   | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/OpenQA/JobSettings.pm b/lib/OpenQA/JobSettings.pm
index b0e364c8c..0d438c0e4 100644
--- a/lib/OpenQA/JobSettings.pm
+++ b/lib/OpenQA/JobSettings.pm
@@ -121,7 +121,7 @@ sub parse_url_settings {
     for my $setting (keys %$settings) {
         my ($short, $do_extract) = get_url_short($setting);
         next unless ($short);
-        next if ($settings->{$short});
+        next if defined($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});
diff --git a/t/api/02-iso-download.t b/t/api/02-iso-download.t
index 53b67ebf5..914d30552 100644
--- a/t/api/02-iso-download.t
+++ b/t/api/02-iso-download.t
@@ -311,4 +311,13 @@ subtest 'placeholder expansions work with _URL-derived settings' => sub {
     );
 };
 
+subtest 'test suite sets short asset setting to false value' => sub {
+    $test_suites->find({name => 'kde'})->settings->create({key => 'ISO', value => ''});
+    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 $overriddenjob = get_job($rsp->json->{ids}->[0]);
+    is($overriddenjob->{settings}->{ISO}, '', 'false-evaluating ISO in template overrides posted ISO_URL');
+};
+
 done_testing();
-- 
2.29.2