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.

Sign up for GitHub

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

Jump to bottom

Allow autocomplete based on phone sync #26031

Merged
merged 17 commits into from
Mar 11, 2021
Merged

Allow autocomplete based on phone sync #26031

merged 17 commits into from
Mar 11, 2021

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Mar 9, 2021 •
edited
Loading

  • Based on Provisioning API to IBootstrap #26029
  • Store phonebook sync matches in a database
  • Remove "known user" matches when the user changes their phone number
  • Remove any matches when the user is deleted
  • Add a config option to "Restrict username autocompletion to users with phone sync matches" in the sharing settings
    • (needs some thinking how we can phrase it so it works with the existing options in a good way)
  • Respect that setting on the autocomplete endpoint.
  • Write integration tests to assure this doesn't break

@nickvergessen nickvergessen added this to the Nextcloud 22 milestone Mar 9, 2021
@nickvergessen nickvergessen force-pushed the feature/noid/allow-autocomplete-based-on-phone-sync branch from cfbcd82 to f17a576 Compare March 10, 2021 11:09
@nickvergessen
Add a KnownUsers database with model
f1bb4e5 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Store when a user knows another user based on phone number
f62d9b5 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Delete matches when a user changes their phone number
55a5d26 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Delete matches when the user is being deleted
7baefcf 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/noid/allow-autocomplete-based-on-phone-sync branch 2 times, most recently from 58edfee to fb933ed Compare March 10, 2021 14:22
@nickvergessen
Add a service to find out if a user knows another user
c7be18c 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Add a config setting to restrict autocompletion to phonebook matches
b71268e 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Restrict autocompletion also based on the phonebook known users
236aa19 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Add integration tests for autocomplete/get (similar to sharees API)
a310a2a 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Add a hint that the settings are OR based
177ae33 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/noid/allow-autocomplete-based-on-phone-sync branch from c122f9b to f7d1cf0 Compare March 10, 2021 16:20
@nickvergessen
Add a setting to restrict returning a full match unless in phonebook …
5b53b6f 
…or same group

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/noid/allow-autocomplete-based-on-phone-sync branch from f7d1cf0 to 5b53b6f Compare March 10, 2021 16:26
@nickvergessen
Fix CS hopefully
78efabb 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen marked this pull request as ready for review March 10, 2021 17:37
@nickvergessen
Change label also in the acceptance tests
cf8f9c1 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Also execute the new tests on drone
1683e02 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Also clear the knownUser when changing via the settings endpoint
07095fd 
Signed-off-by: Joas Schilling <coding@schilljs.com>
ChristophWurst
ChristophWurst approved these changes Mar 10, 2021
View reviewed changes
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.

I squinted but the code looks fine

@nickvergessen
Bump version to run migration
45dbc47 
Signed-off-by: Joas Schilling <coding@schilljs.com>
PVince81
PVince81 approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 code looks fine with minor comments

and one thing that disturbed me most was the naming of $knownTo and $knownUser which are easy to mix up

lib/private/Contacts/ContactsMenu/ContactsStore.php Outdated Show resolved Hide resolved
lib/private/Contacts/ContactsMenu/ContactsStore.php Outdated Show resolved Hide resolved
lib/private/KnownUser/KnownUserMapper.php Show resolved Hide resolved
lib/private/KnownUser/KnownUserMapper.php Show resolved Hide resolved
lib/private/KnownUser/KnownUserService.php Show resolved Hide resolved
lib/private/KnownUser/KnownUserService.php Outdated Show resolved Hide resolved
@nickvergessen
Rename some parameters and methods to make the API more clear
61ed57b 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Clean up the logic of the contacts store a bit
b4f6aca 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/noid/allow-autocomplete-based-on-phone-sync branch from 5ddb55c to b4f6aca Compare March 10, 2021 19:49
PVince81
PVince81 approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@nickvergessen nickvergessen added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Mar 10, 2021
@nickvergessen
Copy link
Member Author

/backport to stable21

@nickvergessen nickvergessen requested a review from blizzz March 10, 2021 20:49
@nickvergessen nickvergessen merged commit 56b08c0 into master Mar 11, 2021
@nickvergessen nickvergessen deleted the feature/noid/allow-autocomplete-based-on-phone-sync branch March 11, 2021 07:29
@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@nickvergessen nickvergessen mentioned this pull request Mar 11, 2021
Merged
@skjnldsv
Copy link
Member

@nickvergessen Documentation missing?

@nickvergessen
Copy link
Member Author

How to upload your phone's addressbook? Or the admin setting?

@skjnldsv
Copy link
Member

How to upload your phone's addressbook? Or the admin setting?

admin setting

server/apps/settings/lib/Settings/Admin/Sharing.php

Lines 88 to 94 in d9f8522

'allowShareDialogUserEnumeration' => $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes'),
'restrictUserEnumerationToGroup' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no'),
'restrictUserEnumerationToPhone' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no'),
'restrictUserEnumerationFullMatch' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes'),
'restrictUserEnumerationFullMatchUserId' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_userid', 'yes'),
'restrictUserEnumerationFullMatchEmail' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_email', 'yes'),
'restrictUserEnumerationFullMatchIgnoreSecondDN' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_ignore_second_dn', 'no'),

@nickvergessen nickvergessen mentioned this pull request Oct 25, 2023
Merged
@nickvergessen
Copy link
Member Author

Added the new checkbox in nextcloud/documentation#11241 and updated the screenshot and labels that were brought in by other changes in the meantime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@PVince81 PVince81 PVince81 approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@blizzz blizzz Awaiting requested review from blizzz

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: settings feature: sharing feature: users and groups
Projects
None yet
Milestone
Nextcloud 22
Development

Successfully merging this pull request may close these issues.
5 participants
@nickvergessen @skjnldsv @PVince81 @ChristophWurst @MorrisJobke