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

Allow to disable system addressbook sync #13765

Closed
wants to merge 1 commit into from

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Jan 23, 2019

On some instances, users don't know each other and it doesn't makes
sense to expose users as contacts (through Contacts Menu, Calendar
Attendees,…) to everyone.

A parameter inside settings/admin/groupware allows to enable/disable the syncing of this system addressbook.

If this feature is disabled, the occ dav:sync-system-addressbook command empties the system addressbook (it's automatically refilled if the feature is activated again and synced).

  • Fix and add tests

@tcitworld tcitworld added the 2. developing Work in progress label Jan 23, 2019
@tcitworld tcitworld self-assigned this Jan 23, 2019
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

single quotes ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

@georgehrke
Copy link
Member

@schiessle Will this have any effect on federation?

Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

Can we have purgeSystemAddressBook and the SyncSystemAddressBook command covered by a unit test please? :)

Code-changes look good besides what @skjnldsv noted.

@jancborchardt
Copy link
Member

@tcitworld can you post a quick screenshot for review? :)

@georgehrke
Copy link
Member

c8ac8537-e646-4298-a56f-50d35385cbdd

@tcitworld
Copy link
Member Author

Tests added.

I'm waiting to see if #13796 or this one is merged first to adapt the settings section.

@tcitworld tcitworld added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 27, 2019
@georgehrke
Copy link
Member

#13796 was merged.

Can you please add "Also install the Contacts app, or connect your desktop & mobile for syncing ↗" similar to #13796 :)

@tcitworld tcitworld force-pushed the disable-systemaddressbook-sync branch from ae94278 to a9002a2 Compare February 1, 2019 12:09
@tcitworld
Copy link
Member Author

Just done it ! :)

@MorrisJobke
Copy link
Member

@schiessle Will this have any effect on federation?

Yes - this is basically how users are exchanged between trusted servers. If this is disabled then the users are not exchanged anymore.

@tcitworld
Copy link
Member Author

@MorrisJobke Do you want an extra warning then ?

@georgehrke

This comment has been minimized.

@tcitworld

This comment has been minimized.

@tcitworld
Copy link
Member Author

@MorrisJobke @georgehrke We now empty the addressbook instead of removing it, so that it doesn't break Federated Cloud Sharing.

@tcitworld tcitworld force-pushed the disable-systemaddressbook-sync branch from 10caa17 to 82d9fb6 Compare February 4, 2019 11:42
@MorrisJobke
Copy link
Member

@MorrisJobke @georgehrke We now empty the addressbook instead of removing it, so that it doesn't break Federated Cloud Sharing.

Still it empties the basic concept of what trusted servers rely on. I would like to have @schiessle's opinion here.

@tcitworld
Copy link
Member Author

@schiessle Can you have a look ⬆️

On some instances, users don't know each other and it doesn't makes
sense to expose users as contacts (through Contacts Menu, Calendar
Attendees,…) to everyone.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix and add tests

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Change settings template according to #13796

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Empty system addressbook instead of removing it

This makes Federated Cloud Sharing not fail when synching trusted
servers addressbooks (only synching an empty one, not bumping on an
non-existent one).

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix tests

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld tcitworld force-pushed the disable-systemaddressbook-sync branch from 82d9fb6 to bf2873e Compare February 28, 2019 11:09
@tcitworld
Copy link
Member Author

Rebased.

@tcitworld tcitworld added this to the Nextcloud 16 milestone Feb 28, 2019
This was referenced Mar 4, 2019
@schiessle
Copy link
Member

schiessle commented Mar 21, 2019

I'm know I'm late to this PR, sorry about that.

occ dav:sync-system-addressbook is used to sync users across servers. It syncs with all trusted servers. A trusted server relationship between two servers is created if both admins knowledge the other server as trusted, aka adding it to this list in the system settings (admin settings -> sharing):

image

If you don't want to sync user lists accross server you can simply disable the "Federation"-app.

But if I understand the initial post correctly:

On some instances, users don't know each other and it doesn't makes
sense to expose users as contacts (through Contacts Menu, Calendar
Attendees,…) to everyone.

The main concern are internal users exposed to each other. So it seems like some occ commands and features are mixed up here.

If you have a huge instance where not everyone should see everyone you could use the sharing admin setting "Restrict users to only share with users in their groups" to auto-complete only users in the same group. This way you can divide the users by groups.

If the user should be able to share with everyone as long as they know the complete uid but disable auto-completion and suggestions you can set

'sharing.maxAutocompleteResults' => 0,
in the config.php

Anyway, I would not provide a setting to kill the local address book completely because it might be used for many different things. Therefore I also don't know if "groupware" is the right category.

Sorry for the lengthy response and sorry if I misunderstand something.

@tcitworld
Copy link
Member Author

tcitworld commented Mar 21, 2019

Thank you for answering.

Sorry for not being clear enough, the issue was not really about the (Federated) Sharees API, but more about the OC\ContactsManager.

The original aim of this PR was to make only the user's addressbooks contacts available through ContactsMenu and calendar attendees.

I digged the code up and ended up here, but maybe it would be better to hook into OC\ContactsManager instead, by filtering with $addressBook->getUri(), so that the system addressbook is still available for other uses.

EDIT: But it seems OC\ContactsManager is called at a bunch of other places too, so that won't do it. 😞

@tcitworld
Copy link
Member Author

Closed until something good come out.

@tcitworld tcitworld closed this Mar 21, 2019
@MorrisJobke MorrisJobke deleted the disable-systemaddressbook-sync branch March 21, 2019 12:31
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 feature: dav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants