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

fix(CardDAV): allow disabling of the system address book #39925

Merged
merged 1 commit into from Sep 1, 2023

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Aug 17, 2023

via config option and OCC command

Summary

Allow admins to disable the System Addressbook systemwide.

This might cause complaints from connected clients if the SAB is the default addressbook in their env.

TODO

  • Documentation

Checklist

@miaulalala miaulalala self-assigned this Aug 17, 2023
@miaulalala miaulalala added bug 2. developing Work in progress feature: carddav Related to CardDAV internals 27-feedback labels Aug 17, 2023
@ChristophWurst
Copy link
Member

config:app:set/delete should be sufficient. we don't need dedicated commands right now: https://docs.nextcloud.com/server/stable/admin_manual/configuration_server/occ_command.html#config-commands

@miaulalala
Copy link
Contributor Author

/backport to stable27

@miaulalala miaulalala marked this pull request as ready for review August 18, 2023 06:13
@miaulalala miaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 18, 2023
Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

I feel like just adding an if to the above block where the system address book is added in the first place would be much more efficient, why not do that instead?

(in any case, you need to get the getAppValue call outside of the array_map so that it doesn't get called for each iteration)

Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

Code is fine but I didn't check if that has other consequences elsewhere.

apps/dav/lib/CardDAV/UserAddressBooks.php Outdated Show resolved Hide resolved
@miaulalala
Copy link
Contributor Author

Failing cypress test is unrelated methinks:

  Login with a new user and open the files app
    1) "after all" hook for "See the default file welcome.txt in the files list"


  0 passing (7s)
  1 failing

  1) Login with a new user and open the files app
       "after all" hook for "See the default file welcome.txt in the files list":
     CypressError: The following error originated from your application code, not from Cypress. It was caused by an unhandled promise rejection.

…ption

Signed-off-by: Anna Larch <anna@nextcloud.com>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 1, 2023
@skjnldsv skjnldsv merged commit b0fae95 into master Sep 1, 2023
34 of 38 checks passed
@mbiebl
Copy link

mbiebl commented Sep 1, 2023

Applied this patch and everything is working nicely again after disabling the SAB. Thanks a lot!

@sudoer
Copy link

sudoer commented Sep 1, 2023

I was able to comment out this block of code, and I saw that the "Accounts" address book was no longer exported, as we had hoped. So I am looking forward to the configuration-based fix soon.

I did have a question. When this pull request is merged, how would someone set the configuration option? Is it in a config file? Or is there an "occ" command to set the option? Is it eventually stored in the database, or in the code? I imagine this question is a bit outside of the scope of this PR, but it sure would help to have a pointer to how to enable/disable it. Thanks!

@tcitworld
Copy link
Member

php occ config:app:set dav system_addressbook_exposed --value=no

@MicKress
Copy link

MicKress commented Sep 4, 2023

Hi, I have a question. Will the deactivation of system address book only be possible via occ-Command or will there be an point in the administration panel? If you have a managed nextcloud, you cannot often work with occ-commands. Best Regards

@exuded
Copy link

exuded commented May 13, 2024

Hello, when using the command given in the documentation, I get the following error:
CleanShot 2024-05-13 at 11 56 35

I am a bit confused because in the command is no "-a" part

@tcitworld
Copy link
Member

@exuded Please ask for help on the forum or open your own issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish 27-feedback bug feature: carddav Related to CardDAV internals
Projects
Development

Successfully merging this pull request may close these issues.

Provide a way to hide / remove the system address book globally
9 participants