opendev/gerrit
Code Issues Proposed changes

Remove access of groups related db tables from AccountManager

Browse Source 
GroupsUpdate will be the common class to gather all write access on the
database tables related to groups, similarly as AccountsUpdate is for
accounts.

Change-Id: I01312c33c09a0c4dc92c749e201b59d832437a03
This commit is contained in:
Alice Kober-Sotzek
2017-07-25 16:15:13 +02:00
parent 1ff8a88201
commit 18ed1e07a5
5 changed files with 204 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -16,7 +16,6 @@ package com.google.gerrit.server.account;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.audit.AuditService;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.Permission;
@@ -24,7 +23,6 @@ import com.google.gerrit.common.errors.NameAlreadyUsedException;
import com.google.gerrit.extensions.client.AccountFieldName;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroupMember;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.Sequences;
@@ -32,6 +30,8 @@ import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.group.GroupsUpdate;
import com.google.gerrit.server.group.ServerInitiated;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.account.InternalAccountQuery;
import com.google.gwtorm.server.OrmException;
@@ -42,7 +42,6 @@ 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;
@@ -68,10 +67,10 @@ public class AccountManager {
private final ChangeUserName.Factory changeUserNameFactory;
private final ProjectCache projectCache;
private final AtomicBoolean awaitsFirstAccountCheck;
private final AuditService auditService;
private final Provider<InternalAccountQuery> accountQueryProvider;
private final ExternalIds externalIds;
private final ExternalIdsUpdate.Server externalIdsUpdateFactory;
private final Provider<GroupsUpdate> groupsUpdateProvider;
@Inject
AccountManager(
@@ -86,10 +85,10 @@ public class AccountManager {
IdentifiedUser.GenericFactory userFactory,
ChangeUserName.Factory changeUserNameFactory,
ProjectCache projectCache,
AuditService auditService,
Provider<InternalAccountQuery> accountQueryProvider,
ExternalIds externalIds,
ExternalIdsUpdate.Server externalIdsUpdateFactory) {
ExternalIdsUpdate.Server externalIdsUpdateFactory,
@ServerInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
this.schema = schema;
this.sequences = sequences;
this.accounts = accounts;
@@ -102,10 +101,10 @@ public class AccountManager {
this.projectCache = projectCache;
this.awaitsFirstAccountCheck =
new AtomicBoolean(cfg.getBoolean("capability", "makeFirstUserAdmin", true));
this.auditService = auditService;
this.accountQueryProvider = accountQueryProvider;
this.externalIds = externalIds;
this.externalIdsUpdateFactory = externalIdsUpdateFactory;
this.groupsUpdateProvider = groupsUpdateProvider;
}
/** @return user identified by this external identity string */
@@ -247,6 +246,8 @@ public class AccountManager {
awaitsFirstAccountCheck.set(isFirstAccount);
}
IdentifiedUser user = userFactory.create(newId);
if (isFirstAccount) {
// This is the first user account on our site. Assume this user
// is going to be the site's administrator and just make them that
@@ -260,17 +261,15 @@ public class AccountManager {
.getPermission(GlobalCapability.ADMINISTRATE_SERVER);
AccountGroup.UUID uuid = admin.getRules().get(0).getGroup().getUUID();
AccountGroup g = db.accountGroups().byUUID(uuid).iterator().next();
AccountGroup.Id adminId = g.getId();
AccountGroupMember m = new AccountGroupMember(new AccountGroupMember.Key(newId, adminId));
auditService.dispatchAddAccountsToGroup(newId, Collections.singleton(m));
db.accountGroupMembers().insert(Collections.singleton(m));
GroupsUpdate groupsUpdate = groupsUpdateProvider.get();
// The user initiated this request by logging in. -> Attribute all modifications to that user.
groupsUpdate.setCurrentUser(user);
groupsUpdate.addGroupMember(db, uuid, newId);
}
if (who.getUserName() != null) {
// Only set if the name hasn't been used yet, but was given to us.
//
IdentifiedUser user = userFactory.create(newId);
try {
changeUserNameFactory.create(user, who.getUserName()).call();
} catch (NameAlreadyUsedException e) {

108
gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java Normal file
View File

@@ -0,0 +1,108 @@
// 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.group;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.audit.AuditService;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroupMember;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
public class GroupsUpdate {
public interface Factory {
GroupsUpdate create(@Nullable IdentifiedUser currentUser);
}
private final Groups groups;
private final AuditService auditService;
private final AccountCache accountCache;
@Nullable private IdentifiedUser currentUser;
@Inject
GroupsUpdate(
Groups groups,
AuditService auditService,
AccountCache accountCache,
@Assisted @Nullable IdentifiedUser currentUser) {
this.groups = groups;
this.auditService = auditService;
this.accountCache = accountCache;
this.currentUser = currentUser;
}
/**
* Uses the identity of the specified user to mark database modifications executed by this {@code
* GroupsUpdate}. For NoteDb, this identity is used as author and committer for all related
* commits.
*
* <p><strong>Note</strong>: Please use this method with care and rather consider to use the
* correct annotation on the provider of this class instead.
*
* @param currentUser the user to which modifications should be attributed, or {@code null} if the
* Gerrit server identity should be used
*/
public void setCurrentUser(@Nullable IdentifiedUser currentUser) {
this.currentUser = currentUser;
}
public void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId)
throws OrmException, IOException {
Optional<AccountGroup> foundGroup = groups.get(db, groupUuid);
if (!foundGroup.isPresent()) {
// TODO(aliceks): Throw an exception?
return;
}
AccountGroup group = foundGroup.get();
addGroupMembers(db, group, ImmutableSet.of(accountId));
}
private void addGroupMembers(ReviewDb db, AccountGroup group, Set<Account.Id> accountIds)
throws OrmException, IOException {
AccountGroup.Id groupId = group.getId();
Set<AccountGroupMember> newMembers = new HashSet<>();
for (Account.Id accountId : accountIds) {
boolean isMember = groups.isMember(db, group, accountId);
if (!isMember) {
AccountGroupMember.Key key = new AccountGroupMember.Key(accountId, groupId);
newMembers.add(new AccountGroupMember(key));
}
}
if (newMembers.isEmpty()) {
return;
}
if (currentUser != null) {
auditService.dispatchAddAccountsToGroup(currentUser.getAccountId(), newMembers);
}
db.accountGroupMembers().insert(newMembers);
for (AccountGroupMember newMember : newMembers) {
accountCache.evict(newMember.getAccountId());
}
}
}

16
gerrit-server/src/main/java/com/google/gerrit/server/group/Module.java
View File

@@ -22,10 +22,12 @@ import com.google.gerrit.audit.GroupMemberAuditListener;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.RestApiModule;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.group.AddIncludedGroups.UpdateIncludedGroup;
import com.google.gerrit.server.group.AddMembers.UpdateMember;
import com.google.gerrit.server.group.DeleteIncludedGroups.DeleteIncludedGroup;
import com.google.gerrit.server.group.DeleteMembers.DeleteMember;
import com.google.inject.Provides;
public class Module extends RestApiModule {
@Override
@@ -68,7 +70,21 @@ public class Module extends RestApiModule {
delete(INCLUDED_GROUP_KIND).to(DeleteIncludedGroup.class);
factory(CreateGroup.Factory.class);
factory(GroupsUpdate.Factory.class);
DynamicSet.bind(binder(), GroupMemberAuditListener.class).to(DbGroupMemberAuditListener.class);
}
@Provides
@ServerInitiated
GroupsUpdate provideServerInitiatedGroupsUpdate(GroupsUpdate.Factory groupsUpdateFactory) {
return groupsUpdateFactory.create(null);
}
@Provides
@UserInitiated
GroupsUpdate provideUserInitiatedGroupsUpdate(
GroupsUpdate.Factory groupsUpdateFactory, IdentifiedUser currentUser) {
return groupsUpdateFactory.create(currentUser);
}
}

34
gerrit-server/src/main/java/com/google/gerrit/server/group/ServerInitiated.java Normal file
View File

@@ -0,0 +1,34 @@
// 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.group;
import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import com.google.inject.BindingAnnotation;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
/**
* A marker for database modifications which aren't directly related to a user request (e.g. happen
* outside of a request context). Those modifications will be attributed to the Gerrit server by
* using the Gerrit server identity as author and committer for all related NoteDb commits.
*/
@BindingAnnotation
@Target({FIELD, PARAMETER, METHOD})
@Retention(RUNTIME)
public @interface ServerInitiated {}

34
gerrit-server/src/main/java/com/google/gerrit/server/group/UserInitiated.java Normal file
View File

@@ -0,0 +1,34 @@
// 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.group;
import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import com.google.inject.BindingAnnotation;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
/**
* A marker for database modifications which are directly related to a user request (e.g. happen
* inside of a request context). Those modifications will be attributed to the user by using the
* user's identity as author and committer for all related NoteDb commits.
*/
@BindingAnnotation
@Target({FIELD, PARAMETER, METHOD})
@Retention(RUNTIME)
public @interface UserInitiated {}