diff --git a/changelog/unreleased/app-errors.md b/changelog/unreleased/app-errors.md new file mode 100644 index 0000000000..7600ecb651 --- /dev/null +++ b/changelog/unreleased/app-errors.md @@ -0,0 +1,4 @@ +Bugfix: AppProvider: propagate back errors reported by WOPI +on /app/open and return base64-encoded fileids on /app/new + +https://github.com/cs3org/reva/pull/2103 diff --git a/internal/grpc/services/appprovider/appprovider.go b/internal/grpc/services/appprovider/appprovider.go index 8dd446d704..d764a94c85 100644 --- a/internal/grpc/services/appprovider/appprovider.go +++ b/internal/grpc/services/appprovider/appprovider.go @@ -20,6 +20,7 @@ package appprovider import ( "context" + "errors" "os" "time" @@ -35,7 +36,6 @@ import ( "github.com/cs3org/reva/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/pkg/sharedconf" "github.com/mitchellh/mapstructure" - "github.com/pkg/errors" "google.golang.org/grpc" ) @@ -145,9 +145,8 @@ func getProvider(c *config) (app.Provider, error) { func (s *service) OpenInApp(ctx context.Context, req *providerpb.OpenInAppRequest) (*providerpb.OpenInAppResponse, error) { appURL, err := s.provider.GetAppURL(ctx, req.ResourceInfo, req.ViewMode, req.AccessToken) if err != nil { - err := errors.Wrap(err, "appprovider: error calling GetAppURL") res := &providerpb.OpenInAppResponse{ - Status: status.NewInternal(ctx, err, "error getting app URL"), + Status: status.NewInternal(ctx, errors.New("appprovider: error calling GetAppURL"), err.Error()), } return res, nil } diff --git a/internal/http/services/appprovider/appprovider.go b/internal/http/services/appprovider/appprovider.go index 4063283a02..30f949a714 100644 --- a/internal/http/services/appprovider/appprovider.go +++ b/internal/http/services/appprovider/appprovider.go @@ -49,6 +49,10 @@ import ( "github.com/rs/zerolog/log" ) +const ( + idDelimiter string = ":" +) + func init() { global.Register("appprovider", New) } @@ -199,14 +203,17 @@ func (s *svc) handleNew(w http.ResponseWriter, r *http.Request) { return } - // Stat created file and return its file id + // Stat the newly created file statRes, ocmderr, err := statRef(ctx, provider.Reference{Path: target}, client) if err != nil { log.Error().Err(err).Msg("error statting created file") ocmd.WriteError(w, r, ocmderr, "Created file not found", errtypes.NotFound("Created file not found")) return } - js, err := json.Marshal(map[string]interface{}{"file_id": statRes.Id}) + + // Base64-encode the fileid for the web to consume it + b64id := base64.StdEncoding.EncodeToString([]byte(statRes.Id.StorageId + idDelimiter + statRes.Id.OpaqueId)) + js, err := json.Marshal(map[string]interface{}{"file_id": b64id}) if err != nil { ocmd.WriteError(w, r, ocmd.APIErrorServerError, "error marshalling JSON response", err) return @@ -274,12 +281,13 @@ func (s *svc) handleOpen(w http.ResponseWriter, r *http.Request) { } openRes, err := client.OpenInApp(ctx, &openReq) if err != nil { - ocmd.WriteError(w, r, ocmd.APIErrorServerError, "error opening resource", err) + log.Error().Err(err).Msg("error calling OpenInApp") + ocmd.WriteError(w, r, ocmd.APIErrorServerError, err.Error(), err) return } if openRes.Status.Code != rpc.Code_CODE_OK { - ocmd.WriteError(w, r, ocmd.APIErrorServerError, "error opening resource information", - status.NewErrorFromCode(openRes.Status.Code, "appprovider")) + ocmd.WriteError(w, r, ocmd.APIErrorServerError, openRes.Status.Message, + status.NewErrorFromCode(openRes.Status.Code, "error calling OpenInApp")) return } @@ -327,7 +335,7 @@ func (s *svc) getStatInfo(ctx context.Context, fileID string, client gateway.Gat return nil, ocmd.APIErrorInvalidParameter, errors.Wrap(err, "fileID doesn't follow the required format") } - parts := strings.Split(string(decodedID), ":") + parts := strings.Split(string(decodedID), idDelimiter) if !utf8.ValidString(parts[0]) || !utf8.ValidString(parts[1]) { return nil, ocmd.APIErrorInvalidParameter, errors.New("fileID contains illegal characters") } diff --git a/pkg/app/provider/wopi/wopi.go b/pkg/app/provider/wopi/wopi.go index 4624241255..9e91044e71 100644 --- a/pkg/app/provider/wopi/wopi.go +++ b/pkg/app/provider/wopi/wopi.go @@ -137,7 +137,7 @@ func (p *wopiProvider) GetAppURL(ctx context.Context, resource *provider.Resourc q.Add("endpoint", resource.GetId().StorageId) q.Add("viewmode", viewMode.String()) u, ok := ctxpkg.ContextGetUser(ctx) - if ok { // else defaults to "Anonymous Guest" + if ok { // else defaults to "Guest xyz" q.Add("username", u.Username) } @@ -178,20 +178,26 @@ func (p *wopiProvider) GetAppURL(ctx context.Context, resource *provider.Resourc httpReq.Header.Set("Authorization", "Bearer "+p.conf.IOPSecret) httpReq.Header.Set("TokenHeader", token) + // Call the WOPI server and parse the response (body will always contain a payload) openRes, err := p.wopiClient.Do(httpReq) if err != nil { return nil, errors.Wrap(err, "wopi: error performing open request to WOPI server") } defer openRes.Body.Close() - if openRes.StatusCode != http.StatusOK { - return nil, errtypes.InternalError("wopi: unexpected status from WOPI server: " + openRes.Status) - } - body, err := ioutil.ReadAll(openRes.Body) if err != nil { return nil, err } + if openRes.StatusCode != http.StatusOK { + // WOPI returned failure: body contains a user-friendly error message (yet perform a sanity check) + sbody := "" + if body != nil { + sbody = string(body) + } + log.Warn().Msg(fmt.Sprintf("wopi: WOPI server returned HTTP %s, error was: %s", openRes.Status, sbody)) + return nil, errors.New(sbody) + } var result map[string]interface{} err = json.Unmarshal(body, &result)