Fix project-watchers visibility checks.
Previous check: $currentUser can see $matchedAccount
New check: $watcher can see $matchedAccount
When-
- $matchedAccount is the account matching the watched term.
- $currentUser is the user who triggered the API call, e.g. the change
uploader.
Change-Id: If383ffb2a6b0b28c1e54effe22f0a4aae386f8cc
Release-Notes: skip
Bug: Google b/262363435
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 65eb332..46183ad 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -26,6 +26,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.index.Schema;
@@ -402,7 +403,7 @@
private class ByFullName implements Searcher<AccountState> {
@Override
public boolean callerMayAssumeCandidatesAreVisible() {
- return true; // Rely on enforceVisibility from the index.
+ return allowSkippingVisibilityCheck;
}
@Override
@@ -426,7 +427,7 @@
private class ByDefaultSearch extends StringSearcher {
@Override
public boolean callerMayAssumeCandidatesAreVisible() {
- return true; // Rely on enforceVisibility from the index.
+ return allowSkippingVisibilityCheck;
}
@Override
@@ -486,6 +487,8 @@
private final Realm realm;
private final String anonymousCowardName;
private final PermissionBackend permissionBackend;
+ private @Nullable IdentifiedUser asUser;
+ private boolean allowSkippingVisibilityCheck = true;
@Inject
AccountResolver(
@@ -510,6 +513,24 @@
}
/**
+ * Set an account to resolve the users by. If not set, {@link CurrentUser} is used.
+ *
+ * <p>This affects visibility checks.
+ *
+ * @param asUser the user to resolve the other users with.
+ * @return this
+ */
+ public AccountResolver resolveAsUser(IdentifiedUser asUser) {
+ this.asUser = asUser;
+ return this;
+ }
+
+ public AccountResolver forceVisibilityCheck(boolean forceVisibilityCheck) {
+ allowSkippingVisibilityCheck = !forceVisibilityCheck;
+ return this;
+ }
+
+ /**
* Resolves all accounts matching the input string, visible to the current user.
*
* <p>The following input formats are recognized:
@@ -628,6 +649,9 @@
}
private boolean canSee(AccountState accountState) {
+ if (asUser != null) {
+ return accountControlFactory.get(asUser).canSee(accountState);
+ }
return accountControlFactory.get().canSee(accountState);
}
diff --git a/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
index 9299d74..c0db41a 100644
--- a/java/com/google/gerrit/server/mail/send/ProjectWatch.java
+++ b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
@@ -249,6 +249,7 @@
qb = args.queryBuilder.get().asUser(user);
p = qb.isVisible();
}
+ qb.forceVisibilityCheck();
if (filter != null) {
Predicate<ChangeData> filterPredicate = qb.parse(filter);
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index fe4cb5d..a882cb8 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -486,6 +486,7 @@
private final Arguments args;
protected Map<String, String> hasOperandAliases = Collections.emptyMap();
private Map<Account.Id, DestinationList> destinationListByAccount = new HashMap<>();
+ private boolean forceVisibilityCheck = false;
private static final Splitter RULE_SPLITTER = Splitter.on("=");
private static final Splitter PLUGIN_SPLITTER = Splitter.on("_");
@@ -512,6 +513,10 @@
return new ChangeQueryBuilder(builderDef, args.asUser(user));
}
+ public void forceVisibilityCheck() {
+ forceVisibilityCheck = true;
+ }
+
@Operator
public Predicate<ChangeData> age(String value) {
return new AgePredicate(value);
@@ -1677,7 +1682,11 @@
private Set<Account.Id> parseAccount(String who)
throws QueryParseException, IOException, ConfigInvalidException {
try {
- return args.accountResolver.resolve(who).asNonEmptyIdSet();
+ return args.accountResolver
+ .resolveAsUser(args.getUser().asIdentifiedUser())
+ .forceVisibilityCheck(forceVisibilityCheck)
+ .resolve(who)
+ .asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
@@ -1690,6 +1699,13 @@
String who, java.util.function.Predicate<AccountState> activityFilter)
throws QueryParseException, IOException, ConfigInvalidException {
try {
+ if (args.getUser().isIdentifiedUser()) {
+ return args.accountResolver
+ .resolveAsUser(args.getUser().asIdentifiedUser())
+ .forceVisibilityCheck(forceVisibilityCheck)
+ .resolve(who, activityFilter)
+ .asNonEmptyIdSet();
+ }
return args.accountResolver.resolve(who, activityFilter).asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
index 1900158..1ce0ce3 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.AccountGroup;
@@ -348,6 +349,60 @@
}
@Test
+ public void watchOwner() throws Exception {
+ String watchedProject = projectOperations.newProject().create().get();
+ requestScopeOperations.setApiUser(user.id());
+
+ // watch keyword in project as user
+ watch(watchedProject, "owner:admin");
+
+ // push a change with keyword -> should trigger email notification
+ requestScopeOperations.setApiUser(admin.id());
+ TestRepository<InMemoryRepository> watchedRepo =
+ cloneProject(Project.nameKey(watchedProject), admin);
+ PushOneCommit.Result r =
+ pushFactory
+ .create(admin.newIdent(), watchedRepo, "subject", "a.txt", "a1")
+ .to("refs/for/master");
+ r.assertOkStatus();
+
+ // assert email notification for user
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ Message m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(user.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void watchNonVisibleOwner() throws Exception {
+ String watchedProject = projectOperations.newProject().create().get();
+ requestScopeOperations.setApiUser(user.id());
+
+ // watch keyword in project as user
+ watch(watchedProject, "owner:admin");
+
+ // Verify that 'user' can't see 'admin'
+ assertThatAccountIsNotVisible(admin);
+
+ // push a change with keyword -> should trigger email notification
+ requestScopeOperations.setApiUser(admin.id());
+ TestRepository<InMemoryRepository> watchedRepo =
+ cloneProject(Project.nameKey(watchedProject), admin);
+ PushOneCommit.Result r =
+ pushFactory
+ .create(admin.newIdent(), watchedRepo, "subject", "a.txt", "a1")
+ .to("refs/for/master");
+ r.assertOkStatus();
+
+ // assert no email notifications for user
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
+ @Test
public void watchAllProjects() throws Exception {
String anyProject = projectOperations.newProject().create().get();
requestScopeOperations.setApiUser(user.id());
