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

Avatar privacy and new scope #26243

Merged
merged 16 commits into from
Mar 29, 2021
Merged

Avatar privacy and new scope #26243

merged 16 commits into from
Mar 29, 2021

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Mar 22, 2021 •
edited
Loading

Todos

  • Private avatars are now only shown to their owners.
  • Avatar that have not been set to public will only be shown for logged in users.
  • New option for "v2-private" but
    • Add option in menu
    • Make sure the option is working in various places
    • Only make the extra option available for "Avatar", "Phone number", "Twitter", "Website", "Address"
    • Scope menu must always be visible even when federation / public servers option is off
    • Remove federated options when lookup server is disabled
    • Unit tests
  • extend OCS provisioning endpoint "https://vvortex.local/ocs/v2.php/cloud/users/$userId"
    • allow getting scope values
    • allow setting the scope values
    • allow setting other values like phone, etc (if not already)
    • users can get/set fields and scope values for themselves, but not for other users
    • admin cannot get/set fields for other users
    • Unit tests
  • add boolean capability to signal the ability to set scopes

Issues

  • Issue? Switching avatar privacy doesn't reset cache for browsers (Chromium doesn't reload avatars on shift+refresh) => inform that it's only about 1 day caching
  • It seems the wording of the current options is already confusing, as they are about federated sharing => solved
  • Improve performance of placeholder avatar (currently using the uncached guest avatar)
  • Phone, address, twitter, website fields need to be always visible and editable (never disabled)
  • What icons to use for "Private" and "Local" ?
  • Test/check avatar endpoint with mobile clients as they might use the public unauthenticated endpoint and raise tickets accordingly => I was told that they use authentication headers
  • Fix AvatarManagerTest

@PVince81 PVince81 added this to the Nextcloud 22 milestone Mar 22, 2021
@PVince81 PVince81 self-assigned this Mar 22, 2021
@PVince81 PVince81 force-pushed the enh/noid/avatar-privacy-new-scope branch 2 times, most recently from d728469 to 023db70 Compare March 23, 2021 16:00
@PVince81
Copy link
Member Author

PVince81 commented Mar 23, 2021 •
edited
Loading

  • BUG: mapping of old values seems to not work

@PVince81 PVince81 force-pushed the enh/noid/avatar-privacy-new-scope branch 3 times, most recently from 0ee96fa to 5512c24 Compare March 24, 2021 09:32
@PVince81
Copy link
Member Author

Updated top post to also extend the OCS API endpoint to allow clients to get/set scope values

@PVince81
Copy link
Member Author

hah... it appears that the provisioning API is also cheating and using AccountManager->getUser(), so the mapping will not work there. I'll migrate it to use IAccountManager->getAccount() as part of the OCS API extension.

@PVince81
Copy link
Member Author

OCS API has been adjusted.

Next up: unit tests

@nickvergessen nickvergessen self-requested a review March 24, 2021 12:58
@PVince81 PVince81 force-pushed the enh/noid/avatar-privacy-new-scope branch from bc533b8 to a7134e0 Compare March 24, 2021 15:18
@PVince81
Copy link
Member Author

unit tests done

next up would be fixing guest avatar performance, but could also be done separately as the current approach already works

however, still need to settle at least on wording and icons

@PVince81 PVince81 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 24, 2021
@PVince81 PVince81 marked this pull request as ready for review March 24, 2021 16:08
@PVince81
Copy link
Member Author

Added task "add capability". I think a simple boolean should do to signal the ability to read and set scopes.

@PVince81
Copy link
Member Author

  • switch icons to material design ? (so that clients could use the same icon names from the MD library)

@PVince81
Copy link
Member Author

Added capability:

        "provisioning_api": {
          "version": "1.11.0",
          "hasAccountPropertyScopes": true
        },

Added PlaceholderAvatar which is like GuestAvatar but caches the images.
This was done separately to avoid messing with existing UserAvatar logic and their own existing generated images, which would require more logic and migration to make it work right.

@nickvergessen
Copy link
Member

Earn bonus points for expanding https://github.com/nextcloud/server/blob/master/build/integration/features/provisioning-v1.feature

@PVince81
Copy link
Member Author

Earn bonus points for expanding https://github.com/nextcloud/server/blob/master/build/integration/features/provisioning-v1.feature

done

nickvergessen
nickvergessen approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Wording and icons to discuss, but works technically

PVince81 and others added 15 commits March 26, 2021 13:07
@PVince81
Add new v2-private account scope
b81a1c1 
Added new v2-private account manager scope that restricts the scope
further by excluding public link access.

Avatars with v2-private account scope are now showing the guest avatar
instead of the real avatar.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Adjust scopes menu based on conditions
9fb447e 
Now not all fields have the "v2-private" option in place.
Fix dropdown issue when a scope was stored that is not listed after
disabling the lookup server.
Whenever the lookup server upload is disabled, the scope menu is now
displayed where it makes sense to allow switching between the two private
scopes.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Map old account scope properties to new names
278a737 
Use new scope values in settings page.
Adjust all consumers to use the new constants.
Map old scope values to new ones in account property getter.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Make extra user profile fields always editable
5d14fd4 
The fields for phone number, address, website and twitter are now
editable regardless whether federated sharing and the lookup server
are enabled or not.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
OCS allow reading and writing account property scopes
266a6fb 
Extends the provisioning API to allow a user to get and set their own
account property scopes.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Expose avatarScope through provisioning API
2a312fe 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Enhance UsersControllerTest of provisioning API with scopes
cb9d25c 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Add property scope tests for AccountManager
b73df58 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Add capability for editable scopes in provisioning API
b66af8a 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Added PlaceholderAvatar with own cached images
bb008be 
When avatar scope is private, the PlaceholderAvatar is used to deliver a
placeholder avatar based on the user's initials.

This was implemented as a separate class for now to avoid messing with
the existing UserAvatar implementation and its generated vs
non-generated logic.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 @nickvergessen
Change account property capability
aac7b52 
Include version number in capability

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
@PVince81
Update psalm-baseline for Avatar API quirks
5b7a94f 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Add known user check in avatar when v2-private scope
cc54f71 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Int tests for provisioning API scopes
98e4ac3 
Added integration tests for the scope attributes in the provisioning
API.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Update autoloader for PlaceholderAvatar
5a8b7c1 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the enh/noid/avatar-privacy-new-scope branch from c1668fa to 5a8b7c1 Compare March 26, 2021 12:07
@PVince81
Copy link
Member Author

rebased for CI

@PVince81
Capability for federated scope
00d83a5 
Added additional capability in the provisioning API to signal whether
the federation scope values can be used.

This is based on whether the lookup server upload is enabled or not.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member Author

/drone/src/tests/acceptance/features/app-files-sharing.feature:23 again ?! 😦

@PVince81
Copy link
Member Author

can someone force merge maybe ? seems the notifications app might be cached or something... in any case, the test failure is unrelated

@nickvergessen nickvergessen merged commit 602de27 into master Mar 29, 2021
@nickvergessen nickvergessen deleted the enh/noid/avatar-privacy-new-scope branch March 29, 2021 07:01
@nickvergessen
Copy link
Member

/backport to stable21

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Mar 29, 2021
Merged
This was referenced Apr 14, 2021
Merged
Merged
Merged
@danxuliu danxuliu mentioned this pull request Apr 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@schiessle schiessle Awaiting requested review from schiessle

@rullzer rullzer Awaiting requested review from rullzer

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@PVince81 PVince81

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Milestone
Nextcloud 22
Development

Successfully merging this pull request may close these issues.
3 participants
@PVince81 @nickvergessen @MorrisJobke