Resolve Project Owners when checking access right on any ref

A project is visible to a user when the user has Read Access on any
ref of the project. This check whether an access right is granted on
any ref is not taking the access rights granted to the
'Project Owners' group into account. As a result a user does not see
a project if he is project owner and Read Access is only granted to
the 'Project Owners' group. This change ensures that the
'Project Owners' group is properly resolved in this case.

Bug: issue 997
Change-Id: I27cd8293e5c4a01c867a4e076073bf587294e0ba
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>

2 files changed, 27 insertions, 5 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 25778a6fb8..2a55019761 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
@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.Branch;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.Project;
 import com.google.gerrit.reviewdb.RefRight;
+import com.google.gerrit.reviewdb.SystemConfig;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.ReplicationUser;
 import com.google.gerrit.server.config.GitReceivePackGroups;
@@ -29,6 +30,7 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.assistedinject.Assisted;
 
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
 
@@ -101,6 +103,7 @@ public class ProjectControl {
     ProjectControl create(CurrentUser who, ProjectState ps);
   }
 
+  private final SystemConfig systemConfig;
   private final Set<AccountGroup.Id> uploadGroups;
   private final Set<AccountGroup.Id> receiveGroups;
 
@@ -109,10 +112,12 @@ public class ProjectControl {
   private final ProjectState state;
 
   @Inject
-  ProjectControl(@GitUploadPackGroups Set<AccountGroup.Id> uploadGroups,
+  ProjectControl(final SystemConfig systemConfig,
+      @GitUploadPackGroups Set<AccountGroup.Id> uploadGroups,
       @GitReceivePackGroups Set<AccountGroup.Id> receiveGroups,
       final RefControl.Factory refControlFactory,
       @Assisted CurrentUser who, @Assisted ProjectState ps) {
+    this.systemConfig = systemConfig;
     this.uploadGroups = uploadGroups;
     this.receiveGroups = receiveGroups;
     this.refControlFactory = refControlFactory;
@@ -197,7 +202,7 @@ public class ProjectControl {
   // TODO (anatol.pomazau): Try to merge this method with similar RefRightsForPattern#canPerform
   private boolean canPerformOnAnyRef(ApprovalCategory.Id actionId,
       short requireValue) {
-    final Set<AccountGroup.Id> groups = user.getEffectiveGroups();
+    final Set<AccountGroup.Id> groups = getEffectiveUserGroups();
 
     for (final RefRight pr : state.getAllRights(actionId, true)) {
       if (groups.contains(pr.getAccountGroupId())
@@ -209,6 +214,22 @@ public class ProjectControl {
     return false;
   }
 
+  /**
+   * @return the effective groups of the current user for this project
+   */
+  private Set<AccountGroup.Id> getEffectiveUserGroups() {
+    final Set<AccountGroup.Id> userGroups = user.getEffectiveGroups();
+    if (isOwner()) {
+      final Set<AccountGroup.Id> userGroupsOnProject =
+          new HashSet<AccountGroup.Id>(userGroups.size() + 1);
+      userGroupsOnProject.addAll(userGroups);
+      userGroupsOnProject.add(systemConfig.ownerGroupId);
+      return Collections.unmodifiableSet(userGroupsOnProject);
+    } else {
+      return userGroups;
+    }
+  }
+
   private boolean canPerformOnAllRefs(ApprovalCategory.Id actionId,
       short requireValue) {
     boolean canPerform = false;
@@ -238,10 +259,10 @@ public class ProjectControl {
   }
 
   public boolean canRunUploadPack() {
-    return isAnyIncludedIn(uploadGroups, user.getEffectiveGroups());
+    return isAnyIncludedIn(uploadGroups, getEffectiveUserGroups());
   }
 
   public boolean canRunReceivePack() {
-    return isAnyIncludedIn(receiveGroups, user.getEffectiveGroups());
+    return isAnyIncludedIn(receiveGroups, getEffectiveUserGroups());
   }
 }
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
index cf5d2643ed..6b0f13a62e 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
@@ -277,7 +277,8 @@ public class RefControlTest extends TestCase {
         return new RefControl(systemConfig, projectControl, ref);
       }
     };
-    return new ProjectControl(Collections.<AccountGroup.Id> emptySet(),
+    return new ProjectControl(systemConfig,
+        Collections.<AccountGroup.Id> emptySet(),
         Collections.<AccountGroup.Id> emptySet(), refControlFactory,
         new MockUser(memberOf), newProjectState());
   }