User tests: Successful: Unsuccessful:
Pull Request for Issue #15960 .
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.
Super Users can assign any category
That "Guest" category isn't available
None
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_categories |
Title |
|
Title |
|
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.
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.
I have tested this item
Works, categories now are visible and can be assigned.
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
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,
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
@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
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)
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
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.
I was thinking that title would be mentioned ...
Why stick to answering my mistake in my initial comment(s) and mentioning the title ?
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
And to add to weirdness described above
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
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 ??
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.
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,
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)
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
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
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
As said multiple times already, please open a new issue. It's offtopic here.
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 |
Can we please just fix
JAccess::getAuthorisedViewLevels()
at some point instead of continuing to add all of this bypass crap?