Improve Setting class and other core classes that inherit from it

[sebastianpiskorski]

Goal of this ticket is to improve Piwik\Plugin\Settings class and/or Piwik\Settings\Setting and other core classes that inherits after latter one.

Classes should provide better control of who have access to data that they manage. Because this data takes part in rendering of specific views which are beyond possibilities of control by plugins.

Simplest way would be to provide a way to change SystemSetting::$writableByCurrentUser to false. If state of $writableByCurrentUser is such, then it is not rendered on Plugin settings page.
Be careful! If state of SystemSetting::$readableByCurrentUser is set to false, then page throws error.

[tsteur]

Be careful! If state of SystemSetting::$readableByCurrentUser is set to false, then page throws error.

That's on purpose. You can change this by changing $readableByCurrentUser to true if it is supposed to be readable by other users apart from the super user.

We could make $writableByCurrentUser public as well but I'm not sure if we should for SystemSetting. You could just create a new Setting class that extends it similar to SystemSetting class and set permissions to your need for now. See for example: https://github.com/piwik/piwik/blob/2.14.0-b7/core/Settings/SystemSetting.php

$readableByCurrentUser can be already changed

[tsteur]

Maybe you can also provide an example I do not yet really understand the problem. In the actual plugin settings only those settings are shown that a user has write permission, so there should be no problem.

All the settings by QueuedTracking are set to $readableByCurrentUser=true so when accessing it there should be no problem unless there is maybe a bug in Piwik platform.

[sebastianpiskorski]

@tsteur The problem is that SystemSettings class is used in cases where SystemSetting for many Plugins are used. And if all SystemSettings have single not modifiable interface, you don't have control (from any place in code) over what is displayed to user (except the path that was designed in first place and AFAIK piwik is supposed to be highly modifiable through plugins). Also part that is modifiable throws error when you modify it to desired settings.

Particular example is this url ?module=CoreAdminHome&action=adminPluginSettings

If I modify $readableByCurrentUser to false then Piwik displays Exception page, and other plugins settings are not accessible.
If I modify $readableByCurrentUser to false then Piwik hides those settings from current user.

It is either issue with CoreAdminHome plugin or Piwik\Settings\Setting class or both. Any way current implementation is problematic in management. Especially when you want to introduce user types other than hardcoded in Piwik\Access class.

[tsteur]

you don't have control (from any place in code) over what is displayed to user. (except the path that was designed in first place and AFAIK piwik is supposed to be highly modifiable through plugins)

That's on purpose currently to keep things simple until we specifically develop API's to make it possible

If I modify $readableByCurrentUser to false then Piwik displays Exception page, and other plugins settings are not accessible.
If I modify $readableByCurrentUser to false then Piwik hides those settings from current user.

If someone sets $readable... to false we could also set $writable to false automatically, that would work at least in your case. On the other side we might want to ignore $readable=false when someone has $writable=true permission as someone needs to read a value when having write permission. From what I understand is that you set $readable=false for super users? I still don't really get the problem but looks like this seems to be the problem? Also when reading:

introduce user types other than hardcoded in Piwik\Access class.

When kinda user type do you introduce there? Piwik is not really built for this so we might have to change a lot. Maybe easiest way would be to just hide the adminPluginSettings page entirely?

Apart from this it seems like a problem that a super user can have write permission without read permission. Although by definition a super user has in Piwik read permission to more or less anything.

[sebastianpiskorski]

@tsteur Problem lays here https://github.com/PiwikPRO/plugin-CloudAdmin/issues/63

We have superUser and magicAdmin. magicAdmin stands above superUser with permissions at least in the case of Cloud. This magicAdmin is admin user for whole cloud. AFAFIK we cannot influence superUser in matter of Setting but we need to some times.

We cannot hide adminPluginSettings page entirely because some of setting from other plugins that should be available only for admin are displayed there.

I think that Setting classes even System Settings should be modifiable at least under some conditions.

[tsteur]

I thought about it for a while and think we could change SystemSetting to be writable only if readable and writable is true: https://github.com/piwik/piwik/blob/2.14.0-b10/core/Settings/SystemSetting.php#L61 this should fix your problem?

If not I think in this case an event (when getting all settings that are writable by current user eg here https://github.com/piwik/piwik/blob/2.14.0-b10/core/Plugin/Settings.php#L137 or https://github.com/piwik/piwik/blob/2.14.0-b10/core/Settings/Manager.php#L101) should be more appropriate than allowing users to change $isWritableByCurrentUser but would be good if we didn't need that event.

[sebastianpiskorski]

@tsteur This doesn't slove anything, because SystemSetting is writable only if user has SuperUser access and in this case we want to change it. We want that superUser don't have access to some of SystemSettings, and now he have ti by design. And we can not modify $isWritableByCurrentUser setting. $isReadableByCurrentUser setting throws an exception when set to false, which is useless from UI point of view.

To explain what I mean it in simple words. I think that superUser is too hardcoded in Piwik and we cannot modify who is superUser for real. We have a need to modify some of superUser privileges. From my point of view, there should be only normal users and all access should be defined by configuration. Like in ACL design pattern.

[tsteur]

$isReadableByCurrentUser setting throws an exception when set to false, which is useless from UI point of view.

That's what I'm saying is the bug. Once we make the change there should no exception anymore and it should be not displayed in the UI.

From my point of view, there should be only normal users and all access should be defined by configuration. Like in ACL design pattern.

That would be a different issue see eg #1568

[sebastianpiskorski]

That's what I'm saying is the bug. Once we make the change there should no exception anymore and it > should be not displayed in the UI.

That would be nice but it will solve only part of the problem. Because one thing is that setting shouldn't be writeable by the user other than magicAdmin. It should be readable by the Tracker and I'm not sure on which user Tracker works. So I'm not sure if operating only on isReadableByCurrentUser will solve whole issue.

Maybe there should be some third parameter like isUnvailableForUser, on the other side it will create more complex code which is not so good.

[tsteur]

Tracker wouldn't be a problem as it will be readable there. I reckon re the other concern an event to "filter" writable settings might be best solution and maybe there should be a better solution re the magicAdmin some time so that it is part of the platform and not a custom solution.

[sebastianpiskorski]

Possibility to filter available setting would solve the issue if it will disable settings for user. Even if the user will cook up request it should be thrown away.

[mattab]

maybe there should be a better solution re the magicAdmin some time so that it is part of the platform and not a custom solution.

@tsteur That's a new idea, and maybe best solution for mid/long term...

@sebastianpiskorski is this issue a blocker for you, or just a suggestion?
I'm wondering what we should do and whether it's urgent :)

[sebastianpiskorski]

@mattab This is a suggestion and also a nice step in direction of access control granularity I think. At least in the area of Piwik Settings. So far I've manage to overcome this limitation.

[sgiehl]

This topic didn't come up again in the last few years, so I'll close it for now. Not even sure if the same problem still exists, as there has been a lot happened to the code since 2015. If it comes up again, we can create a new one.