Added final class access control when opening object detail.

[Nould]

I use custom "MakeSelectFilter" method, which allows to see FunctionalCI (for selection in list), but denies access to ApplicationSolution for users from another organization. When user has acces to class (e.g. FunctionalCI) and tries to open its derived class (e.g. ApplicationSolution) detail, to which he has no access, app crashed. This can be solution of derived class access control.

[piRGoif]

Hello, Not sure of what problem you are trying to solve ? Can you attach a use case, maybe your XML customization as a gist, and a step by step guide to reproduce ?

[piRGoif]

Oh, also I see you based on support/2.7 : this is not a good idea (as said in our CONTRIBUTING.md), I highly doubt we will integrate your work in a support branch (at that time, this branch holds next 2.7.5 devs). Be prepared to rebase on develop ! The decision will be made by our products team, but I need more info as asked above before presenting them this PR - after this presentation I'll be able to tell you if you need to rebase or not.

[Nould]

Hello, I have no XML extension. I only modified MakeSelectFilter method in UserRightsProfile class. It can be done by including custom user rights addon in configuration. My modification looks like:

public function MakeSelectFilter($sClass, $aAllowedOrgs, $aSettings = array(), $sAttCode = null)
	{
		if ($sAttCode == null)
		{
			$sAttCode = $this->GetOwnerOrganizationAttCode($sClass);
		}
		if (empty($sAttCode))
		{
			return $oFilter  = new DBObjectSearch($sClass);
		}

		$oExpression = new FieldExpression($sAttCode, $sClass);
		$oFilter  = new DBObjectSearch($sClass);
		$oListExpr = ListExpression::FromScalars($aAllowedOrgs);
		
		$oCondition = new BinaryExpression($oExpression, 'IN', $oListExpr);
		$oFilter->AddConditionExpression($oCondition);

                //START OF CUSTOM CODE
		if($sClass == 'Team' || $sClass == 'Person' || $sClass == 'Attachment' || $sClass == 'Location' || $sClass == 'Organization') {	
		        //No restriction, all users can see these classes even from another organization
			$oFilter = new DBObjectSearch($sClass);
		}

		if ($sClass == 'FunctionalCI')
		{
                        //Support agent can see FunctionalCI with final class ApplicationSolution even from another organization
			if (in_array("Support Agent", UserRights::ListProfiles(UserRights::GetUserObject())))
			{
				$oExpr = new FieldExpression('finalclass', $sClass);
				$oListEx = ListExpression::FromScalars(array('ApplicationSolution'));
				$oCond = new BinaryExpression($oExpr, 'IN', $oListEx);
				$oFilter->MergeConditionExpression($oCond);        
			} 
		}
                //END OF CUSTOM CODE
                ...
                //rest of official code...
        }

Now user with allowed organization (URP_UserOrg) Organization A can add FunctionalCI (with final class ApplicationSolution) from Organization B (he can add it to the list of FunctionalCIs e.g. in UserRequest ). But when user tries to open added FunctionalCI, the app tries to redirect user to its finalclass object (ApplicationSolution) detail and then the app crashes. I would like to get standard info message, that user has no permission to access this object. Is it understandable?

[Nould]

Oh, also I see you based on support/2.7 : this is not a good idea (as said in our CONTRIBUTING.md), I highly doubt we will integrate your work in a support branch (at that time, this branch holds next 2.7.5 devs). Be prepared to rebase on develop ! The decision will be made by our products team, but I need more info as asked above before presenting them this PR - after this presentation I'll be able to tell you if you need to rebase or not.

I'm sorry, I'll rebase it later, if necessary.

[Hipska]

As far as I can see, the only reason why something is failing is because the user can see Organization he normally has no access to. So, can't this class be removed from your exclusion custom code?

[Nould]

I just tried it, but it's not the solution, the problem is still there.

[Hipska]

So, if a user doesn't have permissions/access to an Org anymore, how can it then select that Org when creating a new AppSol?

[Nould]

User doesn't create new ApplicationSolution. Someone from Organization B creates an ApplicationSolution. User from Organization A can make link to ApplicationSolution (from Organization B) inside FuntionalCIs Tab on e.g. UserRequest.

[Hipska]

Okay, and what happens then in that case?

[Nould]

User (from Organization A) makes a link to ApplicationSolution (from Organization B). Then he goes to detail of added ApplicationSolution from UserRequests FunctionalCIs Tab -> application tries to redirect user to FunctionalCI (with finalclass ApplicationSolution) detail and crashes (there is no information about missing permission).

[Hipska]

Okay, now I get the use case, but it seems there is already permission handling in place: https://github.com/Combodo/iTop/blob/94fdc79be58b298ff5d1215c3b6103c0b1f19fed/pages/UI.php#L425-L430

Or for 2.7 branch: https://github.com/Combodo/iTop/blob/9af4846372f4b3160f45bd081b170f31615f878b/pages/UI.php#L146-L151

[Nould]

I don't think It is the same handling we are talking about. In my opinion, this is handling of permission to view object of some class (it depends on user role). It doesn't handle the permission based on organization (which depends on user's allowed organization). In the use case I described, I get the following error when I try to show object detail:

Uncaught Error: Call to a member function GetKey() on null in C:\xampp\htdocs\itop-official\application\utils.inc.php:1255\nStack trace:\n#0 C:\xampp\htdocs\itop-official\application\displayblock.class.inc.php(1876): utils::GetPopupMenuItems(Object(iTopWebPage), 3, NULL, Array)\n#1 C:\xampp\htdocs\itop-official\application\displayblock.class.inc.php(243): MenuBlock->GetRenderContent(Object(iTopWebPage), Array, -1)\n#2 C:\xampp\htdocs\itop-official\application\displayblock.class.inc.php(207): DisplayBlock->GetDisplay(Object(iTopWebPage), -1, Array)\n#3 C:\xampp\htdocs\itop-official\application\cmdbabstract.class.inc.php(263): DisplayBlock->Display(Object(iTopWebPage), -1)\n#4 C:\xampp\htdocs\itop-official\application\cmdbabstract.class.inc.php(1033): cmdbAbstractObject->DisplayBareHeader(Object(iTopWebPage), false)\n#5 C:\xampp\htdocs\itop-official\pages\UI.php(153): cmdbAbstractObject->DisplayDetails(Object(iTopWebPage))\n#6 C:\xampp\htdocs\itop-official\pages\UI.php(455): DisplayDetails(Object(iTopWebPage), 'Functio in C:\xampp\htdocs\itop-official\application\utils.inc.php on line 1255, referer: http://localhost/itop-official/pages/UI.php?operation=details&class=UserRequest&id=1&c[menu]=UserRequest%3AOpenRequests

[Hipska]

Indeed, because you did some modifications. If you would also do the needed modifications in method UserRights::IsActionAllowed, a normal message would be displayed instead of this crash and stack-trace.

[Nould]

Yes, it is probably possible to modify this method and user can get some message instead of crash. But this is not the same message as missing permissions to object due to allowed organizations. Eg. If user want to see detail of FunctionalCI from another organization, he gets message "Sorry, this object does not exist (or you are not allowed to view it)." in current window. When I modify method UserRights::IsActionAllowed to deny access, user gets new page "iTop - Fatal error" with "access denied" message, which is not so "user friendly".

[piRGoif]

Sorry, I was very busy last weeks.

Thanks for all the explanations, I understand better what you are trying to do.

Obviously modifying iTop core files (like \UserRightsAddOnAPI::MakeSelectFilter) isn't a good idea AT ALL. And also, you're trying to workaround silos that are implemented deep in the OQL engine ! There are ways to modify iTop silos but for now we don't have public documentation on that topic. Those types of modifications though were done for some Combodo customers, so if you're interested on support in this area I'm sorry but you'll need a support contract.

Anyway, many thanks for this PR, we appreciate a lot people trying to improve robustness and adding logs / clear error messages ! I would have done it differently though, but I'll do a review to explain :)

[piRGoif]

Hello, 2 year have passed, all of our apologies :/ But for this PR to be processed, we need a way to reproduce easily, and now it seems to me very complicated to do. I'm closing this PR, if you have any clear explanation on how to reproduce the crash feel free to add a comment with a step by step procedure.