Merge changes I5aaf20ee,Ib9312e97
* changes:
Resolve "self" even if inactive
Add query test that resolving "self" fails for anonymous users
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index 8078922..c326b66 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -2153,8 +2153,8 @@
link:config-gerrit.txt#accounts.visibility[visible] to the calling user are not
considered.
-In all cases _except_ a bare account ID, inactive accounts are not considered.
-Inactive accounts may only be referenced by bare ID.
+In all cases _except_ a bare account ID and `self`/`me`, inactive accounts are
+not considered. Inactive accounts should only be referenced by bare ID.
If the input is a bare account ID, this will always resolve to exactly
one account if there is a visible account with that ID, and zero accounts
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 3dea6a9..9b4952b 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -68,7 +68,7 @@
* </ol>
*
* <p>The result never includes accounts that are not visible to the calling user. It also never
- * includes inactive accounts, with one specific exception noted in method Javadoc.
+ * includes inactive accounts, with a small number of specific exceptions noted in method Javadoc.
*/
@Singleton
public class AccountResolver {
@@ -252,6 +252,11 @@
private class BySelf extends StringSearcher {
@Override
+ public boolean callerShouldFilterOutInactiveCandidates() {
+ return false;
+ }
+
+ @Override
public boolean callerMayAssumeCandidatesAreVisible() {
return true;
}
@@ -279,8 +284,6 @@
private class ByExactAccountId extends AccountIdSearcher {
@Override
public boolean callerShouldFilterOutInactiveCandidates() {
- // The only case where we *don't* enforce that the account is active is when passing an exact
- // numeric account ID.
return false;
}
@@ -497,9 +500,9 @@
*
* <ul>
* <li>The strings {@code "self"} and {@code "me"}, if the current user is an {@link
- * IdentifiedUser}.
- * <li>A bare account ID ({@code "18419"}). In this case, and <strong>only</strong> this case,
- * may return exactly one inactive account. This case short-circuits if the input matches.
+ * 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"}).
diff --git a/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java b/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java
index 63fda12..d99fa72 100644
--- a/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java
@@ -105,6 +105,19 @@
}
@Test
+ public void bySelfInactive() throws Exception {
+ gApi.accounts().id(user.id.get()).setActive(false);
+
+ requestScopeOperations.setApiUser(user.id);
+ assertThat(gApi.accounts().id("self").getActive()).isFalse();
+
+ Result result = resolveAsResult("self");
+ assertThat(result.asIdSet()).containsExactly(user.id);
+ assertThat(result.isSelf()).isTrue();
+ assertThat(result.asUniqueUser()).isSameAs(self.get());
+ }
+
+ @Test
public void byExactAccountId() throws Exception {
Account.Id existingId = accountOperations.newAccount().create();
Account.Id idWithExistingIdAsFullname =
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 524b399..da62b7c 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -64,6 +64,7 @@
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.index.FieldDef;
@@ -3095,6 +3096,44 @@
assertQuery("project:repo+foo", change);
}
+ @Test
+ public void selfFailsForAnonymousUser() throws Exception {
+ for (String query : ImmutableList.of("assignee:self", "starredby:self", "is:starred")) {
+ assertQuery(query);
+ RequestContext oldContext = requestContext.setContext(anonymousUserProvider::get);
+
+ try {
+ requestContext.setContext(anonymousUserProvider::get);
+ assertThatAuthException(query)
+ .hasMessageThat()
+ .isEqualTo("Must be signed-in to use this operator");
+ } finally {
+ requestContext.setContext(oldContext);
+ }
+ }
+ }
+
+ @Test
+ public void selfSucceedsForInactiveAccount() throws Exception {
+ Account.Id user2 =
+ accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId();
+
+ TestRepository<Repo> repo = createProject("repo");
+ Change change = insert(repo, newChange(repo));
+ AssigneeInput ain = new AssigneeInput();
+ ain.assignee = user2.toString();
+ gApi.changes().id(change.getId().get()).setAssignee(ain);
+
+ RequestContext adminContext = requestContext.setContext(newRequestContext(user2));
+ assertQuery("assignee:self", change);
+
+ requestContext.setContext(adminContext);
+ gApi.accounts().id(user2.get()).setActive(false);
+
+ requestContext.setContext(newRequestContext(user2));
+ assertQuery("assignee:self", change);
+ }
+
protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
return newChange(repo, null, null, null, null, false);
}
@@ -3224,6 +3263,15 @@
}
}
+ protected ThrowableSubject assertThatAuthException(Object query) throws Exception {
+ try {
+ newQuery(query).get();
+ throw new AssertionError("expected AuthException for query: " + query);
+ } catch (AuthException e) {
+ return assertThat(e);
+ }
+ }
+
protected TestRepository<Repo> createProject(String name) throws Exception {
gApi.projects().create(name).get();
return new TestRepository<>(repoManager.openRepository(new Project.NameKey(name)));
