From 2c43580f340bfe24fc8de50488a5e3e05c2da563 Mon Sep 17 00:00:00 2001
From: Marius Kittler <mkittler@suse.de>
Date: Wed, 29 Jan 2025 17:14:40 +0100
Subject: [PATCH] =?UTF-8?q?Avoid=20restricting=20`/tests/=E2=80=A6/asset/?=
 =?UTF-8?q?=E2=80=A6`=20needlessly?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Allow unauthorized access to `/tests/…/asset/…` even when
  `[auth] require_for_assets = 1` is configured because this route never
  returns any assets itself; it just redirects to `/assets/…` which will
  still require authorization
* Allow making exceptions for authorized asset downloads on reverse proxy
  level without having to take `/tests/…/asset/…` into account as well
* See https://progress.opensuse.org/issues/175902
---
 lib/OpenQA/WebAPI.pm | 7 +++----
 t/03-auth.t          | 7 ++++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/OpenQA/WebAPI.pm b/lib/OpenQA/WebAPI.pm
index 5a93a54d16c..d20772fce87 100644
--- a/lib/OpenQA/WebAPI.pm
+++ b/lib/OpenQA/WebAPI.pm
@@ -151,7 +151,6 @@ sub startup ($self) {
     my $test_r = $r->any($test_path);
     $test_r = $test_r->under('/')->to('test#referer_check');
     my $test_auth = $auth->any($test_path => {format => 0});
-    my $test_asset_r = $require_auth_for_assets ? $auth_any_user->any($test_path) : $test_r;
     $test_r->get('/')->name('test')->to('test#show');
     $test_r->get('/ajax')->name('job_next_previous_ajax')->to('test#job_next_previous_ajax');
     $test_r->get('/modules/:moduleid/fails')->name('test_module_fails')->to('test#module_fails');
@@ -177,9 +176,9 @@ sub startup ($self) {
     $test_r->get('/video' => sub ($c) { $c->render_testfile('test/video') })->name('video');
     $test_r->get('/logfile' => sub ($c) { $c->render_testfile('test/logfile') })->name('logfile');
     # adding assetid => qr/\d+/ doesn't work here. wtf?
-    $test_asset_r->get('/asset/<assetid:num>')->name('test_asset_id')->to('file#test_asset');
-    $test_asset_r->get('/asset/#assettype/#assetname')->name('test_asset_name')->to('file#test_asset');
-    $test_asset_r->get('/asset/#assettype/#assetname/*subpath')->name('test_asset_name_path')->to('file#test_asset');
+    $test_r->get('/asset/<assetid:num>')->name('test_asset_id')->to('file#test_asset');
+    $test_r->get('/asset/#assettype/#assetname')->name('test_asset_name')->to('file#test_asset');
+    $test_r->get('/asset/#assettype/#assetname/*subpath')->name('test_asset_name_path')->to('file#test_asset');
 
     my $developer_auth = $test_r->under('/developer')->to('session#ensure_admin');
     $developer_auth->get('/ws-console')->name('developer_ws_console')->to('developer#ws_console');
diff --git a/t/03-auth.t b/t/03-auth.t
index e1c68c8ae29..d8a724c6440 100644
--- a/t/03-auth.t
+++ b/t/03-auth.t
@@ -21,7 +21,7 @@ use Mojolicious;
 
 my $file_api_mock = Test::MockModule->new('OpenQA::WebAPI::Controller::File');
 $file_api_mock->redefine(download_asset => sub ($self) { $self->render(text => 'asset-ok') });
-$file_api_mock->redefine(test_asset => sub ($self) { $self->render(text => 'test-asset-ok') });
+$file_api_mock->redefine(test_asset => sub ($self) { $self->redirect_to('/assets/iso/test.iso') });
 
 my $tempdir = tempdir("/tmp/$FindBin::Script-XXXX")->make_path;
 $ENV{OPENQA_CONFIG} = $tempdir;
@@ -42,11 +42,12 @@ combined_like { test_auth_method_startup('Fake')->status_is(302) } mojo_has_requ
 
 subtest 'restricted asset downloads with setting `[auth] require_for_assets = 1`' => sub {
     my $t = test_auth_method_startup('Fake', "require_for_assets = 1\n");
+    $t->ua->max_redirects(1);    # follow redirection from `/tests/…/asset/…` to `/assets/…`
     my $expected_redirect = '/login?return_page=%2Fassets%2Fiso%2Ftest.iso';
     $t->get_ok('/assets/iso/test.iso')->status_is(200)->content_is('asset-ok', 'can access asset when logged in');
     $t->get_ok('/tests/42/asset/iso/test.iso')->status_is(200);
-    $t->content_is('test-asset-ok', 'can access test asset when logged in');
-    $t->get_ok('/logout')->status_is(302);
+    $t->content_is('asset-ok', 'can access test asset when logged in');
+    $t->get_ok('/logout')->status_is(200, 'logged out');
     $t->get_ok('/assets/iso/test.iso')->status_is(403, '403 response when logged out');
     $t->content_unlike(qr/asset-ok/, 'asset not accessible when logged out');
     $t->get_ok('/tests/42/asset/iso/test.iso')->status_is(403, '403 response via test when logged out');