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

don't blame randome people for background email updates #10687

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Aug 14, 2018

Commit 1)

We have the feature that when, for example, the email address was changed, a notification is sent to the affected user (https://github.com/nextcloud/server/blob/master/settings/Hooks.php#L168-L179).

Now it is possible that a user record is refreshed from its original source while the user was looked up by other means. Specifically happens on two scenarios in LDAP, and at least one, when ajax as backgroud job mode is used, is not going to change (soon). When this happens, the affected user is notified by "your acquaintance updated your email address", which she didn't herself.

admin_audit now logs email address changes, so this can always be looked up by the admin.

Is there any issue with changing this behaviour? It was reported by a customer and should be backported.

Commit 2)

Latest in 14, we do not need to force a refresh on LDAP user with userExists check, since auth is repeated regularly in the background with the csrf update. IIRC. The Exception check is not necessary anymore as it happens higher in the stack already.

the information is being collected with admin_audit

Signed-off-by: Arthur Schiwon <[email protected]>
and remove some deprecated code

Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
try {
$uuid = $this->access->getUserMapper()->getUUIDByDN($dn);
if (!$uuid) {
return false;
}
$newDn = $this->access->getUserDnByUuid($uuid);
//check if renamed user is still valid by reapplying the ldap filter
if (!is_array($this->access->readAttribute($newDn, '', $this->access->connection->ldapUserFilter))) {
if ($newDn === $dn || !is_array($this->access->readAttribute($newDn, '', $this->access->connection->ldapUserFilter))) {
Copy link
Member

Choose a reason for hiding this comment

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

This is just hardening right?

Copy link
Member Author

Choose a reason for hiding this comment

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

and avoiding an uneccesary request to LDAP

@@ -173,15 +174,11 @@ public function onChangeEmail(IUser $user, $oldMailAddress) {
$this->config->getSystemValue('default_language', 'en')
)
);

$text = $this->l->t('%1$s changed your email address on %2$s.', [$actor->getDisplayName(), $instanceUrl]);
Copy link
Member

Choose a reason for hiding this comment

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

My guess is this case was there to notify you if an admin changed your e-mail... right?

Copy link
Member Author

@blizzz blizzz Aug 16, 2018

Choose a reason for hiding this comment

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

Actually, that's further below. Though i don't know what that works at it checks the user who is logged in. But it works.

Copy link
Member

Choose a reason for hiding this comment

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

The one below is for the CLI change. That one here is for all web based accesses.

Copy link
Member

Choose a reason for hiding this comment

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

This was for admin cases where its done via the UI. So you know whome to contact if someone changes your details.

The admin strings below are for not logged in users, aka CLI commands.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke merged commit 9cf11bd into master Aug 20, 2018
@MorrisJobke MorrisJobke deleted the fix/noid/false-positive-change-mention branch August 20, 2018 12:37
@MorrisJobke
Copy link
Member

@blizzz Please prepare the back ports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants