Extra ACL checks (yay) #11244

wilsonge · 2016-07-22T09:53:56Z

Pull Request for Issue #11162 .

Summary of Changes

Adds bonus ACL checks for the categories on the fly

Testing Instructions

Before Patch:

Create a new user with "manager" rights
Remove the "Create" permission for managers in each component (com_content, com_contact, com_newsfeed and com_banners)
=> User can still create "categories on the fly" when editing an item

After Patch:

Create a new user with "manager" rights
Remove the "Create" permission for managers in com_content
=> User cannot create "categories on the fly" when editing an item

izharaazmi · 2016-07-22T09:58:18Z

administrator/components/com_newsfeeds/models/newsfeed.php

@@ -503,9 +503,17 @@ protected function getReorderConditions($table)
 	 */
 	protected function preprocessForm(JForm $form, $data, $group = 'content')
 	{
+		// Check if article is associated
+		$canCreateCategories = JFactory::getUser()->authorise('core.create', 'com_newsfeeds');


Should/Does core.create on item also grants the permission to create categories for them?

@izharaazmi go to Article -> Categories and click Options, what you see is the com_content permissions.
Same goes for all of the components with categories.

The question is if the user is allowed to create category items in the com_newsfeeds component.
So, correct. core.create - com_newsfeeds.

infograf768 · 2016-07-22T10:44:59Z

Why:
// Check if article is associated everywhere?

andrepereiradasilva · 2016-07-22T10:46:20Z

administrator/components/com_banners/models/banner.php

+	 *
+	 * @return  void
+	 *
+	 * @since    3.0


wilsonge · 2016-07-22T10:51:27Z

administrator/components/com_contact/models/contact.php

 	 */
 	protected function preprocessForm(JForm $form, $data, $group = 'content')
 	{
+		// Determine correct permissions to check.
+		if ($this->getState('contact.id'))


This check is in all the other components - but missing from contacts so added this as well for consistency

wilsonge · 2016-07-22T10:51:39Z

Because I fail at copy/pasting code JM :) Fixed!

izharaazmi · 2016-07-22T11:04:01Z

I though core.create - com_xyz tells us whether the user is allowed to create item in com_xyz. But here I see the same permission is used to check whether if he is allowed to create categories for com_xyz items.

So how do we handle the practical case when the site admin has predefined set of categories and wants the authors to only create articles in those categories and NOT allow creating new categories. Am I missing on something?

wilsonge · 2016-07-22T11:08:46Z

Is that case handled at the moment in category creation?

izharaazmi · 2016-07-22T11:13:36Z

I'm not sure if this is possible in Joomla yet. I never did the site management so never had to go through that process. I just assumed it to be there because I believe this is so basic need, isn't it?

andrepereiradasilva · 2016-07-22T11:16:24Z

I'm not sure if this is possible in Joomla yet. I never did the site management so never had to go through that process. I just assumed it to be there because I believe this is so basic need, isn't it?

since, for what i know, you're the first to ask doesn't seem a basic need 😄 .

izharaazmi · 2016-07-22T11:23:47Z

Just to clarify my doubts.
Is it okay to allow authors to create an article category such as 'politics', 'cinema' etc. on my website dealing with 'medical science'? As I can see the authors do have 'core.create' on articles.
Am I really the first to ask this?

mbabker · 2016-07-22T12:12:15Z

If they have ACL permission to create items they can create items. It's on the site admin to review that stuff, there isn't a way to programmatically state "my site is medical science so raise a red flag if someone starts creating art related content on the site".

So how do we handle the practical case when the site admin has predefined set of categories and wants the authors to only create articles in those categories and NOT allow creating new categories.

Leave core.create at the default inherited state, create user groups with an ACL tree that doesn't explicitly allow (or deny) creating items at the component level, but specify they are allowed to create items within categories. Unfortunately for your case IIRC that means they can create categories within that category as well as items, which is again the programatic correct behavior. You'd need to extend the ACL system if you wanted to prevent the creation of specific item types even if the ACL allows the user to create items under a certain level.

izharaazmi · 2016-07-22T12:35:19Z

Thanks @mbabker. Now I can see what I was seeking for is not yet possible in Joomla.

... that means they can create categories within that category as well as items, which is again the programatic correct behaviour.

However, it is still desirable to avoid allowing create everything in a component (or item ACL) based on a single rule core.create and delete everything in a component (or item ACL) with a single rule core.delete.

Similarly, in com_users we (super administrator) may not want to allow an administrator/manager to create user groups or access level and still allow to create new users. Currently what I understand is we can't.

Wouldn't it be nice as something in the line of:

core.create - com_content to allow content creation in articles component,
core.category.create - com_content for category creation in articles component.
and likewise in other relevant places. Hence giving more precise control on access over specific item type.

mbabker · 2016-07-22T12:48:16Z

Wouldn't it be nice as something in the line of:

No. core.create is a generalized permission level authorizing users to create items at a given level. It does not know about or care about the different item types that can fall under that item, all it cares about is whether you've given the user permission to create stuff. As a generalized solution, this is all Joomla really needs to care about.

I personally think bloating Joomla with permissions (i.e. content.create.article or user.edit.group, remember the core. "namespace" is reserved for what should be global actions) which may offer the most granular ACL control to a site is going to create too complicated of an interface for anyone to sanely manage their site's ACL. Now assuming the ACL APIs are written correctly, programmatically you should be able to extend the base definitions and add more granular controls if you choose to do so.

At the end of the day, the current ACL is just fine, even with its known limitations, as a working starting point and generalized solution (because that's ultimately what the CMS application is, a compromise of a bunch of high level generalized solutions which aren't really optimized for anything) and the API should be strong enough to let folks extend it with additional custom rulesets if they really need a more fine tuned control set.

izharaazmi · 2016-07-22T12:59:14Z

Hmm... I understand the situation here. Agree to you.

Now as an expert advice would you please tell me whether I'm doing it right if I use following rules for my own extensions, or there are known better alternatives?

core.create = create everything.
component.create.abc = create item type abc
component.create.xyz = create item type xyz

and similar for delete, edit, state etc.

mbabker · 2016-07-22T13:02:45Z

That'd be the way to go about it, you'd just have to deal with the different possibilities that come out of that config set (i.e. if core.create is set to explicit deny following core's structure the component options should also be deny, or if you've allowed core.create but explicit denied one of the component options, things like that).

izharaazmi · 2016-07-22T13:08:49Z

Thanks again @mbabker. I appreciate the time and effort taken to explain the whole thing.

wojsmol · 2016-07-22T16:09:49Z

administrator/components/com_banners/models/banner.php

+	 */
+	protected function preprocessForm(JForm $form, $data, $group = 'content')
+	{
+		// Check if article is associated


wrong comment

This got removed in the subsequent commit 5 hours ago :P

bembelimen · 2016-07-22T17:37:19Z

@wilsonge you have to add the ACL check to the saving process, too:

https://github.com/wilsonge/joomla-cms/blob/6d3d1293d88eed23528ef2193a5846215d6b8f38/administrator/components/com_categories/helpers/categories.php#L151-L152

and/or

https://github.com/wilsonge/joomla-cms/blob/6d3d1293d88eed23528ef2193a5846215d6b8f38/administrator/components/com_content/models/article.php#L508-L519 (and the other components)

wilsonge · 2016-07-29T09:20:47Z

@bembelimen Checks added to save method - sorry for the delay in adding them in and nice spot!

brianteeman · 2016-07-29T10:52:46Z

I have tested this item ✅ successfully on 0e7d0f8

Tested com_content, com_contact, com_newsfeed and com_banner - all good

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

MATsxm · 2016-07-31T15:22:46Z

I have tested this item ✅ successfully on 0e7d0f8

Able to reproduce then works as described also tested for com_content, com_contact, com_newsfeed and com_banner

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

wilsonge · 2016-07-31T15:25:34Z

Thanks! Merging my own PR so I can get an RC2 out

infograf768 · 2016-07-31T17:17:17Z

i guess similar patch is necessary for webkinks.

* Extra ACL checks (yay) * Fix comments + since * Add checks into save method

Extra ACL checks (yay)

204d991

joomla-cms-bot added the PR-staging label Jul 22, 2016

wilsonge mentioned this pull request Jul 22, 2016

Create new Category on the fly is heavily broken #11162

Closed

izharaazmi reviewed Jul 22, 2016
View reviewed changes

andrepereiradasilva reviewed Jul 22, 2016
View reviewed changes

Fix comments + since

6d3d129

wilsonge reviewed Jul 22, 2016
View reviewed changes

wojsmol reviewed Jul 22, 2016
View reviewed changes

wilsonge added this to the Joomla 3.6.1 milestone Jul 28, 2016

wilsonge added the Release Blocker label Jul 29, 2016

wilsonge added 2 commits July 29, 2016 10:05

Merge branch 'staging' into category-on-fly-acl

06e1602

Add checks into save method

0e7d0f8

wilsonge merged commit e15a63d into joomla:staging Jul 31, 2016

wilsonge deleted the category-on-fly-acl branch July 31, 2016 15:25

zero-24 mentioned this pull request Jul 31, 2016

Create Categories on-the-fly on Link edit form joomla-extensions/weblinks#252

Merged

roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Sep 11, 2016

Extra ACL checks (yay) (joomla#11244)

e6d49f4

* Extra ACL checks (yay) * Fix comments + since * Add checks into save method

zero-24 removed the Release Blocker label Oct 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra ACL checks (yay) #11244

Extra ACL checks (yay) #11244

wilsonge commented Jul 22, 2016 •

edited by joomla-cms-bot

izharaazmi Jul 22, 2016

andrepereiradasilva Jul 22, 2016

infograf768 commented Jul 22, 2016

andrepereiradasilva Jul 22, 2016

wilsonge Jul 22, 2016

wilsonge Jul 22, 2016

wilsonge commented Jul 22, 2016

izharaazmi commented Jul 22, 2016

wilsonge commented Jul 22, 2016

izharaazmi commented Jul 22, 2016

andrepereiradasilva commented Jul 22, 2016 •

edited

izharaazmi commented Jul 22, 2016

mbabker commented Jul 22, 2016

izharaazmi commented Jul 22, 2016 •

edited

mbabker commented Jul 22, 2016

izharaazmi commented Jul 22, 2016 •

edited

mbabker commented Jul 22, 2016

izharaazmi commented Jul 22, 2016

wojsmol Jul 22, 2016

wilsonge Jul 22, 2016

bembelimen commented Jul 22, 2016

wilsonge commented Jul 29, 2016

brianteeman commented Jul 29, 2016

MATsxm commented Jul 31, 2016

wilsonge commented Jul 31, 2016

infograf768 commented Jul 31, 2016 •

edited

Extra ACL checks (yay) #11244

Extra ACL checks (yay) #11244

Conversation

wilsonge commented Jul 22, 2016 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

izharaazmi Jul 22, 2016

Choose a reason for hiding this comment

andrepereiradasilva Jul 22, 2016

Choose a reason for hiding this comment

infograf768 commented Jul 22, 2016

andrepereiradasilva Jul 22, 2016

Choose a reason for hiding this comment

wilsonge Jul 22, 2016

Choose a reason for hiding this comment

wilsonge Jul 22, 2016

Choose a reason for hiding this comment

wilsonge commented Jul 22, 2016

izharaazmi commented Jul 22, 2016

wilsonge commented Jul 22, 2016

izharaazmi commented Jul 22, 2016

andrepereiradasilva commented Jul 22, 2016 • edited

izharaazmi commented Jul 22, 2016

mbabker commented Jul 22, 2016

izharaazmi commented Jul 22, 2016 • edited

mbabker commented Jul 22, 2016

izharaazmi commented Jul 22, 2016 • edited

mbabker commented Jul 22, 2016

izharaazmi commented Jul 22, 2016

wojsmol Jul 22, 2016

Choose a reason for hiding this comment

wilsonge Jul 22, 2016

Choose a reason for hiding this comment

bembelimen commented Jul 22, 2016

wilsonge commented Jul 29, 2016

brianteeman commented Jul 29, 2016

MATsxm commented Jul 31, 2016

wilsonge commented Jul 31, 2016

infograf768 commented Jul 31, 2016 • edited

wilsonge commented Jul 22, 2016 •

edited by joomla-cms-bot

andrepereiradasilva commented Jul 22, 2016 •

edited

izharaazmi commented Jul 22, 2016 •

edited

izharaazmi commented Jul 22, 2016 •

edited

infograf768 commented Jul 31, 2016 •

edited