From afcee32db10b988ca39ff007ae7af16fbae691ac Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 9 Jun 2010 12:44:25 -0700 Subject: [PATCH 01/33] Avoid writing the same cookie twice in one request Some servlet containers format the Cookie object directly into a response header string at the time we call addCookie. Changing the value of the Cookie after its been added will have no impact on what is sent to the client. Avoid setting an empty session token during login(). Change-Id: I515ed67a4763641ac44e25b7234a9b682ae314c2 Signed-off-by: Shawn O. Pearce --- .../com/google/gerrit/httpd/WebSession.java | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java index a2b7fc2ee6..9cf678a045 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java @@ -147,7 +147,9 @@ public final class WebSession { final Account.Id id = res.getAccountId(); final AccountExternalId.Key identity = res.getExternalId(); - logout(); + if (val != null) { + manager.destroy(key); + } key = manager.createKey(id); val = manager.createVal(key, id, rememberMe, identity); @@ -186,22 +188,22 @@ public final class WebSession { ageSeconds = manager.getCookieAge(val); } - if (outCookie == null) { - String path = authConfig.getCookiePath(); - if (path == null || path.isEmpty()) { - path = request.getContextPath(); - if (path.isEmpty()) { - path = "/"; - } + String path = authConfig.getCookiePath(); + if (path == null || path.isEmpty()) { + path = request.getContextPath(); + if (path.isEmpty()) { + path = "/"; } - outCookie = new Cookie(ACCOUNT_COOKIE, token); - outCookie.setPath(path); - outCookie.setMaxAge(ageSeconds); - outCookie.setSecure(authConfig.getCookieSecure()); - response.addCookie(outCookie); - } else { - outCookie.setValue(token); - outCookie.setMaxAge(ageSeconds); } + + if (outCookie != null) { + throw new IllegalStateException("Cookie " + ACCOUNT_COOKIE + " was set"); + } + + outCookie = new Cookie(ACCOUNT_COOKIE, token); + outCookie.setPath(path); + outCookie.setMaxAge(ageSeconds); + outCookie.setSecure(authConfig.getCookieSecure()); + response.addCookie(outCookie); } } From 6acda54742093d17581bf38309589a9c5b22a979 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 26 Oct 2011 16:57:39 -0700 Subject: [PATCH 02/33] Apply user preferences when loading site Change-Id: Id4f00b736bd262b0723dad2f741e0d420cca9ef9 --- gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java | 1 + 1 file changed, 1 insertion(+) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java index f77553489b..43ac009c94 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java @@ -308,6 +308,7 @@ public class Gerrit implements EntryPoint { } if (result.accountDiffPref != null) { myAccountDiffPref = result.accountDiffPref; + applyUserPreferences(); } onModuleLoad2(); } From a6b36ae97fbcb4cf30b59e94c01783288970ff29 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 26 Oct 2011 16:57:26 -0700 Subject: [PATCH 03/33] Embed the XSRF token in the host page again If the servlet container forces 'HttpOnly' on us for our account session cookie, we can't use that to prevent cross site forgery. Instead embed a token that is unique to this session into the host page, and have the web UI echo that token back on each request. We'll validate the token matches the session cookie on the server, and then simply never rotate it within the lifespan of the session. Change-Id: Ia9678335b7446eab8a6ee7f043e03f928707b1ad Signed-off-by: Shawn O. Pearce --- .../gerrit/common/data/HostPageData.java | 1 + .../java/com/google/gerrit/client/Gerrit.java | 12 +++++-- .../com/google/gerrit/httpd/WebSession.java | 12 ++++--- .../gerrit/httpd/WebSessionManager.java | 32 ++++++++++++++++--- .../gerrit/httpd/raw/HostPageServlet.java | 14 ++++++-- 5 files changed, 56 insertions(+), 15 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/HostPageData.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/HostPageData.java index 66b0c3bc10..d90d68b9f3 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/HostPageData.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/HostPageData.java @@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.AccountDiffPreference; public class HostPageData { public Account account; public AccountDiffPreference accountDiffPref; + public String xsrfToken; public GerritConfig config; public Theme theme; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java index 43ac009c94..4b3070f5df 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java @@ -73,13 +73,13 @@ public class Gerrit implements EntryPoint { public static final GerritResources RESOURCES = GWT.create(GerritResources.class); public static final SystemInfoService SYSTEM_SVC; - private static final String SESSION_COOKIE = "GerritAccount"; private static String myHost; private static GerritConfig myConfig; private static HostPageData.Theme myTheme; private static Account myAccount; private static AccountDiffPreference myAccountDiffPref; + private static String xsrfToken; private static MorphingTabPanel menuLeft; private static LinkMenuBar menuRight; @@ -265,10 +265,15 @@ public class Gerrit implements EntryPoint { } static void deleteSessionCookie() { - Cookies.removeCookie(SESSION_COOKIE); myAccount = null; myAccountDiffPref = null; + xsrfToken = null; refreshMenuBar(); + + // If the cookie was HttpOnly, this request to delete it will + // most likely not be successful. We can try anyway though. + // + Cookies.removeCookie("GerritAccount"); } public void onModuleLoad() { @@ -305,6 +310,7 @@ public class Gerrit implements EntryPoint { myTheme = result.theme; if (result.account != null) { myAccount = result.account; + xsrfToken = result.xsrfToken; } if (result.accountDiffPref != null) { myAccountDiffPref = result.accountDiffPref; @@ -437,7 +443,7 @@ public class Gerrit implements EntryPoint { JsonUtil.setDefaultXsrfManager(new XsrfManager() { @Override public String getToken(JsonDefTarget proxy) { - return Cookies.getCookie(SESSION_COOKIE); + return xsrfToken; } @Override diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java index 9cf678a045..d2fd6eebfa 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java @@ -124,12 +124,14 @@ public final class WebSession { return val != null; } - String getToken() { - return isSignedIn() ? key.getToken() : null; + public String getToken() { + return isSignedIn() ? val.getXsrfToken() : null; } public boolean isTokenValid(final String inputToken) { - return isSignedIn() && key.getToken().equals(inputToken); + return isSignedIn() // + && val.getXsrfToken() != null // + && val.getXsrfToken().equals(inputToken); } public AccountExternalId.Key getLastLoginExternalId() { @@ -152,7 +154,7 @@ public final class WebSession { } key = manager.createKey(id); - val = manager.createVal(key, id, rememberMe, identity); + val = manager.createVal(key, id, rememberMe, identity, null); saveCookie(); } @@ -164,7 +166,7 @@ public final class WebSession { /** Set the user account for this current request only. */ void setUserAccountId(Account.Id id) { key = new Key("id:" + id); - val = new Val(id, 0, false, null); + val = new Val(id, 0, false, null, ""); } public void logout() { diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSessionManager.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSessionManager.java index 43c99adc8e..4ae5c30e4a 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSessionManager.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSessionManager.java @@ -78,12 +78,13 @@ class WebSessionManager { final Account.Id who = val.getAccountId(); final boolean remember = val.isPersistentCookie(); final AccountExternalId.Key lastLogin = val.getExternalId(); + final String xsrfToken = val.getXsrfToken(); - return createVal(key, who, remember, lastLogin); + return createVal(key, who, remember, lastLogin, xsrfToken); } Val createVal(final Key key, final Account.Id who, final boolean remember, - final AccountExternalId.Key lastLogin) { + final AccountExternalId.Key lastLogin, String xsrfToken) { // Refresh the cookie every hour or when it is half-expired. // This reduces the odds that the user session will be kicked // early but also avoids us needing to refresh the cookie on @@ -94,7 +95,17 @@ class WebSessionManager { final long refresh = Math.min(halfAgeRefresh, minRefresh); final long refreshCookieAt = now() + refresh; - final Val val = new Val(who, refreshCookieAt, remember, lastLogin); + if (xsrfToken == null) { + // If we don't yet have a token for this session, establish one. + // + final int nonceLen = 20; + final ByteArrayOutputStream buf; + final byte[] rnd = new byte[nonceLen]; + prng.nextBytes(rnd); + xsrfToken = CookieBase64.encode(rnd); + } + + Val val = new Val(who, refreshCookieAt, remember, lastLogin, xsrfToken); self.put(key, val); return val; } @@ -162,13 +173,16 @@ class WebSessionManager { private transient long refreshCookieAt; private transient boolean persistentCookie; private transient AccountExternalId.Key externalId; + private transient String xsrfToken; Val(final Account.Id accountId, final long refreshCookieAt, - final boolean persistentCookie, final AccountExternalId.Key externalId) { + final boolean persistentCookie, final AccountExternalId.Key externalId, + final String xsrfToken) { this.accountId = accountId; this.refreshCookieAt = refreshCookieAt; this.persistentCookie = persistentCookie; this.externalId = externalId; + this.xsrfToken = xsrfToken; } Account.Id getAccountId() { @@ -187,6 +201,10 @@ class WebSessionManager { return persistentCookie; } + String getXsrfToken() { + return xsrfToken; + } + private void writeObject(final ObjectOutputStream out) throws IOException { writeVarInt32(out, 1); writeVarInt32(out, accountId.get()); @@ -202,6 +220,9 @@ class WebSessionManager { writeString(out, externalId.get()); } + writeVarInt32(out, 5); + writeString(out, xsrfToken); + writeVarInt32(out, 0); } @@ -223,6 +244,9 @@ class WebSessionManager { case 4: externalId = new AccountExternalId.Key(readString(in)); continue; + case 5: + xsrfToken = readString(in); + continue; default: throw new IOException("Unknown tag found in object: " + tag); } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java index 1c2e5b8ab4..450d75711f 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java @@ -17,6 +17,7 @@ package com.google.gerrit.httpd.raw; import com.google.gerrit.common.data.GerritConfig; import com.google.gerrit.common.data.HostPageData; import com.google.gerrit.httpd.HtmlDomUtil; +import com.google.gerrit.httpd.WebSession; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.SitePaths; @@ -61,6 +62,7 @@ public class HostPageServlet extends HttpServlet { private static final String HPD_ID = "gerrit_hostpagedata"; private final Provider currentUser; + private final Provider session; private final GerritConfig config; private final HostPageData.Theme signedOutTheme; private final HostPageData.Theme signedInTheme; @@ -71,10 +73,12 @@ public class HostPageServlet extends HttpServlet { private volatile Page page; @Inject - HostPageServlet(final Provider cu, final SitePaths sp, - final ThemeFactory themeFactory, final GerritConfig gc, - final ServletContext servletContext) throws IOException, ServletException { + HostPageServlet(final Provider cu, final Provider w, + final SitePaths sp, final ThemeFactory themeFactory, + final GerritConfig gc, final ServletContext servletContext) + throws IOException, ServletException { currentUser = cu; + session = w; config = gc; signedOutTheme = themeFactory.getSignedOutTheme(); signedInTheme = themeFactory.getSignedInTheme(); @@ -163,6 +167,10 @@ public class HostPageServlet extends HttpServlet { json(((IdentifiedUser) user).getAccount(), w); w.write(";"); + w.write(HPD_ID + ".xsrfToken="); + json(session.get().getToken(), w); + w.write(";"); + w.write(HPD_ID + ".accountDiffPref="); json(((IdentifiedUser) user).getAccountDiffPreference(), w); w.write(";"); From 00b02365268a41640170388616d14fc54e7b9f6e Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 14 Jun 2010 17:55:38 -0700 Subject: [PATCH 04/33] Don't allow registering for cleanup after cleanup runs We only run the cleanup code once per request. After its run, any new objects added onto the cleanup list for this request are wrong and cannot be satisifed. If we don't have this guard, a potential code change could ask for a ReviewDb to be opened after cleanup was done, leaking the database connection. Change-Id: I854f6790b36cad4ad07356a7239c8648dc3937e4 Signed-off-by: Shawn O. Pearce --- .../main/java/com/google/gerrit/server/RequestCleanup.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/RequestCleanup.java b/gerrit-server/src/main/java/com/google/gerrit/server/RequestCleanup.java index 23c0a6fd6d..d836646bc9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/RequestCleanup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/RequestCleanup.java @@ -33,16 +33,21 @@ public class RequestCleanup implements Runnable { LoggerFactory.getLogger(RequestCleanup.class); private final List cleanup = new LinkedList(); + private boolean ran; /** Register a task to be completed after the request ends. */ public void add(final Runnable task) { synchronized (cleanup) { + if (ran) { + throw new IllegalStateException("Request has already been cleaned up"); + } cleanup.add(task); } } public void run() { synchronized (cleanup) { + ran = true; for (final Iterator i = cleanup.iterator(); i.hasNext();) { try { i.next().run(); From 9ca86937bc460906c263a6f8f3c980943142b087 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 14 Jun 2010 17:57:19 -0700 Subject: [PATCH 05/33] Ensure HttpLog can always get the user identity Preload the user before the request starts, to ensure we have it when the request is over for the log file. If we don't preload before dispatching, a database connection may not be available at the end of the request if nobody else has asked for the CurrentUser object. Change-Id: Ic82a4b9d15705a20b0425b292fa14b78b934d97f Signed-off-by: Shawn O. Pearce --- .../gerrit/pgm/http/jetty/JettyServer.java | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java index 7ab8d30254..a309e4eadb 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java @@ -29,6 +29,7 @@ import com.google.inject.Injector; import com.google.inject.Provider; import com.google.inject.Singleton; import com.google.inject.servlet.GuiceFilter; +import com.google.inject.servlet.GuiceHelper; import com.google.inject.servlet.GuiceServletContextListener; import org.eclipse.jetty.io.EndPoint; @@ -67,6 +68,10 @@ import java.util.Set; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + @Singleton public class JettyServer { static class Lifecycle implements LifecycleListener { @@ -117,7 +122,23 @@ public class JettyServer { Handler app = makeContext(env, cfg); if (cfg.getBoolean("httpd", "requestlog", !reverseProxy)) { - RequestLogHandler handler = new RequestLogHandler(); + RequestLogHandler handler = new RequestLogHandler() { + @Override + public void handle(String target, Request baseRequest, + final HttpServletRequest req, final HttpServletResponse rsp) + throws IOException, ServletException { + // Force the user to construct, so it's available to our HttpLog + // later on when the request gets logged out to the access file. + // + GuiceHelper.runInContext(req, rsp, new Runnable() { + @Override + public void run() { + userProvider.get(); + } + }); + super.handle(target, baseRequest, req, rsp); + } + }; handler.setRequestLog(new HttpLog(site, userProvider)); handler.setHandler(app); app = handler; From 2b11da0913f9a222623489e16d7c0b0b5fa485e3 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 6 Sep 2011 16:18:12 -0700 Subject: [PATCH 06/33] Support gitweb.type=disabled Change-Id: I7d2d37658b45a9f779314d365c4aa14066fc7a3a Signed-off-by: Shawn O. Pearce --- Documentation/config-gerrit.txt | 5 +++-- .../com/google/gerrit/common/data/GitWebType.java | 3 +++ .../java/com/google/gerrit/httpd/GitWebConfig.java | 11 ++++++++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 66c8863566..fdac4a58ae 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1054,9 +1054,10 @@ For example, "?p=$project.git;h=$commit". [[gitweb.type]]gitweb.type:: + Optional type of affiliated gitweb service. This allows using -alternatives to gitweb, such as cgit. +alternatives to gitweb, such as cgit. If set to disabled there +is no gitweb hyperlinking support. + -Valid values are `gitweb`, `cgit` or `custom`. +Valid values are `gitweb`, `cgit`, `disabled` or `custom`. [[gitweb.type]]gitweb.revision:: + diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GitWebType.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GitWebType.java index 7eb6955f30..1cef884dec 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GitWebType.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GitWebType.java @@ -40,6 +40,9 @@ public class GitWebType { } else if (name.equalsIgnoreCase("custom")) { type = new GitWebType(); + } else if (name.equalsIgnoreCase("disabled")) { + type = null; + } else { type = null; } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitWebConfig.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitWebConfig.java index b37a152f28..ff55e2fd84 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitWebConfig.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitWebConfig.java @@ -41,6 +41,15 @@ public class GitWebConfig { final String cfgCgi = cfg.getString("gitweb", null, "cgi"); type = GitWebType.fromName(cfg.getString("gitweb", null, "type")); + if (type == null) { + url = null; + gitweb_cgi = null; + gitweb_css = null; + gitweb_js = null; + git_logo_png = null; + return; + } + type.setBranch(cfg.getString("gitweb", null, "branch")); type.setProject(cfg.getString("gitweb", null, "project")); type.setRevision(cfg.getString("gitweb", null, "revision")); @@ -57,7 +66,7 @@ public class GitWebConfig { } if ((cfgUrl != null && cfgUrl.isEmpty()) - || (cfgCgi != null && cfgCgi.isEmpty()) || type == null) { + || (cfgCgi != null && cfgCgi.isEmpty())) { // Either setting was explicitly set to the empty string disabling // gitweb for this server. Disable the configuration. // From ebf043277d78274c7381d78bb49ba4410533c58b Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 6 Sep 2011 10:49:06 -0700 Subject: [PATCH 07/33] Package source JARs for antlr, httpd, server Change-Id: I9879c8d995b83925bc3a798a5e3668be133f8a3d Signed-off-by: Shawn O. Pearce --- gerrit-antlr/pom.xml | 12 ++++++++++++ gerrit-httpd/pom.xml | 16 ++++++++++++++++ gerrit-server/pom.xml | 12 ++++++++++++ 3 files changed, 40 insertions(+) diff --git a/gerrit-antlr/pom.xml b/gerrit-antlr/pom.xml index 4b9e3ad009..8ee4786af3 100644 --- a/gerrit-antlr/pom.xml +++ b/gerrit-antlr/pom.xml @@ -52,6 +52,18 @@ limitations under the License. + + + org.apache.maven.plugins + maven-source-plugin + + + + jar + + + + diff --git a/gerrit-httpd/pom.xml b/gerrit-httpd/pom.xml index 5154515276..ccc9441ce2 100644 --- a/gerrit-httpd/pom.xml +++ b/gerrit-httpd/pom.xml @@ -93,4 +93,20 @@ limitations under the License. ${project.version} + + + + + org.apache.maven.plugins + maven-source-plugin + + + + jar + + + + + + diff --git a/gerrit-server/pom.xml b/gerrit-server/pom.xml index 3302e36e26..1c766f68f6 100644 --- a/gerrit-server/pom.xml +++ b/gerrit-server/pom.xml @@ -215,6 +215,18 @@ limitations under the License. + + + org.apache.maven.plugins + maven-source-plugin + + + + jar + + + + From cf6c62366f9d628fe2d7581526727e7cbaa81c8c Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 6 Sep 2011 16:47:19 -0700 Subject: [PATCH 08/33] Don't rely on Ehcache CacheConfiguration clone() Its cleaner to not rely on cloning the cache configuration. Instead just refactor out the code to configure the cache to a new method and always build a new configuration object. Change-Id: Ide3359eacb3768fef7cd08b14307e6bdbffb378e Signed-off-by: Shawn O. Pearce --- .../google/gerrit/server/cache/CachePool.java | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/cache/CachePool.java b/gerrit-server/src/main/java/com/google/gerrit/server/cache/CachePool.java index 5230ff6b53..ed596da649 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/cache/CachePool.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/cache/CachePool.java @@ -22,7 +22,6 @@ import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.inject.Inject; -import com.google.inject.ProvisionException; import com.google.inject.Singleton; import net.sf.ehcache.CacheManager; @@ -214,8 +213,8 @@ public class CachePool { } } - private void configureDefaultCache() { - final CacheConfiguration c = new CacheConfiguration(); + private CacheConfiguration newConfiguration() { + CacheConfiguration c = new CacheConfiguration(); c.setMaxElementsInMemory(1024); c.setMemoryStoreEvictionPolicyFromObject(MemoryStoreEvictionPolicy.LFU); @@ -232,19 +231,17 @@ public class CachePool { c.setDiskSpoolBufferSizeMB(5); c.setDiskExpiryThreadIntervalSeconds(60 * 60); } + return c; + } - mgr.setDefaultCacheConfiguration(c); + private void configureDefaultCache() { + mgr.setDefaultCacheConfiguration(newConfiguration()); } private CacheConfiguration newCache(final String name) { - try { - final CacheConfiguration c; - c = mgr.getDefaultCacheConfiguration().clone(); - c.setName(name); - return c; - } catch (CloneNotSupportedException e) { - throw new ProvisionException("Cannot configure cache " + name, e); - } + CacheConfiguration c = newConfiguration(); + c.setName(name); + return c; } } } From 3ab5d64571db850efe05aac86433a506c059338c Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 10 Oct 2011 12:23:32 -0700 Subject: [PATCH 09/33] Avoid NPE when group is missing Change-Id: If4f1f0ff086132fcad7147b375bc0317a7628790 --- .../main/java/com/google/gerrit/server/mail/ChangeEmail.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index 4b5d279d85..508408843a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -71,7 +71,8 @@ public abstract class ChangeEmail extends OutgoingEmail { final IdentifiedUser user = args.identifiedUserFactory.create(id); final Set gids = user.getEffectiveGroups(); for (final AccountGroup.UUID gid : gids) { - if (args.groupCache.get(gid).isEmailOnlyAuthors()) { + AccountGroup group = args.groupCache.get(gid); + if (group != null && group.isEmailOnlyAuthors()) { emailOnlyAuthors = true; break; } From 510f2991edb1c372d25a094f8287b8329ba364da Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 27 Sep 2011 12:17:50 -0700 Subject: [PATCH 10/33] Fix token saving redirect in container auth When I changed the URL scheme I forgot to update this jump page that redirects users from /#TOKEN to /login/TOKEN to force using the container based authentication. Also correct "/login//" to be just "/login/". Change-Id: If089e3d8e41b62192bd09edd2f02e3ee00064644 Signed-off-by: Shawn O. Pearce --- .../src/main/java/com/google/gerrit/client/Gerrit.java | 7 +++++-- .../gerrit/httpd/auth/container/HttpLoginServlet.java | 4 +++- .../gerrit/httpd/auth/ldap/LoginRedirectServlet.java | 2 ++ .../google/gerrit/httpd/auth/container/LoginRedirect.html | 7 +++++-- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java index 4b3070f5df..40bf7c4b2d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java @@ -241,12 +241,15 @@ public class Gerrit implements EntryPoint { } /** Sign the user into the application. */ - public static void doSignIn(final String token) { + public static void doSignIn(String token) { switch (myConfig.getAuthType()) { case HTTP: case HTTP_LDAP: case CLIENT_SSL_CERT_LDAP: - Location.assign(Location.getPath() + "login/" + token); + if (!token.startsWith("/")) { + token = "/" + token; + } + Location.assign(Location.getPath() + "login" + token); break; case DEVELOPMENT_BECOME_ANY_ACCOUNT: diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/container/HttpLoginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/container/HttpLoginServlet.java index 8e80a0cec1..5df004e972 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/container/HttpLoginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/container/HttpLoginServlet.java @@ -80,7 +80,7 @@ class HttpLoginServlet extends HttpServlet { protected void doGet(final HttpServletRequest req, final HttpServletResponse rsp) throws ServletException, IOException { final String token = getToken(req); - if ("logout".equals(token) || "signout".equals(token)) { + if ("/logout".equals(token) || "/signout".equals(token)) { req.getRequestDispatcher("/logout").forward(req, rsp); return; } @@ -166,6 +166,8 @@ class HttpLoginServlet extends HttpServlet { String token = req.getPathInfo(); if (token == null || token.isEmpty()) { token = PageLinks.MINE; + } else if (!token.startsWith("/")) { + token = "/" + token; } return token; } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LoginRedirectServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LoginRedirectServlet.java index 2726a7808a..da6e227674 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LoginRedirectServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LoginRedirectServlet.java @@ -77,6 +77,8 @@ class LoginRedirectServlet extends HttpServlet { String token = req.getPathInfo(); if (token == null || token.isEmpty()) { token = PageLinks.MINE; + } else if (!token.startsWith("/")) { + token = "/" + token; } return token; } diff --git a/gerrit-httpd/src/main/resources/com/google/gerrit/httpd/auth/container/LoginRedirect.html b/gerrit-httpd/src/main/resources/com/google/gerrit/httpd/auth/container/LoginRedirect.html index 856ee71f3c..72b589fcf0 100644 --- a/gerrit-httpd/src/main/resources/com/google/gerrit/httpd/auth/container/LoginRedirect.html +++ b/gerrit-httpd/src/main/resources/com/google/gerrit/httpd/auth/container/LoginRedirect.html @@ -8,14 +8,17 @@ var token; if (p >= 0) { token = href.substring(p + 1); + if (token.length != 0 && token.charAt(0) == '/') { + token = token.substring(1); + } href = href.substring(0, p); } else { - token = 'mine'; + token = ''; } window.location.replace(href + 'login/' + token); -

Redirecting to Gerrit Code Review.

+

Redirecting to Gerrit Code Review.

From 94860ee63283a30e27ec99974c0cb858eaeb2176 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 29 Sep 2011 13:11:08 -0700 Subject: [PATCH 11/33] rules.enable: Support disabling project rules The rules.enable variable can be set to false in gerrit.config, allowing a site administrator to globally disable execution of the per-project Prolog rules. Change-Id: Ie15cbea9c9dbc386ff75c8ab08c30474d8f717aa Signed-off-by: Shawn O. Pearce --- Documentation/config-gerrit.txt | 11 +++++++++++ .../main/java/com/google/gerrit/rules/RulesCache.java | 4 +++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index fdac4a58ae..e2a749c4c2 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1614,6 +1614,17 @@ A name of a group which exists in the database. Zero, one or many groups are allowed. Each on its own line. Groups which don't exist in the database are ignored. +[[rules]]Section rules +~~~~~~~~~~~~~~~~~~~~~~ + +[[rules.enable]]rules.enable:: ++ +If true, Gerrit will load and excute 'rules.pl' files in each +project's refs/meta/config branch, if present. When set to false, +only the default internal rules will be used. ++ +Default is true, to execute project specific rules. + [[sendemail]]Section sendemail ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/RulesCache.java b/gerrit-server/src/main/java/com/google/gerrit/rules/RulesCache.java index 690d5ca870..8d9633fc3b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/rules/RulesCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/rules/RulesCache.java @@ -90,6 +90,7 @@ public class RulesCache { } } + private final boolean enableProjectRules; private final File cacheDir; private final File rulesDir; private final GitRepositoryManager gitMgr; @@ -99,6 +100,7 @@ public class RulesCache { @Inject protected RulesCache(@GerritServerConfig Config config, SitePaths site, GitRepositoryManager gm) { + enableProjectRules = config.getBoolean("rules", null, "enable", true); cacheDir = site.resolve(config.getString("cache", null, "directory")); rulesDir = cacheDir != null ? new File(cacheDir, "rules") : null; gitMgr = gm; @@ -117,7 +119,7 @@ public class RulesCache { Project.NameKey project, ObjectId rulesId) throws CompileException { - if (project == null || rulesId == null) { + if (!enableProjectRules || project == null || rulesId == null) { return defaultMachine; } From dba97648ae4185d1a22bfdfdeaa8cd43c67ed73b Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 7 Sep 2011 20:12:31 -0700 Subject: [PATCH 12/33] Allow disabling certain features of HostPageServlet The user agent detection might not always be reliable for some environments, so permit disabling it if the server administrator wants the check to happen in the browser rather than the server. The server administrator may also wish to disable the automatic refresh logic associated with the site header, footer and CSS. For example, these assets might never change anyway without doing a full server restart, so checking the modification time on each incoming request is not worth the system call. Change-Id: Id2db1f53bb30e9cfac79f9d64d37e58737428094 Signed-off-by: Shawn O. Pearce --- Documentation/config-gerrit.txt | 19 +++++++++++++++++++ .../gerrit/httpd/raw/HostPageServlet.java | 11 ++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index e2a749c4c2..07de720b49 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1736,6 +1736,25 @@ should expire. + By default, unset, so no Expiry-Date header is generated. + +[[site]]Section site +~~~~~~~~~~~~~~~~~~~~ + +[[site.checkUserAgent]]site.checkUserAgent:: ++ +If true the server checks the User-Agent HTTP header and sends the +correct JavaScript to the client as part of the initial page load. +This usually reduces a round-trip for the client, allowing the UI to +start more quickly. If false, a tiny JavaScript loader is sent to the +client instead to determine the correct code to use. Default is true. + +[[site.refreshHeaderFooter]]site.refreshHeaderFooter:: ++ +If true the server checks the site header, footer and CSS files for +updated versions. If false, a server restart is required to change +any of these resources. Default is true, allowing automatic reloads. + + [[sshd]] Section sshd ~~~~~~~~~~~~~~~~~~~~~ diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java index 450d75711f..f105df0a55 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java @@ -20,6 +20,7 @@ import com.google.gerrit.httpd.HtmlDomUtil; import com.google.gerrit.httpd.WebSession; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.gwt.user.server.rpc.RPCServletUtils; import com.google.gwtexpui.linker.server.Permutation; @@ -29,6 +30,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.slf4j.Logger; @@ -70,12 +72,14 @@ public class HostPageServlet extends HttpServlet { private final Document template; private final String noCacheName; private final PermutationSelector selector; + private final boolean refreshHeaderFooter; private volatile Page page; @Inject HostPageServlet(final Provider cu, final Provider w, final SitePaths sp, final ThemeFactory themeFactory, - final GerritConfig gc, final ServletContext servletContext) + final GerritConfig gc, final ServletContext servletContext, + @GerritServerConfig final Config cfg) throws IOException, ServletException { currentUser = cu; session = w; @@ -83,6 +87,7 @@ public class HostPageServlet extends HttpServlet { signedOutTheme = themeFactory.getSignedOutTheme(); signedInTheme = themeFactory.getSignedInTheme(); site = sp; + refreshHeaderFooter = cfg.getBoolean("site", "refreshHeaderFooter", true); final String pageName = "HostPage.html"; template = HtmlDomUtil.parseFile(getClass(), pageName); @@ -99,7 +104,7 @@ public class HostPageServlet extends HttpServlet { final String src = "gerrit/gerrit.nocache.js"; selector = new PermutationSelector("gerrit"); - if (IS_DEV) { + if (IS_DEV || !cfg.getBoolean("site", "checkUserAgent", true)) { noCacheName = src; } else { final Element devmode = HtmlDomUtil.find(template, "gerrit_gwtdevmode"); @@ -140,7 +145,7 @@ public class HostPageServlet extends HttpServlet { private Page get() { Page p = page; - if (p.isStale()) { + if (refreshHeaderFooter && p.isStale()) { final Page newPage; try { newPage = new Page(); From 5d6de5281ce98c66f8755f21f089a07350c1af64 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 7 Oct 2011 18:00:16 -0700 Subject: [PATCH 13/33] Define Git-over-HTTP mirror URL Allow site administrators to direct users to another HTTP server for change download, just like they can already do with the anonymous Git protocol. This lets admins move read traffic over to a replicated HTTP environment, which may be able to handle higher traffic volumes. Change-Id: I9fce6ad3cd42c52e50ce9da6850847444a7c032c --- Documentation/config-gerrit.txt | 10 ++++ .../gerrit/common/data/GerritConfig.java | 12 +++++ .../PatchSetComplexDisclosurePanel.java | 47 +++++++++++-------- .../gerrit/httpd/GerritConfigProvider.java | 1 + 4 files changed, 51 insertions(+), 19 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 07de720b49..ce79ddf52a 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1021,6 +1021,16 @@ By default unset, as the git daemon must be configured externally by the system administrator, and might not even be running on the same host as Gerrit. +[[gerrit.gitHttpUrl]]gerrit.gitHttpUrl:: ++ +Optional base URL for repositories available over the HTTP +protocol. For example, set this to `http://mirror.example.com/base/` +to have Gerrit display URLs from this server, rather than itself. ++ +By default unset, as the HTTP daemon must be configured externally +by the system administrator, and might not even be running on the +same host as Gerrit. + [[gerrit.replicateOnStartup]]gerrit.replicateOnStartup:: + If true, replicates to all remotes on startup to ensure they are diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java index 0ef68df55f..c316ba9127 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java @@ -35,6 +35,7 @@ public class GerritConfig implements Cloneable { protected AuthType authType; protected Set downloadSchemes; protected String gitDaemonUrl; + protected String gitHttpUrl; protected String sshdAddress; protected Project.NameKey wildProject; protected ApprovalTypes approvalTypes; @@ -110,6 +111,17 @@ public class GerritConfig implements Cloneable { gitDaemonUrl = url; } + public String getGitHttpUrl() { + return gitHttpUrl; + } + + public void setGitHttpUrl(String url) { + if (url != null && !url.endsWith("/")) { + url += "/"; + } + gitHttpUrl = url; + } + public String getSshdAddress() { return sshdAddress; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java index 003a731259..7209a33b6a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java @@ -204,8 +204,12 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O && (allowedSchemes.contains(DownloadScheme.ANON_HTTP) || allowedSchemes.contains(DownloadScheme.DEFAULT_DOWNLOADS))) { StringBuilder r = new StringBuilder(); - r.append(GWT.getHostPageBaseURL()); - r.append("p/"); + if (Gerrit.getConfig().getGitHttpUrl() != null) { + r.append(Gerrit.getConfig().getGitHttpUrl()); + } else { + r.append(GWT.getHostPageBaseURL()); + r.append("p/"); + } r.append(projectName); r.append(" "); r.append(patchSet.getRefName()); @@ -241,24 +245,29 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O && Gerrit.getUserAccount().getUserName().length() > 0 && (allowedSchemes.contains(DownloadScheme.HTTP) || allowedSchemes.contains(DownloadScheme.DEFAULT_DOWNLOADS))) { - String base = GWT.getHostPageBaseURL(); - int p = base.indexOf("://"); - int s = base.indexOf('/', p + 3); - if (s < 0) { - s = base.length(); - } - String host = base.substring(p + 3, s); - if (host.contains("@")) { - host = host.substring(host.indexOf('@') + 1); - } - final StringBuilder r = new StringBuilder(); - r.append(base.substring(0, p + 3)); - r.append(Gerrit.getUserAccount().getUserName()); - r.append('@'); - r.append(host); - r.append(base.substring(s)); - r.append("p/"); + if (Gerrit.getConfig().getGitHttpUrl() != null + && changeDetail.isAllowsAnonymous()) { + r.append(Gerrit.getConfig().getGitHttpUrl()); + } else { + String base = GWT.getHostPageBaseURL(); + int p = base.indexOf("://"); + int s = base.indexOf('/', p + 3); + if (s < 0) { + s = base.length(); + } + String host = base.substring(p + 3, s); + if (host.contains("@")) { + host = host.substring(host.indexOf('@') + 1); + } + + r.append(base.substring(0, p + 3)); + r.append(Gerrit.getUserAccount().getUserName()); + r.append('@'); + r.append(host); + r.append(base.substring(s)); + r.append("p/"); + } r.append(projectName); r.append(" "); r.append(patchSet.getRefName()); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java index 60486ac9c7..6cd5973f3a 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java @@ -95,6 +95,7 @@ class GerritConfigProvider implements Provider { config.setUseContributorAgreements(cfg.getBoolean("auth", "contributoragreements", false)); config.setGitDaemonUrl(cfg.getString("gerrit", null, "canonicalgiturl")); + config.setGitHttpUrl(cfg.getString("gerrit", null, "gitHttpUrl")); config.setUseContactInfo(contactStore != null && contactStore.isEnabled()); config.setDownloadSchemes(schemeConfig.getDownloadScheme()); config.setAuthType(authConfig.getAuthType()); From 6af6f5f78407d229054d4974cf26842414c3d697 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 8 Jun 2010 17:38:43 -0700 Subject: [PATCH 14/33] Allow sshd.listenAddress = off to disable the daemon We might not want to run the internal SSHD, ever, on this system. In such cases permit off for listenAddress so that we don't initialize a server key, or even try to load it at startup. Change-Id: Ia57c3aa24413d64e10e0440f758b3b18f881ddd9 Signed-off-by: Shawn O. Pearce --- Documentation/config-gerrit.txt | 2 ++ .../com/google/gerrit/pgm/init/InitSshd.java | 15 ++++++++++++++- .../java/com/google/gerrit/sshd/SshDaemon.java | 16 +++++++++++++++- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index ce79ddf52a..0e75701033 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1784,6 +1784,8 @@ default of 29418. If multiple values are supplied, the daemon will listen on all of them. + +To disable the internal SSHD, set listenAddress to `off`. ++ By default, *:29418. [[sshd.advertisedAddress]]sshd.advertisedAddress:: diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitSshd.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitSshd.java index 482405e9d6..bfc0eaf2ce 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitSshd.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitSshd.java @@ -54,13 +54,20 @@ class InitSshd implements InitStep { String hostname = "*"; int port = 29418; String listenAddress = sshd.get("listenAddress"); - if (listenAddress != null && !listenAddress.isEmpty()) { + if (isOff(listenAddress)) { + hostname = "off"; + } else if (listenAddress != null && !listenAddress.isEmpty()) { final InetSocketAddress addr = SocketUtil.parse(listenAddress, port); hostname = SocketUtil.hostname(addr); port = addr.getPort(); } hostname = ui.readString(hostname, "Listen on address"); + if (isOff(hostname)) { + sshd.set("listenAddress", "off"); + return; + } + port = ui.readInt(port, "Listen on port"); sshd.set("listenAddress", SocketUtil.format(hostname, port)); @@ -73,6 +80,12 @@ class InitSshd implements InitStep { generateSshHostKeys(); } + private static boolean isOff(String listenHostname) { + return "off".equalsIgnoreCase(listenHostname) + || "none".equalsIgnoreCase(listenHostname) + || "no".equalsIgnoreCase(listenHostname); + } + private void generateSshHostKeys() throws InterruptedException, IOException { if (!site.ssh_key.exists() // && !site.ssh_rsa.exists() // diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java index fe9d9eb153..d382a575fd 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java @@ -216,7 +216,7 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { @Override public synchronized void start() { - if (acceptor == null) { + if (acceptor == null && !listen.isEmpty()) { checkConfig(); acceptor = createAcceptor(); @@ -257,6 +257,10 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { } private List computeHostKeys() { + if (listen.isEmpty()) { + return Collections.emptyList(); + } + final List keys = myHostKeys(); final ArrayList r = new ArrayList(); for (final PublicKey pub : keys) { @@ -348,6 +352,10 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { return bind; } + if (want.length == 1 && isOff(want[0])) { + return bind; + } + for (final String desc : want) { try { bind.add(SocketUtil.resolve(desc, DEFAULT_PORT)); @@ -358,6 +366,12 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { return bind; } + private static boolean isOff(String listenHostname) { + return "off".equalsIgnoreCase(listenHostname) + || "none".equalsIgnoreCase(listenHostname) + || "no".equalsIgnoreCase(listenHostname); + } + @SuppressWarnings("unchecked") private void initProviderBouncyCastle() { setKeyExchangeFactories(Arrays.> asList( From 7af8f48af9ad7299bdcbcf0dfff691ff8a828165 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 9 Jun 2010 14:22:58 -0700 Subject: [PATCH 15/33] daemon: Allow httpd without sshd We now support turning off the SSHD, but leaving the HTTPD on. This matches what happens when sshd.listenAddress = off in gerrit.config, but is done much earlier in the daemon initialization. Because the SSHD has no executor threads in this setup, we cannot register the QoS filter that defers execution onto the SSHD workers. We probably don't want to flat out disable QoS here, having a way to slow down non-interactive or abusive accounts by moving them into a smaller worker pool can be very useful, but I just don't have the time to rework the QoS filter for the HTTP only case right now. Change-Id: I181acef999b1bcb3c439e26b9eec45e06cd89804 Signed-off-by: Shawn O. Pearce --- .../java/com/google/gerrit/pgm/Daemon.java | 21 +++++++------ .../google/gerrit/server/ssh/NoSshInfo.java | 29 ++++++++++++++++++ .../gerrit/server/ssh/NoSshKeyCache.java | 30 +++++++++++++++++++ .../google/gerrit/server/ssh/NoSshModule.java | 28 +++++++++++++++++ 4 files changed, 99 insertions(+), 9 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshKeyCache.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshModule.java diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 96c97b0ba1..651daa0982 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -32,6 +32,7 @@ import com.google.gerrit.server.config.CanonicalWebUrlProvider; import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.MasterNodeStartup; import com.google.gerrit.server.schema.SchemaVersionCheck; +import com.google.gerrit.server.ssh.NoSshModule; import com.google.gerrit.sshd.SshModule; import com.google.gerrit.sshd.commands.MasterCommandModule; import com.google.gerrit.sshd.commands.SlaveCommandModule; @@ -114,10 +115,6 @@ public class Daemon extends SiteProgram { if (slave && httpd) { throw die("Cannot combine --slave and --enable-httpd"); } - if (httpd && !sshd) { - // TODO Support HTTP without SSH. - throw die("--enable-httpd currently requires --enable-sshd"); - } if (consoleLog) { } else { @@ -217,11 +214,15 @@ public class Daemon extends SiteProgram { private Injector createSshInjector() { final List modules = new ArrayList(); - modules.add(new SshModule()); - if (slave) { - modules.add(new SlaveCommandModule()); + if (sshd) { + modules.add(new SshModule()); + if (slave) { + modules.add(new SlaveCommandModule()); + } else { + modules.add(new MasterCommandModule()); + } } else { - modules.add(new MasterCommandModule()); + modules.add(new NoSshModule()); } return sysInjector.createChildInjector(modules); } @@ -240,7 +241,9 @@ public class Daemon extends SiteProgram { private Injector createWebInjector() { final List modules = new ArrayList(); modules.add(sshInjector.getInstance(WebModule.class)); - modules.add(sshInjector.getInstance(ProjectQoSFilter.Module.class)); + if (sshd) { + modules.add(sshInjector.getInstance(ProjectQoSFilter.Module.class)); + } return sysInjector.createChildInjector(modules); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java b/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java new file mode 100644 index 0000000000..882181db42 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java @@ -0,0 +1,29 @@ +// Copyright (C) 2010 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.google.gerrit.server.ssh; + +import com.google.gerrit.server.ssh.SshInfo; + +import com.jcraft.jsch.HostKey; + +import java.util.Collections; +import java.util.List; + +class NoSshInfo implements SshInfo { + @Override + public List getHostKeys() { + return Collections.emptyList(); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshKeyCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshKeyCache.java new file mode 100644 index 0000000000..ad18b3f57a --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshKeyCache.java @@ -0,0 +1,30 @@ +// Copyright (C) 2010 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.google.gerrit.server.ssh; + +import com.google.gerrit.common.errors.InvalidSshKeyException; +import com.google.gerrit.reviewdb.AccountSshKey; + +class NoSshKeyCache implements SshKeyCache { + @Override + public void evict(String username) { + } + + @Override + public AccountSshKey create(AccountSshKey.Id id, String encoded) + throws InvalidSshKeyException { + throw new InvalidSshKeyException(); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshModule.java new file mode 100644 index 0000000000..21b1a549ea --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshModule.java @@ -0,0 +1,28 @@ +// Copyright (C) 2010 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.google.gerrit.server.ssh; + +import com.google.inject.AbstractModule; + +/** + * Disables the SSH support by stubbing out relevant objects. + */ +public class NoSshModule extends AbstractModule { + @Override + protected void configure() { + bind(SshInfo.class).to(NoSshInfo.class); + bind(SshKeyCache.class).to(NoSshKeyCache.class); + } +} From f2a22761dd0d67a249b368e7b26fed42cb9c2713 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 9 Jun 2010 17:22:29 -0700 Subject: [PATCH 16/33] Refactor how we tie the SSH objects into the HTTP injector Split the SSH stuff into its own Guice module, allowing more direct replacement of the SSH objects when the SSH daemon is disabled, or is not being loaded into a particular server runtime. Change-Id: Ibe79f394110e4d3d0d21c5f1473f306ed8f49e7c Signed-off-by: Shawn O. Pearce --- .../com/google/gerrit/httpd/WebModule.java | 15 +----- .../google/gerrit/httpd/WebSshGlueModule.java | 46 +++++++++++++++++++ .../java/com/google/gerrit/pgm/Daemon.java | 2 + .../gerrit/httpd/WebAppInitializer.java | 1 + 4 files changed, 50 insertions(+), 14 deletions(-) create mode 100644 gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSshGlueModule.java diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java index 96ca017fa4..6895f49294 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java @@ -24,7 +24,6 @@ import com.google.gerrit.httpd.auth.ldap.LdapAuthModule; import com.google.gerrit.httpd.auth.openid.OpenIdModule; import com.google.gerrit.httpd.gitweb.GitWebModule; import com.google.gerrit.httpd.rpc.UiRpcModule; -import com.google.gerrit.reviewdb.AuthType; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.RemotePeer; @@ -38,12 +37,9 @@ import com.google.gerrit.server.config.FactoryModule; import com.google.gerrit.server.config.GerritRequestModule; import com.google.gerrit.server.contact.ContactStore; import com.google.gerrit.server.contact.ContactStoreProvider; -import com.google.gerrit.server.ssh.SshInfo; -import com.google.gerrit.server.ssh.SshKeyCache; import com.google.inject.AbstractModule; import com.google.inject.Inject; import com.google.inject.Injector; -import com.google.inject.Provider; import com.google.inject.ProvisionException; import com.google.inject.servlet.RequestScoped; import com.google.inject.servlet.ServletModule; @@ -53,20 +49,14 @@ import java.net.SocketAddress; import javax.annotation.Nullable; public class WebModule extends FactoryModule { - private final Provider sshInfoProvider; - private final Provider sshKeyCacheProvider; private final AuthConfig authConfig; private final boolean wantSSL; private final GitWebConfig gitWebConfig; @Inject - WebModule(final Provider sshInfoProvider, - final Provider sshKeyCacheProvider, - final AuthConfig authConfig, + WebModule(final AuthConfig authConfig, @CanonicalWebUrl @Nullable final String canonicalUrl, final Injector creatingInjector) { - this.sshInfoProvider = sshInfoProvider; - this.sshKeyCacheProvider = sshKeyCacheProvider; this.authConfig = authConfig; this.wantSSL = canonicalUrl != null && canonicalUrl.startsWith("https:"); @@ -129,9 +119,6 @@ public class WebModule extends FactoryModule { install(new GerritRequestModule()); install(new ProjectServlet.Module()); - bind(SshInfo.class).toProvider(sshInfoProvider); - bind(SshKeyCache.class).toProvider(sshKeyCacheProvider); - bind(GitWebConfig.class).toInstance(gitWebConfig); if (gitWebConfig.getGitwebCGI() != null) { install(new GitWebModule()); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSshGlueModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSshGlueModule.java new file mode 100644 index 0000000000..82e9da76a6 --- /dev/null +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSshGlueModule.java @@ -0,0 +1,46 @@ +// Copyright (C) 2010 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.google.gerrit.httpd; + +import com.google.gerrit.server.ssh.SshInfo; +import com.google.gerrit.server.ssh.SshKeyCache; +import com.google.inject.AbstractModule; +import com.google.inject.Inject; +import com.google.inject.Provider; + +/** + * Pulls objects from the SSH injector over the HTTP injector. + *

+ * This mess is only necessary because we build up two different injectors, in + * order to have different request scopes. But some HTTP RPCs can cause changes + * to the SSH side of the house, and thus needs access to it. + */ +public class WebSshGlueModule extends AbstractModule { + private final Provider sshInfoProvider; + private final Provider sshKeyCacheProvider; + + @Inject + WebSshGlueModule(Provider sshInfoProvider, + Provider sshKeyCacheProvider) { + this.sshInfoProvider = sshInfoProvider; + this.sshKeyCacheProvider = sshKeyCacheProvider; + } + + @Override + protected void configure() { + bind(SshInfo.class).toProvider(sshInfoProvider); + bind(SshKeyCache.class).toProvider(sshKeyCacheProvider); + } +} diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 651daa0982..f5e6304722 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -18,6 +18,7 @@ import static com.google.gerrit.server.schema.DataSourceProvider.Context.MULTI_U import com.google.gerrit.httpd.HttpCanonicalWebUrlProvider; import com.google.gerrit.httpd.WebModule; +import com.google.gerrit.httpd.WebSshGlueModule; import com.google.gerrit.lifecycle.LifecycleManager; import com.google.gerrit.pgm.http.jetty.JettyEnv; import com.google.gerrit.pgm.http.jetty.JettyModule; @@ -241,6 +242,7 @@ public class Daemon extends SiteProgram { private Injector createWebInjector() { final List modules = new ArrayList(); modules.add(sshInjector.getInstance(WebModule.class)); + modules.add(sshInjector.getInstance(WebSshGlueModule.class)); if (sshd) { modules.add(sshInjector.getInstance(ProjectQoSFilter.Module.class)); } diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index ac05f0fe49..c89c338726 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -197,6 +197,7 @@ public class WebAppInitializer extends GuiceServletContextListener { private Injector createWebInjector() { final List modules = new ArrayList(); modules.add(sshInjector.getInstance(WebModule.class)); + modules.add(sshInjector.getInstance(WebSshGlueModule.class)); return sysInjector.createChildInjector(modules); } From d41cfc5fb31b4f3f61be978270e6ffe715fdb35b Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 10 Jun 2010 18:12:34 -0700 Subject: [PATCH 17/33] Disable SSH Keys in the web UI if SSHD is disabled If the SSHD was turned off by the site administrator, we don't know its address, and we shouldn't offer users the SSH Keys panel as anything they might configure through it is useless. Change-Id: Ifcea988d837f572c8fc57d18c5d0b7aab6bbb121 Signed-off-by: Shawn O. Pearce --- .../gerrit/client/account/RegisterScreen.java | 26 ++++++++++--------- .../gerrit/client/account/SettingsScreen.java | 4 ++- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/RegisterScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/RegisterScreen.java index 8c99d45389..605d7f3797 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/RegisterScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/RegisterScreen.java @@ -99,18 +99,20 @@ public class RegisterScreen extends AccountScreen { formBody.add(fp); } - final FlowPanel sshKeyGroup = new FlowPanel(); - sshKeyGroup.setStyleName(Gerrit.RESOURCES.css().registerScreenSection()); - sshKeyGroup.add(new SmallHeading(Util.C.welcomeSshKeyHeading())); - final HTML whySshKey = new HTML(Util.C.welcomeSshKeyText()); - whySshKey.setStyleName(Gerrit.RESOURCES.css().registerScreenExplain()); - sshKeyGroup.add(whySshKey); - sshKeyGroup.add(new SshPanel() { - { - setKeyTableVisible(false); - } - }); - formBody.add(sshKeyGroup); + if (Gerrit.getConfig().getSshdAddress() != null) { + final FlowPanel sshKeyGroup = new FlowPanel(); + sshKeyGroup.setStyleName(Gerrit.RESOURCES.css().registerScreenSection()); + sshKeyGroup.add(new SmallHeading(Util.C.welcomeSshKeyHeading())); + final HTML whySshKey = new HTML(Util.C.welcomeSshKeyText()); + whySshKey.setStyleName(Gerrit.RESOURCES.css().registerScreenExplain()); + sshKeyGroup.add(whySshKey); + sshKeyGroup.add(new SshPanel() { + { + setKeyTableVisible(false); + } + }); + formBody.add(sshKeyGroup); + } final FlowPanel choices = new FlowPanel(); choices.setStyleName(Gerrit.RESOURCES.css().registerScreenNextLinks()); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java index 93560182ef..97b2efb6de 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java @@ -26,7 +26,9 @@ public abstract class SettingsScreen extends MenuScreen { link(Util.C.tabPreferences(), PageLinks.SETTINGS_PREFERENCES); link(Util.C.tabWatchedProjects(), PageLinks.SETTINGS_PROJECTS); link(Util.C.tabContactInformation(), PageLinks.SETTINGS_CONTACT); - link(Util.C.tabSshKeys(), PageLinks.SETTINGS_SSHKEYS); + if (Gerrit.getConfig().getSshdAddress() != null) { + link(Util.C.tabSshKeys(), PageLinks.SETTINGS_SSHKEYS); + } link(Util.C.tabHttpAccess(), PageLinks.SETTINGS_HTTP_PASSWORD); link(Util.C.tabWebIdentities(), PageLinks.SETTINGS_WEBIDENT); link(Util.C.tabMyGroups(), PageLinks.SETTINGS_MYGROUPS); From 7474c6927286d3046ee9d5b17689a9da251888f5 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 10 Jun 2010 14:54:25 -0700 Subject: [PATCH 18/33] Make Address, EmailHeader visible to other EmailSenders Change-Id: If1412645f4d68a5d0ebd70e4790a8ba79a4eac6c Signed-off-by: Shawn O. Pearce --- .../google/gerrit/server/mail/Address.java | 18 +++++-- .../gerrit/server/mail/EmailHeader.java | 47 ++++++++++++------- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java index 1640e0594f..624e626b7b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java @@ -16,8 +16,8 @@ package com.google.gerrit.server.mail; import java.io.UnsupportedEncodingException; -class Address { - static Address parse(final String in) { +public class Address { + public static Address parse(final String in) { final int lt = in.indexOf('<'); final int gt = in.indexOf('>'); final int at = in.indexOf("@"); @@ -37,15 +37,23 @@ class Address { final String name; final String email; - Address(String email) { + public Address(String email) { this(null, email); } - Address(String name, String email) { + public Address(String name, String email) { this.name = name; this.email = email; } + public String getName() { + return name; + } + + public String getEmail() { + return email; + } + @Override public String toString() { try { @@ -55,7 +63,7 @@ class Address { } } - String toHeaderString() throws UnsupportedEncodingException { + public String toHeaderString() throws UnsupportedEncodingException { if (name != null) { return quotedPhrase(name) + " <" + email + ">"; } else if (isSimple()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailHeader.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailHeader.java index 10f6510ffa..16f9062301 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailHeader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailHeader.java @@ -20,28 +20,33 @@ import java.io.Writer; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Iterator; +import java.util.Collections; import java.util.List; import java.util.Locale; -abstract class EmailHeader { - abstract boolean isEmpty(); +public abstract class EmailHeader { + public abstract boolean isEmpty(); - abstract void write(Writer w) throws IOException; + public abstract void write(Writer w) throws IOException; - static class String extends EmailHeader { + public static class String extends EmailHeader { private java.lang.String value; - String(java.lang.String v) { + public String(java.lang.String v) { value = v; } + public java.lang.String getString() { + return value; + } + @Override - boolean isEmpty() { + public boolean isEmpty() { return value == null || value.length() == 0; } @Override - void write(Writer w) throws IOException { + public void write(Writer w) throws IOException { if (needsQuotedPrintable(value)) { w.write(quotedPrintable(value)); } else { @@ -84,20 +89,24 @@ abstract class EmailHeader { return r.toString(); } - static class Date extends EmailHeader { + public static class Date extends EmailHeader { private java.util.Date value; - Date(java.util.Date v) { + public Date(java.util.Date v) { value = v; } + public java.util.Date getDate() { + return value; + } + @Override - boolean isEmpty() { + public boolean isEmpty() { return value == null; } @Override - void write(Writer w) throws IOException { + public void write(Writer w) throws IOException { final SimpleDateFormat fmt; // Mon, 1 Jun 2009 10:49:44 -0700 fmt = new SimpleDateFormat("EEE, d MMM yyyy HH:mm:ss Z", Locale.ENGLISH); @@ -105,17 +114,21 @@ abstract class EmailHeader { } } - static class AddressList extends EmailHeader { + public static class AddressList extends EmailHeader { private final List

list = new ArrayList
(); - AddressList() { + public AddressList() { } - AddressList(Address addr) { + public AddressList(Address addr) { add(addr); } - void add(Address addr) { + public List
getAddressList() { + return Collections.unmodifiableList(list); + } + + public void add(Address addr) { list.add(addr); } @@ -128,12 +141,12 @@ abstract class EmailHeader { } @Override - boolean isEmpty() { + public boolean isEmpty() { return list.isEmpty(); } @Override - void write(Writer w) throws IOException { + public void write(Writer w) throws IOException { int len = 8; boolean firstAddress = true; boolean needComma = false; From 3075a4766a2e9080de95a4684168fb3d0d9b74cc Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 10 Jun 2010 14:51:14 -0700 Subject: [PATCH 19/33] Move SmtpEmailSender to its own module This way we can choose to bind a different type of message delivery system. Change-Id: I2ce4956637708111ae61e1ea75350b3a234c52ef Signed-off-by: Shawn O. Pearce --- .../src/main/java/com/google/gerrit/pgm/Daemon.java | 2 ++ .../google/gerrit/server/config/GerritGlobalModule.java | 3 --- .../com/google/gerrit/server/mail/SmtpEmailSender.java | 8 ++++++++ .../java/com/google/gerrit/httpd/WebAppInitializer.java | 2 ++ 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index f5e6304722..8008b92002 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -32,6 +32,7 @@ import com.google.gerrit.server.config.CanonicalWebUrlModule; import com.google.gerrit.server.config.CanonicalWebUrlProvider; import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.MasterNodeStartup; +import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.schema.SchemaVersionCheck; import com.google.gerrit.server.ssh.NoSshModule; import com.google.gerrit.sshd.SshModule; @@ -187,6 +188,7 @@ public class Daemon extends SiteProgram { modules.add(SchemaVersionCheck.module()); modules.add(new LogFileCompressor.Module()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); + modules.add(new SmtpEmailSender.Module()); if (httpd) { modules.add(new CanonicalWebUrlModule() { @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 7bca406213..0076d3cade 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -50,10 +50,8 @@ import com.google.gerrit.server.git.SecureCredentialsProvider; import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.TransferConfig; import com.google.gerrit.server.git.WorkQueue; -import com.google.gerrit.server.mail.EmailSender; import com.google.gerrit.server.mail.FromAddressGenerator; import com.google.gerrit.server.mail.FromAddressGeneratorProvider; -import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.patch.PatchListCacheImpl; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.AccessControlModule; @@ -175,7 +173,6 @@ public class GerritGlobalModule extends FactoryModule { bind(FromAddressGenerator.class).toProvider( FromAddressGeneratorProvider.class).in(SINGLETON); - bind(EmailSender.class).to(SmtpEmailSender.class).in(SINGLETON); bind(PatchSetInfoFactory.class); bind(IdentifiedUser.GenericFactory.class).in(SINGLETON); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java index 3c5e64a2d4..f214255f72 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.Version; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.AbstractModule; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -40,6 +41,13 @@ import java.util.Set; /** Sends email via a nearby SMTP server. */ @Singleton public class SmtpEmailSender implements EmailSender { + public static class Module extends AbstractModule { + @Override + protected void configure() { + bind(EmailSender.class).to(SmtpEmailSender.class); + } + } + public static enum Encryption { NONE, SSL, TLS; } diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index c89c338726..f51fbaf360 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -25,6 +25,7 @@ import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.GerritServerConfigModule; import com.google.gerrit.server.config.MasterNodeStartup; import com.google.gerrit.server.config.SitePath; +import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.schema.DataSourceProvider; import com.google.gerrit.server.schema.DatabaseModule; import com.google.gerrit.server.schema.SchemaModule; @@ -177,6 +178,7 @@ public class WebAppInitializer extends GuiceServletContextListener { private Injector createSysInjector() { final List modules = new ArrayList(); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); + modules.add(new SmtpEmailSender.Module()); modules.add(new CanonicalWebUrlModule() { @Override protected Class> provider() { From 28d20fb4a03059c73c69cee65c2b88104116caa5 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sat, 7 Aug 2010 09:32:45 -0700 Subject: [PATCH 20/33] Move GitRepositoryManager setup out of SchemaModule Although we always need a GitRepositoryManager, lets set that up by itself so its easier to decide to use a different implementation. Change-Id: I690db7019231e86d3bcbe4fbac47eee58a29aa10 Signed-off-by: Shawn O. Pearce --- .../com/google/gerrit/pgm/util/SiteProgram.java | 2 ++ .../server/git/LocalDiskRepositoryManager.java | 16 ++++++++++++++++ .../gerrit/server/schema/SchemaModule.java | 11 ----------- .../google/gerrit/httpd/WebAppInitializer.java | 2 ++ 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java index 340168c7a2..7bc6c0af16 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java @@ -20,6 +20,7 @@ import static com.google.inject.Stage.PRODUCTION; import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.server.config.GerritServerConfigModule; import com.google.gerrit.server.config.SitePath; +import com.google.gerrit.server.git.LocalDiskRepositoryManager; import com.google.gerrit.server.schema.DataSourceProvider; import com.google.gerrit.server.schema.DatabaseModule; import com.google.gerrit.server.schema.SchemaModule; @@ -164,6 +165,7 @@ public abstract class SiteProgram extends AbstractProgram { modules.add(new GerritServerConfigModule()); modules.add(new DatabaseModule()); modules.add(new SchemaModule()); + modules.add(new LocalDiskRepositoryManager.Module()); try { return Guice.createInjector(PRODUCTION, modules); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java index 268eed3953..f1493def99 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java @@ -15,9 +15,11 @@ package com.google.gerrit.server.git; import com.google.gerrit.lifecycle.LifecycleListener; +import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.reviewdb.Project; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; +import com.google.inject.AbstractModule; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -54,6 +56,20 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { private static final String UNNAMED = "Unnamed repository; edit this file to name it for gitweb."; + public static class Module extends AbstractModule { + @Override + protected void configure() { + bind(GitRepositoryManager.class).to(LocalDiskRepositoryManager.class); + + install(new LifecycleModule() { + @Override + protected void configure() { + listener().to(LocalDiskRepositoryManager.Lifecycle.class); + } + }); + } + } + public static class Lifecycle implements LifecycleListener { private final Config cfg; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaModule.java index ea277d7f1d..8dc7a3f75c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaModule.java @@ -16,14 +16,11 @@ package com.google.gerrit.server.schema; import static com.google.inject.Scopes.SINGLETON; -import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.GerritPersonIdentProvider; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.AllProjectsNameProvider; import com.google.gerrit.server.config.FactoryModule; -import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.LocalDiskRepositoryManager; import org.eclipse.jgit.lib.PersonIdent; @@ -39,13 +36,5 @@ public class SchemaModule extends FactoryModule { bind(AllProjectsName.class) .toProvider(AllProjectsNameProvider.class) .in(SINGLETON); - - bind(GitRepositoryManager.class).to(LocalDiskRepositoryManager.class); - install(new LifecycleModule() { - @Override - protected void configure() { - listener().to(LocalDiskRepositoryManager.Lifecycle.class); - } - }); } } diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index f51fbaf360..988ce4aac5 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -25,6 +25,7 @@ import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.GerritServerConfigModule; import com.google.gerrit.server.config.MasterNodeStartup; import com.google.gerrit.server.config.SitePath; +import com.google.gerrit.server.git.LocalDiskRepositoryManager; import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.schema.DataSourceProvider; import com.google.gerrit.server.schema.DatabaseModule; @@ -170,6 +171,7 @@ public class WebAppInitializer extends GuiceServletContextListener { modules.add(new GerritServerConfigModule()); } modules.add(new SchemaModule()); + modules.add(new LocalDiskRepositoryManager.Module()); modules.add(SchemaVersionCheck.module()); modules.add(new AuthConfigModule()); return dbInjector.createChildInjector(modules); From 93a4434a002e9996734a529b0865f4e72ded5e89 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 8 Sep 2011 19:03:07 -0700 Subject: [PATCH 21/33] Make WebSession an abstract interface This permits other alternative implementations of session management, such as a stateless token like Gerrit used to support, or an external product that provides user identity through other means, such as SSL peer certificates authenticated on every HTTP request. Change-Id: I43f315f0a19968010be3437d2a0baaab79834ac7 Signed-off-by: Shawn O. Pearce --- .../gerrit/httpd/CacheBasedWebSession.java | 218 ++++++++++++++++++ .../com/google/gerrit/httpd/WebModule.java | 2 - .../com/google/gerrit/httpd/WebSession.java | 188 +-------------- .../java/com/google/gerrit/pgm/Daemon.java | 2 + .../gerrit/httpd/WebAppInitializer.java | 1 + 5 files changed, 231 insertions(+), 180 deletions(-) create mode 100644 gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java new file mode 100644 index 0000000000..d27d68494a --- /dev/null +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java @@ -0,0 +1,218 @@ +// Copyright (C) 2009 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.google.gerrit.httpd; + +import static java.util.concurrent.TimeUnit.HOURS; + +import com.google.gerrit.httpd.WebSessionManager.Key; +import com.google.gerrit.httpd.WebSessionManager.Val; +import com.google.gerrit.reviewdb.Account; +import com.google.gerrit.reviewdb.AccountExternalId; +import com.google.gerrit.server.AccessPath; +import com.google.gerrit.server.AnonymousUser; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.AuthResult; +import com.google.gerrit.server.cache.Cache; +import com.google.gerrit.server.cache.CacheModule; +import com.google.gerrit.server.cache.EvictionPolicy; +import com.google.gerrit.server.config.AuthConfig; +import com.google.inject.Inject; +import com.google.inject.Module; +import com.google.inject.Provider; +import com.google.inject.TypeLiteral; +import com.google.inject.servlet.RequestScoped; + +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +@RequestScoped +public final class CacheBasedWebSession implements WebSession { + private static final String ACCOUNT_COOKIE = "GerritAccount"; + + public static Module module() { + return new CacheModule() { + @Override + protected void configure() { + final String cacheName = WebSessionManager.CACHE_NAME; + final TypeLiteral> type = + new TypeLiteral>() {}; + disk(type, cacheName) // + .memoryLimit(1024) // reasonable default for many sites + .maxAge(12, HOURS) // expire sessions if they are inactive + .evictionPolicy(EvictionPolicy.LRU) // keep most recently used + ; + bind(WebSessionManager.class); + bind(WebSession.class) + .to(CacheBasedWebSession.class) + .in(RequestScoped.class); + } + }; + } + + private final HttpServletRequest request; + private final HttpServletResponse response; + private final WebSessionManager manager; + private final AuthConfig authConfig; + private final Provider anonymousProvider; + private final IdentifiedUser.RequestFactory identified; + private AccessPath accessPath = AccessPath.WEB_UI; + private Cookie outCookie; + + private Key key; + private Val val; + + @Inject + CacheBasedWebSession(final HttpServletRequest request, + final HttpServletResponse response, final WebSessionManager manager, + final AuthConfig authConfig, + final Provider anonymousProvider, + final IdentifiedUser.RequestFactory identified) { + this.request = request; + this.response = response; + this.manager = manager; + this.authConfig = authConfig; + this.anonymousProvider = anonymousProvider; + this.identified = identified; + + final String cookie = readCookie(); + if (cookie != null) { + key = new Key(cookie); + val = manager.get(key); + } else { + key = null; + val = null; + } + + if (isSignedIn() && val.needsCookieRefresh()) { + // Cookie is more than half old. Send the cookie again to the + // client with an updated expiration date. We don't dare to + // change the key token here because there may be other RPCs + // queued up in the browser whose xsrfKey would not get updated + // with the new token, causing them to fail. + // + val = manager.createVal(key, val); + saveCookie(); + } + } + + private String readCookie() { + final Cookie[] all = request.getCookies(); + if (all != null) { + for (final Cookie c : all) { + if (ACCOUNT_COOKIE.equals(c.getName())) { + final String v = c.getValue(); + return v != null && !"".equals(v) ? v : null; + } + } + } + return null; + } + + public boolean isSignedIn() { + return val != null; + } + + public String getToken() { + return isSignedIn() ? val.getXsrfToken() : null; + } + + public boolean isTokenValid(final String inputToken) { + return isSignedIn() // + && val.getXsrfToken() != null // + && val.getXsrfToken().equals(inputToken); + } + + public AccountExternalId.Key getLastLoginExternalId() { + return val != null ? val.getExternalId() : null; + } + + public CurrentUser getCurrentUser() { + if (isSignedIn()) { + return identified.create(accessPath, val.getAccountId()); + } + return anonymousProvider.get(); + } + + public void login(final AuthResult res, final boolean rememberMe) { + final Account.Id id = res.getAccountId(); + final AccountExternalId.Key identity = res.getExternalId(); + + if (val != null) { + manager.destroy(key); + } + + key = manager.createKey(id); + val = manager.createVal(key, id, rememberMe, identity, null); + saveCookie(); + } + + /** Change the access path from the default of {@link AccessPath#WEB_UI}. */ + public void setAccessPath(AccessPath path) { + accessPath = path; + } + + /** Set the user account for this current request only. */ + public void setUserAccountId(Account.Id id) { + key = new Key("id:" + id); + val = new Val(id, 0, false, null, ""); + } + + public void logout() { + if (val != null) { + manager.destroy(key); + key = null; + val = null; + saveCookie(); + } + } + + private void saveCookie() { + final String token; + final int ageSeconds; + + if (key == null) { + token = ""; + ageSeconds = 0 /* erase at client */; + } else { + token = key.getToken(); + ageSeconds = manager.getCookieAge(val); + } + + String path = authConfig.getCookiePath(); + if (path == null || path.isEmpty()) { + path = request.getContextPath(); + if (path.isEmpty()) { + path = "/"; + } + } + + if (outCookie != null) { + throw new IllegalStateException("Cookie " + ACCOUNT_COOKIE + " was set"); + } + + outCookie = new Cookie(ACCOUNT_COOKIE, token); + outCookie.setSecure(isSecure(request)); + outCookie.setPath(path); + outCookie.setMaxAge(ageSeconds); + outCookie.setSecure(authConfig.getCookieSecure()); + response.addCookie(outCookie); + } + + private static boolean isSecure(final HttpServletRequest req) { + return req.isSecure() || "https".equals(req.getScheme()); + } +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java index 6895f49294..732cc3e03f 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java @@ -138,8 +138,6 @@ public class WebModule extends FactoryModule { bind(SocketAddress.class).annotatedWith(RemotePeer.class).toProvider( HttpRemotePeerProvider.class).in(RequestScoped.class); - install(WebSession.module()); - bind(CurrentUser.class).toProvider(HttpCurrentUserProvider.class).in( RequestScoped.class); bind(IdentifiedUser.class).toProvider(HttpIdentifiedUserProvider.class).in( diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java index d2fd6eebfa..869b8e7ed6 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java @@ -14,198 +14,30 @@ package com.google.gerrit.httpd; -import static java.util.concurrent.TimeUnit.HOURS; - -import com.google.gerrit.httpd.WebSessionManager.Key; -import com.google.gerrit.httpd.WebSessionManager.Val; import com.google.gerrit.reviewdb.Account; import com.google.gerrit.reviewdb.AccountExternalId; import com.google.gerrit.server.AccessPath; -import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.CurrentUser; -import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AuthResult; -import com.google.gerrit.server.cache.Cache; -import com.google.gerrit.server.cache.CacheModule; -import com.google.gerrit.server.cache.EvictionPolicy; -import com.google.gerrit.server.config.AuthConfig; -import com.google.inject.Inject; -import com.google.inject.Module; -import com.google.inject.Provider; -import com.google.inject.TypeLiteral; -import com.google.inject.servlet.RequestScoped; -import javax.servlet.http.Cookie; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; +public interface WebSession { + public boolean isSignedIn(); -@RequestScoped -public final class WebSession { - private static final String ACCOUNT_COOKIE = "GerritAccount"; + public String getToken(); - static Module module() { - return new CacheModule() { - @Override - protected void configure() { - final String cacheName = WebSessionManager.CACHE_NAME; - final TypeLiteral> type = - new TypeLiteral>() {}; - disk(type, cacheName) // - .memoryLimit(1024) // reasonable default for many sites - .maxAge(12, HOURS) // expire sessions if they are inactive - .evictionPolicy(EvictionPolicy.LRU) // keep most recently used - ; - bind(WebSessionManager.class); - bind(WebSession.class).in(RequestScoped.class); - } - }; - } + public boolean isTokenValid(String inputToken); - private final HttpServletRequest request; - private final HttpServletResponse response; - private final WebSessionManager manager; - private final AuthConfig authConfig; - private final Provider anonymousProvider; - private final IdentifiedUser.RequestFactory identified; - private AccessPath accessPath = AccessPath.WEB_UI; - private Cookie outCookie; + public AccountExternalId.Key getLastLoginExternalId(); - private Key key; - private Val val; + public CurrentUser getCurrentUser(); - @Inject - WebSession(final HttpServletRequest request, - final HttpServletResponse response, final WebSessionManager manager, - final AuthConfig authConfig, - final Provider anonymousProvider, - final IdentifiedUser.RequestFactory identified) { - this.request = request; - this.response = response; - this.manager = manager; - this.authConfig = authConfig; - this.anonymousProvider = anonymousProvider; - this.identified = identified; - - final String cookie = readCookie(); - if (cookie != null) { - key = new Key(cookie); - val = manager.get(key); - } else { - key = null; - val = null; - } - - if (isSignedIn() && val.needsCookieRefresh()) { - // Cookie is more than half old. Send the cookie again to the - // client with an updated expiration date. We don't dare to - // change the key token here because there may be other RPCs - // queued up in the browser whose xsrfKey would not get updated - // with the new token, causing them to fail. - // - val = manager.createVal(key, val); - saveCookie(); - } - } - - private String readCookie() { - final Cookie[] all = request.getCookies(); - if (all != null) { - for (final Cookie c : all) { - if (ACCOUNT_COOKIE.equals(c.getName())) { - final String v = c.getValue(); - return v != null && !"".equals(v) ? v : null; - } - } - } - return null; - } - - public boolean isSignedIn() { - return val != null; - } - - public String getToken() { - return isSignedIn() ? val.getXsrfToken() : null; - } - - public boolean isTokenValid(final String inputToken) { - return isSignedIn() // - && val.getXsrfToken() != null // - && val.getXsrfToken().equals(inputToken); - } - - public AccountExternalId.Key getLastLoginExternalId() { - return val != null ? val.getExternalId() : null; - } - - CurrentUser getCurrentUser() { - if (isSignedIn()) { - return identified.create(accessPath, val.getAccountId()); - } - return anonymousProvider.get(); - } - - public void login(final AuthResult res, final boolean rememberMe) { - final Account.Id id = res.getAccountId(); - final AccountExternalId.Key identity = res.getExternalId(); - - if (val != null) { - manager.destroy(key); - } - - key = manager.createKey(id); - val = manager.createVal(key, id, rememberMe, identity, null); - saveCookie(); - } + public void login(AuthResult res, boolean rememberMe); /** Change the access path from the default of {@link AccessPath#WEB_UI}. */ - void setAccessPath(AccessPath path) { - accessPath = path; - } + public void setAccessPath(AccessPath path); /** Set the user account for this current request only. */ - void setUserAccountId(Account.Id id) { - key = new Key("id:" + id); - val = new Val(id, 0, false, null, ""); - } + public void setUserAccountId(Account.Id id); - public void logout() { - if (val != null) { - manager.destroy(key); - key = null; - val = null; - saveCookie(); - } - } - - private void saveCookie() { - final String token; - final int ageSeconds; - - if (key == null) { - token = ""; - ageSeconds = 0 /* erase at client */; - } else { - token = key.getToken(); - ageSeconds = manager.getCookieAge(val); - } - - String path = authConfig.getCookiePath(); - if (path == null || path.isEmpty()) { - path = request.getContextPath(); - if (path.isEmpty()) { - path = "/"; - } - } - - if (outCookie != null) { - throw new IllegalStateException("Cookie " + ACCOUNT_COOKIE + " was set"); - } - - outCookie = new Cookie(ACCOUNT_COOKIE, token); - outCookie.setPath(path); - outCookie.setMaxAge(ageSeconds); - outCookie.setSecure(authConfig.getCookieSecure()); - response.addCookie(outCookie); - } + public void logout(); } diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 8008b92002..9125e29d2b 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -16,6 +16,7 @@ package com.google.gerrit.pgm; import static com.google.gerrit.server.schema.DataSourceProvider.Context.MULTI_USER; +import com.google.gerrit.httpd.CacheBasedWebSession; import com.google.gerrit.httpd.HttpCanonicalWebUrlProvider; import com.google.gerrit.httpd.WebModule; import com.google.gerrit.httpd.WebSshGlueModule; @@ -245,6 +246,7 @@ public class Daemon extends SiteProgram { final List modules = new ArrayList(); modules.add(sshInjector.getInstance(WebModule.class)); modules.add(sshInjector.getInstance(WebSshGlueModule.class)); + modules.add(CacheBasedWebSession.module()); if (sshd) { modules.add(sshInjector.getInstance(ProjectQoSFilter.Module.class)); } diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index 988ce4aac5..979bc6b522 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -202,6 +202,7 @@ public class WebAppInitializer extends GuiceServletContextListener { final List modules = new ArrayList(); modules.add(sshInjector.getInstance(WebModule.class)); modules.add(sshInjector.getInstance(WebSshGlueModule.class)); + modules.add(CacheBasedWebSession.module()); return sysInjector.createChildInjector(modules); } From f522b24a1fc771fb40265fe803d14270481ccde0 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 9 Sep 2011 19:12:52 -0700 Subject: [PATCH 22/33] Extract Git /p/ module configuration Move the configuration into its own module, this will make it easier to control whether or not the Git access over HTTP is even installed in a running server, or how it permits access. Change-Id: I54074dbac96d47f32d444565993823f54348d722 Signed-off-by: Shawn O. Pearce --- .../gerrit/httpd/GitOverHttpModule.java | 42 +++++++++++++++++++ .../gerrit/httpd/ProjectAccessPathFilter.java | 2 +- .../com/google/gerrit/httpd/UrlModule.java | 15 ------- .../com/google/gerrit/httpd/WebModule.java | 2 +- .../java/com/google/gerrit/pgm/Daemon.java | 2 + .../gerrit/httpd/WebAppInitializer.java | 1 + 6 files changed, 47 insertions(+), 17 deletions(-) create mode 100644 gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java new file mode 100644 index 0000000000..9b9ec86a6d --- /dev/null +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java @@ -0,0 +1,42 @@ +// Copyright (C) 2009 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.google.gerrit.httpd; + +import com.google.gerrit.server.config.AuthConfig; +import com.google.inject.Inject; +import com.google.inject.servlet.ServletModule; + +/** + * Configures Git access over {@code "/p/PROJECT_NAME"} URLs. + */ +public class GitOverHttpModule extends ServletModule { + private final AuthConfig authConfig; + + @Inject + GitOverHttpModule(AuthConfig authConfig) { + this.authConfig = authConfig; + } + + @Override + protected void configureServlets() { + filter("/p/*").through(ProjectAccessPathFilter.class); + if (authConfig.isTrustContainerAuth()) { + filter("/p/*").through(ContainerAuthFilter.class); + } else { + filter("/p/*").through(ProjectDigestFilter.class); + } + serve("/p/*").with(ProjectServlet.class); + } +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectAccessPathFilter.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectAccessPathFilter.java index 934c4a5866..70ed153987 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectAccessPathFilter.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectAccessPathFilter.java @@ -30,7 +30,7 @@ import javax.servlet.ServletResponse; /** Set the WebSession to {@link AccessPath#GIT}. */ @Singleton -class ProjectAccessPathFilter implements Filter { +public class ProjectAccessPathFilter implements Filter { private final Provider session; @Inject diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java index 8d6d68453b..6c145a27c3 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java @@ -24,7 +24,6 @@ import com.google.gerrit.httpd.raw.SshInfoServlet; import com.google.gerrit.httpd.raw.StaticServlet; import com.google.gerrit.httpd.raw.ToolServlet; import com.google.gerrit.reviewdb.Change; -import com.google.gerrit.server.config.AuthConfig; import com.google.gwtexpui.server.CacheControlFilter; import com.google.inject.Key; import com.google.inject.Provider; @@ -38,12 +37,6 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; class UrlModule extends ServletModule { - private final AuthConfig authConfig; - - public UrlModule(AuthConfig authConfig) { - this.authConfig = authConfig; - } - @Override protected void configureServlets() { filter("/*").through(Key.get(CacheControlFilter.class)); @@ -60,14 +53,6 @@ class UrlModule extends ServletModule { serve("/static/*").with(StaticServlet.class); serve("/tools/*").with(ToolServlet.class); - filter("/p/*").through(ProjectAccessPathFilter.class); - if (authConfig.isTrustContainerAuth()) { - filter("/p/*").through(ContainerAuthFilter.class); - } else { - filter("/p/*").through(ProjectDigestFilter.class); - } - serve("/p/*").with(ProjectServlet.class); - serve("/Main.class").with(notFound()); serve("/com/google/gerrit/launcher/*").with(notFound()); serve("/servlet/*").with(notFound()); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java index 732cc3e03f..09e3f29dbd 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java @@ -114,7 +114,7 @@ public class WebModule extends FactoryModule { throw new ProvisionException("Unsupported loginType: " + authConfig.getAuthType()); } - install(new UrlModule(authConfig)); + install(new UrlModule()); install(new UiRpcModule()); install(new GerritRequestModule()); install(new ProjectServlet.Module()); diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 9125e29d2b..95a5d541ce 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -17,6 +17,7 @@ package com.google.gerrit.pgm; import static com.google.gerrit.server.schema.DataSourceProvider.Context.MULTI_USER; import com.google.gerrit.httpd.CacheBasedWebSession; +import com.google.gerrit.httpd.GitOverHttpModule; import com.google.gerrit.httpd.HttpCanonicalWebUrlProvider; import com.google.gerrit.httpd.WebModule; import com.google.gerrit.httpd.WebSshGlueModule; @@ -245,6 +246,7 @@ public class Daemon extends SiteProgram { private Injector createWebInjector() { final List modules = new ArrayList(); modules.add(sshInjector.getInstance(WebModule.class)); + modules.add(sysInjector.getInstance(GitOverHttpModule.class)); modules.add(sshInjector.getInstance(WebSshGlueModule.class)); modules.add(CacheBasedWebSession.module()); if (sshd) { diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index 979bc6b522..dc10141a0e 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -201,6 +201,7 @@ public class WebAppInitializer extends GuiceServletContextListener { private Injector createWebInjector() { final List modules = new ArrayList(); modules.add(sshInjector.getInstance(WebModule.class)); + modules.add(sysInjector.getInstance(GitOverHttpModule.class)); modules.add(sshInjector.getInstance(WebSshGlueModule.class)); modules.add(CacheBasedWebSession.module()); return sysInjector.createChildInjector(modules); From 7bec78aa47a0f2bfd5b1bf35bd41223b52e197b3 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 9 Sep 2011 10:10:39 -0700 Subject: [PATCH 23/33] Support auth.type = CUSTOM_EXTENSION This new auth type configures the web UI similar to the way HTTP works, allowing the UI to redirect sign-in requests through the /login/* URL and sign-out requests through /logout. No URL handlers are installed in the application for this type of authentication system. Instead it is assumed additional code has been injected into the same environment via Guice to handle the authentication. This is currently a very advanced usage of Gerrit Code Review's server and is not recommended for most site administrators, so the type is not documented at this time. This change is a first step towards supporting other types of user authentication, with the idea of eventually having a plugin system to permit other third party authenticators. Change-Id: I95fcbfc6f486513f7c7105a1b7005ab78b1f4073 Signed-off-by: Shawn O. Pearce --- .../gerrit/common/data/GerritConfig.java | 9 +++++++++ .../java/com/google/gerrit/client/Gerrit.java | 2 ++ .../client/account/AccountConstants.java | 1 + .../client/account/AccountConstants.properties | 1 + .../client/account/MyPasswordScreen.java | 18 +++++++++++++++++- .../gerrit/httpd/GerritConfigProvider.java | 5 +++++ .../com/google/gerrit/httpd/WebModule.java | 2 ++ .../com/google/gerrit/reviewdb/AuthType.java | 3 +++ .../gerrit/server/account/DefaultRealm.java | 2 +- .../gerrit/server/config/AuthConfig.java | 1 + .../server/config/GerritGlobalModule.java | 3 +++ 11 files changed, 45 insertions(+), 2 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java index c316ba9127..c4533306bb 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java @@ -26,6 +26,7 @@ import java.util.Set; public class GerritConfig implements Cloneable { protected String registerUrl; + protected String httpPasswordUrl; protected List allowedOpenIDs; protected GitwebLink gitweb; @@ -52,6 +53,14 @@ public class GerritConfig implements Cloneable { registerUrl = u; } + public String getHttpPasswordUrl() { + return httpPasswordUrl; + } + + public void setHttpPasswordUrl(String url) { + httpPasswordUrl = url; + } + public List getAllowedOpenIDs() { return allowedOpenIDs; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java index 40bf7c4b2d..f605d610aa 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java @@ -246,6 +246,7 @@ public class Gerrit implements EntryPoint { case HTTP: case HTTP_LDAP: case CLIENT_SSL_CERT_LDAP: + case CUSTOM_EXTENSION: if (!token.startsWith("/")) { token = "/" + token; } @@ -563,6 +564,7 @@ public class Gerrit implements EntryPoint { case LDAP: case LDAP_BIND: + case CUSTOM_EXTENSION: if (cfg.getRegisterUrl() != null) { menuRight.add(anchor(C.menuRegister(), cfg.getRegisterUrl())); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java index 53a2b58495..c886216778 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java @@ -56,6 +56,7 @@ public interface AccountConstants extends Constants { String buttonChangeUserName(); String buttonClearPassword(); String buttonGeneratePassword(); + String linkObtainPassword(); String invalidUserName(); String invalidUserEmail(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties index ffc521878d..499f051b9a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties @@ -37,6 +37,7 @@ buttonSetUserName = Select Username buttonChangeUserName = Change Username buttonClearPassword = Clear Password buttonGeneratePassword = Generate Password +linkObtainPassword = Obtain Password invalidUserName = Username must contain only letters, numbers, _, - or . invalidUserEmail = Email format is wrong. sshKeyInvalid = Invalid Key diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java index 2202ac88c0..65525ebcda 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java @@ -23,11 +23,12 @@ import com.google.gerrit.reviewdb.AccountExternalId; import com.google.gwt.event.dom.client.ClickEvent; import com.google.gwt.event.dom.client.ClickHandler; import com.google.gwt.i18n.client.LocaleInfo; +import com.google.gwt.user.client.ui.Anchor; import com.google.gwt.user.client.ui.Button; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.Grid; -import com.google.gwt.user.client.ui.Widget; import com.google.gwt.user.client.ui.HTMLTable.CellFormatter; +import com.google.gwt.user.client.ui.Widget; import com.google.gwtexpui.clippy.client.CopyableLabel; import java.util.List; @@ -42,6 +43,16 @@ public class MyPasswordScreen extends SettingsScreen { protected void onInitUI() { super.onInitUI(); + String url = Gerrit.getConfig().getHttpPasswordUrl(); + if (url != null) { + Anchor link = new Anchor(); + link.setText(Util.C.linkObtainPassword()); + link.setHref(url); + link.setTarget("_blank"); + add(link); + return; + } + password = new CopyableLabel(""); password.addStyleName(Gerrit.RESOURCES.css().accountPassword()); @@ -84,6 +95,11 @@ public class MyPasswordScreen extends SettingsScreen { protected void onLoad() { super.onLoad(); + if (password == null) { + display(); + return; + } + enableUI(false); Util.ACCOUNT_SEC .myExternalIds(new ScreenLoadCallback>(this) { diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java index 6cd5973f3a..d1a2f72f13 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java @@ -91,6 +91,11 @@ class GerritConfigProvider implements Provider { case LDAP_BIND: config.setRegisterUrl(cfg.getString("auth", null, "registerurl")); break; + + case CUSTOM_EXTENSION: + config.setRegisterUrl(cfg.getString("auth", null, "registerurl")); + config.setHttpPasswordUrl(cfg.getString("auth", null, "httpPasswordUrl")); + break; } config.setUseContributorAgreements(cfg.getBoolean("auth", "contributoragreements", false)); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java index 09e3f29dbd..bdadff564a 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java @@ -110,6 +110,8 @@ public class WebModule extends FactoryModule { }); break; + case CUSTOM_EXTENSION: + break; default: throw new ProvisionException("Unsupported loginType: " + authConfig.getAuthType()); } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AuthType.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AuthType.java index 5d69e21da8..843f41bf0c 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AuthType.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AuthType.java @@ -73,6 +73,9 @@ public enum AuthType { */ LDAP_BIND, + /** Login is managed by additional, unspecified code. */ + CUSTOM_EXTENSION, + /** Development mode to enable becoming anyone you want. */ DEVELOPMENT_BECOME_ANY_ACCOUNT; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java index 77afb13e0c..2f270a9ecd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java @@ -21,7 +21,7 @@ import com.google.inject.Inject; import java.util.Collections; import java.util.Set; -public final class DefaultRealm implements Realm { +public class DefaultRealm implements Realm { private final EmailExpander emailExpander; private final AccountByEmailCache byEmail; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/AuthConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/AuthConfig.java index f1a932856b..88639b7a70 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/AuthConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/AuthConfig.java @@ -140,6 +140,7 @@ public class AuthConfig { case LDAP: case LDAP_BIND: case CLIENT_SSL_CERT_LDAP: + case CUSTOM_EXTENSION: // Its safe to assume yes for an HTTP authentication type, as the // only way in is through some external system that the admin trusts // diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 0076d3cade..2b7f7691be 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -127,6 +127,9 @@ public class GerritGlobalModule extends FactoryModule { install(new LdapModule()); break; + case CUSTOM_EXTENSION: + break; + default: bind(Realm.class).to(DefaultRealm.class); break; From 4ec33a2138e19561d0f973985c7c0e34be5dea45 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 7 Oct 2011 18:09:57 -0700 Subject: [PATCH 24/33] Move WorkQueue out of GerritGlobalModule This makes it easier to better manage the thread queue within the JVM, declare it once at the "top level" makes it less likely we bound more than one work queue into the same JVM, creating too many threads. Change-Id: I22d6b98e925e2d712193af02aff94c260f1395e7 --- .../src/main/java/com/google/gerrit/pgm/Daemon.java | 2 ++ .../google/gerrit/server/config/GerritGlobalModule.java | 3 --- .../java/com/google/gerrit/server/git/WorkQueue.java | 9 +++++++++ .../java/com/google/gerrit/httpd/WebAppInitializer.java | 2 ++ 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 95a5d541ce..63c7b5caaa 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -34,6 +34,7 @@ import com.google.gerrit.server.config.CanonicalWebUrlModule; import com.google.gerrit.server.config.CanonicalWebUrlProvider; import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.MasterNodeStartup; +import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.schema.SchemaVersionCheck; import com.google.gerrit.server.ssh.NoSshModule; @@ -189,6 +190,7 @@ public class Daemon extends SiteProgram { final List modules = new ArrayList(); modules.add(SchemaVersionCheck.module()); modules.add(new LogFileCompressor.Module()); + modules.add(new WorkQueue.Module()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new SmtpEmailSender.Module()); if (httpd) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 2b7f7691be..86381e4e8a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -49,7 +49,6 @@ import com.google.gerrit.server.git.ReplicationQueue; import com.google.gerrit.server.git.SecureCredentialsProvider; import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.TransferConfig; -import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.mail.FromAddressGenerator; import com.google.gerrit.server.mail.FromAddressGeneratorProvider; import com.google.gerrit.server.patch.PatchListCacheImpl; @@ -162,7 +161,6 @@ public class GerritGlobalModule extends FactoryModule { bind(PermissionCollection.Factory.class); bind(FileTypeRegistry.class).to(MimeUtilFileTypeRegistry.class); - bind(WorkQueue.class); bind(ToolsCatalog.class); bind(EventFactory.class); bind(TransferConfig.class); @@ -188,7 +186,6 @@ public class GerritGlobalModule extends FactoryModule { @Override protected void configure() { listener().to(CachePool.Lifecycle.class); - listener().to(WorkQueue.Lifecycle.class); listener().to(VelocityLifecycle.class); } }); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java index 14eacca238..87e688086a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.git; import com.google.gerrit.lifecycle.LifecycleListener; +import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.reviewdb.Project.NameKey; import com.google.gerrit.server.util.IdGenerator; import com.google.inject.Inject; @@ -61,6 +62,14 @@ public class WorkQueue { } } + public static class Module extends LifecycleModule { + @Override + protected void configure() { + bind(WorkQueue.class); + listener().to(Lifecycle.class); + } + } + private static final Logger log = LoggerFactory.getLogger(WorkQueue.class); private static final UncaughtExceptionHandler LOG_UNCAUGHT_EXCEPTION = new UncaughtExceptionHandler() { diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index dc10141a0e..24e58866d0 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -26,6 +26,7 @@ import com.google.gerrit.server.config.GerritServerConfigModule; import com.google.gerrit.server.config.MasterNodeStartup; import com.google.gerrit.server.config.SitePath; import com.google.gerrit.server.git.LocalDiskRepositoryManager; +import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.schema.DataSourceProvider; import com.google.gerrit.server.schema.DatabaseModule; @@ -179,6 +180,7 @@ public class WebAppInitializer extends GuiceServletContextListener { private Injector createSysInjector() { final List modules = new ArrayList(); + modules.add(new WorkQueue.Module()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new SmtpEmailSender.Module()); modules.add(new CanonicalWebUrlModule() { From b92b81371b75052a4465602430a3ba044a8fe4f5 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 7 Oct 2011 18:25:11 -0700 Subject: [PATCH 25/33] Remove static initialization of Velocity Use a RuntimeInstance that is passed through Guice, rather than relying on the static singleton. This makes configuration easier as we could later work with different templates that aren't in the mail directory/namespace by setting up a different RuntimeInstance. Change-Id: Ib198a40bfe0f8028f7e65e8e021ae1f7d544d608 --- .../server/config/GerritGlobalModule.java | 45 ++----------- .../gerrit/server/mail/EmailArguments.java | 9 +-- .../gerrit/server/mail/OutgoingEmail.java | 28 ++++++--- .../server/mail/VelocityRuntimeProvider.java | 63 +++++++++++++++++++ 4 files changed, 91 insertions(+), 54 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/mail/VelocityRuntimeProvider.java diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 86381e4e8a..1f801ea3cf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -17,7 +17,6 @@ package com.google.gerrit.server.config; import static com.google.inject.Scopes.SINGLETON; import com.google.gerrit.common.data.ApprovalTypes; -import com.google.gerrit.lifecycle.LifecycleListener; import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.reviewdb.AuthType; import com.google.gerrit.rules.PrologModule; @@ -51,6 +50,7 @@ import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.TransferConfig; import com.google.gerrit.server.mail.FromAddressGenerator; import com.google.gerrit.server.mail.FromAddressGeneratorProvider; +import com.google.gerrit.server.mail.VelocityRuntimeProvider; import com.google.gerrit.server.patch.PatchListCacheImpl; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.AccessControlModule; @@ -65,51 +65,14 @@ import com.google.gerrit.server.util.IdGenerator; import com.google.gerrit.server.workflow.FunctionState; import com.google.inject.Inject; -import org.apache.velocity.app.Velocity; -import org.apache.velocity.runtime.RuntimeConstants; +import org.apache.velocity.runtime.RuntimeInstance; import org.eclipse.jgit.lib.Config; -import java.util.Properties; - /** Starts global state with standard dependencies. */ public class GerritGlobalModule extends FactoryModule { private final AuthType loginType; - public static class VelocityLifecycle implements LifecycleListener { - private final SitePaths site; - - @Inject - VelocityLifecycle(final SitePaths site) { - this.site = site; - } - - @Override - public void start() { - String rl = "resource.loader"; - String pkg = "org.apache.velocity.runtime.resource.loader"; - Properties p = new Properties(); - - p.setProperty(rl, "file, class"); - p.setProperty("file." + rl + ".class", pkg + ".FileResourceLoader"); - p.setProperty("file." + rl + ".path", site.mail_dir.getAbsolutePath()); - p.setProperty("class." + rl + ".class", pkg + ".ClasspathResourceLoader"); - p.setProperty(RuntimeConstants.RUNTIME_LOG_LOGSYSTEM_CLASS, - "org.apache.velocity.runtime.log.SimpleLog4JLogSystem" ); - p.setProperty("runtime.log.logsystem.log4j.category", "velocity"); - - try { - Velocity.init(p); - } catch(Exception e) { - throw new RuntimeException(e); - } - } - - @Override - public void stop() { - } - } - @Inject GerritGlobalModule(final AuthConfig authConfig, @GerritServerConfig final Config config) { @@ -172,6 +135,9 @@ public class GerritGlobalModule extends FactoryModule { bind(MergeQueue.class).to(ChangeMergeQueue.class).in(SINGLETON); factory(ReloadSubmitQueueOp.Factory.class); + bind(RuntimeInstance.class) + .toProvider(VelocityRuntimeProvider.class) + .in(SINGLETON); bind(FromAddressGenerator.class).toProvider( FromAddressGeneratorProvider.class).in(SINGLETON); @@ -186,7 +152,6 @@ public class GerritGlobalModule extends FactoryModule { @Override protected void configure() { listener().to(CachePool.Lifecycle.class); - listener().to(VelocityLifecycle.class); } }); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java index ae4aff0bbf..15f6846c60 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java @@ -21,7 +21,6 @@ import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.CanonicalWebUrl; -import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchSetInfoFactory; @@ -31,6 +30,8 @@ import com.google.gerrit.server.query.change.ChangeQueryRewriter; import com.google.inject.Inject; import com.google.inject.Provider; +import org.apache.velocity.runtime.RuntimeInstance; + import javax.annotation.Nullable; class EmailArguments { @@ -49,7 +50,7 @@ class EmailArguments { final ChangeQueryBuilder.Factory queryBuilder; final Provider queryRewriter; final Provider db; - final SitePaths site; + final RuntimeInstance velocityRuntime; @Inject EmailArguments(GitRepositoryManager server, ProjectCache projectCache, @@ -61,7 +62,7 @@ class EmailArguments { AllProjectsName allProjectsName, ChangeQueryBuilder.Factory queryBuilder, Provider queryRewriter, Provider db, - SitePaths site) { + RuntimeInstance velocityRuntime) { this.server = server; this.projectCache = projectCache; this.groupCache = groupCache; @@ -76,6 +77,6 @@ class EmailArguments { this.queryBuilder = queryBuilder; this.queryRewriter = queryRewriter; this.db = db; - this.site = site; + this.velocityRuntime = velocityRuntime; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java index e6a1ebcf75..c4b837cbb4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java @@ -20,8 +20,9 @@ import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.mail.EmailHeader.AddressList; import org.apache.commons.lang.StringUtils; +import org.apache.velocity.Template; import org.apache.velocity.VelocityContext; -import org.apache.velocity.app.Velocity; +import org.apache.velocity.runtime.RuntimeInstance; import org.eclipse.jgit.util.SystemReader; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -346,23 +347,30 @@ public abstract class OutgoingEmail { protected String velocify(String template) throws EmailException { try { StringWriter w = new StringWriter(); - Velocity.evaluate(velocityContext, w, "OutgoingEmail", template); + args.velocityRuntime.evaluate(velocityContext, w, "OutgoingEmail", template); return w.toString(); - } catch(Exception e) { - throw new EmailException("Velocity template " + template, e); + } catch (Exception e) { + throw new EmailException("Cannot format velocity template: " + template, e); } } protected String velocifyFile(String name) throws EmailException { - if (!Velocity.resourceExists(name)) { - name = "com/google/gerrit/server/mail/" + name; - } try { + RuntimeInstance runtime = args.velocityRuntime; + Template template; + try { + template = runtime.getTemplate(name, "UTF-8"); + } catch (org.apache.velocity.exception.ResourceNotFoundException notFound) { + name = "com/google/gerrit/server/mail/" + name; + template = runtime.getTemplate(name, "UTF-8"); + } StringWriter w = new StringWriter(); - Velocity.mergeTemplate(name, "UTF-8", velocityContext, w); + template.merge(velocityContext, w); return w.toString(); - } catch(Exception e) { - throw new EmailException("Velocity template " + name + ".\n", e); + } catch (EmailException e) { + throw e; + } catch (Exception e) { + throw new EmailException("Cannot format velocity template " + name, e); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/VelocityRuntimeProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/VelocityRuntimeProvider.java new file mode 100644 index 0000000000..3be9dc9ee2 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/VelocityRuntimeProvider.java @@ -0,0 +1,63 @@ +// Copyright (C) 2011 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.google.gerrit.server.mail; + +import com.google.gerrit.server.config.SitePaths; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.ProvisionException; + +import org.apache.velocity.runtime.RuntimeConstants; +import org.apache.velocity.runtime.RuntimeInstance; + +import java.util.Properties; + +/** Configures Velocity template engine for sending email. */ +public class VelocityRuntimeProvider implements Provider { + private final SitePaths site; + + @Inject + VelocityRuntimeProvider(SitePaths site) { + this.site = site; + } + + public RuntimeInstance get() { + String rl = "resource.loader"; + String pkg = "org.apache.velocity.runtime.resource.loader"; + + Properties p = new Properties(); + p.setProperty(RuntimeConstants.RUNTIME_LOG_LOGSYSTEM_CLASS, + "org.apache.velocity.runtime.log.SimpleLog4JLogSystem" ); + p.setProperty("runtime.log.logsystem.log4j.category", "velocity"); + + if (site.mail_dir.isDirectory()) { + p.setProperty(rl, "file, class"); + p.setProperty("file." + rl + ".class", pkg + ".FileResourceLoader"); + p.setProperty("file." + rl + ".path", site.mail_dir.getAbsolutePath()); + p.setProperty("class." + rl + ".class", pkg + ".ClasspathResourceLoader"); + } else { + p.setProperty(rl, "class"); + p.setProperty("class." + rl + ".class", pkg + ".ClasspathResourceLoader"); + } + + RuntimeInstance ri = new RuntimeInstance(); + try { + ri.init(p); + } catch (Exception err) { + throw new ProvisionException("Cannot configure Velocity templates", err); + } + return ri; + } +} From f16431b7b4df72e3df95ccb79f7b9186de2ad561 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 18 Oct 2011 12:42:40 -0700 Subject: [PATCH 26/33] Move replication queue binding out of GerritGlobalModule Handling replication by push over Git might not be the best strategy for all installations. Allow this binding to be managed at the daemon or webapp container level. Change-Id: I090e03a7b04a04489df0b2a7d45f333b65d50b45 --- gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java | 2 ++ .../google/gerrit/server/config/GerritGlobalModule.java | 3 --- .../java/com/google/gerrit/server/git/PushReplication.java | 7 +++++++ .../java/com/google/gerrit/httpd/WebAppInitializer.java | 2 ++ 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 63c7b5caaa..5a29044e18 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -34,6 +34,7 @@ import com.google.gerrit.server.config.CanonicalWebUrlModule; import com.google.gerrit.server.config.CanonicalWebUrlProvider; import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.MasterNodeStartup; +import com.google.gerrit.server.git.PushReplication; import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.schema.SchemaVersionCheck; @@ -193,6 +194,7 @@ public class Daemon extends SiteProgram { modules.add(new WorkQueue.Module()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new SmtpEmailSender.Module()); + modules.add(new PushReplication.Module()); if (httpd) { modules.add(new CanonicalWebUrlModule() { @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 1f801ea3cf..227f4a71f0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -42,9 +42,7 @@ import com.google.gerrit.server.git.ChangeMergeQueue; import com.google.gerrit.server.git.GitModule; import com.google.gerrit.server.git.MergeQueue; import com.google.gerrit.server.git.PushAllProjectsOp; -import com.google.gerrit.server.git.PushReplication; import com.google.gerrit.server.git.ReloadSubmitQueueOp; -import com.google.gerrit.server.git.ReplicationQueue; import com.google.gerrit.server.git.SecureCredentialsProvider; import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.TransferConfig; @@ -128,7 +126,6 @@ public class GerritGlobalModule extends FactoryModule { bind(EventFactory.class); bind(TransferConfig.class); - bind(ReplicationQueue.class).to(PushReplication.class).in(SINGLETON); factory(SecureCredentialsProvider.Factory.class); factory(PushAllProjectsOp.Factory.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/PushReplication.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/PushReplication.java index a3d327a7fd..d359a673ad 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/PushReplication.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/PushReplication.java @@ -72,6 +72,13 @@ import java.util.concurrent.TimeUnit; public class PushReplication implements ReplicationQueue { static final Logger log = LoggerFactory.getLogger(PushReplication.class); + public static class Module extends AbstractModule { + @Override + protected void configure() { + bind(ReplicationQueue.class).to(PushReplication.class); + } + } + static { // Install our own factory which always runs in batch mode, as we // have no UI available for interactive prompting. diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index 24e58866d0..89581a1750 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -26,6 +26,7 @@ import com.google.gerrit.server.config.GerritServerConfigModule; import com.google.gerrit.server.config.MasterNodeStartup; import com.google.gerrit.server.config.SitePath; import com.google.gerrit.server.git.LocalDiskRepositoryManager; +import com.google.gerrit.server.git.PushReplication; import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.mail.SmtpEmailSender; import com.google.gerrit.server.schema.DataSourceProvider; @@ -183,6 +184,7 @@ public class WebAppInitializer extends GuiceServletContextListener { modules.add(new WorkQueue.Module()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new SmtpEmailSender.Module()); + modules.add(new PushReplication.Module()); modules.add(new CanonicalWebUrlModule() { @Override protected Class> provider() { From 493a7acaa8fd173b5f5896517b2f528af592fcc4 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 10 Oct 2011 12:44:27 -0700 Subject: [PATCH 27/33] Support Velocity 1.5 This is a tiny backport to enable use of Velocity 1.5 rather than 1.6.4. The change is necessary to run under some servlet containers that are incorrectly loading the wrong Velocity binary into the application's classpath. Change-Id: Ibbb83354cb04994ec347bb0b23c0cdfcf9b03d28 --- .../gerrit/server/mail/OutgoingEmail.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java index c4b837cbb4..2aad489f78 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java @@ -22,11 +22,14 @@ import com.google.gerrit.server.mail.EmailHeader.AddressList; import org.apache.commons.lang.StringUtils; import org.apache.velocity.Template; import org.apache.velocity.VelocityContext; +import org.apache.velocity.context.InternalContextAdapterImpl; import org.apache.velocity.runtime.RuntimeInstance; +import org.apache.velocity.runtime.parser.node.SimpleNode; import org.eclipse.jgit.util.SystemReader; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.StringReader; import java.io.StringWriter; import java.net.MalformedURLException; import java.net.URL; @@ -346,9 +349,19 @@ public abstract class OutgoingEmail { protected String velocify(String template) throws EmailException { try { - StringWriter w = new StringWriter(); - args.velocityRuntime.evaluate(velocityContext, w, "OutgoingEmail", template); - return w.toString(); + RuntimeInstance runtime = args.velocityRuntime; + String templateName = "OutgoingEmail"; + SimpleNode tree = runtime.parse(new StringReader(template), templateName); + InternalContextAdapterImpl ica = new InternalContextAdapterImpl(velocityContext); + ica.pushCurrentTemplateName(templateName); + try { + tree.init(ica, runtime); + StringWriter w = new StringWriter(); + tree.render(ica, w); + return w.toString(); + } finally { + ica.popCurrentTemplateName(); + } } catch (Exception e) { throw new EmailException("Cannot format velocity template: " + template, e); } From e03119b1adc3436230b11b8bd01462fadd8f47ce Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 19 Oct 2011 17:00:58 -0700 Subject: [PATCH 28/33] Fix reference of Database to SchemaFactory Change-Id: Ia1444857c00c532a65d2ca7908420b72ad882d5d --- .../gerrit/server/config/RequestScopedReviewDbProvider.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/RequestScopedReviewDbProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/RequestScopedReviewDbProvider.java index 5aa78cbb2d..9c3da74885 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/RequestScopedReviewDbProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/RequestScopedReviewDbProvider.java @@ -17,7 +17,7 @@ package com.google.gerrit.server.config; import com.google.gerrit.reviewdb.ReviewDb; import com.google.gerrit.server.RequestCleanup; import com.google.gwtorm.client.OrmException; -import com.google.gwtorm.jdbc.Database; +import com.google.gwtorm.client.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.ProvisionException; @@ -26,11 +26,11 @@ import com.google.inject.Singleton; /** Provides {@link ReviewDb} database handle live only for this request. */ @Singleton final class RequestScopedReviewDbProvider implements Provider { - private final Database schema; + private final SchemaFactory schema; private final Provider cleanup; @Inject - RequestScopedReviewDbProvider(final Database schema, + RequestScopedReviewDbProvider(final SchemaFactory schema, final Provider cleanup) { this.schema = schema; this.cleanup = cleanup; From ae122ef21d2d6c1434b93184de598fae5010dea4 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 19 Oct 2011 09:15:01 -0700 Subject: [PATCH 29/33] Support gwtorm 1.2 In 1.2 gwtorm requires accessor @Query methods to be unique names within the interface. This means the arguments must not be overloaded, and required fixing up a few interfaces that relied on method overloading to select the right query statement. 1.2 also requires a unique id number for each relation in the schema interface, similar to how earlier versions have a unique column number in each entity. These can be used by NoSQL based systems for unique identity, specially the protobuf backend that exists to support the NoSQL backends of gwtorm. Change-Id: I19421b966e336ee0e89e6743d79e989860d8c470 --- .../changedetail/PatchSetDetailFactory.java | 4 +- .../PatchSetPublishDetailFactory.java | 2 +- .../httpd/rpc/patch/PatchScriptFactory.java | 4 +- .../reviewdb/PatchLineCommentAccess.java | 16 +++--- .../com/google/gerrit/reviewdb/ReviewDb.java | 54 +++++++++---------- .../gerrit/server/patch/PublishComments.java | 2 +- pom.xml | 2 +- 7 files changed, 44 insertions(+), 40 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java index f478d667a6..78f4ded474 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java @@ -128,7 +128,7 @@ class PatchSetDetailFactory extends Handler { byKey.put(p.getKey(), p); } - for (final PatchLineComment c : db.patchComments().published(psIdNew)) { + for (final PatchLineComment c : db.patchComments().publishedByPatchSet(psIdNew)) { final Patch p = byKey.get(c.getKey().getParentKey()); if (p != null) { p.setCommentCount(p.getCommentCount() + 1); @@ -148,7 +148,7 @@ class PatchSetDetailFactory extends Handler { // quickly locate where they have pending drafts, and review them. // final Account.Id me = ((IdentifiedUser) user).getAccountId(); - for (final PatchLineComment c : db.patchComments().draft(psIdNew, me)) { + for (final PatchLineComment c : db.patchComments().draftByPatchSetAuthor(psIdNew, me)) { final Patch p = byKey.get(c.getKey().getParentKey()); if (p != null) { p.setDraftCount(p.getDraftCount() + 1); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java index cbddad1e71..8ec1549b8c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java @@ -84,7 +84,7 @@ final class PatchSetPublishDetailFactory extends Handler final ChangeControl control = changeControlFactory.validateFor(changeId); change = control.getChange(); patchSetInfo = infoFactory.get(patchSetId); - drafts = db.patchComments().draft(patchSetId, user.getAccountId()).toList(); + drafts = db.patchComments().draftByPatchSetAuthor(patchSetId, user.getAccountId()).toList(); aic.want(change.getOwner()); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java index 9ad8e3aeeb..5a60623828 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java @@ -287,7 +287,7 @@ class PatchScriptFactory extends Handler { private void loadPublished(final Map byKey, final AccountInfoCacheFactory aic, final String file) throws OrmException { - for (PatchLineComment c : db.patchComments().published(changeId, file)) { + for (PatchLineComment c : db.patchComments().publishedByChangeFile(changeId, file)) { if (comments.include(c)) { aic.want(c.getAuthor()); } @@ -303,7 +303,7 @@ class PatchScriptFactory extends Handler { private void loadDrafts(final Map byKey, final AccountInfoCacheFactory aic, final Account.Id me, final String file) throws OrmException { - for (PatchLineComment c : db.patchComments().draft(changeId, file, me)) { + for (PatchLineComment c : db.patchComments().draftByChangeFileAuthor(changeId, file, me)) { if (comments.include(c)) { aic.want(me); } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java index 26785a8b81..ddff0f19a6 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java @@ -30,35 +30,39 @@ public interface PatchLineCommentAccess extends @Query("WHERE key.patchKey = ? AND status = '" + PatchLineComment.STATUS_PUBLISHED + "' ORDER BY lineNbr,writtenOn") - ResultSet published(Patch.Key patch) throws OrmException; + ResultSet publishedByPatch(Patch.Key patch) + throws OrmException; @Query("WHERE key.patchKey.patchSetId.changeId = ?" + " AND key.patchKey.fileName = ? AND status = '" + PatchLineComment.STATUS_PUBLISHED + "' ORDER BY lineNbr,writtenOn") - ResultSet published(Change.Id id, String file) + ResultSet publishedByChangeFile(Change.Id id, String file) throws OrmException; @Query("WHERE key.patchKey.patchSetId = ? AND status = '" + PatchLineComment.STATUS_PUBLISHED + "'") - ResultSet published(PatchSet.Id patchset) + ResultSet publishedByPatchSet(PatchSet.Id patchset) throws OrmException; @Query("WHERE key.patchKey.patchSetId = ? AND status = '" + PatchLineComment.STATUS_DRAFT + "' AND author = ? ORDER BY key.patchKey,lineNbr,writtenOn") - ResultSet draft(PatchSet.Id patchset, Account.Id author) + ResultSet draftByPatchSetAuthor + (PatchSet.Id patchset, Account.Id author) throws OrmException; @Query("WHERE key.patchKey = ? AND status = '" + PatchLineComment.STATUS_DRAFT + "' AND author = ? ORDER BY lineNbr,writtenOn") - ResultSet draft(Patch.Key patch, Account.Id author) + ResultSet draftByPatchAuthor + (Patch.Key patch, Account.Id author) throws OrmException; @Query("WHERE key.patchKey.patchSetId.changeId = ?" + " AND key.patchKey.fileName = ? AND author = ? AND status = '" + PatchLineComment.STATUS_DRAFT + "' ORDER BY lineNbr,writtenOn") - ResultSet draft(Change.Id id, String file, Account.Id author) + ResultSet draftByChangeFileAuthor + (Change.Id id, String file, Account.Id author) throws OrmException; @Query("WHERE status = '" + PatchLineComment.STATUS_DRAFT diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/ReviewDb.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/ReviewDb.java index b75b91bb1f..08d6783b4b 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/ReviewDb.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/ReviewDb.java @@ -32,85 +32,85 @@ import com.google.gwtorm.client.Sequence; public interface ReviewDb extends Schema { /* If you change anything, update SchemaVersion.C to use a new version. */ - @Relation + @Relation(id = 1) SchemaVersionAccess schemaVersion(); - @Relation + @Relation(id = 2) SystemConfigAccess systemConfig(); - @Relation + @Relation(id = 3) ApprovalCategoryAccess approvalCategories(); - @Relation + @Relation(id = 4) ApprovalCategoryValueAccess approvalCategoryValues(); - @Relation + @Relation(id = 5) ContributorAgreementAccess contributorAgreements(); - @Relation + @Relation(id = 6) AccountAccess accounts(); - @Relation + @Relation(id = 7) AccountExternalIdAccess accountExternalIds(); - @Relation + @Relation(id = 8) AccountSshKeyAccess accountSshKeys(); - @Relation + @Relation(id = 9) AccountAgreementAccess accountAgreements(); - @Relation + @Relation(id = 10) AccountGroupAccess accountGroups(); - @Relation + @Relation(id = 11) AccountGroupNameAccess accountGroupNames(); - @Relation + @Relation(id = 12) AccountGroupMemberAccess accountGroupMembers(); - @Relation + @Relation(id = 13) AccountGroupMemberAuditAccess accountGroupMembersAudit(); - @Relation + @Relation(id = 14) AccountGroupIncludeAccess accountGroupIncludes(); - @Relation + @Relation(id = 15) AccountGroupIncludeAuditAccess accountGroupIncludesAudit(); - @Relation + @Relation(id = 16) AccountGroupAgreementAccess accountGroupAgreements(); - @Relation + @Relation(id = 17) AccountDiffPreferenceAccess accountDiffPreferences(); - @Relation + @Relation(id = 18) StarredChangeAccess starredChanges(); - @Relation + @Relation(id = 19) AccountProjectWatchAccess accountProjectWatches(); - @Relation + @Relation(id = 20) AccountPatchReviewAccess accountPatchReviews(); - @Relation + @Relation(id = 21) ChangeAccess changes(); - @Relation + @Relation(id = 22) PatchSetApprovalAccess patchSetApprovals(); - @Relation + @Relation(id = 23) ChangeMessageAccess changeMessages(); - @Relation + @Relation(id = 24) PatchSetAccess patchSets(); - @Relation + @Relation(id = 25) PatchSetAncestorAccess patchSetAncestors(); - @Relation + @Relation(id = 26) PatchLineCommentAccess patchComments(); - @Relation + @Relation(id = 27) TrackingIdAccess trackingIds(); /** Create the next unique id for an {@link Account}. */ diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java index 1d43453684..34194af058 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java @@ -280,7 +280,7 @@ public class PublishComments implements Callable { } private List drafts() throws OrmException { - return db.patchComments().draft(patchSetId, user.getAccountId()).toList(); + return db.patchComments().draftByPatchSetAuthor(patchSetId, user.getAccountId()).toList(); } private void email() { diff --git a/pom.xml b/pom.xml index a984a83bd2..366b6bd280 100644 --- a/pom.xml +++ b/pom.xml @@ -47,7 +47,7 @@ limitations under the License. 1.0.0.201106090707-r.19-g8d88a84 - 1.1.5 + 1.2-SNAPSHOT 1.2.5 1.2.4 2.3.0 From 4041cdff679b3e99e59e33f84628091a5f431e2d Mon Sep 17 00:00:00 2001 From: Jeff Schumacher Date: Fri, 6 Aug 2010 15:15:12 -0700 Subject: [PATCH 30/33] Add command to output a Protobuf message file for the DB Write out a Protobuf message file describing the data within our NoSQL DB. Change-Id: I5ed78756ac7ff333e6c68894ebf7850eff1a0048 --- .../java/com/google/gerrit/pgm/ProtoGen.java | 75 +++++++++++++++++++ .../com/google/gerrit/pgm/ProtoGenHeader.txt | 22 ++++++ 2 files changed, 97 insertions(+) create mode 100644 gerrit-pgm/src/main/java/com/google/gerrit/pgm/ProtoGen.java create mode 100644 gerrit-pgm/src/main/resources/com/google/gerrit/pgm/ProtoGenHeader.txt diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/ProtoGen.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/ProtoGen.java new file mode 100644 index 0000000000..bc7b25469a --- /dev/null +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/ProtoGen.java @@ -0,0 +1,75 @@ +// Copyright (C) 2010 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.google.gerrit.pgm; + +import com.google.gerrit.pgm.util.AbstractProgram; +import com.google.gerrit.reviewdb.ReviewDb; +import com.google.gwtorm.schema.java.JavaSchemaModel; + +import org.eclipse.jgit.storage.file.LockFile; +import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.IO; +import org.kohsuke.args4j.Option; + +import java.io.BufferedWriter; +import java.io.File; +import java.io.InputStream; +import java.io.OutputStreamWriter; +import java.io.PrintWriter; +import java.nio.ByteBuffer; + +public class ProtoGen extends AbstractProgram { + @Option(name = "--output", aliases = {"-o"}, required = true, metaVar = "FILE", usage = "File to write .proto into") + private File file; + + @Override + public int run() throws Exception { + LockFile lock = new LockFile(file.getAbsoluteFile(), FS.DETECTED); + if (!lock.lock()) { + throw die("Cannot lock " + file); + } + try { + JavaSchemaModel jsm = new JavaSchemaModel(ReviewDb.class); + PrintWriter out = new PrintWriter(new BufferedWriter( + new OutputStreamWriter(lock.getOutputStream(), "UTF-8"))); + try { + String header; + InputStream in = getClass().getResourceAsStream("ProtoGenHeader.txt"); + try { + ByteBuffer buf = IO.readWholeStream(in, 1024); + int ptr = buf.arrayOffset() + buf.position(); + int len = buf.remaining(); + header = new String(buf.array(), ptr, len, "UTF-8"); + } finally { + in.close(); + } + + String version = com.google.gerrit.common.Version.getVersion(); + out.write(header.replace("@@VERSION@@", version)); + jsm.generateProto(out); + out.flush(); + } finally { + out.close(); + } + if (!lock.commit()) { + throw die("Could not write to " + file); + } + } finally { + lock.unlock(); + } + System.out.println("Created " + file.getPath()); + return 0; + } +} diff --git a/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/ProtoGenHeader.txt b/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/ProtoGenHeader.txt new file mode 100644 index 0000000000..757e8e2bc2 --- /dev/null +++ b/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/ProtoGenHeader.txt @@ -0,0 +1,22 @@ +// Copyright (C) 2011 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. +// +// Gerrit Code Review (version @@VERSION@@) + +syntax = "proto2"; + +option java_api_version = 2; +option java_package = "com.google.gerrit.proto.reviewdb"; + +package devtools.gerritcodereview; From 2d82b4a84ec59a5b8bcfaf8081b1a8c2e6a546a4 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 19 Oct 2011 17:01:36 -0700 Subject: [PATCH 31/33] Minor ORM cleanups to support other backends Change-Id: I788da58dffd81dc9b7b0344b304491b4f38d459a --- .../com/google/gerrit/reviewdb/AccountAgreement.java | 4 ++++ .../gerrit/reviewdb/AccountGroupAgreement.java | 4 ++++ .../gerrit/reviewdb/AccountGroupIncludeAudit.java | 8 ++++++++ .../gerrit/reviewdb/AccountGroupMemberAudit.java | 8 ++++++++ .../gerrit/reviewdb/AccountGroupNameAccess.java | 2 +- .../google/gerrit/reviewdb/AccountProjectWatch.java | 8 ++++++++ .../gerrit/reviewdb/PatchLineCommentAccess.java | 12 ------------ .../com/google/gerrit/reviewdb/PatchSetAccess.java | 4 ---- .../com/google/gerrit/reviewdb/PatchSetApproval.java | 8 ++++++++ .../com/google/gerrit/reviewdb/StarredChange.java | 8 ++++++++ .../java/com/google/gerrit/reviewdb/TrackingId.java | 12 ++++++++++++ 11 files changed, 61 insertions(+), 17 deletions(-) diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountAgreement.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountAgreement.java index 39ed66dcf1..57b9340a0a 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountAgreement.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountAgreement.java @@ -45,6 +45,10 @@ public final class AccountAgreement implements AbstractAgreement { return accountId; } + public ContributorAgreement.Id getContributorAgreementId() { + return claId; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {claId}; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupAgreement.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupAgreement.java index e7c4ad0645..fd5d58b419 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupAgreement.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupAgreement.java @@ -47,6 +47,10 @@ public final class AccountGroupAgreement implements AbstractAgreement { return groupId; } + public ContributorAgreement.Id getContributorAgreementId() { + return claId; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {claId}; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupIncludeAudit.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupIncludeAudit.java index e99e48094d..326dfd3539 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupIncludeAudit.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupIncludeAudit.java @@ -49,6 +49,14 @@ public final class AccountGroupIncludeAudit { return groupId; } + public AccountGroup.Id getIncludedId() { + return includeId; + } + + public Timestamp getAddedOn() { + return addedOn; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {includeId}; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupMemberAudit.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupMemberAudit.java index e2ce939e7f..7547efbd09 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupMemberAudit.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupMemberAudit.java @@ -49,6 +49,14 @@ public final class AccountGroupMemberAudit { return accountId; } + public AccountGroup.Id getGroupId() { + return groupId; + } + + public Timestamp getAddedOn() { + return addedOn; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {groupId}; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupNameAccess.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupNameAccess.java index 8a2fb6bb80..12db5ce2c8 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupNameAccess.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountGroupNameAccess.java @@ -27,7 +27,7 @@ public interface AccountGroupNameAccess extends AccountGroupName get(AccountGroup.NameKey name) throws OrmException; @Query("ORDER BY name") - ResultSet all(); + ResultSet all() throws OrmException; @Query("WHERE name.name >= ? AND name.name <= ? ORDER BY name LIMIT ?") ResultSet suggestByName(String nameA, String nameB, diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountProjectWatch.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountProjectWatch.java index c18ae823cd..6c62e12b6e 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountProjectWatch.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountProjectWatch.java @@ -56,6 +56,14 @@ public final class AccountProjectWatch { return accountId; } + public Project.NameKey getProjectName() { + return projectName; + } + + public Filter getFilter() { + return filter; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {projectName, filter}; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java index ddff0f19a6..fbf38cf712 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchLineCommentAccess.java @@ -28,11 +28,6 @@ public interface PatchLineCommentAccess extends @Query("WHERE key.patchKey.patchSetId.changeId = ?") ResultSet byChange(Change.Id id) throws OrmException; - @Query("WHERE key.patchKey = ? AND status = '" - + PatchLineComment.STATUS_PUBLISHED + "' ORDER BY lineNbr,writtenOn") - ResultSet publishedByPatch(Patch.Key patch) - throws OrmException; - @Query("WHERE key.patchKey.patchSetId.changeId = ?" + " AND key.patchKey.fileName = ? AND status = '" + PatchLineComment.STATUS_PUBLISHED + "' ORDER BY lineNbr,writtenOn") @@ -51,13 +46,6 @@ public interface PatchLineCommentAccess extends (PatchSet.Id patchset, Account.Id author) throws OrmException; - @Query("WHERE key.patchKey = ? AND status = '" - + PatchLineComment.STATUS_DRAFT - + "' AND author = ? ORDER BY lineNbr,writtenOn") - ResultSet draftByPatchAuthor - (Patch.Key patch, Account.Id author) - throws OrmException; - @Query("WHERE key.patchKey.patchSetId.changeId = ?" + " AND key.patchKey.fileName = ? AND author = ? AND status = '" + PatchLineComment.STATUS_DRAFT + "' ORDER BY lineNbr,writtenOn") diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetAccess.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetAccess.java index fa594f7694..c9d4b4f6e8 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetAccess.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetAccess.java @@ -27,10 +27,6 @@ public interface PatchSetAccess extends Access { @Query("WHERE id.changeId = ? ORDER BY id.patchSetId") ResultSet byChange(Change.Id id) throws OrmException; - @Query("WHERE id.changeId = ? AND revision = ?") - ResultSet byChangeRevision(Change.Id id, RevId rev) - throws OrmException; - @Query("WHERE revision = ? LIMIT 2") ResultSet byRevision(RevId rev) throws OrmException; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetApproval.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetApproval.java index 341d085551..a3698f47c0 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetApproval.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/PatchSetApproval.java @@ -51,6 +51,14 @@ public final class PatchSetApproval { return patchSetId; } + public Account.Id getAccountId() { + return accountId; + } + + public ApprovalCategory.Id getCategoryId() { + return categoryId; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {accountId, categoryId}; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/StarredChange.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/StarredChange.java index 7e4359b480..426bd11e10 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/StarredChange.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/StarredChange.java @@ -43,6 +43,10 @@ public class StarredChange { return accountId; } + public Change.Id getChangeId() { + return changeId; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {changeId}; @@ -59,6 +63,10 @@ public class StarredChange { key = k; } + public StarredChange.Key getKey() { + return key; + } + public Account.Id getAccountId() { return key.accountId; } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/TrackingId.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/TrackingId.java index d59e492732..1548807fd2 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/TrackingId.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/TrackingId.java @@ -102,6 +102,14 @@ public final class TrackingId { return changeId; } + public TrackingId.Id getTrackingId() { + return trackingId; + } + + public TrackingId.System getTrackingSystem() { + return trackingSystem; + } + @Override public com.google.gwtorm.client.Key[] members() { return new com.google.gwtorm.client.Key[] {trackingId, trackingSystem}; @@ -123,6 +131,10 @@ public final class TrackingId { key = new Key(ch, new TrackingId.Id(id), new TrackingId.System(s)); } + public TrackingId.Key getKey() { + return key; + } + public Change.Id getChangeId() { return key.changeId; } From d3743450a2981a3cada725e1af54f229bdc1738d Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 28 Oct 2011 15:57:04 -0700 Subject: [PATCH 32/33] Avoid opening extra ReviewDb connection in PatchSetInfoFactory Every caller already had a ReviewDb "handy". Rather than making a new connection, use the existing one from the caller to read records about the patch set from the database. Change-Id: I68de1aa293f81b4318d681851706556bb73a5fd7 --- .../rpc/changedetail/PatchSetDetailFactory.java | 2 +- .../changedetail/PatchSetPublishDetailFactory.java | 2 +- .../java/com/google/gerrit/rules/StoredValues.java | 2 +- .../java/com/google/gerrit/server/git/MergeOp.java | 2 +- .../com/google/gerrit/server/mail/ChangeEmail.java | 2 +- .../gerrit/server/patch/PatchSetInfoFactory.java | 14 ++------------ .../gerrit/server/patch/PublishComments.java | 2 +- 7 files changed, 8 insertions(+), 18 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java index 78f4ded474..9ef715ff66 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java @@ -138,7 +138,7 @@ class PatchSetDetailFactory extends Handler { detail = new PatchSetDetail(); detail.setPatchSet(patchSet); - detail.setInfo(infoFactory.get(psIdNew)); + detail.setInfo(infoFactory.get(db, psIdNew)); detail.setPatches(patches); final CurrentUser user = control.getCurrentUser(); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java index 8ec1549b8c..32a58d0148 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java @@ -83,7 +83,7 @@ final class PatchSetPublishDetailFactory extends Handler final Change.Id changeId = patchSetId.getParentKey(); final ChangeControl control = changeControlFactory.validateFor(changeId); change = control.getChange(); - patchSetInfo = infoFactory.get(patchSetId); + patchSetInfo = infoFactory.get(db, patchSetId); drafts = db.patchComments().draftByPatchSetAuthor(patchSetId, user.getAccountId()).toList(); aic.want(change.getOwner()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java index cdcbb41fd6..de85742ebc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java +++ b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java @@ -51,7 +51,7 @@ public final class StoredValues { PatchSetInfoFactory patchInfoFactory = env.getInjector().getInstance(PatchSetInfoFactory.class); try { - return patchInfoFactory.get(patchSetId); + return patchInfoFactory.get(REVIEW_DB.get(engine), patchSetId); } catch (PatchSetInfoNotAvailableException e) { throw new SystemException(e.getMessage()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index fd6f94b6c4..9bfef55dd6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -1254,7 +1254,7 @@ public class MergeOp { // Go back to the patch set that was actually merged. // try { - c.setCurrentPatchSet(patchSetInfoFactory.get(merged)); + c.setCurrentPatchSet(patchSetInfoFactory.get(schema, merged)); } catch (PatchSetInfoNotAvailableException e1) { log.error("Cannot read merged patch set " + merged, e1); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index 508408843a..3ef747c97e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -136,7 +136,7 @@ public abstract class ChangeEmail extends OutgoingEmail { if (patchSet != null && patchSetInfo == null) { try { - patchSetInfo = args.patchSetInfoFactory.get(patchSet.getId()); + patchSetInfo = args.patchSetInfoFactory.get(args.db.get(), patchSet.getId()); } catch (PatchSetInfoNotAvailableException err) { patchSetInfo = null; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java index 16a958224a..0c70eacb8d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java @@ -25,7 +25,6 @@ import com.google.gerrit.reviewdb.UserIdentity; import com.google.gerrit.server.account.AccountByEmailCache; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gwtorm.client.OrmException; -import com.google.gwtorm.client.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -49,15 +48,12 @@ import java.util.Set; @Singleton public class PatchSetInfoFactory { private final GitRepositoryManager repoManager; - private final SchemaFactory schemaFactory; private final AccountByEmailCache byEmailCache; @Inject public PatchSetInfoFactory(final GitRepositoryManager grm, - final SchemaFactory schemaFactory, final AccountByEmailCache byEmailCache) { this.repoManager = grm; - this.schemaFactory = schemaFactory; this.byEmailCache = byEmailCache; } @@ -68,16 +64,13 @@ public class PatchSetInfoFactory { info.setAuthor(toUserIdentity(src.getAuthorIdent())); info.setCommitter(toUserIdentity(src.getCommitterIdent())); info.setRevId(src.getName()); - return info; } - public PatchSetInfo get(PatchSet.Id patchSetId) - throws PatchSetInfoNotAvailableException { - ReviewDb db = null; + public PatchSetInfo get(ReviewDb db, PatchSet.Id patchSetId) + throws PatchSetInfoNotAvailableException { Repository repo = null; try { - db = schemaFactory.open(); final PatchSet patchSet = db.patchSets().get(patchSetId); final Change change = db.changes().get(patchSet.getId().getParentKey()); final Project.NameKey projectKey = change.getProject(); @@ -97,9 +90,6 @@ public class PatchSetInfoFactory { } catch (IOException e) { throw new PatchSetInfoNotAvailableException(e); } finally { - if (db != null) { - db.close(); - } if (repo != null) { repo.close(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java index 34194af058..838c31fdfe 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java @@ -288,7 +288,7 @@ public class PublishComments implements Callable { if (message != null) { final CommentSender cm = commentSenderFactory.create(change); cm.setFrom(user.getAccountId()); - cm.setPatchSet(patchSet, patchSetInfoFactory.get(patchSetId)); + cm.setPatchSet(patchSet, patchSetInfoFactory.get(db, patchSetId)); cm.setChangeMessage(message); cm.setPatchLineComments(drafts); cm.send(); From 77c684b417cf101dafef86f9efbd20a287e764dd Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 28 Oct 2011 15:57:51 -0700 Subject: [PATCH 33/33] Use transactions to handle comments when possible If the database supports transactions (see gwtorm, not all do) try to perform some update operations as a single transaction on the change. This may allow the database to batch together any record updates, saving some time. Change-Id: I5af9efe3a541d83515109e3bf9a3497b3d8127de --- .../rpc/patch/PatchDetailServiceImpl.java | 49 +++++++----- .../gerrit/httpd/rpc/patch/SaveDraft.java | 75 ++++++++++--------- .../gerrit/server/patch/PublishComments.java | 25 ++++--- 3 files changed, 88 insertions(+), 61 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java index 0f3b4f8eae..90a6253e54 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java @@ -110,18 +110,25 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements final AsyncCallback callback) { run(callback, new Action() { public VoidResult run(ReviewDb db) throws OrmException, Failure { - final PatchLineComment comment = db.patchComments().get(commentKey); - if (comment == null) { - throw new Failure(new NoSuchEntityException()); + Change.Id id = commentKey.getParentKey().getParentKey().getParentKey(); + db.changes().beginTransaction(id); + try { + final PatchLineComment comment = db.patchComments().get(commentKey); + if (comment == null) { + throw new Failure(new NoSuchEntityException()); + } + if (!getAccountId().equals(comment.getAuthor())) { + throw new Failure(new NoSuchEntityException()); + } + if (comment.getStatus() != PatchLineComment.Status.DRAFT) { + throw new Failure(new IllegalStateException("Comment published")); + } + db.patchComments().delete(Collections.singleton(comment)); + db.commit(); + return VoidResult.INSTANCE; + } finally { + db.rollback(); } - if (!getAccountId().equals(comment.getAuthor())) { - throw new Failure(new NoSuchEntityException()); - } - if (comment.getStatus() != PatchLineComment.Status.DRAFT) { - throw new Failure(new IllegalStateException("Comment published")); - } - db.patchComments().delete(Collections.singleton(comment)); - return VoidResult.INSTANCE; } }); } @@ -142,14 +149,20 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements Account.Id account = getAccountId(); AccountPatchReview.Key key = new AccountPatchReview.Key(patchKey, account); - AccountPatchReview apr = db.accountPatchReviews().get(key); - if (apr == null && reviewed) { - db.accountPatchReviews().insert( - Collections.singleton(new AccountPatchReview(patchKey, account))); - } else if (apr != null && !reviewed) { - db.accountPatchReviews().delete(Collections.singleton(apr)); + db.accounts().beginTransaction(account); + try { + AccountPatchReview apr = db.accountPatchReviews().get(key); + if (apr == null && reviewed) { + db.accountPatchReviews().insert( + Collections.singleton(new AccountPatchReview(patchKey, account))); + } else if (apr != null && !reviewed) { + db.accountPatchReviews().delete(Collections.singleton(apr)); + } + db.commit(); + return VoidResult.INSTANCE; + } finally { + db.rollback(); } - return VoidResult.INSTANCE; } }); } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java index 1fbc559416..0f146353aa 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java @@ -49,7 +49,6 @@ class SaveDraft extends Handler { this.changeControlFactory = changeControlFactory; this.db = db; this.currentUser = currentUser; - this.comment = comment; } @@ -62,42 +61,50 @@ class SaveDraft extends Handler { final Patch.Key patchKey = comment.getKey().getParentKey(); final PatchSet.Id patchSetId = patchKey.getParentKey(); final Change.Id changeId = patchKey.getParentKey().getParentKey(); - changeControlFactory.validateFor(changeId); - if (db.patchSets().get(patchSetId) == null) { - throw new NoSuchChangeException(changeId); - } - final Account.Id me = currentUser.getAccountId(); - if (comment.getKey().get() == null) { - if (comment.getLine() < 1) { - throw new IllegalStateException("Comment line must be >= 1, not " - + comment.getLine()); - } - - if (comment.getParentUuid() != null) { - final PatchLineComment parent = - db.patchComments().get( - new PatchLineComment.Key(patchKey, comment.getParentUuid())); - if (parent == null || parent.getSide() != comment.getSide()) { - throw new IllegalStateException("Parent comment must be on same side"); - } - } - - final PatchLineComment nc = - new PatchLineComment(new PatchLineComment.Key(patchKey, ChangeUtil - .messageUUID(db)), comment.getLine(), me, comment.getParentUuid()); - nc.setSide(comment.getSide()); - nc.setMessage(comment.getMessage()); - db.patchComments().insert(Collections.singleton(nc)); - return nc; - - } else { - if (!me.equals(comment.getAuthor())) { + db.changes().beginTransaction(changeId); + try { + changeControlFactory.validateFor(changeId); + if (db.patchSets().get(patchSetId) == null) { throw new NoSuchChangeException(changeId); } - comment.updated(); - db.patchComments().update(Collections.singleton(comment)); - return comment; + + final Account.Id me = currentUser.getAccountId(); + if (comment.getKey().get() == null) { + if (comment.getLine() < 1) { + throw new IllegalStateException("Comment line must be >= 1, not " + + comment.getLine()); + } + + if (comment.getParentUuid() != null) { + final PatchLineComment parent = + db.patchComments().get( + new PatchLineComment.Key(patchKey, comment.getParentUuid())); + if (parent == null || parent.getSide() != comment.getSide()) { + throw new IllegalStateException("Parent comment must be on same side"); + } + } + + final PatchLineComment nc = + new PatchLineComment(new PatchLineComment.Key(patchKey, ChangeUtil + .messageUUID(db)), comment.getLine(), me, comment.getParentUuid()); + nc.setSide(comment.getSide()); + nc.setMessage(comment.getMessage()); + db.patchComments().insert(Collections.singleton(nc)); + db.commit(); + return nc; + + } else { + if (!me.equals(comment.getAuthor())) { + throw new NoSuchChangeException(changeId); + } + comment.updated(); + db.patchComments().update(Collections.singleton(comment)); + db.commit(); + return comment; + } + } finally { + db.rollback(); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java index 838c31fdfe..a27312767a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java @@ -116,18 +116,25 @@ public class PublishComments implements Callable { } drafts = drafts(); - publishDrafts(); + db.changes().beginTransaction(changeId); + try { + publishDrafts(); - final boolean isCurrent = patchSetId.equals(change.currentPatchSetId()); - if (isCurrent && change.getStatus().isOpen()) { - publishApprovals(ctl); - } else if (! approvals.isEmpty()) { - throw new InvalidChangeOperationException("Change is closed"); - } else { - publishMessageOnly(); + final boolean isCurrent = patchSetId.equals(change.currentPatchSetId()); + if (isCurrent && change.getStatus().isOpen()) { + publishApprovals(ctl); + } else if (!approvals.isEmpty()) { + throw new InvalidChangeOperationException("Change is closed"); + } else { + publishMessageOnly(); + } + + touchChange(); + db.commit(); + } finally { + db.rollback(); } - touchChange(); email(); fireHook(); return VoidResult.INSTANCE;