Cleanup MemberResource and IncludedGroupResource It can be easier to describe the nested member resource as being a subclass of the parent. This allows views to act on the parent using the passed in resource, which is logical. Any changes made by the view are within the scope of the parent and not the scope of the other entity. This means the child resource should extend GroupResource. Most methods on the child refer to the group that is being accessed by the view. An additional field specifies a user or included group that is being accessed. GroupResources come with a GroupControl that was already used to verify the current user can view the group. In a view we can now rely on the incoming GroupControl, rather than making a new instance. This simplifies some view code. Nested child collections should be ensuring the user can see the member before creating the child resource. Verify this is true when parsing the incoming IdString by asking the control if the current user can see the member. Simplify the way group resources are downcast to the internal AccountGroup instance. The downcast is a critical part of each view and should be consistent. If a group is accessed that is not an internal group some views do not exist or cannot be used. Change-Id: I28628b2d04ae59a98b63d6143b620afd9765c122
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java index d1cc0e2..0c366a7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java
@@ -18,7 +18,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -32,7 +31,6 @@ import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuidAudit; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.BadRequestHandler; -import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.account.GroupIncludeCache; @@ -65,54 +63,47 @@ } } - private final GroupControl.Factory groupControlFactory; private final Provider<GroupsCollection> groupsCollection; private final GroupIncludeCache groupIncludeCache; private final ReviewDb db; - private final Provider<CurrentUser> self; @Inject - public AddIncludedGroups(final GroupControl.Factory groupControlFactory, - final Provider<GroupsCollection> groupsCollection, - final GroupIncludeCache groupIncludeCache, final ReviewDb db, - final Provider<CurrentUser> self) { - this.groupControlFactory = groupControlFactory; + public AddIncludedGroups(Provider<GroupsCollection> groupsCollection, + GroupIncludeCache groupIncludeCache, + ReviewDb db) { this.groupsCollection = groupsCollection; this.groupIncludeCache = groupIncludeCache; this.db = db; - this.self = self; } @Override public List<GroupInfo> apply(GroupResource resource, Input input) throws MethodNotAllowedException, AuthException, BadRequestException, OrmException { - final GroupDescription.Basic group = resource.getGroup(); - if (!(group instanceof GroupDescription.Internal)) { + AccountGroup group = resource.toAccountGroup(); + if (group == null) { throw new MethodNotAllowedException(); } - input = Input.init(input); - final AccountGroup internalGroup = ((GroupDescription.Internal) group).getAccountGroup(); - final GroupControl control = groupControlFactory.controlFor(internalGroup); - final Map<AccountGroup.UUID, AccountGroupIncludeByUuid> newIncludedGroups = Maps.newHashMap(); - final List<AccountGroupIncludeByUuidAudit> newIncludedGroupsAudits = Lists.newLinkedList(); - final BadRequestHandler badRequest = new BadRequestHandler("adding groups"); - final List<GroupInfo> result = Lists.newLinkedList(); - final Account.Id me = ((IdentifiedUser) self.get()).getAccountId(); + GroupControl control = resource.getControl(); + Map<AccountGroup.UUID, AccountGroupIncludeByUuid> newIncludedGroups = Maps.newHashMap(); + List<AccountGroupIncludeByUuidAudit> newIncludedGroupsAudits = Lists.newLinkedList(); + BadRequestHandler badRequest = new BadRequestHandler("adding groups"); + List<GroupInfo> result = Lists.newLinkedList(); + Account.Id me = ((IdentifiedUser) control.getCurrentUser()).getAccountId(); - for (final String includedGroup : input.groups) { + for (String includedGroup : input.groups) { try { - final GroupResource includedGroupResource = groupsCollection.get().parse(includedGroup); + GroupResource includedGroupResource = groupsCollection.get().parse(includedGroup); if (!control.canAddGroup(includedGroupResource.getGroupUUID())) { throw new AuthException(String.format("Cannot add group: %s", includedGroupResource.getName())); } if (!newIncludedGroups.containsKey(includedGroupResource.getGroupUUID())) { - final AccountGroupIncludeByUuid.Key agiKey = - new AccountGroupIncludeByUuid.Key(internalGroup.getId(), + AccountGroupIncludeByUuid.Key agiKey = + new AccountGroupIncludeByUuid.Key(group.getId(), includedGroupResource.getGroupUUID()); AccountGroupIncludeByUuid agi = db.accountGroupIncludesByUuid().get(agiKey); if (agi == null) { @@ -132,7 +123,7 @@ if (!newIncludedGroups.isEmpty()) { db.accountGroupIncludesByUuidAudit().insert(newIncludedGroupsAudits); db.accountGroupIncludesByUuid().insert(newIncludedGroups.values()); - for (final AccountGroupIncludeByUuid agi : newIncludedGroups.values()) { + for (AccountGroupIncludeByUuid agi : newIncludedGroups.values()) { groupIncludeCache.evictMemberIn(agi.getIncludeUUID()); } groupIncludeCache.evictMembersOf(group.getGroupUUID()); @@ -148,7 +139,7 @@ private final Provider<AddIncludedGroups> put; private final String id; - PutIncludedGroup(final Provider<AddIncludedGroups> put, String id) { + PutIncludedGroup(Provider<AddIncludedGroups> put, String id) { this.put = put; this.id = id; } @@ -162,9 +153,8 @@ List<GroupInfo> list = put.get().apply(resource, in); if (list.size() == 1) { return list.get(0); - } else { - throw new IllegalStateException(); } + throw new IllegalStateException(); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java index d623f9d..b3c4986 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java
@@ -17,7 +17,6 @@ import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.errors.InactiveAccountException; import com.google.gerrit.common.errors.NoSuchAccountException; import com.google.gerrit.extensions.restapi.AuthException; @@ -32,7 +31,6 @@ import com.google.gerrit.reviewdb.client.AuthType; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.BadRequestHandler; -import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountException; @@ -41,8 +39,8 @@ import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.config.AuthConfig; -import com.google.gerrit.server.group.MembersCollection.MemberInfo; import com.google.gerrit.server.group.AddMembers.Input; +import com.google.gerrit.server.group.MembersCollection.MemberInfo; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -71,51 +69,44 @@ } } - private final GroupControl.Factory groupControlFactory; private final AccountManager accountManager; private final AuthType authType; private final AccountResolver accountResolver; private final AccountCache accountCache; private final ReviewDb db; - private final Provider<CurrentUser> self; @Inject - AddMembers(final GroupControl.Factory groupControlFactory, - final AccountManager accountManager, - final AuthConfig authConfig, - final AccountResolver accountResolver, - final AccountCache accountCache, final ReviewDb db, - final Provider<CurrentUser> self) { - this.groupControlFactory = groupControlFactory; + AddMembers(AccountManager accountManager, + AuthConfig authConfig, + AccountResolver accountResolver, + AccountCache accountCache, + ReviewDb db) { this.accountManager = accountManager; this.authType = authConfig.getAuthType(); this.accountResolver = accountResolver; this.accountCache = accountCache; this.db = db; - this.self = self; } @Override public List<MemberInfo> apply(GroupResource resource, Input input) throws AuthException, MethodNotAllowedException, BadRequestException, OrmException { - final GroupDescription.Basic group = resource.getGroup(); - if (!(group instanceof GroupDescription.Internal)) { + AccountGroup internalGroup = resource.toAccountGroup(); + if (internalGroup == null) { throw new MethodNotAllowedException(); } - input = Input.init(input); - final AccountGroup internalGroup = ((GroupDescription.Internal) group).getAccountGroup(); - final GroupControl control = groupControlFactory.controlFor(internalGroup); - final Map<Account.Id, AccountGroupMember> newAccountGroupMembers = Maps.newHashMap(); - final List<AccountGroupMemberAudit> newAccountGroupMemberAudits = Lists.newLinkedList(); - final BadRequestHandler badRequest = new BadRequestHandler("adding new group members"); - final List<MemberInfo> result = Lists.newLinkedList(); - final Account.Id me = ((IdentifiedUser) self.get()).getAccountId(); + GroupControl control = resource.getControl(); + Map<Account.Id, AccountGroupMember> newAccountGroupMembers = Maps.newHashMap(); + List<AccountGroupMemberAudit> newAccountGroupMemberAudits = Lists.newLinkedList(); + BadRequestHandler badRequest = new BadRequestHandler("adding new group members"); + List<MemberInfo> result = Lists.newLinkedList(); + Account.Id me = ((IdentifiedUser) control.getCurrentUser()).getAccountId(); - for (final String nameOrEmail : input.members) { - final Account a = findAccount(nameOrEmail); + for (String nameOrEmail : input.members) { + Account a = findAccount(nameOrEmail); if (a == null) { badRequest.addError(new NoSuchAccountException(nameOrEmail)); continue; @@ -131,7 +122,7 @@ } if (!newAccountGroupMembers.containsKey(a.getId())) { - final AccountGroupMember.Key key = + AccountGroupMember.Key key = new AccountGroupMember.Key(a.getId(), internalGroup.getId()); AccountGroupMember m = db.accountGroupMembers().get(key); if (m == null) { @@ -147,14 +138,14 @@ db.accountGroupMembersAudit().insert(newAccountGroupMemberAudits); db.accountGroupMembers().insert(newAccountGroupMembers.values()); - for (final AccountGroupMember m : newAccountGroupMembers.values()) { + for (AccountGroupMember m : newAccountGroupMembers.values()) { accountCache.evict(m.getAccountId()); } return result; } - private Account findAccount(final String nameOrEmail) throws OrmException { + private Account findAccount(String nameOrEmail) throws OrmException { Account r = accountResolver.find(nameOrEmail); if (r == null) { switch (authType) { @@ -175,7 +166,7 @@ } try { - final AuthRequest req = AuthRequest.forUser(user); + AuthRequest req = AuthRequest.forUser(user); req.setSkipAuthentication(true); return accountCache.get(accountManager.authenticate(req).getAccountId()) .getAccount(); @@ -191,7 +182,7 @@ private final Provider<AddMembers> put; private final String id; - PutMember(final Provider<AddMembers> put, String id) { + PutMember(Provider<AddMembers> put, String id) { this.put = put; this.id = id; } @@ -205,9 +196,8 @@ List<MemberInfo> list = put.get().apply(resource, in); if (list.size() == 1) { return list.get(0); - } else { - throw new IllegalStateException(); } + throw new IllegalStateException(); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java index cbf6876..e8d439c0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java
@@ -16,7 +16,6 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.errors.NoSuchAccountException; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; @@ -44,17 +43,14 @@ import java.util.Map; public class DeleteMembers implements RestModifyView<GroupResource, Input> { - private final GroupControl.Factory groupControlFactory; private final AccountResolver accountResolver; private final AccountCache accountCache; private final ReviewDb db; private final Provider<CurrentUser> self; @Inject - DeleteMembers(final GroupControl.Factory groupControlFactory, - final AccountResolver accountResolver, final AccountCache accountCache, - final ReviewDb db, final Provider<CurrentUser> self) { - this.groupControlFactory = groupControlFactory; + DeleteMembers(AccountResolver accountResolver, AccountCache accountCache, + ReviewDb db, Provider<CurrentUser> self) { this.accountResolver = accountResolver; this.accountCache = accountCache; this.db = db; @@ -65,15 +61,13 @@ public Object apply(GroupResource resource, Input input) throws AuthException, MethodNotAllowedException, BadRequestException, OrmException { - final GroupDescription.Basic group = resource.getGroup(); - if (!(group instanceof GroupDescription.Internal)) { + AccountGroup internalGroup = resource.toAccountGroup(); + if (internalGroup == null) { throw new MethodNotAllowedException(); } - input = Input.init(input); - final AccountGroup internalGroup = ((GroupDescription.Internal) group).getAccountGroup(); - final GroupControl control = groupControlFactory.controlFor(internalGroup); + final GroupControl control = resource.getControl(); final Map<Account.Id, AccountGroupMember> members = getMembers(internalGroup.getId()); final List<AccountGroupMember> toRemove = Lists.newLinkedList(); final BadRequestHandler badRequest = new BadRequestHandler("removing group members"); @@ -159,8 +153,8 @@ throws AuthException, MethodNotAllowedException, BadRequestException, OrmException, NoSuchGroupException { AddMembers.Input in = new AddMembers.Input(); - in._oneMember = resource.getUser().getAccountId().toString(); - return delete.get().apply(resource.getGroup(), in); + in._oneMember = resource.getMember().getAccountId().toString(); + return delete.get().apply(resource, in); } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java index e05316f..5ce50a1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java
@@ -15,7 +15,6 @@ package com.google.gerrit.server.group; import com.google.common.base.Strings; -import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -23,10 +22,10 @@ class GetDescription implements RestReadView<GroupResource> { @Override public String apply(GroupResource resource) throws ResourceNotFoundException { - if (!resource.isInternal()) { + AccountGroup group = resource.toAccountGroup(); + if (group == null) { throw new ResourceNotFoundException(); } - AccountGroup group = GroupDescriptions.toAccountGroup(resource.getGroup()); return Strings.nullToEmpty(group.getDescription()); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java index c6c6c76..36f0914 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java
@@ -14,15 +14,11 @@ package com.google.gerrit.server.group; -import com.google.gerrit.extensions.restapi.AuthException; -import com.google.gerrit.extensions.restapi.BadRequestException; -import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestReadView; class GetGroup implements RestReadView<GroupResource> { @Override - public Object apply(GroupResource resource) throws AuthException, - BadRequestException, ResourceConflictException, Exception { - return new GroupInfo(resource.getControl().getGroup()); + public Object apply(GroupResource resource) { + return new GroupInfo(resource.getGroup()); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java index 175ee77..7d82c6b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java
@@ -19,6 +19,6 @@ public class GetIncludedGroup implements RestReadView<IncludedGroupResource> { @Override public GroupInfo apply(IncludedGroupResource rsrc) { - return new GroupInfo(rsrc.getGroup()); + return new GroupInfo(rsrc.getMemberDescription()); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetMember.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetMember.java index 57fa909..feaa09b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetMember.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetMember.java
@@ -20,6 +20,6 @@ public class GetMember implements RestReadView<MemberResource> { @Override public MemberInfo apply(MemberResource resource) { - return MembersCollection.parse(resource.getUser().getAccount()); + return MembersCollection.parse(resource.getMember().getAccount()); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupInfo.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupInfo.java index 43bf10b..4e894e4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupInfo.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupInfo.java
@@ -16,6 +16,7 @@ import com.google.common.base.Strings; import com.google.gerrit.common.data.GroupDescription; +import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -37,8 +38,9 @@ url = Strings.emptyToNull(group.getUrl()); visibleToAll = group.isVisibleToAll() ? true : null; - if (group instanceof GroupDescription.Internal) { - set(((GroupDescription.Internal) group).getAccountGroup()); + AccountGroup internalGroup = GroupDescriptions.toAccountGroup(group); + if (internalGroup != null) { + set(internalGroup); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java index 1da7b0e..32513a7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java
@@ -15,6 +15,7 @@ package com.google.gerrit.server.group; import com.google.gerrit.common.data.GroupDescription; +import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.extensions.restapi.RestResource; import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -31,6 +32,10 @@ this.control = control; } + GroupResource(GroupResource rsrc) { + this.control = rsrc.getControl(); + } + public GroupDescription.Basic getGroup() { return control.getGroup(); } @@ -43,8 +48,8 @@ return getGroup().getGroupUUID(); } - public boolean isInternal() { - return getGroup() instanceof GroupDescription.Internal; + public AccountGroup toAccountGroup() { + return GroupDescriptions.toAccountGroup(getGroup()); } public GroupControl getControl() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupResource.java index 7d12151..89b99a1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupResource.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupResource.java
@@ -14,15 +14,27 @@ package com.google.gerrit.server.group; +import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.extensions.restapi.RestView; -import com.google.gerrit.server.account.GroupControl; +import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.inject.TypeLiteral; public class IncludedGroupResource extends GroupResource { public static final TypeLiteral<RestView<IncludedGroupResource>> INCLUDED_GROUP_KIND = new TypeLiteral<RestView<IncludedGroupResource>>() {}; - IncludedGroupResource(final GroupControl control) { - super(control); + private final GroupDescription.Basic member; + + IncludedGroupResource(GroupResource group, GroupDescription.Basic member) { + super(group); + this.member = member; + } + + public AccountGroup.UUID getMember() { + return getMemberDescription().getGroupUUID(); + } + + public GroupDescription.Basic getMemberDescription() { + return member; } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java index fb5d13c..ab444c9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java
@@ -21,6 +21,7 @@ import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestView; +import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuid; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.group.AddIncludedGroups.PutIncludedGroup; @@ -55,25 +56,31 @@ } @Override - public IncludedGroupResource parse(GroupResource parent, IdString id) + public IncludedGroupResource parse(GroupResource resource, IdString id) throws ResourceNotFoundException, OrmException { - if (!parent.isInternal()) { + AccountGroup parent = resource.toAccountGroup(); + if (parent == null) { throw new ResourceNotFoundException(id); } - GroupDescription.Internal p = (GroupDescription.Internal) parent.getGroup(); - GroupResource included = groupsCollection.get().parse(id.get()); - AccountGroupIncludeByUuid in = dbProvider.get() - .accountGroupIncludesByUuid().get( - new AccountGroupIncludeByUuid.Key( - p.getAccountGroup().getId(), - included.getGroupUUID())); - if (in != null) { - return new IncludedGroupResource(included.getControl()); + GroupDescription.Basic member = + groupsCollection.get().parse(id.get()).getGroup(); + + if (isMember(parent, member) + && resource.getControl().canSeeGroup(member.getGroupUUID())) { + return new IncludedGroupResource(resource, member); } throw new ResourceNotFoundException(id); } + private boolean isMember(AccountGroup parent, GroupDescription.Basic member) + throws OrmException { + return dbProvider.get().accountGroupIncludesByUuid().get( + new AccountGroupIncludeByUuid.Key( + parent.getId(), + member.getGroupUUID())) != null; + } + @SuppressWarnings("unchecked") @Override public PutIncludedGroup create(GroupResource group, IdString id) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java index d6f135d..4e264f8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java
@@ -15,12 +15,11 @@ package com.google.gerrit.server.group; import static com.google.common.base.Strings.nullToEmpty; + import com.google.common.collect.Lists; -import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuid; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.GroupControl; @@ -50,7 +49,7 @@ @Override public List<GroupInfo> apply(GroupResource rsrc) throws ResourceNotFoundException, OrmException { - if (!rsrc.isInternal()) { + if (rsrc.toAccountGroup() == null) { throw new ResourceNotFoundException(rsrc.getGroupUUID().get()); } @@ -58,7 +57,7 @@ List<GroupInfo> included = Lists.newArrayList(); for (AccountGroupIncludeByUuid u : dbProvider.get() .accountGroupIncludesByUuid() - .byGroup(groupId(rsrc))) { + .byGroup(rsrc.toAccountGroup().getId())) { try { GroupControl i = controlFactory.controlFor(u.getIncludeUUID()); if (ownerOfParent || i.isVisible()) { @@ -83,9 +82,4 @@ }); return included; } - - private static AccountGroup.Id groupId(GroupResource rsrc) { - GroupDescription.Basic d = rsrc.getGroup(); - return ((GroupDescription.Internal) d).getAccountGroup().getId(); - } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/MemberResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/MemberResource.java index a121e98..52a37a8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/MemberResource.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/MemberResource.java
@@ -16,21 +16,20 @@ import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.account.AccountResource; import com.google.inject.TypeLiteral; -public class MemberResource extends AccountResource { +public class MemberResource extends GroupResource { public static final TypeLiteral<RestView<MemberResource>> MEMBER_KIND = new TypeLiteral<RestView<MemberResource>>() {}; - private final GroupResource group; + private final IdentifiedUser user; public MemberResource(GroupResource group, IdentifiedUser user) { - super(user); - this.group = group; + super(group); + this.user = user; } - public GroupResource getGroup() { - return group; + public IdentifiedUser getMember() { + return user; } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java index 24d7c31..1c1aa2c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java
@@ -23,40 +23,34 @@ import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.extensions.restapi.Url; 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.AccountResolver; -import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.group.AddMembers.PutMember; import com.google.inject.Inject; import com.google.inject.Provider; public class MembersCollection implements ChildCollection<GroupResource, MemberResource>, - AcceptsCreate<GroupResource>{ - + AcceptsCreate<GroupResource> { private final DynamicMap<RestView<MemberResource>> views; private final Provider<ListMembers> list; private final IdentifiedUser.GenericFactory userGenericFactory; - private final GroupCache groupCache; private final AccountResolver accountResolver; private final Provider<ReviewDb> db; private final Provider<AddMembers> put; @Inject - MembersCollection(final DynamicMap<RestView<MemberResource>> views, - final Provider<ListMembers> list, - final IdentifiedUser.GenericFactory userGenericFactory, - final GroupCache groupCache, - final AccountResolver accountResolver, - final Provider<ReviewDb> db, - final Provider<AddMembers> put) { + MembersCollection(DynamicMap<RestView<MemberResource>> views, + Provider<ListMembers> list, + IdentifiedUser.GenericFactory userGenericFactory, + AccountResolver accountResolver, + Provider<ReviewDb> db, + Provider<AddMembers> put) { this.views = views; this.list = list; this.userGenericFactory = userGenericFactory; - this.groupCache = groupCache; this.accountResolver = accountResolver; this.db = db; this.put = put; @@ -71,19 +65,22 @@ @Override public MemberResource parse(GroupResource parent, IdString id) throws ResourceNotFoundException, Exception { + if (parent.toAccountGroup() == null) { + throw new ResourceNotFoundException(id); + } + Account a = accountResolver.find(id.get()); if (a == null) { throw new ResourceNotFoundException(id); } - AccountGroup group = groupCache.get(parent.getControl().getGroup().getGroupUUID()); - if (group == null) { - throw new ResourceNotFoundException(id); - } - - if (db.get().accountGroupMembers() - .get(new AccountGroupMember.Key(a.getId(), group.getId())) != null) { - return new MemberResource(parent, userGenericFactory.create(a.getId())); + AccountGroupMember.Key key = new AccountGroupMember.Key( + a.getId(), + parent.toAccountGroup().getId()); + if (db.get().accountGroupMembers().get(key) != null + && parent.getControl().canSeeMember(a.getId())) { + IdentifiedUser user = userGenericFactory.create(a.getId()); + return new MemberResource(parent, user); } throw new ResourceNotFoundException(id); } @@ -99,8 +96,8 @@ return views; } - public static MemberInfo parse(final Account account) { - final MemberInfo accountInfo = new MemberInfo(); + public static MemberInfo parse(Account account) { + MemberInfo accountInfo = new MemberInfo(); accountInfo.setId(account.getId()); accountInfo.fullName = account.getFullName(); accountInfo.preferredEmail = account.getPreferredEmail();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java index 3aebf01..7fbd8fa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java
@@ -15,7 +15,6 @@ package com.google.gerrit.server.group; import com.google.common.base.Strings; -import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.DefaultInput; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; @@ -57,14 +56,14 @@ delete = true; } - if (!resource.isInternal()) { + if (resource.toAccountGroup() == null) { throw new MethodNotAllowedException(); } else if (!resource.getControl().isOwner()) { throw new AuthException("Not group owner"); } AccountGroup group = db.accountGroups().get( - GroupDescriptions.toAccountGroup(resource.getGroup()).getId()); + resource.toAccountGroup().getId()); if (group == null) { throw new ResourceNotFoundException(); }