[com_fields] Reload task belongs to the component controller #16665

laoneo · 2017-06-13T11:33:18Z

Pull Request for Issue #15995.

Summary of Changes

When a category changes for an article the page got reloaded to determine the custom fields. Actually this action got delegated to the com_fields controller. On that point the data is unvalidated stored in the session. This can lead to time shifts when the Joomla timezone is not UTC, more information can be found in issue #15995.

This pr reloads the page trough the component controller and not the com_fields one.

Testing Instructions

Set the global Joomla timezone to a different one than UTC, for example Berlin
Create two article categories
Create a calendar custom field
Set show time to yes in the field
Save the field
Create an article
Change the category of the article

Expected result

The time in the custom field remains.

Actual result

The time in the custom field shifts the same hours as the Joomla timezone has.

ghost · 2017-06-13T13:08:12Z

I have tested this item 🔴 unsuccessfully on 64d9bb2

Set Time-Zone to "Berlin",
applied PR,
change Category > +2h in Calendar-Field
changed back to 1st Category > additional 2h (4 Hours)

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16665.}

laoneo · 2017-06-13T13:38:37Z

Do the publishing dates also shift?

ghost · 2017-06-13T14:45:48Z

I have tested this item ✅ successfully on 64d9bb2

Test again, all works fine now.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16665.}

wilsonge · 2017-06-13T16:09:11Z

We need to be careful of ACL here - this is going to expose public data if accessed directly from a URL

laoneo · 2017-06-13T18:40:13Z

Really, it uses the same code and checks as the save function. Which line do you think can be dangerous?

Bakual · 2017-06-13T18:53:05Z

Really, it uses the same code and checks as the save function. Which line do you think can be dangerous?

Maybe I miss something but I don't see any ACL checks at all here.
The save method uses $this->allowSave($data, $key) to check if saving is allowed which I don't see in your reload method.
We may also have to use checkEditId($context, $id) to verify that the item was correctly opened before.

laoneo · 2017-06-14T04:28:42Z

Why is the allowSave check needed, it doesn't save, it just reloads the page to an url like /cms/administrator/index.php?option=com_content&view=article&layout=edit&id=1. The checkedit id is done in the controller itself. I mean at the end it performs the same action as when you want to edit the article. If you paste that url directly into the browser and you didn't go trough the edit task, then it kicks you back to the list with a warning to not get directly to the edit page. I'v just tested it and it worked as expected.
If I'm now not completely wrong then I don't understand your points here. What else needs to be checked?

Bakual · 2017-06-14T06:46:04Z

Why is the allowSave check needed, it doesn't save, it just reloads the page to an url like /cms/administrator/index.php?option=com_content&view=article&layout=edit&id=1.

It does save the data into the session. It at least feels wrong to have a controller task without any ACL check. Which means anyone could run it. It may not be an issue by itself, but it may bite us back and there is no reason this task should be available unauthorised. Using allowSave or allowEdit (if that fits better due to not saving into db) is a save check we can and should do imho. I don't see a reason to not do it.

As for checkEditId, you're right. I just remembered we check it but didn't remember where exactly 😄

wilsonge · 2017-06-14T13:05:48Z

Does this not allow accessing data from any JControllerForm instance (including in the frontend (e.g. com_contact)) without checking the published/acl levels for the item though?

laoneo · 2017-06-14T13:08:25Z

I don't se how, it only stores the jform data into the session.

mbabker · 2017-06-14T13:12:36Z

It is still a POST request receiving user data. It should therefore be properly validated and access controlled. Especially if that data stored into the session can be used in a follow on request (which HAS been used in security issues we have fixed in the last 2 years).

wilsonge · 2017-06-14T13:14:51Z

The session is data that the user controls. That means the user has the data which they shouldn't have been allowed to access (for ACL reasons) - and even if it isn't displayed on the page. It's trivial to get the session data if you wanted it.

laoneo · 2017-06-14T13:15:37Z

Does that help d10e8fc?

Bakual · 2017-06-14T14:05:37Z

Imho, the ACL check is now fine.

Looking at the redirects, why don't you just use $this->redirect()? Also $app->close() is useless even when you use $app->redirect() directly, since that method already closes the app. Or do I miss something?

laoneo · 2017-06-15T05:52:39Z

Just to be sure. Probably not needed.

Bakual · 2017-06-15T06:51:17Z

If there is no specific reason to use $app->redirect() directly, can you just use $this->setRedirect($url, $msg, $type) and $this->redirect() then? So we are consistent with how we process redirects in other places in the controllers.

laoneo · 2017-06-15T06:59:09Z

True, done.

wilsonge · 2017-06-15T11:35:10Z

RE ACL: I'm honestly unsure given what this function does whether it should be edit or read acl permissions we should be using. But this looks significantly better now :)

laoneo · 2017-06-15T16:37:35Z

It should check edit, as it belongs to the edit action.

…d-form # Conflicts: # administrator/components/com_fields/helpers/fields.php

infograf768 · 2017-07-07T08:46:14Z

@laoneo
I can't get the calendar field anymore
( ! ) Warning: Invalid argument supplied for foreach() in /Applications/MAMP/htdocs/testnew/trunkgitnew/libraries/cms/helper/content.php on line 74

infograf768 · 2017-07-07T08:49:37Z

infograf768 · 2017-07-07T09:08:33Z

administrator/components/com_admin/script.php

 			'/administrator/components/com_messages/layouts/toolbar',
 			'/administrator/components/com_messages/layouts',
+			// Joomla! __DEPLOY_VERSION__
+			'/components/com_fields/controllers',


this targetting a folder. Should be the file.
'/components/com_fields/controllers/field.php',

This folder has only one file, so better to remove it completely.

better do both. see https://github.com/Digital-Peak/joomla-cms/blob/feb148b97dbab493f21a02a289ddf8d06df03692/administrator/components/com_admin/script.php#L1599

https://github.com/Digital-Peak/joomla-cms/blob/feb148b97dbab493f21a02a289ddf8d06df03692/administrator/components/com_admin/script.php#L1599
better do both

As comment says, deleting only the folder may not work when ftp is used.
It is safer to at least delete the file.

If a new article is created and the title is not set and the category has changed, then the validated data is false, will not be set and the admin is loosing all its entered data. So we need to filter here only.

laoneo · 2017-07-07T11:32:02Z

Good catch. Fixed it in the last commits.

infograf768 · 2017-07-08T06:10:51Z

Works now. Remains the necessary changes in script.php

laoneo · 2017-07-08T07:40:35Z

Ok, done.

infograf768 · 2017-07-09T05:33:18Z

I have tested this item ✅ successfully on fb925e7

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16665.}

ghost · 2017-07-09T06:07:00Z

I have tested this item ✅ successfully on fb925e7

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16665.}

ghost · 2017-07-09T06:07:21Z

RTC after two successful tests.

laoneo · 2017-07-09T12:45:25Z

Thanks guys!

Reload task belongs to the component controller

64d9bb2

joomla-cms-bot added the PR-staging label Jun 13, 2017

laoneo mentioned this pull request Jun 13, 2017

Timezone offset added to Article Publishing Dates when changing categories if fields are applying to categories. #15995

Closed

joomla-cms-bot removed the PR-staging label Jun 13, 2017

Add an extra check if the record can be edited.

d10e8fc

joomla-cms-bot added the PR-staging label Jun 14, 2017

Use the controller redirect functions

aecca87

laoneo mentioned this pull request Jun 16, 2017

resolve category field change event issue for calendar fields in Persian language #16721

Closed

farzadtaheri mentioned this pull request Jun 27, 2017

Calendar Fixes #16794

Merged

Merge remote-tracking branch 'remotes/upstream/staging' into cf/reloa…

0f2a7b9

…d-form # Conflicts: # administrator/components/com_fields/helpers/fields.php

infograf768 reviewed Jul 7, 2017

View reviewed changes

laoneo added 2 commits July 7, 2017 13:27

Move type field to correct reload function

edb3801

Only use filter function as we are not validating the data on reload

feb148b

If a new article is created and the title is not set and the category has changed, then the validated data is false, will not be set and the admin is loosing all its entered data. So we need to filter here only.

Add the file to the remove script

fb925e7

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 9, 2017

infograf768 added this to the Joomla 3.7.4 milestone Jul 9, 2017

infograf768 assigned rdeutz Jul 9, 2017

rdeutz merged commit 0774500 into joomla:staging Jul 9, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 9, 2017

laoneo deleted the cf/reload-form branch July 9, 2017 16:17

Uh oh!

[com_fields] Reload task belongs to the component controller #16665

[com_fields] Reload task belongs to the component controller #16665

Uh oh!

Conversation

laoneo commented Jun 13, 2017 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Expected result

Actual result

Uh oh!

ghost commented Jun 13, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laoneo commented Jun 13, 2017

Uh oh!

ghost commented Jun 13, 2017

Uh oh!

wilsonge commented Jun 13, 2017

Uh oh!

laoneo commented Jun 13, 2017

Uh oh!

Bakual commented Jun 13, 2017

Uh oh!

laoneo commented Jun 14, 2017

Uh oh!

Bakual commented Jun 14, 2017

Uh oh!

wilsonge commented Jun 14, 2017

Uh oh!

laoneo commented Jun 14, 2017

Uh oh!

mbabker commented Jun 14, 2017

Uh oh!

wilsonge commented Jun 14, 2017

Uh oh!

laoneo commented Jun 14, 2017

Uh oh!

Bakual commented Jun 14, 2017

Uh oh!

laoneo commented Jun 15, 2017

Uh oh!

Bakual commented Jun 15, 2017

Uh oh!

laoneo commented Jun 15, 2017

Uh oh!

wilsonge commented Jun 15, 2017

Uh oh!

laoneo commented Jun 15, 2017

Uh oh!

infograf768 commented Jul 7, 2017

Uh oh!

infograf768 commented Jul 7, 2017

Uh oh!

infograf768 Jul 7, 2017

Choose a reason for hiding this comment

Uh oh!

laoneo Jul 7, 2017

Choose a reason for hiding this comment

Uh oh!

infograf768 Jul 7, 2017

Choose a reason for hiding this comment

Uh oh!

infograf768 Jul 7, 2017

Choose a reason for hiding this comment

Uh oh!

laoneo Jul 7, 2017

Choose a reason for hiding this comment

Uh oh!

infograf768 Jul 8, 2017

Choose a reason for hiding this comment

Uh oh!

laoneo commented Jul 7, 2017

Uh oh!

infograf768 commented Jul 8, 2017

Uh oh!

laoneo commented Jul 8, 2017

Uh oh!

infograf768 commented Jul 9, 2017

laoneo commented Jun 13, 2017 •

edited by joomla-cms-bot

Loading

ghost commented Jun 13, 2017 •

edited by ghost

Loading