Skip to content

Commit

Permalink
OCDAV: map bad request and unimplemented codes (#1354)
Browse files Browse the repository at this point in the history 
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
  • Loading branch information
@butonic
butonic authored Dec 3, 2020
1 parent 1fb968b commit 783e35c
Show file tree
Hide file tree
Showing 15 changed files with 267 additions and 523 deletions.
5 changes: 5 additions & 0 deletions changelog/unreleased/ocdav-handle-bad-request.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Enhancement: Map bad request and unimplement to http status codes

We now return a 400 bad request when a grpc call fails with an invalid argument status and a 501 not implemented when it fails with an unimplemented status. This prevents 500 errors when a user tries to add resources to the Share folder or a storage does not implement an action.

https://github.com/cs3org/reva/pull/1354
53 changes: 17 additions & 36 deletions internal/http/services/owncloud/ocdav/copy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,13 @@ import (
"github.com/cs3org/reva/internal/http/services/datagateway"
"github.com/cs3org/reva/pkg/appctx"
"github.com/cs3org/reva/pkg/rhttp"
"go.opencensus.io/trace"
)

func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
ctx := r.Context()
log := appctx.GetLogger(ctx)
ctx, span := trace.StartSpan(ctx, "head")
defer span.End()

ns = applyLayout(ctx, ns)

Expand All @@ -55,8 +57,8 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
}
dst = path.Join(ns, dst)

log.Info().Str("source", src).Str("destination", dst).
Str("overwrite", overwrite).Str("depth", depth).Msg("copy")
sublog := appctx.GetLogger(ctx).With().Str("src", src).Str("dst", dst).Logger()
sublog.Debug().Str("overwrite", overwrite).Str("depth", depth).Msg("copy")

overwrite = strings.ToUpper(overwrite)
if overwrite == "" {
Expand All @@ -75,7 +77,7 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {

client, err := s.getClient()
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand All @@ -87,23 +89,13 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
srcStatReq := &provider.StatRequest{Ref: ref}
srcStatRes, err := client.Stat(ctx, srcStatReq)
if err != nil {
log.Error().Err(err).Msg("error sending grpc stat request")
sublog.Error().Err(err).Msg("error sending grpc stat request")
w.WriteHeader(http.StatusInternalServerError)
return
}

if srcStatRes.Status.Code != rpc.Code_CODE_OK {
switch srcStatRes.Status.Code {
case rpc.Code_CODE_NOT_FOUND:
log.Debug().Str("src", src).Interface("status", srcStatRes.Status).Msg("resource not found")
w.WriteHeader(http.StatusNotFound)
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Str("src", src).Interface("status", srcStatRes.Status).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Str("src", src).Interface("status", srcStatRes.Status).Msg("grpc stat request failed")
w.WriteHeader(http.StatusInternalServerError)
}
handleErrorStatus(&sublog, w, srcStatRes.Status)
return
}

Expand All @@ -114,19 +106,12 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
dstStatReq := &provider.StatRequest{Ref: ref}
dstStatRes, err := client.Stat(ctx, dstStatReq)
if err != nil {
log.Error().Err(err).Msg("error sending grpc stat request")
sublog.Error().Err(err).Msg("error sending grpc stat request")
w.WriteHeader(http.StatusInternalServerError)
return
}
if dstStatRes.Status.Code != rpc.Code_CODE_OK && dstStatRes.Status.Code != rpc.Code_CODE_NOT_FOUND {
switch dstStatRes.Status.Code {
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Str("dst", dst).Interface("status", dstStatRes.Status).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Str("dst", dst).Interface("status", dstStatRes.Status).Msg("grpc stat request failed")
w.WriteHeader(http.StatusInternalServerError)
}
handleErrorStatus(&sublog, w, srcStatRes.Status)
return
}

Expand All @@ -135,7 +120,7 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
successCode = http.StatusNoContent // 204 if target already existed, see https://tools.ietf.org/html/rfc4918#section-9.8.5

if overwrite == "F" {
log.Warn().Str("dst", dst).Msg("dst already exists")
sublog.Warn().Str("overwrite", overwrite).Msg("dst already exists")
w.WriteHeader(http.StatusPreconditionFailed) // 412, see https://tools.ietf.org/html/rfc4918#section-9.8.5
return
}
Expand All @@ -149,21 +134,17 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
intStatReq := &provider.StatRequest{Ref: ref}
intStatRes, err := client.Stat(ctx, intStatReq)
if err != nil {
log.Error().Err(err).Msg("error sending grpc stat request")
sublog.Error().Err(err).Msg("error sending grpc stat request")
w.WriteHeader(http.StatusInternalServerError)
return
}
if intStatRes.Status.Code != rpc.Code_CODE_OK {
switch intStatRes.Status.Code {
case rpc.Code_CODE_NOT_FOUND:
if intStatRes.Status.Code == rpc.Code_CODE_NOT_FOUND {
// 409 if intermediate dir is missing, see https://tools.ietf.org/html/rfc4918#section-9.8.5
sublog.Debug().Str("parent", intermediateDir).Interface("status", intStatRes.Status).Msg("conflict")
w.WriteHeader(http.StatusConflict)
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Str("dst", dst).Interface("status", intStatRes.Status).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Str("dst", dst).Interface("status", intStatRes.Status).Msg("grpc stat request failed")
w.WriteHeader(http.StatusInternalServerError)
} else {
handleErrorStatus(&sublog, w, srcStatRes.Status)
}
return
}
Expand All @@ -172,7 +153,7 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {

err = s.descend(ctx, client, srcStatRes.Info, dst, depth == "infinity")
if err != nil {
log.Error().Err(err).Msg("error descending directory")
sublog.Error().Err(err).Str("depth", depth).Msg("error descending directory")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand Down
26 changes: 11 additions & 15 deletions internal/http/services/owncloud/ocdav/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,23 @@ import (
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
"github.com/cs3org/reva/pkg/appctx"
"go.opencensus.io/trace"
)

func (s *svc) handleDelete(w http.ResponseWriter, r *http.Request, ns string) {
ctx := r.Context()
log := appctx.GetLogger(ctx)
ctx, span := trace.StartSpan(ctx, "head")
defer span.End()

ns = applyLayout(ctx, ns)

fn := path.Join(ns, r.URL.Path)

sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()

client, err := s.getClient()
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand All @@ -48,22 +52,14 @@ func (s *svc) handleDelete(w http.ResponseWriter, r *http.Request, ns string) {
req := &provider.DeleteRequest{Ref: ref}
res, err := client.Delete(ctx, req)
if err != nil {
log.Error().Err(err).Msg("error performing delete grpc request")
sublog.Error().Err(err).Msg("error performing delete grpc request")
w.WriteHeader(http.StatusInternalServerError)
return
}

switch res.Status.Code {
case rpc.Code_CODE_OK:
w.WriteHeader(http.StatusNoContent)
case rpc.Code_CODE_NOT_FOUND:
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("resource not found")
w.WriteHeader(http.StatusNotFound)
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Str("path", fn).Interface("status", res.Status).Msg("grpc delete request failed")
w.WriteHeader(http.StatusInternalServerError)
if res.Status.Code != rpc.Code_CODE_OK {
handleErrorStatus(&sublog, w, res.Status)
return
}
w.WriteHeader(http.StatusNoContent)
}
27 changes: 27 additions & 0 deletions internal/http/services/owncloud/ocdav/error.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ package ocdav

import (
"encoding/xml"
"net/http"

rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
"github.com/rs/zerolog"
)

type code int
Expand Down Expand Up @@ -49,3 +53,26 @@ func Marshal(e exception) ([]byte, error) {
Message: e.message,
})
}

func handleErrorStatus(log *zerolog.Logger, w http.ResponseWriter, s *rpc.Status) {
switch s.Code {
case rpc.Code_CODE_OK:
log.Debug().Interface("status", s).Msg("ok")
w.WriteHeader(http.StatusOK)
case rpc.Code_CODE_NOT_FOUND:
log.Debug().Interface("status", s).Msg("resource not found")
w.WriteHeader(http.StatusNotFound)
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Interface("status", s).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
case rpc.Code_CODE_INVALID_ARGUMENT:
log.Debug().Interface("status", s).Msg("bad request")
w.WriteHeader(http.StatusBadRequest)
case rpc.Code_CODE_UNIMPLEMENTED:
log.Debug().Interface("status", s).Msg("not implemented")
w.WriteHeader(http.StatusNotImplemented)
default:
log.Error().Interface("status", s).Msg("grpc request failed")
w.WriteHeader(http.StatusInternalServerError)
}
}
44 changes: 14 additions & 30 deletions internal/http/services/owncloud/ocdav/get.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"time"

"github.com/cs3org/reva/internal/http/services/datagateway"
"go.opencensus.io/trace"

rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
Expand All @@ -36,15 +37,18 @@ import (

func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {
ctx := r.Context()
log := appctx.GetLogger(ctx)
ctx, span := trace.StartSpan(ctx, "head")
defer span.End()

ns = applyLayout(ctx, ns)

fn := path.Join(ns, r.URL.Path)

sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()

client, err := s.getClient()
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand All @@ -56,29 +60,19 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {
}
sRes, err := client.Stat(ctx, sReq)
if err != nil {
log.Error().Err(err).Msg("error sending grpc stat request")
sublog.Error().Err(err).Msg("error sending grpc stat request")
w.WriteHeader(http.StatusInternalServerError)
return
}

if sRes.Status.Code != rpc.Code_CODE_OK {
switch sRes.Status.Code {
case rpc.Code_CODE_NOT_FOUND:
log.Debug().Str("path", fn).Interface("status", sRes.Status).Msg("resource not found")
w.WriteHeader(http.StatusNotFound)
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Str("path", fn).Interface("status", sRes.Status).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Str("path", fn).Interface("status", sRes.Status).Msg("grpc stat request failed")
w.WriteHeader(http.StatusInternalServerError)
}
handleErrorStatus(&sublog, w, sRes.Status)
return
}

info := sRes.Info
if info.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER {
log.Warn().Msg("resource is a folder and cannot be downloaded")
sublog.Warn().Msg("resource is a folder and cannot be downloaded")
w.WriteHeader(http.StatusNotImplemented)
return
}
Expand All @@ -91,23 +85,13 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {

dRes, err := client.InitiateFileDownload(ctx, dReq)
if err != nil {
log.Error().Err(err).Msg("error initiating file download")
sublog.Error().Err(err).Msg("error initiating file download")
w.WriteHeader(http.StatusInternalServerError)
return
}

if dRes.Status.Code != rpc.Code_CODE_OK {
switch dRes.Status.Code {
case rpc.Code_CODE_NOT_FOUND:
log.Debug().Str("path", fn).Interface("status", dRes.Status).Msg("resource not found")
w.WriteHeader(http.StatusNotFound)
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Str("path", fn).Interface("status", dRes.Status).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Str("path", fn).Interface("status", dRes.Status).Msg("grpc initiate file download request failed")
w.WriteHeader(http.StatusInternalServerError)
}
handleErrorStatus(&sublog, w, dRes.Status)
return
}

Expand All @@ -120,7 +104,7 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {

httpReq, err := rhttp.NewRequest(ctx, "GET", ep, nil)
if err != nil {
log.Error().Err(err).Msg("error creating http request")
sublog.Error().Err(err).Msg("error creating http request")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand All @@ -129,7 +113,7 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {

httpRes, err := httpClient.Do(httpReq)
if err != nil {
log.Error().Err(err).Msg("error performing http request")
sublog.Error().Err(err).Msg("error performing http request")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand All @@ -156,6 +140,6 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {
}
*/
if _, err := io.Copy(w, httpRes.Body); err != nil {
log.Error().Err(err).Msg("error finishing copying data to response")
sublog.Error().Err(err).Msg("error finishing copying data to response")
}
}
22 changes: 8 additions & 14 deletions internal/http/services/owncloud/ocdav/head.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,23 @@ import (
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
"github.com/cs3org/reva/pkg/appctx"
"github.com/cs3org/reva/pkg/utils"
"go.opencensus.io/trace"
)

func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) {
ctx := r.Context()
log := appctx.GetLogger(ctx)
ctx, span := trace.StartSpan(ctx, "head")
defer span.End()

ns = applyLayout(ctx, ns)

fn := path.Join(ns, r.URL.Path)

sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()

client, err := s.getClient()
if err != nil {
log.Error().Err(err).Msg("error getting grpc client")
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand All @@ -51,23 +55,13 @@ func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) {
req := &provider.StatRequest{Ref: ref}
res, err := client.Stat(ctx, req)
if err != nil {
log.Error().Err(err).Msg("error sending grpc stat request")
sublog.Error().Err(err).Msg("error sending grpc stat request")
w.WriteHeader(http.StatusInternalServerError)
return
}

if res.Status.Code != rpc.Code_CODE_OK {
switch res.Status.Code {
case rpc.Code_CODE_NOT_FOUND:
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("resource not found")
w.WriteHeader(http.StatusNotFound)
case rpc.Code_CODE_PERMISSION_DENIED:
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("permission denied")
w.WriteHeader(http.StatusForbidden)
default:
log.Error().Str("path", fn).Interface("status", res.Status).Msg("grpc stat request failed")
w.WriteHeader(http.StatusInternalServerError)
}
handleErrorStatus(&sublog, w, res.Status)
return
}

Expand Down
Loading

0 comments on commit 783e35c

Please sign in to comment.