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. > * 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 > -- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
