? Success

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
1 Jun 2017

Pull Request for Issue #15960 .

Summary of Changes

This PR bypassed the access level check introduced with #12931 for Superusers, similar to how it is done for articles (see https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_content/models/articles.php#L231-L237)

On a sidenote: I removed a second instance of $user = JFactory::getUser(); in that method since $user at that point already exists.

Testing Instructions

  • Create a category and set it to access level "Guest" (or any other level where SuperUsers aren't part of)
  • Edit an article and try assigning that category.

Expected result

Super Users can assign any category

Actual result

That "Guest" category isn't available

Documentation Changes Required

None

avatar Bakual Bakual - open - 1 Jun 2017
avatar Bakual Bakual - change - 1 Jun 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Jun 2017
Category Administration com_categories
avatar mbabker
mbabker - comment - 1 Jun 2017

Can we please just fix JAccess::getAuthorisedViewLevels() at some point instead of continuing to add all of this bypass crap?

avatar Bakual Bakual - change - 1 Jun 2017
Title
Bypass viewlevel check for cetagoryedit field when Superuser
Bypass viewlevel check for categoryedit field when Superuser
avatar Bakual Bakual - edited - 1 Jun 2017
avatar Bakual Bakual - change - 1 Jun 2017
Title
Bypass viewlevel check for cetagoryedit field when Superuser
Bypass viewlevel check for categoryedit field when Superuser
avatar Bakual
Bakual - comment - 1 Jun 2017

Can we please just fix JAccess::getAuthorisedViewLevels() at some point instead of continuing to add all of this bypass crap?

Not sure if that would be an accurate fix. Because in other places (eg frontend?) we may not want to show all items for a superuser. Anyway it would be a change in behavior of the method and should probably only be done with J4.

avatar mbabker
mbabker - comment - 1 Jun 2017

I don't care when we fix it, we just need to. If our system design is that super users should always see all the things, then we need to fix the ACL and view access logic for that, not bypass it.

avatar idefax
idefax - comment - 1 Jun 2017

I have tested this item successfully on 846b353

Works, categories now are visible and can be assigned.


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

avatar idefax idefax - test_item - 1 Jun 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 1 Jun 2017

I have tested this item successfully on 846b353


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

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 1 Jun 2017 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 1 Jun 2017
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 1 Jun 2017

RTC after two successful tests.

avatar ggppdk
ggppdk - comment - 2 Jun 2017

This fix is correct but i argue it is incomplete,

The most proper usage of the Access levels for categoryedit field

is to consider both core.create ACL and view access levels
but consider them in an appropriate way

After the core.create ACL is considered,

  • and user is not able to create records inside a category
  • then the category should be

either shown as DISABLED if user has the needed access level to view the category
or HIDE the category if user does not have the needed access level to view it

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 2 Jun 2017

@ggppdk set Status back on "Pending"?

avatar ggppdk
ggppdk - comment - 2 Jun 2017

@ggppdk set Status back on "Pending"?

No, no need to remove RTC or something,
this PR is good, it restores previous behaviour, and argueablly a more correct behaviour

What i suggested above can be a different PR, if people involved here would agree with what i suggested

avatar ggppdk
ggppdk - comment - 2 Jun 2017

Sorry i just checked this PR more carefully

And it does

		if (!$user->authorise('core.admin'))
		{
			$groups = implode(',', $user->getAuthorisedViewLevels());
			$subQuery->where('access IN (' . $groups . ')');
		}

It adds special behaviour of access levels for super user
i do not like it

I suggested a different thing

NOT to use access levels at all when considering record creation / assignment of records to a category
Instead only consider core.create ACL

then if lacking core.create ACL, only then consider view access levels
to show category as disabled (if viewable) or as hide it (if not viewable)

avatar ggppdk
ggppdk - comment - 2 Jun 2017

It adds special behaviour of access levels for super user

So my comment that this restores previous behaviour was also wrong

This PR justs adds a bypass of view access levels only for super user

Previous behaviour was not to use view access levels for any user and instead only use core.create ACL

avatar Bakual
Bakual - comment - 2 Jun 2017

Thanks for confirming that this PR does exactly what it says in the title.
The intention isn't to restore previous behaviour. It's to bypass viewlevel check for superusers. Same as we do eg for articles.

avatar ggppdk
ggppdk - comment - 2 Jun 2017

I was thinking that title would be mentioned ...

avatar ggppdk
ggppdk - comment - 2 Jun 2017

@Bakual

Why stick to answering my mistake in my initial comment(s) and mentioning the title ?

  • instead of commenting on my argument that both existing behaviour and also this PR are wrong

I was hoping i would get some support on getting a meanigful behaviour here

Now a user in order to submit to a category it is obligatory to be able to list articles / records of other authors in that category

Really wrong mixing of core.create ACL with the access levels

avatar ggppdk
ggppdk - comment - 2 Jun 2017

And to add to weirdness described above

  • why consider access levels for CREATing but not for DELETing ? or PUBLISHing
  • you would also need to update the language strings for create ACL ... and mention the mixing with access levels
  • update the core.create ACL checks of controllers / models / views or whereever core.create is used, because their a security concern here of giving false message to people configuring a web-site

that some "non-viewable" categories are not possible to have users create articles even if they have core.create ACL privilege in them

which is not true, some open the form to checkout the item and then can submit to a category that user has create privilege, to the controller and create items
(i have tried it just now)

so you would also need to update the create ACL checks of the controllers too

why did we

  • suddenly after years yes after years

that we need to complement / combine core.create with the view levels , this weird and i argue above wrong way ?

Also another crazy example i have a category with guest access level
my author sthat do not have guest access level,
they should not be able to create articles in the category ??

avatar Bakual
Bakual - comment - 2 Jun 2017

As said, the intention of this PR is not to revert the viewlevel checking. It just tries to fix a bug where a SuperUser can edit an article but the categoryedit field will not show the assigned category due to missing viewlevel. After saving, that article will be saved with the "default" category instead of the original one. Which is clearly a bug.

Be aware that this only can happen to SuperUsers, because other users will not be able to edit the article to begin with. They will not see the article in the backend manager or in frontend and thus never see an edit link for it.

Also regarding the guest category level. I'd say you need to add the author group to that guest viewlevel anyway. Otherwise they would not be able to edit the article after it had been created, which is likely not what you want as well.

If you think the current viewlevel check is wrong, feel free to open an issue/PR about it. It may or may not be a valid issue. But lets not discuss that in this PR because this one tries to fix a valid bug.

avatar ggppdk
ggppdk - comment - 3 Jun 2017

About continuing discussion in seperate PR to REVERT adding the view access level check, and causing all problems i described above

ok, and i am sorry for bothering you here in this thread,
but please allow 1 more comment , since i have no time next a fews days to be involved in any PRs / issue / discussions / conflicts

About:

Also regarding the guest category level.

i meant it is a problem with any level,

  • i do not want people to list / view a category,
  • but i want them to be able to submit in it !!

now we think it is good to make it impossible

I'd say you need to add the author group to that guest viewlevel anyway. Otherwise they would not be able to edit the article after it had been created, which is likely not what you want as well.

here i am not sure what you mean with the above

$user->authorise('core.edit', $asset)
$user->authorise('core.edit.own', $asset)   .... + being owner

code like the above is used everywhere inside joomla and in hundreds of 3rd party components too

(and hiding it in a listing does not make it not editable)

Do we want to patch all ACL code of joomla and hunderds of 3rd party components to combine

access levels with

core.create
core.edit
core.edit.state
core.delete
core.whatever

this thing with suddenly combining access levels with Task-Specific combine ACL is just crazy

Please read this phrase
"if user has access to do action"

but which action ?

if you ask about creating then we do not have a general access privilege

instead we have a specific one , distiguishable from access to other tasks !!

it is core.create

again why we want to start mixing it with access levels ?
if we decide that you want to combine it with the
we would need to introduce a new ACL ??:

core.view

so that to be able to distiguish viewing ?

Finally i am sorry for answering here again , i will open a new issue / make a PR
when i get some time,

in the meantime people

please read my comments in this answer, and here:
#16411 (comment)

avatar Bakual
Bakual - comment - 3 Jun 2017

Not going to answer in detail because it really is offtopic.
Keep in mind that the viewlevel check for the category is live since 3.7.0.
Checking the article viewlevels itself is done since 1.7.3 (6 years now).

It is certainly not a new concept by itself

avatar ggppdk
ggppdk - comment - 3 Jun 2017

Keep in mind that the viewlevel check for the category is live since 3.7.0.
Checking the article viewlevels itself is done since 1.7.3 (6 years now).

yes you are right

It is certainly not a new concept by itself

you are right again

and i understand that the access level check in categoryedit was added,
it is because of using view access levels in article manager to list articles
and now also category view level too

but there are performance wise alternatives to listing articles in article manager using the access levels
e.g. list articles if viewable AND / OR in editable categories

and then if article is shown as not editable
(because of lacking core.edit.* on the article asset but having core.edit.* on category),
then this ok, because this happens already with using access levels only

  • also note the security concern that i mentioned above (i would not call it a security issue as it is not),
    editable articles are just hidden, do we want to to update all Joomla and 3rd party extension ACL checks too ?

and if you tell me that was happening before too,
i would say overriding category ACL on specific article assets was rare,

now that articles are hidden based on the view level of category too, we have made a very rare security concern to be much more common

avatar Bakual
Bakual - comment - 3 Jun 2017

As said multiple times already, please open a new issue. It's offtopic here.

avatar rdeutz rdeutz - change - 13 Jun 2017
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-06-13 14:49:23
Closed_By rdeutz
avatar rdeutz rdeutz - close - 13 Jun 2017
avatar rdeutz rdeutz - merge - 13 Jun 2017

Add a Comment

Login with GitHub to post a comment