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

Extra ACL checks (yay) #11244

Merged
merged 4 commits into from Jul 31, 2016

Conversation

Projects
None yet
@wilsonge
Copy link
Contributor

commented Jul 22, 2016

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
@@ -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');

This comment has been minimized.

Copy link
@izharaazmi

izharaazmi Jul 22, 2016

Contributor

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

This comment has been minimized.

Copy link
@andrepereiradasilva

andrepereiradasilva Jul 22, 2016

Contributor

@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

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

Why:
// Check if article is associated everywhere?

*
* @return void
*
* @since 3.0

This comment has been minimized.

Copy link
@andrepereiradasilva

This comment has been minimized.

Copy link
@wilsonge

wilsonge Jul 22, 2016

Author Contributor

Fixed ty!

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

This comment has been minimized.

Copy link
@wilsonge

wilsonge Jul 22, 2016

Author Contributor

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

@wilsonge

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2016

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

@izharaazmi

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2016

Is that case handled at the moment in category creation?

@izharaazmi

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

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

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

This comment has been minimized.

Copy link
@wojsmol

wojsmol Jul 22, 2016

Contributor

wrong comment

This comment has been minimized.

Copy link
@wilsonge

wilsonge Jul 22, 2016

Author Contributor

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

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

wilsonge added some commits Jul 29, 2016

@wilsonge

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2016

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

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2016

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 wilsonge merged commit e15a63d into joomla:staging Jul 31, 2016

2 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilsonge wilsonge deleted the wilsonge:category-on-fly-acl branch Jul 31, 2016

@wilsonge

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2016

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

@infograf768

This comment has been minimized.

Copy link
Member

commented Jul 31, 2016

i guess similar patch is necessary for webkinks.

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

Extra ACL checks (yay) (joomla#11244)
* Extra ACL checks (yay)

* Fix comments + since

* Add checks into save method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.