Skip to content

Commit f374ded

Browse files
butonicbutonic
committed
OCDAV: map bad request and unimplemented codes
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
1 parent fdc6df0 commit f374ded

File tree

15 files changed

+267
-518
lines changed

15 files changed

+267
-518
lines changed

Diff for: ‎changelog/unreleased/ocdav-handle-bad-request.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Enhancement: Map bad request and unimplement to http status codes
2+
3+
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.
4+
5+
https://github.com/cs3org/reva/pull/1354

Diff for: ‎internal/http/services/owncloud/ocdav/copy.go

+17-36
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,13 @@ import (
3232
"github.com/cs3org/reva/internal/http/services/datagateway"
3333
"github.com/cs3org/reva/pkg/appctx"
3434
"github.com/cs3org/reva/pkg/rhttp"
35+
"go.opencensus.io/trace"
3536
)
3637

3738
func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
3839
ctx := r.Context()
39-
log := appctx.GetLogger(ctx)
40+
ctx, span := trace.StartSpan(ctx, "head")
41+
defer span.End()
4042

4143
ns = applyLayout(ctx, ns)
4244

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

58-
log.Info().Str("source", src).Str("destination", dst).
59-
Str("overwrite", overwrite).Str("depth", depth).Msg("copy")
60+
sublog := appctx.GetLogger(ctx).With().Str("src", src).Str("dst", dst).Logger()
61+
sublog.Debug().Str("overwrite", overwrite).Str("depth", depth).Msg("copy")
6062

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

7678
client, err := s.getClient()
7779
if err != nil {
78-
log.Error().Err(err).Msg("error getting grpc client")
80+
sublog.Error().Err(err).Msg("error getting grpc client")
7981
w.WriteHeader(http.StatusInternalServerError)
8082
return
8183
}
@@ -87,23 +89,13 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
8789
srcStatReq := &provider.StatRequest{Ref: ref}
8890
srcStatRes, err := client.Stat(ctx, srcStatReq)
8991
if err != nil {
90-
log.Error().Err(err).Msg("error sending grpc stat request")
92+
sublog.Error().Err(err).Msg("error sending grpc stat request")
9193
w.WriteHeader(http.StatusInternalServerError)
9294
return
9395
}
9496

9597
if srcStatRes.Status.Code != rpc.Code_CODE_OK {
96-
switch srcStatRes.Status.Code {
97-
case rpc.Code_CODE_NOT_FOUND:
98-
log.Debug().Str("src", src).Interface("status", srcStatRes.Status).Msg("resource not found")
99-
w.WriteHeader(http.StatusNotFound)
100-
case rpc.Code_CODE_PERMISSION_DENIED:
101-
log.Debug().Str("src", src).Interface("status", srcStatRes.Status).Msg("permission denied")
102-
w.WriteHeader(http.StatusForbidden)
103-
default:
104-
log.Error().Str("src", src).Interface("status", srcStatRes.Status).Msg("grpc stat request failed")
105-
w.WriteHeader(http.StatusInternalServerError)
106-
}
98+
handleErrorStatus(&sublog, w, srcStatRes.Status)
10799
return
108100
}
109101

@@ -114,19 +106,12 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
114106
dstStatReq := &provider.StatRequest{Ref: ref}
115107
dstStatRes, err := client.Stat(ctx, dstStatReq)
116108
if err != nil {
117-
log.Error().Err(err).Msg("error sending grpc stat request")
109+
sublog.Error().Err(err).Msg("error sending grpc stat request")
118110
w.WriteHeader(http.StatusInternalServerError)
119111
return
120112
}
121113
if dstStatRes.Status.Code != rpc.Code_CODE_OK && dstStatRes.Status.Code != rpc.Code_CODE_NOT_FOUND {
122-
switch dstStatRes.Status.Code {
123-
case rpc.Code_CODE_PERMISSION_DENIED:
124-
log.Debug().Str("dst", dst).Interface("status", dstStatRes.Status).Msg("permission denied")
125-
w.WriteHeader(http.StatusForbidden)
126-
default:
127-
log.Error().Str("dst", dst).Interface("status", dstStatRes.Status).Msg("grpc stat request failed")
128-
w.WriteHeader(http.StatusInternalServerError)
129-
}
114+
handleErrorStatus(&sublog, w, srcStatRes.Status)
130115
return
131116
}
132117

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

137122
if overwrite == "F" {
138-
log.Warn().Str("dst", dst).Msg("dst already exists")
123+
sublog.Warn().Str("overwrite", overwrite).Msg("dst already exists")
139124
w.WriteHeader(http.StatusPreconditionFailed) // 412, see https://tools.ietf.org/html/rfc4918#section-9.8.5
140125
return
141126
}
@@ -149,21 +134,17 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
149134
intStatReq := &provider.StatRequest{Ref: ref}
150135
intStatRes, err := client.Stat(ctx, intStatReq)
151136
if err != nil {
152-
log.Error().Err(err).Msg("error sending grpc stat request")
137+
sublog.Error().Err(err).Msg("error sending grpc stat request")
153138
w.WriteHeader(http.StatusInternalServerError)
154139
return
155140
}
156141
if intStatRes.Status.Code != rpc.Code_CODE_OK {
157-
switch intStatRes.Status.Code {
158-
case rpc.Code_CODE_NOT_FOUND:
142+
if intStatRes.Status.Code == rpc.Code_CODE_NOT_FOUND {
159143
// 409 if intermediate dir is missing, see https://tools.ietf.org/html/rfc4918#section-9.8.5
144+
sublog.Debug().Str("parent", intermediateDir).Interface("status", intStatRes.Status).Msg("conflict")
160145
w.WriteHeader(http.StatusConflict)
161-
case rpc.Code_CODE_PERMISSION_DENIED:
162-
log.Debug().Str("dst", dst).Interface("status", intStatRes.Status).Msg("permission denied")
163-
w.WriteHeader(http.StatusForbidden)
164-
default:
165-
log.Error().Str("dst", dst).Interface("status", intStatRes.Status).Msg("grpc stat request failed")
166-
w.WriteHeader(http.StatusInternalServerError)
146+
} else {
147+
handleErrorStatus(&sublog, w, srcStatRes.Status)
167148
}
168149
return
169150
}
@@ -172,7 +153,7 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) {
172153

173154
err = s.descend(ctx, client, srcStatRes.Info, dst, depth == "infinity")
174155
if err != nil {
175-
log.Error().Err(err).Msg("error descending directory")
156+
sublog.Error().Err(err).Str("depth", depth).Msg("error descending directory")
176157
w.WriteHeader(http.StatusInternalServerError)
177158
return
178159
}

Diff for: ‎internal/http/services/owncloud/ocdav/delete.go

+11-10
Original file line numberDiff line numberDiff line change
@@ -25,19 +25,24 @@ import (
2525
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
2626
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
2727
"github.com/cs3org/reva/pkg/appctx"
28+
"github.com/rs/zerolog/log"
29+
"go.opencensus.io/trace"
2830
)
2931

3032
func (s *svc) handleDelete(w http.ResponseWriter, r *http.Request, ns string) {
3133
ctx := r.Context()
32-
log := appctx.GetLogger(ctx)
34+
ctx, span := trace.StartSpan(ctx, "head")
35+
defer span.End()
3336

3437
ns = applyLayout(ctx, ns)
3538

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

41+
sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
42+
3843
client, err := s.getClient()
3944
if err != nil {
40-
log.Error().Err(err).Msg("error getting grpc client")
45+
sublog.Error().Err(err).Msg("error getting grpc client")
4146
w.WriteHeader(http.StatusInternalServerError)
4247
return
4348
}
@@ -48,7 +53,7 @@ func (s *svc) handleDelete(w http.ResponseWriter, r *http.Request, ns string) {
4853
req := &provider.DeleteRequest{Ref: ref}
4954
res, err := client.Delete(ctx, req)
5055
if err != nil {
51-
log.Error().Err(err).Msg("error performing delete grpc request")
56+
sublog.Error().Err(err).Msg("error performing delete grpc request")
5257
w.WriteHeader(http.StatusInternalServerError)
5358
return
5459
}
@@ -57,13 +62,9 @@ func (s *svc) handleDelete(w http.ResponseWriter, r *http.Request, ns string) {
5762
case rpc.Code_CODE_OK:
5863
w.WriteHeader(http.StatusNoContent)
5964
case rpc.Code_CODE_NOT_FOUND:
60-
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("resource not found")
61-
w.WriteHeader(http.StatusNotFound)
62-
case rpc.Code_CODE_PERMISSION_DENIED:
63-
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("permission denied")
64-
w.WriteHeader(http.StatusForbidden)
65+
log.Debug().Str("storageid", sRes.Info.Id.StorageId).Str("key", key).Interface("status", res.Status).Msg("resource not found")
66+
w.WriteHeader(http.StatusConflict)
6567
default:
66-
log.Error().Str("path", fn).Interface("status", res.Status).Msg("grpc delete request failed")
67-
w.WriteHeader(http.StatusInternalServerError)
68+
handleErrorStatus(&sublog, w, res.Status)
6869
}
6970
}

Diff for: ‎internal/http/services/owncloud/ocdav/error.go

+27
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ package ocdav
2020

2121
import (
2222
"encoding/xml"
23+
"net/http"
24+
25+
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
26+
"github.com/rs/zerolog"
2327
)
2428

2529
type code int
@@ -49,3 +53,26 @@ func Marshal(e exception) ([]byte, error) {
4953
Message: e.message,
5054
})
5155
}
56+
57+
func handleErrorStatus(log *zerolog.Logger, w http.ResponseWriter, s *rpc.Status) {
58+
switch s.Code {
59+
case rpc.Code_CODE_OK:
60+
log.Debug().Interface("status", s).Msg("ok")
61+
w.WriteHeader(http.StatusOK)
62+
case rpc.Code_CODE_NOT_FOUND:
63+
log.Debug().Interface("status", s).Msg("resource not found")
64+
w.WriteHeader(http.StatusNotFound)
65+
case rpc.Code_CODE_PERMISSION_DENIED:
66+
log.Debug().Interface("status", s).Msg("permission denied")
67+
w.WriteHeader(http.StatusForbidden)
68+
case rpc.Code_CODE_INVALID_ARGUMENT:
69+
log.Debug().Interface("status", s).Msg("bad request")
70+
w.WriteHeader(http.StatusBadRequest)
71+
case rpc.Code_CODE_UNIMPLEMENTED:
72+
log.Debug().Interface("status", s).Msg("not implemented")
73+
w.WriteHeader(http.StatusNotImplemented)
74+
default:
75+
log.Error().Interface("status", s).Msg("grpc request failed")
76+
w.WriteHeader(http.StatusInternalServerError)
77+
}
78+
}

Diff for: ‎internal/http/services/owncloud/ocdav/get.go

+14-30
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"time"
2727

2828
"github.com/cs3org/reva/internal/http/services/datagateway"
29+
"go.opencensus.io/trace"
2930

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

3738
func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {
3839
ctx := r.Context()
39-
log := appctx.GetLogger(ctx)
40+
ctx, span := trace.StartSpan(ctx, "head")
41+
defer span.End()
4042

4143
ns = applyLayout(ctx, ns)
4244

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

47+
sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
48+
4549
client, err := s.getClient()
4650
if err != nil {
47-
log.Error().Err(err).Msg("error getting grpc client")
51+
sublog.Error().Err(err).Msg("error getting grpc client")
4852
w.WriteHeader(http.StatusInternalServerError)
4953
return
5054
}
@@ -56,29 +60,19 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {
5660
}
5761
sRes, err := client.Stat(ctx, sReq)
5862
if err != nil {
59-
log.Error().Err(err).Msg("error sending grpc stat request")
63+
sublog.Error().Err(err).Msg("error sending grpc stat request")
6064
w.WriteHeader(http.StatusInternalServerError)
6165
return
6266
}
6367

6468
if sRes.Status.Code != rpc.Code_CODE_OK {
65-
switch sRes.Status.Code {
66-
case rpc.Code_CODE_NOT_FOUND:
67-
log.Debug().Str("path", fn).Interface("status", sRes.Status).Msg("resource not found")
68-
w.WriteHeader(http.StatusNotFound)
69-
case rpc.Code_CODE_PERMISSION_DENIED:
70-
log.Debug().Str("path", fn).Interface("status", sRes.Status).Msg("permission denied")
71-
w.WriteHeader(http.StatusForbidden)
72-
default:
73-
log.Error().Str("path", fn).Interface("status", sRes.Status).Msg("grpc stat request failed")
74-
w.WriteHeader(http.StatusInternalServerError)
75-
}
69+
handleErrorStatus(&sublog, w, sRes.Status)
7670
return
7771
}
7872

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

9286
dRes, err := client.InitiateFileDownload(ctx, dReq)
9387
if err != nil {
94-
log.Error().Err(err).Msg("error initiating file download")
88+
sublog.Error().Err(err).Msg("error initiating file download")
9589
w.WriteHeader(http.StatusInternalServerError)
9690
return
9791
}
9892

9993
if dRes.Status.Code != rpc.Code_CODE_OK {
100-
switch dRes.Status.Code {
101-
case rpc.Code_CODE_NOT_FOUND:
102-
log.Debug().Str("path", fn).Interface("status", dRes.Status).Msg("resource not found")
103-
w.WriteHeader(http.StatusNotFound)
104-
case rpc.Code_CODE_PERMISSION_DENIED:
105-
log.Debug().Str("path", fn).Interface("status", dRes.Status).Msg("permission denied")
106-
w.WriteHeader(http.StatusForbidden)
107-
default:
108-
log.Error().Str("path", fn).Interface("status", dRes.Status).Msg("grpc initiate file download request failed")
109-
w.WriteHeader(http.StatusInternalServerError)
110-
}
94+
handleErrorStatus(&sublog, w, dRes.Status)
11195
return
11296
}
11397

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

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

130114
httpRes, err := httpClient.Do(httpReq)
131115
if err != nil {
132-
log.Error().Err(err).Msg("error performing http request")
116+
sublog.Error().Err(err).Msg("error performing http request")
133117
w.WriteHeader(http.StatusInternalServerError)
134118
return
135119
}
@@ -156,6 +140,6 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {
156140
}
157141
*/
158142
if _, err := io.Copy(w, httpRes.Body); err != nil {
159-
log.Error().Err(err).Msg("error finishing copying data to response")
143+
sublog.Error().Err(err).Msg("error finishing copying data to response")
160144
}
161145
}

Diff for: ‎internal/http/services/owncloud/ocdav/head.go

+8-14
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,23 @@ import (
2828
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
2929
"github.com/cs3org/reva/pkg/appctx"
3030
"github.com/cs3org/reva/pkg/utils"
31+
"go.opencensus.io/trace"
3132
)
3233

3334
func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) {
3435
ctx := r.Context()
35-
log := appctx.GetLogger(ctx)
36+
ctx, span := trace.StartSpan(ctx, "head")
37+
defer span.End()
3638

3739
ns = applyLayout(ctx, ns)
3840

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

43+
sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
44+
4145
client, err := s.getClient()
4246
if err != nil {
43-
log.Error().Err(err).Msg("error getting grpc client")
47+
sublog.Error().Err(err).Msg("error getting grpc client")
4448
w.WriteHeader(http.StatusInternalServerError)
4549
return
4650
}
@@ -51,23 +55,13 @@ func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) {
5155
req := &provider.StatRequest{Ref: ref}
5256
res, err := client.Stat(ctx, req)
5357
if err != nil {
54-
log.Error().Err(err).Msg("error sending grpc stat request")
58+
sublog.Error().Err(err).Msg("error sending grpc stat request")
5559
w.WriteHeader(http.StatusInternalServerError)
5660
return
5761
}
5862

5963
if res.Status.Code != rpc.Code_CODE_OK {
60-
switch res.Status.Code {
61-
case rpc.Code_CODE_NOT_FOUND:
62-
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("resource not found")
63-
w.WriteHeader(http.StatusNotFound)
64-
case rpc.Code_CODE_PERMISSION_DENIED:
65-
log.Debug().Str("path", fn).Interface("status", res.Status).Msg("permission denied")
66-
w.WriteHeader(http.StatusForbidden)
67-
default:
68-
log.Error().Str("path", fn).Interface("status", res.Status).Msg("grpc stat request failed")
69-
w.WriteHeader(http.StatusInternalServerError)
70-
}
64+
handleErrorStatus(&sublog, w, res.Status)
7165
return
7266
}
7367

0 commit comments

Comments
 (0)