Removed storageID and UIURL from config.

StorageID is auto-resolved in the gateway service, whereas the UIURL's doc was misleading: we need to decide whether to drop it entirely (surely it won't stay in the config). En passant, this fixes cs3org#991 and includes other minor changes concerning error handling and logging.
glpatcern · Jul 29, 2020 · f35b85c · f35b85c
1 parent 20ab30e
commit f35b85c
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 45 deletions.
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#L58)
 {{< 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#L59)
 {{< 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/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"
@@ -58,8 +57,6 @@ type config struct {
 	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."`
 }
 
 // New creates a new AppProviderService
@@ -102,6 +99,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 +113,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 +121,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,23 +149,25 @@ 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("endpoint", req.StorageID.String())
 	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)
 	}
+	// else defaults to "Anonymous Guest"
 
-	httpReq.Header.Set("Authorization", "Bearer "+iopsecret)
+	httpReq.Header.Set("Authorization", "Bearer "+s.conf.IopSecret)
 	httpReq.Header.Set("TokenHeader", req.AccessToken)
 
 	httpReq.URL.RawQuery = q.Encode()
@@ -245,7 +242,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
@@ -56,17 +56,14 @@ func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFil
 	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
 	}
 
@@ -86,6 +83,11 @@ func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFil
 		}, nil
 	}
 
+	if req.storageID == nil {
+		// if the client did not provide a storageID in the request, use the one resolved out of Stat()
+		req.storageID = fileInfo.id.storageID
+	}
+
 	appProviderClient, err := pool.GetAppProviderClient(provider.Address)
 	if err != nil {
 		err = errors.Wrap(err, "gateway: error calling GetAppProviderClient")
@@ -96,7 +98,7 @@ func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFil
 
 	res, err := appProviderClient.OpenFileInAppProvider(ctx, req)
 	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