Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collaboration short tokens #10391

Merged
merged 9 commits into from
Oct 24, 2024
Merged

Collaboration short tokens #10391

merged 9 commits into from
Oct 24, 2024

Conversation

jvillafanez
Copy link
Member

Description

Generate short tokens to be used as access tokens for WOPI.

Currently, the access tokens being used might be too long. In particular, Office Online complains about the URL (which contains the access token) is too long and refuses to work.

A new config option COLLABORATION_WOPI_SHORTTOKENS has been added in order to overcome that limitation. Note that this option can be used with any WOPI application (Collabora, OnlyOffice, etc)
In order to use this option, you must configure a store so OCIS can store the access tokens.

By default, the store will be configured to connect to "127.0.0.1:9233", which is where the nats service should be listening. If you have the collaboration in a different container, you probably want to connect to a different container such as "ocis:9233".
In addition, the TTL is configured to 30 minutes. You can rise the value if needed, but lowering it will likely cause problems because valid tokens will be deleted prematurely.

To sum up, if you enable the COLLABORATION_WOPI_SHORTTOKENS, make sure either OCIS_PERSISTENT_STORE_NODES or COLLABORATION_STORE_NODES are configured properly to store the tokens.

Related Issue

https://github.com/owncloud/enterprise/issues/6790

Motivation and Context

The URL is too long with the access token and we need to make it shorter.

How Has This Been Tested?

Manually tested with Collabora. The access token has around 48 chars.

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:

@jvillafanez jvillafanez self-assigned this Oct 23, 2024
Copy link

update-docs bot commented Oct 23, 2024

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.

@micbar
Copy link
Contributor

micbar commented Oct 23, 2024

@jvillafanez
Thanks for the improvement.

I tested it in a test environment with MS365 and MSOO. It works perfectly fine.

With this improvement, the online WOPI validator is running also.

I could find a small bug in PutRelative. Will issue a new PR.

See #10403

The "real" access token will be stored using the short token as key.
This short token will be sent to the clients to be used as access token
for the WOPI server.

This is configurable, and requires a store in order to keep the tokens.
MD5 will make the token shorter. The timestamp will help to prevent
collisions since the tokens must be generated at the same nanosecond
(assuming the md5 sum generates the same hash, which is unlikely).

Using MD5 shouldn't be a security issue. The "real" access token is
already encrypted, and it's visible and accessible if short tokens
aren't used.
@jvillafanez jvillafanez force-pushed the collaboration_short_tokens branch from b9c9775 to d5671f8 Compare October 24, 2024 06:52
@micbar
Copy link
Contributor

micbar commented Oct 24, 2024

After reading the whole code, i have one basic question.

In the proxy, we are also using nats to cache the access tokens for OIDC. There we are using a go micro cache, not a store.

IMHO when running on a single node, we should use the memory.

@rhafer @kobergj do you agree?

@micbar
Copy link
Contributor

micbar commented Oct 24, 2024

After reading the whole code, i have one basic question.

In the proxy, we are also using nats to cache the access tokens for OIDC. There we are using a go micro cache, not a store.

IMHO when running on a single node, we should use the memory.

@rhafer @kobergj do you agree?

Clarified after re-thinking:

@jvillafanez your implementation is correct, the store needs to be persistent to survive a reboot.

@mmattel
Copy link
Contributor

mmattel commented Oct 24, 2024

@jvillafanez
As you have added a store for the collaboration service, please update readme like we have in https://github.com/owncloud/ocis/tree/master/services/postprocessing#storing-postprocessing-data

You just can copy/paste the block into the collaboration readme and adapt it slightly to fit.

We should also add some content in the readme about the problem of long URL's and its solution.

@jvillafanez jvillafanez force-pushed the collaboration_short_tokens branch 2 times, most recently from d161def to 8221b29 Compare October 24, 2024 07:51
@jvillafanez jvillafanez force-pushed the collaboration_short_tokens branch from 8221b29 to b3f72d7 Compare October 24, 2024 07:57
@mmattel
Copy link
Contributor

mmattel commented Oct 24, 2024

@jvillafanez I need to insist asking if COLLABORATION_WOPI_SHORTTOKENS is boolean and if it defaults to false. If yes, nothing need to be done. If not, we need a default and an explanation which setting to take.

@mmattel
Copy link
Contributor

mmattel commented Oct 24, 2024

@jp, @micbar I have added some content to the collaboration readme via a commit.
In prep, I have done the same already to the admin docs (waiting for this PR to get merged).

@jvillafanez
Copy link
Member Author

Looks good to me. @mmattel if there is no additional typo, you can change the PR to ready to review.

@mmattel mmattel marked this pull request as ready for review October 24, 2024 09:15
@mmattel mmattel requested review from micbar and kobergj October 24, 2024 09:15
Nodes: []string{"127.0.0.1:9233"},
Database: "collaboration",
Table: "",
TTL: 30 * time.Minute,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvillafanez How does the WebOffice app refreshes the token?
I am aware of the 30 min, but i never saw a Web Office actually refresh the WOPI token.

Copy link
Member Author

@jvillafanez jvillafanez Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any info on how the refresh is supposed to work. What I found is in https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/concepts#the-access_token_ttl-property

To prevent data loss for users, Office for the web prompts 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 expires, which it determines based on the access_token_ttl value.

So I guess you're expected to close the editor and reopen it (with a fresh token)

import "time"

// Store configures the store to use
type Store struct {
Copy link
Contributor

@micbar micbar Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These variables should have the introductionVersion 7.0.0.

@mmattel
Copy link
Contributor

mmattel commented Oct 24, 2024

There is no changelog, I will add one.

Copy link

sonarcloud bot commented Oct 24, 2024

@mmattel mmattel merged commit 52d965f into master Oct 24, 2024
4 checks passed
@mmattel mmattel deleted the collaboration_short_tokens branch October 24, 2024 13:59
ownclouders pushed a commit that referenced this pull request Oct 24, 2024
@micbar micbar mentioned this pull request Nov 8, 2024
87 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants