[#14268] - [4.0] New authorization library and database normalization
- Closed
- 7 Mar 2018
- Medium
- Build: 4.0-dev
- # 14268
- Diff
- klas:acl40-rebased1
- Failure continuous-integration/drone/pr the build failed Details
- Failure continuous-integration/travis-ci/pr The Travis CI build failed Details
- Failure continuous-integration/appveyor/pr AppVeyor build failed Details
User tests: Successful: Unsuccessful:
Design goals
- Drastically improve performance
- Allow multiple implementations trough common interface
Objectives
- Database normalization: replace JSON rules in assets table with separate permissions table
- Remove unnecessary complexity: replace Rule and Rules with easier to understand and use authorization matrix
- Replace preloading with multiple items authorization: blindly preloading all items can lead to huge memory consumption on sites with millions of items (edit: in fact number seems to be even lower #13542 ), also it needs to be controlled by the application, not by the library.
Same performance boost as with Access library preloading is achieved with mutiple items authorization in one call (or using filter query). - Common interface allows new implementations using different authorization models. Any implementation must implement AuthorizeInterface. By default there are JooomlaLegacy (see bellow) and Joomla (using only new data model) implementations
- Full Backwards compatibility (with depreciation): Access is proxied to JoomlaLegacy implementation, that merges old and new data models when getting data. Any application using API should continue to work. Saves are done only into new table.
I also tried to make everything as secure and strict as possible to minimize possibility of other classes interfering with the results of authorization (e.g. trough forced usage of getters and setters).
Benchmarks
Check for 'edit' authorization on 1000 items
OLD ACL with preload
Time: 12.51 ms / 168.24 ms Memory: 0.614 MB / 6.46 MB Application: Before JAccess::preloadComponents (all components)
Time: 2.17 ms / 170.41 ms Memory: 0.037 MB / 6.50 MB Application: After JAccess::preloadComponents (all components)
Time: 1.70 ms / 172.11 ms Memory: 0.025 MB / 6.52 MB Application: Before JAccess::preloadPermissions (com_content)
Time: 9.32 ms / 181.43 ms Memory: 0.727 MB / 7.25 MB Application: After JAccess::preloadPermissions (com_content)
Time: 93.16 ms / 274.59 ms Memory: 7.382 MB / 14.63 MB Application: Start Article multiple access check
Time: 195.13 ms / 469.72 ms Memory: 0.479 MB / 15.11 MB Application: Stop Article multiple access check
Total: 301,48 ms
New ACL with multiple check - JoomlaLegacy implementation
Time: 108.51 ms / 284.34 ms Memory: 8.412 MB / 14.85 MB Application: Start Article multiple access check
Time: 97.78 ms / 382.13 ms Memory: 0.655 MB / 15.51 MB Application: Stop Article multiple access check
Total: 97.78 ms
New ACL with multiple check - Joomla implementation
Time: 109.70 ms / 288.07 ms Memory: 8.338 MB / 14.78 MB Application: Start Article multiple access check
Time: 77.02 ms / 365.10 ms Memory: 1.240 MB / 16.02 MB Application: Stop Article multiple access check
Total: 77.02 ms
Todo
- Add unit tests for Joomla implementation
- Replace JAccess calls in core with new API
- Convert sample data and create upgrade sql.
Testing Instructions
Functional tests
Due to database changes you will need 2 installations, one with old databse/code and another with a new one. Choose plain/empty Joomla on installation, sample data is not converted yet.
Any ACL check should continue to work as before.
Basic check is to see that global and component permissions show the same values as before:
Open global configuration ->permissions tab and compare computed permissions for each group. Do the same for e.g. com_content and/or any other component.
Next check is to test permissions inheritance (tests are mostly the same as in #12850):
Global -> Component [-> Category] -> Item and also user group inheritance (child group must inherit from parent e.g. Registered->Author->Editor->Publisher).
Basic Joomla ACL rule to consider here is that anything denied on higher can't be allowed on the lower level
Some examples:
-
In global config child groups should inherit from their parent
-
Simple component (without categories, ex: com_modules):
- component permissions must inherit from global config.
- item must inherit from component permissions.
-
More complex component (with categories, ex: com_content):
- component permissions must inherit from global config.
- root categories permissions must inherit from component permissions.
- child categories permissions must inherit from parent categories permissions.
- item must inherit from his category permissions.
Note: In all the cases above you need to also check user group inheritence.
Other tests
- When a asset for a item does not exist in the #__assets table it should fallback to the component asset or to root asset (in case of a component).
- Super user can access everything
- Uer without super right permission can't become a super user
Performance test
Set max_execution_time=300 or something like that and memory_limit=-1 in php.ini . This should give you enough resources to execute this insert and test.
Due to database changes you will need 2 installations (choose plain/empty Joomla on installation, sample data is not converted yet). On both:
-
Disable tags and fields, otherwise your browser will crash due to number of queries displayed
-
Create user 1, Assign author group
-
Insert 1000 articles, under user1 - change user id in the script to match it's id. Also adjust category id if it is not the same.
CLI script for article insertion:
insertarticles.zip -
Create user2 with editor rights.
-
Create menu with category list and set # Articles to List to 100
-
Set Show tags to No in com_content options (or browser will crash due to the number of queries)
-
Activate debugging
-
In the site with old Access go to Access library and comment out all JDEBUG lines in getAssetRules. Also go to com_content/models/articles.php and add the following to getItems function (after $globalParams definition):
$profiler = JProfiler::getInstance('Application');
JDEBUG ? $profiler->mark('Start Article multiple access check') : null;
foreach ($items as &$item)
{
$asset = 'com_content.article.' . $item->id;
// Check general edit permission first.
$user->authorise('core.edit', $asset);
}
JDEBUG ? $profiler->mark('Stop Article multiple access check') : null;
Documentation
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_config com_users Front End com_content SQL Installation Libraries Unit Tests |
| Labels |
Added:
?
?
|
||
| Category | Administration com_config com_users Front End com_content SQL Installation Libraries Unit Tests | ⇒ | Administration com_config Front End com_content SQL Installation Libraries Unit Tests |
Double requirement's purpose was to force existence of APPENDSUPPORT constant and some properties as it can't be done trough interface alone. Can be replaced with a method, or simply by method returning false if it is not implemented, but in my opinion a constant is clearer.
In general I tried to make everything as secure and strict as possible to minimize possibility of other classes interfering with the results of authorization (e.g. trough forced usage of getters and setters).
You can have constants on interfaces. So that alone shouldn't force it.
On Fri, Mar 3, 2017 at 5:46 PM Klas notifications@github.com wrote:
Double requirement's purpose was to force existence of APPENDSUPPORT
constant as it can't be done trough interface alone. Can be replaced with a
method, but in my opinion a constant is clearer.In general I tried to make everything as secure and strict as possible to
minimize possibility of other classes interfering with the results of
authorization (e.g. trough forced usage of getters and setters).—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#14268 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoT3swRFfraJ-8RU_i9OI3fmAu5Kvks5riKZLgaJpZM4MNB29
.
--
- Michael Please pardon any errors, this message was sent from my iPhone.
Interface constants cannot be overridden by a class/interface that inherits them and I wanted that possibility.
I removed both, a requirement and a constant, as method returning false is enough - it does not matter to application whether method has failed or it is just not implemented. Thank you for your observations.
| Category | Administration com_config Front End com_content SQL Installation Libraries Unit Tests | ⇒ | Administration com_config com_languages Front End com_content SQL Installation Libraries JavaScript Unit Tests |
| Category | Administration com_config Front End com_content SQL Installation Libraries Unit Tests com_languages JavaScript | ⇒ | Administration com_config Front End com_content SQL Installation Libraries Unit Tests |
@klas Trying to install a copy of your branch (with sample data) https://github.com/klas/joomla-cms/tree/acl40-rebased1 to test but receiving this error:
Error
Duplicate entry '1-core.login.site-1' for key 'idx_aid_per_val'
Table 'joomla_acl.#__assets' doesn't exist
Specified key was too long; max key length is 767 bytes
Please check and correct it. Once it is fixed, I will test it.
Hi,
do not use sample data, sample was not converted yet (forgot to add this into instructions). But I will also re-check,it might be something got broken with recent merges.
Look like you need to re-check. Install still fails even without sample data
Sorry for this, will check as soon as possible.
No problem. Just let me know when it is ready.
@joomdonation Installation works now for me (on MariaDB) - I made a mistake when changing indexes, primary index on #__assets needs limitation on rules column, otherwise index is too big.
As noted above in conversation with @csthomas I will revisit all indexes and remove unneeded ones
I did a fresh install of your branch. Then I wanted to give the Registered users access to the back end. It kicked me out of the admin area and since then I can't log in again.
Please hold on all tests before I retest everything, something got really borked after latest merges/chages
| Category | Administration com_config Front End com_content SQL Installation Libraries Unit Tests | ⇒ | Administration com_config com_languages Front End com_content SQL Installation Libraries JavaScript Unit Tests |
@joomdonation @laoneo Should be ok now (I hope), I forgot to change column in Access table when I changed db column as group is reserved word in mysql..
I spent sometime to test this PR today. Overall, it works good. Tested several cases:
-
Tested global configuration. Try to change permission setting for a group, saved properly. Check calculated inherit permission for child groups and it is calculated properly, too
-
Tested com_content permission
- Component permission is inherited properly from global permission
- Articles permission is inherited properly from category
- However, individual article permission seems doesn't work well. Only checked with Denied permission. I set all permission (including Edit permission) of an article for Registered group to be denied (see this screenshot https://drive.google.com/file/d/0B2RoJ_24efMQWkRTWWM0QU51R1U/view?usp=drivesdk )
However, when I logged in to frontend, I still see edit icon which allow editing this article (at the moment, there is fatal error when trying to edit from frontend, however, ACL is calculated wrong for some reasons)
Note that I tested with an article created by sample data if there is any difference
do not use sample data, sample was not converted yet
I tried to reproduce this and it works ok for me without sample data.
Please note that edit.own permission set to allowed on the component or the category would override edit set to denied on article, if article author is the same as current user (there is no edit.own on article level), so please check that.
Ah, you are correct. I didn't know that edit.own allows edit the item although edit set to Denied
I disabled edit.own permission and the edit button is gone. So it works OK
In terms of Functional tests, I think it can be merged. Someone please help with Performance test
thnx @joomdonation , I think you can mark it as successful test on JIssues tracker as it works functionally
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14268.
| Title |
|
||||||
(title updated to show its for j4)
| Title |
|
||||||
| Category | Administration com_config Front End com_content SQL Installation Libraries Unit Tests com_languages JavaScript | ⇒ | Administration com_config com_languages Front End com_content SQL Installation Libraries Unit Tests |
| Category | Administration com_config Front End com_content SQL Installation Libraries Unit Tests com_languages | ⇒ | Administration com_config com_languages Front End com_content com_users SQL Installation Language & Strings Libraries Unit Tests |
| Labels |
Added:
?
|
||
| Category | Administration com_config Front End com_content SQL Installation Libraries Unit Tests com_languages com_users Language & Strings | ⇒ | Administration com_config com_languages Front End com_content SQL Installation Libraries Unit Tests |
| Labels |
Removed:
?
|
||
| Milestone |
Added: |
||
| Milestone |
Added: |
||
Design goals
- Drastically improve performance
- Allow multiple implementations trough common interface
The first goal seems to be addressed, however I find nothing about the more important second goal. Did I miss something?
@nibra not sure I understand your question, we even discussed this interface to be added to JoomlaX at JAB - it is there https://github.com/joomla/joomla-cms/pull/14268/files#diff-94cc01d6ce6b857d00687b098f98d0aa
Ok, I obviously missed it ;) Comments will follow.
At JAB we discussed that $actorType is not needed (where this info is needed data object can be passed to $actor) in check() and that appendFilterQuery could be avoided. This is already on my todo list.
Currently I'm waiting to get some sort of confirmation that this pull will get merged if it satisfies all obvious requirements (arhitectural, code, test etc) before I will get back to work. There is no point in investing more time in case those that hold the keys think this is not needed, hope this sounds reasonable
OK I've had a long long time to think about this. And I had some long discussions with various people in the community and extension development community at JAB and JWC this year. After some careful thought I’m going to choose to reject this PR for the following reasons:
- I’m unconvinced by whether there would actually be an ability in Joomla to actually use different authorisation models - You’d need to overload the DIC entry in a plugin event (I’m assuming that the getInstance method would have to go in favour of having the authorisation system stored in the container) - but the authorisation model has to be loaded to load the plugin system in the first place (unless you do some sort of hack around the plugin system like we have in the current authorisation plugins).
- The only practical use case I’ve heard people regularly talk about would be for doing a full integration of Joomla with a LDAP server. Whilst obviously you could technically do this - the interface is really geared for having all the permissions stored in the db (as evidenced by the appendFilterQuery) - so I’m not sure the interface chosen here actually is going to solve many problems for users.
- I’ve had a chat with some people about the new db schema. Whilst most people agreed that your new schema was going to give some performance improvement, there were several issues raised about the schema implemented. I’m not going to try and Chinese whisper what people said - but if you want I can tag Eli who I consulted with on this at JWC who I’m sure will be happy to write the issues he talked to me about at JWC in his own words (they were solvable)
- I’m worried about the migration path. Most your deprecations are for 4.2 (and whilst that’s obviously flexible to a point), since we moved to SemVer - we wouldn’t be able to do this until 5.0. I also can’t see any easy way to move from the existing asset tables to your new permissions table - I assume a 1 off schema migration could be written - it seems incredibly risky when performance gains are really only going to be seen on maybe 1% of the top Joomla sites.
For the record I’d definitely be interested to see if we can improve the lazy loading of the system because as you say there are definitely massive issues in the way the existing system preloads that causes huge performances implications on only a few hundred ACL groups.
I know that having this open for a year has been hard for you and I’d like to thank you for being so polite in pressing me to make a decision on this - I take full responsibility for taking so long to sit on this.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-03-07 22:29:21 |
| Closed_By | ⇒ | wilsonge |
Just for the record - appendFilterQuery is to be dropped as it really does not belong in the interface.
Joomla continues to have a bottle neck in the permissions 2 years later.
The short answer was to enlarge the column, but this is also being pushed back as a bad choice (as a performance impact) which I understand, and agree, but what other options do we have? The greater performance impact is not being able to manage large sets of permissions, and therefore limiting Joomla.
So yes five years ago I made this choice in JCB
vdm-io/Joomla-Component-Builder#616
So for five years 5000+ extensions have changed this table, and have enjoyed larger permissions with little to no noticeable impact.
We must resolve this, or at least allow a workaround.
Requiring the interface is fine. Extension should not be required, though. Inheritance should serve convenience only.