diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java index 87105f8e4e..918b7a6a33 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java @@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static java.nio.charset.StandardCharsets.US_ASCII; import com.google.gerrit.common.Nullable; -import com.google.gerrit.common.TimeUtil; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupMember; @@ -109,10 +108,15 @@ public class AccountCreator { } externalIdsUpdate.create().insert(extIds); - Account a = new Account(id, TimeUtil.nowTs()); - a.setFullName(fullName); - a.setPreferredEmail(email); - accountsUpdate.create().insert(db, a); + accountsUpdate + .create() + .insert( + db, + id, + a -> { + a.setFullName(fullName); + a.setPreferredEmail(email); + }); if (groups != null) { for (String n : groups) { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 5fbf8ca330..50bc0bed9d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -32,6 +32,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toSet; +import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import static org.junit.Assert.fail; import com.google.common.collect.FluentIterable; @@ -74,6 +75,7 @@ import com.google.gerrit.gpg.testutil.TestKey; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.AccountByEmailCache; +import com.google.gerrit.server.account.AccountConfig; import com.google.gerrit.server.account.WatchConfig; import com.google.gerrit.server.account.WatchConfig.NotifyType; import com.google.gerrit.server.account.externalids.ExternalId; @@ -91,7 +93,6 @@ import com.google.inject.Provider; import java.io.ByteArrayOutputStream; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.EnumSet; import java.util.HashSet; import java.util.Iterator; @@ -106,6 +107,7 @@ import org.eclipse.jgit.api.errors.TransportException; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; @@ -114,6 +116,7 @@ import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.PushCertificateIdent; import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.RemoteRefUpdate; +import org.eclipse.jgit.treewalk.TreeWalk; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -162,8 +165,8 @@ public class AccountIT extends AbstractDaemonTest { externalIdsUpdate = externalIdsUpdateFactory.create(); savedExternalIds = new ArrayList<>(); - savedExternalIds.addAll(getExternalIds(admin)); - savedExternalIds.addAll(getExternalIds(user)); + savedExternalIds.addAll(externalIds.byAccount(admin.id)); + savedExternalIds.addAll(externalIds.byAccount(user.id)); } @After @@ -172,8 +175,8 @@ public class AccountIT extends AbstractDaemonTest { // savedExternalIds is null when we don't run SSH tests and the assume in // @Before in AbstractDaemonTest prevents this class' @Before method from // being executed. - externalIdsUpdate.delete(getExternalIds(admin)); - externalIdsUpdate.delete(getExternalIds(user)); + externalIdsUpdate.delete(externalIds.byAccount(admin.id)); + externalIdsUpdate.delete(externalIds.byAccount(user.id)); externalIdsUpdate.insert(savedExternalIds); } } @@ -190,10 +193,6 @@ public class AccountIT extends AbstractDaemonTest { } } - private Collection getExternalIds(TestAccount account) throws Exception { - return accountCache.get(account.getId()).getExternalIds(); - } - @After public void deleteGpgKeys() throws Exception { String ref = REFS_GPG_KEYS; @@ -220,6 +219,67 @@ public class AccountIT extends AbstractDaemonTest { create(3); // account creation + external ID creation + adding SSH keys } + private void create(int expectedAccountReindexCalls) throws Exception { + String name = "foo"; + TestAccount foo = accountCreator.create(name); + AccountInfo info = gApi.accounts().id(foo.id.get()).get(); + assertThat(info.username).isEqualTo(name); + assertThat(info.name).isEqualTo(name); + accountIndexedCounter.assertReindexOf(foo, expectedAccountReindexCalls); + + // check user branch + try (Repository repo = repoManager.openRepository(allUsers); + RevWalk rw = new RevWalk(repo); + ObjectReader or = repo.newObjectReader()) { + Ref ref = repo.exactRef(RefNames.refsUsers(foo.getId())); + assertThat(ref).isNotNull(); + RevCommit c = rw.parseCommit(ref.getObjectId()); + long timestampDiffMs = + Math.abs( + c.getCommitTime() * 1000L + - accountCache.get(foo.getId()).getAccount().getRegisteredOn().getTime()); + assertThat(timestampDiffMs).isAtMost(ChangeRebuilderImpl.MAX_WINDOW_MS); + + // Check the 'account.config' file. + try (TreeWalk tw = TreeWalk.forPath(or, AccountConfig.ACCOUNT_CONFIG, c.getTree())) { + assertThat(tw).isNotNull(); + Config cfg = new Config(); + cfg.fromText(new String(or.open(tw.getObjectId(0), OBJ_BLOB).getBytes(), UTF_8)); + assertThat(cfg.getString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_FULL_NAME)) + .isEqualTo(name); + } + } + } + + @Test + public void createAnonymousCoward() throws Exception { + TestAccount anonymousCoward = accountCreator.create(); + accountIndexedCounter.assertReindexOf(anonymousCoward); + + // check user branch + try (Repository repo = repoManager.openRepository(allUsers); + RevWalk rw = new RevWalk(repo); + ObjectReader or = repo.newObjectReader()) { + Ref ref = repo.exactRef(RefNames.refsUsers(anonymousCoward.getId())); + assertThat(ref).isNotNull(); + RevCommit c = rw.parseCommit(ref.getObjectId()); + long timestampDiffMs = + Math.abs( + c.getCommitTime() * 1000L + - accountCache + .get(anonymousCoward.getId()) + .getAccount() + .getRegisteredOn() + .getTime()); + assertThat(timestampDiffMs).isAtMost(ChangeRebuilderImpl.MAX_WINDOW_MS); + + // No account properties were set, hence an 'account.config' file was not created. + try (TreeWalk tw = TreeWalk.forPath(or, AccountConfig.ACCOUNT_CONFIG, c.getTree())) { + assertThat(tw).isNull(); + } + } + } + @Test public void get() throws Exception { AccountInfo info = gApi.accounts().id("admin").get(); @@ -693,6 +753,36 @@ public class AccountIT extends AbstractDaemonTest { accountIndexedCounter.assertReindexOf(admin); } + @Test + public void pushAccountConfigToUserBranchForReviewIsRejectedOnSubmit() throws Exception { + String userRefName = RefNames.refsUsers(admin.id); + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRefName + ":userRef"); + allUsersRepo.reset("userRef"); + + Config ac = getAccountConfig(allUsersRepo); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS, "OOO"); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(MagicBranch.NEW_CHANGE + userRefName); + r.assertOkStatus(); + accountIndexedCounter.assertNoReindex(); + assertThat(r.getChange().change().getDest().get()).isEqualTo(userRefName); + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); + exception.expect(ResourceConflictException.class); + exception.expectMessage( + String.format("update of %s not allowed", AccountConfig.ACCOUNT_CONFIG)); + gApi.changes().id(r.getChangeId()).current().submit(); + } + @Test public void pushWatchConfigToUserBranch() throws Exception { TestRepository allUsersRepo = cloneProject(allUsers); @@ -734,6 +824,28 @@ public class AccountIT extends AbstractDaemonTest { WatchConfig.WATCH_CONFIG, admin.getId().get(), project.get(), invalidNotifyValue)); } + @Test + public void pushAccountConfigToUserBranchIsRejected() throws Exception { + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, RefNames.refsUsers(admin.id) + ":userRef"); + allUsersRepo.reset("userRef"); + + Config ac = getAccountConfig(allUsersRepo); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS, "OOO"); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(RefNames.REFS_USERS_SELF); + r.assertErrorStatus("account update not allowed"); + } + @Test @Sandboxed public void cannotCreateUserBranch() throws Exception { @@ -1071,26 +1183,6 @@ public class AccountIT extends AbstractDaemonTest { assertThat(checkInfo.checkAccountsResult.problems).containsExactlyElementsIn(expectedProblems); } - public void create(int expectedAccountReindexCalls) throws Exception { - TestAccount foo = accountCreator.create("foo"); - AccountInfo info = gApi.accounts().id(foo.id.get()).get(); - assertThat(info.username).isEqualTo("foo"); - accountIndexedCounter.assertReindexOf(foo, expectedAccountReindexCalls); - - // check user branch - try (Repository repo = repoManager.openRepository(allUsers); - RevWalk rw = new RevWalk(repo)) { - Ref ref = repo.exactRef(RefNames.refsUsers(foo.getId())); - assertThat(ref).isNotNull(); - RevCommit c = rw.parseCommit(ref.getObjectId()); - long timestampDiffMs = - Math.abs( - c.getCommitTime() * 1000L - - accountCache.get(foo.getId()).getAccount().getRegisteredOn().getTime()); - assertThat(timestampDiffMs).isAtMost(ChangeRebuilderImpl.MAX_WINDOW_MS); - } - } - private void assertSequenceNumbers(List sshKeys) { int seq = 1; for (SshKeyInfo key : sshKeys) { @@ -1212,6 +1304,26 @@ public class AccountIT extends AbstractDaemonTest { assertThat(Iterables.getOnlyElement(accounts)).isEqualTo(expectedAccount.getId()); } + private Config getAccountConfig(TestRepository allUsersRepo) throws Exception { + Config ac = new Config(); + try (TreeWalk tw = + TreeWalk.forPath( + allUsersRepo.getRepository(), + AccountConfig.ACCOUNT_CONFIG, + getHead(allUsersRepo.getRepository()).getTree())) { + assertThat(tw).isNotNull(); + ac.fromText( + new String( + allUsersRepo + .getRevWalk() + .getObjectReader() + .open(tw.getObjectId(0), OBJ_BLOB) + .getBytes(), + UTF_8)); + } + return ac; + } + private static class AccountIndexedCounter implements AccountIndexedListener { private final AtomicLongMap countsByAccount = AtomicLongMap.create(); diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java index 524af50e0d..669b610925 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java +++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java @@ -37,7 +37,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountManager; -import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.account.externalids.ExternalId; @@ -71,8 +70,6 @@ import org.junit.Test; /** Unit tests for {@link GerritPublicKeyChecker}. */ public class GerritPublicKeyCheckerTest { - @Inject private Accounts accounts; - @Inject private AccountsUpdate.Server accountsUpdate; @Inject private AccountManager accountManager; @@ -117,10 +114,8 @@ public class GerritPublicKeyCheckerTest { db = schemaFactory.open(); schemaCreator.create(db); userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId(); - Account userAccount = accounts.get(db, userId); // Note: does not match any key in TestKeys. - userAccount.setPreferredEmail("user@example.com"); - accountsUpdate.create().update(db, userAccount); + accountsUpdate.create().update(db, userId, a -> a.setPreferredEmail("user@example.com")); user = reloadUser(); requestContext.setContext( diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java index 47850c40e8..0ee720adb3 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java @@ -42,6 +42,7 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -111,7 +112,7 @@ class RunAsFilter implements Filter { Account target; try { target = accountResolver.find(db.get(), runas); - } catch (OrmException e) { + } catch (OrmException | IOException | ConfigInvalidException e) { log.warn("cannot resolve account for " + RUN_AS, e); replyError(req, res, SC_INTERNAL_SERVER_ERROR, "cannot resolve " + RUN_AS, e); return; diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java index ec7fb68f03..e721b7a88a 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java @@ -49,6 +49,7 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -231,7 +232,7 @@ class BecomeAnyAccountLoginServlet extends HttpServlet { } try (ReviewDb db = schema.open()) { return auth(accounts.get(db, id)); - } catch (OrmException e) { + } catch (OrmException | IOException | ConfigInvalidException e) { getServletContext().log("cannot query database", e); return null; } diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/AccountsOnInit.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/AccountsOnInit.java index 9465a54849..81435e0ebd 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/AccountsOnInit.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/AccountsOnInit.java @@ -19,11 +19,13 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.collect.ImmutableSet; import com.google.gerrit.pgm.init.api.InitFlags; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdentProvider; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import java.io.File; @@ -57,7 +59,15 @@ public class AccountsOnInit { ObjectInserter oi = repo.newObjectInserter()) { PersonIdent serverIdent = new GerritPersonIdentProvider(flags.cfg).get(); AccountsUpdate.createUserBranch( - repo, oi, serverIdent, serverIdent, account.getId(), account.getRegisteredOn()); + repo, + new Project.NameKey(allUsers), + GitReferenceUpdated.DISABLED, + null, + oi, + serverIdent, + serverIdent, + account.getId(), + account.getRegisteredOn()); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java index b79f4966a7..2fad708395 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java @@ -44,6 +44,7 @@ import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; +import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.EnumSet; @@ -60,6 +61,7 @@ import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.stream.Stream; import org.apache.commons.lang.mutable.MutableDouble; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -73,7 +75,7 @@ public class ReviewerRecommender { new double[] { BASE_REVIEWER_WEIGHT, BASE_OWNER_WEIGHT, BASE_COMMENT_WEIGHT, }; - private static final long PLUGIN_QUERY_TIMEOUT = 500; //ms + private static final long PLUGIN_QUERY_TIMEOUT = 500; // ms private final ChangeQueryBuilder changeQueryBuilder; private final Config config; @@ -108,7 +110,7 @@ public class ReviewerRecommender { SuggestReviewers suggestReviewers, ProjectControl projectControl, List candidateList) - throws OrmException { + throws OrmException, IOException, ConfigInvalidException { String query = suggestReviewers.getQuery(); double baseWeight = config.getInt("addReviewer", "baseWeight", 1); @@ -196,7 +198,7 @@ public class ReviewerRecommender { } private Map baseRankingForEmptyQuery(double baseWeight) - throws OrmException { + throws OrmException, IOException, ConfigInvalidException { // Get the user's last 25 changes, check approvals try { List result = @@ -225,7 +227,7 @@ public class ReviewerRecommender { private Map baseRankingForCandidateList( List candidates, ProjectControl projectControl, double baseWeight) - throws OrmException { + throws OrmException, IOException, ConfigInvalidException { // Get each reviewer's activity based on number of applied labels // (weighted 10d), number of comments (weighted 0.5d) and number of owned // changes (weighted 1d). diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java index 410dc5cd12..d3083e89cb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java @@ -56,6 +56,7 @@ import java.util.EnumSet; import java.util.List; import java.util.Objects; import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; public class ReviewersUtil { @Singleton @@ -146,7 +147,7 @@ public class ReviewersUtil { ProjectControl projectControl, VisibilityControl visibilityControl, boolean excludeGroups) - throws IOException, OrmException { + throws IOException, OrmException, ConfigInvalidException { String query = suggestReviewers.getQuery(); int limit = suggestReviewers.getLimit(); @@ -212,7 +213,7 @@ public class ReviewersUtil { SuggestReviewers suggestReviewers, ProjectControl projectControl, List candidateList) - throws OrmException { + throws OrmException, IOException, ConfigInvalidException { try (Timer0.Context ctx = metrics.recommendAccountsLatency.start()) { return reviewerRecommender.suggestReviewers( changeNotes, suggestReviewers, projectControl, candidateList); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountConfig.java new file mode 100644 index 0000000000..70ed29eced --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountConfig.java @@ -0,0 +1,260 @@ +// Copyright (C) 2017 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.account; + +import static com.google.common.base.Preconditions.checkState; + +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.git.ValidationError; +import com.google.gerrit.server.git.VersionedMetaData; +import com.google.gerrit.server.mail.send.OutgoingEmailValidator; +import com.google.gwtorm.server.OrmDuplicateKeyException; +import java.io.IOException; +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.List; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.revwalk.RevSort; + +/** + * ‘account.config’ file in the user branch in the All-Users repository that contains the properties + * of the account. + * + *

The 'account.config' file is a git config file that has one 'account' section with the + * properties of the account: + * + * 

+ *   [account]
+ *     active = false
+ *     fullName = John Doe
+ *     preferredEmail = john.doe@foo.com
+ *     status = Overloaded with reviews
+ *
+ * + *

All keys are optional. This means 'account.config' may not exist on the user branch if no + * properties are set. + * + *

Not setting a key and setting a key to an empty string are treated the same way and result in + * a {@code null} value. + * + *

If no value for 'active' is specified, by default the account is considered as active. + * + *

The commit date of the first commit on the user branch is used as registration date of the + * account. The first commit may be an empty commit (if no properties were set and 'account.config' + * doesn't exist). + */ +public class AccountConfig extends VersionedMetaData implements ValidationError.Sink { + public static final String ACCOUNT_CONFIG = "account.config"; + public static final String ACCOUNT = "account"; + public static final String KEY_ACTIVE = "active"; + public static final String KEY_FULL_NAME = "fullName"; + public static final String KEY_PREFERRED_EMAIL = "preferredEmail"; + public static final String KEY_STATUS = "status"; + + @Nullable private final OutgoingEmailValidator emailValidator; + private final Account.Id accountId; + private final String ref; + + private boolean isLoaded; + private Account account; + private Timestamp registeredOn; + private List validationErrors; + + public AccountConfig(@Nullable OutgoingEmailValidator emailValidator, Account.Id accountId) { + this.emailValidator = emailValidator; + this.accountId = accountId; + this.ref = RefNames.refsUsers(accountId); + } + + @Override + protected String getRefName() { + return ref; + } + + /** + * Get the loaded account. + * + * @return loaded account. + * @throws IllegalStateException if the account was not loaded yet + */ + public Account getAccount() { + checkLoaded(); + return account; + } + + /** + * Sets the account. This means the loaded account will be overwritten with the given account. + * + *

Changing the registration date of an account is not supported. + * + * @param account account that should be set + * @throws IllegalStateException if the account was not loaded yet + */ + public void setAccount(Account account) { + checkLoaded(); + this.account = account; + this.registeredOn = account.getRegisteredOn(); + } + + /** + * Creates a new account. + * + * @return the new account + * @throws OrmDuplicateKeyException if the user branch already exists + */ + public Account getNewAccount() throws OrmDuplicateKeyException { + checkLoaded(); + if (revision != null) { + throw new OrmDuplicateKeyException(String.format("account %s already exists", accountId)); + } + this.registeredOn = TimeUtil.nowTs(); + this.account = new Account(accountId, registeredOn); + return account; + } + + @Override + protected void onLoad() throws IOException, ConfigInvalidException { + if (revision != null) { + rw.markStart(revision); + rw.sort(RevSort.REVERSE); + registeredOn = new Timestamp(rw.next().getCommitTime() * 1000L); + + Config cfg = readConfig(ACCOUNT_CONFIG); + + account = parse(cfg); + } + + isLoaded = true; + } + + private Account parse(Config cfg) { + Account account = new Account(accountId, registeredOn); + account.setActive(cfg.getBoolean(ACCOUNT, null, KEY_ACTIVE, true)); + account.setFullName(get(cfg, KEY_FULL_NAME)); + + String preferredEmail = get(cfg, KEY_PREFERRED_EMAIL); + account.setPreferredEmail(preferredEmail); + if (emailValidator != null && !emailValidator.isValid(preferredEmail)) { + error( + new ValidationError( + ACCOUNT_CONFIG, String.format("Invalid preferred email: %s", preferredEmail))); + } + + account.setStatus(get(cfg, KEY_STATUS)); + return account; + } + + @Override + protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException { + checkLoaded(); + + if (revision != null) { + commit.setMessage("Update account\n"); + } else if (account != null) { + commit.setMessage("Create account\n"); + commit.setAuthor(new PersonIdent(commit.getAuthor(), registeredOn)); + commit.setCommitter(new PersonIdent(commit.getCommitter(), registeredOn)); + } + + Config cfg = readConfig(ACCOUNT_CONFIG); + setActive(cfg, account.isActive()); + set(cfg, KEY_FULL_NAME, account.getFullName()); + set(cfg, KEY_PREFERRED_EMAIL, account.getPreferredEmail()); + set(cfg, KEY_STATUS, account.getStatus()); + saveConfig(ACCOUNT_CONFIG, cfg); + return true; + } + + /** + * Sets/Unsets {@code account.active} in the given config. + * + *

{@code account.active} is set to {@code false} if the account is inactive. + * + *

If the account is active {@code account.active} is unset since {@code true} is the default + * if this field is missing. + * + * @param cfg the config + * @param value whether the account is active + */ + private static void setActive(Config cfg, boolean value) { + if (!value) { + cfg.setBoolean(ACCOUNT, null, KEY_ACTIVE, false); + } else { + cfg.unset(ACCOUNT, null, KEY_ACTIVE); + } + } + + /** + * Sets/Unsets the given key in the given config. + * + *

The key unset if the value is {@code null}. + * + * @param cfg the config + * @param key the key + * @param value the value + */ + private static void set(Config cfg, String key, String value) { + if (!Strings.isNullOrEmpty(value)) { + cfg.setString(ACCOUNT, null, key, value); + } else { + cfg.unset(ACCOUNT, null, key); + } + } + + /** + * Gets the given key from the given config. + * + *

Empty values are returned as {@code null} + * + * @param cfg the config + * @param key the key + * @return the value, {@code null} if key was not set or key was set to empty string + */ + private static String get(Config cfg, String key) { + return Strings.emptyToNull(cfg.getString(ACCOUNT, null, key)); + } + + private void checkLoaded() { + checkState(isLoaded, "account not loaded yet"); + } + + /** + * Get the validation errors, if any were discovered during load. + * + * @return list of errors; empty list if there are no errors. + */ + public List getValidationErrors() { + if (validationErrors != null) { + return ImmutableList.copyOf(validationErrors); + } + return ImmutableList.of(); + } + + @Override + public void error(ValidationError error) { + if (validationErrors == null) { + validationErrors = new ArrayList<>(4); + } + validationErrors.add(error); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java index 861667b4f8..a6f1e2b790 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java @@ -16,7 +16,6 @@ package com.google.gerrit.server.account; import com.google.common.base.Strings; import com.google.gerrit.audit.AuditService; -import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.Permission; @@ -39,10 +38,13 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.List; import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.slf4j.Logger; @@ -150,7 +152,7 @@ public class AccountManager { private void update(ReviewDb db, AuthRequest who, ExternalId extId) throws OrmException, IOException, ConfigInvalidException { IdentifiedUser user = userFactory.create(extId.accountId()); - Account toUpdate = null; + List> accountUpdates = new ArrayList<>(); // If the email address was modified by the authentication provider, // update our records to match the changed email. @@ -159,8 +161,7 @@ public class AccountManager { String oldEmail = extId.email(); if (newEmail != null && !newEmail.equals(oldEmail)) { if (oldEmail != null && oldEmail.equals(user.getAccount().getPreferredEmail())) { - toUpdate = load(toUpdate, user.getAccountId(), db); - toUpdate.setPreferredEmail(newEmail); + accountUpdates.add(a -> a.setPreferredEmail(newEmail)); } externalIdsUpdateFactory @@ -172,8 +173,7 @@ public class AccountManager { if (!realm.allowsEdit(AccountFieldName.FULL_NAME) && !Strings.isNullOrEmpty(who.getDisplayName()) && !eq(user.getAccount().getFullName(), who.getDisplayName())) { - toUpdate = load(toUpdate, user.getAccountId(), db); - toUpdate.setFullName(who.getDisplayName()); + accountUpdates.add(a -> a.setFullName(who.getDisplayName())); } if (!realm.allowsEdit(AccountFieldName.USER_NAME) @@ -184,8 +184,12 @@ public class AccountManager { "Not changing already set username %s to %s", user.getUserName(), who.getUserName())); } - if (toUpdate != null) { - accountsUpdateFactory.create().update(db, toUpdate); + if (!accountUpdates.isEmpty()) { + Account account = + accountsUpdateFactory.create().update(db, user.getAccountId(), accountUpdates); + if (account == null) { + throw new OrmException("Account " + user.getAccountId() + " has been deleted"); + } } if (newEmail != null && !newEmail.equals(oldEmail)) { @@ -194,16 +198,6 @@ public class AccountManager { } } - private Account load(Account toUpdate, Account.Id accountId, ReviewDb db) throws OrmException { - if (toUpdate == null) { - toUpdate = accounts.get(db, accountId); - if (toUpdate == null) { - throw new OrmException("Account " + accountId + " has been deleted"); - } - } - return toUpdate; - } - private static boolean eq(String a, String b) { return (a == null && b == null) || (a != null && a.equals(b)); } @@ -211,18 +205,23 @@ public class AccountManager { private AuthResult create(ReviewDb db, AuthRequest who) throws OrmException, AccountException, IOException, ConfigInvalidException { Account.Id newId = new Account.Id(db.nextAccountId()); - Account account = new Account(newId, TimeUtil.nowTs()); ExternalId extId = ExternalId.createWithEmail(who.getExternalIdKey(), newId, who.getEmailAddress()); - account.setFullName(who.getDisplayName()); - account.setPreferredEmail(extId.email()); boolean isFirstAccount = awaitsFirstAccountCheck.getAndSet(false) && !accounts.hasAnyAccount(); + Account account; try { AccountsUpdate accountsUpdate = accountsUpdateFactory.create(); - accountsUpdate.upsert(db, account); + account = + accountsUpdate.insert( + db, + newId, + a -> { + a.setFullName(who.getDisplayName()); + a.setPreferredEmail(extId.email()); + }); ExternalId existingExtId = externalIds.get(extId.key()); if (existingExtId != null && !existingExtId.accountId().equals(extId.accountId())) { @@ -364,11 +363,16 @@ public class AccountManager { .insert(ExternalId.createWithEmail(who.getExternalIdKey(), to, who.getEmailAddress())); if (who.getEmailAddress() != null) { - Account a = accounts.get(db, to); - if (a.getPreferredEmail() == null) { - a.setPreferredEmail(who.getEmailAddress()); - accountsUpdateFactory.create().update(db, a); - } + accountsUpdateFactory + .create() + .update( + db, + to, + a -> { + if (a.getPreferredEmail() == null) { + a.setPreferredEmail(who.getEmailAddress()); + } + }); byEmailCache.evict(who.getEmailAddress()); } } @@ -428,12 +432,17 @@ public class AccountManager { externalIdsUpdateFactory.create().delete(extId); if (who.getEmailAddress() != null) { - Account a = accounts.get(db, from); - if (a.getPreferredEmail() != null - && a.getPreferredEmail().equals(who.getEmailAddress())) { - a.setPreferredEmail(null); - accountsUpdateFactory.create().update(db, a); - } + accountsUpdateFactory + .create() + .update( + db, + from, + a -> { + if (a.getPreferredEmail() != null + && a.getPreferredEmail().equals(who.getEmailAddress())) { + a.setPreferredEmail(null); + } + }); byEmailCache.evict(who.getEmailAddress()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java index 8ce5f4cd7e..7f66b9c000 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java @@ -23,12 +23,14 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class AccountResolver { @@ -61,7 +63,8 @@ public class AccountResolver { * @return the single account that matches; null if no account matches or there are multiple * candidates. */ - public Account find(ReviewDb db, String nameOrEmail) throws OrmException { + public Account find(ReviewDb db, String nameOrEmail) + throws OrmException, IOException, ConfigInvalidException { Set r = findAll(db, nameOrEmail); if (r.size() == 1) { return byId.get(r.iterator().next()).getAccount(); @@ -90,7 +93,8 @@ public class AccountResolver { * name ("username"). * @return the accounts that match, empty collection if none. Never null. */ - public Set findAll(ReviewDb db, String nameOrEmail) throws OrmException { + public Set findAll(ReviewDb db, String nameOrEmail) + throws OrmException, IOException, ConfigInvalidException { Matcher m = Pattern.compile("^.* \\(([1-9][0-9]*)\\)$").matcher(nameOrEmail); if (m.matches()) { Account.Id id = Account.Id.parse(m.group(1)); @@ -118,7 +122,8 @@ public class AccountResolver { return findAllByNameOrEmail(db, nameOrEmail); } - private boolean exists(ReviewDb db, Account.Id id) throws OrmException { + private boolean exists(ReviewDb db, Account.Id id) + throws OrmException, IOException, ConfigInvalidException { return accounts.get(db, id) != null; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java index 1a82f7b4b5..28ed422022 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java @@ -22,35 +22,70 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Objects; import java.util.Set; import java.util.stream.Stream; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Repository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** Class to access accounts. */ @Singleton public class Accounts { + private static final Logger log = LoggerFactory.getLogger(Accounts.class); + + private final boolean readFromGit; private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; + private final OutgoingEmailValidator emailValidator; @Inject - Accounts(GitRepositoryManager repoManager, AllUsersName allUsersName) { + Accounts( + @GerritServerConfig Config cfg, + GitRepositoryManager repoManager, + AllUsersName allUsersName, + OutgoingEmailValidator emailValidator) { + this.readFromGit = cfg.getBoolean("user", null, "readAccountsFromGit", false); this.repoManager = repoManager; this.allUsersName = allUsersName; + this.emailValidator = emailValidator; } - public Account get(ReviewDb db, Account.Id accountId) throws OrmException { + public Account get(ReviewDb db, Account.Id accountId) + throws OrmException, IOException, ConfigInvalidException { + if (readFromGit) { + try (Repository repo = repoManager.openRepository(allUsersName)) { + return read(repo, accountId); + } + } + return db.accounts().get(accountId); } - public List get(ReviewDb db, Collection accountIds) throws OrmException { + public List get(ReviewDb db, Collection accountIds) + throws OrmException, IOException, ConfigInvalidException { + if (readFromGit) { + List accounts = new ArrayList<>(accountIds.size()); + try (Repository repo = repoManager.openRepository(allUsersName)) { + for (Account.Id accountId : accountIds) { + accounts.add(read(repo, accountId)); + } + } + return accounts; + } + return db.accounts().get(accountIds).toList(); } @@ -59,7 +94,22 @@ public class Accounts { * * @return all accounts */ - public List all(ReviewDb db) throws OrmException { + public List all(ReviewDb db) throws OrmException, IOException { + if (readFromGit) { + Set accountIds = allIds(); + List accounts = new ArrayList<>(accountIds.size()); + try (Repository repo = repoManager.openRepository(allUsersName)) { + for (Account.Id accountId : accountIds) { + try { + accounts.add(read(repo, accountId)); + } catch (Exception e) { + log.error(String.format("Ignoring invalid account %s", accountId.get()), e); + } + } + } + return accounts; + } + return db.accounts().all().toList(); } @@ -103,6 +153,13 @@ public class Accounts { } } + private Account read(Repository allUsersRepository, Account.Id accountId) + throws IOException, ConfigInvalidException { + AccountConfig accountConfig = new AccountConfig(emailValidator, accountId); + accountConfig.load(allUsersRepository); + return accountConfig.getAccount(); + } + private static Stream readUserRefs(Repository repo) throws IOException { return repo.getRefDatabase() .getRefs(RefNames.REFS_USERS) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsCollection.java index 081ea26757..1669c4d06d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsCollection.java @@ -33,6 +33,8 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class AccountsCollection @@ -68,7 +70,8 @@ public class AccountsCollection @Override public AccountResource parse(TopLevelResource root, IdString id) - throws ResourceNotFoundException, AuthException, OrmException { + throws ResourceNotFoundException, AuthException, OrmException, IOException, + ConfigInvalidException { IdentifiedUser user = parseId(id.get()); if (user == null) { throw new ResourceNotFoundException(id); @@ -89,7 +92,8 @@ public class AccountsCollection * account is not visible to the calling user */ public IdentifiedUser parse(String id) - throws AuthException, UnprocessableEntityException, OrmException { + throws AuthException, UnprocessableEntityException, OrmException, IOException, + ConfigInvalidException { return parseOnBehalfOf(null, id); } @@ -104,8 +108,11 @@ public class AccountsCollection * @throws AuthException thrown if 'self' is used as account ID and the current user is not * authenticated * @throws OrmException + * @throws ConfigInvalidException + * @throws IOException */ - public IdentifiedUser parseId(String id) throws AuthException, OrmException { + public IdentifiedUser parseId(String id) + throws AuthException, OrmException, IOException, ConfigInvalidException { return parseIdOnBehalfOf(null, id); } @@ -113,7 +120,8 @@ public class AccountsCollection * Like {@link #parse(String)}, but also sets the {@link CurrentUser#getRealUser()} on the result. */ public IdentifiedUser parseOnBehalfOf(@Nullable CurrentUser caller, String id) - throws AuthException, UnprocessableEntityException, OrmException { + throws AuthException, UnprocessableEntityException, OrmException, IOException, + ConfigInvalidException { IdentifiedUser user = parseIdOnBehalfOf(caller, id); if (user == null) { throw new UnprocessableEntityException(String.format("Account Not Found: %s", id)); @@ -124,7 +132,7 @@ public class AccountsCollection } private IdentifiedUser parseIdOnBehalfOf(@Nullable CurrentUser caller, String id) - throws AuthException, OrmException { + throws AuthException, OrmException, IOException, ConfigInvalidException { if (id.equals("self")) { CurrentUser user = self.get(); if (user.isIdentifiedUser()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java index f6ed598f00..ef501ea58b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -16,15 +16,21 @@ package com.google.gerrit.server.account; import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.index.change.ReindexAfterRefUpdate; +import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -32,7 +38,9 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.sql.Timestamp; +import java.util.List; import java.util.function.Consumer; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -46,8 +54,18 @@ import org.eclipse.jgit.lib.Repository; /** * Updates accounts. * - *

On updating accounts this class takes care to evict them from the account cache and thus - * triggers reindex for them. + *

The account updates are written to both ReviewDb and NoteDb. + * + *

In NoteDb accounts are represented as user branches in the All-Users repository. Optionally a + * user branch can contain a 'account.config' file that stores account properties, such as full + * name, preferred email, status and the active flag. The timestamp of the first commit on a user + * branch denotes the registration date. The initial commit on the user branch may be empty (since + * having an 'account.config' is optional). See {@link AccountConfig} for details of the + * 'account.config' file format. + * + *

On updating accounts the accounts are evicted from the account cache and thus reindexed. The + * eviction from the account cache is done by the {@link ReindexAfterRefUpdate} class which receives + * the event about updating the user branch that is triggered by this class. */ @Singleton public class AccountsUpdate { @@ -60,56 +78,38 @@ public class AccountsUpdate { @Singleton public static class Server { private final GitRepositoryManager repoManager; - private final AccountCache accountCache; + private final GitReferenceUpdated gitRefUpdated; private final AllUsersName allUsersName; + private final OutgoingEmailValidator emailValidator; private final Provider serverIdent; + private final Provider metaDataUpdateServerFactory; @Inject public Server( GitRepositoryManager repoManager, - AccountCache accountCache, + GitReferenceUpdated gitRefUpdated, AllUsersName allUsersName, - @GerritPersonIdent Provider serverIdent) { + OutgoingEmailValidator emailValidator, + @GerritPersonIdent Provider serverIdent, + Provider metaDataUpdateServerFactory) { this.repoManager = repoManager; - this.accountCache = accountCache; + this.gitRefUpdated = gitRefUpdated; this.allUsersName = allUsersName; + this.emailValidator = emailValidator; this.serverIdent = serverIdent; + this.metaDataUpdateServerFactory = metaDataUpdateServerFactory; } public AccountsUpdate create() { PersonIdent i = serverIdent.get(); - return new AccountsUpdate(repoManager, accountCache, allUsersName, i, i); - } - } - - /** - * Factory to create an AccountsUpdate instance for updating accounts by the Gerrit server. - * - *

Using this class will not perform reindexing for the updated accounts and they will also not - * be evicted from the account cache. - * - *

The Gerrit server identity will be used as author and committer for all commits that update - * the accounts. - */ - @Singleton - public static class ServerNoReindex { - private final GitRepositoryManager repoManager; - private final AllUsersName allUsersName; - private final Provider serverIdent; - - @Inject - public ServerNoReindex( - GitRepositoryManager repoManager, - AllUsersName allUsersName, - @GerritPersonIdent Provider serverIdent) { - this.repoManager = repoManager; - this.allUsersName = allUsersName; - this.serverIdent = serverIdent; - } - - public AccountsUpdate create() { - PersonIdent i = serverIdent.get(); - return new AccountsUpdate(repoManager, null, allUsersName, i, i); + return new AccountsUpdate( + repoManager, + gitRefUpdated, + null, + allUsersName, + emailValidator, + i, + () -> metaDataUpdateServerFactory.get().create(allUsersName)); } } @@ -122,29 +122,42 @@ public class AccountsUpdate { @Singleton public static class User { private final GitRepositoryManager repoManager; - private final AccountCache accountCache; + private final GitReferenceUpdated gitRefUpdated; private final AllUsersName allUsersName; + private final OutgoingEmailValidator emailValidator; private final Provider serverIdent; private final Provider identifiedUser; + private final Provider metaDataUpdateUserFactory; @Inject public User( GitRepositoryManager repoManager, - AccountCache accountCache, + GitReferenceUpdated gitRefUpdated, AllUsersName allUsersName, + OutgoingEmailValidator emailValidator, @GerritPersonIdent Provider serverIdent, - Provider identifiedUser) { + Provider identifiedUser, + Provider metaDataUpdateUserFactory) { this.repoManager = repoManager; - this.accountCache = accountCache; + this.gitRefUpdated = gitRefUpdated; this.allUsersName = allUsersName; this.serverIdent = serverIdent; + this.emailValidator = emailValidator; this.identifiedUser = identifiedUser; + this.metaDataUpdateUserFactory = metaDataUpdateUserFactory; } public AccountsUpdate create() { + IdentifiedUser user = identifiedUser.get(); PersonIdent i = serverIdent.get(); return new AccountsUpdate( - repoManager, accountCache, allUsersName, createPersonIdent(i, identifiedUser.get()), i); + repoManager, + gitRefUpdated, + user, + allUsersName, + emailValidator, + createPersonIdent(i, user), + () -> metaDataUpdateUserFactory.get().create(allUsersName)); } private PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) { @@ -153,117 +166,178 @@ public class AccountsUpdate { } private final GitRepositoryManager repoManager; - @Nullable private final AccountCache accountCache; + private final GitReferenceUpdated gitRefUpdated; + @Nullable private final IdentifiedUser currentUser; private final AllUsersName allUsersName; + private final OutgoingEmailValidator emailValidator; private final PersonIdent committerIdent; - private final PersonIdent authorIdent; + private final MetaDataUpdateFactory metaDataUpdateFactory; private AccountsUpdate( GitRepositoryManager repoManager, - @Nullable AccountCache accountCache, + GitReferenceUpdated gitRefUpdated, + @Nullable IdentifiedUser currentUser, AllUsersName allUsersName, + OutgoingEmailValidator emailValidator, PersonIdent committerIdent, - PersonIdent authorIdent) { + MetaDataUpdateFactory metaDataUpdateFactory) { this.repoManager = checkNotNull(repoManager, "repoManager"); - this.accountCache = accountCache; + this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated"); + this.currentUser = currentUser; this.allUsersName = checkNotNull(allUsersName, "allUsersName"); + this.emailValidator = checkNotNull(emailValidator, "emailValidator"); this.committerIdent = checkNotNull(committerIdent, "committerIdent"); - this.authorIdent = checkNotNull(authorIdent, "authorIdent"); + this.metaDataUpdateFactory = checkNotNull(metaDataUpdateFactory, "metaDataUpdateFactory"); } /** * Inserts a new account. * + * @param db ReviewDb + * @param accountId ID of the new account + * @param init consumer to populate the new account + * @return the newly created account + * @throws OrmException if updating the database fails * @throws OrmDuplicateKeyException if the account already exists * @throws IOException if updating the user branch fails + * @throws ConfigInvalidException if any of the account fields has an invalid value */ - public void insert(ReviewDb db, Account account) throws OrmException, IOException { + public Account insert(ReviewDb db, Account.Id accountId, Consumer init) + throws OrmException, IOException, ConfigInvalidException { + AccountConfig accountConfig = read(accountId); + Account account = accountConfig.getNewAccount(); + init.accept(account); + + // Create in ReviewDb db.accounts().insert(ImmutableSet.of(account)); - createUserBranch(account); - evictAccount(account.getId()); - } - /** - * Inserts or updates an account. - * - *

If the account already exists, it is overwritten, otherwise it is inserted. - */ - public void upsert(ReviewDb db, Account account) throws OrmException, IOException { - db.accounts().upsert(ImmutableSet.of(account)); - createUserBranchIfNeeded(account); - evictAccount(account.getId()); - } - - /** Updates the account. */ - public void update(ReviewDb db, Account account) throws OrmException, IOException { - db.accounts().update(ImmutableSet.of(account)); - evictAccount(account.getId()); + // Create in NoteDb + commitNew(accountConfig); + return account; } /** * Gets the account and updates it atomically. * + *

Changing the registration date of an account is not supported. + * * @param db ReviewDb * @param accountId ID of the account * @param consumer consumer to update the account, only invoked if the account exists * @return the updated account, {@code null} if the account doesn't exist - * @throws OrmException if updating the account fails + * @throws OrmException if updating the database fails + * @throws IOException if updating the user branch fails + * @throws ConfigInvalidException if any of the account fields has an invalid value */ - public Account atomicUpdate(ReviewDb db, Account.Id accountId, Consumer consumer) - throws OrmException, IOException { - Account account = - db.accounts() - .atomicUpdate( - accountId, - a -> { - consumer.accept(a); - return a; - }); - evictAccount(accountId); + public Account update(ReviewDb db, Account.Id accountId, Consumer consumer) + throws OrmException, IOException, ConfigInvalidException { + return update(db, accountId, ImmutableList.of(consumer)); + } + + /** + * Gets the account and updates it atomically. + * + *

Changing the registration date of an account is not supported. + * + * @param db ReviewDb + * @param accountId ID of the account + * @param consumers consumers to update the account, only invoked if the account exists + * @return the updated account, {@code null} if the account doesn't exist + * @throws OrmException if updating the database fails + * @throws IOException if updating the user branch fails + * @throws ConfigInvalidException if any of the account fields has an invalid value + */ + public Account update(ReviewDb db, Account.Id accountId, List> consumers) + throws OrmException, IOException, ConfigInvalidException { + if (consumers.isEmpty()) { + return null; + } + + // Update in ReviewDb + db.accounts() + .atomicUpdate( + accountId, + a -> { + consumers.stream().forEach(c -> c.accept(a)); + return a; + }); + + // Update in NoteDb + AccountConfig accountConfig = read(accountId); + Account account = accountConfig.getAccount(); + consumers.stream().forEach(c -> c.accept(account)); + commit(accountConfig); + return account; } - /** Deletes the account. */ + /** + * Replaces the account. + * + *

The existing account with the same account ID is overwritten by the given account. Choosing + * to overwrite an account means that any updates that were done to the account by a racing + * request after the account was read are lost. Updates are also lost if the account was read from + * a stale account index. This is why using {@link #update(ReviewDb, + * com.google.gerrit.reviewdb.client.Account.Id, Consumer)} to do an atomic update is always + * preferred. + * + *

Changing the registration date of an account is not supported. + * + * @param db ReviewDb + * @param account the new account + * @throws OrmException if updating the database fails + * @throws IOException if updating the user branch fails + * @throws ConfigInvalidException if any of the account fields has an invalid value + * @see #update(ReviewDb, com.google.gerrit.reviewdb.client.Account.Id, Consumer) + */ + public void replace(ReviewDb db, Account account) + throws OrmException, IOException, ConfigInvalidException { + // Update in ReviewDb + db.accounts().update(ImmutableSet.of(account)); + + // Update in NoteDb + AccountConfig accountConfig = read(account.getId()); + accountConfig.setAccount(account); + commit(accountConfig); + } + + /** + * Deletes the account. + * + * @param db ReviewDb + * @param account the account that should be deleted + * @throws OrmException if updating the database fails + * @throws IOException if updating the user branch fails + */ public void delete(ReviewDb db, Account account) throws OrmException, IOException { + // Delete in ReviewDb db.accounts().delete(ImmutableSet.of(account)); + + // Delete in NoteDb deleteUserBranch(account.getId()); - evictAccount(account.getId()); } - /** Deletes the account. */ + /** + * Deletes the account. + * + * @param db ReviewDb + * @param accountId the ID of the account that should be deleted + * @throws OrmException if updating the database fails + * @throws IOException if updating the user branch fails + */ public void deleteByKey(ReviewDb db, Account.Id accountId) throws OrmException, IOException { + // Delete in ReviewDb db.accounts().deleteKeys(ImmutableSet.of(accountId)); + + // Delete in NoteDb deleteUserBranch(accountId); - evictAccount(accountId); - } - - private void createUserBranch(Account account) throws IOException { - try (Repository repo = repoManager.openRepository(allUsersName); - ObjectInserter oi = repo.newObjectInserter()) { - String refName = RefNames.refsUsers(account.getId()); - if (repo.exactRef(refName) != null) { - throw new IOException( - String.format( - "User branch %s for newly created account %s already exists.", - refName, account.getId().get())); - } - createUserBranch( - repo, oi, committerIdent, authorIdent, account.getId(), account.getRegisteredOn()); - } - } - - private void createUserBranchIfNeeded(Account account) throws IOException { - try (Repository repo = repoManager.openRepository(allUsersName); - ObjectInserter oi = repo.newObjectInserter()) { - if (repo.exactRef(RefNames.refsUsers(account.getId())) == null) { - createUserBranch( - repo, oi, committerIdent, authorIdent, account.getId(), account.getRegisteredOn()); - } - } } public static void createUserBranch( Repository repo, + Project.NameKey project, + GitReferenceUpdated gitRefUpdated, + @Nullable IdentifiedUser user, ObjectInserter oi, PersonIdent committerIdent, PersonIdent authorIdent, @@ -283,6 +357,7 @@ public class AccountsUpdate { if (result != Result.NEW) { throw new IOException(String.format("Failed to update ref %s: %s", refName, result.name())); } + gitRefUpdated.fire(project, ru, user != null ? user.getAccount() : null); } private static ObjectId createInitialEmptyCommit( @@ -307,12 +382,18 @@ public class AccountsUpdate { private void deleteUserBranch(Account.Id accountId) throws IOException { try (Repository repo = repoManager.openRepository(allUsersName)) { - deleteUserBranch(repo, committerIdent, accountId); + deleteUserBranch(repo, allUsersName, gitRefUpdated, currentUser, committerIdent, accountId); } } public static void deleteUserBranch( - Repository repo, PersonIdent refLogIdent, Account.Id accountId) throws IOException { + Repository repo, + Project.NameKey project, + GitReferenceUpdated gitRefUpdated, + @Nullable IdentifiedUser user, + PersonIdent refLogIdent, + Account.Id accountId) + throws IOException { String refName = RefNames.refsUsers(accountId); Ref ref = repo.exactRef(refName); if (ref == null) { @@ -329,11 +410,37 @@ public class AccountsUpdate { if (result != Result.FORCED) { throw new IOException(String.format("Failed to delete ref %s: %s", refName, result.name())); } + gitRefUpdated.fire(project, ru, user != null ? user.getAccount() : null); } - private void evictAccount(Account.Id accountId) throws IOException { - if (accountCache != null) { - accountCache.evict(accountId); + private AccountConfig read(Account.Id accountId) throws IOException, ConfigInvalidException { + try (Repository repo = repoManager.openRepository(allUsersName)) { + AccountConfig accountConfig = new AccountConfig(emailValidator, accountId); + accountConfig.load(repo); + return accountConfig; } } + + private void commitNew(AccountConfig accountConfig) throws IOException { + // When creating a new account we must allow empty commits so that the user branch gets created + // with an empty commit when no account properties are set and hence no 'account.config' file + // will be created. + commit(accountConfig, true); + } + + private void commit(AccountConfig accountConfig) throws IOException { + commit(accountConfig, false); + } + + private void commit(AccountConfig accountConfig, boolean allowEmptyCommit) throws IOException { + try (MetaDataUpdate md = metaDataUpdateFactory.create()) { + md.setAllowEmpty(allowEmptyCommit); + accountConfig.commit(md); + } + } + + @FunctionalInterface + private static interface MetaDataUpdateFactory { + MetaDataUpdate create() throws IOException; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java index d617365058..ff367e9d6f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java @@ -17,7 +17,7 @@ package com.google.gerrit.server.account; import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_MAILTO; import com.google.gerrit.audit.AuditService; -import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.common.errors.InvalidSshKeyException; @@ -113,12 +113,15 @@ public class CreateAccount implements RestModifyView apply(TopLevelResource rsrc, AccountInput input) + public Response apply(TopLevelResource rsrc, @Nullable AccountInput input) + throws BadRequestException, ResourceConflictException, UnprocessableEntityException, + OrmException, IOException, ConfigInvalidException { + return apply(input != null ? input : new AccountInput()); + } + + public Response apply(AccountInput input) throws BadRequestException, ResourceConflictException, UnprocessableEntityException, OrmException, IOException, ConfigInvalidException { - if (input == null) { - input = new AccountInput(); - } if (input.username != null && !username.equals(input.username)) { throw new BadRequestException("username must match URL"); } @@ -171,10 +174,15 @@ public class CreateAccount implements RestModifyView { + a.setFullName(input.name); + a.setPreferredEmail(input.email); + }); for (AccountGroup.Id groupId : groups) { AccountGroupMember m = new AccountGroupMember(new AccountGroupMember.Key(id, groupId)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java index 3a996f22b6..1fa8ed3004 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java @@ -31,6 +31,7 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.concurrent.atomic.AtomicBoolean; +import org.eclipse.jgit.errors.ConfigInvalidException; @RequiresCapability(GlobalCapability.MODIFY_ACCOUNT) @Singleton @@ -53,7 +54,7 @@ public class DeleteActive implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) - throws RestApiException, OrmException, IOException { + throws RestApiException, OrmException, IOException, ConfigInvalidException { if (self.get() == rsrc.getUser()) { throw new ResourceConflictException("cannot deactivate own account"); } @@ -62,7 +63,7 @@ public class DeleteActive implements RestModifyView { Account account = accountsUpdate .create() - .atomicUpdate( + .update( dbProvider.get(), rsrc.getUser().getAccountId(), a -> { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutActive.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutActive.java index bc7c1d0615..6051a950c3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutActive.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutActive.java @@ -28,6 +28,7 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.concurrent.atomic.AtomicBoolean; +import org.eclipse.jgit.errors.ConfigInvalidException; @RequiresCapability(GlobalCapability.MODIFY_ACCOUNT) @Singleton @@ -45,12 +46,12 @@ public class PutActive implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) - throws ResourceNotFoundException, OrmException, IOException { + throws ResourceNotFoundException, OrmException, IOException, ConfigInvalidException { AtomicBoolean alreadyActive = new AtomicBoolean(false); Account account = accountsUpdate .create() - .atomicUpdate( + .update( dbProvider.get(), rsrc.getUser().getAccountId(), a -> { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java index 091182087b..792e71de4d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java @@ -35,6 +35,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PutName implements RestModifyView { @@ -65,7 +66,7 @@ public class PutName implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) throws AuthException, MethodNotAllowedException, ResourceNotFoundException, OrmException, - IOException, PermissionBackendException { + IOException, PermissionBackendException, ConfigInvalidException { if (self.get() != rsrc.getUser()) { permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT); } @@ -73,7 +74,8 @@ public class PutName implements RestModifyView { } public Response apply(IdentifiedUser user, Input input) - throws MethodNotAllowedException, ResourceNotFoundException, OrmException, IOException { + throws MethodNotAllowedException, ResourceNotFoundException, OrmException, IOException, + ConfigInvalidException { if (input == null) { input = new Input(); } @@ -86,7 +88,7 @@ public class PutName implements RestModifyView { Account account = accountsUpdate .create() - .atomicUpdate(dbProvider.get(), user.getAccountId(), a -> a.setFullName(newName)); + .update(dbProvider.get(), user.getAccountId(), a -> a.setFullName(newName)); if (account == null) { throw new ResourceNotFoundException("account not found"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java index d473d53ad2..98d4ac5cc6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java @@ -32,6 +32,7 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.concurrent.atomic.AtomicBoolean; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PutPreferred implements RestModifyView { @@ -57,7 +58,7 @@ public class PutPreferred implements RestModifyView apply(AccountResource.Email rsrc, Input input) throws AuthException, ResourceNotFoundException, OrmException, IOException, - PermissionBackendException { + PermissionBackendException, ConfigInvalidException { if (self.get() != rsrc.getUser()) { permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT); } @@ -65,12 +66,12 @@ public class PutPreferred implements RestModifyView apply(IdentifiedUser user, String email) - throws ResourceNotFoundException, OrmException, IOException { + throws ResourceNotFoundException, OrmException, IOException, ConfigInvalidException { AtomicBoolean alreadyPreferred = new AtomicBoolean(false); Account account = accountsUpdate .create() - .atomicUpdate( + .update( dbProvider.get(), user.getAccountId(), a -> { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java index 81c0694035..136fc68a7a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java @@ -33,6 +33,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PutStatus implements RestModifyView { @@ -66,7 +67,7 @@ public class PutStatus implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) throws AuthException, ResourceNotFoundException, OrmException, IOException, - PermissionBackendException { + PermissionBackendException, ConfigInvalidException { if (self.get() != rsrc.getUser()) { permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT); } @@ -74,7 +75,7 @@ public class PutStatus implements RestModifyView { } public Response apply(IdentifiedUser user, Input input) - throws ResourceNotFoundException, OrmException, IOException { + throws ResourceNotFoundException, OrmException, IOException, ConfigInvalidException { if (input == null) { input = new Input(); } @@ -83,7 +84,7 @@ public class PutStatus implements RestModifyView { Account account = accountsUpdate .create() - .atomicUpdate( + .update( dbProvider.get(), user.getAccountId(), a -> a.setStatus(Strings.nullToEmpty(newStatus))); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java index 75628015bb..ce31cac875 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java @@ -27,6 +27,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.kohsuke.args4j.CmdLineException; import org.kohsuke.args4j.CmdLineParser; import org.kohsuke.args4j.OptionDef; @@ -82,8 +83,12 @@ public class AccountIdHandler extends OptionHandler { throw new CmdLineException(owner, "user \"" + token + "\" not found"); } } - } catch (OrmException | IOException e) { + } catch (OrmException e) { throw new CmdLineException(owner, "database is down"); + } catch (IOException e) { + throw new CmdLineException(owner, "Failed to load account", e); + } catch (ConfigInvalidException e) { + throw new CmdLineException(owner, "Invalid account config", e); } setter.addValue(accountId); return 1; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java index 7bf42145c8..5073e4a7a0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java @@ -41,7 +41,9 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; import java.util.Collection; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class Abandon extends RetryingRestModifyView @@ -68,7 +70,8 @@ public class Abandon extends RetryingRestModifyView applyImpl( BatchUpdate.Factory updateFactory, TopLevelResource parent, ChangeInput input) throws OrmException, IOException, InvalidChangeOperationException, RestApiException, - UpdateException, PermissionBackendException { + UpdateException, PermissionBackendException, ConfigInvalidException { if (Strings.isNullOrEmpty(input.project)) { throw new BadRequestException("project must be non-empty"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/NotifyUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/NotifyUtil.java index 8516615870..ccc758735c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/NotifyUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/NotifyUtil.java @@ -31,10 +31,12 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class NotifyUtil { @@ -76,7 +78,7 @@ public class NotifyUtil { public ListMultimap resolveAccounts( @Nullable Map notifyDetails) - throws OrmException, BadRequestException { + throws OrmException, BadRequestException, IOException, ConfigInvalidException { if (isNullOrEmpty(notifyDetails)) { return ImmutableListMultimap.of(); } @@ -96,7 +98,7 @@ public class NotifyUtil { } private List find(ReviewDb db, List nameOrEmails) - throws OrmException, BadRequestException { + throws OrmException, BadRequestException, IOException, ConfigInvalidException { List missing = new ArrayList<>(nameOrEmails.size()); List r = new ArrayList<>(nameOrEmails.size()); for (String nameOrEmail : nameOrEmails) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index d6ad28bbb9..f5a185646e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -126,6 +126,7 @@ import java.util.Map; import java.util.Objects; import java.util.OptionalInt; import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; @Singleton @@ -190,14 +191,14 @@ public class PostReview protected Response applyImpl( BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input) throws RestApiException, UpdateException, OrmException, IOException, - PermissionBackendException { + PermissionBackendException, ConfigInvalidException { return apply(updateFactory, revision, input, TimeUtil.nowTs()); } public Response apply( BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input, Timestamp ts) throws RestApiException, UpdateException, OrmException, IOException, - PermissionBackendException { + PermissionBackendException, ConfigInvalidException { // Respect timestamp, but truncate at change created-on time. ts = Ordering.natural().max(ts, revision.getChange().getCreatedOn()); if (revision.getEdit().isPresent()) { @@ -367,7 +368,7 @@ public class PostReview private RevisionResource onBehalfOf(RevisionResource rev, ReviewInput in) throws BadRequestException, AuthException, UnprocessableEntityException, OrmException, - PermissionBackendException { + PermissionBackendException, IOException, ConfigInvalidException { if (in.labels == null || in.labels.isEmpty()) { throw new AuthException( String.format("label required to post review on behalf of \"%s\"", in.onBehalfOf)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index c7b003159d..a7711b68c7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -76,6 +76,7 @@ import java.text.MessageFormat; import java.util.Collection; import java.util.HashSet; import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; @Singleton @@ -148,7 +149,7 @@ public class PostReviewers protected AddReviewerResult applyImpl( BatchUpdate.Factory updateFactory, ChangeResource rsrc, AddReviewerInput input) throws IOException, OrmException, RestApiException, UpdateException, - PermissionBackendException { + PermissionBackendException, ConfigInvalidException { if (input.reviewer == null) { throw new BadRequestException("missing reviewer field"); } @@ -170,7 +171,7 @@ public class PostReviewers public Addition prepareApplication( ChangeResource rsrc, AddReviewerInput input, boolean allowGroup) - throws OrmException, IOException, PermissionBackendException { + throws OrmException, IOException, PermissionBackendException, ConfigInvalidException { String reviewer = input.reviewer; ReviewerState state = input.state(); NotifyHandling notify = input.notify; @@ -219,7 +220,7 @@ public class PostReviewers ListMultimap accountsToNotify, boolean allowGroup, boolean allowByEmail) - throws OrmException, PermissionBackendException { + throws OrmException, PermissionBackendException, IOException, ConfigInvalidException { Account.Id accountId = null; try { accountId = accounts.parse(reviewer).getAccountId(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java index 53f127e687..9ef445d59b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java @@ -45,6 +45,7 @@ import java.io.IOException; import java.io.OutputStream; import java.util.Collection; import org.apache.commons.compress.archivers.ArchiveOutputStream; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; @@ -82,7 +83,7 @@ public class PreviewSubmit implements RestReadView { @Override public BinaryResult apply(RevisionResource rsrc) - throws OrmException, RestApiException, UpdateException { + throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException { if (Strings.isNullOrEmpty(format)) { throw new BadRequestException("format is not specified"); } @@ -110,7 +111,7 @@ public class PreviewSubmit implements RestReadView { } private BinaryResult getBundles(RevisionResource rsrc, ArchiveFormat f) - throws OrmException, RestApiException, UpdateException { + throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException { ReviewDb db = dbProvider.get(); ChangeControl control = rsrc.getControl(); IdentifiedUser caller = control.getUser().asIdentifiedUser(); @@ -125,7 +126,12 @@ public class PreviewSubmit implements RestReadView { .setContentType(f.getMimeType()) .setAttachmentName("submit-preview-" + change.getChangeId() + "." + format); return bin; - } catch (OrmException | RestApiException | UpdateException | RuntimeException e) { + } catch (OrmException + | RestApiException + | UpdateException + | IOException + | ConfigInvalidException + | RuntimeException e) { op.close(); throw e; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java index eab06fb0d6..cbb5fa3bc5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java @@ -35,6 +35,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; import java.util.Optional; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PublishChangeEdit @@ -85,7 +86,8 @@ public class PublishChangeEdit @Override protected Response applyImpl( BatchUpdate.Factory updateFactory, ChangeResource rsrc, PublishChangeEditInput in) - throws IOException, OrmException, RestApiException, UpdateException { + throws IOException, OrmException, RestApiException, UpdateException, + ConfigInvalidException { CreateChange.checkValidCLA(rsrc.getControl().getProjectControl()); Optional edit = editUtil.byChange(rsrc.getChange()); if (!edit.isPresent()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutAssignee.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutAssignee.java index b07d24b38a..a0862d6c44 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutAssignee.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutAssignee.java @@ -42,6 +42,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PutAssignee extends RetryingRestModifyView @@ -73,7 +74,7 @@ public class PutAssignee extends RetryingRestModifyView applyImpl( BatchUpdate.Factory updateFactory, ChangeResource resource, Input input) throws IOException, UnchangedCommitMessageException, RestApiException, UpdateException, - PermissionBackendException, OrmException { + PermissionBackendException, OrmException, ConfigInvalidException { PatchSet ps = psUtil.current(db.get(), resource.getNotes()); if (ps == null) { throw new ResourceConflictException("current revision is missing"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java index 0762f0e125..8794083b7c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java @@ -30,7 +30,9 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; import java.util.Collection; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class Reviewers implements ChildCollection { @@ -69,7 +71,8 @@ public class Reviewers implements ChildCollection { @@ -70,7 +72,8 @@ public class RevisionReviewers implements ChildCollection apply(ChangeResource rsrc) - throws AuthException, BadRequestException, OrmException, IOException { + throws AuthException, BadRequestException, OrmException, IOException, ConfigInvalidException { if (!self.get().isIdentifiedUser()) { throw new AuthException("Authentication required"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckAccess.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckAccess.java index 84db266392..c89684c060 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckAccess.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckAccess.java @@ -39,6 +39,7 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class CheckAccess implements RestModifyView { @@ -67,7 +68,8 @@ public class CheckAccess implements RestModifyView queryProvider; private final ChangeNotes.Factory notesFactory; - private final Accounts accounts; private final AccountsUpdate.Server accountsUpdate; private final AccountResolver accountResolver; private final PermissionBackend permissionBackend; @@ -376,7 +375,6 @@ public class ReceiveCommits { Sequences seq, Provider queryProvider, ChangeNotes.Factory notesFactory, - Accounts accounts, AccountsUpdate.Server accountsUpdate, AccountResolver accountResolver, PermissionBackend permissionBackend, @@ -416,7 +414,6 @@ public class ReceiveCommits { this.seq = seq; this.queryProvider = queryProvider; this.notesFactory = notesFactory; - this.accounts = accounts; this.accountsUpdate = accountsUpdate; this.accountResolver = accountResolver; this.permissionBackend = permissionBackend; @@ -815,7 +812,11 @@ public class ReceiveCommits { } catch (ResourceConflictException e) { addMessage(e.getMessage()); reject(magicBranchCmd, "conflict"); - } catch (RestApiException | OrmException | UpdateException e) { + } catch (RestApiException + | OrmException + | UpdateException + | IOException + | ConfigInvalidException e) { logError("Error submitting changes to " + project.getName(), e); reject(magicBranchCmd, "error during submit"); } @@ -2258,7 +2259,7 @@ public class ReceiveCommits { } private void submit(Collection create, Collection replace) - throws OrmException, RestApiException, UpdateException { + throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException { Map bySha = Maps.newHashMapWithExpectedSize(create.size() + replace.size()); for (CreateRequest r : create) { checkNotNull(r.change, "cannot submit new change %s; op may not have run", r.changeId); @@ -2783,13 +2784,22 @@ public class ReceiveCommits { if (defaultName && user.hasEmailAddress(c.getCommitterIdent().getEmailAddress())) { try { - Account a = accounts.get(db, user.getAccountId()); - if (a != null && Strings.isNullOrEmpty(a.getFullName())) { - a.setFullName(c.getCommitterIdent().getName()); - accountsUpdate.create().update(db, a); - user.getAccount().setFullName(a.getFullName()); + String committerName = c.getCommitterIdent().getName(); + Account account = + accountsUpdate + .create() + .update( + db, + user.getAccountId(), + a -> { + if (Strings.isNullOrEmpty(a.getFullName())) { + a.setFullName(committerName); + } + }); + if (account != null && Strings.isNullOrEmpty(account.getFullName())) { + user.getAccount().setFullName(account.getFullName()); } - } catch (OrmException e) { + } catch (OrmException | IOException | ConfigInvalidException e) { logWarn("Cannot default full_name", e); } finally { defaultName = false; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java index 6b6d97f611..8b7df19b86 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java @@ -75,6 +75,7 @@ public abstract class VersionedMetaData { } protected RevCommit revision; + protected RevWalk rw; protected ObjectReader reader; protected ObjectInserter inserter; protected DirCache newTree; @@ -153,11 +154,13 @@ public abstract class VersionedMetaData { * @throws ConfigInvalidException */ public void load(RevWalk walk, ObjectId id) throws IOException, ConfigInvalidException { + this.rw = walk; this.reader = walk.getObjectReader(); try { revision = id != null ? walk.parseCommit(id) : null; onLoad(); } finally { + walk = null; reader = null; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index ff755ead31..bdb0db998a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.AccountConfig; import com.google.gerrit.server.account.WatchConfig; import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker; import com.google.gerrit.server.config.AllUsersName; @@ -57,9 +58,11 @@ import java.net.URL; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Objects; import java.util.regex.Pattern; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.notes.NoteMap; @@ -67,6 +70,7 @@ import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.FooterLine; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.util.SystemReader; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -120,7 +124,8 @@ public class CommitValidators { new ConfigValidator(refctl, rw, allUsers), new BannedCommitsValidator(rejectCommits), new PluginCommitValidationListener(pluginValidators), - new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker))); + new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), + new AccountValidator(allUsers))); } public CommitValidators forGerritCommits( @@ -135,7 +140,8 @@ public class CommitValidators { new ChangeIdValidator(refctl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo), new ConfigValidator(refctl, rw, allUsers), new PluginCommitValidationListener(pluginValidators), - new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker))); + new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), + new AccountValidator(allUsers))); } public CommitValidators forMergedCommits(PermissionBackend.ForRef perm, RefControl refControl) { @@ -679,6 +685,60 @@ public class CommitValidators { } } + /** Rejects updates to 'account.config' in user branches. */ + public static class AccountValidator implements CommitValidationListener { + private final AllUsersName allUsers; + + public AccountValidator(AllUsersName allUsers) { + this.allUsers = allUsers; + } + + @Override + public List onCommitReceived(CommitReceivedEvent receiveEvent) + throws CommitValidationException { + if (!allUsers.equals(receiveEvent.project.getNameKey())) { + return Collections.emptyList(); + } + + if (receiveEvent.command.getRefName().startsWith(MagicBranch.NEW_CHANGE)) { + // no validation on push for review, will be checked on submit by + // MergeValidators.AccountValidator + return Collections.emptyList(); + } + + Account.Id accountId = Account.Id.fromRef(receiveEvent.refName); + if (accountId == null) { + return Collections.emptyList(); + } + + try { + ObjectId newBlobId = getAccountConfigBlobId(receiveEvent.revWalk, receiveEvent.commit); + + ObjectId oldId = receiveEvent.command.getOldId(); + ObjectId oldBlobId = + !ObjectId.zeroId().equals(oldId) + ? getAccountConfigBlobId(receiveEvent.revWalk, oldId) + : null; + if (!Objects.equals(oldBlobId, newBlobId)) { + throw new CommitValidationException("account update not allowed"); + } + } catch (IOException e) { + String m = String.format("Validating update for account %s failed", accountId.get()); + log.error(m, e); + throw new CommitValidationException(m, e); + } + return Collections.emptyList(); + } + + private ObjectId getAccountConfigBlobId(RevWalk rw, ObjectId id) throws IOException { + RevCommit commit = rw.parseCommit(id); + try (TreeWalk tw = + TreeWalk.forPath(rw.getObjectReader(), AccountConfig.ACCOUNT_CONFIG, commit.getTree())) { + return tw != null ? tw.getObjectId(0) : null; + } + } + } + private static CommitValidationMessage invalidEmail( RevCommit c, String type, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java index 298c65058f..af9f6d52ea 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java @@ -20,12 +20,16 @@ import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.registration.DynamicMap.Entry; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.AccountConfig; import com.google.gerrit.server.config.AllProjectsName; +import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.config.ProjectConfigEntry; import com.google.gerrit.server.git.CodeReviewCommit; @@ -35,7 +39,10 @@ import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import com.google.inject.Provider; import java.io.IOException; import java.util.List; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -48,6 +55,7 @@ public class MergeValidators { private final DynamicSet mergeValidationListeners; private final ProjectConfigValidator.Factory projectConfigValidatorFactory; + private final AccountValidator.Factory accountValidatorFactory; public interface Factory { MergeValidators create(); @@ -56,9 +64,11 @@ public class MergeValidators { @Inject MergeValidators( DynamicSet mergeValidationListeners, - ProjectConfigValidator.Factory projectConfigValidatorFactory) { + ProjectConfigValidator.Factory projectConfigValidatorFactory, + AccountValidator.Factory accountValidatorFactory) { this.mergeValidationListeners = mergeValidationListeners; this.projectConfigValidatorFactory = projectConfigValidatorFactory; + this.accountValidatorFactory = accountValidatorFactory; } public void validatePreMerge( @@ -72,7 +82,8 @@ public class MergeValidators { List validators = ImmutableList.of( new PluginMergeValidationListener(mergeValidationListeners), - projectConfigValidatorFactory.create()); + projectConfigValidatorFactory.create(), + accountValidatorFactory.create()); for (MergeValidationListener validator : validators) { validator.onPreMerge(repo, commit, destProject, destBranch, patchSetId, caller); @@ -210,4 +221,59 @@ public class MergeValidators { } } } + + public static class AccountValidator implements MergeValidationListener { + public interface Factory { + AccountValidator create(); + } + + private final Provider dbProvider; + private final AllUsersName allUsersName; + private final ChangeData.Factory changeDataFactory; + + @Inject + public AccountValidator( + Provider dbProvider, + AllUsersName allUsersName, + ChangeData.Factory changeDataFactory) { + this.dbProvider = dbProvider; + this.allUsersName = allUsersName; + this.changeDataFactory = changeDataFactory; + } + + @Override + public void onPreMerge( + Repository repo, + CodeReviewCommit commit, + ProjectState destProject, + Branch.NameKey destBranch, + PatchSet.Id patchSetId, + IdentifiedUser caller) + throws MergeValidationException { + if (!allUsersName.equals(destProject.getProject().getNameKey()) + || Account.Id.fromRef(destBranch.get()) == null) { + return; + } + + if (commit.getParentCount() > 1) { + // for merge commits we cannot ensure that the 'account.config' file is not modified, since + // for merge commits file modifications that come in through the merge don't appear in the + // file list that is returned by ChangeData#currentFilePaths() + throw new MergeValidationException("cannot submit merge commit to user branch"); + } + + ChangeData cd = + changeDataFactory.create( + dbProvider.get(), destProject.getProject().getNameKey(), patchSetId.getParentKey()); + try { + if (cd.currentFilePaths().contains(AccountConfig.ACCOUNT_CONFIG)) { + throw new MergeValidationException( + String.format("update of %s not allowed", AccountConfig.ACCOUNT_CONFIG)); + } + } catch (OrmException e) { + log.error("Cannot validate account update", e); + throw new MergeValidationException("account validation unavailable"); + } + } + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java index 5c1a292cde..04be41e15c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java @@ -52,6 +52,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class AddMembers implements RestModifyView { @@ -115,7 +116,7 @@ public class AddMembers implements RestModifyView { @Override public List apply(GroupResource resource, Input input) throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, - IOException { + IOException, ConfigInvalidException { AccountGroup internalGroup = resource.toAccountGroup(); if (internalGroup == null) { throw new MethodNotAllowedException(); @@ -143,7 +144,8 @@ public class AddMembers implements RestModifyView { } Account findAccount(String nameOrEmailOrId) - throws AuthException, UnprocessableEntityException, OrmException, IOException { + throws AuthException, UnprocessableEntityException, OrmException, IOException, + ConfigInvalidException { try { return accounts.parse(nameOrEmailOrId).getAccount(); } catch (UnprocessableEntityException e) { @@ -235,7 +237,7 @@ public class AddMembers implements RestModifyView { @Override public AccountInfo apply(GroupResource resource, PutMember.Input input) throws AuthException, MethodNotAllowedException, ResourceNotFoundException, OrmException, - IOException { + IOException, ConfigInvalidException { AddMembers.Input in = new AddMembers.Input(); in._oneMember = id; try { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java index d692e59681..af92acf034 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java @@ -55,6 +55,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Locale; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; @@ -115,7 +116,7 @@ public class CreateGroup implements RestModifyView @Override public GroupInfo apply(TopLevelResource resource, GroupInput input) throws AuthException, BadRequestException, UnprocessableEntityException, - ResourceConflictException, OrmException, IOException { + ResourceConflictException, OrmException, IOException, ConfigInvalidException { if (input == null) { input = new GroupInput(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java index d9b1c3d277..6be46d6dca 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java @@ -38,6 +38,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class DeleteMembers implements RestModifyView { @@ -64,7 +65,7 @@ public class DeleteMembers implements RestModifyView { @Override public Response apply(GroupResource resource, Input input) throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, - IOException { + IOException, ConfigInvalidException { AccountGroup internalGroup = resource.toAccountGroup(); if (internalGroup == null) { throw new MethodNotAllowedException(); @@ -125,7 +126,7 @@ public class DeleteMembers implements RestModifyView { @Override public Response apply(MemberResource resource, Input input) throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, - IOException { + IOException, ConfigInvalidException { AddMembers.Input in = new AddMembers.Input(); in._oneMember = resource.getMember().getAccountId().toString(); return delete.get().apply(resource, in); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java index 8f4d65e0f5..dbc06769d8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java @@ -32,6 +32,8 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class MembersCollection @@ -63,7 +65,8 @@ public class MembersCollection @Override public MemberResource parse(GroupResource parent, IdString id) - throws MethodNotAllowedException, AuthException, ResourceNotFoundException, OrmException { + throws MethodNotAllowedException, AuthException, ResourceNotFoundException, OrmException, + IOException, ConfigInvalidException { if (parent.toAccountGroup() == null) { throw new MethodNotAllowedException(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index 59fdf45e97..161233edb4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -732,7 +732,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate label(String name) throws QueryParseException, OrmException { + public Predicate label(String name) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { Set accounts = null; AccountGroup.UUID group = null; @@ -846,7 +847,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate starredby(String who) throws QueryParseException, OrmException { + public Predicate starredby(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return starredby(parseAccount(who)); } @@ -863,7 +865,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate watchedby(String who) throws QueryParseException, OrmException { + public Predicate watchedby(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { Set m = parseAccount(who); List p = Lists.newArrayListWithCapacity(m.size()); @@ -887,7 +890,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate draftby(String who) throws QueryParseException, OrmException { + public Predicate draftby(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { Set m = parseAccount(who); List> p = Lists.newArrayListWithCapacity(m.size()); for (Account.Id id : m) { @@ -905,7 +909,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate visibleto(String who) throws QueryParseException, OrmException { + public Predicate visibleto(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { if (isSelf(who)) { return is_visible(); } @@ -942,12 +947,14 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate o(String who) throws QueryParseException, OrmException { + public Predicate o(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return owner(who); } @Operator - public Predicate owner(String who) throws QueryParseException, OrmException { + public Predicate owner(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return owner(parseAccount(who)); } @@ -960,7 +967,7 @@ public class ChangeQueryBuilder extends QueryBuilder { } private Predicate ownerDefaultField(String who) - throws QueryParseException, OrmException { + throws QueryParseException, OrmException, IOException, ConfigInvalidException { Set accounts = parseAccount(who); if (accounts.size() > MAX_ACCOUNTS_PER_DEFAULT_FIELD) { return Predicate.any(); @@ -969,7 +976,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate assignee(String who) throws QueryParseException, OrmException { + public Predicate assignee(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return assignee(parseAccount(who)); } @@ -991,22 +999,24 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate r(String who) throws QueryParseException, OrmException { + public Predicate r(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return reviewer(who); } @Operator - public Predicate reviewer(String who) throws QueryParseException, OrmException { + public Predicate reviewer(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return reviewer(who, false); } private Predicate reviewerDefaultField(String who) - throws QueryParseException, OrmException { + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return reviewer(who, true); } private Predicate reviewer(String who, boolean forDefaultField) - throws QueryParseException, OrmException { + throws QueryParseException, OrmException, IOException, ConfigInvalidException { Predicate byState = reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField); if (Objects.equals(byState, Predicate.any())) { @@ -1020,7 +1030,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate cc(String who) throws QueryParseException, OrmException { + public Predicate cc(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return reviewerByState(who, ReviewerStateInternal.CC, false); } @@ -1073,7 +1084,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate commentby(String who) throws QueryParseException, OrmException { + public Predicate commentby(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return commentby(parseAccount(who)); } @@ -1086,7 +1098,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate from(String who) throws QueryParseException, OrmException { + public Predicate from(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { Set ownerIds = parseAccount(who); return Predicate.or(owner(ownerIds), commentby(ownerIds)); } @@ -1110,7 +1123,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate reviewedby(String who) throws QueryParseException, OrmException { + public Predicate reviewedby(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { return IsReviewedPredicate.create(parseAccount(who)); } @@ -1192,7 +1206,7 @@ public class ChangeQueryBuilder extends QueryBuilder { if (!Objects.equals(p, Predicate.any())) { predicates.add(p); } - } catch (OrmException | QueryParseException e) { + } catch (OrmException | IOException | ConfigInvalidException | QueryParseException e) { // Skip. } try { @@ -1200,13 +1214,13 @@ public class ChangeQueryBuilder extends QueryBuilder { if (!Objects.equals(p, Predicate.any())) { predicates.add(p); } - } catch (OrmException | QueryParseException e) { + } catch (OrmException | IOException | ConfigInvalidException | QueryParseException e) { // Skip. } predicates.add(file(query)); try { predicates.add(label(query)); - } catch (OrmException | QueryParseException e) { + } catch (OrmException | IOException | ConfigInvalidException | QueryParseException e) { // Skip. } predicates.add(commit(query)); @@ -1245,7 +1259,8 @@ public class ChangeQueryBuilder extends QueryBuilder { return Predicate.and(predicates); } - private Set parseAccount(String who) throws QueryParseException, OrmException { + private Set parseAccount(String who) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { if (isSelf(who)) { return Collections.singleton(self()); } @@ -1291,7 +1306,7 @@ public class ChangeQueryBuilder extends QueryBuilder { public Predicate reviewerByState( String who, ReviewerStateInternal state, boolean forDefaultField) - throws QueryParseException, OrmException { + throws QueryParseException, OrmException, IOException, ConfigInvalidException { Predicate reviewerByEmailPredicate = null; if (args.index.getSchema().hasField(ChangeField.REVIEWER_BY_EMAIL)) { Address address = Address.tryParse(who); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java index a0c03b6f5b..3cfd91c7c6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java @@ -35,7 +35,7 @@ import java.util.concurrent.TimeUnit; /** A version of the database schema. */ public abstract class SchemaVersion { /** The current schema version. */ - public static final Class C = Schema_153.class; + public static final Class C = Schema_154.class; public static int getBinaryVersion() { return guessVersion(C); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_146.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_146.java index 4896f3a2ec..6ba1b65adc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_146.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_146.java @@ -20,6 +20,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -88,7 +89,15 @@ public class Schema_146 extends SchemaVersion { rewriteUserBranch(repo, rw, oi, emptyTree, ref, e.getValue()); } else { AccountsUpdate.createUserBranch( - repo, oi, serverIdent, serverIdent, e.getKey(), e.getValue()); + repo, + allUsersName, + GitReferenceUpdated.DISABLED, + null, + oi, + serverIdent, + serverIdent, + e.getKey(), + e.getValue()); } } } catch (IOException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_147.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_147.java index 48d1e7e10d..29ae7d5a19 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_147.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_147.java @@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -68,7 +69,8 @@ public class Schema_147 extends SchemaVersion { .collect(toSet()); accountIdsFromUserBranches.removeAll(accountIdsFromReviewDb); for (Account.Id accountId : accountIdsFromUserBranches) { - AccountsUpdate.deleteUserBranch(repo, serverIdent, accountId); + AccountsUpdate.deleteUserBranch( + repo, allUsersName, GitReferenceUpdated.DISABLED, null, serverIdent, accountId); } } catch (IOException e) { throw new OrmException("Failed to delete user branches for non-existing accounts.", e); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_154.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_154.java new file mode 100644 index 0000000000..3d686bb4ac --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_154.java @@ -0,0 +1,105 @@ +// Copyright (C) 2017 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.schema; + +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.account.AccountConfig; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import java.io.IOException; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.HashSet; +import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Repository; + +/** Migrate accounts to NoteDb. */ +public class Schema_154 extends SchemaVersion { + private final GitRepositoryManager repoManager; + private final AllUsersName allUsersName; + private final Provider serverIdent; + + @Inject + Schema_154( + Provider prior, + GitRepositoryManager repoManager, + AllUsersName allUsersName, + @GerritPersonIdent Provider serverIdent) { + super(prior); + this.repoManager = repoManager; + this.allUsersName = allUsersName; + this.serverIdent = serverIdent; + } + + @Override + protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException, SQLException { + try { + try (Repository repo = repoManager.openRepository(allUsersName)) { + for (Account account : scanAccounts(db)) { + updateAccountInNoteDb(repo, account); + } + } + } catch (IOException | ConfigInvalidException e) { + throw new OrmException("Migrating accounts to NoteDb failed", e); + } + } + + private Set scanAccounts(ReviewDb db) throws SQLException { + try (Statement stmt = newStatement(db); + ResultSet rs = + stmt.executeQuery( + "SELECT account_id," + + " registered_on," + + " full_name, " + + " preferred_email," + + " status," + + " inactive" + + " FROM accounts")) { + Set s = new HashSet<>(); + while (rs.next()) { + Account a = new Account(new Account.Id(rs.getInt(1)), rs.getTimestamp(2)); + a.setFullName(rs.getString(3)); + a.setPreferredEmail(rs.getString(4)); + a.setStatus(rs.getString(5)); + a.setActive(rs.getString(6).equals("N")); + s.add(a); + } + return s; + } + } + + private void updateAccountInNoteDb(Repository allUsersRepo, Account account) + throws IOException, ConfigInvalidException { + MetaDataUpdate md = + new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, allUsersRepo); + PersonIdent ident = serverIdent.get(); + md.getCommitBuilder().setAuthor(ident); + md.getCommitBuilder().setCommitter(ident); + AccountConfig accountConfig = new AccountConfig(null, account.getId()); + accountConfig.load(allUsersRepo); + accountConfig.setAccount(account); + accountConfig.commit(md); + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index 042865ec4c..83230510ec 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java @@ -33,14 +33,20 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; +import com.google.gerrit.server.account.AccountConfig; import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.config.AllProjectsName; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.schema.SchemaCreator; import com.google.gerrit.server.util.ManualRequestContext; import com.google.gerrit.server.util.OneOffRequestContext; @@ -58,6 +64,8 @@ import java.util.Arrays; import java.util.Iterator; import java.util.List; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Repository; import org.junit.After; import org.junit.Before; import org.junit.Ignore; @@ -82,6 +90,8 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { @Inject protected GerritApi gApi; + @Inject @GerritPersonIdent Provider serverIdent; + @Inject protected IdentifiedUser.GenericFactory userFactory; @Inject private Provider anonymousUser; @@ -98,6 +108,10 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { @Inject protected AllProjectsName allProjects; + @Inject protected AllUsersName allUsers; + + @Inject protected GitRepositoryManager repoManager; + protected LifecycleManager lifecycle; protected Injector injector; protected ReviewDb db; @@ -383,12 +397,25 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { public void reindex() throws Exception { AccountInfo user1 = newAccountWithFullName("tester", "Test Usre"); - // update account in the database so that account index is stale + // update account in ReviewDb without reindex so that account index is stale String newName = "Test User"; - Account account = accounts.get(db, new Account.Id(user1._accountId)); + Account.Id accountId = new Account.Id(user1._accountId); + Account account = accounts.get(db, accountId); account.setFullName(newName); db.accounts().update(ImmutableSet.of(account)); + // update account in NoteDb without reindex so that account index is stale + try (Repository repo = repoManager.openRepository(allUsers)) { + MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsers, repo); + PersonIdent ident = serverIdent.get(); + md.getCommitBuilder().setAuthor(ident); + md.getCommitBuilder().setCommitter(ident); + AccountConfig accountConfig = new AccountConfig(null, accountId); + accountConfig.load(repo); + accountConfig.getAccount().setFullName(newName); + accountConfig.commit(md); + } + assertQuery("name:" + quote(user1.name), user1); assertQuery("name:" + quote(newName)); @@ -469,11 +496,16 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { if (email != null) { accountManager.link(id, AuthRequest.forEmail(email)); } - Account a = accounts.get(db, id); - a.setFullName(fullName); - a.setPreferredEmail(email); - a.setActive(active); - accountsUpdate.create().update(db, a); + accountsUpdate + .create() + .update( + db, + id, + a -> { + a.setFullName(fullName); + a.setPreferredEmail(email); + a.setActive(active); + }); return id; } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index e8a3c03195..2b8536b276 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -208,13 +208,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { db = schemaFactory.open(); userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId(); - Account userAccount = accounts.get(db, userId); String email = "user@example.com"; externalIdsUpdate.create().insert(ExternalId.createEmail(userId, email)); - userAccount.setPreferredEmail(email); - accountsUpdate.create().update(db, userAccount); + accountsUpdate.create().update(db, userId, a -> a.setPreferredEmail(email)); user = userFactory.create(userId); - requestContext.setContext(newRequestContext(userAccount.getId())); + requestContext.setContext(newRequestContext(userId)); } protected RequestContext newRequestContext(Account.Id requestUserId) { @@ -2340,11 +2338,16 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { if (email != null) { accountManager.link(id, AuthRequest.forEmail(email)); } - Account a = accounts.get(db, id); - a.setFullName(fullName); - a.setPreferredEmail(email); - a.setActive(active); - accountsUpdate.create().update(db, a); + accountsUpdate + .create() + .update( + db, + id, + a -> { + a.setFullName(fullName); + a.setPreferredEmail(email); + a.setActive(active); + }); return id; } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index fe2a8f3e92..0de5fad74d 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java @@ -319,11 +319,16 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests { if (email != null) { accountManager.link(id, AuthRequest.forEmail(email)); } - Account a = accounts.get(db, id); - a.setFullName(fullName); - a.setPreferredEmail(email); - a.setActive(active); - accountsUpdate.create().update(db, a); + accountsUpdate + .create() + .update( + db, + id, + a -> { + a.setFullName(fullName); + a.setPreferredEmail(email); + a.setActive(active); + }); return id; } } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CreateGroupCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CreateGroupCommand.java index 4bb176c0a5..a742c35698 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CreateGroupCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CreateGroupCommand.java @@ -37,6 +37,7 @@ import com.google.inject.Inject; import java.io.IOException; import java.util.HashSet; import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; @@ -103,7 +104,7 @@ final class CreateGroupCommand extends SshCommand { @Inject private AddIncludedGroups addIncludedGroups; @Override - protected void run() throws Failure, OrmException, IOException { + protected void run() throws Failure, OrmException, IOException, ConfigInvalidException { try { GroupResource rsrc = createGroup(); @@ -119,7 +120,8 @@ final class CreateGroupCommand extends SshCommand { } } - private GroupResource createGroup() throws RestApiException, OrmException, IOException { + private GroupResource createGroup() + throws RestApiException, OrmException, IOException, ConfigInvalidException { GroupInput input = new GroupInput(); input.description = groupDescription; input.visibleToAll = visibleToAll; @@ -132,7 +134,8 @@ final class CreateGroupCommand extends SshCommand { return groups.parse(TopLevelResource.INSTANCE, IdString.fromUrl(group.id)); } - private void addMembers(GroupResource rsrc) throws RestApiException, OrmException, IOException { + private void addMembers(GroupResource rsrc) + throws RestApiException, OrmException, IOException, ConfigInvalidException { AddMembers.Input input = AddMembers.Input.fromMembers( initialMembers.stream().map(Object::toString).collect(toList())); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/LsUserRefs.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/LsUserRefs.java index 4d93fe8976..bbe736deab 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/LsUserRefs.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/LsUserRefs.java @@ -35,6 +35,7 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import java.io.IOException; import java.util.Map; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; @@ -79,7 +80,7 @@ public class LsUserRefs extends SshCommand { Account userAccount; try { userAccount = accountResolver.find(db, userName); - } catch (OrmException e) { + } catch (OrmException | IOException | ConfigInvalidException e) { throw die(e); } if (userAccount == null) { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetAccountCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetAccountCommand.java index 19231698ba..033b4c6589 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetAccountCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetAccountCommand.java @@ -292,7 +292,8 @@ final class SetAccountCommand extends SshCommand { } private void putPreferred(String email) - throws RestApiException, OrmException, IOException, PermissionBackendException { + throws RestApiException, OrmException, IOException, PermissionBackendException, + ConfigInvalidException { for (EmailInfo e : getEmails.apply(rsrc)) { if (e.email.equals(email)) { putPreferred.apply(new AccountResource.Email(user, email), null);