CreateRefControl: Pass CurrentUser to Reachable
In ReceiveCommits context CurrentUser can be anonymous.
When CreateRefControl calls Reachable, Reachable uses the
PermissionBackend to filter refs visible to the CurrentUser.
This causes PermissionBackend to evaluate the pushing user
as anonymous even if they are authenticated.
When pushing annotated tags over HTTP with only permission CREATE_TAG
and the ref that the tag is reachable isn't visible to anonymous users,
Reachable will conclude that the tag isn't reachable from anywhere.
This in turn makes CreateRefControl conclude that the user isn't
allowed to push the tag and throw an AuthException:
"update for creating new commit object not permitted"
This did not affect pushes over SSH since a RequestContext is created
(by SshScope) before ReceiveCommits is called so CurrentUser was
available.
By passing the authenticated user to Reachable and use it to filter
the refs, we can assure that authenticated users are allowed to
push annotated tags when they have permission to create them.
Rewritten tests for pushing tags:
* Test with both SSH and HTTP.
* Remove READ for anonymous users.
This effectively also removes all other permissions for Anonymous
users since all permissions are dependent on READ.
Bug: Issue 8952
Change-Id: Iaa3e3620ca46c9c5a0e43f9b948f9d057ca861bf
diff --git a/java/com/google/gerrit/server/project/CreateRefControl.java b/java/com/google/gerrit/server/project/CreateRefControl.java
index f89e298..485910e 100644
--- a/java/com/google/gerrit/server/project/CreateRefControl.java
+++ b/java/com/google/gerrit/server/project/CreateRefControl.java
@@ -78,7 +78,7 @@
PermissionBackend.ForRef perm = permissionBackend.user(user.get()).ref(branch);
if (object instanceof RevCommit) {
perm.check(RefPermission.CREATE);
- checkCreateCommit(repo, (RevCommit) object, ps.getNameKey(), perm);
+ checkCreateCommit(user, repo, (RevCommit) object, ps.getNameKey(), perm);
} else if (object instanceof RevTag) {
RevTag tag = (RevTag) object;
try (RevWalk rw = new RevWalk(repo)) {
@@ -99,7 +99,7 @@
RevObject target = tag.getObject();
if (target instanceof RevCommit) {
- checkCreateCommit(repo, (RevCommit) target, ps.getNameKey(), perm);
+ checkCreateCommit(user, repo, (RevCommit) target, ps.getNameKey(), perm);
} else {
checkCreateRef(user, repo, branch, target);
}
@@ -120,7 +120,11 @@
* new commit to the repository.
*/
private void checkCreateCommit(
- Repository repo, RevCommit commit, Project.NameKey project, PermissionBackend.ForRef forRef)
+ Provider<? extends CurrentUser> user,
+ Repository repo,
+ RevCommit commit,
+ Project.NameKey project,
+ PermissionBackend.ForRef forRef)
throws AuthException, PermissionBackendException {
try {
// If the user has update (push) permission, they can create the ref regardless
@@ -130,7 +134,7 @@
} catch (AuthException denied) {
// Fall through to check reachability.
}
- if (reachable.fromHeadsOrTags(project, repo, commit)) {
+ if (reachable.fromHeadsOrTags(project, repo, commit, user)) {
// If the user has no push permissions, check whether the object is
// merged into a branch or tag readable by this user. If so, they are
// not effectively "pushing" more objects, so they can create the ref
diff --git a/java/com/google/gerrit/server/project/Reachable.java b/java/com/google/gerrit/server/project/Reachable.java
index 63fda19..f85aa62 100644
--- a/java/com/google/gerrit/server/project/Reachable.java
+++ b/java/com/google/gerrit/server/project/Reachable.java
@@ -18,15 +18,18 @@
import com.google.common.collect.Maps;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.IncludedInResolver;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collection;
import java.util.Map;
+import java.util.Optional;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefDatabase;
@@ -52,10 +55,20 @@
/** @return true if a commit is reachable from a given set of refs. */
public boolean fromRefs(
Project.NameKey project, Repository repo, RevCommit commit, Map<String, Ref> refs) {
+ return fromRefs(project, repo, commit, refs, Optional.empty());
+ }
+
+ private boolean fromRefs(
+ Project.NameKey project,
+ Repository repo,
+ RevCommit commit,
+ Map<String, Ref> refs,
+ Optional<Provider<? extends CurrentUser>> userProvider) {
try (RevWalk rw = new RevWalk(repo)) {
Map<String, Ref> filtered =
- permissionBackend
- .currentUser()
+ userProvider
+ .map(up -> permissionBackend.user(up.get()))
+ .orElse(permissionBackend.currentUser())
.project(project)
.filter(refs, repo, RefFilterOptions.builder().setFilterTagsSeparately(true).build());
return IncludedInResolver.includedInAny(repo, rw, commit, filtered.values());
@@ -67,7 +80,11 @@
}
/** @return true if a commit is reachable from a repo's branches and tags. */
- boolean fromHeadsOrTags(Project.NameKey project, Repository repo, RevCommit commit) {
+ boolean fromHeadsOrTags(
+ Project.NameKey project,
+ Repository repo,
+ RevCommit commit,
+ Provider<? extends CurrentUser> userProvider) {
try {
RefDatabase refdb = repo.getRefDatabase();
Collection<Ref> heads = refdb.getRefsByPrefix(Constants.R_HEADS);
@@ -76,7 +93,7 @@
for (Ref r : Iterables.concat(heads, tags)) {
refs.put(r.getName(), r);
}
- return fromRefs(project, repo, commit, refs);
+ return fromRefs(project, repo, commit, refs, Optional.of(userProvider));
} catch (IOException e) {
logger.atSevere().withCause(e).log(
"Cannot verify permissions to commit object %s in repository %s", commit.name(), project);
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AbstractHttpPushTag.java b/javatests/com/google/gerrit/acceptance/rest/project/AbstractHttpPushTag.java
new file mode 100644
index 0000000..58ae63f
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AbstractHttpPushTag.java
@@ -0,0 +1,31 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.rest.project;
+
+import com.google.gerrit.acceptance.GitUtil;
+import org.eclipse.jgit.transport.CredentialsProvider;
+import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider;
+import org.junit.Before;
+
+public abstract class AbstractHttpPushTag extends AbstractPushTag {
+
+ @Before
+ public void cloneProjectOverHttp() throws Exception {
+ // clone with user to avoid inherited tag permissions of admin user
+ CredentialsProvider.setDefault(
+ new UsernamePasswordCredentialsProvider(user.username, user.httpPassword));
+ testRepo = GitUtil.cloneProject(project, user.getHttpUrl(server) + "/" + project.get());
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java b/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
index 95bc5a6..12468c3 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
@@ -21,14 +21,16 @@
import static com.google.gerrit.acceptance.GitUtil.updateAnnotatedTag;
import static com.google.gerrit.acceptance.rest.project.AbstractPushTag.TagType.ANNOTATED;
import static com.google.gerrit.acceptance.rest.project.AbstractPushTag.TagType.LIGHTWEIGHT;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.common.base.MoreObjects;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
-import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.project.testing.Util;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.PushResult;
@@ -37,7 +39,6 @@
import org.junit.Before;
import org.junit.Test;
-@NoHttpd
public abstract class AbstractPushTag extends AbstractDaemonTest {
enum TagType {
LIGHTWEIGHT(Permission.CREATE),
@@ -55,11 +56,9 @@
@Before
public void setUpTestEnvironment() throws Exception {
- // clone with user to avoid inherited tag permissions of admin user
- testRepo = cloneProject(project, user);
-
initialHead = getRemoteHead();
tagType = getTagType();
+ removeAnonymousRead();
}
protected abstract TagType getTagType();
@@ -251,6 +250,17 @@
removePermission(project, "refs/tags/*", Permission.PUSH);
}
+ private void removeAnonymousRead() throws Exception {
+ AccountGroup.UUID anonymous = systemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID();
+ AccountGroup.UUID registered = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+ String allRefs = RefNames.REFS + "*";
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ Util.block(u.getConfig(), Permission.READ, anonymous, allRefs);
+ Util.allow(u.getConfig(), Permission.READ, registered, allRefs);
+ u.save();
+ }
+ }
+
private void commit(PersonIdent ident, String subject) throws Exception {
commitBuilder().ident(ident).message(subject + " (" + System.nanoTime() + ")").create();
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AbstractSshPushTag.java b/javatests/com/google/gerrit/acceptance/rest/project/AbstractSshPushTag.java
new file mode 100644
index 0000000..35297ae
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AbstractSshPushTag.java
@@ -0,0 +1,29 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.rest.project;
+
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.UseSsh;
+import org.junit.Before;
+
+@NoHttpd
+@UseSsh
+public abstract class AbstractSshPushTag extends AbstractPushTag {
+ @Before
+ public void cloneProjectOverSsh() throws Exception {
+ // clone with user to avoid inherited tag permissions of admin user
+ testRepo = cloneProject(project, user);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/BUILD b/javatests/com/google/gerrit/acceptance/rest/project/BUILD
index e8a13c9..975a711 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/BUILD
+++ b/javatests/com/google/gerrit/acceptance/rest/project/BUILD
@@ -43,7 +43,9 @@
name = "push_tag_util",
testonly = True,
srcs = [
+ "AbstractHttpPushTag.java",
"AbstractPushTag.java",
+ "AbstractSshPushTag.java",
],
deps = [
"//java/com/google/gerrit/acceptance:lib",
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java b/javatests/com/google/gerrit/acceptance/rest/project/HttpPushAnnotatedTagIT.java
similarity index 85%
rename from javatests/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java
rename to javatests/com/google/gerrit/acceptance/rest/project/HttpPushAnnotatedTagIT.java
index 24c8ed0..bb08f41 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/HttpPushAnnotatedTagIT.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2017 The Android Open Source Project
+// Copyright (C) 2020 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
@@ -14,7 +14,7 @@
package com.google.gerrit.acceptance.rest.project;
-public class PushAnnotatedTagIT extends AbstractPushTag {
+public class HttpPushAnnotatedTagIT extends AbstractHttpPushTag {
@Override
protected TagType getTagType() {
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java b/javatests/com/google/gerrit/acceptance/rest/project/HttpPushLightweightTagIT.java
similarity index 85%
rename from javatests/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java
rename to javatests/com/google/gerrit/acceptance/rest/project/HttpPushLightweightTagIT.java
index 20d83a0..c89c332 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/HttpPushLightweightTagIT.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2017 The Android Open Source Project
+// Copyright (C) 2020 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
@@ -14,7 +14,7 @@
package com.google.gerrit.acceptance.rest.project;
-public class PushLightweightTagIT extends AbstractPushTag {
+public class HttpPushLightweightTagIT extends AbstractHttpPushTag {
@Override
protected TagType getTagType() {
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java b/javatests/com/google/gerrit/acceptance/rest/project/SshPushAnnotatedTagIT.java
similarity index 85%
copy from javatests/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java
copy to javatests/com/google/gerrit/acceptance/rest/project/SshPushAnnotatedTagIT.java
index 24c8ed0..df7636a 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/SshPushAnnotatedTagIT.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2017 The Android Open Source Project
+// Copyright (C) 2020 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
@@ -14,7 +14,7 @@
package com.google.gerrit.acceptance.rest.project;
-public class PushAnnotatedTagIT extends AbstractPushTag {
+public class SshPushAnnotatedTagIT extends AbstractSshPushTag {
@Override
protected TagType getTagType() {
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java b/javatests/com/google/gerrit/acceptance/rest/project/SshPushLightweightTagIT.java
similarity index 75%
copy from javatests/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java
copy to javatests/com/google/gerrit/acceptance/rest/project/SshPushLightweightTagIT.java
index 20d83a0..bcf7eb9 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/SshPushLightweightTagIT.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2017 The Android Open Source Project
+// Copyright (C) 2020 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
@@ -14,7 +14,12 @@
package com.google.gerrit.acceptance.rest.project;
-public class PushLightweightTagIT extends AbstractPushTag {
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.UseSsh;
+
+@NoHttpd
+@UseSsh
+public class SshPushLightweightTagIT extends AbstractSshPushTag {
@Override
protected TagType getTagType() {
