portals-jetspeed-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Jencks (JIRA)" <jetspeed-...@portals.apache.org>
Subject [jira] Updated: (JS2-475) Proposed changes in portal permissions
Date Sun, 29 Jan 2006 20:59:05 GMT
     [ http://issues.apache.org/jira/browse/JS2-475?page=all ]

David Jencks updated JS2-475:
-----------------------------

    Attachment: JS2-475-additional.diff

Thanks for comitting the important parts of the patch.  I think there are a few places that
could use additional cleanup, see this new patch.  Most of these remove unused code, but there
is also one switch to using a mask rather than a string in a permissions constructor argument.

I've tried to minimize whitespace changes in this patch.

> Proposed changes in portal permissions
> --------------------------------------
>
>          Key: JS2-475
>          URL: http://issues.apache.org/jira/browse/JS2-475
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks
>     Assignee: David Le Strat
>      Fix For: 2.1-dev
>  Attachments: JS2-475-additional.diff, permissions-fragment1.diff, permissions-fragment3.jar,
permissions1.diff, permissions1.jar
>
> (from email, with added comments)
> I've been looking at the portal permissions and how they are used and think a few things
can be simplified and speeded up.  If there are no objections to this general direction I
will prepare an initial patch.
> 1. FolderPermission duplicates the parseActions method from PortalResourcePermission,
and in fact calls it's copy again.  I think this can be eliminated.
> 2. PortalResourcePermission.parseActions seems to have some rather odd code:
>                 if (token.equals(JetspeedActions.VIEW))
>                     mask |= JetspeedActions.MASK_VIEW;
>                 else if (token.equals(JetspeedActions.VIEW) || token.equals(JetspeedActions.RESTORE))
>                     mask |= JetspeedActions.MASK_VIEW;
> I think this can be simplified.
> 3. I may not have found all the constructor uses, but I think that subject should be
removed from all the portal permissions.  I haven't found any uses of the constructor including
a non-null subject (although I might have missed some).  In addition to the resulting simplification,
I believe the subject has no place in the permissions.  The JACC defined permissions for web
and ejb do not include a subject.  JACC does allow for unchecked permissions, which are difficult
to imagine if the permissions involved may include a subject.  I think a generally more satisfactory
approach is to rely on the policy implementation to determine the subject itself.
> --This requires converting several direct uses of the PermissionManager to AccessController.checkPermission.
 This is more generatlly consistent with use of Policy to check permissions.
> 4. Currently each construction of a  portal permission involves string parsing to decipher
an actions string.  It looks to me as if this can occur hundreds of times for a medium sized
portal page.  Futhermore, this action string appears to be constructed using ad-hoc string
manipulations in AbstractBaseElement.checkPermissions(String actions).  Similarly, the constraints
implementation seems to do an enormous amount of string comparison to match actions.  I think
that this can be entirely converted to integer masks with bitwise operations.  I'd propose
to do this in steps, starting with the permissions and working backwards until I hit the contraints
implementation, then converting it.
> -- initial patch completely converts permissions to use mask for runtime evaluations.
 Constraints remain as before.
> 5. Some of the constants are duplicated between SecuredResource and JetspeedActions:
moved to JetspeedActions only.
> 6. There are lots of little bugs like wrongly implemented equals methods in portal permissions
> 7. I've fixed the javadoc in the classes in the patch.
> 8. This includes the fixes for JS2-472
> I don't really know how to test my changes thoroughly.  AFAICT they appear to work with
my geronimo/jacc integration (latest version will be posted soon).
> I apologize for the number of white space changes in the diff.  I pressed the "reformat"
button in idea.  The patch is generated with svk and I have sometimes had troubles applying
these with patch so I'm also attaching copies of the source files.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org


Mime
View raw message