feat(dav): store scopes for properties and filter locally scoped properties for federated address book sync #38048

miaulalala · 2023-05-03T17:31:00Z

Resolves: Store local properties in system addressbook too #38021

Summary

Adds a users property scopes to the system address book. Private properties are not written at all.
When syncing with a federated share, the locally scoped properties should not be shared with the remote host.

Todo

Store scopes for properties
Write local properties to system address book too
Filter locally scoped properties for federated address book sync
Repair step to add scopes to existing card data
CardDav sync token adjustment for cards inaccessible to federated instances

How to test

Needs a federated instance setup

Change a property on a local user to be scope local only
trigger address book sync from remote host
check the db for the changed contact by looking into the second system address book.
Before

BEGIN:VCARD
VERSION:3.0
PRODID:-//Sabre//Sabre VObject 4.4.2//EN
UID:admin
FN;X-NC-SCOPE=v2-federated:admin
N;X-NC-SCOPE=v2-federated:admin;;;;
ADR;TYPE=OTHER;X-NC-SCOPE=v2-local:Testing test test test;;;;;;
EMAIL;TYPE=OTHER;X-NC-SCOPE=v2-federated:miau_lalala@gmx.net
TEL;TYPE=OTHER;X-NC-SCOPE=v2-local:+435454454544
CLOUD:admin@http://localhost
END:VCARD

After

BEGIN:VCARD
VERSION:3.0
PRODID:-//Sabre//Sabre VObject 4.4.2//EN
UID:admin
FN;X-NC-SCOPE=v2-federated:admin
N;X-NC-SCOPE=v2-federated:admin;;;;
EMAIL;TYPE=OTHER;X-NC-SCOPE=v2-federated:miau_lalala@gmx.net
CLOUD:admin@http://localhost
END:VCARD

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

apps/dav/lib/CardDAV/UserAddressBooks.php

ChristophWurst

👍 code looks very good

ChristophWurst · 2023-05-03T18:28:29Z

Only one remark. Existing users with local properties don't get a trigger to update their vcards, right? Only when a user property changes we regenerate the system card

ChristophWurst

Tested with my database user. Set all props to local and now I suddenly have an entry in the system addres book. All props are scoped to v2-local 👍

Didn't test federation

miaulalala · 2023-05-03T18:32:34Z

Only one remark. Existing users with local properties don't get a trigger to update their vcards, right? Only when a user property changes we regenerate the system card

Yes that is indeed an issue, everything that already exists and doesn't update is gonna stay the same. Maybe a repair step could help?

ChristophWurst · 2023-05-03T18:37:24Z

\OCA\DAV\CardDAV\SyncService::syncInstance can do the full instance update. Repair step would run for every future migration. This is a one-time thing. A migration without db schema feels wrong too. So yeah, repair step, but you have to add a flag to oc_preferences whether users were updated or not.

apps/dav/lib/CardDAV/SystemAddressbook.php

apps/dav/lib/Migration/Version1027Date20230504122946.php

ChristophWurst · 2023-05-04T13:32:46Z

Is this generally ready to review?

apps/dav/lib/CardDAV/Converter.php

tcitworld · 2023-05-05T07:27:39Z

At this point there's hopefully a way to factorize things in SystemAddressbook.php.

apps/dav/lib/CardDAV/SystemAddressbook.php

ChristophWurst · 2023-05-05T11:46:41Z

To be sure the filtered sync changes work as expected I'd like to throw DAVxc5 and Thunderbird against the federated address book using system:sharedsecret. That should work, right?

miaulalala · 2023-05-05T11:48:41Z

To be sure the filtered sync changes work as expected I'd like to throw DAVxc5 and Thunderbird against the federated address book using system:sharedsecret. That should work, right?

If you know the shared secret that should work, yes.

ChristophWurst · 2023-05-05T12:59:46Z

I'm struggling to connect to the collection with the system user. ~~I have a debugger attached at \OCA\Federation\DAV\FedAuth::validateUserPass but it doesn't go there apparently.~~ It does but I can't list the system user address book home nor the SAB

Could not access /remote.php/dav/addressbooks/users/system/system/ (not WebDAV-enabled?):
404 Not Found

Edit: had the wrong URL

Authentication required for Nextcloud on server `localhost':
Username: system
Password: 
dav:/remote.php/dav/addressbooks/system/system/system/> ls
Listing collection `/remote.php/dav/addressbooks/system/system/system/': collection is empty.

miaulalala · 2023-05-05T13:18:35Z

drone fails because of merge conflict

apps/dav/lib/CardDAV/Converter.php

apps/dav/lib/CardDAV/SystemAddressbook.php

+		}
+
+		if (!$this->carddavBackend instanceof SyncSupport) {
+			return null;


apps/dav/lib/CardDAV/SystemAddressbook.php

+			throw new UnsupportedLimitOnInitialSyncException();
+		}
+
+		if (!$this->carddavBackend instanceof SyncSupport) {


ChristophWurst · 2023-05-08T08:27:12Z

Listing collection `/remote.php/dav/addressbooks/system/system/system/': collection is empty.

Unintentionally tested the sharing enumeration restriction. Works 👍

Without it, I finally get my system contacts:

dav:/remote.php/dav/addressbooks/system/system/system/> ls
Listing collection `/remote.php/dav/addressbooks/system/system/system/': succeeded.
        Database:admin.vcf                   515  May  4 20:01
        Database:asdfasdfdasfsdfdsfasd.vcf        248  May  4 19:46
        Database:asdfasfasfasdf.vcf          259  May  4 20:26

My user's location is only shown in the downloaded .vcf if set to federated or published. If set to local it's not readable for the remote instance 👍

ChristophWurst

I let davx5 play federated instance and sync the addressbook. Works on master, then switched to this branch, rebuilt the SAB and synced again. No local props show up on the phone 👍

ChristophWurst · 2023-05-09T13:15:43Z

Fixed the static analysis issue with nullable changes. Even tried to fix upstream but the usual suspect was faster: sabre-io/dav@9b10ec6

apps/dav/lib/CardDAV/SystemAddressbook.php

+	/**
+	 * @throws UnsupportedLimitOnInitialSyncException
+	 */
+	public function getChanges($syncToken, $syncLevel, $limit = null) {


…erties for federated address book sync Signed-off-by: Anna Larch <anna@nextcloud.com>

ChristophWurst · 2023-05-10T09:05:32Z

contacts menu integration test fails reliably -> merge

miaulalala self-assigned this May 3, 2023

miaulalala added 2. developing Work in progress feature: federation feature: carddav Related to CardDAV internals labels May 3, 2023

miaulalala commented May 3, 2023

View reviewed changes

apps/dav/lib/CardDAV/UserAddressBooks.php Outdated Show resolved Hide resolved

miaulalala changed the title ~~Filter locally scoped properties for federated address book sync~~ Store sopes for properties and filter locally scoped properties for federated address book sync May 3, 2023

miaulalala changed the title ~~Store sopes for properties and filter locally scoped properties for federated address book sync~~ Store scopes for properties and filter locally scoped properties for federated address book sync May 3, 2023

miaulalala requested review from st3iny, skjnldsv and hamza221 May 3, 2023 17:45

ChristophWurst reviewed May 3, 2023

View reviewed changes

ChristophWurst approved these changes May 3, 2023

View reviewed changes

ChristophWurst requested a review from JohannesGGE May 3, 2023 18:30

miaulalala force-pushed the enh/add-x-nc-scope-property branch from 98e22d2 to 5b85828 Compare May 3, 2023 18:43

github-advanced-security bot found potential problems May 3, 2023

View reviewed changes

apps/dav/lib/CardDAV/SystemAddressbook.php Fixed Show fixed Hide fixed

apps/dav/lib/CardDAV/SystemAddressbook.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 4, 2023

View reviewed changes

apps/dav/lib/CardDAV/SystemAddressbook.php Fixed Show fixed Hide fixed

ChristophWurst reviewed May 4, 2023

View reviewed changes

apps/dav/lib/Migration/Version1027Date20230504122946.php Outdated Show resolved Hide resolved

miaulalala changed the title ~~Store scopes for properties and filter locally scoped properties for federated address book sync~~ feat(dav): store scopes for properties and filter locally scoped properties for federated address book sync May 4, 2023

ChristophWurst mentioned this pull request May 4, 2023

feat(carddav): Map user's additional emails into the SAB card #38073

Merged

4 tasks

ChristophWurst approved these changes May 4, 2023

View reviewed changes

ChristophWurst marked this pull request as ready for review May 4, 2023 15:16

ChristophWurst requested a review from tcitworld as a code owner May 4, 2023 15:16

ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 4, 2023

ChristophWurst reviewed May 4, 2023

View reviewed changes

apps/dav/lib/CardDAV/Converter.php Show resolved Hide resolved

JohannesGGE approved these changes May 4, 2023

View reviewed changes

ChristophWurst reviewed May 5, 2023

View reviewed changes

apps/dav/lib/CardDAV/SystemAddressbook.php Outdated Show resolved Hide resolved

ChristophWurst mentioned this pull request May 5, 2023

feat(dav): expose system address book #37734

Merged

5 tasks

github-advanced-security bot found potential problems May 8, 2023

View reviewed changes

ChristophWurst force-pushed the enh/add-x-nc-scope-property branch from 5adc674 to 92ddc95 Compare May 8, 2023 07:52

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

ChristophWurst approved these changes May 9, 2023

View reviewed changes

ChristophWurst force-pushed the enh/add-x-nc-scope-property branch from f0303d3 to 6bf009b Compare May 9, 2023 13:10

github-advanced-security bot found potential problems May 9, 2023

View reviewed changes

feat(dav): store scopes for properties and filter locally scoped prop…

bd80a1b

…erties for federated address book sync Signed-off-by: Anna Larch <anna@nextcloud.com>

ChristophWurst force-pushed the enh/add-x-nc-scope-property branch from 9f30e1f to bd80a1b Compare May 9, 2023 17:19

ChristophWurst enabled auto-merge May 9, 2023 17:32

ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 9, 2023

ChristophWurst mentioned this pull request May 10, 2023

feat(devmanual): Add user managers doc nextcloud/documentation#10355

Merged

ChristophWurst disabled auto-merge May 10, 2023 09:05

ChristophWurst merged commit 1fd8f41 into master May 10, 2023

ChristophWurst deleted the enh/add-x-nc-scope-property branch May 10, 2023 09:05

ChristophWurst mentioned this pull request May 10, 2023

feat(users): Store and load a user's manager #38013

Merged

10 tasks

skjnldsv mentioned this pull request May 10, 2023

27.0.0 beta 2 #38139

Merged

ChristophWurst mentioned this pull request May 22, 2023

Contacts menu doesn't list users with users that set their display name to "local only" #10317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dav): store scopes for properties and filter locally scoped properties for federated address book sync #38048

feat(dav): store scopes for properties and filter locally scoped properties for federated address book sync #38048

miaulalala commented May 3, 2023 •

edited by ChristophWurst

Loading

ChristophWurst left a comment

ChristophWurst commented May 3, 2023

ChristophWurst left a comment

miaulalala commented May 3, 2023

ChristophWurst commented May 3, 2023

ChristophWurst commented May 4, 2023

tcitworld commented May 5, 2023

ChristophWurst commented May 5, 2023

miaulalala commented May 5, 2023

ChristophWurst commented May 5, 2023 •

edited

Loading

miaulalala commented May 5, 2023

ChristophWurst commented May 8, 2023

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

ChristophWurst left a comment

ChristophWurst commented May 9, 2023

ChristophWurst commented May 10, 2023

feat(dav): store scopes for properties and filter locally scoped properties for federated address book sync #38048

feat(dav): store scopes for properties and filter locally scoped properties for federated address book sync #38048

Conversation

miaulalala commented May 3, 2023 • edited by ChristophWurst Loading

Summary

Todo

How to test

Checklist

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst commented May 3, 2023

ChristophWurst left a comment

Choose a reason for hiding this comment

miaulalala commented May 3, 2023

ChristophWurst commented May 3, 2023

ChristophWurst commented May 4, 2023

tcitworld commented May 5, 2023

ChristophWurst commented May 5, 2023

miaulalala commented May 5, 2023

ChristophWurst commented May 5, 2023 • edited Loading

miaulalala commented May 5, 2023

ChristophWurst commented May 8, 2023

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst commented May 9, 2023

ChristophWurst commented May 10, 2023

miaulalala commented May 3, 2023 •

edited by ChristophWurst

Loading

ChristophWurst commented May 5, 2023 •

edited

Loading