Skip to content

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
@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?

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 <blizzz@arthur-schiwon.de>
@elzody elzody force-pushed the fix/noid/share-fallback-internal-share branch from 1caa4f6 to c47507c Compare May 6, 2025 21:25
Copy link
Contributor

@elzody elzody left a comment

Choose a reason for hiding this comment

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

@blizzz I would assume it could need some backports?

@elzody elzody merged commit 307c09a into main May 6, 2025
68 checks passed
@elzody elzody deleted the fix/noid/share-fallback-internal-share branch May 6, 2025 21:38
@github-project-automation github-project-automation bot moved this from 🏗️ In progress to ☑️ Done in 📝 Office team May 6, 2025
@blizzz
Copy link
Member Author

blizzz commented May 7, 2025

/backport to stable31

@blizzz
Copy link
Member Author

blizzz commented May 7, 2025

/backport to stable30

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

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants