collaboration fixes #9165

wkloucek · 2024-05-14T12:56:28Z

Description

owncloud.dev lists a default for COLLABORATION_JWT_SECRET, is fixed by leveraging ocis init
separates REVA jwt and WOPI jwt secret
COLLABORATION_CS3API_GATEWAY_NAME wasn't listed on owncloud.dev. because of a superfluous whitespace

Related Issue

none

Motivation and Context

How Has This Been Tested?

not yet tested -> TODO

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

update-docs · 2024-05-14T12:56:33Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

wkloucek · 2024-05-14T13:08:41Z

@micbar @jvillafanez do those changes make sense? If yes, could you please bring them to a mergeable state or open another PR? If you have questions, I'm happy to answer those. I just didn't know how to put this feedback into an ticket 😆

jvillafanez · 2024-05-14T13:50:47Z

owncloud.dev lists a default for COLLABORATION_JWT_SECRET, is fixed by leveraging ocis init

Any better alternative? It seems weird to me having to partially configure a service that I won't use (the service isn't started by default because we need an external WOPI app).
If there are no better options, we probably should include a comment in the default collaboration config to say that the JWT token will be initialized during the init command, otherwise it might be confusing if we don't find the default JWT token within the collaboration service

wkloucek · 2024-05-14T14:17:45Z

It seems weird to me having to partially configure a service that I won't use (the service isn't started by default because we need an external WOPI app).

The idea of ocis server and ocis init is to pretend there are no services and make it easier for users to use oCIS. Therefore this is by design.

Right now there is no other deployment method than "pretend there are no services" except the oCIS Helm Chart from what I know.

If there are no better options, we probably should include a comment in the default collaboration config to say that the JWT token will be initialized during the init command, otherwise it might be confusing if we don't find the default JWT token within the collaboration service

Actually the validation step are missing, compare

ocis/services/collaboration/pkg/config/parser/parse.go

Lines 35 to 38 in 4fd692f

    
           // Validate validates the configuration 
        
           func Validate(cfg *config.Config) error { 
        
           	return nil 
        
           }

to eg.

ocis/services/proxy/pkg/config/parser/parse.go

Lines 37 to 39 in 4fd692f

    
           if cfg.MachineAuthAPIKey == "" { 
        
           	return shared.MissingMachineAuthApiKeyError(cfg.Service.Name) 
        
           }

butonic · 2024-05-15T07:45:33Z

there are some changes in this PR that I also made in #9148

wkloucek · 2024-05-15T08:07:19Z

there are some changes in this PR that I also made in #9148

#9148 looks interesting, too. I didn't get so far when looking at it in owncloud/ocis-charts#567

jvillafanez · 2024-05-15T08:32:23Z

ocis/pkg/init/init.go

@@ -289,6 +294,10 @@ func CreateConfig(insecure, forceOverwrite bool, configPath, adminPassword strin
 	if err != nil {
 		return fmt.Errorf("could not generate random password for tokenmanager: %s", err)
 	}
+	collaborationJwtSecret, err := generators.GenerateRandomPassword(passwordLength)


Just saying that the original code used the uniuri lib, which used alphanumeric characters. Current code will also include additional symbols. I don't know if they could cause issues

Good question!

jvillafanez · 2024-05-15T08:33:26Z

Code looks fine to me, except for the "settings.json" file which should be removed

by mistake

- separate REVA jwt secret from WOPI jwt secret - fix gatway service name configuration

butonic · 2024-05-24T12:06:33Z

ocis-pkg/shared/errors.go

@@ -85,3 +85,11 @@ func MissingServiceAccountSecret(service string) error {
 		"the config/corresponding environment variable).",
 		service, defaults.BaseConfigPath())
 }
+
+func MissingWOPISecretError(service string) error {


the wopi secret is a different thing then our internal jwt secret. so we need a seperate error for it. IIRC this becomes relevant when using the office365 proxy.

butonic · 2024-05-24T12:08:41Z

ocis/pkg/init/init.go

+type WopiApp struct {
+	Insecure bool   `yaml:"insecure"`
+	Secret   string `yaml:"secret"`
+}
+
+type Collaboration struct {
+	WopiApp WopiApp `yaml:"wopiapp"`
+}
+


when initializing ocis we initialize all secrets and insecure flags. In a subsequent PR I'll allow running the collaboration service behind the proxy. Then configuring the secrets this will become more mandatory. Might as well do it now.

butonic · 2024-05-24T12:11:06Z

services/collaboration/README.md

-* `COLLABORATION_HTTP_ADDR`:\
-  The external address of the collaboration service. The target app (onlyoffice, collabora, etc) will use this address to read and write files from Infinite Scale.\
-  For example: `https://wopiserver.example.com`.
+* `COLLABORATION_WOPIAPP_INSECURE`:\
+  In case you are using a self signed certificate for the WOPI app you can tell the collaboration service to allow an insecure connection.

-* `COLLABORATION_HTTP_SCHEME`:\
-  The scheme to be used when accessing the collaboration service. Either `http` or `https`. This will be used to finally build the URL that the WOPI app needs in order to contact the collaboration service.
+* `COLLABORATION_WOPIAPP_WOPISRC`:\
+  The external address of the collaboration service. The target app (onlyoffice, collabora, etc) will use this address to read and write files from Infinite Scale. \
+  For example: `https://wopi.example.com`.


All other services use {SERVICENAME}_HTTP_ADDR to set the bind address. We should not change this here.

Instead we can configure the COLLABORATION_WOPIAPP_WOPISRC directly.

butonic · 2024-05-24T12:14:46Z

services/collaboration/pkg/config/config.go

-	JWTSecret string `yaml:"jwt_secret" env:"OCIS_JWT_SECRET;COLLABORATION_JWT_SECRET" desc:"Used as JWT token and to encrypt access token." introductionVersion:"5.1"`
+	TokenManager *TokenManager `yaml:"token_manager"`


OCIS_JWT_SECRET is used to verify the signature of reva acces tokens. The WOPI secret secures communication bewenn the collaboration service and the WOPI application. That is why I changed this to follow the usual tokenmanager pattern and introduced COLLABORATION_WOPIAPP_SECRET.

butonic · 2024-05-24T12:15:26Z

services/collaboration/pkg/config/defaults/defaultconfig.go

 		GRPC: config.GRPC{
-			Addr:      "0.0.0.0:9301",
-			Namespace: "com.owncloud.collaboration",
+			Addr:      "127.0.0.1:9301",


grpc can listen to localhost by default

butonic · 2024-05-24T12:15:55Z

services/collaboration/pkg/config/defaults/defaultconfig.go

@@ -44,12 +40,13 @@ func DefaultConfig() *config.Config {
 			Zpages: false,
 		},
 		WopiApp: config.WopiApp{
-			Addr:     "https://127.0.0.1:8080",
+			Addr:     "https://127.0.0.1:9980",


9980 is the default used by collabora

butonic · 2024-05-24T12:17:14Z

services/collaboration/pkg/config/defaults/defaultconfig.go

 			Insecure: false,
+			WopiSrc:  "https://localhost:9300",


if the collaboration service is started standalone it is reachable on localhost. Config doc string and README give an example when running on a different domain.

butonic · 2024-05-24T12:19:43Z

@jvillafanez @2403905 I rebased and updated and commented the PR, please try this out. I have collabora running in a docker container, run ocis and then use this VSCode config for the collaboration service:

			{
				"name": "wopiserver.ocis.test collaboration",
				"type": "go",
				"request": "launch",
				"mode": "debug",
				"program": "${workspaceFolder:ocis}/ocis/cmd/ocis",
				"args": [
					"collaboration", "server"
				],
				"env": {
					"REVA_GATEWAY": "com.owncloud.api.gateway",
					//"COLLABORATION_WOPIAPP_ADDR": "https://onlyoffice.ocis.test",
					"COLLABORATION_WOPIAPP_ADDR": "https://collabora.ocis.test",
					// share the registry with the ocis 
					"MICRO_REGISTRY_ADDRESS": "127.0.0.1:9233",
					"COLLABORATION_WOPIAPP_WOPISRC": "https://wopiserver.ocis.test", // needs to be routed to the collaboration service from within the collabore container
					
					// set some hardcoded secrets
					// collaboration
					"COLLABORATION_WOPIAPP_SECRET": "some-wopi-secret",
					"OCIS_JWT_SECRET": "some-ocis-jwt-secret", // same JWT secret as used for ocis
				}
			},

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic · 2024-05-24T12:53:58Z

@ScharfViktor how do we set the secrets for the collaboration service in CI?

jvillafanez · 2024-05-24T13:01:13Z

services/collaboration/pkg/config/wopiapp.go

+	WopiSrc  string `yaml:"wopisrc" env:"COLLABORATION_WOPIAPP_WOPISRC" desc:"The WOPISrc base URL containing schema, host and port. Path will be set to /wopi/files/{fileid} if not given. {fileid} will be replaced with the WOPI file id. Set this to the schema and domain where the collaboration service is reachable for the wopi app, such as https://office.owncloud.test." introductionVersion:"5.1"`
+	Secret   string `yaml:"secret" env:"COLLABORATION_WOPIAPP_SECRET" desc:"Used to mint and verify WOPI JWT tokens and encrypt and decrypt the REVA JWT token embedded in the WOPI JWT token." introductionVersion:"5.1"`


I'm not sure if this is the right place for those properties.
The WopiApp should refer to Collabora / OnlyOffice... however, the WopiSrc is generated in the collaboration service and refers to the collaboration service (it's the externally accessible address so Collabora / OnlyOffice can access to the service). It could be confusing as it seems to be related to the Collabora / OnlyOffice app.

The Secret is generated in the collaboration service. With this changes it seems that you might need to match something in Collabora / OnlyOffice.

I put WopiSrc and Secret into the wopiapp because they are both used to communicate with the wopi app. The wopisrc is the entpoint the app should use and the secret is used for the wopi access token JWT. This secret actually need to match the secret of the wopi proxy, so I think they are fine as wopi app related properties.

When we set up a second wopi server for another application we need to change these variables as well.

That is unless we want to make one collaboration service fit to handle multiple wopi apps, as the python wopiserver ... @micbar what is the plan here? I mean technically it would be possible ... then COLLABORATION_WOPIAPP_WOPISRC could be COLLABORATION_WOPISRC. The secret should still be dedicated per app though.

I think we'll need to discuss plans because I assumed we want to "link" one collaboration service to one wopi app. We could spawn multiple replicas of the collaboration service, but all of them would be "linked" to the same wopi app.

I don't see a good use case for using multiple wopi apps at the same time. The main problem is that, having multiple wopi apps available, we have to assume that the same file could be opened an multiple wopi apps by different users at the same time. This is a terrible idea because userA could overwrite the changes of userB (which is something to avoid and the reason of using the wopi apps)

jvillafanez · 2024-05-24T13:07:53Z

services/collaboration/pkg/server/http/server.go

+	_ = chi.Walk(mux, func(method string, route string, handler stdhttp.Handler, middlewares ...func(stdhttp.Handler) stdhttp.Handler) error {
+		options.Logger.Debug().Str("method", method).Str("route", route).Int("middlewares", len(middlewares)).Msg("serving endpoint")
+		return nil
+	})


Do we want to keep this? Walking though the routes seems potentially expensive and I'm not sure if we want do that for something that should be fixed after development.
I mean, unless you're debugging during development, I don't think the routes will change, and I don't think we expect dynamic changes in production.

this is only for startup and helps tremendously to see what is actually being routed. also sometimes a list of all routes is required for security theatre... 😞

ok, then maybe include a comment? Just to make sure we don't accidentally remove this

jvillafanez · 2024-05-24T13:22:05Z

services/collaboration/pkg/service/grpc/v0/service.go

-		Scheme: s.config.HTTP.Scheme,
-		Host:   s.config.HTTP.Addr,
-		Path:   path.Join("wopi", "files", fileRef),
+	wopiSrcURL, err := url.Parse(strings.Replace(s.config.WopiApp.WopiSrc, "{fileid}", fileRef, 1))


A couple of things:
I guess we might want to allow pointing the WopiSrc to a different server... but why? I'd expect the URLs always to point to this server.
Having a variable {fileid} in the URL doesn't seem a good idea. It will likely confuse the admin and we'll have to manage that scenario. It seems better for the admin to provide the server's URL so we can build the rest on our own.

I might be overly flexible here, yes. The hostname will be different when running the wopi server behind a reverse proxy. In kubernetes we can bypass the ingress and make wopi applications directy tock to the collaboration service without even exposing it.

COLLABORATION_WOPIAPP_WOPISRC should be set to just the domain. But we could announce different WOPI_SRC endpoints for every wopi app ...

If an admin specifies a domain AND a path we could also die when parsing the url in the config validation ...

ScharfViktor · 2024-05-24T13:28:46Z

@ScharfViktor how do we set the secrets for the collaboration service in CI?

we can set secrets here
https://drone.owncloud.com/owncloud/ocis/settings/secrets

and use it like:

"OCIS_JWT_SECRET": {
            "from_secret": "ocis_jwt_secret",
        },

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

micbar

Nice progress. Two nitpicks, but then we should merge it.

services/collaboration/README.md

Co-authored-by: Michael Barz <michael.barz@zeitgestalten.eu>

butonic · 2024-05-31T07:04:53Z

Flaky run https://drone.owncloud.com/owncloud/ocis/35538/42/6

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

sonarcloud · 2024-05-31T09:10:47Z

Quality Gate failed

Failed conditions
4 New Bugs (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

butonic · 2024-05-31T11:49:39Z

SonarCloud 'Bugs' were unrelated to this PR

collaboration fixes

wkloucek mentioned this pull request May 14, 2024

replace wopi server by collaboration service owncloud/ocis-charts#567

Draft

11 tasks

jvillafanez reviewed May 15, 2024

View reviewed changes

2403905 previously approved these changes May 22, 2024

View reviewed changes

butonic mentioned this pull request May 24, 2024

route /wopi to collaboration service #9148

Draft

wkloucek added 3 commits May 24, 2024 12:43

- let ocis init generate jwt token for collaboration service

901a5e2

- separate REVA jwt secret from WOPI jwt secret - fix gatway service name configuration

use reva gateway default address

2e5b57f

fix service namespaces

1e13072

butonic force-pushed the collaboration-fixes branch from 4fd692f to 7d916f7 Compare May 24, 2024 12:04

butonic reviewed May 24, 2024

View reviewed changes

butonic requested review from jvillafanez and 2403905 May 24, 2024 12:18

butonic force-pushed the collaboration-fixes branch 2 times, most recently from 1a77f37 to dd99a79 Compare May 24, 2024 12:26

incorporate feedback

955c195

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic force-pushed the collaboration-fixes branch from dd99a79 to 955c195 Compare May 24, 2024 12:53

butonic requested review from micbar, phil-davis and individual-it as code owners May 24, 2024 12:53

jvillafanez reviewed May 24, 2024

View reviewed changes

reduce config

ad76fac

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic force-pushed the collaboration-fixes branch 3 times, most recently from 16f2d52 to e9f9100 Compare May 30, 2024 11:26

set a sharted jwt in CI

2f14b24

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic force-pushed the collaboration-fixes branch from e9f9100 to 2f14b24 Compare May 30, 2024 11:54

butonic mentioned this pull request May 30, 2024

add the Collaboration stack debugging doc #9113

Open

9 tasks

micbar approved these changes May 30, 2024

View reviewed changes

services/collaboration/README.md Outdated Show resolved Hide resolved

services/collaboration/README.md Outdated Show resolved Hide resolved

micbar requested review from jvillafanez and butonic May 30, 2024 18:22

remove WOPI when talking about collaboration apps

2e3d9e1

Co-authored-by: Michael Barz <michael.barz@zeitgestalten.eu>

saw-jan mentioned this pull request May 31, 2024

[QA] Configure ocis collaboration service for e2e tests owncloud/web#10960

Open

5 tasks

incease sonar.ws.timeout from 60 to 120

53ac446

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic approved these changes May 31, 2024

View reviewed changes

add comment

6f7f05d

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic merged commit 3db878e into owncloud:master May 31, 2024
3 of 4 checks passed

ownclouders pushed a commit that referenced this pull request May 31, 2024

Merge pull request #9165 from wkloucek/collaboration-fixes

cd5294f

collaboration fixes

wkloucek deleted the collaboration-fixes branch May 31, 2024 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

collaboration fixes #9165

collaboration fixes #9165

wkloucek commented May 14, 2024 •

edited

update-docs bot commented May 14, 2024

wkloucek commented May 14, 2024

jvillafanez commented May 14, 2024

wkloucek commented May 14, 2024

butonic commented May 15, 2024

wkloucek commented May 15, 2024

jvillafanez May 15, 2024

wkloucek May 15, 2024

jvillafanez commented May 15, 2024

butonic May 24, 2024

butonic May 24, 2024

butonic May 24, 2024

butonic May 24, 2024

butonic May 24, 2024

butonic May 24, 2024 •

edited

butonic May 24, 2024

butonic commented May 24, 2024 •

edited

butonic commented May 24, 2024

jvillafanez May 24, 2024

butonic May 28, 2024

jvillafanez May 28, 2024

jvillafanez May 24, 2024

butonic May 28, 2024 •

edited

jvillafanez May 28, 2024

jvillafanez May 24, 2024

butonic May 28, 2024

ScharfViktor commented May 24, 2024

micbar left a comment

butonic commented May 31, 2024

sonarcloud bot commented May 31, 2024

butonic commented May 31, 2024

		JWTSecret string `yaml:"jwt_secret" env:"OCIS_JWT_SECRET;COLLABORATION_JWT_SECRET" desc:"Used as JWT token and to encrypt access token." introductionVersion:"5.1"`
		TokenManager *TokenManager `yaml:"token_manager"`

		WopiSrc string `yaml:"wopisrc" env:"COLLABORATION_WOPIAPP_WOPISRC" desc:"The WOPISrc base URL containing schema, host and port. Path will be set to /wopi/files/{fileid} if not given. {fileid} will be replaced with the WOPI file id. Set this to the schema and domain where the collaboration service is reachable for the wopi app, such as https://office.owncloud.test." introductionVersion:"5.1"`
		Secret string `yaml:"secret" env:"COLLABORATION_WOPIAPP_SECRET" desc:"Used to mint and verify WOPI JWT tokens and encrypt and decrypt the REVA JWT token embedded in the WOPI JWT token." introductionVersion:"5.1"`

collaboration fixes #9165

collaboration fixes #9165

Conversation

wkloucek commented May 14, 2024 • edited

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

update-docs bot commented May 14, 2024

wkloucek commented May 14, 2024

jvillafanez commented May 14, 2024

wkloucek commented May 14, 2024

butonic commented May 15, 2024

wkloucek commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic commented May 24, 2024 • edited

butonic commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScharfViktor commented May 24, 2024

micbar left a comment

Choose a reason for hiding this comment

butonic commented May 31, 2024

sonarcloud bot commented May 31, 2024

Quality Gate failed

butonic commented May 31, 2024

wkloucek commented May 14, 2024 •

edited

butonic May 24, 2024 •

edited

butonic commented May 24, 2024 •

edited

butonic May 28, 2024 •

edited