opendev/gerrit
Code Issues Proposed changes

Merge changes I2304bfca,I378cc296,Ie08e50ea,Ia32983e2,Id51a0504

Browse Source 
* changes:
  config-sso.txt: Update sections that talk about external IDs
  Document accounts in NoteDb
  Allow to push updates of account.config file
  Remove support for writing accounts from ReviewDb
  Remove support for reading accounts from ReviewDb
This commit is contained in:
Edwin Kempin
2017-08-29 16:16:01 +00:00
committed by Gerrit Code Review
parent 2e9bbcde2c c3090eef40
commit 886fbdcc81
33 changed files with 1081 additions and 359 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
View File

@@ -208,7 +208,7 @@ public class AccountCacheImpl implements AccountCache {
private Optional<AccountState> load(ReviewDb db, Account.Id who)
throws OrmException, IOException, ConfigInvalidException {
Account account = accounts.get(db, who);
Account account = accounts.get(who);
if (account == null) {
return Optional.empty();
}

125
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
View File

@@ -144,7 +144,7 @@ public class AccountManager {
}
// return the identity to the caller.
update(db, who, id);
update(who, id);
return new AuthResult(id.accountId(), who.getExternalIdKey(), false);
}
} catch (OrmException | ConfigInvalidException e) {
@@ -152,7 +152,7 @@ public class AccountManager {
}
}
private void update(ReviewDb db, AuthRequest who, ExternalId extId)
private void update(AuthRequest who, ExternalId extId)
throws OrmException, IOException, ConfigInvalidException {
IdentifiedUser user = userFactory.create(extId.accountId());
List<Consumer<Account>> accountUpdates = new ArrayList<>();
@@ -188,8 +188,7 @@ public class AccountManager {
}
if (!accountUpdates.isEmpty()) {
Account account =
accountsUpdateFactory.create().update(db, user.getAccountId(), accountUpdates);
Account account = accountsUpdateFactory.create().update(user.getAccountId(), accountUpdates);
if (account == null) {
throw new OrmException("Account " + user.getAccountId() + " has been deleted");
}
@@ -214,7 +213,6 @@ public class AccountManager {
AccountsUpdate accountsUpdate = accountsUpdateFactory.create();
account =
accountsUpdate.insert(
db,
newId,
a -> {
a.setFullName(who.getDisplayName());
@@ -224,7 +222,7 @@ public class AccountManager {
ExternalId existingExtId = externalIds.get(extId.key());
if (existingExtId != null && !existingExtId.accountId().equals(extId.accountId())) {
// external ID is assigned to another account, do not overwrite
accountsUpdate.delete(db, account);
accountsUpdate.delete(account);
throw new AccountException(
"Cannot assign external ID \""
+ extId.key().get()
@@ -277,7 +275,7 @@ public class AccountManager {
+ "\" to account "
+ newId
+ "; name already in use.";
handleSettingUserNameFailure(db, account, extId, message, e, false);
handleSettingUserNameFailure(account, extId, message, e, false);
} catch (InvalidUserNameException e) {
String message =
"Cannot assign user name \""
@@ -285,10 +283,10 @@ public class AccountManager {
+ "\" to account "
+ newId
+ "; name does not conform.";
handleSettingUserNameFailure(db, account, extId, message, e, false);
handleSettingUserNameFailure(account, extId, message, e, false);
} catch (OrmException e) {
String message = "Cannot assign user name";
handleSettingUserNameFailure(db, account, extId, message, e, true);
handleSettingUserNameFailure(account, extId, message, e, true);
}
}
@@ -302,7 +300,6 @@ public class AccountManager {
* deletes the newly created account and throws an {@link AccountUserNameException}. In any case
* the error message is logged.
*
* @param db the database
* @param account the newly created account
* @param extId the newly created external id
* @param errorMessage the error message
@@ -313,12 +310,7 @@ public class AccountManager {
* @throws OrmException thrown if cleaning the database failed
*/
private void handleSettingUserNameFailure(
ReviewDb db,
Account account,
ExternalId extId,
String errorMessage,
Exception e,
boolean logException)
Account account, ExternalId extId, String errorMessage, Exception e, boolean logException)
throws AccountUserNameException, OrmException, IOException, ConfigInvalidException {
if (logException) {
log.error(errorMessage, e);
@@ -333,7 +325,7 @@ public class AccountManager {
// such an account cannot be used for uploading changes,
// this is why the best we can do here is to fail early and cleanup
// the database
accountsUpdateFactory.create().delete(db, account);
accountsUpdateFactory.create().delete(account);
externalIdsUpdateFactory.create().delete(extId);
throw new AccountUserNameException(errorMessage, e);
}
@@ -350,34 +342,31 @@ public class AccountManager {
*/
public AuthResult link(Account.Id to, AuthRequest who)
throws AccountException, OrmException, IOException, ConfigInvalidException {
try (ReviewDb db = schema.open()) {
ExternalId extId = externalIds.get(who.getExternalIdKey());
if (extId != null) {
if (!extId.accountId().equals(to)) {
throw new AccountException("Identity in use by another account");
}
update(db, who, extId);
} else {
externalIdsUpdateFactory
.create()
.insert(ExternalId.createWithEmail(who.getExternalIdKey(), to, who.getEmailAddress()));
if (who.getEmailAddress() != null) {
accountsUpdateFactory
.create()
.update(
db,
to,
a -> {
if (a.getPreferredEmail() == null) {
a.setPreferredEmail(who.getEmailAddress());
}
});
}
ExternalId extId = externalIds.get(who.getExternalIdKey());
if (extId != null) {
if (!extId.accountId().equals(to)) {
throw new AccountException("Identity in use by another account");
}
update(who, extId);
} else {
externalIdsUpdateFactory
.create()
.insert(ExternalId.createWithEmail(who.getExternalIdKey(), to, who.getEmailAddress()));
return new AuthResult(to, who.getExternalIdKey(), false);
if (who.getEmailAddress() != null) {
accountsUpdateFactory
.create()
.update(
to,
a -> {
if (a.getPreferredEmail() == null) {
a.setPreferredEmail(who.getEmailAddress());
}
});
}
}
return new AuthResult(to, who.getExternalIdKey(), false);
}
/**
@@ -437,40 +426,36 @@ public class AccountManager {
return;
}
try (ReviewDb db = schema.open()) {
List<ExternalId> extIds = new ArrayList<>(extIdKeys.size());
for (ExternalId.Key extIdKey : extIdKeys) {
ExternalId extId = externalIds.get(extIdKey);
if (extId != null) {
if (!extId.accountId().equals(from)) {
throw new AccountException(
"Identity '" + extIdKey.get() + "' in use by another account");
}
extIds.add(extId);
} else {
throw new AccountException("Identity '" + extIdKey.get() + "' not found");
List<ExternalId> extIds = new ArrayList<>(extIdKeys.size());
for (ExternalId.Key extIdKey : extIdKeys) {
ExternalId extId = externalIds.get(extIdKey);
if (extId != null) {
if (!extId.accountId().equals(from)) {
throw new AccountException("Identity '" + extIdKey.get() + "' in use by another account");
}
extIds.add(extId);
} else {
throw new AccountException("Identity '" + extIdKey.get() + "' not found");
}
}
externalIdsUpdateFactory.create().delete(extIds);
externalIdsUpdateFactory.create().delete(extIds);
if (extIds.stream().anyMatch(e -> e.email() != null)) {
accountsUpdateFactory
.create()
.update(
db,
from,
a -> {
if (a.getPreferredEmail() != null) {
for (ExternalId extId : extIds) {
if (a.getPreferredEmail().equals(extId.email())) {
a.setPreferredEmail(null);
break;
}
if (extIds.stream().anyMatch(e -> e.email() != null)) {
accountsUpdateFactory
.create()
.update(
from,
a -> {
if (a.getPreferredEmail() != null) {
for (ExternalId extId : extIds) {
if (a.getPreferredEmail().equals(extId.email())) {
a.setPreferredEmail(null);
break;
}
}
});
}
}
});
}
}
}

9
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
View File

@@ -98,7 +98,7 @@ public class AccountResolver {
Matcher m = Pattern.compile("^.* \\(([1-9][0-9]*)\\)$").matcher(nameOrEmail);
if (m.matches()) {
Account.Id id = Account.Id.parse(m.group(1));
if (exists(db, id)) {
if (accounts.get(id) != null) {
return Collections.singleton(id);
}
return Collections.emptySet();
@@ -106,7 +106,7 @@ public class AccountResolver {
if (nameOrEmail.matches("^[1-9][0-9]*$")) {
Account.Id id = Account.Id.parse(nameOrEmail);
if (exists(db, id)) {
if (accounts.get(id) != null) {
return Collections.singleton(id);
}
return Collections.emptySet();
@@ -122,11 +122,6 @@ public class AccountResolver {
return findAllByNameOrEmail(db, nameOrEmail);
}
private boolean exists(ReviewDb db, Account.Id id)
throws OrmException, IOException, ConfigInvalidException {
return accounts.get(db, id) != null;
}
/**
* Locate exactly one account matching the name or name/email string.
*

60
gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java
View File

@@ -20,12 +20,9 @@ import static java.util.stream.Collectors.toSet;
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;
@@ -36,7 +33,6 @@ 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;
@@ -46,47 +42,35 @@ import org.slf4j.LoggerFactory;
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(
@GerritServerConfig Config cfg,
GitRepositoryManager repoManager,
AllUsersName allUsersName,
OutgoingEmailValidator emailValidator) {
this.readFromGit = cfg.getBoolean("user", null, "readAccountsFromGit", true);
this.repoManager = repoManager;
this.allUsersName = allUsersName;
this.emailValidator = emailValidator;
}
public Account get(ReviewDb db, Account.Id accountId)
throws OrmException, IOException, ConfigInvalidException {
if (readFromGit) {
try (Repository repo = repoManager.openRepository(allUsersName)) {
return read(repo, accountId);
}
public Account get(Account.Id accountId) throws IOException, ConfigInvalidException {
try (Repository repo = repoManager.openRepository(allUsersName)) {
return read(repo, accountId);
}
return db.accounts().get(accountId);
}
public List<Account> get(ReviewDb db, Collection<Account.Id> accountIds)
throws OrmException, IOException, ConfigInvalidException {
if (readFromGit) {
List<Account> accounts = new ArrayList<>(accountIds.size());
try (Repository repo = repoManager.openRepository(allUsersName)) {
for (Account.Id accountId : accountIds) {
accounts.add(read(repo, accountId));
}
public List<Account> get(Collection<Account.Id> accountIds)
throws IOException, ConfigInvalidException {
List<Account> 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();
return accounts;
}
/**
@@ -94,23 +78,19 @@ public class Accounts {
*
* @return all accounts
*/
public List<Account> all(ReviewDb db) throws OrmException, IOException {
if (readFromGit) {
Set<Account.Id> accountIds = allIds();
List<Account> 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);
}
public List<Account> all() throws IOException {
Set<Account.Id> accountIds = allIds();
List<Account> 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();
return accounts;
}
/**

12
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java
View File

@@ -16,11 +16,8 @@ package com.google.gerrit.server.account;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.externalids.ExternalIds;
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;
@@ -28,22 +25,19 @@ import java.util.List;
@Singleton
public class AccountsConsistencyChecker {
private final Provider<ReviewDb> dbProvider;
private final Accounts accounts;
private final ExternalIds externalIds;
@Inject
AccountsConsistencyChecker(
Provider<ReviewDb> dbProvider, Accounts accounts, ExternalIds externalIds) {
this.dbProvider = dbProvider;
AccountsConsistencyChecker(Accounts accounts, ExternalIds externalIds) {
this.accounts = accounts;
this.externalIds = externalIds;
}
public List<ConsistencyProblemInfo> check() throws OrmException, IOException {
public List<ConsistencyProblemInfo> check() throws IOException {
List<ConsistencyProblemInfo> problems = new ArrayList<>();
for (Account account : accounts.all(dbProvider.get())) {
for (Account account : accounts.all()) {
if (account.getPreferredEmail() != null) {
if (!externalIds
.byAccount(account.getId())

81
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -17,12 +17,10 @@ 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;
@@ -32,7 +30,6 @@ 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;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -50,7 +47,7 @@ import org.eclipse.jgit.lib.Repository;
/**
* Updates accounts.
*
* <p>The account updates are written to both ReviewDb and NoteDb.
* <p>The account updates are written to NoteDb.
*
* <p>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
@@ -189,24 +186,19 @@ public class AccountsUpdate {
/**
* 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 Account insert(ReviewDb db, Account.Id accountId, Consumer<Account> init)
throws OrmException, IOException, ConfigInvalidException {
public Account insert(Account.Id accountId, Consumer<Account> init)
throws OrmDuplicateKeyException, IOException, ConfigInvalidException {
AccountConfig accountConfig = read(accountId);
Account account = accountConfig.getNewAccount();
init.accept(account);
// Create in ReviewDb
db.accounts().insert(ImmutableSet.of(account));
// Create in NoteDb
commitNew(accountConfig);
return account;
@@ -217,17 +209,15 @@ public class AccountsUpdate {
*
* <p>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 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, Consumer<Account> consumer)
throws OrmException, IOException, ConfigInvalidException {
return update(db, accountId, ImmutableList.of(consumer));
public Account update(Account.Id accountId, Consumer<Account> consumer)
throws IOException, ConfigInvalidException {
return update(accountId, ImmutableList.of(consumer));
}
/**
@@ -235,44 +225,26 @@ public class AccountsUpdate {
*
* <p>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<Consumer<Account>> consumers)
throws OrmException, IOException, ConfigInvalidException {
public Account update(Account.Id accountId, List<Consumer<Account>> consumers)
throws IOException, ConfigInvalidException {
if (consumers.isEmpty()) {
return null;
}
// Update in ReviewDb
Account reviewDbAccount =
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();
if (account != null) {
consumers.stream().forEach(c -> c.accept(account));
commit(accountConfig);
return account;
} else if (reviewDbAccount != null) {
// user branch doesn't exist yet
accountConfig.setAccount(reviewDbAccount);
commitNew(accountConfig);
}
return reviewDbAccount;
return account;
}
/**
@@ -281,25 +253,18 @@ public class AccountsUpdate {
* <p>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.
* a stale account index. This is why using {@link
* #update(com.google.gerrit.reviewdb.client.Account.Id, Consumer)} to do an atomic update is
* always preferred.
*
* <p>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)
* @see #update(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
public void replace(Account account) throws IOException, ConfigInvalidException {
AccountConfig accountConfig = read(account.getId());
accountConfig.setAccount(account);
commit(accountConfig);
@@ -308,32 +273,20 @@ public class AccountsUpdate {
/**
* 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());
public void delete(Account account) throws IOException {
deleteByKey(account.getId());
}
/**
* 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
public void deleteByKey(Account.Id accountId) throws IOException {
deleteUserBranch(accountId);
}

1
gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
View File

@@ -174,7 +174,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
accountsUpdate
.create()
.insert(
db,
id,
a -> {
a.setFullName(input.name);

10
gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java
View File

@@ -23,7 +23,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Account;
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.PutName.Input;
@@ -46,7 +45,6 @@ public class PutName implements RestModifyView<AccountResource, Input> {
private final Provider<CurrentUser> self;
private final Realm realm;
private final PermissionBackend permissionBackend;
private final Provider<ReviewDb> dbProvider;
private final AccountsUpdate.Server accountsUpdate;
@Inject
@@ -54,12 +52,10 @@ public class PutName implements RestModifyView<AccountResource, Input> {
Provider<CurrentUser> self,
Realm realm,
PermissionBackend permissionBackend,
Provider<ReviewDb> dbProvider,
AccountsUpdate.Server accountsUpdate) {
this.self = self;
this.realm = realm;
this.permissionBackend = permissionBackend;
this.dbProvider = dbProvider;
this.accountsUpdate = accountsUpdate;
}
@@ -74,7 +70,7 @@ public class PutName implements RestModifyView<AccountResource, Input> {
}
public Response<String> apply(IdentifiedUser user, Input input)
throws MethodNotAllowedException, ResourceNotFoundException, OrmException, IOException,
throws MethodNotAllowedException, ResourceNotFoundException, IOException,
ConfigInvalidException {
if (input == null) {
input = new Input();
@@ -86,9 +82,7 @@ public class PutName implements RestModifyView<AccountResource, Input> {
String newName = input.name;
Account account =
accountsUpdate
.create()
.update(dbProvider.get(), user.getAccountId(), a -> a.setFullName(newName));
accountsUpdate.create().update(user.getAccountId(), a -> a.setFullName(newName));
if (account == null) {
throw new ResourceNotFoundException("account not found");
}

7
gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java
View File

@@ -19,7 +19,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Account;
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.PutPreferred.Input;
@@ -39,18 +38,15 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
static class Input {}
private final Provider<CurrentUser> self;
private final Provider<ReviewDb> dbProvider;
private final PermissionBackend permissionBackend;
private final AccountsUpdate.Server accountsUpdate;
@Inject
PutPreferred(
Provider<CurrentUser> self,
Provider<ReviewDb> dbProvider,
PermissionBackend permissionBackend,
AccountsUpdate.Server accountsUpdate) {
this.self = self;
this.dbProvider = dbProvider;
this.permissionBackend = permissionBackend;
this.accountsUpdate = accountsUpdate;
}
@@ -66,13 +62,12 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
}
public Response<String> apply(IdentifiedUser user, String email)
throws ResourceNotFoundException, OrmException, IOException, ConfigInvalidException {
throws ResourceNotFoundException, IOException, ConfigInvalidException {
AtomicBoolean alreadyPreferred = new AtomicBoolean(false);
Account account =
accountsUpdate
.create()
.update(
dbProvider.get(),
user.getAccountId(),
a -> {
if (email.equals(a.getPreferredEmail())) {

11
gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java
View File

@@ -21,7 +21,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Account;
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.PutStatus.Input;
@@ -48,18 +47,15 @@ public class PutStatus implements RestModifyView<AccountResource, Input> {
}
private final Provider<CurrentUser> self;
private final Provider<ReviewDb> dbProvider;
private final PermissionBackend permissionBackend;
private final AccountsUpdate.Server accountsUpdate;
@Inject
PutStatus(
Provider<CurrentUser> self,
Provider<ReviewDb> dbProvider,
PermissionBackend permissionBackend,
AccountsUpdate.Server accountsUpdate) {
this.self = self;
this.dbProvider = dbProvider;
this.permissionBackend = permissionBackend;
this.accountsUpdate = accountsUpdate;
}
@@ -75,7 +71,7 @@ public class PutStatus implements RestModifyView<AccountResource, Input> {
}
public Response<String> apply(IdentifiedUser user, Input input)
throws ResourceNotFoundException, OrmException, IOException, ConfigInvalidException {
throws ResourceNotFoundException, IOException, ConfigInvalidException {
if (input == null) {
input = new Input();
}
@@ -84,10 +80,7 @@ public class PutStatus implements RestModifyView<AccountResource, Input> {
Account account =
accountsUpdate
.create()
.update(
dbProvider.get(),
user.getAccountId(),
a -> a.setStatus(Strings.nullToEmpty(newStatus)));
.update(user.getAccountId(), a -> a.setStatus(Strings.nullToEmpty(newStatus)));
if (account == null) {
throw new ResourceNotFoundException("account not found");
}

13
gerrit-server/src/main/java/com/google/gerrit/server/account/SetInactiveFlag.java
View File

@@ -19,11 +19,8 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
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.concurrent.atomic.AtomicBoolean;
@@ -32,23 +29,20 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
@Singleton
public class SetInactiveFlag {
private final Provider<ReviewDb> dbProvider;
private final AccountsUpdate.Server accountsUpdate;
@Inject
SetInactiveFlag(Provider<ReviewDb> dbProvider, AccountsUpdate.Server accountsUpdate) {
this.dbProvider = dbProvider;
SetInactiveFlag(AccountsUpdate.Server accountsUpdate) {
this.accountsUpdate = accountsUpdate;
}
public Response<?> deactivate(IdentifiedUser user)
throws RestApiException, OrmException, IOException, ConfigInvalidException {
throws RestApiException, IOException, ConfigInvalidException {
AtomicBoolean alreadyInactive = new AtomicBoolean(false);
Account account =
accountsUpdate
.create()
.update(
dbProvider.get(),
user.getAccountId(),
a -> {
if (!a.isActive()) {
@@ -67,13 +61,12 @@ public class SetInactiveFlag {
}
public Response<String> activate(IdentifiedUser user)
throws ResourceNotFoundException, OrmException, IOException, ConfigInvalidException {
throws ResourceNotFoundException, IOException, ConfigInvalidException {
AtomicBoolean alreadyActive = new AtomicBoolean(false);
Account account =
accountsUpdate
.create()
.update(
dbProvider.get(),
user.getAccountId(),
a -> {
if (a.isActive()) {

4
gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
View File

@@ -224,10 +224,10 @@ public class ConsistencyChecker {
private void checkOwner() {
try {
if (accounts.get(db.get(), change().getOwner()) == null) {
if (accounts.get(change().getOwner()) == null) {
problem("Missing change owner: " + change().getOwner());
}
} catch (OrmException | IOException | ConfigInvalidException e) {
} catch (IOException | ConfigInvalidException e) {
error("Failed to look up owner", e);
}
}

4
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@@ -127,7 +127,7 @@ import com.google.gerrit.server.git.strategy.SubmitStrategy;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.gerrit.server.git.validators.MergeValidators;
import com.google.gerrit.server.git.validators.MergeValidators.AccountValidator;
import com.google.gerrit.server.git.validators.MergeValidators.AccountMergeValidator;
import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
@@ -393,7 +393,7 @@ public class GerritGlobalModule extends FactoryModule {
bind(AnonymousUser.class);
factory(AbandonOp.Factory.class);
factory(AccountValidator.Factory.class);
factory(AccountMergeValidator.Factory.class);
factory(RefOperationValidators.Factory.class);
factory(OnSubmitValidators.Factory.class);
factory(MergeValidators.Factory.class);

3
gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -2726,7 +2726,6 @@ class ReceiveCommits {
accountsUpdate
.create()
.update(
db,
user.getAccountId(),
a -> {
if (Strings.isNullOrEmpty(a.getFullName())) {
@@ -2736,7 +2735,7 @@ class ReceiveCommits {
if (account != null && Strings.isNullOrEmpty(account.getFullName())) {
user.getAccount().setFullName(account.getFullName());
}
} catch (OrmException | IOException | ConfigInvalidException e) {
} catch (IOException | ConfigInvalidException e) {
logWarn("Cannot default full_name", e);
} finally {
defaultName = false;

91
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/AccountValidator.java Normal file
View File

@@ -0,0 +1,91 @@
// 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.git.validators;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountConfig;
import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
import com.google.inject.Provider;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import javax.inject.Inject;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevWalk;
public class AccountValidator {
private final Provider<IdentifiedUser> self;
private final OutgoingEmailValidator emailValidator;
@Inject
public AccountValidator(Provider<IdentifiedUser> self, OutgoingEmailValidator emailValidator) {
this.self = self;
this.emailValidator = emailValidator;
}
public List<String> validate(
Account.Id accountId, RevWalk rw, @Nullable ObjectId oldId, ObjectId newId)
throws IOException {
Account oldAccount = null;
if (oldId != null && !ObjectId.zeroId().equals(oldId)) {
try {
oldAccount = loadAccount(accountId, rw, oldId);
} catch (ConfigInvalidException e) {
// ignore, maybe the new commit is repairing it now
}
}
Account newAccount;
try {
newAccount = loadAccount(accountId, rw, newId);
} catch (ConfigInvalidException e) {
return ImmutableList.of(
String.format(
"commit '%s' has an invalid '%s' file for account '%s': %s",
newId.name(), AccountConfig.ACCOUNT_CONFIG, accountId.get(), e.getMessage()));
}
List<String> messages = new ArrayList<>();
if (accountId.equals(self.get().getAccountId()) && !newAccount.isActive()) {
messages.add("cannot deactivate own account");
}
if (newAccount.getPreferredEmail() != null
&& (oldAccount == null
|| !newAccount.getPreferredEmail().equals(oldAccount.getPreferredEmail()))) {
if (!emailValidator.isValid(newAccount.getPreferredEmail())) {
messages.add(
String.format(
"invalid preferred email '%s' for account '%s'",
newAccount.getPreferredEmail(), accountId.get()));
}
}
return ImmutableList.copyOf(messages);
}
private Account loadAccount(Account.Id accountId, RevWalk rw, ObjectId commit)
throws IOException, ConfigInvalidException {
rw.reset();
AccountConfig accountConfig = new AccountConfig(null, accountId);
accountConfig.load(rw, commit);
return accountConfig.getAccount();
}
}

49
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@@ -32,7 +32,6 @@ import com.google.gerrit.reviewdb.client.Branch;
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;
@@ -58,11 +57,9 @@ 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;
@@ -70,7 +67,6 @@ 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;
@@ -88,6 +84,7 @@ public class CommitValidators {
private final DynamicSet<CommitValidationListener> pluginValidators;
private final AllUsersName allUsers;
private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker;
private final AccountValidator accountValidator;
private final String installCommitMsgHookCommand;
private final ProjectCache projectCache;
@@ -99,12 +96,14 @@ public class CommitValidators {
DynamicSet<CommitValidationListener> pluginValidators,
AllUsersName allUsers,
ExternalIdsConsistencyChecker externalIdsConsistencyChecker,
AccountValidator accountValidator,
ProjectCache projectCache) {
this.gerritIdent = gerritIdent;
this.canonicalWebUrl = canonicalWebUrl;
this.pluginValidators = pluginValidators;
this.allUsers = allUsers;
this.externalIdsConsistencyChecker = externalIdsConsistencyChecker;
this.accountValidator = accountValidator;
this.installCommitMsgHookCommand =
cfg != null ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null;
this.projectCache = projectCache;
@@ -133,7 +132,7 @@ public class CommitValidators {
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountValidator(allUsers)));
new AccountCommitValidator(allUsers, accountValidator)));
}
public CommitValidators forGerritCommits(
@@ -158,7 +157,7 @@ public class CommitValidators {
new ConfigValidator(branch, user, rw, allUsers),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountValidator(allUsers)));
new AccountCommitValidator(allUsers, accountValidator)));
}
public CommitValidators forMergedCommits(PermissionBackend.ForRef perm, IdentifiedUser user) {
@@ -709,11 +708,13 @@ public class CommitValidators {
}
/** Rejects updates to 'account.config' in user branches. */
public static class AccountValidator implements CommitValidationListener {
public static class AccountCommitValidator implements CommitValidationListener {
private final AllUsersName allUsers;
private final AccountValidator accountValidator;
public AccountValidator(AllUsersName allUsers) {
public AccountCommitValidator(AllUsersName allUsers, AccountValidator accountValidator) {
this.allUsers = allUsers;
this.accountValidator = accountValidator;
}
@Override
@@ -725,7 +726,7 @@ public class CommitValidators {
if (receiveEvent.command.getRefName().startsWith(MagicBranch.NEW_CHANGE)) {
// no validation on push for review, will be checked on submit by
// MergeValidators.AccountValidator
// MergeValidators.AccountMergeValidator
return Collections.emptyList();
}
@@ -735,15 +736,19 @@ public class CommitValidators {
}
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");
List<String> errorMessages =
accountValidator.validate(
accountId,
receiveEvent.revWalk,
receiveEvent.command.getOldId(),
receiveEvent.commit);
if (!errorMessages.isEmpty()) {
throw new CommitValidationException(
"invalid account configuration",
errorMessages
.stream()
.map(m -> new CommitValidationMessage(m, true))
.collect(toList()));
}
} catch (IOException e) {
String m = String.format("Validating update for account %s failed", accountId.get());
@@ -752,14 +757,6 @@ public class CommitValidators {
}
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(

44
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.git.validators;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.api.projects.ProjectConfigEntryType;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -47,6 +48,7 @@ import java.io.IOException;
import java.util.List;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -55,7 +57,7 @@ public class MergeValidators {
private final DynamicSet<MergeValidationListener> mergeValidationListeners;
private final ProjectConfigValidator.Factory projectConfigValidatorFactory;
private final AccountValidator.Factory accountValidatorFactory;
private final AccountMergeValidator.Factory accountValidatorFactory;
public interface Factory {
MergeValidators create();
@@ -65,7 +67,7 @@ public class MergeValidators {
MergeValidators(
DynamicSet<MergeValidationListener> mergeValidationListeners,
ProjectConfigValidator.Factory projectConfigValidatorFactory,
AccountValidator.Factory accountValidatorFactory) {
AccountMergeValidator.Factory accountValidatorFactory) {
this.mergeValidationListeners = mergeValidationListeners;
this.projectConfigValidatorFactory = projectConfigValidatorFactory;
this.accountValidatorFactory = accountValidatorFactory;
@@ -222,23 +224,26 @@ public class MergeValidators {
}
}
public static class AccountValidator implements MergeValidationListener {
public static class AccountMergeValidator implements MergeValidationListener {
public interface Factory {
AccountValidator create();
AccountMergeValidator create();
}
private final Provider<ReviewDb> dbProvider;
private final AllUsersName allUsersName;
private final ChangeData.Factory changeDataFactory;
private final AccountValidator accountValidator;
@Inject
public AccountValidator(
public AccountMergeValidator(
Provider<ReviewDb> dbProvider,
AllUsersName allUsersName,
ChangeData.Factory changeDataFactory) {
ChangeData.Factory changeDataFactory,
AccountValidator accountValidator) {
this.dbProvider = dbProvider;
this.allUsersName = allUsersName;
this.changeDataFactory = changeDataFactory;
this.accountValidator = accountValidator;
}
@Override
@@ -250,30 +255,33 @@ public class MergeValidators {
PatchSet.Id patchSetId,
IdentifiedUser caller)
throws MergeValidationException {
if (!allUsersName.equals(destProject.getProject().getNameKey())
|| Account.Id.fromRef(destBranch.get()) == null) {
Account.Id accountId = Account.Id.fromRef(destBranch.get());
if (!allUsersName.equals(destProject.getProject().getNameKey()) || accountId == 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));
if (!cd.currentFilePaths().contains(AccountConfig.ACCOUNT_CONFIG)) {
return;
}
} catch (OrmException e) {
log.error("Cannot validate account update", e);
throw new MergeValidationException("account validation unavailable");
}
try (RevWalk rw = new RevWalk(repo)) {
List<String> errorMessages = accountValidator.validate(accountId, rw, null, commit);
if (!errorMessages.isEmpty()) {
throw new MergeValidationException(
"invalid account configuration: " + Joiner.on("; ").join(errorMessages));
}
} catch (IOException e) {
log.error("Cannot validate account update", e);
throw new MergeValidationException("account validation unavailable");
}
}
}
}

11
gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
View File

@@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat;
import static java.util.stream.Collectors.toList;
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.accounts.Accounts.QueryRequest;
@@ -397,14 +396,9 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
public void reindex() throws Exception {
AccountInfo user1 = newAccountWithFullName("tester", "Test Usre");
// update account in ReviewDb without reindex so that account index is stale
String newName = "Test User";
// update account without reindex so that account index is stale
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
String newName = "Test User";
try (Repository repo = repoManager.openRepository(allUsers)) {
MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsers, repo);
PersonIdent ident = serverIdent.get();
@@ -499,7 +493,6 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
accountsUpdate
.create()
.update(
db,
id,
a -> {
a.setFullName(fullName);

3
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -214,7 +214,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
String email = "user@example.com";
externalIdsUpdate.create().insert(ExternalId.createEmail(userId, email));
accountsUpdate.create().update(db, userId, a -> a.setPreferredEmail(email));
accountsUpdate.create().update(userId, a -> a.setPreferredEmail(email));
user = userFactory.create(userId);
requestContext.setContext(newRequestContext(userId));
}
@@ -2469,7 +2469,6 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
accountsUpdate
.create()
.update(
db,
id,
a -> {
a.setFullName(fullName);

1
gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
View File

@@ -323,7 +323,6 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
accountsUpdate
.create()
.update(
db,
id,
a -> {
a.setFullName(fullName);