From 8fa42c6cc84c4d79dec5464ea11845d802a68ccd Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 11 May 2010 11:22:48 -0700 Subject: [PATCH] Cleanup redundant access control evaluation During a security audit of Gerrit, Julien Tinnes identified that this conditional is redundant with the else block, and can be removed. So drop it from the code to simplify the logic. Change-Id: I917f88e63ade3ebf0bc18575fc84f8d2885032e9 Suggested-by: Julien Tinnes Signed-off-by: Shawn O. Pearce --- .../gerrit/server/project/ProjectControl.java | 6 +----- .../google/gerrit/server/project/RefControl.java | 13 +------------ 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index d03a0e827b..705c3b160f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -157,11 +157,7 @@ public class ProjectControl { for (final RefRight pr : state.getLocalRights(actionId)) { if (groups.contains(pr.getAccountGroupId())) { - if (val < 0 && pr.getMaxValue() > 0) { - val = pr.getMaxValue(); - } else { - val = Math.max(pr.getMaxValue(), val); - } + val = Math.max(pr.getMaxValue(), val); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index 018f1a4e0d..973302c387 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -241,18 +241,7 @@ public class RefControl { for (RefRight right : filterMostSpecific(allRights)) { if (groups.contains(right.getAccountGroupId())) { - if (val < 0 && right.getMaxValue() > 0) { - // If one of the user's groups had denied them access, but - // this group grants them access, prefer the grant over - // the denial. We have to break the tie somehow and we - // prefer being "more open" to being "more closed". - // - val = right.getMaxValue(); - } else { - // Otherwise we use the largest value we can get. - // - val = Math.max(right.getMaxValue(), val); - } + val = Math.max(right.getMaxValue(), val); } } return val >= level;