fix:enable only for specific groups

[pako81]

Fixes: https://github.com/owncloud/enterprise/issues/6277

When only selecting the app option Enable only for specific groups, impersonation fails with an Can not impersonate error. Currently, the logic to impersonate the user is missing when only this app option is set. This PR just adds a call to the impersonateUser() method once all conditions are verified.

[CLAassistant]

All committers have signed the CLA.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[pako81]

There still is a combination which could be confusing and specifically:

MariaDB [owncloud10133]> select * from oc_appconfig where appid='impersonate';
+-------------+---------------------------------+------------------+
| appid       | configkey                       | configvalue      |
+-------------+---------------------------------+------------------+
| impersonate | enabled                         | ["test group"]   |
| impersonate | impersonate_all_groupadmins     | false            |
| impersonate | impersonate_include_groups      | true             |
| impersonate | impersonate_include_groups_list | ["test group 2"] |
| impersonate | installed_version               | 0.5.3            |
| impersonate | types                           |                  |
+-------------+---------------------------------+------------------+

You - as admin- basically end up in this configuration when first selecting test group 2 from the User authentication page and then go back to the apps page and select test group for the Enable only for specific groups option (the other way round is not possible as, as long the app option is set, the User authentication settings are no longer displayed).

In this configuration I would expect subadmins of test group to still be able to impersonate (since the correspondent code path should be called before the one for impersonate_include_groups_list) but this not the case: impersonate column is shown but icon is missing. For subadmins of test group 2 no column and no icon is shown.

Maybe this is expected as the two options should be considered as mutually exclusive?

[jvillafanez]

Maybe this is expected as the two options should be considered as mutually exclusive?

I think we have to test also with #236. For now, let's merge both patches and retest that scenario.

[jvillafanez]

I think that section is part of the validation, and the return value should be below

[jvillafanez]

The touched section seems to be part of the validation process in case the app is enabled for specific groups. Computation to decide whether impersonation should happen is below.
I think what's missing is including the "app is enabled for specific groups" piece in order to decide whether the user can impersonate or not.

Admins should be able to impersonate always, regardless of the "app enabled groups" setting.
If you aren't an admin, you should be able to impersonate only if you have the app enabled for your group. That's likely what's missing in the conditions in lines

impersonate/controller/settingscontroller.php

Lines 248 to 250 in 5642c9e

	if ($allowSubAdminsImpersonate === "true") {
	return $this->impersonateUser($impersonator, $target, $user);
	} elseif ($includedGroups !== '') {

Maybe something like below (needs refinement):

// admin is unconditionally allowed to impersonate
if ($this->groupManager->isAdmin($currentUser->getUID())) {
  return $this->impersonateUser($impersonator, $target, $user);
}

if (!$this->groupManager->isInGroup($impersonator, $appEnabledGroupIds)) {
  return new JSONResponse(['error' => 'cannotImpersonate', 'message' => $this->l->t('Can not impersonate')], http::STATUS_NOT_FOUND);
}

$includedGroups = $this->config->getValue('impersonate', 'impersonate_include_groups_list', '');
$allowSubAdminsImpersonate = $this->config->getValue('impersonate', 'impersonate_all_groupadmins', "false");
if ($allowSubAdminsImpersonate === "true") {
  return $this->impersonateUser($impersonator, $target, $user);
} elseif ($includedGroups !== '') {

That should cover the server-side validation, but we might still need changes in order to show the impersonation icon or not.

[pako81]

I think what's missing is including the "app is enabled for specific groups" piece in order to decide whether the user can impersonate or not.

But is this not what https://github.com/owncloud/impersonate/blob/master/controller/settingscontroller.php#L206-L238 exactly does? If $appEnabled !== "yes" means that we have an array of groups for the enabled configkey.

Also, I guess having:

if (!$this->groupManager->isInGroup($impersonator, $appEnabledGroupIds)) {
  return new JSONResponse(['error' => 'cannotImpersonate', 'message' => $this->l->t('Can not impersonate')], http::STATUS_NOT_FOUND);
}

outside of https://github.com/owncloud/impersonate/blob/master/controller/settingscontroller.php#L206-L238 would not work as there we are already outside the condition "enable app for certain groups" so i.e. $appEnabledGroupIds is not defined?

[pako81]

To be more clear, currently $this->impersonateUser() is called on three different conditions:

• https://github.com/owncloud/impersonate/blob/master/controller/settingscontroller.php#L242: when admin --> not our case
• https://github.com/owncloud/impersonate/blob/master/controller/settingscontroller.php#L248: when $allowSubAdminsImpersonate === "true" --> not our case
• https://github.com/owncloud/impersonate/blob/master/controller/settingscontroller.php#L255: when $includedGroups !== '' --> also not our case

So to my eyes we are currently missing the call for when only the "Enable only for specific groups" app option is set.

[jvillafanez]

The question is, what does having the "Enable only for specific groups" option imply?
Right now, I think that only members of those groups will be able to use impersonation, AND the target must also be a member of any of those groups. If both conditions apply, then we keep restricting the access according to the configuration of the app.

What isn't clear is whether the "target must also be a member of any of those groups" condition makes sense or not.

To clarify a bit, based on the example provided (#237 (comment)), the impersonator must be member of the "test group" and subadmin of "test group 2", and the target user must be member of both "test group" and "test group 2".

[pako81]

To clarify a bit, based on the example provided (#237 (comment)), the impersonator must be member of the "test group" and subadmin of "test group 2", and the target user must be member of both "test group" and "test group 2".

I would probably not consider that case as having both options enabled does not make sense to me.

What isn't clear is whether the "target must also be a member of any of those groups" condition makes sense or not.

Not sure this is still true but https://github.com/owncloud/impersonate/blob/master/controller/settingscontroller.php#L221-L222 seems to indicate this is needed.

[jvillafanez]

I guess we need the javascript piece to show the "logged in as" warning message on the top of the screen. The js code might not load if the app isn't enabled for the target user, that would explain why it's needed.

I'd vote for a "won't fix" and / or ensuring that the "Enable only for specific groups" option can't be used for the app.

[pako81]

Yes, I agree. So close this and remove the "Enable only for specific groups" option in a separate PR?