Ensure that only administrators can change the global capabilities
Only Gerrit server administrators (members of the groups that have
the 'administrateServer' capability) should be able to edit the
global capabilities because being able to edit the global capabilities
means being able to assign the 'administrateServer' capability.
Because of this we disallow on the AllProjects project to assign
1. the 'owner' access rights on 'refs/*'
Project owners (members of groups to which the 'owner' access right
is assigned) are able to edit the access control list of the projects
they own. Hence being owner of the AllProjects project would allow to
edit the global capabilities and assign the 'administrateServer'
capabilitiy without being Gerrit administrator.
In earlier Gerrit versions (2.1.x) it was already implemented like
this but the corresponding checks got lost.
2. the 'push' access right on 'refs/meta/config'
Being able to push configuration changes to the AllProjects project
allows to edit the global capabilities and hence a user with this
access right could assign the 'administrateServer' capability without
being Gerrit administrator.
This change ensures that from the Gerrit WebUI (ProjectAccessScreen) it
is not possible to assign on the AllProjects project the 'owner' access
right on 'refs/*' and the 'push' access right on 'refs/meta/config'.
In addition this change ensures that an 'owner' access right that is
assigned for 'refs/*' on the AllProjects project has no effect and that
only Gerrit administrators with the 'push' access right can push
configuration changes to the AllProjects project.
It is still possible to assign both access rights ('owner' on 'refs/*'
and 'push' on 'refs/meta/config') on the AllProjects project by directly
editing its 'project.config' file and pushing to 'refs/meta/config'.
To fix this it would be needed to reject assigning these access rights
on the AllProjects project as invalid configuration, however doing this
would mean to break existing configurations of the AllProjects project
that assign these access rights. At the moment there is no migration
framework in place that would allow to migrate 'project.config' files.
Hence this check is currently not done and these access rights in this
case have simply no effect.
Change-Id: Icbee947742a0bc0cf0a26cad0df3e37ad2713be4
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java
index 4067349..60bf19c 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java
@@ -75,6 +75,16 @@
return LABEL + labelName;
}
+ public static boolean canBeOnAllProjects(String ref, String permissionName) {
+ if (AccessSection.ALL.equals(ref)) {
+ return !OWNER.equals(permissionName);
+ }
+ if (AccessSection.REF_CONFIG.equals(ref)) {
+ return !PUSH.equals(permissionName);
+ }
+ return true;
+ }
+
protected String name;
protected boolean exclusiveGroup;
protected List<PermissionRule> rules;
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java
index 490378e..a398c36 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java
@@ -21,6 +21,9 @@
/** Pattern that matches all branches in a project. */
public static final String HEADS = "refs/heads/*";
+ /** Configuration settings for a project {@code refs/meta/config} */
+ public static final String REF_CONFIG = "refs/meta/config";
+
/** Prefix that triggers a regular expression pattern. */
public static final String REGEX_PREFIX = "^";
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java
index 93739c2..7481ba7 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java
@@ -223,22 +223,16 @@
if (AccessSection.GLOBAL_CAPABILITIES.equals(value.getName())) {
for (String varName : Util.C.capabilityNames().keySet()) {
- if (value.getPermission(varName) == null) {
- perms.add(varName);
- }
+ addPermission(varName, perms);
}
} else if (RefConfigSection.isValid(value.getName())) {
for (ApprovalType t : Gerrit.getConfig().getApprovalTypes()
.getApprovalTypes()) {
String varName = Permission.LABEL + t.getCategory().getLabelName();
- if (value.getPermission(varName) == null) {
- perms.add(varName);
- }
+ addPermission(varName, perms);
}
for (String varName : Util.C.permissionNames().keySet()) {
- if (value.getPermission(varName) == null) {
- perms.add(varName);
- }
+ addPermission(varName, perms);
}
}
if (perms.isEmpty()) {
@@ -251,6 +245,19 @@
}
}
+ private void addPermission(final String permissionName,
+ final List<String> permissionList) {
+ if (value.getPermission(permissionName) != null) {
+ return;
+ }
+ if (Gerrit.getConfig().getWildProject()
+ .equals(projectAccess.getProjectName())
+ && !Permission.canBeOnAllProjects(value.getName(), permissionName)) {
+ return;
+ }
+ permissionList.add(permissionName);
+ }
+
@Override
public void flush() {
List<Permission> src = permissions.getList();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
index 7de1fc1..a74577f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.project;
+import com.google.common.collect.Lists;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.Permission;
@@ -95,20 +96,24 @@
? new CapabilityCollection(config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES))
: null;
- HashSet<AccountGroup.UUID> groups = new HashSet<AccountGroup.UUID>();
- AccessSection all = config.getAccessSection(AccessSection.ALL);
- if (all != null) {
- Permission owner = all.getPermission(Permission.OWNER);
- if (owner != null) {
- for (PermissionRule rule : owner.getRules()) {
- GroupReference ref = rule.getGroup();
- if (ref.getUUID() != null) {
- groups.add(ref.getUUID());
+ if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) {
+ localOwners = Collections.emptySet();
+ } else {
+ HashSet<AccountGroup.UUID> groups = new HashSet<AccountGroup.UUID>();
+ AccessSection all = config.getAccessSection(AccessSection.ALL);
+ if (all != null) {
+ Permission owner = all.getPermission(Permission.OWNER);
+ if (owner != null) {
+ for (PermissionRule rule : owner.getRules()) {
+ GroupReference ref = rule.getGroup();
+ if (ref.getUUID() != null) {
+ groups.add(ref.getUUID());
+ }
}
}
}
+ localOwners = Collections.unmodifiableSet(groups);
}
- localOwners = Collections.unmodifiableSet(groups);
}
boolean needsRefresh(long generation) {
@@ -175,6 +180,18 @@
Collection<AccessSection> fromConfig = config.getAccessSections();
sm = new ArrayList<SectionMatcher>(fromConfig.size());
for (AccessSection section : fromConfig) {
+ if (isAllProjects) {
+ List<Permission> copy =
+ Lists.newArrayListWithCapacity(section.getPermissions().size());
+ for (Permission p : section.getPermissions()) {
+ if (Permission.canBeOnAllProjects(section.getName(), p.getName())) {
+ copy.add(p);
+ }
+ }
+ section = new AccessSection(section.getName());
+ section.setPermissions(copy);
+ }
+
SectionMatcher matcher = SectionMatcher.wrap(section);
if (matcher != null) {
sm.add(matcher);
@@ -271,4 +288,8 @@
}
return projectCache.get(getProject().getParent(allProjectsName));
}
+
+ public boolean isAllProjects() {
+ return isAllProjects;
+ }
}
\ No newline at end of file
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 2f99271..a6182d1 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
@@ -155,7 +155,14 @@
// rules. Allowing this to be done by a non-project-owner opens
// a security hole enabling editing of access rules, and thus
// granting of powers beyond pushing to the configuration.
- return false;
+
+ // On the AllProjects project the owner access right cannot be assigned,
+ // this why for the AllProjects project we allow administrators to push
+ // configuration changes if they have push without being project owner.
+ if (!(projectControl.getProjectState().isAllProjects() &&
+ getCurrentUser().getCapabilities().canAdministrateServer())) {
+ return false;
+ }
}
return canPerform(Permission.PUSH)
&& canWrite();
