SSH: Don't use providers in commands

SSH commands are created in request scope. That why it's not needed to
use providers for current user and review db, that are also provided in
the same scope. Providers are needed when data is used across scope
boundaries, e.g. PatchSetParser that is bound in singleton scope must
use providers to access current user and review db.

Change-Id: I53b5d1c4004cd0fbbc6b5f6cdb798b2201e2a9be
This commit is contained in:
David Ostrovsky
2016-04-17 19:05:25 +02:00
committed by David Ostrovsky
parent 29a1f982bb
commit e3d68ae823
17 changed files with 63 additions and 75 deletions

View File

@@ -32,6 +32,11 @@ public class CapabilityUtils {
.getLogger(CapabilityUtils.class);
public static void checkRequiresCapability(Provider<CurrentUser> userProvider,
String pluginName, Class<?> clazz) throws AuthException {
checkRequiresCapability(userProvider.get(), pluginName, clazz);
}
public static void checkRequiresCapability(CurrentUser user,
String pluginName, Class<?> clazz)
throws AuthException {
RequiresCapability rc = getClassAnnotation(clazz, RequiresCapability.class);
@@ -45,7 +50,6 @@ public class CapabilityUtils {
RequiresAnyCapability.class.getSimpleName()));
throw new AuthException("cannot check capability");
}
CurrentUser user = userProvider.get();
CapabilityControl ctl = user.getCapabilities();
if (ctl.canAdministrateServer()) {
return;

View File

@@ -66,7 +66,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
private final GroupControl.GenericFactory genericGroupControlFactory;
private final Provider<IdentifiedUser> identifiedUser;
private final IdentifiedUser.GenericFactory userFactory;
private final Provider<GetGroups> accountGetGroups;
private final GetGroups accountGetGroups;
private final GroupJson json;
private final GroupBackend groupBackend;
@@ -149,7 +149,8 @@ public class ListGroups implements RestReadView<TopLevelResource> {
final GroupControl.GenericFactory genericGroupControlFactory,
final Provider<IdentifiedUser> identifiedUser,
final IdentifiedUser.GenericFactory userFactory,
final Provider<GetGroups> accountGetGroups, GroupJson json,
final GetGroups accountGetGroups,
GroupJson json,
GroupBackend groupBackend) {
this.groupCache = groupCache;
this.groupControlFactory = groupControlFactory;
@@ -197,7 +198,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
}
if (user != null) {
return accountGetGroups.get().apply(
return accountGetGroups.apply(
new AccountResource(userFactory.create(user)));
}

View File

@@ -35,7 +35,6 @@ import com.google.gerrit.server.query.QueryParseException;
import com.google.gson.Gson;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -73,7 +72,7 @@ public class OutputStreamQuery {
TEXT, JSON
}
private final Provider<ReviewDb> db;
private final ReviewDb db;
private final GitRepositoryManager repoManager;
private final ChangeQueryBuilder queryBuilder;
private final QueryProcessor queryProcessor;
@@ -97,7 +96,7 @@ public class OutputStreamQuery {
@Inject
OutputStreamQuery(
Provider<ReviewDb> db,
ReviewDb db,
GitRepositoryManager repoManager,
ChangeQueryBuilder queryBuilder,
QueryProcessor queryProcessor,
@@ -239,7 +238,7 @@ public class OutputStreamQuery {
ChangeControl cc = d.changeControl().forUser(user);
LabelTypes labelTypes = cc.getLabelTypes();
ChangeAttribute c = eventFactory.asChangeAttribute(db.get(), d.change());
ChangeAttribute c = eventFactory.asChangeAttribute(db, d.change());
eventFactory.extend(c, d.change());
if (!trackingFooters.isEmpty()) {
@@ -248,7 +247,7 @@ public class OutputStreamQuery {
}
if (includeAllReviewers) {
eventFactory.addAllReviewers(db.get(), c, d.notes());
eventFactory.addAllReviewers(db, c, d.notes());
}
if (includeSubmitRecords) {
@@ -276,7 +275,7 @@ public class OutputStreamQuery {
}
if (includePatchSets) {
eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(),
eventFactory.addPatchSets(db, rw, c, d.patchSets(),
includeApprovals ? d.approvals().asMap() : null,
includeFiles, d.change(), labelTypes);
}
@@ -285,7 +284,7 @@ public class OutputStreamQuery {
PatchSet current = d.currentPatchSet();
if (current != null) {
c.currentPatchSet =
eventFactory.asPatchSetAttribute(db.get(), rw, d.change(), current);
eventFactory.asPatchSetAttribute(db, rw, d.change(), current);
eventFactory.addApprovals(c.currentPatchSet,
d.currentApprovals(), labelTypes);
@@ -303,7 +302,7 @@ public class OutputStreamQuery {
if (includeComments) {
eventFactory.addComments(c, d.messages());
if (includePatchSets) {
eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(),
eventFactory.addPatchSets(db, rw, c, d.patchSets(),
includeApprovals ? d.approvals().asMap() : null,
includeFiles, d.change(), labelTypes);
for (PatchSetAttribute attribute : c.patchSets) {

View File

@@ -20,7 +20,6 @@ import com.google.common.util.concurrent.Atomics;
import com.google.gerrit.extensions.annotations.RequiresCapability;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.inject.Provider;
import org.apache.sshd.server.Command;
import org.apache.sshd.server.Environment;
@@ -33,12 +32,12 @@ import java.util.concurrent.atomic.AtomicReference;
/** Command that executes some other command. */
public class AliasCommand extends BaseCommand {
private final DispatchCommandProvider root;
private final Provider<CurrentUser> currentUser;
private final CurrentUser currentUser;
private final CommandName command;
private final AtomicReference<Command> atomicCmd;
AliasCommand(@CommandName(Commands.ROOT) DispatchCommandProvider root,
Provider<CurrentUser> currentUser, CommandName command) {
CurrentUser currentUser, CommandName command) {
this.root = root;
this.currentUser = currentUser;
this.command = command;
@@ -108,12 +107,11 @@ public class AliasCommand extends BaseCommand {
private void checkRequiresCapability(Command cmd) throws UnloggedFailure {
RequiresCapability rc = cmd.getClass().getAnnotation(RequiresCapability.class);
if (rc != null) {
CurrentUser user = currentUser.get();
CapabilityControl ctl = user.getCapabilities();
CapabilityControl ctl = currentUser.getCapabilities();
if (!ctl.canPerform(rc.value()) && !ctl.canAdministrateServer()) {
String msg = String.format(
"fatal: %s does not have \"%s\" capability.",
user.getUserName(), rc.value());
currentUser.getUserName(), rc.value());
throw new UnloggedFailure(BaseCommand.STATUS_NOT_ADMIN, msg);
}
}

View File

@@ -29,7 +29,7 @@ public class AliasCommandProvider implements Provider<Command> {
private DispatchCommandProvider root;
@Inject
private Provider<CurrentUser> currentUser;
private CurrentUser currentUser;
public AliasCommandProvider(CommandName command) {
this.command = command;

View File

@@ -33,7 +33,6 @@ import com.google.gerrit.sshd.SshScope.Context;
import com.google.gerrit.util.cli.CmdLineParser;
import com.google.gerrit.util.cli.EndOfOptionsHandler;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.apache.sshd.common.SshException;
import org.apache.sshd.server.Command;
@@ -89,10 +88,10 @@ public abstract class BaseCommand implements Command {
private WorkQueue.Executor executor;
@Inject
private Provider<CurrentUser> user;
private CurrentUser user;
@Inject
private Provider<SshScope.Context> contextProvider;
private SshScope.Context context;
/** Commands declared by a plugin can be scoped by the plugin name. */
@Inject(optional = true)
@@ -278,7 +277,7 @@ public abstract class BaseCommand implements Command {
final TaskThunk tt = new TaskThunk(thunk);
if (isAdminHighPriorityCommand()
&& user.get().getCapabilities().canAdministrateServer()) {
&& user.getCapabilities().canAdministrateServer()) {
// Admin commands should not block the main work threads (there
// might be an interactive shell there), nor should they wait
// for the main work threads.
@@ -332,8 +331,8 @@ public abstract class BaseCommand implements Command {
if (!(e instanceof UnloggedFailure)) {
final StringBuilder m = new StringBuilder();
m.append("Internal server error");
if (user.get().isIdentifiedUser()) {
final IdentifiedUser u = user.get().asIdentifiedUser();
if (user.isIdentifiedUser()) {
final IdentifiedUser u = user.asIdentifiedUser();
m.append(" (user ");
m.append(u.getAccount().getUserName());
m.append(" account ");
@@ -341,7 +340,7 @@ public abstract class BaseCommand implements Command {
m.append(")");
}
m.append(" during ");
m.append(contextProvider.get().getCommandLine());
m.append(context.getCommandLine());
log.error(m.toString(), e);
}
@@ -388,18 +387,16 @@ public abstract class BaseCommand implements Command {
private final class TaskThunk implements CancelableRunnable, ProjectRunnable {
private final CommandRunnable thunk;
private final Context context;
private final String taskName;
private Project.NameKey projectName;
private TaskThunk(final CommandRunnable thunk) {
this.thunk = thunk;
this.context = contextProvider.get();
StringBuilder m = new StringBuilder();
m.append(context.getCommandLine());
if (user.get().isIdentifiedUser()) {
IdentifiedUser u = user.get().asIdentifiedUser();
if (user.isIdentifiedUser()) {
IdentifiedUser u = user.asIdentifiedUser();
m.append(" (").append(u.getAccount().getUserName()).append(")");
}
this.taskName = m.toString();

View File

@@ -23,7 +23,6 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.CapabilityUtils;
import com.google.gerrit.server.args4j.SubcommandHandler;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import org.apache.sshd.server.Command;
@@ -45,7 +44,7 @@ final class DispatchCommand extends BaseCommand {
DispatchCommand create(Map<String, CommandProvider> map);
}
private final Provider<CurrentUser> currentUser;
private final CurrentUser currentUser;
private final Map<String, CommandProvider> commands;
private final AtomicReference<Command> atomicCmd;
@@ -56,7 +55,7 @@ final class DispatchCommand extends BaseCommand {
private List<String> args = new ArrayList<>();
@Inject
DispatchCommand(final Provider<CurrentUser> cu,
DispatchCommand(CurrentUser cu,
@Assisted final Map<String, CommandProvider> all) {
currentUser = cu;
commands = all;

View File

@@ -25,7 +25,6 @@ import com.google.gerrit.server.PeerDaemonUser;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.sshd.SshScope.Context;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.apache.sshd.server.Command;
import org.apache.sshd.server.Environment;
@@ -50,8 +49,8 @@ public final class SuExec extends BaseCommand {
private final DispatchCommandProvider dispatcher;
private boolean enableRunAs;
private Provider<CurrentUser> caller;
private Provider<SshSession> session;
private CurrentUser caller;
private SshSession session;
private IdentifiedUser.GenericFactory userFactory;
private SshScope.Context callingContext;
@@ -69,7 +68,8 @@ public final class SuExec extends BaseCommand {
@Inject
SuExec(final SshScope sshScope,
@CommandName(Commands.ROOT) final DispatchCommandProvider dispatcher,
final Provider<CurrentUser> caller, final Provider<SshSession> session,
final CurrentUser caller,
final SshSession session,
final IdentifiedUser.GenericFactory userFactory,
final SshScope.Context callingContext,
AuthConfig config) {
@@ -112,12 +112,12 @@ public final class SuExec extends BaseCommand {
}
private void checkCanRunAs() throws UnloggedFailure {
if (caller.get() instanceof PeerDaemonUser) {
if (caller instanceof PeerDaemonUser) {
// OK.
} else if (!enableRunAs) {
throw new UnloggedFailure(1,
"fatal: suexec disabled by auth.enableRunAs = false");
} else if (!caller.get().getCapabilities().canRunAs()) {
} else if (!caller.getCapabilities().canRunAs()) {
throw new UnloggedFailure(1, "fatal: suexec not permitted");
}
}
@@ -125,16 +125,15 @@ public final class SuExec extends BaseCommand {
private SshSession newSession() {
final SocketAddress peer;
if (peerAddress == null) {
peer = session.get().getRemoteAddress();
peer = session.getRemoteAddress();
} else {
peer = peerAddress;
}
CurrentUser self = caller.get();
if (self instanceof PeerDaemonUser) {
self = null;
if (caller instanceof PeerDaemonUser) {
caller = null;
}
return new SshSession(session.get(), peer,
userFactory.runAs(peer, accountId, self));
return new SshSession(session, peer,
userFactory.runAs(peer, accountId, caller));
}
private static String join(List<String> args) {

View File

@@ -33,7 +33,6 @@ import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -79,7 +78,7 @@ final class AdminSetParent extends SshCommand {
private AllProjectsName allProjectsName;
@Inject
private Provider<ListChildProjects> listChildProjects;
private ListChildProjects listChildProjects;
private Project.NameKey newParentKey;
@@ -187,7 +186,7 @@ final class AdminSetParent extends SshCommand {
if (newParentKey != null) {
automaticallyExcluded.addAll(getAllParents(newParentKey));
}
for (final ProjectInfo child : listChildProjects.get().apply(
for (final ProjectInfo child : listChildProjects.apply(
new ProjectResource(parent))) {
final Project.NameKey childName = new Project.NameKey(child.name);
if (!excluded.contains(childName)) {

View File

@@ -29,7 +29,6 @@ import com.google.gerrit.server.config.PostCaches;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.kohsuke.args4j.Option;
@@ -51,7 +50,7 @@ final class FlushCaches extends SshCommand {
private boolean list;
@Inject
private Provider<ListCaches> listCaches;
private ListCaches listCaches;
@Inject
private PostCaches postCaches;
@@ -94,7 +93,7 @@ final class FlushCaches extends SshCommand {
@SuppressWarnings("unchecked")
private void doList() {
for (String name : (List<String>) listCaches.get()
for (String name : (List<String>) listCaches
.setFormat(OutputFormat.LIST).apply(new ConfigResource())) {
stderr.print(name);
stderr.print('\n');

View File

@@ -72,7 +72,7 @@ public class ListGroupsCommand extends SshCommand {
final GroupControl.GenericFactory genericGroupControlFactory,
final Provider<IdentifiedUser> identifiedUser,
final IdentifiedUser.GenericFactory userFactory,
final Provider<GetGroups> accountGetGroups,
final GetGroups accountGetGroups,
final GroupJson json,
GroupBackend groupBackend) {
super(groupCache, groupControlFactory, genericGroupControlFactory,

View File

@@ -33,7 +33,6 @@ import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.kohsuke.args4j.Argument;
import org.kohsuke.args4j.Option;
@@ -88,7 +87,7 @@ public class SetReviewersCommand extends SshCommand {
private DeleteReviewer deleteReviewer;
@Inject
private Provider<CurrentUser> userProvider;
private CurrentUser currentUser;
@Inject
private ChangesCollection changesCollection;
@@ -162,8 +161,7 @@ public class SetReviewersCommand extends SshCommand {
}
private void addChangeImpl(String id) throws UnloggedFailure, OrmException {
List<ChangeControl> matched =
changeFinder.find(id, userProvider.get());
List<ChangeControl> matched = changeFinder.find(id, currentUser);
List<ChangeControl> toAdd = new ArrayList<>(changes.size());
for (ChangeControl ctl : matched) {
if (!changes.containsKey(ctl.getId()) && inProject(ctl.getProject())

View File

@@ -38,7 +38,6 @@ import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.gerrit.sshd.SshDaemon;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.apache.sshd.common.io.IoAcceptor;
import org.apache.sshd.common.io.IoSession;
@@ -84,13 +83,13 @@ final class ShowCaches extends SshCommand {
private SshDaemon daemon;
@Inject
private Provider<ListCaches> listCaches;
private ListCaches listCaches;
@Inject
private Provider<GetSummary> getSummary;
private GetSummary getSummary;
@Inject
private Provider<CurrentUser> self;
private CurrentUser self;
@Option(name = "--width", aliases = {"-w"}, metaVar = "COLS", usage = "width of output table")
private int columns = 80;
@@ -155,11 +154,11 @@ final class ShowCaches extends SshCommand {
printDiskCaches(caches);
stdout.print('\n');
if (self.get().getCapabilities().canMaintainServer()) {
if (self.getCapabilities().canMaintainServer()) {
sshSummary();
SummaryInfo summary =
getSummary.get().setGc(gc).setJvm(showJVM).apply(new ConfigResource());
getSummary.setGc(gc).setJvm(showJVM).apply(new ConfigResource());
taskSummary(summary.taskSummary);
memSummary(summary.memSummary);
threadSummary(summary.threadSummary);
@@ -175,7 +174,7 @@ final class ShowCaches extends SshCommand {
private Collection<CacheInfo> getCaches() {
@SuppressWarnings("unchecked")
Map<String, CacheInfo> caches =
(Map<String, CacheInfo>) listCaches.get().apply(new ConfigResource());
(Map<String, CacheInfo>) listCaches.apply(new ConfigResource());
for (Map.Entry<String, CacheInfo> entry : caches.entrySet()) {
CacheInfo cache = entry.getValue();
cache.name = entry.getKey();

View File

@@ -20,16 +20,15 @@ import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.TestSubmitRule;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.inject.Inject;
import com.google.inject.Provider;
/** Command that allows testing of prolog submit-rules in a live instance. */
@CommandMetaData(name = "rule", description = "Test prolog submit rules")
final class TestSubmitRuleCommand extends BaseTestPrologCommand {
@Inject
private Provider<TestSubmitRule> view;
private TestSubmitRule view;
@Override
protected RestModifyView<RevisionResource, TestSubmitRuleInput> createView() {
return view.get();
return view;
}
}

View File

@@ -21,15 +21,14 @@ import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.TestSubmitType;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.inject.Inject;
import com.google.inject.Provider;
@CommandMetaData(name = "type", description = "Test prolog submit type")
final class TestSubmitTypeCommand extends BaseTestPrologCommand {
@Inject
private Provider<TestSubmitType> view;
private TestSubmitType view;
@Override
protected RestModifyView<RevisionResource, TestSubmitRuleInput> createView() {
return view.get();
return view;
}
}

View File

@@ -27,7 +27,6 @@ import com.google.gerrit.server.git.validators.UploadValidators;
import com.google.gerrit.sshd.AbstractGitCommand;
import com.google.gerrit.sshd.SshSession;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.eclipse.jgit.transport.PreUploadHook;
import org.eclipse.jgit.transport.PreUploadHookChain;
@@ -39,7 +38,7 @@ import java.util.List;
/** Publishes Git repositories over SSH using the Git upload-pack protocol. */
final class Upload extends AbstractGitCommand {
@Inject
private Provider<ReviewDb> db;
private ReviewDb db;
@Inject
private TransferConfig config;
@@ -71,7 +70,7 @@ final class Upload extends AbstractGitCommand {
final UploadPack up = new UploadPack(repo);
if (!projectControl.allRefsAreVisible()) {
up.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache, repo,
projectControl, db.get(), true));
projectControl, db, true));
}
up.setPackConfig(config.getPackConfig());
up.setTimeout(config.getTimeout());

View File

@@ -23,7 +23,6 @@ import com.google.gerrit.server.change.ArchiveFormat;
import com.google.gerrit.server.change.GetArchive;
import com.google.gerrit.sshd.AbstractGitCommand;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.eclipse.jgit.api.ArchiveCommand;
import org.eclipse.jgit.api.errors.GitAPIException;
@@ -104,7 +103,7 @@ public class UploadArchive extends AbstractGitCommand {
@Inject
private GetArchive.AllowedFormats allowedFormats;
@Inject
private Provider<ReviewDb> db;
private ReviewDb db;
private Options options = new Options();
/**
@@ -218,7 +217,7 @@ public class UploadArchive extends AbstractGitCommand {
private boolean canRead(ObjectId revId) throws IOException {
try (RevWalk rw = new RevWalk(repo)) {
RevCommit commit = rw.parseCommit(revId);
return projectControl.canReadCommit(db.get(), rw, commit);
return projectControl.canReadCommit(db, rw, commit);
}
}
}