Skip to content

Commit 2b5aec8

Browse files
clement-dufaureDonatien26
clement-dufaure
authored and
Donatien26
committed
[BUG] 🐛 last element of a list can't be removed
Signed-off-by: Clément Dufaure <clement.dufaure@insee.fr>
1 parent 36adaf6 commit 2b5aec8

9 files changed

Lines changed: 159 additions & 75 deletions

File tree

‎sugoi-api-converter/src/test/java/fr/insee/sugoi/converter/ContactsTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ public void testConvertContactToUserJson() throws JsonProcessingException {
196196
OuganextSugoiMapper osm = new OuganextSugoiMapper();
197197
User user = osm.serializeToSugoi(contact, User.class);
198198
String expectedJson =
199-
"{\"lastName\":\"test\",\"firstName\":\"test\",\"mail\":\"tes.tkmgfdl@jhk.gmail\",\"username\":\"test\",\"organization\":{\"identifiant\":\"lorganisation\",\"gpgkey\":null,\"organization\":null,\"address\":null,\"metadatas\":{},\"attributes\":{\"proprietes\":null,\"nomCommun\":null,\"mail\":null,\"domaineDeGestion\":null,\"repertoireDeDistribution\":null,\"description\":null,\"numeroTelephone\":null,\"facSimile\":null}},\"groups\":[],\"habilitations\":[],\"address\":{\"line1\":\"15 rue Gabriel Peri\",\"line2\":\"\",\"line3\":\"\",\"line4\":\"\",\"line5\":\"\",\"line6\":\"\",\"line7\":\"92240 Malakoff\"},\"metadatas\":{\"dateCreation\":null,\"hasPassword\":false},\"attributes\":{\"proprietes\":null,\"insee_roles_applicatifs\":null,\"nomCommun\":\"Test\",\"domaineDeGestion\":\"testDG\",\"repertoireDeDistribution\":null,\"description\":\"description\",\"telephonePortable\":\"061245789636\",\"codePin\":\"AAA=\",\"numeroTelephone\":\"0123456789\",\"identifiantMetier\":\"123456789\",\"civilite\":\"Camarade\",\"facSimile\":\"0123456789\"}}";
199+
"{\"lastName\":\"test\",\"firstName\":\"test\",\"mail\":\"tes.tkmgfdl@jhk.gmail\",\"username\":\"test\",\"organization\":{\"identifiant\":\"lorganisation\",\"gpgkey\":null,\"organization\":null,\"address\":null,\"metadatas\":{},\"attributes\":{\"proprietes\":null,\"nomCommun\":null,\"mail\":null,\"domaineDeGestion\":null,\"repertoireDeDistribution\":null,\"description\":null,\"numeroTelephone\":null,\"facSimile\":null}},\"address\":{\"line1\":\"15 rue Gabriel Peri\",\"line2\":\"\",\"line3\":\"\",\"line4\":\"\",\"line5\":\"\",\"line6\":\"\",\"line7\":\"92240 Malakoff\"},\"metadatas\":{\"dateCreation\":null,\"hasPassword\":false},\"attributes\":{\"proprietes\":null,\"insee_roles_applicatifs\":null,\"nomCommun\":\"Test\",\"domaineDeGestion\":\"testDG\",\"repertoireDeDistribution\":null,\"description\":\"description\",\"telephonePortable\":\"061245789636\",\"codePin\":\"AAA=\",\"numeroTelephone\":\"0123456789\",\"identifiantMetier\":\"123456789\",\"civilite\":\"Camarade\",\"facSimile\":\"0123456789\"}}";
200200
assertEquals(expectedJson, CustomObjectMapper.JsonObjectMapper().writeValueAsString(user));
201201
} catch (Exception e) {
202202
fail(e);

‎sugoi-api-file-store-provider/src/main/java/fr/insee/sugoi/store/file/FileReaderStore.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,13 @@ public Optional<User> getUser(String id) {
6969
String nestedOrgaId = user.getOrganization().getIdentifiant();
7070
user.setOrganization(getOrganization(nestedOrgaId).orElse(null));
7171
}
72+
// Add empty habilitations and groups if not already added
73+
if (user != null && user.getHabilitations() == null) {
74+
user.setHabilitations(new ArrayList<>());
75+
}
76+
if (user != null && user.getGroups() == null) {
77+
user.setGroups(new ArrayList<>());
78+
}
7279
return Optional.of(user);
7380
} else {
7481
return Optional.empty();

‎sugoi-api-file-store-provider/src/main/java/fr/insee/sugoi/store/file/FileWriterStore.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -308,10 +308,7 @@ public ProviderResponse addUserToGroup(
308308
() -> new UserNotFoundException(config.get(GlobalKeysConfig.REALM), userId));
309309
if (user != null) {
310310
try {
311-
if (user.getGroups() == null) {
312-
user.setGroups(new ArrayList<>());
313-
}
314-
user.getGroups().add(new Group(appName, groupName));
311+
user.addGroup(new Group(appName, groupName));
315312
updateUser(user, new ProviderRequest(null, false, null));
316313
if (group.getUsers() == null) {
317314
group.setUsers(new ArrayList<>());

‎sugoi-api-ldap-store-provider/src/main/java/fr/insee/sugoi/store/ldap/LdapReaderStore.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,13 @@ public Optional<User> getUser(String id) {
8989
if (user != null && user.getOrganization() != null) {
9090
user.setOrganization(getOrganization(user.getOrganization().getIdentifiant()).orElse(null));
9191
}
92+
// Add empty habilitations and groups if not already added
93+
if (user != null && user.getHabilitations() == null) {
94+
user.setHabilitations(new ArrayList<>());
95+
}
96+
if (user != null && user.getGroups() == null) {
97+
user.setGroups(new ArrayList<>());
98+
}
9299
return Optional.ofNullable(user);
93100
}
94101

‎sugoi-api-ldap-utils/src/main/java/fr/insee/sugoi/ldap/utils/LdapUtils.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -145,13 +145,12 @@ private static Modification computeMotificationFromNewvalue(
145145
.filter(value -> !value.isBlank())
146146
.toArray(String[]::new);
147147

148-
if (newValues.length == 0) {
149-
// If there are only blank or empty vales, this should be considered as
150-
// delete attribute (no value to set)
151-
return new Modification(ModificationType.DELETE, attributeName);
152-
} else {
153-
// Set new value(s)
154-
return new Modification(ModificationType.REPLACE, attributeName, newValues);
155-
}
148+
// If there are only blank or empty vales, this should be considered as
149+
// delete attribute (no value to set)
150+
// A replace Operation whith an empty values array is equivalent as a delete if
151+
// there are values in current state and is idempotent if already no values in
152+
// ldap
153+
154+
return new Modification(ModificationType.REPLACE, attributeName, newValues);
156155
}
157156
}

‎sugoi-api-ldap-utils/src/main/java/fr/insee/sugoi/ldap/utils/mapper/GenericLdapMapper.java‎

Lines changed: 61 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -123,25 +123,31 @@ private static <SugoiType> List<Attribute> mapObjectToLdapAttributes(
123123
if (mapToModify != null && mapToModify.containsKey(keyToModify)) {
124124
Object sugoiValue = mapToModify.get(keyToModify);
125125
if (sugoiValue != null) {
126-
attributes.addAll(
126+
Attribute mappedValue =
127127
transformSugoiToAttribute(
128128
mappingDefinition.getModelType(),
129129
mappingDefinition.getStoreName(),
130130
sugoiValue,
131-
config));
131+
config);
132+
if (mappedValue != null) {
133+
attributes.add(mappedValue);
134+
}
132135
}
133136
}
134137
} else {
135138
Field sugoiField = entity.getClass().getDeclaredField(mappingDefinition.getSugoiName());
136139
sugoiField.setAccessible(true);
137140
Object sugoiValue = sugoiField.get(entity);
138141
if (sugoiValue != null) {
139-
attributes.addAll(
142+
Attribute mappedValue =
140143
transformSugoiToAttribute(
141144
mappingDefinition.getModelType(),
142145
mappingDefinition.getStoreName(),
143146
sugoiValue,
144-
config));
147+
config);
148+
if (mappedValue != null) {
149+
attributes.add(mappedValue);
150+
}
145151
}
146152
}
147153
}
@@ -219,70 +225,76 @@ private static Object transformLdapAttributeToSugoiAttribute(
219225
}
220226

221227
@SuppressWarnings("unchecked")
222-
private static List<Attribute> transformSugoiToAttribute(
228+
private static Attribute transformSugoiToAttribute(
223229
ModelType type,
224230
String ldapAttributeName,
225231
Object sugoiValue,
226232
Map<RealmConfigKeys, String> config) {
227233
switch (type) {
228234
case STRING:
229-
return List.of(new Attribute(ldapAttributeName, (String) sugoiValue));
235+
return new Attribute(ldapAttributeName, (String) sugoiValue);
230236
case ORGANIZATION:
231-
return List.of(
232-
new Attribute(
233-
ldapAttributeName,
234-
String.format(
235-
"%s=%s,%s",
236-
// TODO should be a param
237-
"uid",
238-
//
239-
((Organization) sugoiValue).getIdentifiant(),
240-
config.get(GlobalKeysConfig.ORGANIZATION_SOURCE))));
237+
return new Attribute(
238+
ldapAttributeName,
239+
String.format(
240+
"%s=%s,%s",
241+
// TODO should be a param
242+
"uid",
243+
//
244+
((Organization) sugoiValue).getIdentifiant(),
245+
config.get(GlobalKeysConfig.ORGANIZATION_SOURCE)));
241246
case ADDRESS:
242247
if (((PostalAddress) sugoiValue).getId() != null
243248
&& config.get(GlobalKeysConfig.ADDRESS_SOURCE) != null) {
244-
return List.of(
245-
new Attribute(
246-
ldapAttributeName,
247-
String.format(
248-
"%s=%s,%s",
249-
// TODO should be a param
250-
"l",
251-
//
252-
((PostalAddress) sugoiValue).getId(),
253-
config.get(GlobalKeysConfig.ADDRESS_SOURCE))));
254-
} else return List.of();
249+
return new Attribute(
250+
ldapAttributeName,
251+
String.format(
252+
"%s=%s,%s",
253+
// TODO should be a param
254+
"l",
255+
//
256+
((PostalAddress) sugoiValue).getId(),
257+
config.get(GlobalKeysConfig.ADDRESS_SOURCE)));
258+
} else return null;
255259
case LIST_HABILITATION:
256-
return ((List<Habilitation>) sugoiValue)
257-
.stream()
258-
// Dont check contents (application nor role) for searching purpose
259-
.filter(habilitation -> habilitation.getId() != null)
260-
.map(habilitation -> new Attribute(ldapAttributeName, habilitation.getId()))
261-
.collect(Collectors.toList());
262-
case LIST_USER:
263-
return List.of();
260+
// Assume that if list is empty it's really to set an empty list (delete all
261+
// values)
262+
// Habilitations list can be set null if not needed
263+
return new Attribute(
264+
ldapAttributeName,
265+
((List<Habilitation>) sugoiValue)
266+
.stream()
267+
// Dont check contents (application nor role) for searching purpose
268+
.filter(habilitation -> habilitation.getId() != null)
269+
.map(habilitation -> habilitation.getId())
270+
.collect(Collectors.toList()));
264271
case LIST_GROUP:
265-
return ((List<Group>) sugoiValue)
266-
.stream()
267-
.map(
268-
group ->
269-
new Attribute(
270-
ldapAttributeName,
272+
// Assume that if list is empty it's really to set an empty list (delete all
273+
// values)
274+
// Groups list can be set null if not needed
275+
return new Attribute(
276+
ldapAttributeName,
277+
((List<Group>) sugoiValue)
278+
.stream()
279+
.map(
280+
group ->
271281
String.format(
272282
// TODO should be a param
273283
"cn=%s,",
274284
//
275-
group.getName())))
276-
.collect(Collectors.toList());
285+
group.getName()))
286+
.collect(Collectors.toList()));
277287
case LIST_STRING:
278-
return ((List<String>) sugoiValue)
279-
.stream()
280-
.map(value -> new Attribute(ldapAttributeName, value))
281-
.collect(Collectors.toList());
288+
// Assume that if list is empty it's really to set an empty list (delete all
289+
// values)
290+
// Lists in attributes map can be set as null if not needed
291+
return new Attribute(
292+
ldapAttributeName,
293+
((List<String>) sugoiValue).stream().map(value -> value).collect(Collectors.toList()));
294+
case LIST_USER:
282295
default:
283-
List.of();
296+
return null;
284297
}
285-
return List.of();
286298
}
287299

288300
private static <ReturnType> void putSugoiAttributeInEntityField(

‎sugoi-api-ldap-utils/src/test/java/fr/insee/sugoi/ldap/utils/mapper/GroupLdapMapperFromObjectTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,8 @@ public void emptyStringShouldBeDeletedWhenModification() {
126126
modifications.stream()
127127
.anyMatch(
128128
mod ->
129-
mod.getModificationType().equals(ModificationType.DELETE)
130-
&& mod.getAttributeName().equals("description")));
129+
mod.getModificationType().equals(ModificationType.REPLACE)
130+
&& mod.getAttributeName().equals("description")
131+
&& mod.getRawValues().length == 0));
131132
}
132133
}

‎sugoi-api-ldap-utils/src/test/java/fr/insee/sugoi/ldap/utils/mapper/UserLdapMapperFromObjectTest.java‎

Lines changed: 55 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,11 @@
1616
import static org.hamcrest.MatcherAssert.assertThat;
1717

1818
import com.unboundid.ldap.sdk.Attribute;
19+
import com.unboundid.ldap.sdk.Modification;
20+
import com.unboundid.ldap.sdk.ModificationType;
1921
import fr.insee.sugoi.core.configuration.GlobalKeysConfig;
2022
import fr.insee.sugoi.ldap.utils.config.LdapConfigKeys;
21-
import fr.insee.sugoi.model.Habilitation;
22-
import fr.insee.sugoi.model.Organization;
23-
import fr.insee.sugoi.model.PostalAddress;
24-
import fr.insee.sugoi.model.RealmConfigKeys;
25-
import fr.insee.sugoi.model.User;
23+
import fr.insee.sugoi.model.*;
2624
import fr.insee.sugoi.model.fixtures.StoreMappingFixture;
2725
import java.util.*;
2826
import org.junit.jupiter.api.BeforeEach;
@@ -172,15 +170,17 @@ public void getUserHabilitationsAttributeFromJavaObject() {
172170
.anyMatch(
173171
attribute ->
174172
attribute.getName().equals("inseeGroupeDefaut")
175-
&& attribute.getValue().equals("property_role_application")));
173+
&& Arrays.asList(attribute.getValues())
174+
.contains("property_role_application")));
176175

177176
assertThat(
178177
"Should have second habilitation in inseeGroupeDefault",
179178
mappedAttributes.stream()
180179
.anyMatch(
181180
attribute ->
182181
attribute.getName().equals("inseeGroupeDefaut")
183-
&& attribute.getValue().equals("property_role_application2")));
182+
&& Arrays.asList(attribute.getValues())
183+
.contains("property_role_application2")));
184184
}
185185

186186
@Test
@@ -229,13 +229,59 @@ public void getUserInseeRoleApplicatifFromJavaObject() {
229229
.anyMatch(
230230
attribute ->
231231
attribute.getName().equals("inseeRoleApplicatif")
232-
&& attribute.getValue().equals("toto")));
232+
&& Arrays.asList(attribute.getValues()).contains("toto")));
233233
assertThat(
234234
"Should have attribute inseeRoleApplicatif tata",
235235
mappedAttributes.stream()
236236
.anyMatch(
237237
attribute ->
238238
attribute.getName().equals("inseeRoleApplicatif")
239-
&& attribute.getValue().equals("tata")));
239+
&& Arrays.asList(attribute.getValues()).contains("tata")));
240+
}
241+
242+
@Test
243+
public void removeInseeRoleApplicatifIfEmptyListSet() {
244+
245+
List<String> inseeRoleApplicatif = new ArrayList<String>();
246+
user.addAttributes("insee_roles_applicatifs", inseeRoleApplicatif);
247+
List<Modification> modifications = userLdapMapper.createMods(user);
248+
249+
assertThat(
250+
"Should remove attribute inseeRoleApplicatif",
251+
modifications.stream()
252+
.anyMatch(
253+
mod ->
254+
mod.getModificationType().equals(ModificationType.REPLACE)
255+
&& mod.getAttributeName().equals("inseeRoleApplicatif")
256+
&& mod.getRawValues().length == 0));
257+
}
258+
259+
@Test
260+
public void dontRemoveInseeRoleApplicatifIfNoListSet() {
261+
262+
// when no explicit insee_roles_applicatifs attribute is set
263+
List<Modification> modifications = userLdapMapper.createMods(user);
264+
265+
assertThat(
266+
"Should not change attribute inseeRoleApplicatif",
267+
modifications.stream()
268+
.noneMatch(mod -> mod.getAttributeName().equals("inseeRoleApplicatif")));
269+
}
270+
271+
@Test
272+
public void removeAllHabilitationsIfEmptyList() {
273+
274+
// Default is a null habilitation list and means no changes on habilitation
275+
// An explicit empty habilitation list means remove all habilitations
276+
user.setHabilitations(new ArrayList<Habilitation>());
277+
List<Modification> modifications = userLdapMapper.createMods(user);
278+
279+
assertThat(
280+
"Should not change attribute inseeGroupeDefaut",
281+
modifications.stream()
282+
.anyMatch(
283+
mod ->
284+
mod.getAttributeName().equals("inseeGroupeDefaut")
285+
&& mod.getValues().length == 0));
240286
}
241287
}

‎sugoi-api-model/src/main/java/fr/insee/sugoi/model/User.java‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,10 @@ public class User implements Serializable {
2929
private byte[] certificate;
3030
private Organization organization;
3131

32-
private List<Group> groups = new ArrayList<>();
33-
private List<Habilitation> habilitations = new ArrayList<>();
32+
// Don't set a default empty list to distinguish case null (not provided) and empty
33+
private List<Group> groups;
34+
// Don't set a default empty list to distinguish case null (not provided) and empty
35+
private List<Habilitation> habilitations;
3436

3537
private PostalAddress address;
3638
private Map<String, Object> metadatas = new HashMap<>();
@@ -60,6 +62,9 @@ public List<Habilitation> getHabilitations() {
6062
}
6163

6264
public void addHabilitation(Habilitation habilitation) {
65+
if (habilitations == null) {
66+
habilitations = new ArrayList<>();
67+
}
6368
this.habilitations.add(habilitation);
6469
}
6570

@@ -111,6 +116,13 @@ public List<Group> getGroups() {
111116
return this.groups;
112117
}
113118

119+
public void addGroup(Group group) {
120+
if (groups == null) {
121+
groups = new ArrayList<>();
122+
}
123+
this.groups.add(group);
124+
}
125+
114126
public void setGroups(List<Group> groups) {
115127
this.groups = groups;
116128
}
@@ -156,6 +168,9 @@ public void setMetadatas(Map<String, Object> metadatas) {
156168
}
157169

158170
public void addGroups(Group group) {
171+
if (this.groups == null) {
172+
this.groups = new ArrayList<>();
173+
}
159174
this.groups.add(group);
160175
}
161176
}

0 commit comments

Comments
 (0)