Merge pull request #1189 from adorsys/1132-User-update-groups

Fix to update user groups
adorsys · Nov 15, 2024 · b53acdc · b53acdc
2 parents dc4944a + c58faf7
commit b53acdc
Show file tree

Hide file tree

Showing 10 changed files with 36 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,11 +6,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 ## [Unreleased]
 
+### Fixed
+- Fix to update user groups [#1132](https://github.com/adorsys/keycloak-config-cli/issues/1132)
+
 ## Fixed
 - otpPolicyAlgorithm ignored during import [#847](https://github.com/adorsys/keycloak-config-cli/issues/847)
 
 ### Added
 - Added Navigation in the readme [#1099](https://github.com/adorsys/keycloak-config-cli/issues/1099)
+
 ### Added
 - improved logging for realm retrieval errors [#1010](https://github.com/adorsys/keycloak-config-cli/issues/1010)
 ### Fixed

diff --git a/src/main/java/de/adorsys/keycloak/config/service/UserImportService.java b/src/main/java/de/adorsys/keycloak/config/service/UserImportService.java
@@ -183,7 +183,11 @@ private void handleGroups() {
                     .toList();
 
             handleGroupsToBeAdded(userGroupsToUpdate, existingUserGroups);
-            handleGroupsToBeRemoved(userGroupsToUpdate, existingUserGroups);
+
+            if (importConfigProperties.getManaged().getGroup()
+                    == ImportConfigProperties.ImportManagedProperties.ImportManagedPropertiesValues.FULL) {
+                handleGroupsToBeRemoved(userGroupsToUpdate, existingUserGroups);
+            }
         }
 
         private void handleGroupsToBeAdded(

diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties
@@ -37,7 +37,7 @@ import.behaviors.sync-user-federation=false
 import.behaviors.checksum-with-cache-key=true
 import.behaviors.checksum-changed=continue
 import.managed.authentication-flow=full
-import.managed.group=full
+import.managed.group=no-delete
 import.managed.required-action=full
 import.managed.client-scope=full
 import.managed.scope-mapping=full

diff --git a/src/test/java/de/adorsys/keycloak/config/service/ImportGroupsIT.java b/src/test/java/de/adorsys/keycloak/config/service/ImportGroupsIT.java
@@ -1689,7 +1689,7 @@ void shouldUpdateRealmUpdateGroupWithSubstringOfExistingGroupName() throws IOExc
 
     @Test
     @Order(78)
-    void shouldUpdateRealmDeleteGroup() throws IOException {
+    void shouldUpdateRealmAddedGroup() throws IOException {
         GroupRepresentation updatedGroup = tryToLoadGroup("/My Added Group").get();
         assertThat(updatedGroup.getName(), Matchers.is(Matchers.equalTo("My Added Group")));
 
@@ -1702,7 +1702,7 @@ void shouldUpdateRealmDeleteGroup() throws IOException {
 
         assertThat(realm.getRealm(), is(REALM_NAME));
 
-        assertThat(tryToLoadGroup("/My Added Group").isPresent(), is(false));
+        assertThat(tryToLoadGroup("/My Added Group").isPresent(), is(true));
         assertThat(tryToLoadGroup("/My Group").isPresent(), is(true));
     }
 

diff --git a/src/test/java/de/adorsys/keycloak/config/service/ImportUserFederationIT.java b/src/test/java/de/adorsys/keycloak/config/service/ImportUserFederationIT.java
@@ -159,7 +159,7 @@ void importFederationChangeUserGroupWithReadonlyProvider() throws IOException {
         assertThat(user.getFirstName(), is("James"));
 
         List<GroupRepresentation> userGroups = getGroupsByUser(user);
-        assertThat(userGroups, hasSize(1));
+        assertThat(userGroups, hasSize(2));
 
         GroupRepresentation group = getGroupsByPath(userGroups, "/realm/group2");
         assertThat(group, is(notNullValue()));
@@ -182,7 +182,7 @@ void importFederationRemoveUserGroupWithReadonlyProvider() throws IOException {
         assertThat(user.getFirstName(), is("James"));
 
         List<GroupRepresentation> userGroups = getGroupsByUser(user);
-        assertThat(userGroups, hasSize(0));
+        assertThat(userGroups, hasSize(2));
     }
 
     @Test
@@ -201,7 +201,7 @@ void importFederationUserChangeAttributeWithReadonlyProvider() throws IOExceptio
         assertThat(user.getFirstName(), is("James"));
 
         List<GroupRepresentation> userGroups = getGroupsByUser(user);
-        assertThat(userGroups, hasSize(0));
+        assertThat(userGroups, hasSize(2));
     }
 
     private List<GroupRepresentation> getGroupsByUser(UserRepresentation user) {

diff --git a/src/test/java/de/adorsys/keycloak/config/service/ImportUsersIT.java b/src/test/java/de/adorsys/keycloak/config/service/ImportUsersIT.java
@@ -400,8 +400,7 @@ void shouldUpdateRealmUpdateUserChangeGroup() throws IOException {
 
     @Test
     @Order(11)
-    void shouldUpdateRealmUpdateUserRemoveGroup() throws IOException {
-        // Create Users
+    void shouldUpdateRealmUpdateUserKeepExistingGroups() throws IOException {
         doImport("11_update_realm_update_user_remove_group.json");
 
         final RealmRepresentation realm = keycloakProvider.getInstance().realm(REALM_NAME).toRepresentation();
@@ -414,10 +413,10 @@ void shouldUpdateRealmUpdateUserRemoveGroup() throws IOException {
         assertThat(user.getFirstName(), is("firstName1"));
 
         List<GroupRepresentation> userGroups = getGroupsByUser(user);
-        assertThat(userGroups, hasSize(1));
+        assertThat(userGroups, hasSize(2));  // User should still be in both groups
 
         GroupRepresentation group1 = getGroupsByPath(userGroups, "/group1");
-        assertThat(group1, nullValue());
+        assertThat(group1.getName(), is("group1"));
 
         GroupRepresentation group2 = getGroupsByPath(userGroups, "/group2");
         assertThat(group2.getName(), is("group2"));
@@ -439,16 +438,17 @@ void shouldUpdateRealmUpdateUserAddSubGroup() throws IOException {
         assertThat(user.getFirstName(), is("firstName1"));
 
         List<GroupRepresentation> userGroups = getGroupsByUser(user);
-        assertThat(userGroups, hasSize(1));
+        assertThat(userGroups, hasSize(3));  // User should now be in all groups
 
         GroupRepresentation group1 = getGroupsByPath(userGroups, "/group1/subgroup1");
         assertThat(group1.getName(), is("subgroup1"));
+        GroupRepresentation group2 = getGroupsByPath(userGroups, "/group2");
+        assertThat(group2.getName(), is("group2"));
     }
 
     @Test
     @Order(13)
-    void shouldUpdateRealmUpdateUserChangeSubGroup() throws IOException {
-        // Create Users
+    void shouldUpdateRealmUpdateUserAddNewSubGroup() throws IOException {
         doImport("13_update_realm_update_user_change_subgroup.json");
 
         final RealmRepresentation realm = keycloakProvider.getInstance().realm(REALM_NAME).toRepresentation();
@@ -461,7 +461,7 @@ void shouldUpdateRealmUpdateUserChangeSubGroup() throws IOException {
         assertThat(user.getFirstName(), is("firstName1"));
 
         List<GroupRepresentation> userGroups = getGroupsByUser(user);
-        assertThat(userGroups, hasSize(2));
+        assertThat(userGroups, hasSize(4));  // User should now be in all groups
 
         GroupRepresentation group1 = getGroupsByPath(userGroups, "/group1/subgroup1");
         assertThat(group1.getName(), is("subgroup1"));
@@ -472,7 +472,7 @@ void shouldUpdateRealmUpdateUserChangeSubGroup() throws IOException {
 
     @Test
     @Order(14)
-    void shouldUpdateRealmUpdateUserRemoveSubGroup() throws IOException {
+    void shouldUpdateRealmUpdateUserKeepExistingSubGroups() throws IOException {
         doImport("14_update_realm_update_user_remove_subgroup.json");
 
         final RealmRepresentation realm = keycloakProvider.getInstance().realm(REALM_NAME).toRepresentation();
@@ -485,10 +485,10 @@ void shouldUpdateRealmUpdateUserRemoveSubGroup() throws IOException {
         assertThat(user.getFirstName(), is("firstName1"));
 
         List<GroupRepresentation> userGroups = getGroupsByUser(user);
-        assertThat(userGroups, hasSize(1));
+        assertThat(userGroups, hasSize(4));
 
         GroupRepresentation group1 = getGroupsByPath(userGroups, "/group1/subgroup1");
-        assertThat(group1, nullValue());
+        assertThat(group1.getName(), is("subgroup1"));
 
         GroupRepresentation group2 = getGroupsByPath(userGroups, "/group2/subgroup2");
         assertThat(group2.getName(), is("subgroup2"));

diff --git a/src/test/resources/import-files/users/11_update_realm_update_user_remove_group.json b/src/test/resources/import-files/users/11_update_realm_update_user_remove_group.json
@@ -17,7 +17,8 @@
       "firstName": "firstName1",
       "lastName": "lastName1",
       "groups": [
-        "group2"
+        "group2",
+        "group1"
       ]
     },
     {

diff --git a/src/test/resources/import-files/users/12_update_realm_update_user_add_subgroup.json b/src/test/resources/import-files/users/12_update_realm_update_user_add_subgroup.json
@@ -27,7 +27,9 @@
       "firstName": "firstName1",
       "lastName": "lastName1",
       "groups": [
-        "/group1/subgroup1"
+        "/group1/subgroup1",
+        "/group1",
+        "/group2"
       ]
     },
     {

diff --git a/src/test/resources/import-files/users/13_update_realm_update_user_change_subgroup.json b/src/test/resources/import-files/users/13_update_realm_update_user_change_subgroup.json
@@ -28,7 +28,9 @@
       "lastName": "lastName1",
       "groups": [
         "/group1/subgroup1",
-        "/group2/subgroup2"
+        "/group2/subgroup2",
+        "/group1",
+        "/group2"
       ]
     },
     {

diff --git a/src/test/resources/import-files/users/14_update_realm_update_user_remove_subgroup.json b/src/test/resources/import-files/users/14_update_realm_update_user_remove_subgroup.json
@@ -27,7 +27,8 @@
       "firstName": "firstName1",
       "lastName": "lastName1",
       "groups": [
-        "/group2/subgroup2"
+        "/group2/subgroup2",
+        "/group1/subgroup1"
       ]
     },
     {