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).
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. * Missing javadocs in lots of places * Missing @since tags * 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. 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
