? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
31 May 2021

Pull Request for Issue # .

Summary of Changes

Our Form API has unset filter which is used to disallow changing certain data if the current user does not have permission to change. We use it to handle other fields such as ordering, published.... https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_contact/src/Model/ContactModel.php#L194-L196 , so created_by should not be treated in different way.

Testing Instructions

  1. Modify this line of code https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/form/field/user.php#L138 to:
<?php if (true) : ?>
  1. Apply this PR to your Joomla 4 installation.

The purpose is to have the the hidden field always rendered so that you can use browser inspect element tool to change hidden input variable to change created by data of a record.

  1. Login to administrator area of your site using a Manager account (by default, Manager account does not have permission to manage users, so he won't be allowed to change Created By data of article)

  2. Access to Content -> Articles, click on an article to edit. Navigate to Publishing tab, look at Created By field, you will see that it is readonly, mean you are not allowed to change Created By of the article
    created_by

  3. Inspect that Created By element, try to change the value of the hidden input (which stores ID of the user who created that article) to a different value
    inspect_created_by

  4. Save article. Then edit it again, check and make sure Created By is not changed (it still keeps the original user, not change to the new user which you changed)

Actual result BEFORE applying this Pull Request

Works, but has to override method validate in some model classes. Code is in-consistent with the way we handle other fields.

Expected result AFTER applying this Pull Request

Works, do not need to override validate method in model classes to just unset the field. Code is consistent with how we handle other fields data.

avatar joomdonation joomdonation - open - 31 May 2021
avatar joomdonation joomdonation - change - 31 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 31 May 2021
Category Administration com_banners com_contact com_content com_fields com_newsfeeds
avatar richard67
richard67 - comment - 31 May 2021

@zero-24 @SniperSister Do you know if it had security related reasons why we did it like we did and not like proposed by this PR here for the created_by field?

avatar wilsonge
wilsonge - comment - 31 May 2021

@wilsonge How do you think about this change?

Seems logical to me

avatar joomdonation
joomdonation - comment - 31 May 2021

Thanks @wilsonge. Could we have testing instructions by code review ? Or need real test?

avatar richard67
richard67 - comment - 31 May 2021

Could we have testing instructions by code review ? Or need real test?

Maybe a compromise: Real code review ? (joking)

avatar joomdonation
joomdonation - comment - 31 May 2021

For this kind of PR, it would be easier for someone who understand how our Form API works to review code than end-user testing. But if real testing is needed, I will try to write testing instructions :)

avatar richard67
richard67 - comment - 31 May 2021

I don't really understand the Form API.

avatar joomdonation
joomdonation - comment - 31 May 2021

I don't really understand the Form API

We could not understand/know everything :).

avatar wilsonge
wilsonge - comment - 31 May 2021

Umm because this is a recent security fix unfortunately I think this one needs testing @joomla/security FYI

avatar joomdonation joomdonation - change - 1 Jun 2021
The description was changed
avatar joomdonation joomdonation - edited - 1 Jun 2021
avatar joomdonation
joomdonation - comment - 1 Jun 2021

OK. I updated the testing instructions. Hope it is clear enough for users to test.

avatar sandramay0905 sandramay0905 - test_item - 1 Jun 2021 - Tested successfully
avatar sandramay0905
sandramay0905 - comment - 1 Jun 2021

I have tested this item successfully on 9a09cac

With and without PR the change of user-id in dev-tools is not saved.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34303.

avatar joomdonation
joomdonation - comment - 6 Jun 2021

@zero-24 @SniperSister Could you please review this PR?

avatar joomdonation joomdonation - change - 9 Jun 2021
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 9 Jun 2021
Category Administration com_banners com_contact com_content com_fields com_newsfeeds Administration com_banners com_categories com_contact com_content com_fields com_newsfeeds
avatar joomdonation
joomdonation - comment - 9 Jun 2021

@zero-24 Done with commit edf0021

avatar zero-24
zero-24 - comment - 9 Jun 2021

@zero-24 Done with commit edf0021

Thanks

avatar joomdonation
joomdonation - comment - 14 Jun 2021

@wilsonge Could this be merged by code review please? We now have one human test and approval from @zero-24 .

avatar wilsonge wilsonge - change - 15 Jun 2021
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-06-15 23:13:42
Closed_By wilsonge
avatar wilsonge wilsonge - close - 15 Jun 2021
avatar wilsonge wilsonge - merge - 15 Jun 2021
avatar wilsonge
wilsonge - comment - 15 Jun 2021

Yup I'm OK with that. Thankyou!

Add a Comment

Login with GitHub to post a comment