New ACL breaks canEdit list plugin

N

Ninpo

Member
I am running the latest version of Joomla and I just got the latest version of fabrik from the git repository.

The problem I am having is the order of events that happens with the new ACL and list plugins when saving a record.

I have a pretty complex form with an established workflow that worked until I updated to 3.5.2 and installed the latest git repository. My workflow is as follows;
  1. An employee creates a reports. Employees can only edit their own reports until they submit them for approval. On that report is a yes/no element that specifies that their shift is completed. Once they select "Yes" they are no longer allowed to edit their own reports.
  2. The report is sent up the chain of command to someone in the custom Joomla Supervisor ACL group for review and editing. Supervisors can only edit employee reports "after" the employee marks the report as "Submitted = yes".
  3. Once the supervisor reviews and edits the employee's report they set the "Report Finalized" yes/no element to "Yes"
  4. Once the report is submitted and finalized it is no longer editable by anyone.
Here is the problem I am facing. The employee submits the report and it is now editable by a supervisor but when the supervisor tries to finalized the report and they hit save they get the following message "Sorry, but you are not authorised to edit this record".

I am using the canEditRow list plugin to evaluate the workflow of this process to set if the report is editable and by who.

PHP: 
$user = JFactory::getUser();
$groups = 19;

$reportFinalized = '{reports_dar___report_finalized_raw}';
$shiftCompleted = '{reports_dar___shift_completed_raw}';

if(is_array($reportFinalized)) {
  $reportFinalized = $reportFinalized[0];
}

if(is_array($shiftCompleted)) {
  $shiftCompleted = $shiftCompleted[0];
}

// Temp check for new ACL bug
print($reportFinalized.' = '.$shiftCompleted);

if(in_array($groups, $user->getAuthorisedGroups())) {
  if ($user->id == '{reports_dar___employee_id_raw}') {
    return ($reportFinalized != 1) || ($shiftCompleted != 1);
  }
  else {
    return ($reportFinalized != 1) && ($shiftCompleted == 1);
  }
}
else {
  return $shiftCompleted != 1;
}

I put the code in a debugger and found the problem. It seems that the new ACL check runs the list plugins on form submission. I use the canEditRow plugin not to check ACL on submission but for the list view editable setting for each record. When the plugin runs the "Sorry, but you are not authorised to edit this record" error is thrown because the check on the "yes/no" element fields are now true since the supervisor set it to be so before form submission. I.e. reports_dar___shift_completed_raw = 1 and reports_dar___report_finalized_raw now equals 1 where as before form submission it was set to 0 which allowed the supervisor edit access to the report in my check "return ($reportFinalized != 1) && ($shiftCompleted == 1);".

I am not sure where to go from here since this workflow permeates about 30 different reports and we need this level of security on the reports.
 
Last edited:
I see 3 possible solutions to this issue.
  1. Do not fire list plugins on form submission. List plugins should be only for the list view and should not affect form submission. If a user is using the canEditRow plugin and they have set a condition in the Eval to check on the value of a field in the form to determine edit status, if that value changes upon editing the form then the condition will be always true and ACL validation will fail upon submission. This can cause a lot of confusion as it was not set prior thus allowing the user to edit the form in the first place.
  2. Give the user the option, at the form level, to run list plugins upon form submission.
  3. Separate form level ACL from list level ACL.
 
It's not a "bug", it was a specific fix for a security issue, which we made about six months ago. There's not much point only checking edit permission on load, if you don't also check them on submit, to catch spoofing. You wouldn't say that group ACL based edit access should only be checked on load and not on submission, that would be a gaping security hole. And throughout Fabrik, anything that affects what a user can view and edit through lists, applies to any access of the data.

You happen to have a corner case which is affected by the change, as it's very unusual for the user to be able to change the form data that affects whether they can edit the form or not. Typically that would be done by someone who has view level based permissions not dependent on the data.

So rather than being affected by a bug now, you are simply no longer able to take advantage of an old bug which has now been fixed.

If you'd like to take out a support sub for a month, I'll add an option to the canedit plugin to turn off checking on submit.

-- hugh
 
So, I had a meeting with my client about this issue and your proposal. They are adamant that this needs to be fixed because this locking functionality is essential to their workflow. They agree to your terms. So my question is, how do we proceed?
 
You know, I think you could probably work round this yourself. If you really don't want to check when submitting a form, doing ...

Code: 
if ($this->app->input->get('view', 'list') !== 'list') {
   return true;
}

... as this first thing in your code should work round it.

Although if that works for you, it'd be nice if you / your client would take out a support sub, help fund answers. :)

-- hugh
 
We actually have a temporary "hack" in place that I put in because it was an emergency. The only problem is that it is not supported and when I update Fabrik, I do not want to re-apply the hack. Also, no telling if what I did will be nullified in the future. We also do not want to branch off of the main code for this one situation, it's not really worth it.

This company deals with highly sensitive data and some of it is legally binding so, when a report has been finalized, it needs to be locked down from editing. They are willing to pay for a one month professional level subscription in exchange for a more permanent and supported fix. Let me know your thoughts.

My "hack" was placed in the form controller.

Thanks!
 
Last edited:
Did you try my suggestion?

That puts everything within your custom code, no reliance on Fabrik. If it's not a list view, just return true.

-- hugh
 
Not yet, I just got back in front of a computer. I will remove my temporary "hack" and try out your suggested solution in my Eval code.
 
I had a look at the code and how it would have to be done internally, and the only two ways I can do it are:

1) A sledgehammer approach, providing a per-form option to skip ACL checks on submission. I can't split out the built in view level / user based ACL checks from the list 'canedit' plugin checks, as they are handled within the same canEdit() method in the list model, called from the form controller. This would be horribly sub-optimal, as your client would then be open to spoofing, where the only ACL checks are done when opening the form, not on submission.

2) A per canedit list plugin approach to specify when the check runs. But unfortunately turns out that's a lot of work, more than I can do on a Pro sub, it'd have to be billed on an hourly basis.

And really, this is something you should be handling in your code, which is needing to base decisions on "what the element value was before it was changed for this submission".

So really the way I suggested isn't optimal either, as it would leave you open to spoofing again. Not as badly as method 1) above, in that normal ACL checks would still apply, but someone could spoof editing a form that your element flag(s) shouldn't let them.

Really the only way you can do this securely is to actually check the original data. Which you can do by loading it from the form model.

Something like ...

PHP: 
// check the task, to see what we're doing ...
if ($this->app->input->get('task', '') === 'form.process') {
   // we're processing a form submit, so need to test if it's an edit ...
   $formModel = $this->model->getFormModel();
   if (!$formModel->isNewRecord()) {
      // we're editing an existing record, so we need to check the original data ...
      $origData = $this->model->getFormModel()->getOrigData();
      $reportFinalized = $origData[0]->reports_dar___report_finalized_raw;
      $shiftCompleted = $origData[0]->reports_dar___shift_completed_raw;
   }
   else {
     // it's a new record, so no "original data", just use form data
      $reportFinalized = $formModel->formData['reports_dar___report_finalized_raw'];
      $shiftCompleted = $origData[0]->formData['reports_dar___shift_completed_raw'];
   }
}
else {
   // it's not a form submit, so just use placeholders
   $reportFinalized='{reports_dar___report_finalized_raw}';
   $shiftCompleted='{reports_dar___shift_completed_raw}';
}

... which would give you the current value in the list data for controlling whether they can see the "Edit" button and load the form, and use the original data on a form submission ... although you'd need to see if it's a new or edit, as new won't have original data.

Note that getOrigData() builds a query and runs it, to load the data for that row from the table, as that's the only place it exists.

-- hugh
 
And really, that's as far as I can go in Community. I've already spent over an hour on this thread.

-- hugh
 
Your suggested solution appears to work. I have applied that fix to all forms, in the system, that have the record locking functionality in place. I will talk to my client about financial compensation.

As a side note, I noticed that there is a new "link" plugin for the list view. This new plugin intrigues me as I can see a use case scenario for it in a new project I am working on, but there is no documentation on it or use examples.
 
Which suggestion? The first (simple) one, or the more secure one?

The link plugin is very simple, you just provide a link with optional {tablename___elementname} placeholders, and you get a button that opens the link. If it's a Fabrik form or list link, we can optionally open it in a popup.

-- hugh
 
We used the simple one for now since the system is not publicly available and if someone has access to spoof data, they have way more to be concerned with. I archived the more secure as a "just in case measure".

I tested out the link plugin and was interesting, but a bit overly simple for our needs at this time. Thank you for the info on the link plugin. I am still conceptualizing one piece of this new project and I thought it could help, but I think it will be a template modification.
 
cheesegrits said:
@Bauer would you like my response to your deleted post?

Hugh
Click to expand...
Sure.

I deleted after reading this thread (and that post brought out my grumpy 'snippy side')...
Can Edit Row.. strange behaviour bug in version 3.7 ?

I ended up using the solution you gave Ninpo as an example.
I don't want to argue about it because I can't decide, or fully grasp, what is right - or if this is the only solution.
All I know is that this must affect a lot of users who had been using the caneditrow list plugin. That 'security fix' broke 3 out of 5 instances in my project where I was using the plugin.
 
Yes, I have 3 people currently affected. But the thing is, they are all Community users. And much as I want to write code and provide support for free, I can't. So if, as these three folk are telling me, this is critical to their projects ... if the three would like to take out a Standard subscription for a couple of months each, that'll about pay for my time to add an option to not check the plugin on submit. It wouldn't pay for all the other bug fixes and improvements to the code they just got by updating, but it'll help with addressing the feature they need changing in the update.

-- hugh
 
I actually had to implement a more secure version of the proposed "complex" version of this that Hugh posted. It seems that when you use the canEditRow list plugin, it negates the ACL set on the List/Form in the Access Tab for the List. I found that I had a situation where I could directly open up a record by going to http://<site url>/index.php?option=com_fabrik&task=form.view&formid=<formID>&rowid=<recordID>. This would then open up the record to guest users even though the List Access settings were all set to some sort of Registered user group, nothing in the List ACL is set to public. My final code is posted below for reference;

PHP: 
// Block all non-registered users
if(empty(JFactory::getUser()->id)) {
  return false;
}

$user = JFactory::getUser();
$groups = 19;

$reportFinalized = '{reports_dar___report_finalized_raw}';
$shiftCompleted = '{reports_dar___shift_completed_raw}';
$myEmpID = '{reports_dar___employee_id_raw}';
$shiftDate ='{reports_dar___date_time_raw}';

if ($this->app->input->get('task', '') === 'form.process' || $this->app->input->get('task', '') === 'process') {
  $formModel = $this->model->getFormModel();
 
  if (!$formModel->isNewRecord()) {
    $origData = $this->model->getFormModel()->getOrigData();
   
    $reportFinalized = $origData[0]->reports_dar___report_finalized_raw;
    $shiftCompleted = $origData[0]->reports_dar___shift_completed_raw;
  }
}

$reportFinalized = is_array($reportFinalized) ? $reportFinalized[0] : $reportFinalized;
$shiftCompleted = is_array($shiftCompleted) ? $shiftCompleted[0] : $shiftCompleted;
$myEmpID = is_array($myEmpID) ? $myEmpID[0] : $myEmpID;
$shiftDate = is_array($shiftDate) ? $shiftDate[0] : $shiftDate;

if(in_array($groups, $user->getAuthorisedGroups())) {
  if ($user->id == $myEmpID) {
    return ($reportFinalized != 1) || ($shiftCompleted != 1);
  }
  else {
    $dateNow = date('Y-m-d');
    $dateCalc = date('Y-m-d', strtotime($shiftDate. ' + 5 days'));
   
    if($dateCalc <= $dateNow) {
      // Allow supervisors to edit reports over 5 days old if not finalized
      return ($reportFinalized != 1);
    } else {
      return ($reportFinalized != 1) && ($shiftCompleted == 1);
    }
  }
}
else {
  return $shiftCompleted != 1;
}
 
Yes, CanEditRow is overriding the list "edit record" (and "view record") access.
But it's only applied to the access level you set in the canEditRow plugin.

If you have list edit/view access for registered users only and you are overriding these settings via canEditRow you must make sure that canEditRow has also Access=registered (so the list access settings are still valid for guests).
 
You must log in or register to reply here.

Useful links

  • Create a Fabrik Account (Downloads)
  • Create a Forum Account
  • Fabrik Wiki
  • Fabrik 4 Changelog /Install /Upgrade
    • We are in need of some funding.
    More details.

    Thank you.

    Members online

    Total: 114 (members: 4, guests: 110)
    Back
    Top