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

JonBendtsen · 2025-10-30T22:16:49Z

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 · 2025-10-30T22:27:29Z

The new error message

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

JonBendtsen · 2025-10-30T22:28:19Z

errors does not seem to be in my changes

hregis · 2025-10-31T06:46:16Z

@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 · 2025-10-31T06:59:29Z

@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 · 2025-10-31T07:09:01Z

@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 · 2025-10-31T07:15:09Z

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

hregis · 2025-10-31T07:20:13Z

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

JonBendtsen · 2025-10-31T07:22:29Z

@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 · 2025-10-31T07:30:39Z

@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 · 2025-10-31T17:49:05Z

@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 · 2025-10-31T17:49:55Z

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

JonBendtsen · 2025-10-31T17:50:54Z

{
  "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 · 2025-10-31T17:52:17Z

@JonBendtsen no no... Just a moment!

JonBendtsen · 2025-10-31T17:52:46Z

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

JonBendtsen · 2025-10-31T17:54:07Z

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

JonBendtsen · 2025-10-31T19:54:11Z

@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 · 2025-11-02T10:24:27Z

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 · 2025-11-09T08:08:33Z

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

eldy · 2025-11-09T17:19:14Z

htdocs/contrat/class/api_contracts.class.php

+
+		$socid = (int) $request_data['socid'];
+		$thirdparties = new Thirdparties();
+		$thirdparty_result = $thirdparties->get((int) $socid);


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.

@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 :-(

@eldy I still do not know how to fix the test described just above

Check if API user has rights to see all thirdparties - though perhaps we should check if the user has rights to this particular thirdparty in this contract?

…f permissions and/or is the sales representative for the thirdparty

…for access

…at the user does not have permission to, then it STILL updates the contract, and then it gets the contract after update and tells me I do not have access

JonBendtsen · 2025-11-24T20:08:46Z

retesting POST with rebased v22 - NON existing socid

{
"socid": "22",
"date_contrat": 1759738652,
"commercial_signature_id": 1,
"commercial_suivi_id": 1,
"note_public": "TEST 22.0.2"
}

no permissions

read and write contacts

{
  "error": {
    "code": 404,
    "message": "Not Found: Thirdparty with id=22 not found or not allowed"
  }
}

existing socid

{
"socid": "100",
"date_contrat": 1759738652,
"commercial_signature_id": 1,
"commercial_suivi_id": 1,
"note_public": "TEST 22.0.2"
}

Response body

{
  "error": {
    "0": "Failed to add contract",
    "code": 500,
    "message": "Internal Server Error: Error creating contract"
  }
}

Dolibarr.log

BEGIN Transaction
 sql=INSERT INTO llx_contrat (datec, fk_soc, fk_user_author, date_contrat, fk_commercial_signature, fk_commercial_suivi, fk_projet, ref, entity, signed_status, note_private, note_public, ref_customer, ref_supplier, ref_ext) VALUES ('2025-11-24 20:54:08', 100, 23, '2025-10-06 10:17:32',1,1,NULL, null, 1, 0, NULL, 'TEST 22.0.2', NULL, NULL, NULL)
 sql=UPDATE llx_contrat SET ref='(PROV3)' WHERE rowid=3
 Contrat::add_contact 1, SALESREPSIGN, internal, 0
 sql=SELECT COUNT(*) as cnt FROM llx_societe_commerciaux as sc WHERE sc.fk_soc = 100 AND sc.fk_user = 23
 Contrat::add_contact The user 23 has not the permission to see all thirdparties and is not an allowed sale representative of the company.
 Contrat::create - 20 - Failed to add contract
ROLLBACK Transaction

Maybe we should catch the permission even before this.

assign sales representative to 100

All ok

JonBendtsen · 2025-11-24T20:13:31Z

Conclusion

@eldy I am giving up.

new post function

	public function post($request_data = null)
	{
		if (!DolibarrApiAccess::$user->hasRight('contrat', 'creer')) {
			throw new RestException(403, "Missing permission: Create/modify contracts/subscriptions");
		}

		$socid = (int) $request_data['socid'];
		$thirdpartytmp = new Societe($this->db);
		$thirdparty_result = $thirdpartytmp->fetch($socid);
		if ($thirdparty_result < 1) {
			throw new RestException(404, 'Thirdparty with id='.$socid.' not found or not allowed');
		}
		if (!DolibarrApi::_checkAccessToResource('societe', $thirdpartytmp->id)) {
			throw new RestException(404, 'Thirdparty with id='.$thirdpartytmp->id.' not found or not allowed');
		}

		dol_syslog("socid=".$socid);
		dol_syslog("thirdparty_result=".$thirdparty_result);
		dol_syslog("thirdpartytmp->id=".$thirdpartytmp->id);

		// Check mandatory fields
		$result = $this->_validate($request_data);

		foreach ($request_data as $field => $value) {
			if ($field === 'caller') {
				// Add a mention of caller so on trigger called after action, we can filter to avoid a loop if we try to sync back again with the caller
				$this->contract->context['caller'] = sanitizeVal($request_data['caller'], 'aZ09');
				continue;
			}

			$this->contract->$field = $this->_checkValForAPI($field, $value, $this->contract);
		}
		/*if (isset($request_data["lines"])) {
		  $lines = array();
		  foreach ($request_data["lines"] as $line) {
			array_push($lines, (object) $line);
		  }
		  $this->contract->lines = $lines;
		}*/
		if ($this->contract->create(DolibarrApiAccess::$user) < 0) {
			throw new RestException(500, "Error creating contract", array_merge(array($this->contract->error), $this->contract->errors));
		}

		return $this->contract->id;
	}

so I am checking if my user has access to this thirdparty, and apparently I do even though this users permissions are:

AND this user is not even sales representative for the socid=100

Okay so I generated a new key for this user and I still have access.

Is the function DolibarrApi::_checkAccessToResource even working correctly?

JonBendtsen · 2025-11-24T20:16:22Z

trying in branch 22.0, commit `7d403a2`

POSTING

{
"socid": "100",
"date_contrat": 1759738652,
"commercial_signature_id": 1,
"commercial_suivi_id": 1,
"note_public": "TEST 22.0.2"
}

and it replies

{
  "error": {
    "0": "Failed to add contract",
    "code": 500,
    "message": "Internal Server Error: Error creating contract"
  }
}

so it's not my branch

JonBendtsen · 2025-11-24T21:20:05Z

@eldy how come the fallback when checking for access to resources is True?

function checkUserAccessToObject
https://github.com/JonBendtsen/dolibarr/blob/22.0/htdocs/core/lib/security.lib.php#L1267

JonBendtsen · 2025-11-24T21:38:35Z

I think the issue is in this segment of the function

https://github.com/JonBendtsen/dolibarr/blob/22.0/htdocs/core/lib/security.lib.php#L1049

@eldy adding this little check will deny access

			} elseif (isModEnabled('societe') && !($user->hasRight('societe', 'lire') && !$user->hasRight('societe', 'client', 'voir'))) {
				dol_syslog("security.lib.php::checkUserAccessToObject True: (isModEnabled('societe') && !(user->hasRight('societe', 'lire') && !user->hasRight('societe', 'client', 'voir')))", LOG_DEBUG);
				return false;

My test user is not sales representative for this socid, and my test user has 0 permissions on any thirdparty.

PR has been submitted here #36408

JonBendtsen marked this pull request as ready for review October 30, 2025 22:26

JonBendtsen changed the title ~~Check if API user has rights to see all thirdparties~~ FIX #35655 Check if API user has rights to see all thirdparties Oct 30, 2025

JonBendtsen force-pushed the patch-1 branch from b51bd60 to 9044921 Compare October 31, 2025 06:24

JonBendtsen mentioned this pull request Oct 31, 2025

QUAL: API contract: RestException update, check if thirdparty exists before creating/updating contract #36007

Merged

JonBendtsen force-pushed the patch-1 branch from 9044921 to b3ec669 Compare October 31, 2025 16:33

eldy reviewed Nov 9, 2025

View reviewed changes

eldy added the PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do) label Nov 9, 2025

JonBendtsen and others added 5 commits November 24, 2025 20:21

Check if API user has rights to see all thirdparties

f25ffc6

Check if API user has rights to see all thirdparties - though perhaps we should check if the user has rights to this particular thirdparty in this contract?

giving a slight better error message

9028fc5

default deny access, allow access if user has the right combination o…

ef4f5be

…f permissions and/or is the sales representative for the thirdparty

both socid exists and access to is checked in api_thirdparty GET :-)

bf8bca3

creating Thirdparties object takes no arguments

584d436

Jon Bendtsen added 2 commits November 24, 2025 20:21

PUT now checks both existing socid and any potentially updated socid …

6e6921b

…for access

This commit has errors, if you use PUT in the API with a new socid th…

48dfa19

…at the user does not have permission to, then it STILL updates the contract, and then it gets the contract after update and tells me I do not have access

JonBendtsen force-pushed the patch-1 branch from dabdfb0 to 48dfa19 Compare November 24, 2025 19:22

JonBendtsen mentioned this pull request Nov 24, 2025

Qual: DENY access to Thirdparty when module is enabled, but no permissions at all #36408

Open

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

JonBendtsen commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

JonBendtsen commented Oct 30, 2025

Uh oh!

JonBendtsen commented Oct 30, 2025

Uh oh!

hregis commented Oct 31, 2025

Uh oh!

JonBendtsen commented Oct 31, 2025

Uh oh!

hregis commented Oct 31, 2025

Uh oh!

hregis commented Oct 31, 2025

Uh oh!

hregis commented Oct 31, 2025

Uh oh!

JonBendtsen commented Oct 31, 2025

Uh oh!

hregis commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonBendtsen commented Oct 31, 2025

Uh oh!

JonBendtsen commented Oct 31, 2025

Uh oh!

JonBendtsen commented Oct 31, 2025

Uh oh!

hregis commented Oct 31, 2025

Uh oh!

JonBendtsen commented Oct 31, 2025

Uh oh!

JonBendtsen commented Oct 31, 2025

Uh oh!

JonBendtsen commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonBendtsen commented Nov 2, 2025

Uh oh!

JonBendtsen commented Nov 9, 2025

Uh oh!

eldy Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

JonBendtsen Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

JonBendtsen Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

JonBendtsen commented Nov 24, 2025

retesting POST with rebased v22 - NON existing socid

no permissions

read and write contacts

existing socid

Response body

Dolibarr.log

assign sales representative to 100

Uh oh!

JonBendtsen commented Nov 24, 2025

Conclusion

new post function

Uh oh!

JonBendtsen commented Nov 24, 2025

trying in branch 22.0, commit 7d403a2

Uh oh!

JonBendtsen commented Nov 24, 2025

Uh oh!

JonBendtsen commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

JonBendtsen commented Oct 30, 2025 •

edited

Loading

hregis commented Oct 31, 2025 •

edited

Loading

JonBendtsen commented Oct 31, 2025 •

edited

Loading

trying in branch 22.0, commit `7d403a2`