Moved users_picker profile custom picker to contacts #3487
Conversation
|
Weird thing: Also worth mentioning: The profile picker component works fine in https://github.com/nextcloud/users_picker and we can reproduce the problem there when downgrading vue to 2.6.14. |
So what is the plan? Who will maintain that code once it's been moved into this repo? |
Co-authored-by: Christoph Wurst <[email protected]> Signed-off-by: Andrey Borysenko <[email protected]>
Signed-off-by: Andrey Borysenko <[email protected]>
andrey18106
force-pushed
the
profile_picker
branch
from
8e97371 to
cf6a674
Compare
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #3487 +/- ##
==========================================
- Coverage 1.81% 0.00% -1.82%
- Complexity 272 299 +27
==========================================
Files 113 27 -86
Lines 6126 903 -5223
Branches 1479 0 -1479
==========================================
- Hits 111 0 -111
+ Misses 5899 903 -4996
+ Partials 116 0 -116
☔ View full report in Codecov by Sentry. |
the contacts maintainers will maintain the code, but we are happy to give the team any code walk-throughs so everybody is comfortable with it. |
Co-authored-by: Christoph Wurst <[email protected]> Signed-off-by: Andrey Borysenko <[email protected]>
Co-authored-by: Christoph Wurst <[email protected]> Signed-off-by: Andrey Borysenko <[email protected]>
Signed-off-by: Andrey Borysenko <[email protected]>
|
discussed with Christoph that this issue can be merged after the icon issue is resolved (https://github.com/nextcloud/contacts/pull/3487#discussion_r1253443738) will check with @andrey18106 |
Signed-off-by: Andrey Borysenko <[email protected]>
There was a problem hiding this comment.
The code looks really good but I notice two critical issues
- User enumeration restrictions are not valued. You can configure Nextcloud to not allow seeing other people. Yet this picker returns any user objects it can find. Ref https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/file_sharing_configuration.html#file-sharing-configuration. You can test this by restricting your local environment and entering the user ID of another user. That other user should not be selectable. You can check the sharing menu. It values all these restrictions.
- The user ID is used as search attribute. This works fine for database users or when LDAP records are mapped by a user-readable attribute. However, LDAP/SAML map by UUID by default. So to pick user a (LDAP UUID=597ae2f6-16a6-1027-98f4-d28b5365dc14) you have to type the exact string
597ae2f6-16a6-1027-98f4-d28b5365dc14to find the user. See https://github.com/nextcloud/server/wiki/How-to-test-LDAP%3F for an LDAP-based user base on your dev env.
To address these two issues I would suggest using something like \OCP\Contacts\IManager::search instead of \OCP\IUserManager::get. It will allow for partial matches (andre for Andrey) and can help restrict enumeration.
| 'title' => $userDisplayName, | ||
| 'subline' => $userEmail ?? $userDisplayName, | ||
| 'email' => $userEmail, | ||
| 'bio' => isset($bio) && $bio !== '' ? substr_replace($bio, '...', 80, strlen($bio)) : null, |
There was a problem hiding this comment.
I think we should use a multibyte-safe string function, e.g. https://www.php.net/manual/en/function.mb-substr.php and then append …. substr_replace and strlen do not work well with multibytes/emojis.
|
|
||
| $userId = $this->getObjectId($referenceText); | ||
| $user = $this->userManager->get($userId); | ||
| if ($user !== null) { |
There was a problem hiding this comment.
nitpick: if you invert the logic and return early for null users the method has less indentation and becomes easier to read
|
Hi @ChristophWurst , is there any chance you or your team could take it from here? |
|
This isn't a light change, so I'll have to allocate resources for the feature. |
|
@andrey18106 I see the security problems have been fixed in your repo. Could you please update this branch? Cheers |
|
Maybe it's better to clarify if we actually want to proceed with this PR given the timeline this is already open? |
Moving the users_picker as it was planned with @DaphneMuller and @julien-nc.
But there is a conflict in NcSelect component ([Vue warn]: You may have an infinite update loop in a component render function.). It works with vue 2.7.14, but not with vue (2.6.14) in contacts, maybe @ChristophWurst can help with it and decide where do we want to move the profile picker.