Extend app registry with AddProvider method and mimetype filters

[ishank011]

Closes #1779

[lgtm-com]

This pull request introduces 1 alert when merging be54c41 into 07adb3f - view on LGTM.com

new alerts:

• 1 for Useless assignment to field

[labkode]

Needs cs3org/cs3apis#131 to be merged first. After, that:

• Point to upstream go-cs3apis
• Document breaking change in changelog

[lgtm-com]

This pull request fixes 20 alerts when merging 0c1b9fe into 7ecc818 - view on LGTM.com

fixed alerts:

• 20 for Uncontrolled data used in path expression

[glpatcern]

(Related to the above) this configuration parameter is specified in the WOPI server config, and would need to be duplicated in some Reva config. Why is it important to provide this information to the web UI?

[wkloucek]

We are currently using this in the oCIS WOPI extension to not have the access token in the browsers' address bar. If you have the access token in the address bar it could easily leak to other people (copy and paste)

https://github.com/owncloud/ocis-wopiserver/blob/e23ff205ec4474e4297ba39248636dca5fbb9402/ui/app.js#L84-L105 shows the form push with the access token. Downside is that you cant copy the URL and open it in a new tab

[wkloucek]

The same form post method is also recommended for embedding into iframes: https://wopi.readthedocs.io/en/latest/hostpage.html

So there might be some usecases where you need them separate

[glpatcern]

Makes sense, indeed we currently embed those URLs in an iframe so the user only sees a cernbox URL and can't really copy/paste it (yet of course by inspecting the iframe's content you'd see the access_token).

At this point I'd at least rename the struct to AppResponse, including the fields AppURL and AccessToken, just to be more generic and not WOPI specific. Other applications that do not implement an access token mechanism may leave it empty. Also, I'd still drop the TTL value - eventually, what rules is the TTL of the main JWT obtained by the user when logging in the first time, right?

[wkloucek]

Depending on how you define a TTL, the access token ttl is WOPI specific since it is the UNIX timestamp of the moment when the storage / REVA JWT will expire.
In Collabora providing access_token_ttl information triggers a warning for the user that editing will be no longer possible after the expiry (because after that Collabora cannot save the document obviously).

https://wopi.readthedocs.io/projects/wopirest/en/latest/concepts.html#term-access-token-ttl:

To prevent data loss for users, Office for the web will prompt users to save and refresh their sessions if the access token for their session is close to expiring. In order to do this, Office for the web needs to know when the access token will expire, which it determines based on the access_token_ttl value.

[glpatcern]

ok, to be exact it's the min(Reva_JWT_TTL, WOPI_access_token_TTL). I must say I had forgotten about this feature of the WOPI protocol, but then the value must be computed on the fly, not taken from the config (yeah, as admitted by the WOPI docs, the name is misleading, it should really be called access_token_expiration_time): can we extract the expiration time of a Reva JWT? If so, we can safely assume that the WOPI access_token's validity is never shorter (its default value in the WOPI server config is 24h) and return the Reva JWT TTL.

[wkloucek]

yeah of course we can take the exp property from the WOPI jwt access token (if we do so without validating the WOPI access token, we also don't need the WOPI jwt secret in here, which is fine for only taking the exp from it)

The expiration duration of the REVA jwt access token should already be known in that case because we are issuing it

[glpatcern]

If we have the expiration time of the REVA jwt access token, that's more than enough - we can assume the WOPI jwt access token is issued with a longer expiration in all cases - at least it should be configured like that.

[wkloucek]

From my viewpoint it would be desirable if the user could also decide to open a file in readonly mode even if he has permissions to open it in readwrite mode. (We didn't takle this in oCIS WOPI extension because we have only one open action in the Web UI until now, but this will be more flexible in the future owncloud/web#5135)

[glpatcern]

That's a very valid point, and the viewMode should indeed come from the client (the web UI) exactly like today it is an external parameter passed to the GRPC open-in-app command.

[wkloucek]

The current logic could be used as a fallback if no viewMode was given. I originally used the logic to determine maximum permissions of a user with the stat call because we have no possibility to determine the ViewMode in ownCloud Web currently.

[ishank011]

Yep, that would be ideal. For now, the idea was to mimic the logic of ocis-wopiserver so we could use it directly with web. We also need to discuss changing the endpoint

[wkloucek]

I also thought about how we could make this more generic, so that ownCloud Web just displays the available apps. Ideally this would be done by a extension (which is shipped with oCIS by default) or even integrated in Web directly. We probably should have a discussion with @kulmann and @dragotin about that topic. But I don't know if it is within the scope of this PR!?

[ishank011]

Yes that is incorporated in the PR! We had the FindAppProviders call which returns all apps for a specific mime type and now we also have a default app configured for each mime type. So we can expose the former to get a list of all apps and then the open call would have the app name as a parameter. Would be good to discuss this at the Friday meeting

[ishank011]

Tested it with the wopiserver and everything works.

There's one workaround that I had to use. We expect the app provider to register itself to the app registry, but it can't happen in the New method as the rgrpc package first retrieves these methods of all the registered services and then starts the GRPC server. So I start a goroutine from the New method of the app provider service which waits for a couple of seconds to send the AddAppProvider call, assuming that in this time, the server has been started (this can be made configurable). Thoughts? @labkode @glpatcern

reva/internal/grpc/services/appprovider/appprovider.go

Lines 87 to 106 in de61bcc

	service := &service{
	conf: c,
	provider: provider,
	}
	go service.registerProvider()
	return service, nil
	}
	func (s *service) registerProvider() {
	// Give the appregistry service time to come up
	time.Sleep(2 * time.Second)
	ctx := context.Background()
	log := logger.New().With().Int("pid", os.Getpid()).Logger()
	pInfo, err := s.provider.GetAppProviderInfo(ctx)
	if err != nil {
	log.Error().Err(err).Msgf("error registering app provider: could not get provider info")
	return
	}

[ishank011]

The checks seemed to have disappeared for the PR, weird. Force pushing didn't work either

[wkloucek]

The checks seemed to have disappeared for the PR, weird. Force pushing didn't work either

@ishank011 https://www.githubstatus.com/ Webhook status is degraded

[ishank011]

Ah okay. Thanks @wkloucek!

[wkloucek]

AccessTokenTTL is not really used for minting a token with that lifetime. We should use the actual lifetime of the token.

[ishank011]

Yes, but I don't want to add the token dismantling logic to an HTTP service. It's being done in multiple places in ocis and I would like to get rid of that there as well. I'll try to think of changes in the CS3APIs definitions through which this can be passed to this layer.

[wkloucek]

Sounds reasonable :-) I don't like this as well

[wkloucek]

@ishank011 I don't fully get the strategy how to get a list of supported mime types by client applications (not by the app provider itself). With the current implementation I would need to register an app provider multiple times (depending on the count of client applications) and change the description of each instance to the client application's name.

Example:
The app provider "wopi" supports opening files in different client applications: "Collabora", "Microsoft Office Online Server", "CodiMD" and "Etherpad". As a user I don't want to see that I can open the file with "wopi" but I want to see if I can open the file in "Collabora", ... And I only want to see the file action if the open action is possible for that client application.

[ishank011]

Hi @wkloucek. Yes you would need multiple instances of the appprovider, each pointing to a different client app, all of which can be served by the same wopi server though. At initialization, each of the appproviders will call their discovery URLs, retrieve the mime types and register themselves as "Collabora", "Microsoft Office Online Server", "CodiMD", and so on. From the web app, we can call FindAppProviders with the file name, which will give you a list of all apps supporting that mimetype. Then we'll call OpenInApp with that particular app name.

We need to finalize how the calls from web would look like and for that, we have a meeting scheduled tomorrow

[wkloucek]

We said, that we first will go with an external extension for Web and later try to move it in to the Web repository.

Currently there are limitations what a Web extension can do when registering file handlers, therefore I already wrote down requirements which also cover the ones for the AppProvider integration: owncloud/web#5135

So basically we are currently limited by the topics mentioned in this issue.

In my opinion we need some simple map like this in the frondent:

{
  "application/vnd.oasis.opendocument.text": {
    "appprovider": [
      { "provider": "collabora.example.test", "displayname": "Collabora" },
      { "provider": "onlyoffice.example.test", "displayname": "Only Office" }
    ]
  },
  "application/vnd.oasis.opendocument.spreadsheet": {
    "appprovider": [
      { "provider": "collabora.example.test", "displayname": "Collabora" },
      { "provider": "onlyoffice.example.test", "displayname": "Only Office" }
    ]
  }
}

But we should consider to add more information to it, since some appproviders might support opening files from x while other do not. Shares, Federated Shares, ... come into my mind when thinking about that.
Also some appproviders might support multiple modes to open a file: open in read-write-mode and open in read-only-mode.

[ishank011]

So with the new workflow, we won’t need to list these extensions statically. There’s an endpoint called GetStorageProvides which can be exposed through a REST layer which will be called for each file and the list of supported apps should be populated from that response. And the response is similar to what you mentioned but instead of returning providers for all mime types, we return only those for the passed file.

Not opening shares is very reva specific so I don’t think it’ll fit in CS3APIs. That logic can be implemented in the app drivers and we can return the appropriate status code.

But the option to restrict view modes in apps itself makes a lot of sense. I’ll add it to the PR

[labkode]

Merging as we discussed 1 week ago to move forward.

[wkloucek]

@ishank011 @labkode this made it to master, could you please take care to remove it?

[ishank011]

@wkloucek yep, created a PR #1922