Refactor AppProvider workflow and protocol (#1035)

* Refactored the OpenFileInAppProvider workflow

This included changing the gRPC protocol and removing
storageID and UIURL from config.

En passant, this fixes #991 and includes other minor changes
concerning error handling and logging.

* Linting and other fixes + changelog

* Added configuration for ODF and MD files

* Fixed error handling + improved logging of a stack trace

* Further fixes following tests

* Improved URL handling following review

Also cleared some TODOs and reduced HTTP timeout to 5s

* Refactored the /wopi/cbox/endpoints HTTP call

This is now in its own function, as it does not need to be executed
at each incoming request. Added a TODO for implementing a refresh
every day or so.

* Do not use for now a shared map.

TODO for a future PR: refresh the map of apps URLs every day or week,
and cache it at the service level (protected by a multi-reader Lock).

changelog/unreleased/appprovider-fixes.md

@@ -0,0 +1,7 @@
+Enhancement: Refactor AppProvider workflow
+
+Simplified the app-provider configuration: storageID is worked out
+automatically and UIURL is suppressed for now.
+Implemented the new gRPC protocol from the gateway to the appprovider.
+
+https://github.com/cs3org/reva/pull/1035

cmd/reva/open-file-in-app-provider.go

@@ -22,20 +22,18 @@ import (
 	"fmt"
 	"os"
 
-	providerpb "github.com/cs3org/go-cs3apis/cs3/app/provider/v1beta1"
+	gateway "github.com/cs3org/go-cs3apis/cs3/gateway/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 {
 	cmd := newCommand("open-file-in-app-provider")
 	cmd.Description = func() string { return "Open a file in an external app provider" }
 	cmd.Usage = func() string {
-		return "Usage: open-file-in-app-provider [-flags] <path> <viewMode (view, read, write)>"
+		return "Usage: open-file-in-app-provider [-flags] [-viewmode view|read|write] <path>"
 	}
-	viewMode := cmd.String("viewMode", "view", "the view permissions, defaults to view")
+	viewMode := cmd.String("viewmode", "view", "the view permissions, defaults to view")
 
 	cmd.Action = func() error {
 		ctx := getAuthContext()
@@ -45,7 +43,7 @@ func openFileInAppProviderCommand() *command {
 		}
 		path := cmd.Args()[0]
 
-		viewMode := getViewMode(*viewMode)
+		vm := getViewMode(*viewMode)
 
 		client, err := getClient()
 		if err != nil {
@@ -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 := &gateway.OpenFileInAppProviderRequest{Ref: ref, ViewMode: vm}
 
 		openRes, err := client.OpenFileInAppProvider(ctx, openRequest)
 		if err != nil {
@@ -79,15 +72,15 @@ func openFileInAppProviderCommand() *command {
 	return cmd
 }
 
-func getViewMode(viewMode string) providerpb.OpenFileInAppProviderRequest_ViewMode {
+func getViewMode(viewMode string) gateway.OpenFileInAppProviderRequest_ViewMode {
 	switch viewMode {
 	case "view":
-		return providerpb.OpenFileInAppProviderRequest_VIEW_MODE_VIEW_ONLY
+		return gateway.OpenFileInAppProviderRequest_VIEW_MODE_VIEW_ONLY
 	case "read":
-		return providerpb.OpenFileInAppProviderRequest_VIEW_MODE_READ_ONLY
+		return gateway.OpenFileInAppProviderRequest_VIEW_MODE_READ_ONLY
 	case "write":
-		return providerpb.OpenFileInAppProviderRequest_VIEW_MODE_READ_WRITE
+		return gateway.OpenFileInAppProviderRequest_VIEW_MODE_READ_WRITE
 	default:
-		return providerpb.OpenFileInAppProviderRequest_VIEW_MODE_INVALID
+		return gateway.OpenFileInAppProviderRequest_VIEW_MODE_INVALID
 	}
 }

docs/content/en/docs/config/grpc/services/appprovider/_index.md

@@ -17,26 +17,10 @@ iopsecret = ""
 {{% /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#L60)
 {{< 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 %}}
-

examples/ocmd/ocmd-server-1.toml

@@ -71,14 +71,16 @@ driver = "memory"
 driver = "demo"
 iopsecret = "testsecret"
 wopiurl = "http://0.0.0.0:8880/"
-storageid = ""
-uiurl = ""
 
 [grpc.services.appregistry]
 driver = "static"
 
-[grpc.services.appregistry.drivers.static.rules]
+[grpc.services.appregistry.static.rules]
 "text/plain" = "localhost:19000"
+"text/markdown" = "localhost:19000"
+"application/vnd.oasis.opendocument.text" = "localhost:19000"
+"application/vnd.oasis.opendocument.spreadsheet" = "localhost:19000"
+"application/vnd.oasis.opendocument.presentation" = "localhost:19000"
 
 [grpc.services.storageprovider]
 driver = "localhome"

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/cheggaaa/pb v1.0.28
 	github.com/coreos/go-oidc v2.2.1+incompatible
 	github.com/cs3org/cato v0.0.0-20200626150132-28a40e643719
-	github.com/cs3org/go-cs3apis v0.0.0-20200728114537-4efa23660dbe
+	github.com/cs3org/go-cs3apis v0.0.0-20200730121022-c4f3d4f7ddfd
 	github.com/dgrijalva/jwt-go v3.2.0+incompatible
 	github.com/eventials/go-tus v0.0.0-20200718001131-45c7ec8f5d59
 	github.com/go-ldap/ldap/v3 v3.2.3

internal/grpc/interceptors/recovery/recovery.go

@@ -45,8 +45,8 @@ func NewStream() grpc.StreamServerInterceptor {
 }
 
 func recoveryFunc(ctx context.Context, p interface{}) (err error) {
-	stack := debug.Stack()
+	debug.PrintStack()
 	log := appctx.GetLogger(ctx)
-	log.Error().Str("stack", string(stack)).Msgf("%+v", p)
+	log.Error().Msgf("%+v", p)
 	return status.Errorf(codes.Internal, "%s", p)
 }

internal/grpc/services/appprovider/appprovider.go

@@ -22,11 +22,12 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"path"
-	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -40,7 +41,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 +57,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 +100,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":
@@ -111,141 +110,141 @@ func getProvider(c *config) (app.Provider, error) {
 	}
 }
 
-func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFileInAppProviderRequest) (*providerpb.OpenFileInAppProviderResponse, error) {
-
-	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())
-
+func (s *service) getWopiAppEndpoints(ctx context.Context) (map[string]interface{}, error) {
 	httpClient := rhttp.GetHTTPClient(
 		rhttp.Context(ctx),
-		// TODO make insecure configurable
-		rhttp.Insecure(true),
-		// TODO make timeout configurable
-		rhttp.Timeout(time.Duration(24*int64(time.Hour))),
+		// calls to WOPI are expected to take a very short time, 5s (though hardcoded) ought to be far enough
+		rhttp.Timeout(time.Duration(5*int64(time.Second))),
 	)
 
-	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.
+	wopiurl, err := url.Parse(s.conf.WopiURL)
+	if err != nil {
+		return nil, err
+	}
+	wopiurl.Path = path.Join(wopiurl.Path, "/wopi/cbox/endpoints")
+	appsReq, err := rhttp.NewRequest(ctx, "GET", wopiurl.String(), nil)
 	if err != nil {
 		return nil, err
 	}
 	appsRes, err := httpClient.Do(appsReq)
 	if err != nil {
-		log.Error().Err(err).Msg("error performing http request")
-		res := &providerpb.OpenFileInAppProviderResponse{
-			Status: status.NewInternal(ctx, err, "error performing http request"),
-		}
-		return res, nil
+		return nil, err
 	}
 	defer appsRes.Body.Close()
 	if appsRes.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(appsRes.StatusCode)),
-		}
-		return res, nil
+		return nil, errors.New("Request to WOPI server returned " + string(appsRes.StatusCode))
 	}
-
 	appsBody, err := ioutil.ReadAll(appsRes.Body)
 	if err != nil {
 		return nil, err
 	}
 
-	httpReq, err := rhttp.NewRequest(ctx, "GET", wopiurl+"wopi/iop/open", nil)
+	appsURLMap := make(map[string]interface{})
+	err = json.Unmarshal(appsBody, &appsURLMap)
+	if err != nil {
+		return nil, err
+	}
+
+	log := appctx.GetLogger(ctx)
+	log.Info().Msg(fmt.Sprintf("Successfully retrieved %d WOPI app endpoints", len(appsURLMap)))
+	return appsURLMap, nil
+}
+
+func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFileInAppProviderRequest) (*providerpb.OpenFileInAppProviderResponse, error) {
+
+	log := appctx.GetLogger(ctx)
+
+	httpClient := rhttp.GetHTTPClient(
+		rhttp.Context(ctx),
+		// calls to WOPI are expected to take a very short time, 5s (though hardcoded) ought to be far enough
+		rhttp.Timeout(time.Duration(5*int64(time.Second))),
+	)
+
+	wopiurl, err := url.Parse(s.conf.WopiURL)
+	if err != nil {
+		return nil, err
+	}
+	wopiurl.Path = path.Join(wopiurl.Path, "/wopi/iop/open")
+	httpReq, err := rhttp.NewRequest(ctx, "GET", wopiurl.String(), 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.NewInvalid(ctx, "appprovider: error performing open request to WOPI, status code: "+strconv.Itoa(openRes.StatusCode)),
 		}
 		return res, nil
 	}
 
 	buf := new(bytes.Buffer)
-	_, err1 := buf.ReadFrom(openRes.Body)
-	if err1 != nil {
-		return nil, err1
+	_, err = buf.ReadFrom(openRes.Body)
+	if err != nil {
+		return nil, err
 	}
-
 	openResBody := buf.String()
 
-	appsBodyMap := make(map[string]interface{})
-	err2 := json.Unmarshal(appsBody, &appsBodyMap)
-	if err2 != nil {
-		return nil, err2
+	// TODO call this e.g. once a day or a week, and cache the content in a shared map protected by a multi-reader Lock
+	appsURLMap, err := s.getWopiAppEndpoints(ctx)
+	if err != nil {
+		res := &providerpb.OpenFileInAppProviderResponse{
+			Status: status.NewInternal(ctx, err, "appprovider: getWopiAppEndpoints failed"),
+		}
+		return res, nil
 	}
-
-	fileExtension := path.Ext(req.Ref.GetPath())
-
-	viewOptions := appsBodyMap[fileExtension]
-
+	viewOptions := appsURLMap[path.Ext(req.ResourceInfo.GetPath())]
 	viewOptionsMap, ok := viewOptions.(map[string]interface{})
 	if !ok {
-		log.Error().Msg("error typecasting to map")
 		res := &providerpb.OpenFileInAppProviderResponse{
-			Status: status.NewInternal(ctx, nil, "error typecasting to map"),
+			Status: status.NewInvalid(ctx, "Incorrect parsing of the App URLs map from the WOPI server"),
 		}
 		return res, nil
 	}
 
 	var viewmode string
-
 	if req.ViewMode == providerpb.OpenFileInAppProviderRequest_VIEW_MODE_READ_WRITE {
 		viewmode = "edit"
 	} else {
 		viewmode = "view"
 	}
 
-	providerURL := fmt.Sprintf("%v", viewOptionsMap[viewmode])
-
-	if strings.Contains(providerURL, "?") {
-		providerURL += "&"
+	appProviderURL := fmt.Sprintf("%v", viewOptionsMap[viewmode])
+	if strings.Contains(appProviderURL, "?") {
+		appProviderURL += "&"
 	} else {
-		providerURL += "?"
+		appProviderURL += "?"
 	}
-
-	appProviderURL := fmt.Sprintf("App URL:\n%sWOPISrc=%s\n", providerURL, openResBody)
+	appProviderURL = fmt.Sprintf("%sWOPISrc=%s", appProviderURL, openResBody)
+	log.Info().Msg(fmt.Sprintf("Returning app provider URL %s", appProviderURL))
 
 	return &providerpb.OpenFileInAppProviderResponse{
 		Status:         status.NewOK(ctx),