Fix personal setting for apps which are not whitelisted by sharidas · Pull Request #36258 · owncloud/core

sharidas · 2019-10-08T17:09:44Z

When apps are not whitelisted for guest user
they should not be visible in the personal settings.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

When apps are not enabled for guest users, they should not be listed in personal settings. In this change a new method is introduced in the user class getExtendedAttributes(). The purpose of this method is to grab the attributes related to the user for the apps. So when this method is invoked an event is emitted which is listened by the apps, and populate the event array with the attributes in the form of key => value. So when logged in as guest user the guest app would populate the event array with isGuest and whiteListedApps. Which could be used by core or any other apps. This feature is used SettingsManagers loadPanel to filter out the apps which are not whitelisted.

Related Issue

Fixes <issue_link>

Motivation and Context

Do not show the apps which are not whitelisted for the guest user.

How Has This Been Tested?

Enable customgroups. Make sure its not listed as whitelisted apps in guest configuration.
As guest user navigate to the settings page.
The settings page should not show customgroups. And it does not.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

lib/private/App/AppManager.php

lib/private/User/User.php

jvillafanez · 2019-10-09T14:46:49Z

lib/private/App/AppManager.php

+			/**
+			 * Check if the user is guest user, if so get the apps which it is allowed for.
+			 */
+			if (\array_key_exists('guests', $userAppAttributes)) {


if (isset($userAppAttributes['guests']['whitelistedApps']) && !\in_array($appName, $userAppAttributes['guests']['whitelistedApps'], true))

this seems a bit better, thought we might need to change it a bit.

Note that we might need to check if the whitelistedApps is an array.
In addition, clarify what is the expected behaviour:

when the "guest" and / or "whitelistedApps" don't exists.

when the "whitelistedApps" is null / empty / string

when the "whitelistedApps" is an empty array

any other additional case.

If guests and whitelistedApps don't exist, then do the normal checks which already existed before this change.

If the whitelistedApps is empty / null / string, then continue with the checks which already existed before this change.

Expectation is whitelistedApps should be an array we would check if the appName is there in any of the elements in the array. Expectation here is when the code will access $userAttributes['guests']['whitelistedApps'] we should get a list of strings.

When whitelistedApps is an empty array then proceed ahead with further checks which were already present before this change.

lib/public/User/UserExtendedAttributesEvent.php

jvillafanez · 2019-10-09T14:58:24Z

lib/public/User/UserExtendedAttributesEvent.php

+	 * @param string $appName
+	 * @param array $data
+	 */
+	public function setAttributes($app, $data) {


Clarify and document clear how this method will behave. Note that it's expected that multiple apps will be listening this event and will use this method to add their own attributes. This means that apps can easily overwrite information of other apps.

What params do we need? Do we need the app name? What for?

What "data" do we need? I expected something like setAttributes(string $attrName, mixed $attrValue), where the attrValue could be an string or an array (or maybe anything)

How are we going to deal with multiple apps? I think we could have a "first in wins" policy: if the attribute doesn't exists, write it and return false; it it already exists, ignore it and return false. This needs to be documented anyway.

How are we going to deal with multiple apps? I think we could have a "first in wins" policy: if the attribute doesn't exists, write it and return false; it it already exists, ignore it and return false

There is another possibile flaw I see here, which is say for example if we use the policy to only add new attributes which don't exist and leave if the attribute already exist. Then it could also happen for example the user updated the app attribute and we might miss that, because we don't update. We only add the new attributes and skip if the attribute already exists. imho, shouldn't we consider to add/update it anyway? Or did I misread your feedback, please let me know your opinion.

The app can detect that case by checking the returned value and take a decision.

Overwriting might be more problematic because that case is more difficult to detect. Debugging might be also more difficult because we don't know which app overwrote the attribute. You'll see a different value than the one you could expect.

Note that there is no specific order in which the apps will be notified, so we have to consider it's a random order. This means that checking the returned value should be considered as mandatory. As said, documenting this is very important.

Yes you are right, the app can do the check and take the decision. So before updating make sure whether the app should update or not. Sure I will document these in my next pr update.

lib/private/User/User.php

lib/private/App/AppManager.php

lib/private/User/User.php

jvillafanez · 2019-10-10T11:42:39Z

lib/private/User/User.php

 	}
+
+	/**
+	 * get the attributes of user for apps


Document this method extensively, specially because the method uses an event and could take a while to get a response. The method isn't cheap to keep calling it over and over.
In addition, consider to include a cache here to speed up the next calls.

I have added documenation for this method. Let me know if this looks ok

I think most of the documentation, if not all, should be copied / moved to the interface. I expect people to check the interface first.

lib/public/IUser.php

lib/public/User/UserExtendedAttributesEvent.php

jvillafanez · 2019-10-10T12:30:44Z

lib/public/User/UserExtendedAttributesEvent.php

+	 * @param string $appName
+	 * @return mixed
+	 */
+	public function getAttribute($appName) {


As far as I know, this isn't needed. Keep things as easy as possible. Adding more methods will increase the confusion.
We only need one method for the listeners to add new user attributes, and another one for the User class to get those new attributes, nothing more.

lib/public/User/UserExtendedAttributesEvent.php

jvillafanez · 2019-10-10T12:55:11Z

One additional thing: the event is "owned" or "controlled" by the User class. This means that it's the User class the one that should decide the format of the event.

The User class doesn't need to know what app has filled the event, and it doesn't have any need for the app name. In addition, the getExtendedAttributes method is expected to return user attributes in a "attr => value" fashion. Returning app names isn't part of the specification.
The expectation is that the method will return something like:

"isGuest" => true,
"loginName" => "Joe_Joe",
"dateOfBirth" => "3/3/1954",
"blogSite" => "https://server/umpa/lumpa"
.....

Noone cares which app added the "blogSite" key.

lib/private/App/AppManager.php

lib/private/User/User.php

jvillafanez · 2019-10-11T07:52:26Z

lib/private/User/User.php

 	}
+
+	/**
+	 * get the attributes of user for apps


I think most of the documentation, if not all, should be copied / moved to the interface. I expect people to check the interface first.

lib/private/User/User.php

lib/public/User/UserExtendedAttributesEvent.php

jvillafanez · 2019-10-11T10:15:11Z

lib/private/App/AppManager.php

+			$userAppAttributes = $user->getExtendedAttributes();
+			/**
+			 * Guests will only have access to some whitelisted apps
+			 * - If the list is missing (no "whitelistedAppsForGuests" key found), the guest will have access to all the apps as any normal user. The same will happen if the "whitelistedAppsForGuests" isn't an array.


adjust text here: if the "whitelistedAppsForGuests" isn't found, the user isn't considered a guest, so he'll have access to all enabled apps.

lib/private/User/User.php

lib/public/User/UserExtendedAttributesEvent.php

jvillafanez · 2019-10-11T11:32:49Z

I think we'll need to protect some methods to be called from the apps. It could be risky to let the apps modify things, and calling again the "getExtendedAttributes" could trigger an infinite loop.

The idea I have for this is to change all these "problematic" methods to check for a flag and throw an exception if the flag is set. This flag will be private, set as active before sending the event, and disable the flag after the sending the event.
I think this is an easy way to allow or deny what methods can be called inside the listeners.

sharidas · 2019-10-14T08:38:00Z

Well passing user object can be dangerous in one way, the apps can change user attributes through the user object. So lets say if the user id is passed, which is also dangerous because the app can access the same user object via userManager. I did an experiment using clone of user object, but unfortunately that does not help here. Because its a shallow copy and say if we try to change the display name, the change does reflect in the accounts table.
I would like to grab inputs here, if we have to protect the set methods in the user object which can alter the user attributes? Another worrying factor here as @jvillafanez suggested in #36258 (comment) if the app call getExtendedAttributes might cause an infinite loop? I would like input not just about the infinete loop, but the complete content of the comment #36258 (comment).

@micbar @PVince81 @VicDeo Let me know your thoughts regarding the same as this input will help us to move ahead with the pr.

codecov · 2019-10-16T18:27:15Z

Codecov Report

Merging #36258 (517931f) into master (a147dd8) will increase coverage by 0.02%.
The diff coverage is 96.22%.

@@             Coverage Diff              @@
##             master   #36258      +/-   ##
============================================
+ Coverage     65.00%   65.02%   +0.02%     
- Complexity    19772    19798      +26     
============================================
  Files          1271     1273       +2     
  Lines         74696    74745      +49     
  Branches       1309     1309              
============================================
+ Hits          48554    48602      +48     
- Misses        25756    25757       +1     
  Partials        386      386

Flag	Coverage Δ	Complexity Δ
javascript	`54.00% <ø> (ø)`	`0.00 <ø> (ø)`
phpunit	`66.23% <96.22%> (+0.02%)`	`0.00 <25.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/RemoteUser.php	`4.00% <0.00%> (-0.17%)`	`24.00 <1.00> (+1.00)`	⬇️
lib/private/App/AppManager.php	`91.15% <100.00%> (+0.20%)`	`56.00 <12.00> (+4.00)`
lib/private/User/User.php	`90.82% <100.00%> (+1.87%)`	`92.00 <5.00> (+14.00)`
lib/public/User/NotPermittedActionException.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
lib/public/User/UserExtendedAttributesEvent.php	`100.00% <100.00%> (ø)`	`6.00 <6.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a147dd8...517931f. Read the comment docs.

lib/private/User/User.php

lib/public/NotPermitUserAccountException.php

jvillafanez · 2019-10-17T07:19:11Z

lib/private/User/User.php

+			 * user.
+			 */
+			$this->allowUserAccountUpdate = false;
+			$this->eventDispatcher->dispatch(UserExtendedAttributesEvent::USER_EXTENDED_ATTRIBUTES, $userExtendedAttributesEvent);


we need to handle exceptions triggered from inside the listeners, specially because the flag could remain active.

I have updated the code to handle the exception thrown. Also I have updated the AppManager where getExtendedAttributes is called. I have added try/catch block. The catch block is kept empty. So that the code flow will happen as if its a regular user and not a guest user.

tests/lib/User/UserExtendedAttributesEventTest.php

tests/lib/User/UserTest.php

jvillafanez · 2019-10-17T07:45:06Z

BTW, we'll need tests for the changes made in the User class

sharidas · 2019-10-17T10:22:51Z

BTW, we'll need tests for the changes made in the User class

I have updated the tests for the exceptions thrown for the User class.

lib/private/User/User.php

tests/lib/User/UserExtendedAttributesEventTest.php

tests/lib/User/UserTest.php

lib/public/User/UserExtendedAttributesEvent.php

jvillafanez · 2019-10-17T11:37:51Z

Some additional notes:

the getExtendedAttributes method should be "protected by the NotPermittedAction". It could be called from the listener, and it will cause an infinite loop.
It's particularly critical to ensure the allowUserAccountUpdate flag is reset after the event is triggered. We'll need unittests covering every related scenario (normal execution, NotPermittedActionException being thrown, random exception being thrown....)
Any use case where we want to rethrow the exception triggered inside the listeners? Note that it isn't just the NotPermittedActionException but also any other exception that could happen within the listeners.

We could probably assume that the exception will be taken care within the listeners, so if any exception leaks, it's probably a big one and needs to be know, so rethrowing instead of swallowing might be fine. In addition, I'm not sure if the event will reach all the listeners if it's the first one the one that throws an exception (we should verify this case).

sharidas · 2019-10-17T14:40:33Z

The getExtendedAttributes is now protected with the flag. So that the listeners cannot call this method, which might cause infinite loop.
Updated the unit test for the flag to test
- normal execution. That is after the normal execution the flag is reset to true.
- With exception NotPermittedActionException. After this exception is thrown, verified that the value of flag is set to true.
- Tested with generic exception Exception. After this exception is thrown, verified that the value of the flag is set to true.
With the current approach the event does not reach the other listeners if the initial/first listener throws the exception. So for instance I added a test code in the comments app ( as this app is found first in the list ), and tried to set the display name. I observed that code did not reached guests listener.
- One solution I see is to catch the exception NotPermittedActionException in the getExtendedAttributes() and then just log it inside the catch block. I think for this method we can catch the NotPermittedActionException exception. But if any other exception comes then lets allow the exception to bubble up? Or another way is we can ask the listeners to catch the exception? Even that way it should work imho. This second solution is like completely relying on apps. If the apps are logging the exception with the appName, then it would be helpful to debug the cause of exception. Personally I am in favour of logging the exception of NotPermittedActionException in the User class and then move ahead. And for any other exceptions let the listener handle it or let the exception bubble up.

jvillafanez · 2019-10-18T07:39:31Z

With the current approach the event does not reach the other listeners if the initial/first listener throws the exception. So for instance I added a test code in the comments app ( as this app is found first in the list ), and tried to set the display name. I observed that code did not reached guests listener.

Looks like it isn't possible to force the event to reach all the listeners. We'd need a custom eventDispatcher (which is out of scope at the moment).
So, for the getExtendedAttributes method, I think the best solution is to let the exceptions (even the NotPermittedActionException) bubble up out of the method because it can't handle those. We'll have to rely on the listeners not to leak any exception.

Get extended attributes for user from the apps. A new method which emits events to get the attributes for user, which is listened by apps to provide the attributes for the specific user. Use case for example: When apps are not whitelisted for guest user they should not be visible in the personal settings. Signed-off-by: Sujith H <sharidasan@owncloud.com>

jvillafanez · 2019-10-18T08:00:20Z

lib/private/User/User.php

+		 * using empty because userExtendedAttributes could either be null or empty array
+		 * or an array of attributes
+		 */
+		if ($clearCache || !isset($this->userExtendedAttributes) || [] === $this->userExtendedAttributes) {


We don't need to retrigger the event if the userExtendedAttributes is an empty array.
You triggered the event, no one listened and you get an empty array. This is a valid result. There is no need to trigger the event again to get another empty array.

jvillafanez · 2019-10-18T08:03:15Z

Hopefully last change. Please make sure the comments are updated and match the code behaviour.

PVince81 · 2019-10-18T10:06:50Z

I don't feel comfortable with this approach, it seems it doesn't fully suit.

Looking at the guests app we are setting an attribute "whitelistedAppsForGuests" which sounds like a generic setting, not something specific to a user. So having this in a user specific attribute feels wrong.

Thinking further: if the user attribute is something that defines what is accessible for a user, then maybe it fits more into a "roles" concept or "permissions" concept, and in this case the attribute would need to be called "whitelistedApps", as it applies only for this one user.

Maybe we should rather introduce the concept of "app access" into core in a more API-like way instead of just attributes. We already have the concept of "enabled apps for users" but here it's a softer one as the app needs to be loaded and its elements need to be discarded.

In a broader view, an app should have a way to find out whether the current user is supposed to see its controls or not, so having an API for "app access" or "app permissions" might be more suitable.

@DeepDiver1975 @micbar any thoughts on this ? does this fit somehow into the discussions related to roles concept ?

jvillafanez · 2019-10-18T10:41:55Z

The problem we've noticed at the last moment is that the all the apps are required to be loaded before calling the getExtendedAttributes method.
For this particular case, the guests app is required to be loaded so we can know what other apps are whitelisted. In the worst case the guests app is loaded the last, we might wrongly assume that the rest of the apps can be loaded and be accessed normally by that user.

I think the idea is worth to keep because it has other uses such as properly expose the login name of the user, expose LDAP attributes for LDAP users, or other information for shibboleth or openIdConnect.

Looking at the guests app we are setting an attribute "whitelistedAppsForGuests" which sounds like a generic setting, not something specific to a user. So having this in a user specific attribute feels wrong

The original idea was to use 2 keys: the "isGuest" in order to identify guests users, and the "whitelistedApps" to include the list. "whitelistedAppsForGuests" was a simple way to merge both keys since the intention was to use that key for both identify a guest user and get the list of apps.

In any case, I'm starting to think this approach won't work for guests. Guests are added in the DB backend and they aren't inside a "guests" backend part of the guests app. When the guests app is disabled, guests users will still be available, but we won't be able to identify them as a guest with this approach because the app will be disabled.

PVince81 · 2019-10-18T11:27:17Z

didn't we have an event (hook) for the moment after which all apps have finished loading ?

sharidas · 2019-10-18T12:00:34Z

didn't we have an event (hook) for the moment after which all apps have finished loading ?

Searched the code base in core, and found this https://github.com/owncloud/core/blob/master/lib/private/legacy/app.php#L162
And this would return false https://github.com/owncloud/core/blob/master/lib/private/legacy/hook.php#L91

Other than this I couldn't find further references.

mmattel · 2022-07-13T08:14:08Z

@phil-davis @sharidas is there any progress?

phil-davis · 2022-07-13T09:54:12Z

@phil-davis @sharidas is there any progress?

This PR got "left behind" a long time ago. I have no idea if anyone will (or should) take it over.

mmattel · 2022-07-30T17:54:51Z

@jvillafanez maybe you could take a look on to bring this forward...

jvillafanez · 2022-08-01T07:33:55Z

Looks good to me. Maybe @DeepDiver1975 can have a second look for the approach.
We might need to rebase the branch and retest.

phil-davis · 2022-08-01T08:05:57Z

We might need to rebase the branch and retest.

Rebased in #40257 - we can review and comment there, and if needed and good it can easily be merged from there.

phil-davis · 2022-08-02T07:50:07Z

PR #40257 is passing now. I had to just fixup some code-style and unit test things. So someone can comment there if this is wanted, and if the code is OK.

phil-davis · 2022-08-09T14:13:31Z

This code has been merged in PR #40257

sharidas added the 2 - Developing label Oct 8, 2019

sharidas added this to the development milestone Oct 8, 2019

sharidas self-assigned this Oct 8, 2019

sharidas force-pushed the fix-personal-setting branch 3 times, most recently from fe9ab19 to 6aab82f Compare October 9, 2019 10:39

jvillafanez reviewed Oct 9, 2019

View reviewed changes

lib/private/App/AppManager.php Outdated Show resolved Hide resolved

lib/private/App/AppManager.php Outdated Show resolved Hide resolved

lib/private/App/AppManager.php Outdated Show resolved Hide resolved

lib/private/User/User.php Outdated Show resolved Hide resolved

sharidas force-pushed the fix-personal-setting branch 2 times, most recently from 485f4aa to 62b28ca Compare October 9, 2019 13:11

jvillafanez reviewed Oct 9, 2019

View reviewed changes

sharidas force-pushed the fix-personal-setting branch from 62b28ca to 5efe9da Compare October 10, 2019 10:44

jvillafanez reviewed Oct 10, 2019

View reviewed changes

sharidas force-pushed the fix-personal-setting branch 2 times, most recently from 8006e75 to 8a3ddc9 Compare October 11, 2019 06:46

jvillafanez reviewed Oct 11, 2019

View reviewed changes

sharidas force-pushed the fix-personal-setting branch 2 times, most recently from 9905cbb to 25b5a55 Compare October 11, 2019 10:40

jvillafanez reviewed Oct 11, 2019

View reviewed changes

sharidas force-pushed the fix-personal-setting branch 2 times, most recently from fb08245 to c677130 Compare October 11, 2019 15:18

sharidas force-pushed the fix-personal-setting branch 4 times, most recently from fbf23af to 2dd0252 Compare October 16, 2019 17:02

sharidas force-pushed the fix-personal-setting branch 2 times, most recently from dab996e to e586d8b Compare October 17, 2019 06:50

sharidas force-pushed the fix-personal-setting branch from e586d8b to c119b28 Compare October 17, 2019 07:07

jvillafanez reviewed Oct 17, 2019

View reviewed changes

sharidas force-pushed the fix-personal-setting branch 2 times, most recently from 769e1a1 to 04a3289 Compare October 17, 2019 10:16

jvillafanez reviewed Oct 17, 2019

View reviewed changes

sharidas force-pushed the fix-personal-setting branch from 04a3289 to 1c6bdc8 Compare October 17, 2019 13:52

sharidas changed the title ~~[WIP] Fix personal setting for apps which are not whitelisted~~ Fix personal setting for apps which are not whitelisted Oct 18, 2019

sharidas force-pushed the fix-personal-setting branch from 1c6bdc8 to 517931f Compare October 18, 2019 07:39

jvillafanez reviewed Oct 18, 2019

View reviewed changes

phil-davis mentioned this pull request Aug 1, 2022

[full-ci] Get extended attributes for user for apps #40257

Merged

11 tasks

phil-davis closed this Aug 9, 2022

phil-davis mentioned this pull request Sep 9, 2022

Provide extended attributes when the event is emitted owncloud/guests#518

Merged

9 tasks

Conversation

sharidas commented Oct 8, 2019

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jvillafanez commented Oct 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jvillafanez commented Oct 11, 2019

Uh oh!

sharidas commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jvillafanez commented Oct 17, 2019

Uh oh!

sharidas commented Oct 14, 2019 •

edited

Loading

codecov bot commented Oct 16, 2019 •

edited

Loading

sharidas commented Oct 17, 2019 •

edited

Loading