Merge "Insert space and colon after comment link"
diff --git a/java/com/google/gerrit/server/account/AccountControl.java b/java/com/google/gerrit/server/account/AccountControl.java
index c80059b..ca63565 100644
--- a/java/com/google/gerrit/server/account/AccountControl.java
+++ b/java/com/google/gerrit/server/account/AccountControl.java
@@ -82,12 +82,12 @@
* accounts.
*/
@UsedAt(UsedAt.Project.PLUGIN_CODE_OWNERS)
- public AccountControl get(IdentifiedUser identifiedUser) {
+ public AccountControl get(CurrentUser user) {
return new AccountControl(
permissionBackend,
projectCache,
groupControlFactory,
- identifiedUser,
+ user,
userFactory,
accountVisibility);
}
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 65eb332..389b292 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -132,12 +132,18 @@
private final String input;
private final ImmutableList<AccountState> list;
private final ImmutableList<AccountState> filteredInactive;
+ private final CurrentUser searchedAsUser;
@VisibleForTesting
- Result(String input, List<AccountState> list, List<AccountState> filteredInactive) {
+ Result(
+ String input,
+ List<AccountState> list,
+ List<AccountState> filteredInactive,
+ CurrentUser searchedAsUser) {
this.input = requireNonNull(input);
this.list = canonicalize(list);
this.filteredInactive = canonicalize(filteredInactive);
+ this.searchedAsUser = requireNonNull(searchedAsUser);
}
private ImmutableList<AccountState> canonicalize(List<AccountState> list) {
@@ -180,13 +186,21 @@
}
}
- public IdentifiedUser asUniqueUser() throws UnresolvableAccountException {
+ private void ensureSelfIsUniqueIdentifiedUser() throws UnresolvableAccountException {
ensureUnique();
+ if (!searchedAsUser.isIdentifiedUser()) {
+ throw new UnresolvableAccountException(this);
+ }
+ }
+
+ public IdentifiedUser asUniqueUser() throws UnresolvableAccountException {
if (isSelf()) {
+ ensureSelfIsUniqueIdentifiedUser();
// In the special case of "self", use the exact IdentifiedUser from the request context, to
// preserve the peer address and any other per-request state.
- return self.get().asIdentifiedUser();
+ return searchedAsUser.asIdentifiedUser();
}
+ ensureUnique();
return userFactory.create(asUnique());
}
@@ -194,8 +208,7 @@
throws UnresolvableAccountException {
ensureUnique();
if (isSelf()) {
- // TODO(dborowitz): This preserves old behavior, but it seems wrong to discard the caller.
- return self.get().asIdentifiedUser();
+ return searchedAsUser.asIdentifiedUser();
}
return userFactory.runAs(
null, list.get(0).account().id(), requireNonNull(caller).getRealUser());
@@ -221,16 +234,57 @@
return false;
}
+ /**
+ * Searches can be done on behalf of either the current user or another provided user. The
+ * results of some searchers, such as BySelf, are affected by the context user.
+ */
+ default boolean requiresContextUser() {
+ return false;
+ }
+
Optional<I> tryParse(String input) throws IOException;
- Stream<AccountState> search(I input) throws IOException, ConfigInvalidException;
+ /**
+ * This method should be implemented for every searcher which doesn't require a context user.
+ *
+ * @param input to search for
+ * @return stream of the matching accounts
+ * @throws IOException by some subclasses
+ * @throws ConfigInvalidException by some subclasses
+ */
+ default Stream<AccountState> search(I input) throws IOException, ConfigInvalidException {
+ throw new IllegalStateException("search(I) default implementation should never be called.");
+ }
+
+ /**
+ * This method should be implemented for every searcher which requires a context user.
+ *
+ * @param input to search for
+ * @param asUser the context user for the search
+ * @return stream of the matching accounts
+ * @throws IOException by some subclasses
+ * @throws ConfigInvalidException by some subclasses
+ */
+ default Stream<AccountState> search(I input, CurrentUser asUser)
+ throws IOException, ConfigInvalidException {
+ if (!requiresContextUser()) {
+ return search(input);
+ }
+ throw new IllegalStateException(
+ "The searcher requires a context user, but doesn't implement search(input, asUser).");
+ }
boolean shortCircuitIfNoResults();
- default Optional<Stream<AccountState>> trySearch(String input)
+ default Optional<Stream<AccountState>> trySearch(String input, CurrentUser asUser)
throws IOException, ConfigInvalidException {
Optional<I> parsed = tryParse(input);
- return parsed.isPresent() ? Optional.of(search(parsed.get())) : Optional.empty();
+ if (parsed.isEmpty()) {
+ return Optional.empty();
+ }
+ return requiresContextUser()
+ ? Optional.of(search(parsed.get(), asUser))
+ : Optional.of(search(parsed.get()));
}
}
@@ -251,7 +305,7 @@
}
}
- private class BySelf extends StringSearcher {
+ private static class BySelf extends StringSearcher {
@Override
public boolean callerShouldFilterOutInactiveCandidates() {
return false;
@@ -263,17 +317,21 @@
}
@Override
+ public boolean requiresContextUser() {
+ return true;
+ }
+
+ @Override
protected boolean matches(String input) {
return "self".equals(input) || "me".equals(input);
}
@Override
- public Stream<AccountState> search(String input) {
- CurrentUser user = self.get();
- if (!user.isIdentifiedUser()) {
+ public Stream<AccountState> search(String input, CurrentUser asUser) {
+ if (!asUser.isIdentifiedUser()) {
return Stream.empty();
}
- return Stream.of(user.asIdentifiedUser().state());
+ return Stream.of(asUser.asIdentifiedUser().state());
}
@Override
@@ -400,9 +458,20 @@
}
private class ByFullName implements Searcher<AccountState> {
+ boolean allowSkippingVisibilityCheck = true;
+
+ ByFullName() {
+ super();
+ }
+
+ ByFullName(boolean allowSkippingVisibilityCheck) {
+ this();
+ this.allowSkippingVisibilityCheck = allowSkippingVisibilityCheck;
+ }
+
@Override
public boolean callerMayAssumeCandidatesAreVisible() {
- return true; // Rely on enforceVisibility from the index.
+ return allowSkippingVisibilityCheck;
}
@Override
@@ -424,9 +493,25 @@
}
private class ByDefaultSearch extends StringSearcher {
+ boolean allowSkippingVisibilityCheck = true;
+
+ ByDefaultSearch() {
+ super();
+ }
+
+ ByDefaultSearch(boolean allowSkippingVisibilityCheck) {
+ this();
+ this.allowSkippingVisibilityCheck = allowSkippingVisibilityCheck;
+ }
+
@Override
public boolean callerMayAssumeCandidatesAreVisible() {
- return true; // Rely on enforceVisibility from the index.
+ return allowSkippingVisibilityCheck;
+ }
+
+ @Override
+ public boolean requiresContextUser() {
+ return true;
}
@Override
@@ -435,14 +520,14 @@
}
@Override
- public Stream<AccountState> search(String input) {
+ public Stream<AccountState> search(String input, CurrentUser asUser) {
// At this point we have no clue. Just perform a whole bunch of suggestions and pray we come
// up with a reasonable result list.
// TODO(dborowitz): This doesn't match the documentation; consider whether it's possible to be
// more strict here.
boolean canSeeSecondaryEmails = false;
try {
- if (permissionBackend.user(self.get()).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
canSeeSecondaryEmails = true;
}
} catch (PermissionBackendException e) {
@@ -477,6 +562,18 @@
.addAll(nameOrEmailSearchers)
.build();
+ private final ImmutableList<Searcher<?>> forcedVisibilitySearchers =
+ ImmutableList.of(
+ new ByNameAndEmail(),
+ new ByEmail(),
+ new FromRealm(),
+ new ByFullName(false),
+ new ByDefaultSearch(false),
+ new BySelf(),
+ new ByExactAccountId(),
+ new ByParenthesizedAccountId(),
+ new ByUsername());
+
private final AccountCache accountCache;
private final AccountControl.Factory accountControlFactory;
private final Emails emails;
@@ -538,12 +635,63 @@
* @throws IOException if an error occurs.
*/
public Result resolve(String input) throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::canSeePredicate, AccountResolver::isActive);
+ return searchImpl(
+ input, searchers, self.get(), this::currentUserCanSeePredicate, AccountResolver::isActive);
}
public Result resolve(String input, Predicate<AccountState> accountActivityPredicate)
throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::canSeePredicate, accountActivityPredicate);
+ return searchImpl(
+ input, searchers, self.get(), this::currentUserCanSeePredicate, accountActivityPredicate);
+ }
+
+ /**
+ * Resolves all accounts matching the input string, visible to the provided user.
+ *
+ * <p>The following input formats are recognized:
+ *
+ * <ul>
+ * <li>The strings {@code "self"} and {@code "me"}, if the provided user is an {@link
+ * IdentifiedUser}. In this case, may return exactly one inactive account.
+ * <li>A bare account ID ({@code "18419"}). In this case, may return exactly one inactive
+ * account. This case short-circuits if the input matches.
+ * <li>An account ID in parentheses following a full name ({@code "Full Name (18419)"}). This
+ * case short-circuits if the input matches.
+ * <li>A username ({@code "username"}).
+ * <li>A full name and email address ({@code "Full Name <email@example>"}). This case
+ * short-circuits if the input matches.
+ * <li>An email address ({@code "email@example"}. This case short-circuits if the input matches.
+ * <li>An account name recognized by the configured {@link Realm#lookup(String)} Realm}.
+ * <li>A full name ({@code "Full Name"}).
+ * <li>As a fallback, a {@link
+ * com.google.gerrit.server.query.account.AccountPredicates#defaultPredicate(Schema,
+ * boolean, String) default search} against the account index.
+ * </ul>
+ *
+ * @param asUser user to resolve the users by.
+ * @param input input string.
+ * @param forceVisibilityCheck whether to force all searchers to check for visibility.
+ * @return a result describing matching accounts. Never null even if the result set is empty.
+ * @throws ConfigInvalidException if an error occurs.
+ * @throws IOException if an error occurs.
+ */
+ public Result resolveAsUser(CurrentUser asUser, String input, boolean forceVisibilityCheck)
+ throws ConfigInvalidException, IOException {
+ return resolveAsUser(asUser, input, AccountResolver::isActive, forceVisibilityCheck);
+ }
+
+ public Result resolveAsUser(
+ CurrentUser asUser,
+ String input,
+ Predicate<AccountState> accountActivityPredicate,
+ boolean forceVisibilityCheck)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(
+ input,
+ forceVisibilityCheck ? forcedVisibilitySearchers : searchers,
+ asUser,
+ new ProvidedUserCanSeePredicate(asUser),
+ accountActivityPredicate);
}
/**
@@ -556,22 +704,23 @@
* instead will be stored as a link to the corresponding Gerrit Account.
*/
public Result resolveIncludeInactive(String input) throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::canSeePredicate, AccountResolver::allVisible);
+ return searchImpl(
+ input,
+ searchers,
+ self.get(),
+ this::currentUserCanSeePredicate,
+ AccountResolver::allVisible);
}
public Result resolveIncludeInactiveIgnoreVisibility(String input)
throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::allVisible);
+ return searchImpl(
+ input, searchers, self.get(), this::allVisiblePredicate, AccountResolver::allVisible);
}
public Result resolveIgnoreVisibility(String input) throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::isActive);
- }
-
- public Result resolveIgnoreVisibility(
- String input, Predicate<AccountState> accountActivityPredicate)
- throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::allVisiblePredicate, accountActivityPredicate);
+ return searchImpl(
+ input, searchers, self.get(), this::allVisiblePredicate, AccountResolver::isActive);
}
/**
@@ -600,7 +749,11 @@
@Deprecated
public Result resolveByNameOrEmail(String input) throws ConfigInvalidException, IOException {
return searchImpl(
- input, nameOrEmailSearchers, this::canSeePredicate, AccountResolver::isActive);
+ input,
+ nameOrEmailSearchers,
+ self.get(),
+ this::currentUserCanSeePredicate,
+ AccountResolver::isActive);
}
/**
@@ -619,16 +772,26 @@
return searchImpl(
input,
ImmutableList.of(new ByNameAndEmail(), new ByEmail(), new ByFullName(), new ByUsername()),
- this::canSeePredicate,
+ self.get(),
+ this::currentUserCanSeePredicate,
AccountResolver::isActive);
}
- private Predicate<AccountState> canSeePredicate() {
- return this::canSee;
+ private Predicate<AccountState> currentUserCanSeePredicate() {
+ return accountControlFactory.get()::canSee;
}
- private boolean canSee(AccountState accountState) {
- return accountControlFactory.get().canSee(accountState);
+ private class ProvidedUserCanSeePredicate implements Supplier<Predicate<AccountState>> {
+ CurrentUser asUser;
+
+ ProvidedUserCanSeePredicate(CurrentUser asUser) {
+ this.asUser = asUser;
+ }
+
+ @Override
+ public Predicate<AccountState> get() {
+ return accountControlFactory.get(asUser.asIdentifiedUser())::canSee;
+ }
}
private Predicate<AccountState> allVisiblePredicate() {
@@ -648,14 +811,16 @@
Result searchImpl(
String input,
ImmutableList<Searcher<?>> searchers,
+ CurrentUser asUser,
Supplier<Predicate<AccountState>> visibilitySupplier,
Predicate<AccountState> accountActivityPredicate)
throws ConfigInvalidException, IOException {
+ requireNonNull(asUser);
visibilitySupplier = Suppliers.memoize(visibilitySupplier::get);
List<AccountState> inactive = new ArrayList<>();
for (Searcher<?> searcher : searchers) {
- Optional<Stream<AccountState>> maybeResults = searcher.trySearch(input);
+ Optional<Stream<AccountState>> maybeResults = searcher.trySearch(input, asUser);
if (!maybeResults.isPresent()) {
continue;
}
@@ -677,22 +842,25 @@
}
if (!list.isEmpty()) {
- return createResult(input, list);
+ return createResult(input, list, asUser);
}
if (searcher.shortCircuitIfNoResults()) {
// For a short-circuiting searcher, return results even if empty.
- return !inactive.isEmpty() ? emptyResult(input, inactive) : createResult(input, list);
+ return !inactive.isEmpty()
+ ? emptyResult(input, inactive, asUser)
+ : createResult(input, list, asUser);
}
}
- return emptyResult(input, inactive);
+ return emptyResult(input, inactive, asUser);
}
- private Result createResult(String input, List<AccountState> list) {
- return new Result(input, list, ImmutableList.of());
+ private Result createResult(String input, List<AccountState> list, CurrentUser searchedAsUser) {
+ return new Result(input, list, ImmutableList.of(), searchedAsUser);
}
- private Result emptyResult(String input, List<AccountState> inactive) {
- return new Result(input, ImmutableList.of(), inactive);
+ private Result emptyResult(
+ String input, List<AccountState> inactive, CurrentUser searchedAsUser) {
+ return new Result(input, ImmutableList.of(), inactive, searchedAsUser);
}
private Stream<AccountState> toAccountStates(Set<Account.Id> ids) {
diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java
index dcbd1ae..8acc925 100644
--- a/java/com/google/gerrit/server/change/RebaseUtil.java
+++ b/java/com/google/gerrit/server/change/RebaseUtil.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.change;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-
import com.google.auto.value.AutoValue;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
@@ -37,7 +35,6 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Inject;
@@ -70,30 +67,28 @@
this.rebaseFactory = rebaseFactory;
}
- public static void verifyRebasePreconditions(
- ProjectCache projectCache, PatchSetUtil patchSetUtil, RevWalk rw, RevisionResource rsrc)
- throws ResourceConflictException, IOException, AuthException, PermissionBackendException {
+ /**
+ * Checks whether the given change fulfills all preconditions to be rebased.
+ *
+ * <p>This method does not check whether the calling user is allowed to rebase the change.
+ */
+ public void verifyRebasePreconditions(RevWalk rw, ChangeNotes changeNotes, PatchSet patchSet)
+ throws ResourceConflictException, IOException {
// Not allowed to rebase if the current patch set is locked.
- patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes());
+ psUtil.checkPatchSetNotLocked(changeNotes);
- rsrc.permissions().check(ChangePermission.REBASE);
- projectCache
- .get(rsrc.getProject())
- .orElseThrow(illegalState(rsrc.getProject()))
- .checkStatePermitsWrite();
-
- if (!rsrc.getChange().isNew()) {
+ Change change = changeNotes.getChange();
+ if (!change.isNew()) {
throw new ResourceConflictException(
- String.format(
- "Change %s is %s", rsrc.getChange().getId(), ChangeUtil.status(rsrc.getChange())));
- } else if (!hasOneParent(rw, rsrc.getPatchSet())) {
+ String.format("Change %s is %s", change.getId(), ChangeUtil.status(change)));
+ }
+
+ if (!hasOneParent(rw, patchSet)) {
throw new ResourceConflictException(
String.format(
"Error rebasing %s. Cannot rebase %s",
- rsrc.getChange().getId(),
- countParents(rw, rsrc.getPatchSet()) > 1
- ? "merge commits"
- : "commit with no ancestor"));
+ change.getId(),
+ countParents(rw, patchSet) > 1 ? "merge commits" : "commit with no ancestor"));
}
}
diff --git a/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
index f4c211d..cbf47c5 100644
--- a/java/com/google/gerrit/server/mail/send/ProjectWatch.java
+++ b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
@@ -253,6 +253,7 @@
qb = WatcherChangeQueryBuilder.asUser(args.queryBuilder.get(), user);
p = qb.isVisible();
}
+ qb.forceAccountVisibilityCheck();
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 510ca01..ca18ab2 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -488,6 +488,7 @@
private final Arguments args;
protected Map<String, String> hasOperandAliases = Collections.emptyMap();
private Map<Account.Id, DestinationList> destinationListByAccount = new HashMap<>();
+ private boolean forceAccountVisibilityCheck = false;
private static final Splitter RULE_SPLITTER = Splitter.on("=");
private static final Splitter PLUGIN_SPLITTER = Splitter.on("_");
@@ -518,6 +519,11 @@
return args;
}
+ /** Whether to force account visibility check when searching for changes by account(s). */
+ public void forceAccountVisibilityCheck() {
+ forceAccountVisibilityCheck = true;
+ }
+
@Operator
public Predicate<ChangeData> age(String value) {
return new AgePredicate(value);
@@ -545,14 +551,14 @@
@Operator
public Predicate<ChangeData> mergedBefore(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
return new BeforePredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_BEFORE, value);
}
@Operator
public Predicate<ChangeData> mergedAfter(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
return new AfterPredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_AFTER, value);
}
@@ -635,7 +641,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
return new IsAttentionPredicate();
}
@@ -678,7 +684,7 @@
}
if ("uploader".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
return ChangePredicates.uploader(self());
}
@@ -701,7 +707,7 @@
}
if ("merge".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.MERGE_SPEC, "is:merge");
+ checkOperatorAvailable(ChangeField.MERGE_SPEC, "is:merge");
return new BooleanPredicate(ChangeField.MERGE_SPEC);
}
@@ -710,7 +716,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
return new IsAttentionPredicate();
}
@@ -723,7 +729,7 @@
}
if ("pure-revert".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
+ checkOperatorAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
return ChangePredicates.pureRevert("1");
}
@@ -739,12 +745,12 @@
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.NOT_READY)),
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.RULE_ERROR)));
}
- checkFieldAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
+ checkOperatorAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
return new IsSubmittablePredicate();
}
if ("started".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.STARTED_SPEC, "is:started");
+ checkOperatorAvailable(ChangeField.STARTED_SPEC, "is:started");
return new BooleanPredicate(ChangeField.STARTED_SPEC);
}
@@ -753,7 +759,7 @@
}
if ("cherrypick".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
return new BooleanPredicate(ChangeField.CHERRY_PICK_SPEC);
}
@@ -888,7 +894,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
+ checkOperatorAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
return ChangePredicates.fuzzyHashtag(hashtag);
}
@@ -898,7 +904,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
+ checkOperatorAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
return ChangePredicates.prefixHashtag(hashtag);
}
@@ -924,7 +930,7 @@
return ChangePredicates.exactTopic(name);
}
- checkFieldAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
+ checkOperatorAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
return ChangePredicates.prefixTopic(name);
}
@@ -990,7 +996,7 @@
@Operator
public Predicate<ChangeData> hasfooter(String footerName) throws QueryParseException {
- checkFieldAvailable(ChangeField.FOOTER_NAME, "hasfooter");
+ checkOperatorAvailable(ChangeField.FOOTER_NAME, "hasfooter");
return ChangePredicates.hasFooter(footerName);
}
@@ -1141,7 +1147,9 @@
@Operator
public Predicate<ChangeData> message(String text) throws QueryParseException {
if (text.startsWith("^")) {
- checkFieldAvailable(ChangeField.COMMIT_MESSAGE_EXACT, "messageexact");
+ checkFieldAvailable(
+ ChangeField.COMMIT_MESSAGE_EXACT,
+ "'message' operator with regular expression is not supported on this gerrit host");
return new RegexMessagePredicate(text);
}
return ChangePredicates.message(text);
@@ -1149,13 +1157,14 @@
@Operator
public Predicate<ChangeData> subject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
+ checkOperatorAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
return ChangePredicates.subject(value);
}
@Operator
public Predicate<ChangeData> prefixsubject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
+ checkOperatorAvailable(
+ ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
return ChangePredicates.prefixSubject(value);
}
@@ -1243,7 +1252,7 @@
@Operator
public Predicate<ChangeData> uploader(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploader");
return uploader(parseAccount(who, (AccountState s) -> true));
}
@@ -1258,7 +1267,7 @@
@Operator
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
return attention(parseAccount(who, (AccountState s) -> true));
}
@@ -1303,7 +1312,7 @@
@Operator
public Predicate<ChangeData> uploaderin(String group) throws QueryParseException, IOException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
if (g == null) {
@@ -1569,8 +1578,8 @@
@Operator
public Predicate<ChangeData> cherryPickOf(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
if (Ints.tryParse(value) != null) {
return ChangePredicates.cherryPickOf(Change.id(Ints.tryParse(value)));
}
@@ -1642,11 +1651,16 @@
return Predicate.or(predicates);
}
- protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String operator)
+ private void checkOperatorAvailable(SchemaField<ChangeData, ?> field, String operator)
+ throws QueryParseException {
+ checkFieldAvailable(
+ field, String.format("'%s' operator is not supported on this gerrit host", operator));
+ }
+
+ protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String errorMessage)
throws QueryParseException {
if (!args.index.getSchema().hasField(field)) {
- throw new QueryParseException(
- String.format("'%s' operator is not supported on this gerrit host", operator));
+ throw new QueryParseException(errorMessage);
}
}
@@ -1697,7 +1711,9 @@
private Set<Account.Id> parseAccount(String who)
throws QueryParseException, IOException, ConfigInvalidException {
try {
- return args.accountResolver.resolve(who).asNonEmptyIdSet();
+ return args.accountResolver
+ .resolveAsUser(args.getUser(), who, forceAccountVisibilityCheck)
+ .asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
@@ -1710,7 +1726,9 @@
String who, java.util.function.Predicate<AccountState> activityFilter)
throws QueryParseException, IOException, ConfigInvalidException {
try {
- return args.accountResolver.resolve(who, activityFilter).asNonEmptyIdSet();
+ return args.accountResolver
+ .resolveAsUser(args.getUser(), who, activityFilter, forceAccountVisibilityCheck)
+ .asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 1a8f07a..8a8d2ca 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -87,6 +87,13 @@
@Override
public Response<ChangeInfo> apply(RevisionResource rsrc, RebaseInput input)
throws UpdateException, RestApiException, IOException, PermissionBackendException {
+ rsrc.permissions().check(ChangePermission.REBASE);
+
+ projectCache
+ .get(rsrc.getProject())
+ .orElseThrow(illegalState(rsrc.getProject()))
+ .checkStatePermitsWrite();
+
Change change = rsrc.getChange();
try (Repository repo = repoManager.openRepository(change.getProject());
ObjectInserter oi = repo.newObjectInserter();
@@ -94,7 +101,7 @@
RevWalk rw = CodeReviewCommit.newRevWalk(reader);
BatchUpdate bu =
updateFactory.create(change.getProject(), rsrc.getUser(), TimeUtil.now())) {
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, rsrc);
+ rebaseUtil.verifyRebasePreconditions(rw, rsrc.getNotes(), rsrc.getPatchSet());
RebaseChangeOp rebaseOp =
rebaseUtil.getRebaseOp(
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 4754c69..786bba7 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -113,7 +113,11 @@
@Override
public Response<RebaseChainInfo> apply(ChangeResource tipRsrc, RebaseInput input)
throws IOException, PermissionBackendException, RestApiException, UpdateException {
+ tipRsrc.permissions().check(ChangePermission.REBASE);
+
Project.NameKey project = tipRsrc.getProject();
+ projectCache.get(project).orElseThrow(illegalState(project)).checkStatePermitsWrite();
+
CurrentUser user = tipRsrc.getUser();
List<Change.Id> upToDateAncestors = new ArrayList<>();
@@ -136,7 +140,8 @@
RevisionResource revRsrc =
new RevisionResource(changeResourceFactory.create(changeData, user), ps);
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, revRsrc);
+ revRsrc.permissions().check(ChangePermission.REBASE);
+ rebaseUtil.verifyRebasePreconditions(rw, changeData.notes(), ps);
boolean isUpToDate = false;
RebaseChangeOp rebaseOp = null;
diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
index 16617b4..cf1eee0 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;
@@ -400,6 +401,111 @@
}
@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 watchChangesCommentedBySelf() throws Exception {
+ String watchedProject = projectOperations.newProject().create().get();
+ requestScopeOperations.setApiUser(user.id());
+
+ // user watches all changes that have a comment by themselves
+ watch(watchedProject, "commentby:self");
+
+ // pushing a change as admin should not trigger an email to user
+ 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();
+ assertThat(sender.getMessages()).isEmpty();
+
+ // commenting by admin should not trigger an email to user
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.message = "A Comment";
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+ assertThat(sender.getMessages()).isEmpty();
+
+ // commenting by user matches the project watch, but doesn't send an email to user because
+ // CC_ON_OWN_COMMENTS is false by default, so the user is removed from the TO list, but an email
+ // is sent to the admin user
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ Message m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(admin.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
+
+ // commenting by admin now triggers an email to user because the change has a comment by user
+ // and hence matches the project watch
+ requestScopeOperations.setApiUser(admin.id());
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+ messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ 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
public void watchAllProjects() throws Exception {
String anyProject = projectOperations.newProject().create().get();
requestScopeOperations.setApiUser(user.id());
diff --git a/javatests/com/google/gerrit/server/account/AccountResolverTest.java b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
index 3658834..37728f7 100644
--- a/javatests/com/google/gerrit/server/account/AccountResolverTest.java
+++ b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
@@ -23,6 +23,8 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountResolver.Result;
import com.google.gerrit.server.account.AccountResolver.Searcher;
import com.google.gerrit.server.account.AccountResolver.StringSearcher;
@@ -35,6 +37,8 @@
import org.junit.Test;
public class AccountResolverTest {
+ private final CurrentUser user = new AnonymousUser();
+
private static class TestSearcher extends StringSearcher {
private final String pattern;
private final boolean shortCircuit;
@@ -282,7 +286,7 @@
AccountResolver resolver = newAccountResolver();
assertThat(
new UnresolvableAccountException(
- resolver.new Result("foo", ImmutableList.of(), ImmutableList.of())))
+ resolver.new Result("foo", ImmutableList.of(), ImmutableList.of(), user)))
.hasMessageThat()
.isEqualTo("Account 'foo' not found");
}
@@ -292,7 +296,7 @@
AccountResolver resolver = newAccountResolver();
UnresolvableAccountException e =
new UnresolvableAccountException(
- resolver.new Result("self", ImmutableList.of(), ImmutableList.of()));
+ resolver.new Result("self", ImmutableList.of(), ImmutableList.of(), user));
assertThat(e.isSelf()).isTrue();
assertThat(e).hasMessageThat().isEqualTo("Resolving account 'self' requires login");
}
@@ -302,7 +306,7 @@
AccountResolver resolver = newAccountResolver();
UnresolvableAccountException e =
new UnresolvableAccountException(
- resolver.new Result("me", ImmutableList.of(), ImmutableList.of()));
+ resolver.new Result("me", ImmutableList.of(), ImmutableList.of(), user));
assertThat(e.isSelf()).isTrue();
assertThat(e).hasMessageThat().isEqualTo("Resolving account 'me' requires login");
}
@@ -314,7 +318,10 @@
new UnresolvableAccountException(
resolver
.new Result(
- "foo", ImmutableList.of(newAccount(3), newAccount(1)), ImmutableList.of())))
+ "foo",
+ ImmutableList.of(newAccount(3), newAccount(1)),
+ ImmutableList.of(),
+ user)))
.hasMessageThat()
.isEqualTo(
"Account 'foo' is ambiguous (at most 3 shown):\n1: Anonymous Name (1)\n3: Anonymous Name (3)");
@@ -329,7 +336,8 @@
.new Result(
"foo",
ImmutableList.of(),
- ImmutableList.of(newInactiveAccount(3), newInactiveAccount(1)))))
+ ImmutableList.of(newInactiveAccount(3), newInactiveAccount(1)),
+ user)))
.hasMessageThat()
.isEqualTo(
"Account 'foo' only matches inactive accounts. To use an inactive account, retry"
@@ -352,10 +360,11 @@
Supplier<Predicate<AccountState>> visibilitySupplier,
Predicate<AccountState> activityPredicate)
throws Exception {
- return newAccountResolver().searchImpl(input, searchers, visibilitySupplier, activityPredicate);
+ return newAccountResolver()
+ .searchImpl(input, searchers, user, visibilitySupplier, activityPredicate);
}
- private static AccountResolver newAccountResolver() {
+ private AccountResolver newAccountResolver() {
return new AccountResolver(null, null, null, null, null, null, null, null, "Anonymous Name");
}
