FIX #35655 Check if API user has rights to see all thirdparties

[JonBendtsen]

FIX #35655 Check if API user has rights to see all thirdparties

This PR is a fix for #35655, checking if the api user/key has access to the thirdparty used in the contract

[JonBendtsen]

The new error message

{
  "error": {
    "code": 403,
    "message": "Forbidden: Access not allowed for login *********. No read permission on all thirdparties."
  }
}

[JonBendtsen]

errors does not seem to be in my changes

[hregis]

@JonBendtsen
we should also check $user->hasRight('societe', 'lire') ?
otherwise a user may not have the right $user->hasRight('societe', 'client', 'voir') but be associated as a salesperson for the client ! You should check whether the user is a salesperson for the client or not, otherwise you risk breaking something.

[JonBendtsen]

@JonBendtsen we should also check $user->hasRight('societe', 'lire') ? otherwise a user may not have the right `$user->hasRight('societe', 'client', 'voir')

if they have $user->hasRight('societe', 'client', 'voir') wouldn't they also have $user->hasRight('societe', 'lire') ?

How do I check if they are a sales person for the client?

[hregis]

@JonBendtsen

If the user does not have "$user->hasRight('societe', 'lire')", they do not have the right.

If the user has "$user->hasRight('societe', 'lire')" but not "$user->hasRight('societe', 'client', 'voir')", it is necessary to check if they are associated as a salesperson with the client.

[hregis]

@JonBendtsen check function getSalesRepresentatives() in societe.class.php (use $mode parameter with value 1)

[hregis]

@JonBendtsen maybe you can see and use the "protected static function _checkAccessToResource()" in api.class.php

[JonBendtsen]

@JonBendtsen maybe you can see and use the "protected static function _checkAccessToResource()" in api.class.php

yeah, but I need to get the socid, and there is no $socid, would that be during the loop, and field == 'socid' then I use the $value?

[hregis]

@JonBendtsen when you have a fetch of contract before you can use "$contract->socid" and for the "post" check if "socid" exists and use "$request_data['socid']"
just if the user not have "$user->hasRight('societe', 'client', 'voir')"

[JonBendtsen]

@hregis do you like this solution?

		$deny_access = true;
		$why_deny_access = '';
		if (DolibarrApiAccess::$user->hasRight('societe', 'lire')) {
			if (DolibarrApiAccess::$user->hasRight('societe', 'client', 'voir')) {
				$deny_access = false;
			} else {
				$why_deny_access = 'Extend access to all third parties';
				if (DolibarrApi::_checkAccessToResource('societe', $this->contract->socid)) {
					$deny_access = false;
				} else {
					$why_deny_access = $why_deny_access.' and NOT sales representative for this thirdparty='.$this->contract->socid;
				}
			}
		} else {
			$why_deny_access = 'Read third parties linked to user';
		}
		if ($deny_access) {
			throw new RestException(403, 'Access not allowed for login '.DolibarrApiAccess::$user->login.' - missing permissions: '.$why_deny_access);
		}

See results in comments below

[JonBendtsen]

{
  "error": {
    "code": 403,
    "message": "Forbidden: Access not allowed for login dash.board - missing permissions: Read third parties linked to user"
  }
}

[JonBendtsen]

{
  "error": {
    "code": 403,
    "message": "Forbidden: Access not allowed for login dash.board - missing permissions: Extend access to all third parties and NOT sales representative for this thirdparty=100"
  }
}

[hregis]

@JonBendtsen no no... Just a moment!

[JonBendtsen]

{
  "module": null,
  "id": "2",
  "entity": "1",
  "import_key": null,
  "array_options": [],
  "array_languages": null,
  "contacts_ids": null,
  "contacts_ids_internal": null,
  "linkedObjectsIds": [],
  ...

[JonBendtsen]

{
  "module": null,
  "id": "2",
  "entity": "1",
  "import_key": null,
  "array_options": [],
  "array_languages": null,
 ...

[JonBendtsen]

@JonBendtsen no no... Just a moment!

@hregis Do you like more the method used in the post function? that does save a lot of coding

[JonBendtsen]

PUT updated to use same method as in POST, and "he's checking it twice" ;-) both if there is access to the old socid and the new socid if it is being updated

[JonBendtsen]

@eldy did this PR get burried and out of your sight?

[eldy]

Using the get of another api is doing a different job that what we want. We just want to check if thirdparty exists.
For this, a standard :

$thirdpartytmp = new Societe($db);
$thirdparty_result = $thirdpartytmp>fetch($socid);
if ($thirdparty_result <= 0) {
    throw new RestException(404, 'Thirdparty not found or not allowed');
}

is enough (we don't need to load the discounts, check the massemailing status, ... done by the get of API).
If use with permission to create contract has no permission on thirdparties, he should not have more detail if thirdparty exists or not (only the one who haspermission to read thirdparties is allowed to get this information). This is another reason why we should not use the getofapi but the get of thirdparty here.
It is samefor the put.

[JonBendtsen]

@eldy I have made the requested changes - but I can not get PUT to work reliably

If I use a non existing socid, then it fails as it should.
If I don't have access to the existing socid in the contract, then it fails as it should.

If I do have access to the existing socid in the contract and I update with the same socid id it works, BUT if I try to change this to a new socid that I do not have access to, then it does indeed make the change to the socid to the one without access, and then it tells me I have no access.

I do not know how to fix this issue :-(