Skip to content

fix(Wopi): fall back to super share if share token is not available #4712

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Apr 28, 2025

  • Target version: main

Summary

On internal shares the controller is called without the share token. But necessary information, like share attributes, might be necessary to know and are available from the super share of the SharedMount in that case.

This can be testes as following:

  1. In Admin settings → Office enable Secure view
  2. and there tick Show watermark for shares without download permission (but no other if its options!)
  3. Share a document with an internal user as read-only and without download-privileges
  4. As this user, open the document

It is expected that the document is opened with the watermark, but it does not. This is because we do not store (for we do not know) the share token of the document in DocumentController::token(). Hence we try now in the WopiController whether the share token is present and otherwise fall back to the super share.

The super share combines any relevant present share. While some field stay empty (shareWith, shareToken, as they cannot be combined) the share attributes are. So if there would be another share of this document with the user, that does have download permissions, then the watermarking would not happen, as expected.

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required

@blizzz blizzz requested a review from a team April 28, 2025 13:55
@blizzz blizzz added bug Something isn't working 3. to review Ready to be reviewed labels Apr 28, 2025
@github-project-automation github-project-automation bot moved this to 🧭 Planning evaluation (don't pick) in 📝 Office team Apr 28, 2025
@blizzz blizzz moved this from 🧭 Planning evaluation (don't pick) to 👀 In review in 📝 Office team Apr 28, 2025
@blizzz blizzz force-pushed the fix/noid/share-fallback-internal-share branch from 89ef31c to ea2be66 Compare April 28, 2025 14:01
@blizzz
Copy link
Member Author

blizzz commented Apr 28, 2025

Only now I see that you are doing the almost same in other places, actually, but with SharedStorage instead of SharedMount. Do you want me to switch?

@juliusknorr
Copy link
Member

Only now I see that you are doing the almost same in other places, actually, but with SharedStorage instead of SharedMount. Do you want me to switch?

Shouldn't make much of a difference, right? At least I cannot see any obvious one. I'd be fine either way but prefer consistency

@blizzz
Copy link
Member Author

blizzz commented Apr 29, 2025

Only now I see that you are doing the almost same in other places, actually, but with SharedStorage instead of SharedMount. Do you want me to switch?

Shouldn't make much of a difference, right? At least I cannot see any obvious one. I'd be fine either way but prefer consistency

No difference, but on the same line with consistency. I'll update then.

@blizzz blizzz added 2. developing Work in progress and removed 3. to review Ready to be reviewed labels Apr 29, 2025
@blizzz blizzz moved this from 👀 In review to 🏗️ In progress in 📝 Office team Apr 29, 2025
@juliusknorr
Copy link
Member

Rebase should have passing CI then as well

@blizzz blizzz force-pushed the fix/noid/share-fallback-internal-share branch from ea2be66 to d95f68c Compare April 30, 2025 09:53
@blizzz blizzz requested a review from juliusknorr April 30, 2025 09:53
On internal shares the controller is called without the share token. But
necessary information, like share attributes, might be necessary to know
and are available from the super share of the SharedStorage in that case.

For this approach was used elsewhere, too, some repetitive code was
consolidated in the Helper class.

Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz blizzz force-pushed the fix/noid/share-fallback-internal-share branch from d95f68c to 1caa4f6 Compare April 30, 2025 10:00
@blizzz
Copy link
Member Author

blizzz commented Apr 30, 2025

Also consolidated the handling of getting the share a bit across the occurrences.

@blizzz
Copy link
Member Author

blizzz commented Apr 30, 2025

Cypress still keeps failing though. Is that what should have been fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug Something isn't working
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

2 participants