Return explicit error when preauth is denied
requested to merge 412605-return-401-status-when-fixed-pre-authorizer-gets-401-from-gitlab-rails into master
What does this MR do and why?
This MR aims to handle and improve how requests to Rails from Workhorse are handled when a HTTP 401 error is returned when authorizing uploads. Currently when a pre-auth request is denied by Rails (HTTP 401), Workhorse incorrectly returns a HTTP 500 instead of the original HTTP 401.
How to set up and validate locally
Replicating the bug in master
- Change into your
<GDK_ROOT>/gitlab
directory. - Check out the current GitLab
master
branch. - Apply the following patch to make it easier to create the unauthorised scenario:
diff --git a/lib/api/internal/workhorse.rb b/lib/api/internal/workhorse.rb index 910cf52bc3bc..d1fa6b18a66d 100644 --- a/lib/api/internal/workhorse.rb +++ b/lib/api/internal/workhorse.rb @@ -25,7 +25,7 @@ def request_authenticated? namespace 'internal' do namespace 'workhorse' do post 'authorize_upload' do - unauthorized! unless request_authenticated? + unauthorized! # unless request_authenticated? status 200 { TempPath: File.join(::Gitlab.config.uploads.storage_path, 'uploads/tmp') }
- Change into your
<GDK_ROOT>
directory. - Start up your GDK with
gdk start
. - Build a new workhorse by running
make gitlab-workhorse-update
. - Restart Workhorse
gdk restart gitlab-workhorse
. - Run
gdk tail workhorse | grep -A1 error
to watch Workhorse output for errors. - Once your GDK is ready, open up any project that has a repository setup.
- Click the
+
button and then 'Upload file': - Select any file to upload and click 'Upload file'.
- Observe the following similar lines in the output of the
gdk tail workhorse | grep -A1 error
command, noting aHTTP 500
status code/error is returned:2023-12-14_05:09:31.12270 gitlab-workhorse : {"correlation_id":"01HHKBNPR5N5PDX283J35FHRA2","error":"handleFileUploads: extract files from multipart: no api response: status 401","level":"error","method":"POST","msg":"","time":"2023-12-14T16:09:31+11:00","uri":"/root/codeowners-test/-/create/main"} 2023-12-14_05:09:31.12285 gitlab-workhorse : {"content_type":"text/plain; charset=utf-8","correlation_id":"01HHKBNPR5N5PDX283J35FHRA2","duration_ms":108,"host":"gdk.test:3000","level":"info","method":"POST","msg":"access","proto":"HTTP/1.1","referrer":"http://gdk.test:3000/root/codeowners-test","remote_addr":"172.16.123.1:57696","remote_ip":"172.16.123.1","route":"","status":500,"system":"http","time":"2023-12-14T16:09:31+11:00","ttfb_ms":105,"uri":"/root/codeowners-test/-/create/main","user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:120.0) Gecko/20100101 Firefox/120.0","written_bytes":22}
Testing the bug is fixed in 412605-return-401-status-when-fixed-pre-authorizer-gets-401-from-gitlab-rails
- Repeat all steps above, changing the GitLab branch in step 2. from
master
to412605-return-401-status-when-fixed-pre-authorizer-gets-401-from-gitlab-rails
. - Observe the following similar lines in the output of the
gdk tail workhorse | grep -A1 error
command, noting aHTTP 401
status code/error is returned instead:2023-12-14_05:11:26.48925 gitlab-workhorse : {"correlation_id":"01HHKBS7CCD0WCXT88AED48MB0","error":"no api response: status 401","level":"error","method":"POST","msg":"","time":"2023-12-14T16:11:26+11:00","uri":"/root/codeowners-test/-/create/main"} 2023-12-14_05:11:26.48939 gitlab-workhorse : {"content_type":"text/plain; charset=utf-8","correlation_id":"01HHKBS7CCD0WCXT88AED48MB0","duration_ms":139,"host":"gdk.test:3000","level":"info","method":"POST","msg":"access","proto":"HTTP/1.1","referrer":"http://gdk.test:3000/root/codeowners-test","remote_addr":"172.16.123.1:57898","remote_ip":"172.16.123.1","route":"","status":401,"system":"http","time":"2023-12-14T16:11:26+11:00","ttfb_ms":135,"uri":"/root/codeowners-test/-/create/main","user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:120.0) Gecko/20100101 Firefox/120.0","written_bytes":17}
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #412605 (closed)
Edited by Ash McKenzie