Fix account data visibility after disabling public addressbook upload #25417

danxuliu · 2021-02-01T04:04:53Z

When the lookup server upload was disabled the scope menu was hidden in the properties of the profile information. If any property was then changed the request did not contain any scope information, so the UsersController automatically set them to null. However, as the controller fully overwrote the previous properties this caused the scope of the properties to end being null. In practice, this caused that if a user changed her name from the profile information when uploading to the public address book was disabled that user could not be searched either in the local instance or from trusted servers.

Moreover, the user was not able to reset the visibility from the profile information, even if uploading to the public address book was enabled again.

Although this pull request fixes the controller it does not repair any corrupted visibility, as it is not possible to know whether the user had a public, contacts or private visibility before it got corrupted. Repairing the visibility by setting by default anything above private could be seen as a privacy issue, and setting the visibility to private would hide the problem from the user if that user wants to be found by other contacts. Update: since #26243 any invalid scope is treated as local. Despite that, except for the original last commit (which is still needed and thus included in the stable19 and stable20 backports), the rest of the commits still apply.

Nevertheless, I was wondering if it would be worth adding a repair step to check if the visibility of any property is null and then create a notification to the user so she can fix it. Or adding a command to fix the visibility to some value (but this would be something to be explicitly done by the admin, so it would be her responsibility).

How to test (main scenario)

Log in as an admin
Open the settings
Open the (Administration) Sharing section
Disable Allow users to publish their data to a global and public address book
Log out as the admin
Log in as a regular user (admins are not shown in the contacts menu)
Open the Personal info section
Set the user name to Test
In a private window, log in as another user
Open the contacts menu and search for "Test"

Result with this pull request

The user is found.

Result without this pull request

The user is not found.

How to test (corrupted visibility subscenario) - `stable19` and `stable20`

(Generate a wrong database state by doing all the following steps in the parent commit of this branch)
Open the settings
Open the (Administration) Sharing section
Disable Allow users to publish their data to a global and public address book
Open the Personal info section
Set the user name to Test
(Checkout this branch and reload the page)

Result with this pull request

Visibility of properties can be set. Visibilities with an invalid value (all of them) will show an error.

Result without this pull request

Visibility of properties can not be set. Even if Allow users to publish their data to a global and public address book is enabled again the visibility can still not be set, as the scope is null.

How to test (corrupted visibility subscenario) - `stable21` and later

(Generate a wrong database state by doing all the following steps in the parent commit of this branch)
Open the settings
Open the (Administration) Sharing section
Disable Allow users to publish their data to a global and public address book
Open the Personal info section
Set the user name to Test
(Checkout this branch and reload the page)

Result with this pull request

Visibility of properties can be set. Even if there is only one value available it will be possible to show the menu to read its detailed description.

Result without this pull request

Visibility of properties can not be set. If Allow users to publish their data to a global and public address book is enabled again the visibility will be shown as Local (and in the database it will be null).

How to test (public visibility when public upload is disabled subscenario)

Open the settings
Open the Personal info section
Set the visibility of user name to Public
Open the (Administration) Sharing section
Disable Allow users to publish their data to a global and public address book
Open the Personal info section

Result with this pull request

Visibility of user name shows "Public" as selected, but disabled. Changing the visibility to a more restricted value removes the "Public" option.

Result without this pull request

No visibility is shown.

rullzer · 2021-02-02T20:42:39Z

Master is Nextcloud 22 now.
If this should go into 21 it should be backported.

danxuliu · 2021-02-02T20:52:57Z

/backport to stable21

danxuliu · 2021-03-02T11:44:13Z

Rebased and fixed up.

rullzer

Didn't test but I trust your test writing skills

danxuliu · 2021-03-04T23:31:59Z

Didn't test but I trust your test writing skills

I hope we will not regret that :-P

danxuliu · 2021-03-06T06:47:19Z

I pushed the backports to stable19 and stable20 before this was merged because they needed some adjustments but I will probably be on vacation when this is merged.

danxuliu · 2021-04-19T08:47:37Z

Rebased and adjusted to changes introduced in #26243. Acceptance tests failure is known and unrelated.

I dropped the last commit because it was no longer needed (it handled corrupted scope values by showing them with an error icon in the UI, although that no longer applies due to them being now returned as local scope), but the rest still applies.

I have also found some issues related to #26243 (it should be possible to set federated scope even if the lookup server is disabled, and contact data should be shown in contacts menu even if scope is local), but for clarity they will be addressed in other pull requests.

MorrisJobke · 2021-04-20T20:06:09Z

Conflicts 😢

blizzz

Code looks ok, tests very much appreciated :) I did some smoke testing, which was fine, too.

danxuliu · 2021-04-21T07:14:01Z

Conflicts 😢

Rebased and resolved :-)

danxuliu · 2021-04-21T07:21:28Z

Conflicts cry

Rebased and resolved :-)

Uh... wait, I spoke too soon, most tests fail, let me check :-)

danxuliu · 2021-04-21T08:15:12Z

It seems that most of the failures were internal GitHub errors. There are some PHP CS errors, but they seem to be in master as well.

The Handlebars test has failed again, but in my system build/compile-handlebars-templates.sh passes. So I do not know if it is a problem with GitHub or my system 🤷

Besides that, I noticed a small issue related to the previous conflict (in some areas of the disabled menu item the cursor was a pointer, because all items in a link element use cursor: pointer and the menu trigger is now a link), so I fixed the CSS to handle that.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Right now it makes no difference, but this should make future tests clearer, specially in case of failure. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

"AccountManager::updateUser()" wipes previous user data with whichever user data is given (except for some adjustments, like resetting the verified status when needed). As the controller overrode the properties those properties would lose some of their attributes even if they are not affected by the changes made by the controller. Now the controller only modifies the attributes set ("value" and "scope") to prevent that. Note that with this change the controller no longer removes the "verified" status, but this is not a problem because, as mentioned, "AccountManager::updateUser()" resets them when needed (for example, when the value of the website property changes). This change is a previous step to fix overwritting properties with null values, and it will prevent the controller from making unexpected changes if more attributes are added in the future. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The controller can receive an optional subset of the properties of the user settings; values not given are set to "null" by default. However, those null values overwrote the previously existing values, so in practice any value not given was deleted from the user settings. Now only non null values overwrite the previous values. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

"parsePhoneNumber()" expects a string, so a TypeError would be thrown if the phone number value is null. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Depending on some settings (for example, if lookup server upload is disabled) some items can be hidden in the scope menu. However, if the user selected an scope in the past once the settings were changed the scope was no longer visible in the menu. Now the active scope will be always visible in the menu, although if it is an excluded scope it will be disabled. Selecting any other scope will then hide the excluded and no longer active one. When upload to the lookup server is disabled the scope menu was hidden for display name and email in the personal information settings; now the menu will be always shown to enable the above described behaviour. Note that the menu will be shown even if there is a single available scope so the user can read its description. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu · 2021-04-23T14:48:28Z

After the last rebase everything is now green :-)

danxuliu added bug 3. to review Waiting for reviews feature: settings labels Feb 1, 2021

danxuliu added this to the Nextcloud 21 milestone Feb 1, 2021

danxuliu requested review from rullzer, MorrisJobke and ChristophWurst February 1, 2021 04:04

backportbot-nextcloud bot added the backport-request label Feb 1, 2021

danxuliu mentioned this pull request Feb 1, 2021

21 RC1 #25376

Merged

rullzer modified the milestones: Nextcloud 21, Nextcloud 22 Feb 2, 2021

rullzer force-pushed the fix-account-data-visibility-after-disabling-public-addressbook-upload branch from d543016 to a33daca Compare February 9, 2021 21:04

danxuliu force-pushed the fix-account-data-visibility-after-disabling-public-addressbook-upload branch from a33daca to 588af7a Compare March 2, 2021 11:43

rullzer approved these changes Mar 4, 2021

View reviewed changes

This was referenced Mar 6, 2021

[stable20] Fix account data visibility after disabling public addressbook upload #25973

Merged

[stable19] Fix account data visibility after disabling public addressbook upload #25974

Merged

danxuliu added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 13, 2021

danxuliu force-pushed the fix-account-data-visibility-after-disabling-public-addressbook-upload branch from 588af7a to 8d74cec Compare April 19, 2021 07:06

danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 19, 2021

danxuliu requested review from juliusknorr and blizzz April 19, 2021 08:48

blizzz approved these changes Apr 20, 2021

View reviewed changes

danxuliu force-pushed the fix-account-data-visibility-after-disabling-public-addressbook-upload branch from 8d74cec to 5a083fe Compare April 21, 2021 07:12

danxuliu force-pushed the fix-account-data-visibility-after-disabling-public-addressbook-upload branch from 5a083fe to 0b8248b Compare April 21, 2021 07:54

danxuliu added 10 commits April 23, 2021 11:44

Use constants from interface rather than class

8cae515

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Extract default test data to a helper getter

ef5d538

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Change default test data to values less similar to empty values

190ffd7

Right now it makes no difference, but this should make future tests clearer, specially in case of failure. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Add more unit tests for setting user settings

c5e3d2e

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Fix TypeError when "email" is not given in the controller request

ac0c183

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Add integration tests for searching users in contacts menu

e2c4a17

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Guard against null phone number value

7159a70

"parsePhoneNumber()" expects a string, so a TypeError would be thrown if the phone number value is null. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu force-pushed the fix-account-data-visibility-after-disabling-public-addressbook-upload branch from 0b8248b to da84ed7 Compare April 23, 2021 09:46

danxuliu merged commit 8fc8451 into master Apr 23, 2021

danxuliu deleted the fix-account-data-visibility-after-disabling-public-addressbook-upload branch April 23, 2021 14:48

backportbot-nextcloud bot mentioned this pull request Apr 23, 2021

[stable21] Fix account data visibility after disabling public addressbook upload #26724

Merged

backportbot-nextcloud bot removed the backport-request label Apr 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix account data visibility after disabling public addressbook upload #25417

Fix account data visibility after disabling public addressbook upload #25417

danxuliu commented Feb 1, 2021 •

edited

Loading

rullzer commented Feb 2, 2021

danxuliu commented Feb 2, 2021

danxuliu commented Mar 2, 2021

rullzer left a comment

danxuliu commented Mar 4, 2021

danxuliu commented Mar 6, 2021 •

edited

Loading

danxuliu commented Apr 19, 2021 •

edited

Loading

MorrisJobke commented Apr 20, 2021

blizzz left a comment

danxuliu commented Apr 21, 2021

danxuliu commented Apr 21, 2021

danxuliu commented Apr 21, 2021 •

edited

Loading

danxuliu commented Apr 23, 2021

Fix account data visibility after disabling public addressbook upload #25417

Fix account data visibility after disabling public addressbook upload #25417

Conversation

danxuliu commented Feb 1, 2021 • edited Loading

How to test (main scenario)

Result with this pull request

Result without this pull request

How to test (corrupted visibility subscenario) - stable19 and stable20

Result with this pull request

Result without this pull request

How to test (corrupted visibility subscenario) - stable21 and later

Result with this pull request

Result without this pull request

How to test (public visibility when public upload is disabled subscenario)

Result with this pull request

Result without this pull request

rullzer commented Feb 2, 2021

danxuliu commented Feb 2, 2021

danxuliu commented Mar 2, 2021

rullzer left a comment

Choose a reason for hiding this comment

danxuliu commented Mar 4, 2021

danxuliu commented Mar 6, 2021 • edited Loading

danxuliu commented Apr 19, 2021 • edited Loading

MorrisJobke commented Apr 20, 2021

blizzz left a comment

Choose a reason for hiding this comment

danxuliu commented Apr 21, 2021

danxuliu commented Apr 21, 2021

danxuliu commented Apr 21, 2021 • edited Loading

danxuliu commented Apr 23, 2021

danxuliu commented Feb 1, 2021 •

edited

Loading

How to test (corrupted visibility subscenario) - `stable19` and `stable20`

How to test (corrupted visibility subscenario) - `stable21` and later

danxuliu commented Mar 6, 2021 •

edited

Loading

danxuliu commented Apr 19, 2021 •

edited

Loading

danxuliu commented Apr 21, 2021 •

edited

Loading