[#22942] - Fix form saving failure when a field is not editable by current user ("Edit field value" ACL)
- Closed
- 7 Mar 2019
- Medium
- Build: staging
- # 22942
- Diff
- ggppdk:patch-69
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 2 Failed. Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #22038 , #22519 , #22923
Summary of Changes
When a field is not editable by current user
because 'editfieldvalue' ACL rule is denied for 1 of user's usergroups,
then mark it as readonly instead of current behavior that marks it as disabled
Testing Instructions
- Edit a field and set ACL ("Edit field value") for a usergroup e.g. "Manager" to denied,
- and login as with a "manager" user
Edit an article that shows the field,
- Try to save the form, form saves
- "Inspect element" with browser right click and remove readonly="" . modify field value , article form can be saved and field value is unchanged
- "Inspect element" with browser right click and delete the field, article form can be saved and field value is unchanged
- In field configuration set to hide the field when readonly, article form can be saved, edit article as super admin and you see that field value is not lost
- Also mark field as required, article form again can be saved
Expected result
Field is shown as readonly and it is possible to save the article form without error
Actual result
Field is shown as disabled saving the article form gives "invalid field" error
Documentation Changes Required
None
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_fields |
| Title |
|
||||||
Please also read this comment
#22038 (comment)
Any com_fields that are disabled will appear to be posted because of normalization code
another fix would be add
$group !== 'com_fields'
in the code that throws "invalid field" when a disabled field is posted ...
or maybe modify the check to check the real HTTP request data ?
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Form/Form.php#L2074
No we should not do com_fields specific checks in the Form API readonly is the correct fix as said at other places.
Sure making the field readonly is better than a com_fields specifc hack in the Form API
Further more,
logically, a non-editable is a "readonly" field and not a "disabled" field
and a configuration option exists inside field configuration
"Display When Read-Only"
that speaks of showing / hiding a readonly field ...
I have tested this item
I following the testing instructions and the point number 2 ("Inspect element" with brow....) is not success.
When I try to save the article I get the same error (Warning Invalid field: test).
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22942.
I have tested this item
I can´t checked the issue because the second point testing instructions not be success.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22942.
Step 2 was included at the description to prove no security concerns comes with this PR ...
someone else making a PR may have not added step 2 there at all ...
about the error
-
removing the readonly attribute, cannot cause any error because the posted data do not include any info if a field was posted while being "readonly" or not being readonly ...
-
If during form tampering of the field, you change the field value to have be a value that will make validation to fail (this can depend on the custom field that you used , maybe a date field or a 3rd party field)
example of the above effect a readonly date field that was posted with non-date value ...
- then this is error 'Invalid field' that you got is expected / desired / correct to occur
please try doing the form tampering of step 2 with a value that will not cause validation error by itself (or even a custom 3rd party filtering method)
@kukubayo
thanks for creating a github account for the purpose of testing this PR
I have tested this item
I have tested this fix and the result is that a user can see readonly custom fields.. (show profiel, edit profile) and after edit some fields i succesfully save the profile.
But: the readonly fields are not grayd out.. so visualy it looks like that the user can change the field.. (he can not).. but it would be nice to have a visual aspect (or remark)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22942.
But: the readonly fields are not grayd out.. so visualy it looks like that the user can change the field.. (he can not).. but it would be nice to have a visual aspect (or remark)
hhmm the grayed out effect is styling of the template
with which template did you test ?
But: the readonly fields are not grayd out.. so visualy it looks like that the user can change the field.. (he can not).. but it would be nice to have a visual aspect (or remark)
hhmm the grayed out effect is styling of the template
with which template did you test ?
i have tested in with a custom template..
- before apply the patch the readonly fields are gray, unable to change value and (saving error)
- after patch apply fields are normal, unable to change (no saving error).
so the patch solved the saving error issue on readonly fields but visualy its not easy to see that its readonly..
screenshot after patch
screenshot before patch
- before apply the patch the readonly fields are gray, unable to change value and (saving error)
- after patch apply fields are normal, unable to change (no saving error).
It is an issue with your template CSS
Explanation
Before patch the "non-editable" fields are "disabled" (Tag attribute)
and your template CSS is styling theses (the radio inputs in your case) appropriately
After patch the "non-editable" fields are "readonly" (Tag attribute)
and your template CSS is not styling theses (the radio inputs in your case) appropriately
Solution is to add CSS to your custom template to give feedback to the user that the field is readonly,
or report the issue to the template author,
or maybe report this to the author of the extension if author is using custom CSS to style the radios
any update / news on this issue..
i need this function badly on one of my website's.. can i use the patch for production.. when wil this issue be complete and implemented in the core joomla version?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22942.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-03-07 17:23:48 |
| Closed_By | ⇒ | HLeithner |
Looks like a better patch lets cc @laoneo so he can take a look here too.