2012-03-28 18:29, Denis Gervalle skrev: > On Wed, Mar 28, 2012 at 11:18, Vincent Massol <[email protected]> wrote: > >> Actually I've now looked at the merge and I've made some quick review in >> GitHub (I've only done this very quickly nothing thorough). >> > > Have you read my initial mail about this merge proposal ? > The branch was there for a while, and I see that merging is the best way to > have you look at it :) > > It's a good start but there are quite a few things to improve: >> >> * Test are a bit of a mess: very long methods (should have a lot more test >> methods), some bad practices in several places, creating static mocks >> instead of dynamic mocks. > > > The tests in the bridge are bad, those came directly from the initial > contribution of Andreas, these are not properly mocked, neither properly > structured. I have kept them until we have better.
Writing proper tests have been on my TODO-list since june 2010. Now when it is finally on-track to be included as the default implementation, I'll be happy to write some tests. Best Regards, /Andreas >> * Missing javadocs in lots of places > > > This is unfair, this happen mostly in tests. I will try to improve it > anyway. > > >> * Missing @since tags >> > > Not easy to put @since until we know when we merge, will be improved. Would > be nice not have to do that manually :) > > >> * We also need to stabilize/comment on the API method names themselves. >> I've commented on a few. >> >> I'd like to know what's the test coverage percentage you have? We need at >> least 80 to 90% (if not more) for this type of code IMO since it's very >> sensitive. >> > > I have written the test in the api module. Those in the api module are > properly mocked and covers the most critical part (right, settler, data > structure, cache) at almost 100%, and the whole api module at 60% > currently. I see the api module as the most solid one, and the bridge as a > temporary one, until we go further, but I fully agree that we need to > improve tests in both. Your help is welcome :) > > >> >> Thanks >> -Vincent >> >> On Mar 28, 2012, at 10:33 AM, Vincent Massol wrote: >> >>> +1 if you lead the effort to making this implementation the default in >> the future. >>> >>> I'd like to know what are the next steps though and their timeframe. >>> >>> Denis, do you plan to refactor some existing code to use this new API? >> IMO this would be the best next step to prove that it works. Fo you have >> any idea about what code could be migrated? >>> >>> We need to be careful not to keep this new security module not used for >> too long as otherwise it could "rot". So IMO we need to start using it ASAP >> and then have a plan for switching to it. >>> >>> Thanks >>> -Vincent >>> >>> On Feb 15, 2012, at 11:17 AM, Denis Gervalle wrote: >>> >>>> Hi all, >>>> >>>> As you have probably notice, I have recently committed an >>>> feature-security-authorization branch on platform. I am working on this >> for >>>> a while now and it was the first step to share the outcome of this large >>>> refactoring of the initial work done early last year by Andreas. Since >> the >>>> code was not quality compliant with platform but the general structure >>>> Andreas has build seems to me well appropriate, I have progressively >>>> refactor its code to better fit our real needs. Here is what I have been >>>> done: >>>> >>>> 1) Split in to module api and bridge to allow breaking the currently >>>> unavoidable dependency on oldcore. Now only bridge depends on oldcore, >> and >>>> the api does not depends on bridge. As mush as possible has been >> written in >>>> the api (still some code to migrate), and some temporary internal bridge >>>> are used to access oldcore stuffs since augmenting the existing >>>> document-bridge does not seems appropriate IMO. >>>> >>>> 2) The initial enumeration of rights as been replaced by a Right class, >>>> which could be seen has a pseudo enum, but could be augmented with new >>>> rights. To register a new Right, you have to provide a RightDescription >> to >>>> the AuthorizationManager. The description will define the default state, >>>> the tie resolution policy, the inheritance policy, the list of entity >> types >>>> for which the right is applicable, the implied rights and if the right >>>> could be allowed in read-only mode. So new defined Right will benefit >> the >>>> whole logic of the AuthorizationManager and currently existing one >> could be >>>> declaratively defined. >>>> >>>> 3) Large renaming to better distinguish stuffs, clarify comments and >>>> prepare for future. I have voluntarily not taken existing names to >> clearly >>>> split the old and the new api. In brief, the new right service is now >> named >>>> AuthorizationManager. Internally, it manipulates SecurityReference (as >> well >>>> as UserSecurityReference and GroupSecurityReference, to represent >> entities, >>>> user and group), SecurityRule (representing a right object) and >>>> SecurityAccess (representing an access level in the old nomenclature), >>>> which are store in a SecurityCache using SecurityRuleEntry (a set of >> rules) >>>> and SecurityAccessEntry (the access of a given user). The >>>> AuthorizationManager delegate cache management to a SecurityCacheLoader >>>> which loads rules using a SecurityRuleLoader ; and delegate itself the >>>> computing of the access for a given user and a set of rules to an >>>> AuthorizationSettler. This last one could be overridden to provide >> specific >>>> decision that could not be done in declarative mode. >>>> >>>> 4) Refactoring was necessary to improve consistency and reduce >> complexity, >>>> and simplify as much as possible; while extending the limitations to >> allow >>>> more rights to be registered. This work has been a little bit opposed to >>>> the optimization done by Andreas, in particular on memory usage. But >>>> optimization is often the enemy of clean code. >>>> >>>> 5) Improvement were necessary to better mimic the existing >> implementation >>>> in some peculiar but necessary rules to stay compatible with current >>>> working wiki. I tend to reduce as much as possible what is not done >>>> declaratively, but there are still some special cases, like delete for >>>> creators, deny for other user on explicit allow and admin for wiki owner >>>> that are settle by the authorization settler. My implementation should >> be >>>> almost compatible with the old one, except for groups that are currently >>>> not checked from the entity wiki, but only from the user wiki. This >> needs >>>> some more refactoring for which I feel inconfortable with, some I'd >> like to >>>> share first. >>>> >>>> 6) The AuthorizationManager interface has been simplified, providing 2 >>>> methods for either checking or verifying an access right (the checking >>>> methods throws while the verifying one return a boolean), and one to >>>> register a new right. >>>> >>>> The existing RightService could be bridged on the new implementation >> using >>>> the XWikiCachingRightService class in xwiki.cfg and the new API could be >>>> used side-by-side with the old implementation as well. What should still >>>> really need to be improved is the unit testing, currently some tests are >>>> still awful and incomplete. I already refactor some of them, to provide >> a >>>> better coverage of essential part of the code: the security cache and >> the >>>> default authorization settler. Obviously, any help is welcomed. >>>> >>>> Since I already have an existing wiki using this implementation >>>> successfully and using it for creating new rights for extensions, I >> would >>>> like to merge this new implementation as experimental in platform to >> have >>>> it available for anyone who need it or want to test it, and for you to >> use >>>> in your new experimental development as well. Providing it in platform >> will >>>> encourage it to be finalized and replace the existing implementation. >>>> >>>> Here is my +1 for the merge on 4.x, >>>> >>>> WDYT ? >> >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs >> > > > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
