simplify gateway client usage

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
cs3org · Nov 10, 2022 · 6463144 · 6463144
1 parent d2df5a9
commit 6463144
Show file tree

Hide file tree

Showing 18 changed files with 75 additions and 288 deletions.
diff --git a/changelog/unreleased/dav-unit-tests.md b/changelog/unreleased/dav-unit-tests.md
@@ -3,4 +3,5 @@ Enhancement: Cover ocdav with more unit tests
 We added unit tests to cover more ocdav handlers:
 -   delete
 
-https://github.com/cs3org/reva/pull/3441
+https://github.com/cs3org/reva/pull/3441
+https://github.com/cs3org/reva/pull/3443
diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go
@@ -90,14 +90,7 @@ func (s *svc) handlePathCopy(w http.ResponseWriter, r *http.Request, ns string)
 
 	sublog := appctx.GetLogger(ctx).With().Str("src", src).Str("dst", dst).Logger()
 
-	client, err := s.getClient()
-	if err != nil {
-		sublog.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
-
-	srcSpace, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, src)
+	srcSpace, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, s.gwClient, src)
 	if err != nil {
 		sublog.Error().Err(err).Str("path", src).Msg("failed to look up storage space")
 		w.WriteHeader(http.StatusInternalServerError)
@@ -107,7 +100,7 @@ func (s *svc) handlePathCopy(w http.ResponseWriter, r *http.Request, ns string)
 		errors.HandleErrorStatus(&sublog, w, status)
 		return
 	}
-	dstSpace, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, dst)
+	dstSpace, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, s.gwClient, dst)
 	if err != nil {
 		sublog.Error().Err(err).Str("path", dst).Msg("failed to look up storage space")
 		w.WriteHeader(http.StatusInternalServerError)
@@ -123,7 +116,7 @@ func (s *svc) handlePathCopy(w http.ResponseWriter, r *http.Request, ns string)
 		return
 	}
 
-	if err := s.executePathCopy(ctx, client, w, r, cp); err != nil {
+	if err := s.executePathCopy(ctx, s.gwClient, w, r, cp); err != nil {
 		sublog.Error().Err(err).Str("depth", cp.depth.String()).Msg("error executing path copy")
 		w.WriteHeader(http.StatusInternalServerError)
 	}
@@ -304,13 +297,6 @@ func (s *svc) handleSpacesCopy(w http.ResponseWriter, r *http.Request, spaceID s
 
 	sublog := appctx.GetLogger(ctx).With().Str("spaceid", spaceID).Str("path", r.URL.Path).Str("destination", dst).Logger()
 
-	client, err := s.getClient()
-	if err != nil {
-		sublog.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
-
 	srcRef, err := spacelookup.MakeStorageSpaceReference(spaceID, r.URL.Path)
 	if err != nil {
 		w.WriteHeader(http.StatusBadRequest)
@@ -330,7 +316,7 @@ func (s *svc) handleSpacesCopy(w http.ResponseWriter, r *http.Request, spaceID s
 		return
 	}
 
-	err = s.executeSpacesCopy(ctx, w, client, cp)
+	err = s.executeSpacesCopy(ctx, w, s.gwClient, cp)
 	if err != nil {
 		sublog.Error().Err(err).Str("depth", cp.depth.String()).Msg("error descending directory")
 		w.WriteHeader(http.StatusInternalServerError)
@@ -487,14 +473,7 @@ func (s *svc) executeSpacesCopy(ctx context.Context, w http.ResponseWriter, clie
 }
 
 func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Request, srcRef, dstRef *provider.Reference, log *zerolog.Logger) *copy {
-	client, err := s.getClient()
-	if err != nil {
-		log.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return nil
-	}
-
-	isChild, err := s.referenceIsChildOf(ctx, client, dstRef, srcRef)
+	isChild, err := s.referenceIsChildOf(ctx, s.gwClient, dstRef, srcRef)
 	if err != nil {
 		switch err.(type) {
 		case errtypes.IsNotSupported:
@@ -540,7 +519,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
 	log.Debug().Bool("overwrite", overwrite).Str("depth", depth.String()).Msg("copy")
 
 	srcStatReq := &provider.StatRequest{Ref: srcRef}
-	srcStatRes, err := client.Stat(ctx, srcStatReq)
+	srcStatRes, err := s.gwClient.Stat(ctx, srcStatReq)
 	switch {
 	case err != nil:
 		log.Error().Err(err).Msg("error sending grpc stat request")
@@ -558,7 +537,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
 	}
 
 	dstStatReq := &provider.StatRequest{Ref: dstRef}
-	dstStatRes, err := client.Stat(ctx, dstStatReq)
+	dstStatRes, err := s.gwClient.Stat(ctx, dstStatReq)
 	switch {
 	case err != nil:
 		log.Error().Err(err).Msg("error sending grpc stat request")
@@ -584,7 +563,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
 
 		// delete existing tree
 		delReq := &provider.DeleteRequest{Ref: dstRef}
-		delRes, err := client.Delete(ctx, delReq)
+		delRes, err := s.gwClient.Delete(ctx, delReq)
 		if err != nil {
 			log.Error().Err(err).Msg("error sending grpc delete request")
 			w.WriteHeader(http.StatusInternalServerError)
@@ -602,7 +581,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
 			Path:       utils.MakeRelativePath(p),
 		}
 		intStatReq := &provider.StatRequest{Ref: pRef}
-		intStatRes, err := client.Stat(ctx, intStatReq)
+		intStatRes, err := s.gwClient.Stat(ctx, intStatReq)
 		if err != nil {
 			log.Error().Err(err).Msg("error sending grpc stat request")
 			w.WriteHeader(http.StatusInternalServerError)

diff --git a/internal/http/services/owncloud/ocdav/dav.go b/internal/http/services/owncloud/ocdav/dav.go
@@ -181,18 +181,14 @@ func (h *DavHandler) Handler(s *svc) http.Handler {
 		case "public-files":
 			base := path.Join(ctx.Value(net.CtxKeyBaseURI).(string), "public-files")
 			ctx = context.WithValue(ctx, net.CtxKeyBaseURI, base)
-			c, err := s.getClient()
-			if err != nil {
-				w.WriteHeader(http.StatusNotFound)
-				return
-			}
 
 			var res *gatewayv1beta1.AuthenticateResponse
 			token, _ := router.ShiftPath(r.URL.Path)
 			var hasValidBasicAuthHeader bool
 			var pass string
+			var err error
 			if _, pass, hasValidBasicAuthHeader = r.BasicAuth(); hasValidBasicAuthHeader {
-				res, err = handleBasicAuth(r.Context(), c, token, pass)
+				res, err = handleBasicAuth(r.Context(), s.gwClient, token, pass)
 			} else {
 				q := r.URL.Query()
 				sig := q.Get("signature")
@@ -202,7 +198,7 @@ func (h *DavHandler) Handler(s *svc) http.Handler {
 					w.WriteHeader(http.StatusUnauthorized)
 					return
 				}
-				res, err = handleSignatureAuth(r.Context(), c, token, sig, expiration)
+				res, err = handleSignatureAuth(r.Context(), s.gwClient, token, sig, expiration)
 			}
 
 			switch {
@@ -236,7 +232,7 @@ func (h *DavHandler) Handler(s *svc) http.Handler {
 			r = r.WithContext(ctx)
 
 			// the public share manager knew the token, but does the referenced target still exist?
-			sRes, err := getTokenStatInfo(ctx, c, token)
+			sRes, err := getTokenStatInfo(ctx, s.gwClient, token)
 			switch {
 			case err != nil:
 				log.Error().Err(err).Msg("error sending grpc stat request")

diff --git a/internal/http/services/owncloud/ocdav/delete.go b/internal/http/services/owncloud/ocdav/delete.go
@@ -40,13 +40,7 @@ func (s *svc) handlePathDelete(w http.ResponseWriter, r *http.Request, ns string
 
 	fn := path.Join(ns, r.URL.Path)
 
-	client, err := s.getClient()
-	if err != nil {
-		span.RecordError(err)
-		return http.StatusInternalServerError, err
-	}
-
-	space, rpcStatus, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, fn)
+	space, rpcStatus, err := spacelookup.LookUpStorageSpaceForPath(ctx, s.gwClient, fn)
 	switch {
 	case err != nil:
 		span.RecordError(err)
@@ -74,13 +68,7 @@ func (s *svc) handleDelete(ctx context.Context, w http.ResponseWriter, r *http.R
 		return http.StatusBadRequest, errtypes.BadRequest("invalid if header")
 	}
 
-	client, err := s.getClient()
-	if err != nil {
-		span.RecordError(err)
-		return http.StatusInternalServerError, err
-	}
-
-	res, err := client.Delete(ctx, req)
+	res, err := s.gwClient.Delete(ctx, req)
 	switch {
 	case err != nil:
 		span.RecordError(err)
@@ -98,7 +86,7 @@ func (s *svc) handleDelete(ctx context.Context, w http.ResponseWriter, r *http.R
 			status = http.StatusLocked
 		}
 		// check if user has access to resource
-		sRes, err := client.Stat(ctx, &provider.StatRequest{Ref: ref})
+		sRes, err := s.gwClient.Stat(ctx, &provider.StatRequest{Ref: ref})
 		if err != nil {
 			span.RecordError(err)
 			return http.StatusInternalServerError, err

diff --git a/internal/http/services/owncloud/ocdav/get.go b/internal/http/services/owncloud/ocdav/get.go
@@ -44,13 +44,7 @@ func (s *svc) handlePathGet(w http.ResponseWriter, r *http.Request, ns string) {
 
 	sublog := appctx.GetLogger(ctx).With().Str("path", fn).Str("svc", "ocdav").Str("handler", "get").Logger()
 
-	client, err := s.getClient()
-	if err != nil {
-		sublog.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
-	space, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, fn)
+	space, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, s.gwClient, fn)
 	if err != nil {
 		sublog.Error().Err(err).Str("path", fn).Msg("failed to look up storage space")
 		w.WriteHeader(http.StatusInternalServerError)
@@ -65,17 +59,10 @@ func (s *svc) handlePathGet(w http.ResponseWriter, r *http.Request, ns string) {
 }
 
 func (s *svc) handleGet(ctx context.Context, w http.ResponseWriter, r *http.Request, ref *provider.Reference, dlProtocol string, log zerolog.Logger) {
-	client, err := s.getClient()
-	if err != nil {
-		log.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
-
 	sReq := &provider.StatRequest{
 		Ref: ref,
 	}
-	sRes, err := client.Stat(ctx, sReq)
+	sRes, err := s.gwClient.Stat(ctx, sReq)
 	if err != nil {
 		log.Error().Err(err).Msg("error stat resource")
 		w.WriteHeader(http.StatusInternalServerError)
@@ -92,7 +79,7 @@ func (s *svc) handleGet(ctx context.Context, w http.ResponseWriter, r *http.Requ
 	}
 
 	dReq := &provider.InitiateFileDownloadRequest{Ref: ref}
-	dRes, err := client.InitiateFileDownload(ctx, dReq)
+	dRes, err := s.gwClient.InitiateFileDownload(ctx, dReq)
 	switch {
 	case err != nil:
 		log.Error().Err(err).Msg("error initiating file download")

diff --git a/internal/http/services/owncloud/ocdav/head.go b/internal/http/services/owncloud/ocdav/head.go
@@ -47,14 +47,8 @@ func (s *svc) handlePathHead(w http.ResponseWriter, r *http.Request, ns string)
 	fn := path.Join(ns, r.URL.Path)
 
 	sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
-	client, err := s.getClient()
-	if err != nil {
-		sublog.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
 
-	space, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, fn)
+	space, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, s.gwClient, fn)
 	if err != nil {
 		sublog.Error().Err(err).Str("path", fn).Msg("failed to look up storage space")
 		w.WriteHeader(http.StatusInternalServerError)
@@ -69,15 +63,9 @@ func (s *svc) handlePathHead(w http.ResponseWriter, r *http.Request, ns string)
 }
 
 func (s *svc) handleHead(ctx context.Context, w http.ResponseWriter, r *http.Request, ref *provider.Reference, log zerolog.Logger) {
-	client, err := s.getClient()
-	if err != nil {
-		log.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
 
 	req := &provider.StatRequest{Ref: ref}
-	res, err := client.Stat(ctx, req)
+	res, err := s.gwClient.Stat(ctx, req)
 	if err != nil {
 		log.Error().Err(err).Msg("error sending grpc stat request")
 		w.WriteHeader(http.StatusInternalServerError)

diff --git a/internal/http/services/owncloud/ocdav/locks.go b/internal/http/services/owncloud/ocdav/locks.go
@@ -387,13 +387,8 @@ func (s *svc) handleLock(w http.ResponseWriter, r *http.Request, ns string) (ret
 
 	fn := path.Join(ns, r.URL.Path) // TODO do we still need to jail if we query the registry about the spaces?
 
-	client, err := s.getClient()
-	if err != nil {
-		return http.StatusInternalServerError, err
-	}
-
 	// TODO instead of using a string namespace ns pass in the space with the request?
-	ref, cs3Status, err := spacelookup.LookupReferenceForPath(ctx, client, fn)
+	ref, cs3Status, err := spacelookup.LookupReferenceForPath(ctx, s.gwClient, fn)
 	if err != nil {
 		return http.StatusInternalServerError, err
 	}
@@ -570,13 +565,8 @@ func (s *svc) handleUnlock(w http.ResponseWriter, r *http.Request, ns string) (s
 
 	fn := path.Join(ns, r.URL.Path) // TODO do we still need to jail if we query the registry about the spaces?
 
-	client, err := s.getClient()
-	if err != nil {
-		return http.StatusInternalServerError, err
-	}
-
 	// TODO instead of using a string namespace ns pass in the space with the request?
-	ref, cs3Status, err := spacelookup.LookupReferenceForPath(ctx, client, fn)
+	ref, cs3Status, err := spacelookup.LookupReferenceForPath(ctx, s.gwClient, fn)
 	if err != nil {
 		return http.StatusInternalServerError, err
 	}

diff --git a/internal/http/services/owncloud/ocdav/meta.go b/internal/http/services/owncloud/ocdav/meta.go
@@ -96,12 +96,6 @@ func (h *MetaHandler) handlePathForUser(w http.ResponseWriter, r *http.Request,
 	id := storagespace.FormatResourceID(*rid)
 	sublog := appctx.GetLogger(ctx).With().Str("path", r.URL.Path).Str("resourceid", id).Logger()
 	sublog.Info().Msg("calling get path for user")
-	client, err := s.getClient()
-	if err != nil {
-		sublog.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
 
 	pf, status, err := propfind.ReadPropfind(r.Body)
 	if err != nil {
@@ -117,7 +111,7 @@ func (h *MetaHandler) handlePathForUser(w http.ResponseWriter, r *http.Request,
 	}
 
 	pathReq := &provider.GetPathRequest{ResourceId: rid}
-	pathRes, err := client.GetPath(ctx, pathReq)
+	pathRes, err := s.gwClient.GetPath(ctx, pathReq)
 	if err != nil {
 		sublog.Error().Err(err).Msg("could not send GetPath grpc request: transport error")
 		w.WriteHeader(http.StatusInternalServerError)

diff --git a/internal/http/services/owncloud/ocdav/mkcol.go b/internal/http/services/owncloud/ocdav/mkcol.go
@@ -45,14 +45,10 @@ func (s *svc) handlePathMkcol(w http.ResponseWriter, r *http.Request, ns string)
 		}
 	}
 	sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
-	client, err := s.getClient()
-	if err != nil {
-		return http.StatusInternalServerError, err
-	}
 
 	// stat requested path to make sure it isn't existing yet
 	// NOTE: It could be on another storage provider than the 'parent' of it
-	sr, err := client.Stat(ctx, &provider.StatRequest{
+	sr, err := s.gwClient.Stat(ctx, &provider.StatRequest{
 		Ref: &provider.Reference{
 			Path: fn,
 		},
@@ -72,7 +68,7 @@ func (s *svc) handlePathMkcol(w http.ResponseWriter, r *http.Request, ns string)
 
 	parentPath := path.Dir(fn)
 
-	space, rpcStatus, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, parentPath)
+	space, rpcStatus, err := spacelookup.LookUpStorageSpaceForPath(ctx, s.gwClient, parentPath)
 	switch {
 	case err != nil:
 		return http.StatusInternalServerError, err
@@ -113,13 +109,8 @@ func (s *svc) handleMkcol(ctx context.Context, w http.ResponseWriter, r *http.Re
 		return http.StatusUnsupportedMediaType, fmt.Errorf("extended-mkcol not supported")
 	}
 
-	client, err := s.getClient()
-	if err != nil {
-		return http.StatusInternalServerError, err
-	}
-
 	req := &provider.CreateContainerRequest{Ref: childRef}
-	res, err := client.CreateContainer(ctx, req)
+	res, err := s.gwClient.CreateContainer(ctx, req)
 	switch {
 	case err != nil:
 		return http.StatusInternalServerError, err
@@ -128,7 +119,7 @@ func (s *svc) handleMkcol(ctx context.Context, w http.ResponseWriter, r *http.Re
 		return 0, nil
 	case res.Status.Code == rpc.Code_CODE_PERMISSION_DENIED:
 		// check if user has access to parent
-		sRes, err := client.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{
+		sRes, err := s.gwClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{
 			ResourceId: childRef.GetResourceId(),
 			Path:       utils.MakeRelativePath(path.Dir(childRef.Path)),
 		}})