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.

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

Add a post_user_delete hook #578

Merged
merged 6 commits into from
Nov 10, 2023
Merged

Add a post_user_delete hook #578

merged 6 commits into from
Nov 10, 2023

Conversation

Tagadda
Copy link
Member

@Tagadda Tagadda commented Jun 4, 2023

Problem

Solution

  • Create a hook that deletes users from the DB

PR Status

  • Code finished and ready to be reviewed/tested
  • The fix/enhancement were manually tested (if applicable)

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

@ericgaspar ericgaspar requested review from kay0u and JimboJoe June 4, 2023 09:48
@oleole39
Copy link

oleole39 commented Jun 4, 2023

Thanks a lot for this. I was working on the same point and at the point of forking the repo I notice your fresh PR.
I've tested manually your fix with the following process:

  • add a test user
  • grant it access to Nextcloud
  • Associate an email account (the default associated YNH mail) to this Nextcloud account in Nextcloud Mail
  • Log out from YNH SSO & Nextcloud
  • Copying the new hook file (the one you created with this PR) in the hooks' app folder.
  • restart YNH server (just in case)
  • go to YNH admin panel and delete the test user account
  • sudo -u nextcloud php8.1 --define apc.enable_cli=1 occ user:list shows the user is deleted (although for some reason the same result was already reached without the new hook)
  • check the table oc_mail_accounts in nextcloud MySQL database, the deleted user's mail account info are still there.

Probably additional commands such as occ mail:account:delete (to delete the account) and mail:clean-up (to remove data related to it) needs to be run in the hook (cf. https://help.nextcloud.com/t/how-to-have-nextcloud-mail-stop-retrieving-emails-for-an-account-which-was-not-fully-removed/162587/2). I wonder if this can be considered as a Nextcloud bug (or Nextcloud apps bug) - should user:delete delete all data associated to the user in all Nextcloud apps?

@Tagadda
Copy link
Member Author

Tagadda commented Jun 4, 2023

Copying the new hook file (the one you created with this PR) in the hooks' app folder.

Where did you copy it to ? "app folder" sounds like /etc/yunohost/apps/nextcloud/hooks/, but that's not correct. Hooks live at /etc/yunohost/hooks.d/ (cf. documentation)
Here, you'll need to copy this hook to /etc/yunohost/hooks.d/post_user_delete/50-nextcloud

Anyway, I tested this PR and the table oc_mail_accounts is empty after I removed the user 🚀

@oleole39
Copy link

oleole39 commented Jun 5, 2023

That was indeed my mistake. I did a new test with the file in folder you indicate and it works very well.
Just noticing that it still doesn't delete the reference to "testuser" in the following tables (probably not a big deal though):

  • oc_addressbookchanges (would it remain here for compatibility issue for remaining users - to keep seeing that a user of that name existed at some point?)
  • oc_jobs (jobs history, no need to delete)
  • oc_mail_tags (not sure why it remains here)
  • oc_notifications_settings (not sure why it remains here)
  • oc_storages (not sure why it remains here)

@oleole39
Copy link

oleole39 commented Jun 7, 2023

Just noticing that it still doesn't delete the reference to "testuser" in the following tables (probably not a big deal though):

It could apparently be related to Nextcloud (Server part and Mail & Notifications apps) more than to the Yunohost package.
nextcloud/server#38680
nextcloud/notifications#1572

@oleole39
Copy link

It could apparently be related to Nextcloud (Server part and Mail & Notifications apps) more than to the Yunohost package. nextcloud/server#38680 nextcloud/notifications#1572

@ericgaspar ericgaspar deleted the branch testing August 12, 2023 13:42
@ericgaspar ericgaspar closed this Aug 12, 2023
@ericgaspar ericgaspar reopened this Aug 12, 2023
@ericgaspar
Copy link
Member

!testme

@yunohost-bot
Copy link
Contributor

Fingers crossed!
Test Badge

@ericgaspar
Copy link
Member

any thoughts. shall we merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants