27 Aug
2013
27 Aug
'13
5:41 p.m.
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