Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI fix: user with authorization to create in at least a category can access editor view in menuitems for other categories #17674

Merged
merged 24 commits into from Jan 22, 2020

Conversation

LivioCavallo
Copy link
Contributor

Summary of Changes

Perform an additional authorization check at the category level in com_content edit view file: view.html.php .
This blocks an unauthorized user from accessing the editor in some circumstancies when he does not have "Create" permissions for that category.

Testing Instructions

Create a new User Group: "Manage Category ONE" with 'Public' parent group.
In System / Global Configuration set Permission to allow "Manage Category ONE" group to Login into site.
Create new user 'User1' as member of this group.
Create a new article category "ONE" and set permissions so that "Manage Category ONE" group is allowed to "Create".
Create a new article category "TOP SECRET" and check that "Manage Category ONE" group has no permissions to "Create" in it.
Now create a menuitem, "Create Top Secret", to submit a new article; set Options so that default category "TOP SECRET" is default.

Login as "User1" into frontend and "Create Top Secret".

Expected result

User is not authorized, not having rights to "Create" in "Top Secret" category.

Actual result

User is allowed to enter the editor! He can even browse images folder.

Notes

In J!3.4.4 (and I suppose in some later versions) the user was allowed to save the article!
In J!3.7.5 (I suppose starting from some previous versions) when he clicks on "Save" the operation is blocked with error; this mitigates the problem.
Anyway I think that accessing the editor (though the most dangerous functionalities are disabled) is not a "good thing": the user should be blocked as soon as he tries to access a resource he is not authorized to use, not later, when he tries to close (save) that resource.

$authorised = $user->authorise('core.create', 'com_content') || count($user->getAuthorisedCategories('com_content', 'core.create'));
if ($this->state->params->get('enable_category') == 1)
{
$catid = $app->getMenu()->getActive()->params->get('catid');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you are using $app->getMenu()->getActive()->params ?
For consistency you should use the $this->state->params to get 'catid'
e.g. in the line above you are using it to get 'enable_category' which is also a menu item parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.
Right.

@ggppdk
Copy link
Contributor

ggppdk commented Aug 22, 2017

Also i noticed that parameter name is 'Default category'
but parameter description says:

If set to 'Yes', this page will only let you create articles in the category selected below.

so "Default category" does not sound like best choice
instead of "Default category" a proper parameter name would be

  • Limit category
  • Category Limitation
  • Specific category

submit_new_article

@LivioCavallo
Copy link
Contributor Author

Constrained / Bound category?

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Aug 23, 2017
@@ -30,7 +30,7 @@ COM_CONTENT_CONFIG_LIST_SETTINGS_DESC="These settings apply for List Layouts Opt
COM_CONTENT_CONFIGURATION="Articles: Options"
COM_CONTENT_CREATE_ARTICLE_CANCEL_REDIRECT_MENU_DESC="Select the page the user will be redirected to after Canceling article submission. The default is to redirect to the same article submission page (cleaning form)."
COM_CONTENT_CREATE_ARTICLE_CANCEL_REDIRECT_MENU_LABEL="Cancel Redirect"
COM_CONTENT_CREATE_ARTICLE_CATEGORY_LABEL="Default Category"
COM_CONTENT_CREATE_ARTICLE_CATEGORY_LABEL="Constrained Category"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort alpha order. Swap lines 33 and 34.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooops. Tks

@infograf768
Copy link
Member

Constrained is hard to translate.
"Specific category" reads better imho.

@ggppdk
Copy link
Contributor

ggppdk commented Aug 23, 2017

This is more an UX fix , that an security fix (e.g. the form display of custom fields of an non-accessible category are revealed),

i say UX because, imagine someone spending 10, 20 minutes to fill in the form and then clicking save, to discover that has no access to save the form, some real frustration

@LivioCavallo
Copy link
Contributor Author

So should I change name to: "UX fix: ..." ?
(Or programmer safety? 😉 )

@infograf768
Copy link
Member

Hmm, found a bug, unrelated to this PR.
One can save a Create Article menu item, settting "Default Category" to Yes and NOT filling the "Choose a Category" field.
In frontend, using such a setting forces to save the article created in the category with the smallest Id. catid param is therefore empty.

Therefore I suggest to use

$catid = $this->state->params->get('catid');
if (!empty($catid)
{
   $authorised = $user->authorise('core.create', 'com_content.category.' . $catid);
etc.

as I could not find a way to solve that bug.

@LivioCavallo
Copy link
Contributor Author

Oh! Thanks. Fixed.

There is a way to add client-side field validation to params form field in backend?
In this case it would be useful something like 'showon:', let's say 'requireon:'.

@LivioCavallo
Copy link
Contributor Author

It seems there were problems with drone and appveyor, but I don't find a way to restart them. Is it normal?
Or there is really any error?

@alikon
Copy link
Contributor

alikon commented Aug 24, 2017

i think we still need to manage properly the issue referenced by @infograf768, your fix just show Error save not permitted but maybe is better to open a different issue for that

@alikon
Copy link
Contributor

alikon commented Aug 24, 2017

I have tested this item ✅ successfully on 175a328

as per pr testing info


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

@LivioCavallo
Copy link
Contributor Author

You are right @alikon.
Now we have a new issue: #17708.

@ggppdk
Copy link
Contributor

ggppdk commented Aug 25, 2017

@LivioCavallo

I have made PRs against you patching both places, that 'enable_category' is used

components/com_content/views/form/view.html.php
and
components/com_content/models/form.php

now code will work properly even if user has set 'enable_category' to YES and did not select category

  • it will now behave as if 'enable_category' is NO

fixing is #17708 is now not needed

Require both parameters in view.html.php
@LivioCavallo
Copy link
Contributor Author

Why is AppVeyor failing?

@LivioCavallo
Copy link
Contributor Author

I tested it in Jommla! 3.7.4, 3.7.5 and 3.8 beta. This patch works as expected.
Did someone else test it?

@ghost ghost added the J3 Issue label Apr 5, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@infograf768
Copy link
Member

I have tested this item ✅ successfully on ad148a7


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

@infograf768 infograf768 changed the title Security fix: user with authorization to create in at least a category can access editor view in menuitems for other categories UI fix: user with authorization to create in at least a category can access editor view in menuitems for other categories Aug 8, 2019
@ghost
Copy link

ghost commented Aug 8, 2019

@alikon can you please retest?

@alikon
Copy link
Contributor

alikon commented Aug 8, 2019

I have tested this item ✅ successfully on ad148a7


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

@ghost
Copy link

ghost commented Aug 8, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 8, 2019
@richard67
Copy link
Member

@HLeithner What happens with this PR?

@HLeithner
Copy link
Member

Something take longer then other, thanks for this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants