Skip to content

lib/private/User: always pass old value to 'triggerChange' in User class, do not change user properties if value has not changed #14566

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

Closed
leonklingele-work wants to merge 4 commits into from
Closed

lib/private/User: always pass old value to 'triggerChange' in User class, do not change user properties if value has not changed #14566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8cd34d1
lib/private/User,apps/user_ldap/lib/User: always pass old value to Us…
Mar 6, 2019
edef164
lib/private/User: do not change user properties if value has not changed
Mar 6, 2019
569e280
lib/private/User: add reminder to first change 'enabled' property, th…
Mar 6, 2019
fe9f121
Try to fix unit tests
Apr 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/user_ldap/lib/User/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ public function composeAndStoreDisplayName($displayName, $displayName2 = '') {
if (!empty($oldName) && $user instanceof \OC\User\User) {
// if it was empty, it would be a new record, not a change emitting the trigger could
// potentially cause a UniqueConstraintViolationException, depending on some factors.
$user->triggerChange('displayName', $displayName);
$user->triggerChange('displayName', $displayName, $oldName);
}
}
return $displayName;
Expand Down
25 changes: 13 additions & 12 deletions lib/private/User/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,16 +138,16 @@ public function getDisplayName() {
*/
public function setDisplayName($displayName) {
$displayName = trim($displayName);
if ($this->backend->implementsActions(Backend::SET_DISPLAYNAME) && !empty($displayName)) {
$oldDisplayName = $this->getDisplayName();
if ($this->backend->implementsActions(Backend::SET_DISPLAYNAME) && !empty($displayName) && $displayName !== $oldDisplayName) {
$result = $this->backend->setDisplayName($this->uid, $displayName);
if ($result) {
$this->displayName = $displayName;
$this->triggerChange('displayName', $displayName);
$this->triggerChange('displayName', $displayName, $oldDisplayName);
Copy link
Member

@blizzz blizzz Mar 6, 2019

Choose a reason for hiding this comment

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

Please check the other places where this hook is triggered:

  • lib/private/User/User.php (remove or adapt?)

Also there is

  • apps/user_ldap/lib/User/User.php

because it is not called externally, but when changes in LDAP were discovered. This cannot be removed, should be adopted. It should not intervene with User:setDisplayName.

Copy link
Author

@leonklingele-work leonklingele-work Mar 11, 2019

Choose a reason for hiding this comment

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

Updated.

}
return $result !== false;
} else {
return false;
}
return false;
}

/**
Expand All @@ -159,12 +159,12 @@ public function setDisplayName($displayName) {
*/
public function setEMailAddress($mailAddress) {
$oldMailAddress = $this->getEMailAddress();
if($mailAddress === '') {
$this->config->deleteUserValue($this->uid, 'settings', 'email');
} else {
$this->config->setUserValue($this->uid, 'settings', 'email', $mailAddress);
}
if($oldMailAddress !== $mailAddress) {
if($mailAddress === '') {
$this->config->deleteUserValue($this->uid, 'settings', 'email');
} else {
$this->config->setUserValue($this->uid, 'settings', 'email', $mailAddress);
}
$this->triggerChange('eMailAddress', $mailAddress, $oldMailAddress);
}
}
Expand Down Expand Up @@ -365,7 +365,8 @@ public function setEnabled(bool $enabled = true) {
$oldStatus = $this->isEnabled();
$this->enabled = $enabled;
if ($oldStatus !== $this->enabled) {
$this->triggerChange('enabled', $enabled);
// TODO: First change the value, then trigger the event as done for all other properties.
Copy link
Member

@MorrisJobke MorrisJobke Mar 6, 2019

Choose a reason for hiding this comment

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

I'm fine with moving it one line to the bottom to be after the change was made. 👍 Could you make this change?

Copy link
Author

@leonklingele-work leonklingele-work Mar 6, 2019

Choose a reason for hiding this comment

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

No, this will change behavior: Everyone currently rejecting an changeUser event where enabled is modified by throwing an exception would need to revert the change to enabled (as enabled will then be updated even though the exception is thrown).

Copy link
Member

@MorrisJobke MorrisJobke Mar 6, 2019

Choose a reason for hiding this comment

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

This was never meant for this and doing this is also not the way to handle such stuff. It's highly recommended to not manipulate data via this mechanism. This is also documented and communicated like this.

Also I couldn't find any code that does it like that. Do you know any app that does this?

Copy link
Member

@MorrisJobke MorrisJobke Mar 6, 2019

Choose a reason for hiding this comment

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

Also those events are pure one way only events.

Copy link
Author

@leonklingele-work leonklingele-work Mar 6, 2019

Choose a reason for hiding this comment

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

This is also documented and communicated like this.

Oh, where?

Do you know any app that does this?

Our (Struktur-proprietary) app needs to reject modifications to a user under certain conditions, throwing an exception from a changeUser-hook callback works great.

Copy link
Member

@MorrisJobke MorrisJobke Mar 6, 2019

Choose a reason for hiding this comment

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

This is also documented and communicated like this.

Oh, where?

I thought it was - but https://docs.nextcloud.com/server/stable/developer_manual/app/hooks.html doesn't has it. Maybe it was only on Github and never made it into the docs 😢.

Do you know any app that does this?

Our (Struktur-proprietary) app needs to reject modifications to a user under certain conditions, throwing an exception from a changeUser-hook callback works great.

If at all then there needs to be a pre event to differentiate between the use cases.

Copy link
Author

@leonklingele-work leonklingele-work Mar 6, 2019

Choose a reason for hiding this comment

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

Yup, see #14565.

$this->triggerChange('enabled', $enabled, $oldStatus);
$this->config->setUserValue($this->uid, 'core', 'enabled', $enabled ? 'true' : 'false');
}
}
Expand Down Expand Up @@ -407,9 +408,9 @@ public function setQuota($quota) {
$quota = OC_Helper::computerFileSize($quota);
$quota = OC_Helper::humanFileSize($quota);
}
$this->config->setUserValue($this->uid, 'files', 'quota', $quota);
if($quota !== $oldQuota) {
$this->triggerChange('quota', $quota);
$this->config->setUserValue($this->uid, 'files', 'quota', $quota);
$this->triggerChange('quota', $quota, $oldQuota);
}
}

Expand Down
20 changes: 4 additions & 16 deletions tests/lib/User/UserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,14 +676,8 @@ public function testSetEMailAddressNoChange() {
$config->expects($this->any())
->method('getUserValue')
->willReturn('[email protected]');
$config->expects($this->once())
->method('setUserValue')
->with(
'foo',
'settings',
'email',
'[email protected]'
);
$config->expects($this->never())
->method('setUserValue');

$user = new User('foo', $backend, $this->dispatcher, $emitter, $config);
$user->setEMailAddress('[email protected]');
Expand Down Expand Up @@ -741,14 +735,8 @@ public function testSetQuotaAddressNoChange() {
$config->expects($this->any())
->method('getUserValue')
->willReturn('23 TB');
$config->expects($this->once())
->method('setUserValue')
->with(
'foo',
'files',
'quota',
'23 TB'
);
$config->expects($this->never())
->method('setUserValue');

$user = new User('foo', $backend, $this->dispatcher, $emitter, $config);
$user->setQuota('23 TB');
Expand Down