Skip to content

Commit

Permalink
Allow all authentication methods for asset downloads
Browse files Browse the repository at this point in the history
Commit 4c8ed39 introduced an option to
to restrict asset downloads to logged-in users. This was literally only
allowing asset downloads to users that were logged-in using the web UI.

To move this forward and implement
https://progress.opensuse.org/issues/174301 it is required to allow other
authentication our other API authentication methods as well. This change
therefore enables the use of our normal API authentication methods for
read-only asset routes by simply making use of the auth controller we
normally use for API requests. If authentication for asset is not
configured this has of course no effect.

This allows queries like to return the expected 200/302 response (or 403 in
case the specified credentials are wrong).
```
curl -i -u Demo:…:… 'http://localhost:9526/assets/other/openSUSE-Tumbleweed-DVD-x86_64-Snapshot20230109-Media.iso.sha256'
curl -i -u Demo:…:… 'http://localhost:9526/tests/4416/asset/other/openSUSE-Tumbleweed-DVD-x86_64-Snapshot20250112-Media.iso.sha256'
```

Before, these queries would instead always redirect to the login route of
the web UI.

The change to the web API routes alone would break asset downloads via
links on the web UI as those routes would now always require the CSRF token
to be supplied. This commit therefore also changes the API auth controller
to allow GET requests without CSRF token. We already allow GET requests
without CSRF token on all web UI routes in the session controller so this
should be fine. (Additionally, browsers disallow cross-site scripting
out of the box and the GET API routes never require any authentication
anyway.)
  • Loading branch information
Martchus committed Jan 14, 2025
1 parent 05c983e commit 935d343
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 11 deletions.
2 changes: 1 addition & 1 deletion lib/OpenQA/Shared/Controller/Auth.pm
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ sub auth ($self) {
# Browser with a logged in user
my ($user, $reason) = (undef, 'Not authorized');
if ($user = $self->current_user) {
($user, $reason) = (undef, 'Bad CSRF token!') unless $self->valid_csrf;
($user, $reason) = (undef, 'Bad CSRF token!') unless $self->req->method eq 'GET' || $self->valid_csrf;
}

# No session (probably not a browser)
Expand Down
10 changes: 6 additions & 4 deletions lib/OpenQA/WebAPI.pm
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ sub startup ($self) {

# register basic routes
my $logged_in = $r->under('/')->to('session#ensure_user');
my $auth_any_user = $r->under('/')->to('Auth#auth');
my $auth = $r->under('/')->to('session#ensure_operator');

# Routes used by plugins (UI and API)
Expand Down Expand Up @@ -143,13 +144,14 @@ sub startup ($self) {
# only provide a URL helper - this is overtaken by apache
my $config = $app->config;
my $require_auth_for_assets = $config->{auth}->{require_for_assets};
my $assets_r = $require_auth_for_assets ? $logged_in : $r;
my $assets_r = $require_auth_for_assets ? $auth_any_user : $r;
$assets_r->get('/assets/*assetpath')->name('download_asset')->to('file#download_asset');

my $test_r = $r->any('/tests/<testid:num>');
my $test_path = '/tests/<testid:num>';
my $test_r = $r->any($test_path);
$test_r = $test_r->under('/')->to('test#referer_check');
my $test_auth = $auth->any('/tests/<testid:num>' => {format => 0});
my $test_asset_r = $require_auth_for_assets ? $test_auth : $test_r;
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');
Expand Down
10 changes: 4 additions & 6 deletions t/03-auth.t
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,11 @@ subtest 'restricted asset downloads with setting `[auth] require_for_assets = 1`
$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)->get_ok('/assets/iso/test.iso')->status_is(302);
$t->content_unlike(qr/asset-ok/, 'asset not accessible when logged out');
$t->header_is('Location', $expected_redirect, 'redirect to login when accessing asset');
$t->get_ok('/logout')->status_is(302);
$t->get_ok('/tests/42/asset/iso/test.iso')->status_is(302);
$t->header_like('Location', qr|/login\?return_page=.*test.iso|, 'redirect to login when accessing test asset');
$t->content_unlike(qr/asset-ok/, 'test asset not accessible when 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');
$t->content_unlike(qr/asset-ok/, 'asset via test not accessible when logged out');
};

subtest OpenID => sub {
Expand Down

0 comments on commit 935d343

Please sign in to comment.