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..8f74686a6a 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; @@ -106,6 +108,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 +117,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; @@ -220,6 +224,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(); @@ -1071,26 +1136,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) { 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/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/AccountsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java index 47524f258b..c1b3538cbf 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 @@ -18,14 +18,18 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.collect.ImmutableSet; 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.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; @@ -34,6 +38,7 @@ import com.google.inject.Singleton; import java.io.IOException; import java.sql.Timestamp; 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; @@ -47,8 +52,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 { @@ -61,56 +76,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)); } } @@ -123,29 +120,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) { @@ -154,22 +164,28 @@ 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"); } /** @@ -179,79 +195,116 @@ public class AccountsUpdate { * @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 Account insert(ReviewDb db, Account.Id accountId, Consumer init) - throws OrmException, IOException { - Account account = new Account(accountId, TimeUtil.nowTs()); + 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(accountId); + + // Create in NoteDb + commitNew(accountConfig); return account; } - /** Updates the account. */ - public void update(ReviewDb db, Account account) throws OrmException, IOException { + /** + * Updates the account. + * + *

Changing the registration date of an account is not supported. + * + * @param db ReviewDb + * @param account the 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 + */ + public void update(ReviewDb db, Account account) + throws OrmException, IOException, ConfigInvalidException { + // Update in ReviewDb db.accounts().update(ImmutableSet.of(account)); - evictAccount(account.getId()); + + // Update in NoteDb + AccountConfig accountConfig = read(account.getId()); + accountConfig.setAccount(account); + commit(accountConfig); } /** * 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); + throws OrmException, IOException, ConfigInvalidException { + // Update in ReviewDb + db.accounts() + .atomicUpdate( + accountId, + a -> { + consumer.accept(a); + return a; + }); + + // Update in NoteDb + AccountConfig accountConfig = read(accountId); + Account account = accountConfig.getAccount(); + consumer.accept(account); + commit(accountConfig); return account; } - /** Deletes the account. */ + /** + * 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)); - 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()); - } + // Delete in NoteDb + deleteUserBranch(accountId); } public static void createUserBranch( Repository repo, + Project.NameKey project, + GitReferenceUpdated gitRefUpdated, + @Nullable IdentifiedUser user, ObjectInserter oi, PersonIdent committerIdent, PersonIdent authorIdent, @@ -271,6 +324,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( @@ -295,12 +349,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) { @@ -317,11 +377,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/DeleteActive.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java index 3a996f22b6..bb1d6d8604 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"); } 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..f05cd1ce7e 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,7 +46,7 @@ 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 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..03697c8591 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(); } 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..88c2aecd78 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,7 +66,7 @@ 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 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..8339413798 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(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 8cac1a42a1..9e1374098d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -155,6 +155,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.lib.Constants; @@ -2789,7 +2790,7 @@ public class ReceiveCommits { accountsUpdate.create().update(db, a); user.getAccount().setFullName(a.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/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-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);