From c64497c996545c44b18507bd71496c117c87ef6d Mon Sep 17 00:00:00 2001 From: Giuseppe Lo Presti Date: Wed, 29 Jul 2020 12:19:14 +0200 Subject: [PATCH] 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 #991 and includes other minor changes concerning error handling and logging. --- .../grpc/services/appprovider/_index.md | 20 +------- examples/ocmd/ocmd-server-1.toml | 2 - .../grpc/services/appprovider/appprovider.go | 46 +++++++------------ .../services/appprovider/appprovider_test.go | 6 --- internal/grpc/services/gateway/appprovider.go | 16 +++---- 5 files changed, 24 insertions(+), 66 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 index 4e054efc5b5..b214810c84b 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 2f62f5becbd..07dc93a40d0 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 00c7f8ec931..532e6a32519 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,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("fileid", req.Ref.GetId().OpaqueId) + q.Add("endpoint", req.Ref.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) } + // 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() @@ -178,26 +174,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 } @@ -245,7 +231,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 f36cf3d383c..cb660cc0507 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 b7bee4f2695..17673a603bc 100644 --- a/internal/grpc/services/gateway/appprovider.go +++ b/internal/grpc/services/gateway/appprovider.go @@ -26,7 +26,6 @@ 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" "github.com/cs3org/reva/pkg/errtypes" "github.com/cs3org/reva/pkg/rgrpc/status" "github.com/cs3org/reva/pkg/rgrpc/todo/pool" @@ -34,9 +33,6 @@ import ( ) func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFileInAppProviderRequest) (*providerpb.OpenFileInAppProviderResponse, error) { - - log := appctx.GetLogger(ctx) - c, err := s.find(ctx, req.Ref) if err != nil { if _, ok := err.(errtypes.IsNotFound); ok { @@ -56,17 +52,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 +79,9 @@ func (s *svc) OpenFileInAppProvider(ctx context.Context, req *providerpb.OpenFil }, nil } + // make sure req.Ref is filled in with the required storageid + opaque fileid, not with the path + req.Ref.Spec = &storageprovider.Reference_Id{Id: statRes.Info.Id} + appProviderClient, err := pool.GetAppProviderClient(provider.Address) if err != nil { err = errors.Wrap(err, "gateway: error calling GetAppProviderClient") @@ -96,7 +92,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