Avoid use of @SessionScoped for GitHub login Guice's session scoped beans rely on the fact that the the GuiceServlet has been captured the request and managed in the current thread. This assumption is not true when a request gets filtered outside GuiceServlet, as it happens for the GitHub OAuth Filter. With this change the GitHub login is directly obtained from the HTTP Servlet Request without relying on GuiceServlet on the active request thread. Change-Id: Id9d687e9cb0cbab413e71ab5c369e64a10ec74f6
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java index 38a0eb6..6c6ff4f 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java
@@ -32,14 +32,21 @@ import org.slf4j.LoggerFactory; import com.google.inject.Inject; -import com.google.inject.servlet.SessionScoped; +import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken; import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope; -@SessionScoped public class GitHubLogin { private static final Logger log = LoggerFactory.getLogger(GitHubLogin.class); + @Singleton + public static class Provider extends HttpSessionProvider<GitHubLogin> { + @Inject + public Provider(com.google.inject.Provider<GitHubLogin> provider) { + super(provider); + } + } + public AccessToken token; public GitHub hub; private SortedSet<Scope> scopesSet = new TreeSet<OAuthProtocol.Scope>();
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java new file mode 100644 index 0000000..13cc038 --- /dev/null +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java
@@ -0,0 +1,46 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.googlesource.gerrit.plugins.github.oauth; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; + +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +public abstract class HttpSessionProvider<T> implements ScopedProvider<T> { + + private final Provider<T> provider; + + public HttpSessionProvider(Provider<T> provider) { + this.provider = provider; + } + + @Override + public T get(final HttpServletRequest req) { + HttpSession session = req.getSession(); + String singletonKey = getClass().getName(); + + synchronized (this) { + @SuppressWarnings("unchecked") + T instance = (T) session.getAttribute(singletonKey); + if (instance == null) { + instance = provider.get(); + session.setAttribute(singletonKey, instance); + } + return instance; + } + } +}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java new file mode 100644 index 0000000..b1f408c --- /dev/null +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java
@@ -0,0 +1,20 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.googlesource.gerrit.plugins.github.oauth; + +import javax.servlet.http.HttpServletRequest; + +public interface ScopedProvider<T> { + T get(HttpServletRequest req); +}
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java index ac51ce1..b235809 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java
@@ -16,11 +16,14 @@ import org.apache.http.client.HttpClient; import org.apache.velocity.runtime.RuntimeInstance; +import com.google.inject.TypeLiteral; import com.google.inject.assistedinject.FactoryModuleBuilder; import com.google.inject.name.Names; import com.google.inject.servlet.ServletModule; import com.googlesource.gerrit.plugins.github.filters.GitHubOAuthFilter; import com.googlesource.gerrit.plugins.github.oauth.GitHubHttpProvider; +import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; import com.googlesource.gerrit.plugins.github.replication.RemoteSiteUser; import com.googlesource.gerrit.plugins.github.velocity.PluginVelocityRuntimeProvider; import com.googlesource.gerrit.plugins.github.velocity.VelocityStaticServlet; @@ -28,6 +31,7 @@ import com.googlesource.gerrit.plugins.github.wizard.VelocityControllerServlet; import com.googlesrouce.gerrit.plugins.github.git.CreateProjectStep; import com.googlesrouce.gerrit.plugins.github.git.GitCloneStep; +import com.googlesrouce.gerrit.plugins.github.git.GitImporter; import com.googlesrouce.gerrit.plugins.github.git.PullRequestImportJob; import com.googlesrouce.gerrit.plugins.github.git.ReplicateProjectStep; @@ -36,6 +40,10 @@ @Override protected void configureServlets() { bind(HttpClient.class).toProvider(GitHubHttpProvider.class); + + bind(new TypeLiteral<ScopedProvider<GitHubLogin>>() {}).to(GitHubLogin.Provider.class); + bind(new TypeLiteral<ScopedProvider<GitImporter>>() {}).to(GitImporter.Provider.class); + install(new FactoryModuleBuilder().build(RemoteSiteUser.Factory.class)); install(new FactoryModuleBuilder().implement(GitCloneStep.class,
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java index f8bb6da..19f0000 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java
@@ -21,22 +21,23 @@ import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; import com.google.inject.Inject; -import com.google.inject.Provider; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; import com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig; import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; @Singleton public class GitHubOAuthFilter implements Filter { - private final Provider<GitHubLogin> loginProvider; + private final ScopedProvider<GitHubLogin> loginProvider; private final Scope[] authScopes; @Inject - public GitHubOAuthFilter(final Provider<GitHubLogin> loginProvider, + public GitHubOAuthFilter(final ScopedProvider<GitHubLogin> loginProvider, final GitHubOAuthConfig githubOAuthConfig) { this.loginProvider = loginProvider; this.authScopes = githubOAuthConfig.scopes.toArray(new Scope[0]); @@ -49,7 +50,7 @@ @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - GitHubLogin hubLogin = loginProvider.get(); + GitHubLogin hubLogin = loginProvider.get((HttpServletRequest) request); if (!hubLogin.isLoggedIn(authScopes)) { hubLogin.login(request, response, authScopes); return;
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java index 21a88e7..4ba74a5 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java
@@ -16,7 +16,6 @@ import java.io.IOException; import java.util.Map.Entry; -import javax.mail.internet.ContentType; import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; @@ -40,6 +39,7 @@ import com.google.inject.name.Named; import com.googlesource.gerrit.plugins.github.GitHubConfig.NextPage; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; @Singleton public class VelocityViewServlet extends HttpServlet { @@ -49,14 +49,14 @@ private static final long serialVersionUID = 529071287765413268L; private final RuntimeInstance velocityRuntime; private final Provider<PluginVelocityModel> modelProvider; - private final Provider<GitHubLogin> loginProvider; + private final ScopedProvider<GitHubLogin> loginProvider; private final Provider<IdentifiedUser> userProvider; @Inject public VelocityViewServlet( @Named("PluginRuntimeInstance") final RuntimeInstance velocityRuntime, Provider<PluginVelocityModel> modelProvider, - Provider<GitHubLogin> loginProvider, Provider<IdentifiedUser> userProvider) { + ScopedProvider<GitHubLogin> loginProvider, Provider<IdentifiedUser> userProvider) { this.velocityRuntime = velocityRuntime; this.modelProvider = modelProvider; @@ -103,9 +103,10 @@ private PluginVelocityModel initVelocityModel(HttpServletRequest request) { PluginVelocityModel model = modelProvider.get(); - model.put("myself", loginProvider.get().getMyself()); + GitHubLogin gitHubLogin = loginProvider.get(request); + model.put("myself", gitHubLogin.getMyself()); model.put("user", userProvider.get()); - model.put("hub", loginProvider.get().hub); + model.put("hub", gitHubLogin.hub); for (Entry<String, String[]> reqPar : request.getParameterMap().entrySet()) { model.put(reqPar.getKey(), reqPar.getValue());
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneCancelController.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneCancelController.java index 442a459..a8b2be2 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneCancelController.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneCancelController.java
@@ -24,15 +24,16 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; import com.googlesrouce.gerrit.plugins.github.git.GitImporter; @Singleton public class RepositoriesCloneCancelController implements VelocityController { - private Provider<GitImporter> clonerProvider; + private ScopedProvider<GitImporter> clonerProvider; @Inject - public RepositoriesCloneCancelController(Provider<GitImporter> clonerProvider) { + public RepositoriesCloneCancelController(ScopedProvider<GitImporter> clonerProvider) { this.clonerProvider = clonerProvider; } @@ -40,7 +41,7 @@ public void doAction(IdentifiedUser user, GitHubLogin hubLogin, HttpServletRequest req, HttpServletResponse resp, ControllerErrors errors) throws ServletException, IOException { - clonerProvider.get().cancel(); + clonerProvider.get(req).cancel(); } }
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneController.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneController.java index 6862e7e..88f1abe 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneController.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneController.java
@@ -23,16 +23,16 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.inject.Inject; -import com.google.inject.Provider; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; import com.googlesrouce.gerrit.plugins.github.git.GitImporter; public class RepositoriesCloneController implements VelocityController { private static final String REPO_PARAM_PREFIX = "repo_"; - private final Provider<GitImporter> cloneProvider; + private final ScopedProvider<GitImporter> cloneProvider; @Inject - public RepositoriesCloneController(Provider<GitImporter> cloneProvider) { + public RepositoriesCloneController(ScopedProvider<GitImporter> cloneProvider) { this.cloneProvider = cloneProvider; } @@ -41,7 +41,7 @@ HttpServletRequest req, HttpServletResponse resp, ControllerErrors errorMgr) throws ServletException, IOException { - GitImporter gitCloner = cloneProvider.get(); + GitImporter gitCloner = cloneProvider.get(req); gitCloner.reset(); Set<Entry<String, String[]>> params = req.getParameterMap().entrySet(); for (Entry<String, String[]> param : params) {
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneStatusController.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneStatusController.java index 1fe5551..da21f81 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneStatusController.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/RepositoriesCloneStatusController.java
@@ -24,14 +24,15 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; import com.googlesrouce.gerrit.plugins.github.git.GitImporter; @Singleton public class RepositoriesCloneStatusController extends JobStatusController implements VelocityController { - private Provider<GitImporter> clonerProvider; + private ScopedProvider<GitImporter> clonerProvider; @Inject - public RepositoriesCloneStatusController(Provider<GitImporter> clonerProvider) { + public RepositoriesCloneStatusController(ScopedProvider<GitImporter> clonerProvider) { this.clonerProvider = clonerProvider; } @@ -39,6 +40,6 @@ public void doAction(IdentifiedUser user, GitHubLogin hubLogin, HttpServletRequest req, HttpServletResponse resp, ControllerErrors errors) throws ServletException, IOException { - respondWithJobStatusJson(resp, clonerProvider.get()); + respondWithJobStatusJson(resp, clonerProvider.get(req)); } }
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/VelocityControllerServlet.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/VelocityControllerServlet.java index 35744f9..2680e77 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/VelocityControllerServlet.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/VelocityControllerServlet.java
@@ -14,13 +14,8 @@ package com.googlesource.gerrit.plugins.github.wizard; import java.io.IOException; -import java.io.UnsupportedEncodingException; -import java.net.URLEncoder; -import java.util.Map.Entry; -import java.util.Set; import javax.servlet.RequestDispatcher; -import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -38,7 +33,7 @@ import com.googlesource.gerrit.plugins.github.GitHubConfig; import com.googlesource.gerrit.plugins.github.GitHubConfig.NextPage; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; -import com.googlesource.gerrit.plugins.github.velocity.PluginVelocityModel; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; @Singleton public class VelocityControllerServlet extends HttpServlet { @@ -47,14 +42,14 @@ .getLogger(VelocityControllerServlet.class); private static final String CONTROLLER_PACKAGE = VelocityControllerServlet.class.getPackage().getName(); - private final Provider<GitHubLogin> loginProvider; + private final ScopedProvider<GitHubLogin> loginProvider; private final Provider<IdentifiedUser> userProvider; private final Injector injector; private final Provider<ControllerErrors> errorsProvider; private final GitHubConfig githubConfig; @Inject - public VelocityControllerServlet(final Provider<GitHubLogin> loginProvider, + public VelocityControllerServlet(final ScopedProvider<GitHubLogin> loginProvider, Provider<IdentifiedUser> userProvider, final Injector injector, Provider<ControllerErrors> errorsProvider, GitHubConfig githubConfig) { this.loginProvider = loginProvider; @@ -84,7 +79,7 @@ return; } - GitHubLogin hubLogin = loginProvider.get(); + GitHubLogin hubLogin = loginProvider.get(req); IdentifiedUser user = userProvider.get(); WrappedResponse wrappedResp = new WrappedResponse(resp); controller.doAction(user, hubLogin, req, wrappedResp, errorsProvider.get());
diff --git a/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/GitImporter.java b/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/GitImporter.java index a0edbe6..925bae1 100644 --- a/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/GitImporter.java +++ b/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/GitImporter.java
@@ -13,45 +13,60 @@ // limitations under the License. package com.googlesrouce.gerrit.plugins.github.git; -import java.util.concurrent.ConcurrentHashMap; +import javax.servlet.http.HttpServletRequest; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.google.gerrit.server.IdentifiedUser; import com.google.inject.Inject; -import com.google.inject.servlet.SessionScoped; +import com.google.inject.Singleton; +import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.HttpSessionProvider; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; -@SessionScoped public class GitImporter extends BatchImporter { + + @Singleton + public static class Provider extends HttpSessionProvider<GitImporter> { + @Inject + public Provider(com.google.inject.Provider<GitImporter> provider) { + super(provider); + } + } + private static final Logger log = LoggerFactory.getLogger(GitImporter.class); private final GitCloneStep.Factory cloneFactory; private final CreateProjectStep.Factory projectFactory; private final ReplicateProjectStep.Factory replicateFactory; + private final GitHubLogin githubLogin; @Inject public GitImporter(GitCloneStep.Factory cloneFactory, CreateProjectStep.Factory projectFactory, - ReplicateProjectStep.Factory replicateFactory, - JobExecutor executor, IdentifiedUser user) { + ReplicateProjectStep.Factory replicateFactory, JobExecutor executor, + IdentifiedUser user, ScopedProvider<GitHubLogin> githubLoginProvider, + HttpServletRequest req) { super(executor, user); this.cloneFactory = cloneFactory; this.projectFactory = projectFactory; this.replicateFactory = replicateFactory; + this.githubLogin = githubLoginProvider.get(req); } public void clone(int idx, String organisation, String repository, String description) { try { - GitCloneStep cloneStep = cloneFactory.create( - organisation, repository); - CreateProjectStep projectStep = projectFactory.create(organisation, - repository, description, user.getUserName()); - ReplicateProjectStep replicateStep = replicateFactory.create(organisation, repository); + GitCloneStep cloneStep = cloneFactory.create(organisation, repository); + CreateProjectStep projectStep = + projectFactory.create(organisation, repository, description, + user.getUserName()); + ReplicateProjectStep replicateStep = + replicateFactory.create(organisation, repository, githubLogin); GitImportJob gitCloneJob = - new GitImportJob(idx, organisation, repository, cloneStep, projectStep, - replicateStep); + new GitImportJob(idx, organisation, repository, cloneStep, + projectStep, replicateStep); log.debug("New Git clone job created: " + gitCloneJob); schedule(idx, gitCloneJob); } catch (Throwable e) {
diff --git a/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/ReplicateProjectStep.java b/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/ReplicateProjectStep.java index 9fa2a61..9016391 100644 --- a/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/ReplicateProjectStep.java +++ b/github-plugin/src/main/java/com/googlesrouce/gerrit/plugins/github/git/ReplicateProjectStep.java
@@ -18,7 +18,6 @@ import org.slf4j.LoggerFactory; import com.google.inject.Inject; -import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; import com.googlesource.gerrit.plugins.github.GitHubURL; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; @@ -31,21 +30,21 @@ public interface Factory { ReplicateProjectStep create(@Assisted("organisation") String organisation, - @Assisted("name") String repository); + @Assisted("name") String repository, @Assisted GitHubLogin ghLogin); } @Inject public ReplicateProjectStep(final ReplicationConfig replicationConfig, - final Provider<GitHubLogin> gitHubLoginProvider, @GitHubURL String gitHubUrl, @Assisted("organisation") String organisation, - @Assisted("name") String repository) { + @Assisted("name") String repository, + @Assisted GitHubLogin ghLogin) { super(gitHubUrl, organisation, repository); LOG.debug("Gerrit ReplicateProject " + organisation + "/" + repository); this.replicationConfig = replicationConfig; - this.authUsername = gitHubLoginProvider.get().getMyself().getLogin(); - this.authToken = gitHubLoginProvider.get().token.access_token; + this.authUsername = ghLogin.getMyself().getLogin(); + this.authToken = ghLogin.token.access_token; } @Override