Merge branch 'jv-workhorse-mime-lazy' into 'master'

Workhorse: prepare for lazy upload preauth

See merge request gitlab-org/gitlab!87800

workhorse/internal/upload/artifacts_uploader.go

@@ -43,16 +43,12 @@ type artifactsUploadProcessor struct {
 // Artifacts is like a Multipart but specific for artifacts upload.
 func Artifacts(myAPI *api.API, h http.Handler, p Preparer) http.Handler {
 	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
-		opts, err := p.Prepare(a)
-		if err != nil {
-			helper.Fail500(w, r, fmt.Errorf("UploadArtifacts: error preparing file storage options"))
-			return
-		}
-
 		format := r.URL.Query().Get(ArtifactFormatKey)
-
-		mg := &artifactsUploadProcessor{format: format, SavedFileTracker: SavedFileTracker{Request: r}}
-		interceptMultipartFiles(w, r, h, a, mg, opts)
+		mg := &artifactsUploadProcessor{
+			format:           format,
+			SavedFileTracker: SavedFileTracker{Request: r},
+		}
+		interceptMultipartFiles(w, r, h, mg, &eagerAuthorizer{a}, p)
 	}, "/authorize")
 }
@@ -61,8 +57,7 @@ func (a *artifactsUploadProcessor) generateMetadataFromZip(ctx context.Context,
 	defer metaWriter.Close()
 	metaOpts := &destination.UploadOpts{
-		LocalTempPath:  os.TempDir(),
-		TempFilePrefix: "metadata.gz",
+		LocalTempPath: os.TempDir(),
 	}
 	fileName := file.LocalPath
@@ -87,7 +82,7 @@ func (a *artifactsUploadProcessor) generateMetadataFromZip(ctx context.Context,
 	done := make(chan saveResult)
 	go func() {
 		var result saveResult
-		result.FileHandler, result.error = destination.Upload(ctx, metaReader, -1, metaOpts)
+		result.FileHandler, result.error = destination.Upload(ctx, metaReader, -1, "metadata.gz", metaOpts)
 		done <- result
 	}()

workhorse/internal/upload/body_uploader.go

@@ -23,7 +23,7 @@ func RequestBody(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler {
 			return
 		}
-		fh, err := destination.Upload(r.Context(), r.Body, r.ContentLength, opts)
+		fh, err := destination.Upload(r.Context(), r.Body, r.ContentLength, "upload", opts)
 		if err != nil {
 			helper.Fail500(w, r, fmt.Errorf("RequestBody: upload failed: %v", err))
 			return

workhorse/internal/upload/destination/destination.go

@@ -113,9 +113,9 @@ type consumer interface {
 // Upload persists the provided reader content to all the location specified in opts. A cleanup will be performed once ctx is Done
 // Make sure the provided context will not expire before finalizing upload with GitLab Rails.
-func Upload(ctx context.Context, reader io.Reader, size int64, opts *UploadOpts) (*FileHandler, error) {
+func Upload(ctx context.Context, reader io.Reader, size int64, name string, opts *UploadOpts) (*FileHandler, error) {
 	fh := &FileHandler{
-		Name:      opts.TempFilePrefix,
+		Name:      name,
 		RemoteID:  opts.RemoteID,
 		RemoteURL: opts.RemoteURL,
 	}
@@ -199,13 +199,13 @@ func Upload(ctx context.Context, reader io.Reader, size int64, opts *UploadOpts)
 	}
 	logger := log.WithContextFields(ctx, log.Fields{
-		"copied_bytes":     fh.Size,
-		"is_local":         opts.IsLocalTempFile(),
-		"is_multipart":     opts.IsMultipart(),
-		"is_remote":        !opts.IsLocalTempFile(),
-		"remote_id":        opts.RemoteID,
-		"temp_file_prefix": opts.TempFilePrefix,
-		"client_mode":      clientMode,
+		"copied_bytes": fh.Size,
+		"is_local":     opts.IsLocalTempFile(),
+		"is_multipart": opts.IsMultipart(),
+		"is_remote":    !opts.IsLocalTempFile(),
+		"remote_id":    opts.RemoteID,
+		"client_mode":  clientMode,
+		"filename":     fh.Name,
 	})
 	if opts.IsLocalTempFile() {
@@ -226,7 +226,7 @@ func (fh *FileHandler) newLocalFile(ctx context.Context, opts *UploadOpts) (cons
 		return nil, fmt.Errorf("newLocalFile: mkdir %q: %v", opts.LocalTempPath, err)
 	}
-	file, err := ioutil.TempFile(opts.LocalTempPath, opts.TempFilePrefix)
+	file, err := ioutil.TempFile(opts.LocalTempPath, "gitlab-workhorse-upload")
 	if err != nil {
 		return nil, fmt.Errorf("newLocalFile: create file: %v", err)
 	}

workhorse/internal/upload/destination/destination_test.go

@@ -47,8 +47,8 @@ func TestUploadWrongSize(t *testing.T) {
 	require.NoError(t, err)
 	defer os.RemoveAll(tmpFolder)
-	opts := &destination.UploadOpts{LocalTempPath: tmpFolder, TempFilePrefix: "test-file"}
-	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize+1, opts)
+	opts := &destination.UploadOpts{LocalTempPath: tmpFolder}
+	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize+1, "upload", opts)
 	require.Error(t, err)
 	_, isSizeError := err.(destination.SizeError)
 	require.True(t, isSizeError, "Should fail with SizeError")
@@ -63,8 +63,8 @@ func TestUploadWithKnownSizeExceedLimit(t *testing.T) {
 	require.NoError(t, err)
 	defer os.RemoveAll(tmpFolder)
-	opts := &destination.UploadOpts{LocalTempPath: tmpFolder, TempFilePrefix: "test-file", MaximumSize: test.ObjectSize - 1}
-	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, opts)
+	opts := &destination.UploadOpts{LocalTempPath: tmpFolder, MaximumSize: test.ObjectSize - 1}
+	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", opts)
 	require.Error(t, err)
 	_, isSizeError := err.(destination.SizeError)
 	require.True(t, isSizeError, "Should fail with SizeError")
@@ -79,8 +79,8 @@ func TestUploadWithUnknownSizeExceedLimit(t *testing.T) {
 	require.NoError(t, err)
 	defer os.RemoveAll(tmpFolder)
-	opts := &destination.UploadOpts{LocalTempPath: tmpFolder, TempFilePrefix: "test-file", MaximumSize: test.ObjectSize - 1}
-	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), -1, opts)
+	opts := &destination.UploadOpts{LocalTempPath: tmpFolder, MaximumSize: test.ObjectSize - 1}
+	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), -1, "upload", opts)
 	require.Equal(t, err, destination.ErrEntityTooLarge)
 	require.Nil(t, fh)
 }
@@ -117,7 +117,7 @@ func TestUploadWrongETag(t *testing.T) {
 				osStub.InitiateMultipartUpload(test.ObjectPath)
 			}
 			ctx, cancel := context.WithCancel(context.Background())
-			fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, opts)
+			fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", opts)
 			require.Nil(t, fh)
 			require.Error(t, err)
 			require.Equal(t, 1, osStub.PutsCnt(), "File not uploaded")
@@ -191,13 +191,12 @@ func TestUpload(t *testing.T) {
 			if spec.local {
 				opts.LocalTempPath = tmpFolder
-				opts.TempFilePrefix = "test-file"
 			}
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
-			fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, &opts)
+			fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", &opts)
 			require.NoError(t, err)
 			require.NotNil(t, fh)
@@ -211,9 +210,6 @@ func TestUpload(t *testing.T) {
 				dir := path.Dir(fh.LocalPath)
 				require.Equal(t, opts.LocalTempPath, dir)
-				filename := path.Base(fh.LocalPath)
-				beginsWithPrefix := strings.HasPrefix(filename, opts.TempFilePrefix)
-				require.True(t, beginsWithPrefix, fmt.Sprintf("LocalPath filename %q do not begin with TempFilePrefix %q", filename, opts.TempFilePrefix))
 			} else {
 				require.Empty(t, fh.LocalPath, "LocalPath must be empty for non local uploads")
 			}
@@ -291,7 +287,7 @@ func TestUploadWithS3WorkhorseClient(t *testing.T) {
 				MaximumSize: tc.maxSize,
 			}
-			_, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), tc.objectSize, &opts)
+			_, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), tc.objectSize, "upload", &opts)
 			if tc.expectedErr == nil {
 				require.NoError(t, err)
@@ -324,7 +320,7 @@ func TestUploadWithAzureWorkhorseClient(t *testing.T) {
 		},
 	}
-	_, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, &opts)
+	_, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", &opts)
 	require.NoError(t, err)
 	test.GoCloudObjectExists(t, bucketDir, remoteObject)
@@ -349,7 +345,7 @@ func TestUploadWithUnknownGoCloudScheme(t *testing.T) {
 		},
 	}
-	_, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, &opts)
+	_, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", &opts)
 	require.Error(t, err)
 }
@@ -375,7 +371,7 @@ func TestUploadMultipartInBodyFailure(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
-	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, &opts)
+	fh, err := destination.Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", &opts)
 	require.Nil(t, fh)
 	require.Error(t, err)
 	require.EqualError(t, err, test.MultipartUploadInternalError().Error())
@@ -468,7 +464,7 @@ func TestUploadRemoteFileWithLimit(t *testing.T) {
 				ctx, cancel := context.WithCancel(context.Background())
 				defer cancel()
-				fh, err := destination.Upload(ctx, strings.NewReader(tc.testData), tc.objectSize, &opts)
+				fh, err := destination.Upload(ctx, strings.NewReader(tc.testData), tc.objectSize, "upload", &opts)
 				if tc.expectedErr == nil {
 					require.NoError(t, err)

workhorse/internal/upload/destination/objectstore/test/objectstore_stub.go

@@ -22,7 +22,8 @@ type partsEtagMap map[int]string
 // Instead of storing objects it will just save md5sum.
 type ObjectstoreStub struct {
 	// bucket contains md5sum of uploaded objects
-	bucket map[string]string
+	bucket   map[string]string
+	contents map[string][]byte
 	// overwriteMD5 contains overwrites for md5sum that should be return instead of the regular hash
 	overwriteMD5 map[string]string
 	// multipart is a map of MultipartUploads
@@ -48,6 +49,7 @@ func StartObjectStoreWithCustomMD5(md5Hashes map[string]string) (*ObjectstoreStu
 		multipart:    make(map[string]partsEtagMap),
 		overwriteMD5: make(map[string]string),
 		headers:      make(map[string]*http.Header),
+		contents:     make(map[string][]byte),
 	}
 	for k, v := range md5Hashes {
@@ -82,6 +84,15 @@ func (o *ObjectstoreStub) GetObjectMD5(path string) string {
 	return o.bucket[path]
 }
+// GetObject returns the contents of the uploaded object. The caller must
+// not modify the byte slice.
+func (o *ObjectstoreStub) GetObject(path string) []byte {
+	o.m.Lock()
+	defer o.m.Unlock()
+
+	return o.contents[path]
+}
+
 // GetHeader returns a given HTTP header of the object uploaded to the path
 func (o *ObjectstoreStub) GetHeader(path, key string) string {
 	o.m.Lock()
@@ -154,11 +165,11 @@ func (o *ObjectstoreStub) putObject(w http.ResponseWriter, r *http.Request) {
 	etag, overwritten := o.overwriteMD5[objectPath]
 	if !overwritten {
+		buf, _ := io.ReadAll(r.Body)
+		o.contents[objectPath] = buf
 		hasher := md5.New()
-		io.Copy(hasher, r.Body)
-
-		checksum := hasher.Sum(nil)
-		etag = hex.EncodeToString(checksum)
+		hasher.Write(buf)
+		etag = hex.EncodeToString(hasher.Sum(nil))
 	}
 	o.headers[objectPath] = &r.Header

workhorse/internal/upload/destination/upload_opts.go

@@ -29,8 +29,6 @@ type ObjectStorageConfig struct {
 // UploadOpts represents all the options available for saving a file to object store
 type UploadOpts struct {
-	// TempFilePrefix is the prefix used to create temporary local file
-	TempFilePrefix string
 	// LocalTempPath is the directory where to write a local copy of the file
 	LocalTempPath string
 	// RemoteID is the remote ObjectID provided by GitLab

workhorse/internal/upload/multipart_uploader.go

 package upload
 import (
-	"fmt"
 	"net/http"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
-	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
 )
 // Multipart is a request middleware. If the request has a MIME multipart
@@ -17,12 +15,19 @@ func Multipart(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler {
 	return rails.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
 		s := &SavedFileTracker{Request: r}
-		opts, err := p.Prepare(a)
-		if err != nil {
-			helper.Fail500(w, r, fmt.Errorf("Multipart: error preparing file storage options"))
-			return
-		}
-
-		interceptMultipartFiles(w, r, h, a, s, opts)
+		interceptMultipartFiles(w, r, h, s, &eagerAuthorizer{a}, p)
 	}, "/authorize")
 }
+
+// SkipRailsPreAuthMultipart behaves like Multipart except it does not
+// pre-authorize with Rails. It is intended for use on catch-all routes
+// where we cannot pre-authorize both because we don't know which Rails
+// endpoint to call, and because eagerly pre-authorizing would add too
+// much overhead.
+func SkipRailsPreAuthMultipart(tempPath string, h http.Handler, p Preparer) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		s := &SavedFileTracker{Request: r}
+		fa := &eagerAuthorizer{&api.Response{TempPath: tempPath}}
+		interceptMultipartFiles(w, r, h, s, fa, p)
+	})
+}

workhorse/internal/upload/rewrite.go

@@ -62,13 +62,14 @@ var (
 )
 type rewriter struct {
-	writer          *multipart.Writer
-	preauth         *api.Response
+	writer *multipart.Writer
+	fileAuthorizer
+	Preparer
 	filter          MultipartFormProcessor
 	finalizedFields map[string]bool
 }
-func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, preauth *api.Response, filter MultipartFormProcessor, opts *destination.UploadOpts) error {
+func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, filter MultipartFormProcessor, fa fileAuthorizer, preparer Preparer) error {
 	// Create multipart reader
 	reader, err := r.MultipartReader()
 	if err != nil {
@@ -83,7 +84,8 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 	rew := &rewriter{
 		writer:          writer,
-		preauth:         preauth,
+		fileAuthorizer:  fa,
+		Preparer:        preparer,
 		filter:          filter,
 		finalizedFields: make(map[string]bool),
 	}
@@ -108,7 +110,7 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 		}
 		if filename != "" {
-			err = rew.handleFilePart(r.Context(), name, p, opts)
+			err = rew.handleFilePart(r, name, p)
 		} else {
 			err = rew.copyPart(r.Context(), name, p)
 		}
@@ -128,7 +130,7 @@ func parseAndNormalizeContentDisposition(header textproto.MIMEHeader) (string, s
 	return params["name"], params["filename"]
 }
-func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *destination.UploadOpts) error {
+func (rew *rewriter) handleFilePart(r *http.Request, name string, p *multipart.Part) error {
 	if rew.filter.Count() >= maxFilesAllowed {
 		return ErrTooManyFilesUploaded
 	}
@@ -141,30 +143,34 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 		return fmt.Errorf("illegal filename: %q", filename)
 	}
-	opts.TempFilePrefix = filename
+	apiResponse, err := rew.AuthorizeFile(r)
+	if err != nil {
+		return err
+	}
+	opts, err := rew.Prepare(apiResponse)
+	if err != nil {
+		return err
+	}
 	var inputReader io.ReadCloser
-	var err error
-
-	imageType := exif.FileTypeFromSuffix(filename)
-	switch {
-	case imageType != exif.TypeUnknown:
+	ctx := r.Context()
+	if imageType := exif.FileTypeFromSuffix(filename); imageType != exif.TypeUnknown {
 		inputReader, err = handleExifUpload(ctx, p, filename, imageType)
 		if err != nil {
 			return err
 		}
-	case rew.preauth.ProcessLsif:
-		inputReader, err = handleLsifUpload(ctx, p, opts.LocalTempPath, filename, rew.preauth)
+	} else if apiResponse.ProcessLsif {
+		inputReader, err = handleLsifUpload(ctx, p, opts.LocalTempPath, filename)
 		if err != nil {
 			return err
 		}
-	default:
+	} else {
 		inputReader = ioutil.NopCloser(p)
 	}
 	defer inputReader.Close()
-	fh, err := destination.Upload(ctx, inputReader, -1, opts)
+	fh, err := destination.Upload(ctx, inputReader, -1, filename, opts)
 	if err != nil {
 		switch err {
 		case destination.ErrEntityTooLarge, exif.ErrRemovingExif:
@@ -267,7 +273,7 @@ func isJPEG(r io.Reader) bool {
 	return http.DetectContentType(buf) == "image/jpeg"
 }
-func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string, preauth *api.Response) (io.ReadCloser, error) {
+func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string) (io.ReadCloser, error) {
 	parserConfig := parser.Config{
 		TempPath: tempPath,
 	}
@@ -291,3 +297,15 @@ func (rew *rewriter) copyPart(ctx context.Context, name string, p *multipart.Par
 	return nil
 }
+
+type fileAuthorizer interface {
+	AuthorizeFile(*http.Request) (*api.Response, error)
+}
+
+type eagerAuthorizer struct{ response *api.Response }
+
+func (ea *eagerAuthorizer) AuthorizeFile(r *http.Request) (*api.Response, error) {
+	return ea.response, nil
+}
+
+var _ fileAuthorizer = &eagerAuthorizer{}

workhorse/internal/upload/skip_rails_authorizer.go

-package upload
-
-import (
-	"net/http"
-
-	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
-)
-
-// SkipRailsAuthorizer implements a fake PreAuthorizer that does not call
-// the gitlab-rails API. It must be fast because it gets called on each
-// request proxied to Rails.
-type SkipRailsAuthorizer struct {
-	// TempPath is a directory where workhorse can store files that can later
-	// be accessed by gitlab-rails.
-	TempPath string
-}
-
-func (l *SkipRailsAuthorizer) PreAuthorizeHandler(next api.HandleFunc, _ string) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		next(w, r, &api.Response{TempPath: l.TempPath})
-	})
-}

workhorse/internal/upload/uploads.go

@@ -40,13 +40,13 @@ type MultipartFormProcessor interface {
 // interceptMultipartFiles is the core of the implementation of
 // Multipart.
-func interceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *destination.UploadOpts) {
+func interceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Handler, filter MultipartFormProcessor, fa fileAuthorizer, p Preparer) {
 	var body bytes.Buffer
 	writer := multipart.NewWriter(&body)
 	defer writer.Close()
 	// Rewrite multipart form data
-	err := rewriteFormFilesFromMultipart(r, writer, preauth, filter, opts)
+	err := rewriteFormFilesFromMultipart(r, writer, filter, fa, p)
 	if err != nil {
 		switch err {
 		case ErrInjectedClientParam:

workhorse/internal/upload/uploads_test.go

@@ -12,6 +12,7 @@ import (
 	"net/http/httptest"
 	"net/textproto"
 	"os"
+	"path"
 	"regexp"
 	"strconv"
 	"strings"
@@ -66,19 +67,14 @@ func TestUploadHandlerForwardingRawData(t *testing.T) {
 	httpRequest, err := http.NewRequest("PATCH", ts.URL+"/url/path", bytes.NewBufferString("REQUEST"))
 	require.NoError(t, err)
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
+	tempPath := t.TempDir()
 	response := httptest.NewRecorder()
 	handler := newProxy(ts.URL)
-	apiResponse := &api.Response{TempPath: tempPath}
+	fa := &eagerAuthorizer{&api.Response{TempPath: tempPath}}
 	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(response, httpRequest, handler, apiResponse, nil, opts)
+	interceptMultipartFiles(response, httpRequest, handler, nil, fa, preparer)
 	require.Equal(t, 202, response.Code)
 	require.Equal(t, "RESPONSE", response.Body.String(), "response body")
@@ -86,10 +82,7 @@ func TestUploadHandlerForwardingRawData(t *testing.T) {
 func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 	var filePath string
-
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
+	tempPath := t.TempDir()
 	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) {
 		require.Equal(t, "PUT", r.Method, "method")
@@ -144,12 +137,10 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 	handler := newProxy(ts.URL)
-	apiResponse := &api.Response{TempPath: tempPath}
+	fa := &eagerAuthorizer{&api.Response{TempPath: tempPath}}
 	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
+	interceptMultipartFiles(response, httpRequest, handler, &testFormProcessor{}, fa, preparer)
 	require.Equal(t, 202, response.Code)
 	cancel() // this will trigger an async cleanup
@@ -159,10 +150,6 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) {
 	var filePath string
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
 	tests := []struct {
 		name     string
 		field    string
@@ -213,12 +200,8 @@ func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) {
 			response := httptest.NewRecorder()
 			handler := newProxy(ts.URL)
-			apiResponse := &api.Response{TempPath: tempPath}
-			preparer := &DefaultPreparer{}
-			opts, err := preparer.Prepare(apiResponse)
-			require.NoError(t, err)
-			interceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
+			testInterceptMultipartFiles(t, response, httpRequest, handler, &testFormProcessor{})
 			require.Equal(t, test.response, response.Code)
 			cancel() // this will trigger an async cleanup
@@ -228,10 +211,6 @@ func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) {
 }
 func TestUploadProcessingField(t *testing.T) {
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
 	var buffer bytes.Buffer
 	writer := multipart.NewWriter(&buffer)
@@ -243,12 +222,8 @@ func TestUploadProcessingField(t *testing.T) {
 	httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 	response := httptest.NewRecorder()
-	apiResponse := &api.Response{TempPath: tempPath}
-	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
+	testInterceptMultipartFiles(t, response, httpRequest, nilHandler, &testFormProcessor{})
 	require.Equal(t, 500, response.Code)
 }
@@ -256,15 +231,11 @@ func TestUploadProcessingField(t *testing.T) {
 func TestUploadingMultipleFiles(t *testing.T) {
 	testhelper.ConfigureSecret()
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
 	var buffer bytes.Buffer
 	writer := multipart.NewWriter(&buffer)
 	for i := 0; i < 11; i++ {
-		_, err = writer.CreateFormFile(fmt.Sprintf("file %v", i), "my.file")
+		_, err := writer.CreateFormFile(fmt.Sprintf("file %v", i), "my.file")
 		require.NoError(t, err)
 	}
 	require.NoError(t, writer.Close())
@@ -274,23 +245,18 @@ func TestUploadingMultipleFiles(t *testing.T) {
 	httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 	response := httptest.NewRecorder()
-	apiResponse := &api.Response{TempPath: tempPath}
-	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
+	testInterceptMultipartFiles(t, response, httpRequest, nilHandler, &testFormProcessor{})
 	require.Equal(t, 400, response.Code)
 	require.Equal(t, "upload request contains more than 10 files\n", response.Body.String())
 }
 func TestUploadProcessingFile(t *testing.T) {
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-	_, testServer := test.StartObjectStore()
+	testhelper.ConfigureSecret()
+	tempPath := t.TempDir()
+	objectStore, testServer := test.StartObjectStore()
 	defer testServer.Close()
 	storeUrl := testServer.URL + test.ObjectPath
@@ -298,21 +264,24 @@ func TestUploadProcessingFile(t *testing.T) {
 	tests := []struct {
 		name    string
 		preauth *api.Response
+		content func(t *testing.T) []byte
 	}{
 		{
 			name:    "FileStore Upload",
 			preauth: &api.Response{TempPath: tempPath},
+			content: func(t *testing.T) []byte {
+				entries, err := os.ReadDir(tempPath)
+				require.NoError(t, err)
+				require.Len(t, entries, 1)
+				content, err := os.ReadFile(path.Join(tempPath, entries[0].Name()))
+				require.NoError(t, err)
+				return content
+			},
 		},
 		{
 			name:    "ObjectStore Upload",
-			preauth: &api.Response{RemoteObject: api.RemoteObject{StoreURL: storeUrl}},
-		},
-		{
-			name: "ObjectStore and FileStore Upload",
-			preauth: &api.Response{
-				TempPath:     tempPath,
-				RemoteObject: api.RemoteObject{StoreURL: storeUrl},
-			},
+			preauth: &api.Response{RemoteObject: api.RemoteObject{StoreURL: storeUrl, ID: "123"}},
+			content: func(*testing.T) []byte { return objectStore.GetObject(test.ObjectPath) },
 		},
 	}
@@ -330,26 +299,20 @@ func TestUploadProcessingFile(t *testing.T) {
 			httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 			response := httptest.NewRecorder()
-			apiResponse := &api.Response{TempPath: tempPath}
+			fa := &eagerAuthorizer{test.preauth}
 			preparer := &DefaultPreparer{}
-			opts, err := preparer.Prepare(apiResponse)
-			require.NoError(t, err)
-			interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
+			interceptMultipartFiles(response, httpRequest, nilHandler, &testFormProcessor{}, fa, preparer)
 			require.Equal(t, 200, response.Code)
+			require.Equal(t, "test", string(test.content(t)))
 		})
 	}
-
 }
 func TestInvalidFileNames(t *testing.T) {
 	testhelper.ConfigureSecret()
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
 	for _, testCase := range []struct {
 		filename       string
 		code           int
@@ -376,24 +339,14 @@ func TestInvalidFileNames(t *testing.T) {
 		httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 		response := httptest.NewRecorder()
-		apiResponse := &api.Response{TempPath: tempPath}
-		preparer := &DefaultPreparer{}
-		opts, err := preparer.Prepare(apiResponse)
-		require.NoError(t, err)
-
-		interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
+		testInterceptMultipartFiles(t, response, httpRequest, nilHandler, &SavedFileTracker{Request: httpRequest})
 		require.Equal(t, testCase.code, response.Code)
-		require.Equal(t, testCase.expectedPrefix, opts.TempFilePrefix)
 	}
 }
 func TestContentDispositionRewrite(t *testing.T) {
 	testhelper.ConfigureSecret()
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
 	tests := []struct {
 		desc            string
 		header          string
@@ -442,12 +395,7 @@ func TestContentDispositionRewrite(t *testing.T) {
 			})
 			response := httptest.NewRecorder()
-			apiResponse := &api.Response{TempPath: tempPath}
-			preparer := &DefaultPreparer{}
-			opts, err := preparer.Prepare(apiResponse)
-			require.NoError(t, err)
-
-			interceptMultipartFiles(response, httpRequest, customHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
+			testInterceptMultipartFiles(t, response, httpRequest, customHandler, &SavedFileTracker{Request: httpRequest})
 			upstreamRequest, err := http.ReadRequest(bufio.NewReader(&upstreamRequestBuffer))
 			require.NoError(t, err)
@@ -534,10 +482,6 @@ func TestUploadHandlerRemovingExifCorruptedFile(t *testing.T) {
 }
 func runUploadTest(t *testing.T, image []byte, filename string, httpCode int, tsHandler func(http.ResponseWriter, *http.Request)) {
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
 	var buffer bytes.Buffer
 	writer := multipart.NewWriter(&buffer)
@@ -565,12 +509,8 @@ func runUploadTest(t *testing.T, image []byte, filename string, httpCode int, ts
 	response := httptest.NewRecorder()
 	handler := newProxy(ts.URL)
-	apiResponse := &api.Response{TempPath: tempPath}
-	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
+	testInterceptMultipartFiles(t, response, httpRequest, handler, &testFormProcessor{})
 	require.Equal(t, httpCode, response.Code)
 }
@@ -587,3 +527,12 @@ func waitUntilDeleted(t *testing.T, path string) {
 	}, 10*time.Second, 10*time.Millisecond)
 	require.True(t, os.IsNotExist(err), "expected the file to be deleted")
 }
+
+func testInterceptMultipartFiles(t *testing.T, w http.ResponseWriter, r *http.Request, h http.Handler, filter MultipartFormProcessor) {
+	t.Helper()
+
+	fa := &eagerAuthorizer{&api.Response{TempPath: t.TempDir()}}
+	preparer := &DefaultPreparer{}
+
+	interceptMultipartFiles(w, r, h, filter, fa, preparer)
+}

workhorse/internal/upstream/routes.go

@@ -223,7 +223,7 @@ func configureRoutes(u *upstream) {
 	mimeMultipartUploader := upload.Multipart(api, signingProxy, preparer)
 	uploadPath := path.Join(u.DocumentRoot, "uploads/tmp")
-	tempfileMultipartProxy := upload.Multipart(&upload.SkipRailsAuthorizer{TempPath: uploadPath}, proxy, preparer)
+	tempfileMultipartProxy := upload.SkipRailsPreAuthMultipart(uploadPath, proxy, preparer)
 	ciAPIProxyQueue := queueing.QueueRequests("ci_api_job_requests", tempfileMultipartProxy, u.APILimit, u.APIQueueLimit, u.APIQueueTimeout)
 	ciAPILongPolling := builds.RegisterHandler(ciAPIProxyQueue, redis.WatchKey, u.APICILongPollingDuration)