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.

Sign up for GitHub

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

Jump to bottom

Hide shares by disabled users #39699

Merged
merged 3 commits into from
Aug 28, 2023
Merged

Hide shares by disabled users #39699

merged 3 commits into from
Aug 28, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Aug 3, 2023 •
edited
Loading

Summary

  1. Use occ config:app:set files_sharing hide_disabled_user_shares --value yes to enable
  2. Once enabled shares by disabled user will appear as non-existing
  3. Shares of files owned by disabled users will disappear as well, even if shared by someone else
  4. Compatible with advanced option from user_ldap to disable users missing from LDAP

Checklist

@come-nc come-nc added enhancement 3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update labels Aug 3, 2023
@come-nc come-nc added this to the Nextcloud 28 milestone Aug 3, 2023
@come-nc come-nc self-assigned this Aug 3, 2023
@come-nc come-nc changed the title Set files_sharing:hide_disabled_user_shares to 'yes' to hide shares from disabled users Hide shares by disabled users Aug 3, 2023
@come-nc come-nc force-pushed the enh/hide-shares-from-disabled-users branch 2 times, most recently from 6baadbe to ee34b90 Compare August 7, 2023 15:09
@come-nc come-nc requested review from a team, ArtificialOwl, icewind1991, Fenn-CS, susnux, Altahrim and artonge and removed request for a team August 7, 2023 15:10
Altahrim
Altahrim approved these changes Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@Altahrim Altahrim left a comment

Choose a reason for hiding this comment

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

My comment is not a blocker and will probably be addressed in another PR

@come-nc come-nc mentioned this pull request Aug 14, 2023
Merged
icewind1991
icewind1991 reviewed Aug 14, 2023
View reviewed changes
tests/lib/Share20/ManagerTest.php
@@ -2714,10 +2714,12 @@ public function testGetSharesByExpiredLinkShares() {

public function testGetShareByToken() {
$this->config
->expects($this->once())
->expects($this->exactly(2))
Copy link
Member

Choose a reason for hiding this comment

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

I prefer just removing these counts altogether as they don't actually contribute to the test.

@icewind1991
Copy link
Member

What is the reasoning for not doing this by default?

@AndyScherzinger
Copy link
Member

@come-nc I think having it in 27.1 would be nice to have some (more) upgrade motivation for the requesting party, no?

@come-nc
Copy link
Contributor Author

come-nc commented Aug 28, 2023

What is the reasoning for not doing this by default?

Avoid a behavior change on update which will not please everyone.
Most people want to disable users to prevent them from logging and do not want to lose access to their shared data.

@come-nc
Copy link
Contributor Author

come-nc commented Aug 28, 2023

@come-nc I think having it in 27.1 would be nice to have some (more) upgrade motivation for the requesting party, no?

Not my call, if you think it fits and 27.1 is still open we can backport.

come-nc and others added 3 commits August 28, 2023 09:40
@come-nc
Set files_sharing:hide_disabled_user_shares to 'yes' to hide shares f…
8c9aa9a 
…rom disabled users

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix tests, add test for the new feature
fd0de0a 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @Altahrim
Use nullsafe call syntax instead of additionnal check
24ad2e2 
Co-authored-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
Signed-off-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
@come-nc come-nc force-pushed the enh/hide-shares-from-disabled-users branch from 943cc1c to 24ad2e2 Compare August 28, 2023 07:40
@AndyScherzinger
Copy link
Member

27.1 is still open we can backport.

yes, still open and fine to backport 👍

@AndyScherzinger
Copy link
Member

/backport to stable27

@come-nc come-nc merged commit ac3d7e3 into master Aug 28, 2023
38 checks passed
@come-nc come-nc deleted the enh/hide-shares-from-disabled-users branch August 28, 2023 13:33
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Aug 28, 2023
Merged
@come-nc
Copy link
Contributor Author

come-nc commented Sep 7, 2023

/backport to stable23

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable23
git pull origin stable23

# Create the new backport branch
git checkout -b fix/foo-stable23

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable23

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@come-nc come-nc mentioned this pull request Sep 7, 2023
Merged
@come-nc
Copy link
Contributor Author

come-nc commented Sep 18, 2023

/backport to stable26

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Sep 18, 2023
Merged
@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Nov 13, 2023
@come-nc come-nc mentioned this pull request Nov 13, 2023
Closed
2 tasks
This was referenced Feb 22, 2024
Merged
Merged
@joshtrichards joshtrichards mentioned this pull request Aug 18, 2024
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@icewind1991 icewind1991 icewind1991 left review comments

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@Altahrim Altahrim Altahrim approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@Fenn-CS Fenn-CS Awaiting requested review from Fenn-CS Fenn-CS was automatically assigned from nextcloud/server-backend

@susnux susnux Awaiting requested review from susnux

@artonge artonge Awaiting requested review from artonge

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

Disable shares for deactivated users
6 participants
@come-nc @icewind1991 @AndyScherzinger @nickvergessen @susnux @Altahrim