From ace2c6d9f4effc5db574f01c5dcdb54caec55316 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Wed, 1 May 2013 17:11:28 +0100 Subject: [PATCH 1/5] Fix login servlets when canonicalWebUrl is not set Each login servlet knows enough about the incoming request to be able to not need the canonical web address for redirection purposes. In the case gerrit.canonicalWebUrl is not set, use the incoming request to build up the URL. This solution is a work-around for the fact that somewhere before 2.5 Colby broke the HttpServletRequest scope based version of the @CanonicalWebUrl provider. Because Guice cannot supply the request in some contexts we pass the request into the provider as an argument. Long term all of these authentication methods will be ejected into their own plugins and it will be possible to revisit how this configuration is handled. Change-Id: I0e00b89020860a02b5d6ea77da5c784f5f0bb1b8 --- .../google/gerrit/httpd/CanonicalWebUrl.java | 47 +++++++++++++++++++ .../httpd/HttpCanonicalWebUrlProvider.java | 11 +---- .../auth/container/HttpLoginServlet.java | 8 ++-- .../httpd/auth/ldap/LdapLoginServlet.java | 18 +++---- .../gerrit/httpd/auth/openid/LoginForm.java | 2 +- .../httpd/auth/openid/OpenIdServiceImpl.java | 25 +++++----- 6 files changed, 73 insertions(+), 38 deletions(-) create mode 100644 gerrit-httpd/src/main/java/com/google/gerrit/httpd/CanonicalWebUrl.java diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CanonicalWebUrl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CanonicalWebUrl.java new file mode 100644 index 0000000000..61c0cd1e6f --- /dev/null +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CanonicalWebUrl.java @@ -0,0 +1,47 @@ +// Copyright (C) 2013 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 javax.annotation.Nullable; +import javax.servlet.http.HttpServletRequest; + +import com.google.inject.Inject; +import com.google.inject.Provider; + +public class CanonicalWebUrl { + private final Provider configured; + + @Inject + CanonicalWebUrl( + @com.google.gerrit.server.config.CanonicalWebUrl + @Nullable + Provider provider) { + configured = provider; + } + + public String get(HttpServletRequest req) { + String url = configured.get(); + return url != null ? url : computeFromRequest(req); + } + + static String computeFromRequest(HttpServletRequest req) { + StringBuffer url = req.getRequestURL(); + url.setLength(url.length() - req.getServletPath().length()); + if (url.charAt(url.length() - 1) != '/') { + url.append('/'); + } + return url.toString(); + } +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/HttpCanonicalWebUrlProvider.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/HttpCanonicalWebUrlProvider.java index 5df678b76b..5196e9c3a9 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/HttpCanonicalWebUrlProvider.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/HttpCanonicalWebUrlProvider.java @@ -14,7 +14,6 @@ package com.google.gerrit.httpd; -import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.CanonicalWebUrlProvider; import com.google.gerrit.server.config.GerritServerConfig; import com.google.inject.Inject; @@ -26,7 +25,7 @@ import org.eclipse.jgit.lib.Config; import javax.servlet.http.HttpServletRequest; -/** Sets {@link CanonicalWebUrl} to current HTTP request if not configured. */ +/** Sets {@code CanonicalWebUrl} to current HTTP request if not configured. */ public class HttpCanonicalWebUrlProvider extends CanonicalWebUrlProvider { private Provider requestProvider; @@ -65,13 +64,7 @@ public class HttpCanonicalWebUrlProvider extends CanonicalWebUrlProvider { throw noWeb; } } - - final StringBuffer url = req.getRequestURL(); - url.setLength(url.length() - req.getServletPath().length()); - if (url.charAt(url.length() - 1) != '/') { - url.append('/'); - } - return url.toString(); + return CanonicalWebUrl.computeFromRequest(req); } // We have no way of guessing our HTTP url. 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 696a585170..fe7aa23dd5 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 @@ -15,13 +15,13 @@ package com.google.gerrit.httpd.auth.container; import com.google.gerrit.common.PageLinks; +import com.google.gerrit.httpd.CanonicalWebUrl; import com.google.gerrit.httpd.HtmlDomUtil; import com.google.gerrit.httpd.WebSession; import com.google.gerrit.server.account.AccountException; import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.account.AuthResult; -import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gwtexpui.server.CacheHeaders; import com.google.inject.Inject; import com.google.inject.Provider; @@ -57,13 +57,13 @@ class HttpLoginServlet extends HttpServlet { LoggerFactory.getLogger(HttpLoginServlet.class); private final Provider webSession; - private final Provider urlProvider; + private final CanonicalWebUrl urlProvider; private final AccountManager accountManager; private final HttpAuthFilter authFilter; @Inject HttpLoginServlet(final Provider webSession, - @CanonicalWebUrl @Nullable final Provider urlProvider, + final CanonicalWebUrl urlProvider, final AccountManager accountManager, final HttpAuthFilter authFilter) { this.webSession = webSession; @@ -121,7 +121,7 @@ class HttpLoginServlet extends HttpServlet { } final StringBuilder rdr = new StringBuilder(); - rdr.append(urlProvider.get()); + rdr.append(urlProvider.get(req)); rdr.append('#'); if (arsp.isNew() && !token.startsWith(PageLinks.REGISTER + "/")) { rdr.append(PageLinks.REGISTER); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java index cfae86c8ba..9a2a7dad8f 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java @@ -17,6 +17,7 @@ package com.google.gerrit.httpd.auth.ldap; import com.google.common.base.Objects; import com.google.common.base.Strings; import com.google.gerrit.common.PageLinks; +import com.google.gerrit.httpd.CanonicalWebUrl; import com.google.gerrit.httpd.HtmlDomUtil; import com.google.gerrit.httpd.WebSession; import com.google.gerrit.httpd.template.SiteHeaderFooter; @@ -26,7 +27,7 @@ import com.google.gerrit.server.account.AccountUserNameException; import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.account.AuthResult; import com.google.gerrit.server.auth.AuthenticationUnavailableException; -import com.google.gerrit.server.config.CanonicalWebUrl; +import com.google.gerrit.server.config.SitePaths; import com.google.gwtexpui.server.CacheHeaders; import com.google.inject.Inject; import com.google.inject.Provider; @@ -55,28 +56,24 @@ class LdapLoginServlet extends HttpServlet { private final AccountManager accountManager; private final Provider webSession; - private final Provider urlProvider; + private final CanonicalWebUrl urlProvider; private final SiteHeaderFooter headers; @Inject LdapLoginServlet(AccountManager accountManager, Provider webSession, - @CanonicalWebUrl @Nullable Provider urlProvider, + CanonicalWebUrl urlProvider, SiteHeaderFooter headers) { this.accountManager = accountManager; this.webSession = webSession; this.urlProvider = urlProvider; this.headers = headers; - - if (Strings.isNullOrEmpty(urlProvider.get())) { - log.error("gerrit.canonicalWebUrl must be set in gerrit.config"); - } } private void sendForm(HttpServletRequest req, HttpServletResponse res, @Nullable String errorMessage) throws IOException { String self = req.getRequestURI(); - String cancel = Objects.firstNonNull(urlProvider.get(), "/"); + String cancel = Objects.firstNonNull(urlProvider.get(req), "/"); String token = getToken(req); if (!token.equals("/")) { cancel += "#" + token; @@ -146,11 +143,10 @@ class LdapLoginServlet extends HttpServlet { return; } - String token = getToken(req); StringBuilder dest = new StringBuilder(); - dest.append(urlProvider.get()); + dest.append(urlProvider.get(req)); dest.append('#'); - dest.append(token); + dest.append(getToken(req)); CacheHeaders.setNotCacheable(res); webSession.get().login(ares, "1".equals(remember)); diff --git a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/LoginForm.java b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/LoginForm.java index b8d31ee760..678ec007b9 100644 --- a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/LoginForm.java +++ b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/LoginForm.java @@ -161,7 +161,7 @@ class LoginForm extends HttpServlet { remember = false; } - DiscoveryResult r = impl.discover(id, mode, remember, token); + DiscoveryResult r = impl.discover(req, id, mode, remember, token); switch (r.status) { case VALID: redirect(r, res); diff --git a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java index 52f3b03843..5d74166e52 100644 --- a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java +++ b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java @@ -16,6 +16,7 @@ package com.google.gerrit.httpd.auth.openid; import com.google.gerrit.common.PageLinks; import com.google.gerrit.common.auth.openid.OpenIdUrls; +import com.google.gerrit.httpd.CanonicalWebUrl; import com.google.gerrit.httpd.WebSession; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.IdentifiedUser; @@ -24,7 +25,6 @@ import com.google.gerrit.server.account.AccountException; import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.auth.openid.OpenIdProviderPattern; import com.google.gerrit.server.config.AuthConfig; -import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gwtorm.client.KeyUtil; @@ -63,7 +63,6 @@ import java.net.URL; import java.util.List; import java.util.concurrent.TimeUnit; -import javax.annotation.Nullable; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -93,7 +92,7 @@ class OpenIdServiceImpl { private final Provider webSession; private final Provider identifiedUser; - private final Provider urlProvider; + private final CanonicalWebUrl urlProvider; private final AccountManager accountManager; private final ConsumerManager manager; private final List allowedOpenIDs; @@ -105,7 +104,7 @@ class OpenIdServiceImpl { @Inject OpenIdServiceImpl(final Provider cf, final Provider iu, - @CanonicalWebUrl @Nullable final Provider up, + CanonicalWebUrl up, @GerritServerConfig final Config config, final AuthConfig ac, final AccountManager am) throws ConsumerException, MalformedURLException { @@ -145,10 +144,10 @@ class OpenIdServiceImpl { } @SuppressWarnings("unchecked") - DiscoveryResult discover(final String openidIdentifier, final SignInMode mode, - final boolean remember, final String returnToken) { + DiscoveryResult discover(HttpServletRequest req, String openidIdentifier, + final SignInMode mode, final boolean remember, final String returnToken) { final State state; - state = init(openidIdentifier, mode, remember, returnToken); + state = init(req, openidIdentifier, mode, remember, returnToken); if (state == null) { return new DiscoveryResult(DiscoveryResult.Status.NO_PROVIDER); } @@ -235,7 +234,7 @@ class OpenIdServiceImpl { return; } - state = init(rediscoverIdentifier, mode, remember, returnToken); + state = init(req, rediscoverIdentifier, mode, remember, returnToken); if (state == null) { // Re-discovery must have failed, we can't run a login. // @@ -482,7 +481,7 @@ class OpenIdServiceImpl { } final StringBuilder rdr = new StringBuilder(); - rdr.append(urlProvider.get()); + rdr.append(urlProvider.get(req)); rdr.append('#'); if (isNew && !token.startsWith(PageLinks.REGISTER + "/")) { rdr.append(PageLinks.REGISTER); @@ -507,7 +506,7 @@ class OpenIdServiceImpl { webSession.get().logout(); } final StringBuilder rdr = new StringBuilder(); - rdr.append(urlProvider.get()); + rdr.append(urlProvider.get(req)); rdr.append('#'); rdr.append("SignInFailure"); rdr.append(','); @@ -517,8 +516,8 @@ class OpenIdServiceImpl { rsp.sendRedirect(rdr.toString()); } - private State init(final String openidIdentifier, final SignInMode mode, - final boolean remember, final String returnToken) { + private State init(HttpServletRequest req, final String openidIdentifier, + final SignInMode mode, final boolean remember, final String returnToken) { final List list; try { list = manager.discover(openidIdentifier); @@ -530,7 +529,7 @@ class OpenIdServiceImpl { return null; } - final String contextUrl = urlProvider.get(); + final String contextUrl = urlProvider.get(req); final DiscoveryInformation discovered = manager.associate(list); final UrlEncoded retTo = new UrlEncoded(contextUrl + RETURN_URL); retTo.put(P_MODE, mode.name()); From 0d1ba9518566286a003afb6c4f5e7dfcf14e8bb7 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Thu, 16 May 2013 13:54:25 +0200 Subject: [PATCH 2/5] Fix refreshing PatchScreen when fileList has not yet been loaded Although PatchScreen and PatchTable typically guard against using a not yet loaded PatchTable, refreshing a PatchScreen and the subsequent moving of the pointer lacked such guards. Thereby, a race condition was introduced that lead to issue 1749. We now added guards in both PatchScreen, and PatchTable. Bug: issue 1749 Change-Id: I03ee3e80c198373ebecc50b1135b9c542d8ecfe7 (cherry picked from commit da5ceb4c9c73306b90be4349a4e7e106ab78e676) --- .../java/com/google/gerrit/client/changes/PatchTable.java | 4 +++- .../java/com/google/gerrit/client/patches/PatchScreen.java | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java index d00ef890ef..50fc892f74 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java @@ -159,7 +159,9 @@ public class PatchTable extends Composite { } public void movePointerTo(final Patch.Key k) { - myTable.movePointerTo(k); + if (myTable != null) { + myTable.movePointerTo(k); + } } public void setActive(boolean active) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java index 676cea6878..d914283c0f 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java @@ -361,7 +361,7 @@ public abstract class PatchScreen extends Screen implements lastScript = null; settingsPanel.setEnabled(false); reviewedPanels.populate(patchKey, fileList, patchIndex, getPatchScreenType()); - if (isFirst && fileList != null) { + if (isFirst && fileList != null && fileList.isLoaded()) { fileList.movePointerTo(patchKey); } PatchUtil.DETAIL_SVC.patchScript(patchKey, idSideA, idSideB, // From 490fa71cc798c87e0d0aaace754584c6d702cd83 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Fri, 17 May 2013 23:30:28 +0200 Subject: [PATCH 3/5] Fix PatchScript's mapping of lines below the last edit For comments below the last edit block of a PatchScript, the formulae to obtain the line number shifts between sides subtracted one side's end, but added the other side's beginning (instead of also the end). This mismatch could cause the client to fetch lines beyond a file's end and thereby cause an ArrayIndexofBoundException. We now use the difference of the last edit block's ends to map lines below the last edit. Change-Id: I67d2baa0f5ede3af7348c609034de81e8acf9c17 --- .../com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java index 5019403db0..9a4b89f6f3 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java @@ -346,7 +346,7 @@ class PatchScriptBuilder { } final Edit last = edits.get(edits.size() - 1); - return last.getBeginB() + (a - last.getEndA()); + return last.getEndB() + (a - last.getEndA()); } private int mapB2A(final int b) { @@ -372,7 +372,7 @@ class PatchScriptBuilder { } final Edit last = edits.get(edits.size() - 1); - return last.getBeginA() + (b - last.getEndB()); + return last.getEndA() + (b - last.getEndB()); } private void packContent(boolean ignoredWhitespace) { From 246f9b52854fc1b11dbe14085e2c0ac0f6ddda95 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Sat, 18 May 2013 17:46:54 +0200 Subject: [PATCH 4/5] Reference included plugins by absolute Urls While the use of relative Urls for referencing included plugin repositories works nicely when cloning directly from gerrit-review, it caused problems when cloning from a mirror that only mirrored the main gerrit repository. There 'git submodule update' did not work unless the referenced plugin repositories were also mirrored in exactly the same relative position as on gerrit-review. Hence, we switch to referencing the included plugin repositories by absolute Urls. Change-Id: I1230f398cc7d2323922aff7f7acf92eac30f3ab5 --- .gitmodules | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.gitmodules b/.gitmodules index 32483d6b88..0f7fdabdf6 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,11 +1,11 @@ [submodule "plugins/replication"] path = plugins/replication - url = ../plugins/replication + url = https://gerrit.googlesource.com/plugins/replication [submodule "plugins/reviewnotes"] path = plugins/reviewnotes - url = ../plugins/reviewnotes + url = https://gerrit.googlesource.com/plugins/reviewnotes [submodule "plugins/commit-message-length-validator"] path = plugins/commit-message-length-validator - url = ../plugins/commit-message-length-validator + url = https://gerrit.googlesource.com/plugins/commit-message-length-validator From d04a818db116b635106d7447e57717f31fed0ddc Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 17 May 2013 11:11:00 -0700 Subject: [PATCH 5/5] Work around MySQL refusing to be case sensitive We can't trust MySQL to be case sensitive on VARCHAR columns. Encode UUID sequences using only hex digits where MySQL cannot run into conflicts over 'A' and 'a' being the "same". Bug: issue 851 Change-Id: I0f68efcebe1a75ddd9269b302c27fa14b1151b80 --- .../com/google/gerrit/server/ChangeUtil.java | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 41a97d2fdf..c9fb4fe2b6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -14,7 +14,6 @@ package com.google.gerrit.server; -import com.google.common.base.CharMatcher; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Change; @@ -60,9 +59,7 @@ import org.eclipse.jgit.revwalk.FooterLine; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.ReceiveCommand; -import org.eclipse.jgit.util.Base64; import org.eclipse.jgit.util.ChangeIdUtil; -import org.eclipse.jgit.util.NB; import java.io.IOException; import java.sql.Timestamp; @@ -77,6 +74,8 @@ import java.util.Set; import java.util.regex.Matcher; public class ChangeUtil { + private static final Object uuidLock = new Object(); + private static final int SEED = 0x2418e6f9; private static int uuidPrefix; private static int uuidSeq; @@ -88,25 +87,19 @@ public class ChangeUtil { * @return the new unique identifier. * @throws OrmException the database couldn't be incremented. */ - public static String messageUUID(final ReviewDb db) throws OrmException { - final byte[] raw = new byte[8]; - fill(raw, db); - - // Make the resulting base64 string more URL friendly. - return CharMatcher.is('A').trimLeadingFrom( - CharMatcher.is('=').trimTrailingFrom(Base64.encodeBytes(raw))) - .replace('+', '.') - .replace('/', '-'); - } - - private static synchronized void fill(byte[] raw, ReviewDb db) - throws OrmException { - if (uuidSeq == 0) { - uuidPrefix = db.nextChangeMessageId(); - uuidSeq = Integer.MAX_VALUE; + public static String messageUUID(ReviewDb db) throws OrmException { + int p, s; + synchronized (uuidLock) { + if (uuidSeq == 0) { + uuidPrefix = db.nextChangeMessageId(); + uuidSeq = Integer.MAX_VALUE; + } + p = uuidPrefix; + s = uuidSeq--; } - NB.encodeInt32(raw, 0, uuidPrefix); - NB.encodeInt32(raw, 4, IdGenerator.mix(uuidPrefix, uuidSeq--)); + String u = IdGenerator.format(IdGenerator.mix(SEED, p)); + String l = IdGenerator.format(IdGenerator.mix(p, s)); + return u + '_' + l; } public static void touch(final Change change, ReviewDb db)