Skip to content

Conversation

@infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 8, 2016

Pull Request for Issue # #10653 (comment) and #12338

Moving the access to the xtd-editors buttons to the plugins themselves.
Allow authors to use xtd-articles and pagebreak.

Main reason: displaying the list of articles to an author or to an Editor does not change the kind of articles displayed in the lists, therefore it is useless to prevent authors from using these lists.
Concerning issues with modals in frontend, this now makes sure all xtd-editors modals are only used by logged in users (Image was already taken care of).

To test:
Make sure you have a menu item to Submit Article in frontend, log as a Author, then submit article and use the buttons Articles, Pagebreak, Module.

@dgrammatiko
Copy link
Contributor

@infograf768 I still believe that the javascript should be on the layout file and not in the plugin. But that's another issue...
I will test later on

@infograf768
Copy link
Member Author

@DGT41
Concerning the js, we can, if desired, move it in later J! releases. For now, we just need to make sure someone is logged in and access to these xtd-editors is OK for authors.

@dgrammatiko
Copy link
Contributor

@infograf768 I know, it just bothers me that although someone could (potentially) change the mark up on these views, they have to rewrite the plugins for some lines of js...
But this problem you are solving here, is way more important!

@AlexRed
Copy link
Contributor

AlexRed commented Oct 9, 2016

Now is ok, but is different from the admin panel procedure.
After set the Pagebreak by Author in frontend now show a iframe in the modal windows. Inside the iframe there is the website homepage.
Also for the module button.
Uploading iframe-page-break.png…


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

@infograf768
Copy link
Member Author

infograf768 commented Oct 9, 2016

@AlexRed
Not here on 3.6.3-dev

screen shot 2016-10-09 at 10 39 54
screen shot 2016-10-09 at 10 39 06

screen shot 2016-10-09 at 10 42 00

@AlexRed
Copy link
Contributor

AlexRed commented Oct 9, 2016

Please see the video test: http://www.alexred.com/joomla/page-break01.gif

@infograf768
Copy link
Member Author

@AlexRed
Maybe you should use staging + patch. Here is my video
xtd

@AlexRed
Copy link
Contributor

AlexRed commented Oct 9, 2016

I use today Nightly + patch

@brianteeman
Copy link
Contributor

I get exactly the same behaviour as @AlexRed with current staging

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on ffdfe68


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

@dgrammatiko
Copy link
Contributor

@AlexRed can you try the following:

insert

$this->eName    = JFactory::getApplication()->input->get('e_name', '', 'cmd');

at line 11 on administrator/components/com_content/views/article/tmpl/pagebreak.php

and report if that fixes the problem?

@brianteeman
Copy link
Contributor

@DGT41 no it doesnt fix it and the issue is in more than page break - it can also be seen with other exitor-xtd such as modules

@infograf768
Copy link
Member Author

infograf768 commented Oct 9, 2016

I confirm the issue on a clean installation WHEN using using TinyMCE.

Please folks, test using Codemirror instead of tinyMCE.
@AlexRed @brianteeman

@DGT41
Looks like we do need a similar patch as the one you did for 3.7:
#12324

@brianteeman
Copy link
Contributor

brianteeman commented Oct 9, 2016 via email

@infograf768
Copy link
Member Author

The point is that we have an issue with TinyMCE, NOT with the xtd-editors buttons

@infograf768
Copy link
Member Author

I do confirm: clean install, NO patch, SuperAdmin logged in frontend, using the pagebreak or modules button: we get the same issue you folks noted when using TinyMCE

@brianteeman
Copy link
Contributor

Thanks for wasting our time. When you have fixed this PR please let us know. No point at all testing if it works with a different editor

@infograf768
Copy link
Member Author

Please test what I said:
use staging and log in as admin, try to use the xtd-editors buttons WITHOUT this patch and logged as superadmin in frontend.
This PR is just showing another release blocker issue that was happening BEFORE it.
It has been solved in 3.7 with #12324

You should rather thank me for finding this out a few days before release...

@AlexRed
Copy link
Contributor

AlexRed commented Oct 9, 2016

yes, I can confirm same problem without this patch as superadmin in frontend.


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

@infograf768
Copy link
Member Author

Folks, first patch with #12372 (Thanks @DGT41 )
then you can test again with TinyMCE.

@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on ffdfe68


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

@AlexRed
Copy link
Contributor

AlexRed commented Oct 9, 2016

I have tested this item ✅ successfully on ffdfe68

ok with #12372


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

@dgrammatiko
Copy link
Contributor

RTC

This should be merged together with #12372

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 9, 2016
return;
}
}
elseif ($input->get('view') === 'articles' && $input->get('layout') === 'modal')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need the modal check here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do not think so. we already did not need to check for image as it is done in the plugin.
now that we do check in the plugins for all xtds, these are imho useless.
if you insist on keeping some checks here, we would have to change this code and add all xtds here with the correct permissions, as is now done in the plugins themselves.

Copy link
Member Author

@infograf768 infograf768 Oct 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilsonge
After thought: let's test again to see if we avoid # 33. Will try right now. If needed, I will add all the xtds necessary checks .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, looked fine.

@rdeutz rdeutz merged commit 8acabc8 into joomla:staging Oct 11, 2016
@brianteeman
Copy link
Contributor

Remove the RTC tag

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 11, 2016
@ggppdk
Copy link
Contributor

ggppdk commented Oct 11, 2016

The ACL check on the component for displaying e.g. pagebreak is not proper (and for the XTD buttons)

https://github.com/joomla/joomla-cms/pull/12353/files#diff-58d7af9fe491285c0bec790cf59c6cc3R38

You can have create / edit / edit.own on a category and not have it at the component level,

thus this check is broken for all sites that do not give to their usergroups, these permissions to the component, but instead give them to specific categories (or even specific articles), i had describe this here:

#10653 (comment)

  • if it is decided not to drop the ACL checks for the pagebreak, then one needs to pass and check a specific "current" record (article) asset

@infograf768
Copy link
Member Author

@ggppdk
Indeed, this patch was not aimed at solving fully the ACLs. Just to let author access these xtds with existing ACL (adding create and editown.

As a RC-3 has now been released, I suggest you propose a PR to implement it fully.

@Hackwar
Copy link
Member

Hackwar commented Oct 15, 2016

Please revert this PR. This effectively removes any access checks against the articles view in the backend. Everybody can now access the articles view, even with a guest user. You added checks to prevent a link from being displayed, but not opening the actual resource.

Simply open ?option=com_content&view=articles&layout=modal&tmpl=component&{formtoken}=1 on your site and you get a list of all articles that have public viewlevel.

@Hackwar
Copy link
Member

Hackwar commented Oct 15, 2016

This not only gives you a complete list of the public articles (some of which maybe should not be found yet), it also gives you a complete list of your sites categories, viewlevels, authors and more. A nice truckload of information to further construct attacks against your site.

@infograf768
Copy link
Member Author

@hannes
using the link you provide above as a guest, registered or author, I just get un frontend:
The most recent request was denied because it contained an invalid security token. Please refresh the page and try again.

Now, an author can indeed use the xtd when creating an article, which was not possible before.

When using the same link when not logged in backend just displays the login page

@infograf768
Copy link
Member Author

Now, as I said, we can re-add the checks in
ROOT/components/com_content/content.php, but using instead of only core.edit, also core.create and core.editown

@Hackwar
Copy link
Member

Hackwar commented Oct 16, 2016

Sorry, github swallowed parts of my link. You need to replace {formtoken} with the token. You can get that token from for example the login or contact-form view. This is a serious security issue!

@infograf768
Copy link
Member Author

infograf768 commented Oct 16, 2016

@Hackwar
I can re-add the checks in content.php but also including create an edit.own
Would that be OK for you? I mean: is it OK for you to let authors use these xtds (will be the same isssue for the 3.7 branch and its new xtds)

@infograf768
Copy link
Member Author

Taking into account your comment here:
#12321 (comment)

I will now make a PR towards staging

@Hackwar
Copy link
Member

Hackwar commented Oct 16, 2016

Re-adding that check is a good first step. From my perspective, we need to further check this and consider what we display to editors, etc. Should someone with frontend editing capabilities see all categories, viewlevels and articles in the modal? I'd say that we have to restrict all of this to only those elements that the user would have access to.

@infograf768
Copy link
Member Author

As we can't right now do all necessary checks, this #12435 will let authors access to the xtds while preventing guest or registered users.
It just changes the limitation from editor to author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants