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

Server is not publishing user properties to lookup-server #25290

Open
abangtor opened this issue Jan 23, 2021 · 7 comments · May be fixed by #25300
Open

Server is not publishing user properties to lookup-server #25290

abangtor opened this issue Jan 23, 2021 · 7 comments · May be fixed by #25300

Comments

@abangtor
Copy link

There is a bug in the getUserAccountData method in the RetryJob.php of the lookup-server-connector module.
The properties are written into an one dimensional array $publicData:

$publicData[$property->getName()] = $property->getValue();

Later on the array is read as it is a two dimensional array:
$data['name'] = $publicData[IAccountManager::PROPERTY_DISPLAYNAME]['value'] ?? '';
$data['email'] = $publicData[IAccountManager::PROPERTY_EMAIL]['value'] ?? '';
$data['address'] = $publicData[IAccountManager::PROPERTY_ADDRESS]['value'] ?? '';
$data['website'] = $publicData[IAccountManager::PROPERTY_WEBSITE]['value'] ?? '';
$data['twitter'] = $publicData[IAccountManager::PROPERTY_TWITTER]['value'] ?? '';
$data['phone'] = $publicData[IAccountManager::PROPERTY_PHONE]['value'] ?? '';
$data['twitter_signature'] = $publicData[IAccountManager::PROPERTY_TWITTER]['signature'] ?? '';
$data['website_signature'] = $publicData[IAccountManager::PROPERTY_WEBSITE]['signature'] ?? '';
$data['verificationStatus'] = [
IAccountManager::PROPERTY_WEBSITE => $publicData[IAccountManager::PROPERTY_WEBSITE]['verified'] ?? '',
IAccountManager::PROPERTY_TWITTER => $publicData[IAccountManager::PROPERTY_TWITTER]['verified'] ?? '',
];

This causes the values to be empty and no properties are send to the loopup-server.

I don't know how to fix this exact, since I couldn't find a way to get the signature value from the Account or AccountProperty classes.
My current ugly fix is: $publicData[$property->getName()]['value'] = $property->getValue();

@kesselb kesselb added 1. to develop Accepted and waiting to be taken care of bug feature: federation labels Jan 23, 2021
@kesselb
Copy link
Contributor

kesselb commented Jan 23, 2021

Mind to sent a pull request? I would ignore the signature field for now. Hopefully one of the reviewer will know ;)

$data['name'] = $publicData[IAccountManager::PROPERTY_DISPLAYNAME]['value'] ?? '';

I guess we need to remove ['value'] here to make it work.

abangtor added a commit to abangtor/Nextcloud-server that referenced this issue Jan 24, 2021
Fix of the bug in the lookup_server_connector module to publish public user information to the lookup-server.
As described in issue [nextcloud#25290](nextcloud#25290)
abangtor added a commit to abangtor/Nextcloud-server that referenced this issue Jan 24, 2021
Fix of the bug in the lookup_server_connector module to publish public user information to the lookup-server.
As described in issue [nextcloud#25290](nextcloud#25290)

Signed-off-by: AbangTor <63029179+abangtor@users.noreply.github.com>
faust64 added a commit to Worteks/server that referenced this issue Jan 24, 2021
@faust64
Copy link
Contributor

faust64 commented Jan 24, 2021

So... Digging on that topic, we can find the previous copy of that code, somewhere around there:

30051e7#diff-eb7b5fbd99fede3d2293d9e58f6e38f93f6e13d8240b304a72004665aec8d5ab

Sounds like @kesselb is right, we should be able to just remove the ['value'] on most of those.

Which leaves us with a couple verified. Sounds like we can call some getVerified() method on mail and twitter properties (accounts properties / IAccountProperty)
https://github.com/nextcloud/server/blob/905e1918d2796b9a79025283cd6edf2c40f49d77/lib/public/Accounts/IAccountProperty.php

And a couple signature I'm not sure what to do with.
We can find other occurrences over there:

$signature = $this->signMessage($user, $message);

Though iAccountProperty and AccountManager won't mention signatures ... not sure I'm looking at the right place. I'll start with blank values for now.

edit: and I'm too late ... nevermind then.

@abangtor
Copy link
Author

Yes, I taught so too, that's why I added them as todo in the pull request.
But my pull request failed the Lint / php-cs check (I guess because of the comment...).
Should I close my pull request, since your implementation looks more feature complete with the verified fields?

@kesselb
Copy link
Contributor

kesselb commented Jan 24, 2021

Nah. I made a bad suggestion ;)

$publicData[$property->getName()] = [
    'value' => $property->getValue(),
    'verified' => $property->getVerified()
];

faust64 added a commit to Worteks/server that referenced this issue Jan 24, 2021
Signed-off-by: Samuel MARTIN MORO <faust64@gmail.com>
@abangtor
Copy link
Author

Ok, changed my commit now to the example from @kesselb.
With the verified status.

faust64 added a commit to Worteks/server that referenced this issue Feb 5, 2021
Signed-off-by: Samuel MARTIN MORO <faust64@gmail.com>
Signed-off-by: Samuel <faust64@gmail.com>
@szaimen

This comment was marked as resolved.

@szaimen szaimen added needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 1. to develop Accepted and waiting to be taken care of labels Jan 23, 2023
@szaimen szaimen closed this as completed Mar 6, 2023
@fcharlaix-opendsi
Copy link

@szaimen I can confirm this issue is still present in Nextcloud 25 (25.0.6), even in 26 because this file hasn't been updated since Aug 8 2022.

The PR #25300 fix this issue, you should review it !

@szaimen szaimen reopened this May 24, 2023
@szaimen szaimen added 2. developing Work in progress 27-feedback and removed needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants