Refactored the OpenFileInAppProvider workflow

This included changing the gRPC protocol and removing storageID and UIURL from config. En passant, this fixes cs3org#991 and includes other minor changes concerning error handling and logging.
glpatcern · Aug 4, 2020 · 90b0ba2 · 90b0ba2
1 parent 83c5528
commit 90b0ba2
Show file tree

Hide file tree

Showing 6 changed files with 40 additions and 77 deletions.
diff --git a/cmd/reva/open-file-in-app-provider.go b/cmd/reva/open-file-in-app-provider.go
@@ -25,8 +25,6 @@ import (
 	providerpb "github.com/cs3org/go-cs3apis/cs3/app/provider/v1beta1"
 	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
-	tokenpkg "github.com/cs3org/reva/pkg/token"
-	"github.com/pkg/errors"
 )
 
 func openFileInAppProviderCommand() *command {
@@ -55,13 +53,8 @@ func openFileInAppProviderCommand() *command {
 		ref := &provider.Reference{
 			Spec: &provider.Reference_Path{Path: path},
 		}
-		accessToken, ok := tokenpkg.ContextGetToken(ctx)
-		if !ok || accessToken == "" {
-			err := errors.New("Access token is invalid or empty")
-			return err
-		}
 
-		openRequest := &providerpb.OpenFileInAppProviderRequest{Ref: ref, AccessToken: accessToken, ViewMode: viewMode}
+		openRequest := &providerpb.OpenFileInAppProviderRequest{Ref: ref, ViewMode: viewMode}
 
 		openRes, err := client.OpenFileInAppProvider(ctx, openRequest)
 		if err != nil {

diff --git a/docs/content/en/docs/config/grpc/services/appprovider/_index.md b/docs/content/en/docs/config/grpc/services/appprovider/_index.md
@@ -9,34 +9,18 @@ description: >
 # _struct: config_
 
 {{% dir name="iopsecret" type="string" default="" %}}
-The iopsecret used to connect to the wopiserver. [[Ref]](https://github.com/cs3org/reva/tree/master/internal/grpc/services/appprovider/appprovider.go#L59)
+The iopsecret used to connect to the wopiserver. [[Ref]](https://github.com/cs3org/reva/tree/master/internal/grpc/services/appprovider/appprovider.go#L57)
 {{< highlight toml >}}
 [grpc.services.appprovider]
 iopsecret = ""
 {{< /highlight >}}
 {{% /dir %}}
 
 {{% dir name="wopiurl" type="string" default="" %}}
-The wopiserver's url. [[Ref]](https://github.com/cs3org/reva/tree/master/internal/grpc/services/appprovider/appprovider.go#L60)
+The wopiserver's URL. [[Ref]](https://github.com/cs3org/reva/tree/master/internal/grpc/services/appprovider/appprovider.go#L58)
 {{< highlight toml >}}
 [grpc.services.appprovider]
 wopiurl = ""
 {{< /highlight >}}
 {{% /dir %}}
 
-{{% dir name="uirul" type="string" default="" %}}
-URL to application (eg collabora) URL. [[Ref]](https://github.com/cs3org/reva/tree/master/internal/grpc/services/appprovider/appprovider.go#L61)
-{{< highlight toml >}}
-[grpc.services.appprovider]
-uirul = ""
-{{< /highlight >}}
-{{% /dir %}}
-
-{{% dir name="storageid" type="string" default="" %}}
-The storage id used by the wopiserver to look up the file or storage id defaults to default by the wopiserver if empty. [[Ref]](https://github.com/cs3org/reva/tree/master/internal/grpc/services/appprovider/appprovider.go#L62)
-{{< highlight toml >}}
-[grpc.services.appprovider]
-storageid = ""
-{{< /highlight >}}
-{{% /dir %}}
-
diff --git a/examples/ocmd/ocmd-server-1.toml b/examples/ocmd/ocmd-server-1.toml
@@ -71,8 +71,6 @@ driver = "memory"
 driver = "demo"
 iopsecret = "testsecret"
 wopiurl = "http://0.0.0.0:8880/"
-storageid = ""
-uiurl = ""
 
 [grpc.services.appregistry]
 driver = "static"

diff --git a/internal/grpc/services/appprovider/appprovider.go b/internal/grpc/services/appprovider/appprovider.go
@@ -26,7 +26,6 @@ import (
 	"io/ioutil"
 	"net/http"
 	"path"
-	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -40,7 +39,6 @@ import (
 	"github.com/cs3org/reva/pkg/rhttp"
 	"github.com/cs3org/reva/pkg/user"
 	"github.com/mitchellh/mapstructure"
-	"github.com/pkg/errors"
 	"google.golang.org/grpc"
 )
 
@@ -57,9 +55,7 @@ type config struct {
 	Driver    string                 `mapstructure:"driver"`
 	Demo      map[string]interface{} `mapstructure:"demo"`
 	IopSecret string                 `mapstructure:"iopsecret" docs:";The iopsecret used to connect to the wopiserver."`
-	WopiURL   string                 `mapstructure:"wopiurl" docs:";The wopiserver's url."`
-	UIURL     string                 `mapstructure:"uirul" docs:";URL to application (eg collabora) URL."`
-	StorageID string                 `mapstructure:"storageid" docs:";The storage id used by the wopiserver to look up the file or storage id defaults to default by the wopiserver if empty."`
+	WopiURL   string                 `mapstructure:"wopiurl" docs:";The wopiserver's URL."`
 }
 
 // New creates a new AppProviderService
@@ -102,6 +98,7 @@ func (s *service) UnprotectedEndpoints() []string {
 func (s *service) Register(ss *grpc.Server) {
 	providerpb.RegisterProviderAPIServer(ss, s)
 }
+
 func getProvider(c *config) (app.Provider, error) {
 	switch c.Driver {
 	case "demo":
@@ -115,11 +112,6 @@ func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.Ope
 
 	log := appctx.GetLogger(ctx)
 
-	wopiurl := s.conf.WopiURL
-	iopsecret := s.conf.IopSecret
-	storageID := s.conf.StorageID
-	folderURL := s.conf.UIURL + filepath.Dir(req.Ref.GetPath())
-
 	httpClient := rhttp.GetHTTPClient(
 		rhttp.Context(ctx),
 		// TODO make insecure configurable
@@ -128,7 +120,9 @@ func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.Ope
 		rhttp.Timeout(time.Duration(24*int64(time.Hour))),
 	)
 
-	appsReq, err := rhttp.NewRequest(ctx, "GET", wopiurl+"wopi/cbox/endpoints", nil)
+	// TODO this query will eventually be served by Reva. For the time being it is a remnant of the CERNBox-specific WOPI server,
+	// which justifies the /cbox path in the URL.
+	appsReq, err := rhttp.NewRequest(ctx, "GET", s.conf.WopiURL+"wopi/cbox/endpoints", nil)
 	if err != nil {
 		return nil, err
 	}
@@ -154,50 +148,41 @@ func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.Ope
 		return nil, err
 	}
 
-	httpReq, err := rhttp.NewRequest(ctx, "GET", wopiurl+"wopi/iop/open", nil)
+	httpReq, err := rhttp.NewRequest(ctx, "GET", s.conf.WopiURL+"wopi/iop/open", nil)
 	if err != nil {
 		return nil, err
 	}
 
 	q := httpReq.URL.Query()
-	q.Add("filename", req.Ref.GetPath())
-	q.Add("endpoint", storageID)
+	q.Add("fileid", req.ResourceInfo.GetId().OpaqueId)
+	q.Add("endpoint", req.ResourceInfo.GetId().StorageId)
 	q.Add("viewmode", req.ViewMode.String())
-	q.Add("folderurl", folderURL)
+	// TODO the folder URL should be resolved as e.g. `'https://cernbox.cern.ch/index.php/apps/files/?dir=' + filepath.Dir(req.Ref.GetPath())`
+	// or should be deprecated/removed altogether, needs discussion and decision.
+	q.Add("folderurl", "undefined")
 	u, ok := user.ContextGetUser(ctx)
-
 	if ok {
 		q.Add("username", u.Username)
 	}
-
-	httpReq.Header.Set("Authorization", "Bearer "+iopsecret)
+	// else defaults to "Anonymous Guest"
+	httpReq.Header.Set("Authorization", "Bearer "+s.conf.IopSecret)
 	httpReq.Header.Set("TokenHeader", req.AccessToken)
 
 	httpReq.URL.RawQuery = q.Encode()
 
 	openRes, err := httpClient.Do(httpReq)
 
 	if err != nil {
-		log.Error().Err(err).Msg("error performing http request")
 		res := &providerpb.OpenFileInAppProviderResponse{
-			Status: status.NewInternal(ctx, err, "error performing http request"),
+			Status: status.NewInternal(ctx, err, "appprovider: error performing open request to WOPI"),
 		}
 		return res, nil
 	}
 	defer openRes.Body.Close()
 
 	if openRes.StatusCode != http.StatusOK {
-		log.Error().Err(err).Msg("error performing http request")
-		res := &providerpb.OpenFileInAppProviderResponse{
-			Status: status.NewInternal(ctx, err, "error performing http request, status code: "+strconv.Itoa(openRes.StatusCode)),
-		}
-		return res, nil
-	}
-
-	if err != nil {
-		err := errors.Wrap(err, "appprovidersvc: error calling GetIFrame")
 		res := &providerpb.OpenFileInAppProviderResponse{
-			Status: status.NewInternal(ctx, err, "error getting app's iframe"),
+			Status: status.NewInternal(ctx, err, "appprovider: error performing open request to WOPI, status code: "+strconv.Itoa(openRes.StatusCode)),
 		}
 		return res, nil
 	}
@@ -216,7 +201,7 @@ func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.Ope
 		return nil, err2
 	}
 
-	fileExtension := path.Ext(req.Ref.GetPath())
+	fileExtension := path.Ext(req.ResourceInfo.GetPath())
 
 	viewOptions := appsBodyMap[fileExtension]
 
@@ -245,7 +230,7 @@ func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.Ope
 		providerURL += "?"
 	}
 
-	appProviderURL := fmt.Sprintf("App URL:\n%sWOPISrc=%s\n", providerURL, openResBody)
+	appProviderURL := fmt.Sprintf("%sWOPISrc=%s\n", providerURL, openResBody)
 
 	return &providerpb.OpenFileInAppProviderResponse{
 		Status:         status.NewOK(ctx),

diff --git a/internal/grpc/services/appprovider/appprovider_test.go b/internal/grpc/services/appprovider/appprovider_test.go
@@ -40,16 +40,12 @@ func Test_parseConfig(t *testing.T) {
 				"Demo":      map[string]interface{}{"a": "b", "c": "d"},
 				"IopSecret": "very-secret",
 				"WopiURL":   "https://my.wopi:9871",
-				"UIURL":     "https://the.ui",
-				"StorageID": "123-455",
 			},
 			want: &config{
 				Driver:    "demo",
 				Demo:      map[string]interface{}{"a": "b", "c": "d"},
 				IopSecret: "very-secret",
 				WopiURL:   "https://my.wopi:9871",
-				UIURL:     "", // this field seems not to get parsed see https://github.com/cs3org/reva/issues/991
-				StorageID: "123-455",
 			},
 			wantErr: nil,
 		},
@@ -72,8 +68,6 @@ func Test_parseConfig(t *testing.T) {
 				Demo:      map[string]interface{}(nil),
 				IopSecret: "",
 				WopiURL:   "",
-				UIURL:     "",
-				StorageID: "",
 			},
 			wantErr: nil,
 		},

diff --git a/internal/grpc/services/gateway/appprovider.go b/internal/grpc/services/gateway/appprovider.go
@@ -26,17 +26,14 @@ import (
 	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
-	"github.com/cs3org/reva/pkg/appctx"
+	tokenpkg "github.com/cs3org/reva/pkg/token"
 	"github.com/cs3org/reva/pkg/errtypes"
 	"github.com/cs3org/reva/pkg/rgrpc/status"
 	"github.com/cs3org/reva/pkg/rgrpc/todo/pool"
 	"github.com/pkg/errors"
 )
 
-func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFileInAppProviderRequest) (*providerpb.OpenFileInAppProviderResponse, error) {
-
-	log := appctx.GetLogger(ctx)
-
+func (s *svc) OpenFileInAppProvider(ctx context.Context, req *gateway.OpenFileInAppProviderRequest) (*providerpb.OpenFileInAppProviderResponse, error) {
 	c, err := s.find(ctx, req.Ref)
 	if err != nil {
 		if _, ok := err.(errtypes.IsNotFound); ok {
@@ -49,22 +46,27 @@ func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFil
 		}, nil
 	}
 
+	accessToken, ok := tokenpkg.ContextGetToken(ctx)
+	if !ok || accessToken == "" {
+		return &providerpb.OpenFileInAppProviderResponse{
+			Status: status.NewUnauthenticated(ctx, err, "Access token is invalid or empty"),
+		}, nil
+	}
+
 	statReq := &provider.StatRequest{
 		Ref: req.Ref,
 	}
 
 	statRes, err := c.Stat(ctx, statReq)
 	if err != nil {
-		log.Err(err).Msg("gateway: error calling Stat for the share resource path:" + req.Ref.GetPath())
 		return &providerpb.OpenFileInAppProviderResponse{
-			Status: status.NewInternal(ctx, err, "gateway: error calling Stat for the share resource id"),
+			Status: status.NewInternal(ctx, err, "gateway: error calling Stat on the resource path for the app provider: "+req.Ref.GetPath()),
 		}, nil
 	}
 	if statRes.Status.Code != rpc.Code_CODE_OK {
 		err := status.NewErrorFromCode(statRes.Status.GetCode(), "gateway")
-		log.Err(err).Msg("gateway: error calling Stat for the share resource id:" + req.Ref.GetPath())
 		return &providerpb.OpenFileInAppProviderResponse{
-			Status: status.NewInternal(ctx, err, "error updating received share"),
+			Status: status.NewInternal(ctx, err, "Stat failed on the resource path for the app provider: "+req.Ref.GetPath()),
 		}, nil
 	}
 
@@ -92,9 +94,16 @@ func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFil
 		}, nil
 	}
 
-	res, err := appProviderClient.OpenFileInAppProvider(ctx, req)
+    // build the appProvider specific request with the required extra info that has been obtained
+	appProviderReq := &providerpb.OpenFileInAppProviderRequest{
+		ResourceInfo: fileInfo,
+		ViewMode: req.ViewMode,
+		AccessToken: accessToken
+	}
+
+	res, err := appProviderClient.OpenFileInAppProvider(ctx, appProviderReq)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling c.Open")
+		return nil, errors.Wrap(err, "gateway: error calling OpenFileInAppProvider")
 	}
 
 	return res, nil