Skip to content

Commit

Permalink
Handle NPE on getPermissionsForGroup (Fix #9567)
Browse files Browse the repository at this point in the history
"user" group permissions were not being loaded
properly leading to NPEs. Since modifying data
in the "user" group is not the regular case, a
EMPTY permissions should be a safe default.
  • Loading branch information
joshmoore committed Aug 31, 2012
1 parent dab3015 commit ab59240
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
<constructor-arg ref="systemTypes"/>
<constructor-arg ref="tokenHolder"/>
<constructor-arg ref="securityFilterHolder"/>
<constructor-arg ref="roles"/>
</bean>

<bean name="sessionCache" class="ome.services.sessions.state.SessionCache">
Expand Down
17 changes: 15 additions & 2 deletions components/server/src/ome/security/basic/BasicACLVoter.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import ome.security.SecuritySystem;
import ome.security.SystemTypes;
import ome.system.EventContext;
import ome.system.Roles;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
Expand Down Expand Up @@ -75,12 +76,20 @@ private static enum Scope {

protected final SecurityFilter securityFilter;

protected final Roles roles;

public BasicACLVoter(CurrentDetails cd, SystemTypes sysTypes,
TokenHolder tokenHolder, SecurityFilter securityFilter) {
TokenHolder tokenHolder, SecurityFilter securityFilter) {
this(cd, sysTypes, tokenHolder, securityFilter, new Roles());
}

public BasicACLVoter(CurrentDetails cd, SystemTypes sysTypes,
TokenHolder tokenHolder, SecurityFilter securityFilter, Roles roles) {
this.currentUser = cd;
this.sysTypes = sysTypes;
this.securityFilter = securityFilter;
this.tokenHolder = tokenHolder;
this.roles = roles;
}

// ~ Interface methods
Expand Down Expand Up @@ -355,7 +364,11 @@ public void postProcess(IObject object) {
final BasicEventContext c = currentUser.current();
Permissions grpPermissions = c.getCurrentGroupPermissions();
if (grpPermissions == Permissions.DUMMY && details.getGroup() != null) {
grpPermissions = c.getPermissionsForGroup(details.getGroup().getId());
Long gid = details.getGroup().getId();
grpPermissions = c.getPermissionsForGroup(gid);
if (grpPermissions == null && gid.equals(roles.getUserGroupId())) {
grpPermissions = new Permissions(Permissions.EMPTY);
}
}
final Permissions p = details.getPermissions();
final int allow = allowUpdateOrDelete(c, object, details, grpPermissions,
Expand Down
19 changes: 10 additions & 9 deletions components/server/src/ome/security/basic/CurrentDetails.java
Original file line number Diff line number Diff line change
Expand Up @@ -392,27 +392,28 @@ public void applyContext(Details details, boolean changePerms) {
if (changePerms) {
// Make the permissions match (#8277)
final Permissions groupPerms = c.getCurrentGroupPermissions();
Permissions copy = new Permissions(Permissions.EMPTY);
if (groupPerms != Permissions.DUMMY) {
copy = new Permissions(groupPerms);
details.setPermissions(new Permissions(groupPerms));
} else {
// In the case of the dummy, we will be required to have
// the group id already set in the context.
ExperimenterGroup group = details.getGroup();
if (group != null) {
// Systypes still will have DUMMY values.
Long gid = details.getGroup().getId();
// Ticket:9505. This must be a new copy of the permissions
// in order to prevent the restrictions being modified by
// later objects!
Permissions p = new Permissions(
c.getPermissionsForGroup(gid));
Permissions p = c.getPermissionsForGroup(gid);
if (p != null) {
copy = p;
// Ticket:9505. This must be a new copy of the permissions
// in order to prevent the restrictions being modified by
// later objects!
details.setPermissions(new Permissions(p));
} else if (gid.equals(Long.valueOf(roles.getUserGroupId()))) {
details.setPermissions(new Permissions(Permissions.EMPTY));
} else {
throw new InternalException("No permissions: " + details);
}
}
}
details.setPermissions(copy);
}

}
Expand Down

0 comments on commit ab59240

Please sign in to comment.