Skip to content

fix(sharing): Allow reasonable control for 'Hide download' on fed shares - #55251

Merged
nfebe merged 1 commit into
masterfrom
fix/federated-share-hide-download
Oct 23, 2025
Merged

fix(sharing): Allow reasonable control for 'Hide download' on fed shares#55251
nfebe merged 1 commit into
masterfrom
fix/federated-share-hide-download

Conversation

@nfebe

@nfebenfebe commented Sep 22, 2025

Copy link
Copy Markdown
Contributor

When creating public links from federated shares, users should be able to set the 'Hide download' option independently as long as they are more restrictive than the original share permissions.

Previously, the checkInheritedAttributes method was ignoring user preferences and always overriding the hideDownload setting based solely on inherited permissions, preventing users from disabling downloads even when the parent share allowed them.

This fix implements some sort of inheritance logic:

  • Users can only be MORE restrictive than parent shares, never LESS restrictive
  • If parent hides downloads -> child MUST hide downloads (enforced)
  • If parent allows downloads -> child can CHOOSE to hide or allow downloads
  • If parent forbids downloads entirely -> child cannot enable downloads

@nfebe
nfebe requested a review from a team as a code ownerSeptember 22, 2025 08:42
@nfebe
nfebe requested review from come-nc, icewind1991 and salmart-dev and removed request for a teamSeptember 22, 2025 08:42
@nfebe
nfebeforce-pushed the fix/federated-share-hide-download branch from a32def2 to 6c9b2e3CompareSeptember 22, 2025 08:43
@marcelklehr

marcelklehr commented Sep 22, 2025

Copy link
Copy Markdown
Member

Thank you for working on this! I think it would be good if the possibilities were also somehow visible in the UI. Is this the case with this already?

@nfebe

nfebe commented Sep 22, 2025

Copy link
Copy Markdown
ContributorAuthor

Thank you for working on this! I think it would be good if the possibilities were also somehow visible in the UI. Is this the case with this already?

This changes should solve the issue of the toggle not taking effect in the UI.

@susnuxsusnux added this to the Nextcloud 33 milestone Sep 24, 2025
artonge
artonge reviewed Sep 24, 2025
View reviewed changes
Comment threadapps/files_sharing/lib/Controller/ShareAPIController.php Outdated
Comment threadapps/files_sharing/lib/Controller/ShareAPIController.php
Comment threadapps/files_sharing/lib/Controller/ShareAPIController.php
Comment threadapps/files_sharing/lib/Controller/ShareAPIController.php
@artongeartonge changed the title fix(sharing): Allow reasonable control 4 'Hide download' on fed sharesfix(sharing): Allow reasonable control for 'Hide download' on fed sharesSep 24, 2025
@nfebe
nfebeforce-pushed the fix/federated-share-hide-download branch from 5c51164 to 8ed4c1aCompareOctober 9, 2025 07:47
@nfebe
nfebe requested a review from artongeOctober 9, 2025 07:47
artonge
artonge reviewed Oct 9, 2025
View reviewed changes

@artongeartonge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's have some tests to prevent any regressions

Comment threadapps/files_sharing/lib/Controller/ShareAPIController.php Outdated
@DaphneMuller

DaphneMuller commented Oct 13, 2025

Copy link
Copy Markdown
Contributor

@nfebe is there any chance this pr can be merged soon?

@nfebe
nfebeforce-pushed the fix/federated-share-hide-download branch 3 times, most recently from b50837d to 14298feCompareOctober 20, 2025 14:53
@nfebe
nfebe requested a review from artongeOctober 20, 2025 14:54
@nfebe

nfebe commented Oct 20, 2025

Copy link
Copy Markdown
ContributorAuthor

@nfebe is there any chance this pr can be merged soon?

Updated it and added some tests. (Sorry I have had some laptop downtime recently)

@nfebe
nfebeforce-pushed the fix/federated-share-hide-download branch from 14298fe to 02de83dCompareOctober 20, 2025 16:25
@nfebe
nfebeforce-pushed the fix/federated-share-hide-download branch from 02de83d to 32f92afCompareOctober 21, 2025 16:08
@nfebe
nfebe enabled auto-merge October 21, 2025 16:09
@nfebe
nfebeforce-pushed the fix/federated-share-hide-download branch from 32f92af to 712b0a3CompareOctober 21, 2025 23:18
@DaphneMuller

DaphneMuller commented Oct 23, 2025

Copy link
Copy Markdown
Contributor

@nfebe What is needed to merge this PR?

@nfebe
fix(sharing): Allow reasonable control 4 'Hide download' on fed shares
6401689
When creating public links from federated shares, users should be able to set
the 'Hide download' option independently as long as they are more restrictive
than the original share permissions.
Previously, the `checkInheritedAttributes` method was ignoring user preferences
and always overriding the hideDownload setting based solely on inherited
permissions, preventing users from disabling downloads even when the parent
share allowed them.
This fix implements some sort of inheritance logic:
- Users can only be MORE restrictive than parent shares, never LESS restrictive
- If parent hides downloads -> child MUST hide downloads (enforced)
- If parent allows downloads -> child can CHOOSE to hide or allow downloads
- If parent forbids downloads entirely -> child cannot enable downloads
Signed-off-by: nfebe <fenn25.fn@gmail.com>
@nfebe
nfebeforce-pushed the fix/federated-share-hide-download branch from 712b0a3 to 6401689CompareOctober 23, 2025 15:09
@nfebe
nfebe merged commit ed1647b into masterOct 23, 2025
222 of 230 checks passed
@nfebe
nfebe deleted the fix/federated-share-hide-download branch October 23, 2025 17:37
@DaphneMuller

DaphneMuller commented Oct 24, 2025

Copy link
Copy Markdown
Contributor

Thank you! 🎉

@maximelehericy

maximelehericy commented Nov 4, 2025

Copy link
Copy Markdown

@nfebe any chances to see that backported in 32 or not at all ?

@nfebe

nfebe commented Nov 4, 2025

Copy link
Copy Markdown
ContributorAuthor

/backport to stable32

@nfebe

nfebe commented Nov 4, 2025

Copy link
Copy Markdown
ContributorAuthor

/backport to stable31

@backportbotbackportbotBot mentioned this pull request Nov 4, 2025
Merged
@backportbotbackportbotBot mentioned this pull request Nov 4, 2025
Merged
@nextcloud-botnextcloud-bot mentioned this pull request Jan 6, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artongeartongeartonge approved these changes
@come-nccome-nccome-nc approved these changes
@icewind1991icewind1991Awaiting requested review from icewind1991icewind1991 is a code owner automatically assigned from nextcloud/server-backend
@salmart-devsalmart-devAwaiting requested review from salmart-devsalmart-dev is a code owner automatically assigned from nextcloud/server-backend
@ArtificialOwlArtificialOwlAwaiting requested review from ArtificialOwl

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbugfeature: sharing

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

7 participants

@nfebe@marcelklehr@DaphneMuller@maximelehericy@artonge@come-nc@susnux