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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
lib/private/User: add reminder to first change 'enabled' property, th… 
…en issue event

A new event, 'preChangeUser', should be used for this instead.
See #14565.
  • Loading branch information
Leon Klingele committed Apr 1, 2019
commit 569e2801254b098f23fcd0f44c3fe3df174ca7f5
1 change: 1 addition & 0 deletions lib/private/User/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ public function setEnabled(bool $enabled = true) {
$oldStatus = $this->isEnabled();
$this->enabled = $enabled;
if ($oldStatus !== $this->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