[4.0] [PoC] Leverage access check form the plugin loader in to the plugin by HLeithner · Pull Request #25152 · joomla/joomla-cms

HLeithner · 2019-06-09T16:31:38Z

This is a proof of concept to remove the "always session creation" because of loading plugins incl. full b/c.

I know that this could be done better, this is only to get some feedback.

Summary of Changes

Removed the access level check from the plugin load query.
Add a combat layer for all plugins not being loaded with a service handler
Only load the user session if the plugin is not public (legacy and new)
Add support to override the access field in the plugin manager
Added a helper function in CMSPlugin to check the access level with less overhead

The quickicon joomlaupdate plugin is the only plugin at the moment that uses the service helper. I changed this to support access check on event trigger and disabled the access field as example. (make no sense to do both).

before patch

#0  Joomla\CMS\User\User::getInstance(0) called at [/libraries/src/Access/Access.php:997]
#1  Joomla\CMS\Access\Access::getAuthorisedViewLevels(0) called at [/libraries/src/User/User.php:445]
#2  Joomla\CMS\User\User->getAuthorisedViewLevels() called at [/libraries/src/Plugin/PluginHelper.php:258]
#3  Joomla\CMS\Plugin\PluginHelper::load() called at [/libraries/src/Plugin/PluginHelper.php:180]
#4  Joomla\CMS\Plugin\PluginHelper::importPlugin(system) called at [/libraries/src/Application/CMSApplication.php:229]
#5  Joomla\CMS\Application\CMSApplication->execute() called at [/includes/app.php:63]
#6  require_once(/includes/app.php) called at [/index.php:36]

after patch

#0  Joomla\CMS\User\User::getInstance(0) called at [/libraries/src/Access/Access.php:997]
#1  Joomla\CMS\Access\Access::getAuthorisedViewLevels(0) called at [/libraries/src/User/User.php:445]
#2  Joomla\CMS\User\User->getAuthorisedViewLevels() called at [/libraries/src/Menu/AbstractMenu.php:322]
#3  Joomla\CMS\Menu\AbstractMenu->authorise(101) called at [/libraries/src/Application/SiteApplication.php:97]
#4  Joomla\CMS\Application\SiteApplication->authorise(101) called at [/libraries/src/Application/SiteApplication.php:771]
#5  Joomla\CMS\Application\SiteApplication->route() called at [/libraries/src/Application/SiteApplication.php:235]
#6  Joomla\CMS\Application\SiteApplication->doExecute() called at [/libraries/src/Application/CMSApplication.php:241]
#7  Joomla\CMS\Application\CMSApplication->execute() called at [/includes/app.php:63]
#8  require_once(/includes/app.php) called at [/index.php:36]

This mean we are not finished decoupling the cms from the user session, we have to do something similar to the menu. The simplest is to check for public access before loading the user.

Documentation Changes Required

Plugin authors has to do the access check on each event listener

if (!$this->accessCheck())
{
  return;
}

If the plugin doesn't need support for access levels it can override the access field with a hidden field in the config file.

<config>
  <field name="access" type="hidden" />
</config>

libraries/src/Plugin/PluginHelper.php

libraries/src/Plugin/CMSPlugin.php

mbabker · 2019-06-09T19:15:39Z

libraries/src/Extension/ExtensionManagerTrait.php

+					$pluginAccessLevel = (int) PluginHelper::getPlugin($pluginType, $pluginName)->access;
+
+					// if the accesss level is public we don't need to load the user session
+					if ($publicAccessLevel !== $pluginAccessLevel)


It's everything in this conditional that lets you not load a plugin if ACL doesn't match, which is the scenario that needs to be avoided IMO. If you remove the changes from this file, in theory this PR is good to go for removing the access check from the root query and deferring responsibility into the plugin.

This condition is only a shortcut for plugins that have the access level "public". If the access level is not "public" it will load the user and make a proper access check. this is also be done if the access level is set to guest.

The plugin is not loaded in line 168 if the user has no access to it, that's the same behavior then in the query. Because in the getAuthorisedViewLevels() function every user is automatically added to the public access level.

The plugin is not loaded in line 168 if the user has no access to it, that's the same behavior then in the query.

That's the part I feel like needs to go away. A plugin should always load because there are going to be cases where the plugin needs to handle a failing ACL check for something within it, and with this condition that keeps the plugin from being able to deal with that.

IMO the aim should be the access checks are 100% within the plugin, there should be no checks outside of the plugin determining whether a plugin is even loaded.

Set the access on the "Content - Load Modules" plugin to Registered, and view an article that has the {loadmodule} shortcode within it as a guest. I would expect that the shortcode is removed from the article when it is displayed, but because the plugin isn't loaded at all, the shortcode is going to print on the page. This is the exact type of scenario that moving the ACL checks to be solely within the plugin helps address, anything that's making ACL decisions outside the plugin means something that needs to be addressed in a negative access scenario cannot be done unless the plugin author implements a second ACL like field and instructs users to never touch the core Access field.

Exactly this scenario is covered with this Pr but not for the legacy plugins. Only for plugins using a ServiceProvider.

In line 142 we register the service provider and only come to this code path for legacy code in Line 147

https://github.com/joomla/joomla-cms/pull/25152/files/7820f37ae59a439c90f82ffcf3372d3b70ce05ce#diff-d1c7be7f90bfc88895fd930b7761d997R142

https://github.com/joomla/joomla-cms/pull/25152/files/7820f37ae59a439c90f82ffcf3372d3b70ce05ce#diff-d1c7be7f90bfc88895fd930b7761d997R147

So every plugin for Joomla 4 have to manage the access control when a event is triggered. All legacy plugins are handled like in J3 (not loaded if the access level doesn't match).

Then we're better off keeping the existing behavior instead of trying to replicate it in a round-about way. I think we just need to accept the B/C break and not have any form of ACL in front of plugins, the plugins themselves need to be responsible for handling it. Actually, I'd suggest the internal behavior is worse off if this code path is arbitrarily applying an ACL decision and the other code path isn't; there's too much inconsistency involved, both in the code and in trying to explain to end users why plugins selectively get loaded.

Plugins need to always load, period. It should not be possible for a plugin class to not at a minimum be instantiated if its group is loaded.

I'm completely with you, the plugin should always be loaded. But doing this in J4 for legacy plugins makes no sense if we try to simulate the old code path.
The code path we are looking at is deprecated for Joomla 5 isn't it?

So we have the same behavior for J3 plugins and the new behavior for J4 plugins. The enduser will never notice this. If the plugin is correctly implemented the user will not have the choice to select a access level if the plugin doesn't support it.

Why do we need to simulate the old code path though? And no, it won't be something that goes unnoticed by the end user.

A plugin which uses the service provider approach and has a non-hidden access field will be loaded at all times (unless the developer puts ACL code in the service provider, which is IMO a huge mistake, but we've all seen some "interesting" code in extensions so this is going to be 100% in the realm of possibility)

A "legacy" plugin which has a non-hidden access field can be not loaded if the ACL check fails

How does the user know if a plugin is "legacy" or uses the service provider?

This is creating a by-design inconsistency in the API and one that isn't transparent to the user.

I see a high risk if we load a "legacy" plugin which doesn't do the access check and have no way to warn the user that the access field seen in the plugin configuration is ignored.

We can remove this field for all plugin ins and require the plugin author to implement it by him self.

If that's the way to go I can update the PR. Any opinions by @wilsonge ?

libraries/src/Plugin/PluginHelper.php

HLeithner · 2019-06-10T14:29:06Z

At the moment Benjamin is trying to delete the public access level only with the backend ;-) but I think he is failing.
It's hardcoded in the getAuthorisedViewLevels funtion:
https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Access/Access.php#L994

The parameter I used was not the public access level it was the "default" access level which is used when creating new items.

brianteeman · 2019-06-10T15:12:14Z

The parameter I used was not the public access level it was the "default" access level which is used when creating new items.

Are you saying that you are using the value of "default" access level in global configuration to determine if an item is public?

HLeithner · 2019-06-10T15:18:59Z

The parameter I used was not the public access level it was the "default" access level which is used when creating new items.

Are you saying that you are using the value of "default" access level in global configuration to determine if an item is public?

Yes that was my fault assuming that a config option that class "access" and the title "default access level" could be public... but @bembelimen corrected me that this that only the default value for newly created (content?) items.

Thats the reason for the last commit going to use the hardcoded value "1" instead of the config option.

brianteeman · 2019-06-10T15:27:12Z

If you are referring to an access level with an id of 1 (not at a pc right now) then that wont work. There is nothing to say that a site has an access level with an id of 1

HLeithner · 2019-06-10T15:34:55Z

There is nothing to say that a site has an access level with an id of 11

Not sure what you mean with this.
But if you mean that public is not id 1 then you are wrong, as already explained in #25152 (comment) Joomla always add the access level with the id 1 to the user access level that means every user and non user has the access level which is per definition public.

It's also not possible to delete this access level, if you manage to deleted it your site will no longer work because the login doesn't work any longer and without login you can't see anything because everything that has a access level that should be seen by the public has to be 1.

brianteeman · 2019-06-10T15:48:35Z

Sorry i was unclear. There is nothing to say that the access level with id =1 is in use on the site. It is perfectly possible to build a site that doesn't use the access level that has an id of 1

HLeithner · 2019-06-10T15:52:00Z

Sorry i was unclear. There is nothing to say that the access level with id =1 is in use on the site. It is perfectly possible to build a site that doesn't use the access level that has an id of 1

No it's not possible, the value 1 is hard coded in getAuthorisedViewLevels see
https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Access/Access.php#L994

And even if you get this magically done the code still works because all menu and plugin items will by pass the "short cut" and validate against the user object as it is now.

brianteeman · 2019-06-10T16:15:52Z

Well that's a bug then if you are saying that even if you remove any "User Groups Having Viewing Access" from the "access level" public that it still works

HLeithner · 2019-06-10T16:22:03Z

technical it's a limitation of the system and it's not too bad in my opinion, at least I can't create a use case that needs a change. But I'm sure it's possible to construct one.

mbabker · 2019-06-10T16:24:32Z

Forcing there to be one access group isn't bad. Hardcoding it to ID 1 is. Even on a closed off system, making arbitrary assumptions that a specific row in a specific table will have a specific identifier is a bad idea.

HLeithner · 2019-06-10T16:39:27Z

Forcing there to be one access group isn't bad. Hardcoding it to ID 1 is. Even on a closed off system, making arbitrary assumptions that a specific row in a specific table will have a specific identifier is a bad idea.

Changing this into a configurable value shouldn't be hard because iirc it's only on one position but it will impossible to change it at runtime. If someone wants to implement it I'm happy to look at it, I see no benefit.

SharkyKZ · 2019-06-11T07:39:41Z

No it's not possible, the value 1 is hard coded in getAuthorisedViewLevels see
https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Access/Access.php#L994

I guess we should be using JConfig::$access here. Assuming it holds the ID of public access level.

bembelimen · 2019-06-11T07:58:48Z

I guess we should be using JConfig::$access here. Assuming it holds the ID of public access level.

No it doesn't.

SharkyKZ · 2019-06-11T07:59:45Z

Then what does it do?

bembelimen · 2019-06-11T08:01:20Z

It holds the access level new items (e.g. articles) get as default. See #25152 (comment)

brianteeman · 2019-07-17T14:41:04Z

This is a massive backwards breaking change that serves almost no benefit at all.

wilsonge · 2019-12-08T12:31:25Z

Whilst I understand the benefits of this in terms of there being no active sessions I'm not sure the benefits of this outweigh the cons. I'm going to choose to close this

HLeithner added 2 commits June 9, 2019 17:46

Make plugin access check optional

e0d8e3e

Make plugin access check optional

a615c53

joomla-cms-bot added the PR-4.0-dev label Jun 9, 2019

ghost changed the title ~~[PoC] Leverage access check form the plugin loader in to the plugin~~ [4.0] [PoC] Leverage access check form the plugin loader in to the plugin Jun 9, 2019

ghost added the Documentation Required label Jun 9, 2019

Fix typo

5398449

HLeithner mentioned this pull request Jun 9, 2019

[4.0] Revert the ability to disable session metadata #24765

Closed

Fix caching and function signature

01beb3e

Quy reviewed Jun 9, 2019

View reviewed changes

libraries/src/Plugin/PluginHelper.php Outdated Show resolved Hide resolved

Quy reviewed Jun 9, 2019

View reviewed changes

libraries/src/Plugin/CMSPlugin.php Outdated Show resolved Hide resolved

Move plugin load query to prepared statements.

7820f37

mbabker reviewed Jun 9, 2019

View reviewed changes

cs

fcfc5ee

HLeithner mentioned this pull request Jun 10, 2019

[4.0] Improve menu item access check #25158

Merged

Quy reviewed Jun 10, 2019

View reviewed changes

libraries/src/Plugin/PluginHelper.php Outdated Show resolved Hide resolved

HLeithner added 2 commits June 10, 2019 15:59

cs

84448d9

Hard code public access level

28c10d1

HLeithner referenced this pull request Jun 11, 2019

Don't hardcode public access level

3aef2e2

wilsonge closed this Dec 8, 2019

Uh oh!

Conversation

HLeithner commented Jun 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

before patch

after patch

Documentation Changes Required

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HLeithner commented Jun 10, 2019

Uh oh!

brianteeman commented Jun 10, 2019

Uh oh!

HLeithner commented Jun 10, 2019

Uh oh!

brianteeman commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HLeithner commented Jun 10, 2019

Uh oh!

brianteeman commented Jun 10, 2019

Uh oh!

HLeithner commented Jun 10, 2019

Uh oh!

brianteeman commented Jun 10, 2019

Uh oh!

HLeithner commented Jun 10, 2019

Uh oh!

mbabker commented Jun 10, 2019

Uh oh!

HLeithner commented Jun 10, 2019

Uh oh!

SharkyKZ commented Jun 11, 2019

Uh oh!

bembelimen commented Jun 11, 2019

Uh oh!

SharkyKZ commented Jun 11, 2019

Uh oh!

bembelimen commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianteeman commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilsonge commented Dec 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

HLeithner commented Jun 9, 2019 •

edited

Loading

brianteeman commented Jun 10, 2019 •

edited

Loading

bembelimen commented Jun 11, 2019 •

edited

Loading

brianteeman commented Jul 17, 2019 •

edited

Loading