AppProvider: report errors from WOPI and return base64-encoded fileids

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

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
 	}

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")
 	}

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)
 	}
 
@@ -161,20 +161,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)