Skip to content

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Aug 12, 2024

Summary

When removing a user from a group, remove shares which would not be creatable anymore according to group limit on sharing.

Checklist

@come-nc come-nc added the 2. developing Work in progress label Aug 12, 2024
@come-nc come-nc added this to the Nextcloud 30 milestone Aug 12, 2024
@come-nc come-nc self-assigned this Aug 12, 2024
@come-nc come-nc force-pushed the fix/apply-group-limit-on-remove-from-group branch from c57bb08 to f217c6e Compare August 12, 2024 12:44
@come-nc
Copy link
Contributor Author

come-nc commented Aug 12, 2024

/backport to stable29

@come-nc
Copy link
Contributor Author

come-nc commented Aug 12, 2024

/backport to stable28

@come-nc come-nc force-pushed the fix/apply-group-limit-on-remove-from-group branch from 2744f31 to 244491a Compare August 12, 2024 13:22
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 13, 2024
@come-nc come-nc requested review from a team, ArtificialOwl, provokateurin and yemkareems and removed request for a team August 13, 2024 08:08
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@come-nc come-nc merged commit 142b6e3 into master Aug 13, 2024
@come-nc come-nc deleted the fix/apply-group-limit-on-remove-from-group branch August 13, 2024 10:20
@hamza221
Copy link
Contributor

/backport to stable26

@jonas2515
Copy link

Tbh this seems not very thought through, it makes the behavior of existing shares on settings and group membership changes super inconsistent.

While now existing shares are removed when manipulating the groups of a user, they aren't removed when enabling the "only share between users of group" setting. Neither are they removed when disabling the whole share API, nor when changing the "limit/exclude sharing to/from groups" settings. I assume the behavior for re-shared shares would also have to be considered.

I don't think a simple action like toggling group membership (or toggling a checkbox in settings) should do such destructive things as deleting shares, can we maybe just remove this behavior again?

@come-nc ping just in case you don't receive notifications for merged PRs.

@come-nc
Copy link
Contributor Author

come-nc commented Nov 3, 2025

While now existing shares are removed when manipulating the groups of a user, they aren't removed when enabling the "only share between users of group" setting. Neither are they removed when disabling the whole share API, nor when changing the "limit/exclude sharing to/from groups" settings. I assume the behavior for re-shared shares would also have to be considered.

I don't think a simple action like toggling group membership (or toggling a checkbox in settings) should do such destructive things as deleting shares, can we maybe just remove this behavior again?

I think it’s for this reason that changing settings is not directly deleting shares. But it would be a nice feature to have a button to apply current settings to existing shares indeed.

But this PR is not when changing settings but when changing groups.
It’s fixing what was reported as a security issue, see GHSA-35gc-jc6x-29cm
When configuring sharing restriction based on groups, it should be expected that removing a user from a group has consequences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants