From 90b0ba2c8454027801d46098d6b96e42198ffc2e Mon Sep 17 00:00:00 2001 From: Giuseppe Lo Presti Date: Wed, 29 Jul 2020 12:19:14 +0200 Subject: [PATCH] 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. --- cmd/reva/open-file-in-app-provider.go | 9 +--- .../grpc/services/appprovider/_index.md | 20 +------- examples/ocmd/ocmd-server-1.toml | 2 - .../grpc/services/appprovider/appprovider.go | 49 +++++++------------ .../services/appprovider/appprovider_test.go | 6 --- internal/grpc/services/gateway/appprovider.go | 31 +++++++----- 6 files changed, 40 insertions(+), 77 deletions(-) diff --git a/cmd/reva/open-file-in-app-provider.go b/cmd/reva/open-file-in-app-provider.go index 428ff9f921..d32c9940d8 100644 --- 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 index 4e054efc5b..b214810c84 100644 --- a/docs/content/en/docs/config/grpc/services/appprovider/_index.md +++ b/docs/content/en/docs/config/grpc/services/appprovider/_index.md @@ -9,7 +9,7 @@ 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 = "" @@ -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#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 index 510de46f63..a25f3a31a1 100644 --- 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 index 00c7f8ec93..f9ba7e75c4 100644 --- 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,23 +148,24 @@ 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() @@ -178,26 +173,16 @@ func (s *service) OpenFileInAppProvider(ctx context.Context, req *providerpb.Ope 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 index f36cf3d383..cb660cc050 100644 --- 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 index 003dbabc93..31bf8f4171 100644 --- 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