Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OCDAV: map bad request and unimplemented codes #1354

Merged
merged 1 commit into from
Dec 3, 2020
Merged

OCDAV: map bad request and unimplemented codes #1354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/unreleased/ocdav-handle-bad-request.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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