28 Mar
2012
28 Mar
'12
10:47 p.m.
2012-03-28 18:29, Denis Gervalle skrev:
On Wed, Mar 28, 2012 at 11:18, Vincent Massol
<vincent(a)massol.net> wrote:
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
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs