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

feat(dav): expose system address book #37734

Merged
merged 1 commit into from
May 12, 2023

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Apr 14, 2023

Summary

Expose the system address book

How to test

Enable share enumeration in the backend

image

to see all users.

In the contacts app, the users will have the address book "system":

image

Disable it to see the logged in user only:

With enumeration Without enumeration
image image

You can also check the DAV url /remote.php/dav/addressbooks/users/ to see the new system address book:

image

To Do

  • check if renaming address book to something else but "system" for the new user principal collection is possible to avoid naming conflicts
  • Rename the principal collection Display name to "Accounts" for the sabre collection.

Checklist

@szaimen szaimen added the 2. developing Work in progress label Apr 14, 2023
@szaimen szaimen added this to the Nextcloud 27 milestone Apr 14, 2023
@miaulalala miaulalala self-assigned this Apr 24, 2023
@miaulalala miaulalala added 3. to review Waiting for reviews feature: carddav Related to CardDAV internals and removed 2. developing Work in progress labels Apr 24, 2023
@miaulalala miaulalala marked this pull request as ready for review April 24, 2023 15:11
@miaulalala miaulalala added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 24, 2023
$modified = false;
$row['carddata'] = $this->readBlob($row['carddata'], $modified);
if ($modified) {
$row['size'] = strlen($row['carddata']);

Check notice

Code scanning / Psalm

PossiblyInvalidMethodCall

Cannot call method on possible int variable $result
apps/dav/lib/CardDAV/CardDavBackend.php Fixed Show fixed Hide fixed
apps/dav/lib/CardDAV/CardDavBackend.php Fixed Show fixed Hide fixed
apps/dav/lib/CardDAV/CardDavBackend.php Fixed Show fixed Hide fixed
apps/dav/lib/CardDAV/CardDavBackend.php Fixed Show fixed Hide fixed
apps/dav/lib/CardDAV/SystemAddressbook.php Fixed Show fixed Hide fixed
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

I tested this locally (without federation). I'm impressed! It works quite good.

However, the contacts in the system address book are not indicated to be read-only. The fields can't actually be edited though because saving fails. Could this be a problem with the ACLS of the system address book?

@miaulalala
Copy link
Contributor Author

I tested this locally (without federation). I'm impressed! It works quite good.

However, the contacts in the system address book are not indicated to be read-only. The fields can't actually be edited though because saving fails. Could this be a problem with the ACLS of the system address book?

That's a good question. Can you tell me which acls get sent to the frontend? Maybe that needs a fix somewhere - the acls are set to read only in \OCA\DAV\CardDAV\AddressBook::getACL but could be that the data is lost along the way.

@miaulalala
Copy link
Contributor Author

miaulalala commented Apr 26, 2023

From @ChristophWurst for how we handle the share enumeration:

0) No checkbox checked -> Show only the same user
1) Only "Allow username autocompletion in share dialog" -> show everyone
2) "Allow username autocompletion in share dialog" + "Allow username autocompletion to users within the same groups" -> show only users in intersecting groups
3) "Allow username autocompletion in share dialog" + "Allow username autocompletion to users based on phone number integration" -> show only the same user
4) "Allow username autocompletion in share dialog" + "Allow username autocompletion to users within the same groups" + "Allow username autocompletion to users based on phone number integration" -> show only users in intersecting groups

@miaulalala miaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 26, 2023
@miaulalala

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst

This comment was marked as outdated.

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 11, 2023
@ChristophWurst ChristophWurst force-pushed the enh/expose-system-address-book branch from 44bf18a to b8f770b Compare May 11, 2023 11:56
@ChristophWurst ChristophWurst marked this pull request as ready for review May 11, 2023 11:56
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Works

@ChristophWurst ChristophWurst force-pushed the enh/expose-system-address-book branch 2 times, most recently from 1c1384e to 8e8132f Compare May 11, 2023 12:06
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Just tested with davx5 and it works (again)

@JohannesGGE
Copy link
Contributor

JohannesGGE commented May 11, 2023

Testet with contacts app:

  • I can delete the system adress book in the settings 🙃
  • (Currently no option to disable the adressbook (same as recently contacted). Could be done in another issue but important for large sysAddBooks I think)

@miaulalala miaulalala force-pushed the enh/expose-system-address-book branch from d447caf to 5c2eb73 Compare May 11, 2023 17:04
@ChristophWurst 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 11, 2023
@ChristophWurst
Copy link
Member

Integration test failure is the ususal contact menu
Psalm fails randomly, possibly fixed by #38195

@ChristophWurst ChristophWurst merged commit d5c2e75 into master May 12, 2023
@ChristophWurst ChristophWurst deleted the enh/expose-system-address-book branch May 12, 2023 06:48
@dac2020
Copy link

dac2020 commented Aug 29, 2023

Cannot get help in forums, cannot get any answer here. Not sure it's worth spending any more time with this.

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 feature: carddav Related to CardDAV internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] CardDAV - expose system address book
10 participants