Hi Denis Thanks for your feedback and sorry it took me so long to respond - I was on Holliday :)
You should have asked during the hackathon, it is also a event to team together, and I would have been more than happy to help you understanding the new security authorization module. For us the Hackathon was an event on short notice, but hopefully it was only the start. Thus we are more than happy to keep working with you on this. Thanks for your review and your explanations. ... This is also what will make your extension more fragile, since it really depends on implementation details that may change over time. I noticed that you have taken care of checking these details to fail gracefully which is nice. As you've noticed we are aware of this, and in fact, this is intended. We did not want to replace the way the rights are checked, but to add an additional check, without touching the existing. For sure, right caching is not that easy, and it implies a bit of complexity, especially if you expected some access to change simply based of the elapsed time since the decision has been taken. This was the most elegant solution we could think of in 2.7.2. One of the reasons being, that it does not rely on external events or schedules. Maybe there is a better solution in 5+? I understand why you would like to have a date from the security cache, this is clever idea even if it has not been made for that. Adding the date should not be complex since the cache store SecurityEntries and those are generated by the AuthorizationSettler. But I am afraid that using those dates later is more complex without somewhat "hacking" the AuthorizationManager. Extending the AuthorizationManager is risky. Evicting entries from other places than the SecurityCacheRulesInvalidator is probably a bad idea and could lead to multithreading issues. Moreover, if I have correctly understand your current implementation, it defeat completely the caching of access entries, which is bad for the performance of the security module. At least, you should mention that in the description of your extension, so nobody get surprised. We agree that special invalidations are tricky and possibly dangerous. Thanks for the hint with the performance penalty. We did already mention the performance penalty in the extension description, but obviously this was not clear enough, so Fabian marked it bold to make it more visible. The best alternative I can think about right now, is to extend the SecurityCacheRulesInvalidator in a way that cause entries to be evicted from the cache in due time, when those became invalid. You may for example keep a cached list of cached entries in your own independent cache with their associated publish times. Listening to the security cache events, you may keep that list clean and up to date. Based on that list, it should be possible to program some timer to trigger an extended SecurityCacheRulesInvalidator at appropriate time. Thanks for the hint with the SecurityCacheRulesInvalidator. That could work, although "double caching" has a bit of a hacky taste. Another idea could be to allow multiple components to not just return the valid rights for a page, but also a maximum TTL for the decision made. In that way the caching could set the TTL appropriately to the value returned or the default TTL, whichever is smaller. Fabian will elaborate a bit more on some possible ideas as soon as he finds the time. Edo _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
