Delete group membership request when user is manually added to that g…

…roup (#491)

* Consider both the SCIM and the IAM account API

iam-login-service/src/main/java/it/infn/mw/iam/api/account/group/AccountGroupController.java

@@ -33,22 +33,26 @@
 
 import it.infn.mw.iam.api.common.ErrorDTO;
 import it.infn.mw.iam.api.common.error.NoSuchAccountError;
+import it.infn.mw.iam.api.requests.service.GroupRequestsService;
 import it.infn.mw.iam.core.group.IamGroupService;
 import it.infn.mw.iam.core.group.error.NoSuchGroupError;
 import it.infn.mw.iam.core.user.IamAccountService;
 import it.infn.mw.iam.persistence.model.IamAccount;
 import it.infn.mw.iam.persistence.model.IamGroup;
+import it.infn.mw.iam.persistence.model.IamGroupRequest;
 
 @RestController
 public class AccountGroupController {
 
   private final IamAccountService accountService;
   private final IamGroupService groupService;
+  private final GroupRequestsService groupRequestsService;
 
   @Autowired
-  public AccountGroupController(IamAccountService accountService, IamGroupService groupService) {
+  public AccountGroupController(IamAccountService accountService, IamGroupService groupService, GroupRequestsService groupRequestsService) {
     this.accountService = accountService;
     this.groupService = groupService;
+    this.groupRequestsService = groupRequestsService;
   }
 
   @RequestMapping(value = "/iam/account/{accountUuid}/groups/{groupUuid}", method = POST)
@@ -61,6 +65,12 @@ public void addAccountToGroup(@PathVariable String accountUuid, @PathVariable St
         accountService.findByUuid(accountUuid).orElseThrow(noSuchAccount(accountUuid));
 
     accountService.addToGroup(account, group);
+
+    for (IamGroupRequest r : group.getGroupRequests()) {
+      if (accountUuid.equals(r.getAccount().getUuid())) {
+        groupRequestsService.deleteGroupRequest(r.getUuid());
+      }
+    }
   }
 
   @RequestMapping(value = "/iam/account/{accountUuid}/groups/{groupUuid}", method = DELETE)

iam-login-service/src/main/java/it/infn/mw/iam/api/scim/provisioning/ScimGroupProvisioning.java

@@ -33,6 +33,7 @@
 import com.google.common.base.Strings;
 
 import it.infn.mw.iam.api.common.OffsetPageable;
+import it.infn.mw.iam.api.requests.service.GroupRequestsService;
 import it.infn.mw.iam.api.scim.converter.GroupConverter;
 import it.infn.mw.iam.api.scim.converter.ScimResourceLocationProvider;
 import it.infn.mw.iam.api.scim.exception.IllegalArgumentException;
@@ -52,32 +53,34 @@
 import it.infn.mw.iam.core.user.IamAccountService;
 import it.infn.mw.iam.persistence.model.IamAccount;
 import it.infn.mw.iam.persistence.model.IamGroup;
+import it.infn.mw.iam.persistence.model.IamGroupRequest;
 
 @Service
 @Transactional
-public class ScimGroupProvisioning
-    implements ScimProvisioning<ScimGroup, List<ScimMemberRef>> {
+public class ScimGroupProvisioning implements ScimProvisioning<ScimGroup, List<ScimMemberRef>> {
 
   private static final int GROUP_NAME_MAX_LENGTH = 50;
   private static final int GROUP_FULLNAME_MAX_LENGTH = 512;
 
   private final IamGroupService groupService;
   private final IamAccountService accountService;
-
   private final GroupConverter converter;
+  private final GroupRequestsService groupRequestsService;
 
   private final DefaultGroupMembershipUpdaterFactory groupUpdaterFactory;
 
   private final ScimResourceLocationProvider locationProvider;
 
   @Autowired
   public ScimGroupProvisioning(IamGroupService groupService, IamAccountService accountService,
-      GroupConverter converter, ScimResourceLocationProvider locationProvider, Clock clock) {
+      GroupRequestsService groupRequestsService, GroupConverter converter,
+      ScimResourceLocationProvider locationProvider, Clock clock) {
 
     this.accountService = accountService;
     this.groupService = groupService;
     this.converter = converter;
 
+    this.groupRequestsService = groupRequestsService;
     this.groupUpdaterFactory = new DefaultGroupMembershipUpdaterFactory(accountService);
     this.locationProvider = locationProvider;
   }
@@ -159,10 +162,15 @@ private void executePatchOperation(IamGroup group, ScimPatchOperation<List<ScimM
     patchOperationSanityChecks(op);
     groupUpdaterFactory.getUpdatersForPatchOperation(group, op).forEach(Updater::update);
 
+    for (IamGroupRequest r : group.getGroupRequests()) {
+      for (ScimMemberRef m : op.getValue()) {
+        if (m.getValue().equals(r.getAccount().getUuid())) {
+          groupRequestsService.deleteGroupRequest(r.getUuid());
+        }
+      }
+    }
   }
 
-
-
   private void fullNameSanityChecks(String displayName) {
     if (displayName.length() > GROUP_FULLNAME_MAX_LENGTH) {
       throw new IllegalArgumentException(format(
@@ -258,6 +266,7 @@ public void update(String id, List<ScimPatchOperation<List<ScimMemberRef>>> oper
     IamGroup iamGroup = groupService.findByUuid(id).orElseThrow(noGroupMappedToId(id));
 
     operations.forEach(op -> executePatchOperation(iamGroup, op));
+
   }
 
 
@@ -286,8 +295,7 @@ public ScimListResponse<ScimMemberRef> listAccountMembers(String id,
     return results.build();
   }
 
-  public ScimListResponse<ScimMemberRef> listGroupMembers(String id,
-      ScimPageRequest pageRequest) {
+  public ScimListResponse<ScimMemberRef> listGroupMembers(String id, ScimPageRequest pageRequest) {
 
     IamGroup iamGroup = groupService.findByUuid(id).orElseThrow(noGroupMappedToId(id));
 

iam-login-service/src/main/java/it/infn/mw/iam/api/scim/updater/factory/DefaultGroupMembershipUpdaterFactory.java

@@ -37,6 +37,7 @@ public class DefaultGroupMembershipUpdaterFactory
 
   final IamAccountService accountService;
 
+
   public DefaultGroupMembershipUpdaterFactory(IamAccountService accountService) {
 
     this.accountService = accountService;
@@ -70,6 +71,7 @@ private void prepareAdders(List<AccountUpdater> updaters, List<IamAccount> membe
     for (IamAccount memberToAdd : membersToAdd) {
       GroupMembershipManagement mgmt = new GroupMembershipManagement(memberToAdd, accountService);
       updaters.add(mgmt.addToGroup(group));
+
     }
   }
 

iam-login-service/src/test/java/it/infn/mw/iam/test/api/requests/GroupRequestsDeleteTests.java

@@ -15,10 +15,14 @@
  */
 package it.infn.mw.iam.test.api.requests;
 
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 
 import java.util.UUID;
+import java.util.function.Supplier;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -31,6 +35,11 @@
 
 import it.infn.mw.iam.IamLoginService;
 import it.infn.mw.iam.api.requests.model.GroupRequestDto;
+import it.infn.mw.iam.persistence.model.IamAccount;
+import it.infn.mw.iam.persistence.model.IamGroup;
+import it.infn.mw.iam.persistence.repository.IamAccountRepository;
+import it.infn.mw.iam.persistence.repository.IamGroupRepository;
+import it.infn.mw.iam.persistence.repository.IamGroupRequestRepository;
 import it.infn.mw.iam.test.util.WithAnonymousUser;
 import it.infn.mw.iam.test.util.annotation.IamMockMvcIntegrationTest;
 
@@ -41,10 +50,25 @@
 public class GroupRequestsDeleteTests extends GroupRequestsTestUtils {
 
   private final static String DELETE_URL = "/iam/group_requests/{uuid}";
+  private static final String EXPECTED_USER_NOT_FOUND = "expected user not found";
+  private static final String EXPECTED_GROUP_NOT_FOUND = "expected group not found";
+
+  @Autowired
+  private IamAccountRepository accountRepo;
+
+  @Autowired
+  private IamGroupRepository groupRepo;
+
+  @Autowired
+  private IamGroupRequestRepository groupRequestRepo;
 
   @Autowired
   private MockMvc mvc;
 
+  private Supplier<AssertionError> assertionError(String message) {
+    return () -> new AssertionError(message);
+  }
+
   @Test
   @WithMockUser(roles = {"ADMIN"})
   public void deletePendingGroupRequestAsAdmin() throws Exception {
@@ -105,7 +129,7 @@ public void deleteGroupRequestOfAnotherUser() throws Exception {
   @Test
   @WithAnonymousUser
   public void deleteGroupRequestAsAnonymous() throws Exception {
-    
+
     GroupRequestDto request = savePendingGroupRequest(TEST_100_USERNAME, TEST_001_GROUPNAME);
 
     // @formatter:off
@@ -129,13 +153,46 @@ public void deleteNotExitingGroupRequest() throws Exception {
   @Test
   @WithMockUser(roles = {"ADMIN", "USER"})
   public void deletePendingGroupRequestAsUserWithBothRoles() throws Exception {
-    
+
     GroupRequestDto request = savePendingGroupRequest(TEST_100_USERNAME, TEST_001_GROUPNAME);
-    
+
     // @formatter:off
     mvc.perform(delete(DELETE_URL, request.getUuid()))
       .andExpect(status().isNoContent());
     // @formatter:on
   }
 
+  @Test
+  @WithMockUser(roles = {"ADMIN"})
+  public void deleteGRIfAccountAddedToGroup() throws Exception {
+
+    savePendingGroupRequest(TEST_100_USERNAME, TEST_001_GROUPNAME);
+    savePendingGroupRequest(TEST_101_USERNAME, TEST_001_GROUPNAME);
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_100_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(false));
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_101_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(false));
+
+    IamAccount account = accountRepo.findByUsername(TEST_100_USERNAME)
+      .orElseThrow(assertionError(EXPECTED_USER_NOT_FOUND));
+
+    IamGroup group = groupRepo.findByName(TEST_001_GROUPNAME)
+      .orElseThrow(assertionError(EXPECTED_GROUP_NOT_FOUND));
+
+    mvc.perform(post("/iam/account/" + account.getUuid() + "/groups/" + group.getUuid()))
+      .andExpect(status().isCreated());
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_100_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(true));
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_101_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(false));
+  }
+
 }

iam-login-service/src/test/java/it/infn/mw/iam/test/scim/group/patch/ScimGroupPatchUtils.java

@@ -39,9 +39,10 @@
 import it.infn.mw.iam.api.scim.model.ScimResource;
 import it.infn.mw.iam.api.scim.model.ScimUser;
 import it.infn.mw.iam.test.TestUtils;
+import it.infn.mw.iam.test.api.requests.GroupRequestsTestUtils;
 import it.infn.mw.iam.test.scim.ScimUtils;
 
-public class ScimGroupPatchUtils {
+public class ScimGroupPatchUtils extends GroupRequestsTestUtils {
 
   public static final String GROUP_URI = ScimUtils.getGroupsLocation();
   public static final String USER_URI = ScimUtils.getUsersLocation();

iam-login-service/src/test/java/it/infn/mw/iam/test/scim/group/patch/ScimGroupProvisioningPatchAddTests.java

@@ -16,6 +16,8 @@
 package it.infn.mw.iam.test.scim.group.patch;
 
 import static it.infn.mw.iam.api.scim.model.ScimConstants.SCIM_CONTENT_TYPE;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
@@ -25,6 +27,7 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Supplier;
 
 import org.junit.After;
 import org.junit.Before;
@@ -33,9 +36,16 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.test.context.junit4.SpringRunner;
 
+import it.infn.mw.iam.api.scim.converter.GroupConverter;
+import it.infn.mw.iam.api.scim.converter.UserConverter;
 import it.infn.mw.iam.api.scim.model.ScimGroup;
 import it.infn.mw.iam.api.scim.model.ScimGroupPatchRequest;
 import it.infn.mw.iam.api.scim.model.ScimUser;
+import it.infn.mw.iam.persistence.model.IamAccount;
+import it.infn.mw.iam.persistence.model.IamGroup;
+import it.infn.mw.iam.persistence.repository.IamAccountRepository;
+import it.infn.mw.iam.persistence.repository.IamGroupRepository;
+import it.infn.mw.iam.persistence.repository.IamGroupRequestRepository;
 import it.infn.mw.iam.test.util.WithMockOAuthUser;
 import it.infn.mw.iam.test.util.annotation.IamMockMvcIntegrationTest;
 import it.infn.mw.iam.test.util.oauth.MockOAuth2Filter;
@@ -49,9 +59,28 @@ public class ScimGroupProvisioningPatchAddTests extends ScimGroupPatchUtils {
   @Autowired
   private MockOAuth2Filter mockOAuth2Filter;
 
+  @Autowired
+  private IamAccountRepository iamAccountRepo;
+
+  @Autowired
+  private IamGroupRepository iamGroupRepo;
+
+  @Autowired
+  private UserConverter userConverter;
+
+  @Autowired
+  private GroupConverter groupConverter;
+
+  @Autowired
+  private IamGroupRequestRepository groupRequestRepo;
+
   private ScimGroup engineers;
   private ScimUser lennon, lincoln;
 
+  private Supplier<AssertionError> assertionError(String message) {
+    return () -> new AssertionError(message);
+  }
+
   @Before
   public void setup() throws Exception {
     mockOAuth2Filter.cleanupSecurityContext();
@@ -86,6 +115,53 @@ public void testGroupPatchAddMember() throws Exception {
     assertIsGroupMember(lennon, engineers);
   }
 
+  @Test
+  public void testGroupPatchAddMemberAndDeleteGMR() throws Exception {
+
+    List<ScimUser> members = new ArrayList<ScimUser>();
+    IamAccount account =
+        iamAccountRepo.findByUsername(TEST_100_USERNAME).orElseThrow(assertionError("Not found"));
+    ScimUser scimUser = userConverter.dtoFromEntity(account);
+    IamAccount account2 =
+        iamAccountRepo.findByUsername(TEST_101_USERNAME).orElseThrow(assertionError("Not found"));
+    ScimUser scimUser2 = userConverter.dtoFromEntity(account2);
+    members.add(scimUser);
+
+    IamGroup group =
+        iamGroupRepo.findByName(TEST_001_GROUPNAME).orElseThrow(assertionError("Not found"));
+    ScimGroup scimGroup = groupConverter.dtoFromEntity(group);
+
+    savePendingGroupRequest(scimUser.getUserName(), scimGroup.getDisplayName());
+    savePendingGroupRequest(scimUser2.getUserName(), scimGroup.getDisplayName());
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_100_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(false));
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_101_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(false));
+
+    ScimGroupPatchRequest patchReq = getPatchAddUsersRequest(members);
+
+    //@formatter:off
+    mvc.perform(patch(scimGroup.getMeta().getLocation())
+        .contentType(SCIM_CONTENT_TYPE)
+        .content(objectMapper.writeValueAsString(patchReq)))
+      .andExpect(status().isNoContent());
+    //@formatter:on
+
+    assertIsGroupMember(scimUser, scimGroup);
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_100_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(true));
+
+    assertThat(
+        groupRequestRepo.findByUsernameAndGroup(TEST_101_USERNAME, TEST_001_GROUPNAME).isEmpty(),
+        is(false));
+  }
+
   @Test
   public void testGroupPatchAddMembers() throws Exception {