RequestScopeOperations: Implement additional setter methods
The implementations of scope setting methods in AbstractDaemonTest now
delegate directly to RequestScopeOperations, and can be inlined in a
future change.
Change-Id: I50a4096cb51a671ce5be89be32334b0b0094b350
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 699b2f9..46672d3 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -40,6 +40,7 @@
import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
import com.google.gerrit.acceptance.testsuite.account.TestSshKeys;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GroupDescription;
@@ -82,7 +83,6 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
@@ -277,17 +277,17 @@
protected boolean testRequiresSsh;
protected BlockStrategy noSleepBlockStrategy = t -> {}; // Don't sleep in tests.
- @Inject private ChangeIndexCollection changeIndexes;
- @Inject private AccountIndexCollection accountIndexes;
- @Inject private ProjectIndexCollection projectIndexes;
- @Inject private EventRecorder.Factory eventRecorderFactory;
- @Inject private InProcessProtocol inProcessProtocol;
- @Inject private Provider<AnonymousUser> anonymousUser;
- @Inject private AccountIndexer accountIndexer;
- @Inject private Groups groups;
- @Inject private GroupIndexer groupIndexer;
@Inject private AbstractChangeNotes.Args changeNotesArgs;
+ @Inject private AccountIndexCollection accountIndexes;
+ @Inject private AccountIndexer accountIndexer;
+ @Inject private ChangeIndexCollection changeIndexes;
+ @Inject private EventRecorder.Factory eventRecorderFactory;
+ @Inject private GroupIndexer groupIndexer;
+ @Inject private Groups groups;
+ @Inject private InProcessProtocol inProcessProtocol;
+ @Inject private ProjectIndexCollection projectIndexes;
@Inject private ProjectOperations projectOperations;
+ @Inject private RequestScopeOperations requestScopeOperations;
private ProjectResetter resetter;
private List<Repository> toClose;
@@ -785,8 +785,8 @@
}
private Context newRequestContext(TestAccount account) {
- return atrScope.newContext(
- new SshSession(sshKeys, server, account), identifiedUserFactory.create(account.getId()));
+ requestScopeOperations.setApiUser(account.getId());
+ return atrScope.get();
}
/**
@@ -796,15 +796,15 @@
* reloaded (e.g. the email addresses of the user).
*/
protected Context resetCurrentApiUser() {
- return atrScope.set(newRequestContext(atrScope.get().getSession().getAccount()));
+ return requestScopeOperations.resetCurrentApiUser();
}
protected Context setApiUser(TestAccount account) {
- return atrScope.set(newRequestContext(account));
+ return requestScopeOperations.setApiUser(account.getId());
}
protected Context setApiUserAnonymous() {
- return atrScope.set(atrScope.newContext(null, anonymousUser.get()));
+ return requestScopeOperations.setApiUserAnonymous();
}
protected Account getAccount(Account.Id accountId) {
diff --git a/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java b/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java
index 5abb0c0..8420ff2 100644
--- a/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java
+++ b/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java
@@ -55,7 +55,7 @@
finished = p.finished;
}
- SshSession getSession() {
+ public SshSession getSession() {
return session;
}
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index e7017b8..f829842 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
import static java.util.Objects.requireNonNull;
import static org.apache.log4j.Logger.getLogger;
@@ -50,10 +51,14 @@
import com.google.gerrit.testing.InMemoryRepositoryManager;
import com.google.gerrit.testing.SshMode;
import com.google.inject.AbstractModule;
+import com.google.inject.BindingAnnotation;
import com.google.inject.Injector;
import com.google.inject.Key;
import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Singleton;
import java.lang.annotation.Annotation;
+import java.lang.annotation.Retention;
import java.lang.reflect.Field;
import java.net.InetAddress;
import java.net.InetSocketAddress;
@@ -89,6 +94,11 @@
}
}
+ /** Marker on {@link InetSocketAddress} for test SSH server. */
+ @Retention(RUNTIME)
+ @BindingAnnotation
+ public @interface TestSshServerAddress {}
+
@AutoValue
public abstract static class Description {
public static Description forTestClass(
@@ -511,6 +521,18 @@
install(new AsyncReceiveCommits.Module());
factory(ProjectResetter.Builder.Factory.class);
}
+
+ @Provides
+ @Singleton
+ @Nullable
+ @TestSshServerAddress
+ InetSocketAddress getSshAddress(@GerritServerConfig Config cfg) {
+ String addr = cfg.getString("sshd", null, "listenAddress");
+ // We do not use InitSshd.isOff to avoid coupling GerritServer to the SSH code.
+ return !"off".equalsIgnoreCase(addr)
+ ? SocketUtil.resolve(cfg.getString("sshd", null, "listenAddress"), 0)
+ : null;
+ }
};
return sysInjector.createChildInjector(module);
}
@@ -535,7 +557,6 @@
private ExecutorService daemonService;
private Injector testInjector;
private String url;
- private InetSocketAddress sshdAddress;
private InetSocketAddress httpAddress;
private GerritServer(
@@ -553,12 +574,6 @@
Config cfg = testInjector.getInstance(Key.get(Config.class, GerritServerConfig.class));
url = cfg.getString("gerrit", null, "canonicalWebUrl");
URI uri = URI.create(url);
-
- String addr = cfg.getString("sshd", null, "listenAddress");
- // We do not use InitSshd.isOff to avoid coupling GerritServer to the SSH code.
- if (!"off".equalsIgnoreCase(addr)) {
- sshdAddress = SocketUtil.resolve(cfg.getString("sshd", null, "listenAddress"), 0);
- }
httpAddress = new InetSocketAddress(uri.getHost(), uri.getPort());
}
@@ -566,10 +581,6 @@
return url;
}
- InetSocketAddress getSshdAddress() {
- return sshdAddress;
- }
-
InetSocketAddress getHttpAddress() {
return httpAddress;
}
diff --git a/java/com/google/gerrit/acceptance/SshSession.java b/java/com/google/gerrit/acceptance/SshSession.java
index 52d7f28..fa0bc90 100644
--- a/java/com/google/gerrit/acceptance/SshSession.java
+++ b/java/com/google/gerrit/acceptance/SshSession.java
@@ -19,6 +19,7 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.gerrit.acceptance.testsuite.account.TestAccount;
import com.google.gerrit.acceptance.testsuite.account.TestSshKeys;
import com.jcraft.jsch.ChannelExec;
import com.jcraft.jsch.JSch;
@@ -35,9 +36,9 @@
private Session session;
private String error;
- public SshSession(TestSshKeys sshKeys, GerritServer server, TestAccount account) {
+ public SshSession(TestSshKeys sshKeys, InetSocketAddress addr, TestAccount account) {
this.sshKeys = sshKeys;
- this.addr = server.getSshdAddress();
+ this.addr = addr;
this.account = account;
}
@@ -112,8 +113,14 @@
JSch jsch = new JSch();
jsch.addIdentity(
"KeyPair", TestSshKeys.privateKey(keyPair), keyPair.getPublicKeyBlob(), null);
- session =
- jsch.getSession(account.username, addr.getAddress().getHostAddress(), addr.getPort());
+ String username =
+ account
+ .username()
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ "account " + account.accountId() + " must have a username to use SSH"));
+ session = jsch.getSession(username, addr.getAddress().getHostAddress(), addr.getPort());
session.setConfig("StrictHostKeyChecking", "no");
session.connect();
}
diff --git a/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperations.java b/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperations.java
index 578b895..a63d28a 100644
--- a/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperations.java
+++ b/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperations.java
@@ -14,8 +14,6 @@
package com.google.gerrit.acceptance.testsuite.request;
-import static java.util.Objects.requireNonNull;
-
import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
import com.google.gerrit.acceptance.testsuite.account.TestAccount;
import com.google.gerrit.reviewdb.client.Account;
@@ -29,7 +27,10 @@
/**
* Sets the Guice request scope to the given account.
*
- * <p>The resulting scope has no SSH session attached.
+ * <p>The resulting context has an SSH session attached. In order to use the SSH session returned
+ * by {@link AcceptanceTestRequestScope.Context#getSession()}, SSH must be enabled in the test and
+ * the account must have a username set. However, these are not requirements simply to call this
+ * method.
*
* @param accountId account ID. Must exist; throws an unchecked exception otherwise.
* @return the previous request scope.
@@ -39,12 +40,32 @@
/**
* Sets the Guice request scope to the given account.
*
- * <p>The resulting scope has no SSH session attached.
+ * <p>The resulting context has an SSH session attached. In order to use the SSH session returned
+ * by {@link AcceptanceTestRequestScope.Context#getSession()}, SSH must be enabled in the test and
+ * the account must have a username set. However, these are not requirements simply to call this
+ * method.
*
* @param testAccount test account from {@code AccountOperations}.
* @return the previous request scope.
*/
- default AcceptanceTestRequestScope.Context setApiUser(TestAccount testAccount) {
- return setApiUser(requireNonNull(testAccount).accountId());
- }
+ AcceptanceTestRequestScope.Context setApiUser(TestAccount testAccount);
+
+ /**
+ * Enforces a new request context for the current API user.
+ *
+ * <p>This recreates the {@code IdentifiedUser}, hence everything which is cached in the {@code
+ * IdentifiedUser} is reloaded (e.g. the email addresses of the user).
+ *
+ * <p>The current user must be an identified user.
+ *
+ * @return the previous request scope.
+ */
+ AcceptanceTestRequestScope.Context resetCurrentApiUser();
+
+ /**
+ * Sets the Guice request scope to the anonymous user.
+ *
+ * @return the previous request scope.
+ */
+ AcceptanceTestRequestScope.Context setApiUserAnonymous();
}
diff --git a/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImpl.java
index d60700a..27b71b9 100644
--- a/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImpl.java
@@ -14,16 +14,26 @@
package com.google.gerrit.acceptance.testsuite.request;
+import static com.google.common.base.Preconditions.checkState;
import static java.util.Objects.requireNonNull;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
-import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
+import com.google.gerrit.acceptance.GerritServer.TestSshServerAddress;
+import com.google.gerrit.acceptance.SshSession;
+import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.account.TestAccount;
+import com.google.gerrit.acceptance.testsuite.account.TestSshKeys;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.IdentifiedUser.GenericFactory;
import com.google.gerrit.server.account.AccountCache;
-import com.google.gerrit.server.account.AccountState;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.net.InetSocketAddress;
/**
* The implementation of {@code RequestScopeOperations}.
@@ -35,25 +45,61 @@
public class RequestScopeOperationsImpl implements RequestScopeOperations {
private final AcceptanceTestRequestScope atrScope;
private final AccountCache accountCache;
+ private final AccountOperations accountOperations;
private final IdentifiedUser.GenericFactory userFactory;
+ private final Provider<AnonymousUser> anonymousUserProvider;
+ private final InetSocketAddress sshAddress;
+ private final TestSshKeys testSshKeys;
@Inject
RequestScopeOperationsImpl(
AcceptanceTestRequestScope atrScope,
AccountCache accountCache,
- IdentifiedUser.GenericFactory userFactory) {
+ AccountOperations accountOperations,
+ GenericFactory userFactory,
+ Provider<AnonymousUser> anonymousUserProvider,
+ @Nullable @TestSshServerAddress InetSocketAddress sshAddress,
+ TestSshKeys testSshKeys) {
this.atrScope = atrScope;
this.accountCache = accountCache;
+ this.accountOperations = accountOperations;
this.userFactory = userFactory;
+ this.anonymousUserProvider = anonymousUserProvider;
+ this.sshAddress = sshAddress;
+ this.testSshKeys = testSshKeys;
}
@Override
- public Context setApiUser(Account.Id accountId) {
- AccountState accountState =
+ public AcceptanceTestRequestScope.Context setApiUser(Account.Id accountId) {
+ return setApiUser(accountOperations.account(accountId).get());
+ }
+
+ @Override
+ public AcceptanceTestRequestScope.Context setApiUser(TestAccount testAccount) {
+ return atrScope.set(
+ atrScope.newContext(
+ new SshSession(testSshKeys, sshAddress, testAccount),
+ createIdentifiedUser(testAccount.accountId())));
+ }
+
+ @Override
+ public AcceptanceTestRequestScope.Context resetCurrentApiUser() {
+ CurrentUser user = atrScope.get().getUser();
+ // More special cases for anonymous users etc. can be added as needed.
+ checkState(user.isIdentifiedUser(), "can only reset IdentifiedUser, not %s", user);
+ return setApiUser(user.getAccountId());
+ }
+
+ @Override
+ public AcceptanceTestRequestScope.Context setApiUserAnonymous() {
+ return atrScope.set(atrScope.newContext(null, anonymousUserProvider.get()));
+ }
+
+ private IdentifiedUser createIdentifiedUser(Account.Id accountId) {
+ return userFactory.create(
accountCache
.get(requireNonNull(accountId))
.orElseThrow(
- () -> new IllegalArgumentException("account does not exist: " + accountId));
- return atrScope.set(atrScope.newContext(null, userFactory.create(accountState)));
+ () -> new IllegalArgumentException("account does not exist: " + accountId)));
}
}
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java
index a22b57d..9b498011 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java
@@ -16,63 +16,100 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
+import static com.google.common.truth.Truth8.assertThat;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
+import com.google.gerrit.acceptance.UseSsh;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.account.TestAccount;
+import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.CurrentUser.PropertyKey;
import com.google.gerrit.server.Sequences;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import org.junit.Test;
+@UseSsh
public class RequestScopeOperationsImplTest extends AbstractDaemonTest {
+ private static final AtomicInteger changeCounter = new AtomicInteger();
+
@Inject private AccountOperations accountOperations;
- @Inject private Provider<IdentifiedUser> userProvider;
+ @Inject private Provider<CurrentUser> userProvider;
@Inject private RequestScopeOperationsImpl requestScopeOperations;
@Inject private Sequences sequences;
@Test
public void setApiUserToExistingUserById() throws Exception {
- checkApiUser(admin.getId());
+ fastCheckCurrentUser(admin.getId());
AcceptanceTestRequestScope.Context oldCtx = requestScopeOperations.setApiUser(user.getId());
assertThat(oldCtx.getUser().getAccountId()).isEqualTo(admin.getId());
- checkApiUser(user.getId());
+ checkCurrentUser(user.getId());
}
@Test
public void setApiUserToExistingUserByTestAccount() throws Exception {
- checkApiUser(admin.getId());
+ fastCheckCurrentUser(admin.getId());
TestAccount testAccount =
- accountOperations.account(accountOperations.newAccount().create()).get();
+ accountOperations.account(accountOperations.newAccount().username("tester").create()).get();
AcceptanceTestRequestScope.Context oldCtx = requestScopeOperations.setApiUser(testAccount);
assertThat(oldCtx.getUser().getAccountId()).isEqualTo(admin.getId());
- checkApiUser(testAccount.accountId());
+ checkCurrentUser(testAccount.accountId());
}
@Test
public void setApiUserToNonExistingUser() throws Exception {
- checkApiUser(admin.getId());
+ fastCheckCurrentUser(admin.getId());
try {
requestScopeOperations.setApiUser(new Account.Id(sequences.nextAccountId()));
assert_().fail("expected RuntimeException");
} catch (RuntimeException e) {
// Expected.
}
- checkApiUser(admin.getId());
+ checkCurrentUser(admin.getId());
}
- private void checkApiUser(Account.Id expected) throws Exception {
- // Test all supported ways that an acceptance test might query the active user.
- assertThat(gApi.accounts().self().get()._accountId)
- .named("user from GerritApi")
- .isEqualTo(expected.get());
+ @Test
+ public void resetCurrentApiUserClearsCachedState() throws Exception {
+ requestScopeOperations.setApiUser(user.getId());
+ PropertyKey<String> key = PropertyKey.create();
+ atrScope.get().getUser().put(key, "foo");
+ assertThat(atrScope.get().getUser().get(key)).hasValue("foo");
+
+ AcceptanceTestRequestScope.Context oldCtx = requestScopeOperations.resetCurrentApiUser();
+ checkCurrentUser(user.getId());
+ assertThat(atrScope.get().getUser().get(key)).isEmpty();
+ assertThat(oldCtx.getUser().get(key)).hasValue("foo");
+ }
+
+ @Test
+ public void setApiUserAnonymousSetsAnonymousUser() throws Exception {
+ fastCheckCurrentUser(admin.getId());
+ requestScopeOperations.setApiUserAnonymous();
+ assertThat(userProvider.get()).isInstanceOf(AnonymousUser.class);
+ }
+
+ private void fastCheckCurrentUser(Account.Id expected) {
+ // Check current user quickly, since the full check requires creating changes and is quite slow.
assertThat(userProvider.get().isIdentifiedUser())
.named("user from provider is an IdentifiedUser")
.isTrue();
assertThat(userProvider.get().getAccountId()).named("user from provider").isEqualTo(expected);
+ }
+
+ private void checkCurrentUser(Account.Id expected) throws Exception {
+ // Test all supported ways that an acceptance test might query the active user.
+ fastCheckCurrentUser(expected);
+ assertThat(gApi.accounts().self().get()._accountId)
+ .named("user from GerritApi")
+ .isEqualTo(expected.get());
AcceptanceTestRequestScope.Context ctx = atrScope.get();
assertThat(ctx.getUser().isIdentifiedUser())
.named("user from AcceptanceTestRequestScope.Context is an IdentifiedUser")
@@ -80,5 +117,31 @@
assertThat(ctx.getUser().getAccountId())
.named("user from AcceptanceTestRequestScope.Context")
.isEqualTo(expected);
+ checkSshUser(expected);
+ }
+
+ private void checkSshUser(Account.Id expected) throws Exception {
+ // No "gerrit whoami" command, so the simplest way to check who the user is over SSH is to query
+ // for owner:self.
+ ChangeInput cin = new ChangeInput();
+ cin.project = project.get();
+ cin.branch = "master";
+ cin.subject = "Test change " + changeCounter.incrementAndGet();
+ String changeId = gApi.changes().create(cin).get().changeId;
+ assertThat(gApi.changes().id(changeId).get().owner._accountId).isEqualTo(expected.get());
+ String queryResults =
+ atrScope.get().getSession().exec("gerrit query owner:self change:" + changeId);
+ assertThat(findDistinct(queryResults, "I[0-9a-f]{40}"))
+ .named("Change-Ids in query results:\n%s", queryResults)
+ .containsExactly(changeId);
+ }
+
+ private static ImmutableSet<String> findDistinct(String input, String pattern) {
+ Matcher m = Pattern.compile(pattern).matcher(input);
+ ImmutableSet.Builder<String> b = ImmutableSet.builder();
+ while (m.find()) {
+ b.add(m.group(0));
+ }
+ return b.build();
}
}
