Fix HTTP status code when path is invalid (#2294)

changelog/unreleased/fix-archiver-errors.md

@@ -0,0 +1,7 @@
+Bugfix: Fix HTTP return code when path is invalid
+
+Before when a path was invalid, the archiver returned a
+500 error code.
+Now this is fixed and returns a 404 code.
+
+https://github.com/cs3org/reva/pull/2294

internal/http/services/archiver/handler.go

@@ -44,7 +44,6 @@ import (
 	"github.com/gdexlab/go-render/render"
 	ua "github.com/mileusna/useragent"
 	"github.com/mitchellh/mapstructure"
-	"github.com/pkg/errors"
 	"github.com/rs/zerolog"
 )
 
@@ -193,6 +192,23 @@ func (s *svc) allAllowed(paths []string) error {
 	return nil
 }
 
+func (s *svc) writeHTTPError(rw http.ResponseWriter, err error) {
+	s.log.Error().Msg(err.Error())
+
+	switch err.(type) {
+	case errtypes.NotFound:
+		rw.WriteHeader(http.StatusNotFound)
+	case manager.ErrMaxSize, manager.ErrMaxFileCount:
+		rw.WriteHeader(http.StatusRequestEntityTooLarge)
+	case errtypes.BadRequest:
+		rw.WriteHeader(http.StatusBadRequest)
+	default:
+		rw.WriteHeader(http.StatusInternalServerError)
+	}
+
+	_, _ = rw.Write([]byte(err.Error()))
+}
+
 func (s *svc) Handler() http.Handler {
 	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
 		// get the paths and/or the resources id from the query
@@ -210,9 +226,7 @@ func (s *svc) Handler() http.Handler {
 
 		files, err := s.getFiles(ctx, paths, ids)
 		if err != nil {
-			s.log.Error().Msg(err.Error())
-			rw.WriteHeader(http.StatusBadRequest)
-			_, _ = rw.Write([]byte(err.Error()))
+			s.writeHTTPError(rw, err)
 			return
 		}
 
@@ -221,9 +235,7 @@ func (s *svc) Handler() http.Handler {
 			MaxSize:     s.config.MaxSize,
 		})
 		if err != nil {
-			s.log.Error().Msg(err.Error())
-			rw.WriteHeader(http.StatusInternalServerError)
-			_, _ = rw.Write([]byte(err.Error()))
+			s.writeHTTPError(rw, err)
 			return
 		}
 
@@ -248,14 +260,8 @@ func (s *svc) Handler() http.Handler {
 			err = arch.CreateTar(ctx, rw)
 		}
 
-		if err == manager.ErrMaxFileCount || err == manager.ErrMaxSize {
-			s.log.Error().Msg(err.Error())
-			rw.WriteHeader(http.StatusRequestEntityTooLarge)
-			return
-		}
 		if err != nil {
-			s.log.Error().Msg(err.Error())
-			rw.WriteHeader(http.StatusInternalServerError)
+			s.writeHTTPError(rw, err)
 			return
 		}
 
@@ -277,7 +283,7 @@ func (s *svc) Unprotected() []string {
 func decodeResourceID(encodedID string) (string, string, error) {
 	decodedID, err := base64.URLEncoding.DecodeString(encodedID)
 	if err != nil {
-		return "", "", errors.Wrap(err, "resource ID does not follow the required format")
+		return "", "", errtypes.BadRequest("resource ID does not follow the required format")
 	}
 
 	parts := strings.Split(string(decodedID), ":")

internal/http/services/archiver/manager/archiver.go

@@ -28,20 +28,10 @@ import (
 	"time"
 
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
-	"github.com/cs3org/reva/pkg/errtypes"
 	"github.com/cs3org/reva/pkg/storage/utils/downloader"
 	"github.com/cs3org/reva/pkg/storage/utils/walker"
 )
 
-const (
-	// ErrMaxFileCount is the error returned when the max files count specified in the config has reached
-	ErrMaxFileCount = errtypes.InternalError("reached max files count")
-	// ErrMaxSize is the error returned when the max total files size specified in the config has reached
-	ErrMaxSize = errtypes.InternalError("reached max total files size")
-	// ErrEmptyList is the error returned when an empty list is passed when an archiver is created
-	ErrEmptyList = errtypes.BadRequest("list of files to archive empty")
-)
-
 // Config is the config for the Archiver
 type Config struct {
 	MaxNumFiles int64
@@ -60,7 +50,7 @@ type Archiver struct {
 // NewArchiver creates a new archiver able to create an archive containing the files in the list
 func NewArchiver(files []string, w walker.Walker, d downloader.Downloader, config Config) (*Archiver, error) {
 	if len(files) == 0 {
-		return nil, ErrEmptyList
+		return nil, ErrEmptyList{}
 	}
 
 	dir := getDeepestCommonDir(files)
@@ -140,7 +130,7 @@ func (a *Archiver) CreateTar(ctx context.Context, dst io.Writer) error {
 
 			filesCount++
 			if filesCount > a.config.MaxNumFiles {
-				return ErrMaxFileCount
+				return ErrMaxFileCount{}
 			}
 
 			if !isDir {
@@ -149,7 +139,7 @@ func (a *Archiver) CreateTar(ctx context.Context, dst io.Writer) error {
 				// count the files not only once
 				sizeFiles += int64(info.Size)
 				if sizeFiles > a.config.MaxSize {
-					return ErrMaxSize
+					return ErrMaxSize{}
 				}
 			}
 
@@ -214,7 +204,7 @@ func (a *Archiver) CreateZip(ctx context.Context, dst io.Writer) error {
 
 			filesCount++
 			if filesCount > a.config.MaxNumFiles {
-				return ErrMaxFileCount
+				return ErrMaxFileCount{}
 			}
 
 			if !isDir {
@@ -223,7 +213,7 @@ func (a *Archiver) CreateZip(ctx context.Context, dst io.Writer) error {
 				// count the files not only once
 				sizeFiles += int64(info.Size)
 				if sizeFiles > a.config.MaxSize {
-					return ErrMaxSize
+					return ErrMaxSize{}
 				}
 			}
 

internal/http/services/archiver/manager/archiver_test.go

@@ -207,7 +207,7 @@ func TestCreateTar(t *testing.T) {
 			},
 			files:    []string{"foo"},
 			expected: nil,
-			err:      ErrMaxFileCount,
+			err:      ErrMaxFileCount{},
 		},
 		{
 			name: "one file - error max size reached",
@@ -222,7 +222,7 @@ func TestCreateTar(t *testing.T) {
 			},
 			files:    []string{"foo"},
 			expected: nil,
-			err:      ErrMaxSize,
+			err:      ErrMaxSize{},
 		},
 		{
 			name: "one folder empty",
@@ -250,7 +250,7 @@ func TestCreateTar(t *testing.T) {
 			},
 			files:    []string{"foo"},
 			expected: nil,
-			err:      ErrMaxFileCount,
+			err:      ErrMaxFileCount{},
 		},
 		{
 			name: "one folder - one file in",
@@ -655,7 +655,7 @@ func TestCreateZip(t *testing.T) {
 			},
 			files:    []string{"foo"},
 			expected: nil,
-			err:      ErrMaxFileCount,
+			err:      ErrMaxFileCount{},
 		},
 		{
 			name: "one file - error max size reached",
@@ -670,7 +670,7 @@ func TestCreateZip(t *testing.T) {
 			},
 			files:    []string{"foo"},
 			expected: nil,
-			err:      ErrMaxSize,
+			err:      ErrMaxSize{},
 		},
 		{
 			name: "one folder empty",
@@ -698,7 +698,7 @@ func TestCreateZip(t *testing.T) {
 			},
 			files:    []string{"foo"},
 			expected: nil,
-			err:      ErrMaxFileCount,
+			err:      ErrMaxFileCount{},
 		},
 		{
 			name: "one folder - one file in",

internal/http/services/archiver/manager/errors.go

@@ -0,0 +1,43 @@
+// Copyright 2018-2021 CERN
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// In applying this license, CERN does not waive the privileges and immunities
+// granted to it by virtue of its status as an Intergovernmental Organization
+// or submit itself to any jurisdiction.
+
+package manager
+
+// ErrMaxFileCount is the error returned when the max files count specified in the config has reached
+type ErrMaxFileCount struct{}
+
+// ErrMaxSize is the error returned when the max total files size specified in the config has reached
+type ErrMaxSize struct{}
+
+// ErrEmptyList is the error returned when an empty list is passed when an archiver is created
+type ErrEmptyList struct{}
+
+// Error returns the string error msg for ErrMaxFileCount
+func (ErrMaxFileCount) Error() string {
+	return "reached max files count"
+}
+
+// Error returns the string error msg for ErrMaxSize
+func (ErrMaxSize) Error() string {
+	return "reached max total files size"
+}
+
+// Error returns the string error msg for ErrEmptyList
+func (ErrEmptyList) Error() string {
+	return "list of files to archive empty"
+}

pkg/storage/utils/downloader/downloader.go

@@ -93,7 +93,12 @@ func (r *revaDownloader) Download(ctx context.Context, path string, dst io.Write
 	defer httpRes.Body.Close()
 
 	if httpRes.StatusCode != http.StatusOK {
-		return errtypes.InternalError(httpRes.Status)
+		switch httpRes.StatusCode {
+		case http.StatusNotFound:
+			return errtypes.NotFound(path)
+		default:
+			return errtypes.InternalError(httpRes.Status)
+		}
 	}
 
 	_, err = io.Copy(dst, httpRes.Body)