Skip to content

Commit 2602c22

Browse files
rmartinchmlnarik
rmartinc
authored and
hmlnarik
committed
KEYCLOAK-4640: LDAP memberships are being replaced instead of being added or deleted
1 parent 996389d commit 2602c22

File tree

13 files changed

+592
-101
lines changed

13 files changed

+592
-101
lines changed

‎federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPUtils.java

+30-35
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.util.List;
3939
import java.util.Map;
4040
import java.util.Set;
41+
import javax.naming.directory.SearchControls;
4142

4243
/**
4344
* Allow to directly call some operations against LDAPIdentityStore.
@@ -167,30 +168,10 @@ public static LDAPObject createLDAPGroup(LDAPStorageProvider ldapProvider, Strin
167168
* @param memberChildAttrName used just if membershipType is UID. Usually 'uid'
168169
* @param ldapParent role or group
169170
* @param ldapChild usually user (or child group or child role)
170-
* @param sendLDAPUpdateRequest if true, the method will send LDAP update request too. Otherwise it will skip it
171171
*/
172-
public static void addMember(LDAPStorageProvider ldapProvider, MembershipType membershipType, String memberAttrName, String memberChildAttrName, LDAPObject ldapParent, LDAPObject ldapChild, boolean sendLDAPUpdateRequest) {
173-
174-
Set<String> memberships = getExistingMemberships(memberAttrName, ldapParent);
175-
176-
// Remove membership placeholder if present
177-
if (membershipType == MembershipType.DN) {
178-
for (String membership : memberships) {
179-
if (LDAPConstants.EMPTY_MEMBER_ATTRIBUTE_VALUE.equals(membership)) {
180-
memberships.remove(membership);
181-
break;
182-
}
183-
}
184-
}
185-
172+
public static void addMember(LDAPStorageProvider ldapProvider, MembershipType membershipType, String memberAttrName, String memberChildAttrName, LDAPObject ldapParent, LDAPObject ldapChild) {
186173
String membership = getMemberValueOfChildObject(ldapChild, membershipType, memberChildAttrName);
187-
188-
memberships.add(membership);
189-
ldapParent.setAttribute(memberAttrName, memberships);
190-
191-
if (sendLDAPUpdateRequest) {
192-
ldapProvider.getLdapIdentityStore().update(ldapParent);
193-
}
174+
ldapProvider.getLdapIdentityStore().addMemberToGroup(ldapParent.getDn().toString(), memberAttrName, membership);
194175
}
195176

196177
/**
@@ -204,29 +185,20 @@ public static void addMember(LDAPStorageProvider ldapProvider, MembershipType me
204185
* @param ldapChild usually user (or child group or child role)
205186
*/
206187
public static void deleteMember(LDAPStorageProvider ldapProvider, MembershipType membershipType, String memberAttrName, String memberChildAttrName, LDAPObject ldapParent, LDAPObject ldapChild) {
207-
Set<String> memberships = getExistingMemberships(memberAttrName, ldapParent);
208-
209188
String userMembership = getMemberValueOfChildObject(ldapChild, membershipType, memberChildAttrName);
210-
211-
memberships.remove(userMembership);
212-
213-
// Some membership placeholder needs to be always here as "member" is mandatory attribute on some LDAP servers. But not on active directory! (Placeholder, which not matches any real object is not allowed here)
214-
if (memberships.size() == 0 && membershipType== MembershipType.DN && !ldapProvider.getLdapIdentityStore().getConfig().isActiveDirectory()) {
215-
memberships.add(LDAPConstants.EMPTY_MEMBER_ATTRIBUTE_VALUE);
216-
}
217-
218-
ldapParent.setAttribute(memberAttrName, memberships);
219-
ldapProvider.getLdapIdentityStore().update(ldapParent);
189+
ldapProvider.getLdapIdentityStore().removeMemberFromGroup(ldapParent.getDn().toString(), memberAttrName, userMembership);
220190
}
221191

222192
/**
223193
* Return all existing memberships (values of attribute 'member' ) from the given ldapRole or ldapGroup
224194
*
195+
* @param ldapProvider The ldap provider
225196
* @param memberAttrName usually 'member'
226197
* @param ldapRole
227198
* @return
228199
*/
229-
public static Set<String> getExistingMemberships(String memberAttrName, LDAPObject ldapRole) {
200+
public static Set<String> getExistingMemberships(LDAPStorageProvider ldapProvider, String memberAttrName, LDAPObject ldapRole) {
201+
LDAPUtils.fillRangedAttribute(ldapProvider, ldapRole, memberAttrName);
230202
Set<String> memberships = ldapRole.getAttributeAsSet(memberAttrName);
231203
if (memberships == null) {
232204
memberships = new HashSet<>();
@@ -298,4 +270,27 @@ public static void validateCustomLdapFilter(String customFilter) throws Componen
298270
}
299271
}
300272
}
273+
274+
private static LDAPQuery createLdapQueryForRangeAttribute(LDAPStorageProvider ldapProvider, LDAPObject ldapObject, String name) {
275+
LDAPQuery q = new LDAPQuery(ldapProvider);
276+
q.setSearchDn(ldapObject.getDn().toString());
277+
q.setSearchScope(SearchControls.OBJECT_SCOPE);
278+
q.addReturningLdapAttribute(name + ";range=" + (ldapObject.getCurrentRange(name) + 1) + "-*");
279+
return q;
280+
}
281+
282+
/**
283+
* Performs iterative searches over an LDAPObject to return an attribute that is ranged.
284+
* @param ldapProvider The provider to use
285+
* @param ldapObject The current object with the ranged attribute not complete
286+
* @param name The attribute name
287+
*/
288+
public static void fillRangedAttribute(LDAPStorageProvider ldapProvider, LDAPObject ldapObject, String name) {
289+
LDAPObject newObject = ldapObject;
290+
while (!newObject.isRangeComplete(name)) {
291+
LDAPQuery q = createLdapQueryForRangeAttribute(ldapProvider, ldapObject, name);
292+
newObject = q.getFirstResult();
293+
ldapObject.populateRangedAttribute(newObject, name);
294+
}
295+
}
301296
}

‎federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LDAPObject.java

+34-1
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ public class LDAPObject {
4848
// Copy of "attributes" containing lower-cased keys
4949
private final Map<String, Set<String>> lowerCasedAttributes = new HashMap<>();
5050

51+
// range attributes are always read from 0 to max so just saving the top value
52+
private final Map<String, Integer> rangedAttributes = new HashMap<>();
5153

5254
public String getUuid() {
5355
return uuid;
@@ -123,6 +125,36 @@ public Set<String> getAttributeAsSet(String name) {
123125
return (values == null) ? null : new LinkedHashSet<>(values);
124126
}
125127

128+
public boolean isRangeComplete(String name) {
129+
return !rangedAttributes.containsKey(name);
130+
}
131+
132+
public int getCurrentRange(String name) {
133+
return rangedAttributes.get(name);
134+
}
135+
136+
public boolean isRangeCompleteForAllAttributes() {
137+
return rangedAttributes.isEmpty();
138+
}
139+
140+
public void addRangedAttribute(String name, int max) {
141+
Integer current = rangedAttributes.get(name);
142+
if (current == null || max > current) {
143+
rangedAttributes.put(name, max);
144+
}
145+
}
146+
147+
public void populateRangedAttribute(LDAPObject obj, String name) {
148+
Set<String> newValues = obj.getAttributes().get(name);
149+
if (newValues != null && attributes.containsKey(name)) {
150+
attributes.get(name).addAll(newValues);
151+
if (!obj.isRangeComplete(name)) {
152+
addRangedAttribute(name, obj.getCurrentRange(name));
153+
} else {
154+
rangedAttributes.remove(name);
155+
}
156+
}
157+
}
126158

127159
public Map<String, Set<String>> getAttributes() {
128160
return attributes;
@@ -152,6 +184,7 @@ public int hashCode() {
152184

153185
@Override
154186
public String toString() {
155-
return "LDAP Object [ dn: " + dn + " , uuid: " + uuid + ", attributes: " + attributes + ", readOnly attribute names: " + readOnlyAttributeNames + " ]";
187+
return "LDAP Object [ dn: " + dn + " , uuid: " + uuid + ", attributes: " + attributes +
188+
", readOnly attribute names: " + readOnlyAttributeNames + ", ranges: " + rangedAttributes + " ]";
156189
}
157190
}

‎federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/IdentityStore.java

+16
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,22 @@ public interface IdentityStore {
6565
*/
6666
void remove(LDAPObject ldapObject);
6767

68+
/**
69+
* Adds a member to a group.
70+
* @param groupDn The DN of the group object
71+
* @param memberAttrName The member attribute name
72+
* @param value The value (it can be uid or dn depending the group type)
73+
*/
74+
public void addMemberToGroup(String groupDn, String memberAttrName, String value);
75+
76+
/**
77+
* Removes a member from a group.
78+
* @param groupDn The DN of the group object
79+
* @param memberAttrName The member attribute name
80+
* @param value The value (it can be uid or dn depending the group type)
81+
*/
82+
public void removeMemberFromGroup(String groupDn, String memberAttrName, String value);
83+
6884
// Identity query
6985

7086
List<LDAPObject> fetchQueryResults(LDAPQuery LDAPQuery);

‎federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java

+61-1
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,11 @@
5454
import java.util.NoSuchElementException;
5555
import java.util.Set;
5656
import java.util.TreeSet;
57+
import java.util.regex.Matcher;
58+
import java.util.regex.Pattern;
59+
import javax.naming.directory.AttributeInUseException;
60+
import javax.naming.directory.NoSuchAttributeException;
61+
import javax.naming.directory.SchemaViolationException;
5762

5863
/**
5964
* An IdentityStore implementation backed by an LDAP directory
@@ -65,6 +70,7 @@
6570
public class LDAPIdentityStore implements IdentityStore {
6671

6772
private static final Logger logger = Logger.getLogger(LDAPIdentityStore.class);
73+
private static final Pattern rangePattern = Pattern.compile("([^;]+);range=([0-9]+)-([0-9]+|\\*)");
6874

6975
private final LDAPConfig config;
7076
private final LDAPOperationManager operationManager;
@@ -101,6 +107,44 @@ public void add(LDAPObject ldapObject) {
101107
}
102108
}
103109

110+
@Override
111+
public void addMemberToGroup(String groupDn, String memberAttrName, String value) {
112+
// do not check EMPTY_MEMBER_ATTRIBUTE_VALUE, we save one useless query
113+
// the value will be there forever for objectclasses that enforces the attribute as MUST
114+
BasicAttribute attr = new BasicAttribute(memberAttrName, value);
115+
ModificationItem item = new ModificationItem(DirContext.ADD_ATTRIBUTE, attr);
116+
try {
117+
this.operationManager.modifyAttributesNaming(groupDn, new ModificationItem[]{item}, null);
118+
} catch (AttributeInUseException e) {
119+
logger.debugf("Group %s already contains the member %s", groupDn, value);
120+
} catch (NamingException e) {
121+
throw new ModelException("Could not modify attribute for DN [" + groupDn + "]", e);
122+
}
123+
}
124+
125+
@Override
126+
public void removeMemberFromGroup(String groupDn, String memberAttrName, String value) {
127+
BasicAttribute attr = new BasicAttribute(memberAttrName, value);
128+
ModificationItem item = new ModificationItem(DirContext.REMOVE_ATTRIBUTE, attr);
129+
try {
130+
this.operationManager.modifyAttributesNaming(groupDn, new ModificationItem[]{item}, null);
131+
} catch (NoSuchAttributeException e) {
132+
logger.debugf("Group %s does not contain the member %s", groupDn, value);
133+
} catch (SchemaViolationException e) {
134+
// schema violation removing one member => add the empty attribute, it cannot be other thing
135+
logger.infof("Schema violation in group %s removing member %s. Trying adding empty member attribute.", groupDn, value);
136+
try {
137+
this.operationManager.modifyAttributesNaming(groupDn,
138+
new ModificationItem[]{item, new ModificationItem(DirContext.ADD_ATTRIBUTE, new BasicAttribute(memberAttrName, LDAPConstants.EMPTY_MEMBER_ATTRIBUTE_VALUE))},
139+
null);
140+
} catch (NamingException ex) {
141+
throw new ModelException("Could not modify attribute for DN [" + groupDn + "]", ex);
142+
}
143+
} catch (NamingException e) {
144+
throw new ModelException("Could not modify attribute for DN [" + groupDn + "]", e);
145+
}
146+
}
147+
104148
@Override
105149
public void update(LDAPObject ldapObject) {
106150
checkRename(ldapObject);
@@ -199,7 +243,8 @@ public List<LDAPObject> fetchQueryResults(LDAPQuery identityQuery) {
199243
}
200244

201245
for (SearchResult result : search) {
202-
if (!result.getNameInNamespace().equalsIgnoreCase(baseDN)) {
246+
// don't add the branch in subtree search
247+
if (identityQuery.getSearchScope() != SearchControls.SUBTREE_SCOPE || !result.getNameInNamespace().equalsIgnoreCase(baseDN)) {
203248
results.add(populateAttributedType(result, identityQuery));
204249
}
205250
}
@@ -351,6 +396,21 @@ private LDAPObject populateAttributedType(SearchResult searchResult, LDAPQuery l
351396

352397
String ldapAttributeName = ldapAttribute.getID();
353398

399+
// check for ranged attribute
400+
Matcher m = rangePattern.matcher(ldapAttributeName);
401+
if (m.matches()) {
402+
ldapAttributeName = m.group(1);
403+
// range=X-* means all the attributes returned
404+
if (!m.group(3).equals("*")) {
405+
try {
406+
int max = Integer.parseInt(m.group(3));
407+
ldapObject.addRangedAttribute(ldapAttributeName, max);
408+
} catch (NumberFormatException e) {
409+
logger.warnf("Invalid ranged expresion for attribute: %s", m.group(0));
410+
}
411+
}
412+
}
413+
354414
if (ldapAttributeName.equalsIgnoreCase(getConfig().getUuidLDAPAttributeName())) {
355415
Object uuidValue = ldapAttribute.get();
356416
ldapObject.setUuid(this.operationManager.decodeEntryUUID(uuidValue));

‎federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPOperationManager.java

+34-32
Original file line numberDiff line numberDiff line change
@@ -528,50 +528,52 @@ public void authenticate(String dn, String password) throws AuthenticationExcept
528528
}
529529
}
530530

531-
public void modifyAttributes(final String dn, final ModificationItem[] mods, LDAPOperationDecorator decorator) {
532-
try {
533-
if (logger.isTraceEnabled()) {
534-
logger.tracef("Modifying attributes for entry [%s]: [", dn);
535-
536-
for (ModificationItem item : mods) {
537-
Object values;
531+
public void modifyAttributesNaming(final String dn, final ModificationItem[] mods, LDAPOperationDecorator decorator) throws NamingException {
532+
if (logger.isTraceEnabled()) {
533+
logger.tracef("Modifying attributes for entry [%s]: [", dn);
538534

539-
if (item.getAttribute().size() > 0) {
540-
values = item.getAttribute().get();
541-
} else {
542-
values = "No values";
543-
}
535+
for (ModificationItem item : mods) {
536+
Object values;
544537

545-
String attrName = item.getAttribute().getID().toUpperCase();
546-
if (attrName.contains("PASSWORD") || attrName.contains("UNICODEPWD")) {
547-
values = "********************";
548-
}
538+
if (item.getAttribute().size() > 0) {
539+
values = item.getAttribute().get();
540+
} else {
541+
values = "No values";
542+
}
549543

550-
logger.tracef(" Op [%s]: %s = %s", item.getModificationOp(), item.getAttribute().getID(), values);
544+
String attrName = item.getAttribute().getID().toUpperCase();
545+
if (attrName.contains("PASSWORD") || attrName.contains("UNICODEPWD")) {
546+
values = "********************";
551547
}
552548

553-
logger.tracef("]");
549+
logger.tracef(" Op [%s]: %s = %s", item.getModificationOp(), item.getAttribute().getID(), values);
554550
}
555551

556-
execute(new LdapOperation<Void>() {
552+
logger.tracef("]");
553+
}
557554

558-
@Override
559-
public Void execute(LdapContext context) throws NamingException {
560-
context.modifyAttributes(new LdapName(dn), mods);
561-
return null;
562-
}
555+
execute(new LdapOperation<Void>() {
563556

557+
@Override
558+
public Void execute(LdapContext context) throws NamingException {
559+
context.modifyAttributes(new LdapName(dn), mods);
560+
return null;
561+
}
564562

565-
@Override
566-
public String toString() {
567-
return new StringBuilder("LdapOperation: modify\n")
568-
.append(" dn: ").append(dn).append("\n")
569-
.append(" modificationsSize: ").append(mods.length)
570-
.toString();
571-
}
563+
@Override
564+
public String toString() {
565+
return new StringBuilder("LdapOperation: modify\n")
566+
.append(" dn: ").append(dn).append("\n")
567+
.append(" modificationsSize: ").append(mods.length)
568+
.toString();
569+
}
572570

571+
}, null, decorator);
572+
}
573573

574-
}, null, decorator);
574+
public void modifyAttributes(final String dn, final ModificationItem[] mods, LDAPOperationDecorator decorator) {
575+
try {
576+
modifyAttributesNaming(dn, mods, decorator);
575577
} catch (NamingException e) {
576578
throw new ModelException("Could not modify attribute for DN [" + dn + "]", e);
577579
}

‎federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/MembershipType.java

+5-5
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,12 @@ public enum MembershipType {
5050
@Override
5151
public Set<LDAPDn> getLDAPSubgroups(GroupLDAPStorageMapper groupMapper, LDAPObject ldapGroup) {
5252
CommonLDAPGroupMapperConfig config = groupMapper.getConfig();
53-
return getLDAPMembersWithParent(ldapGroup, config.getMembershipLdapAttribute(), LDAPDn.fromString(config.getLDAPGroupsDn()));
53+
return getLDAPMembersWithParent(groupMapper.getLdapProvider(), ldapGroup, config.getMembershipLdapAttribute(), LDAPDn.fromString(config.getLDAPGroupsDn()));
5454
}
5555

5656
// Get just those members of specified group, which are descendants of "requiredParentDn"
57-
protected Set<LDAPDn> getLDAPMembersWithParent(LDAPObject ldapGroup, String membershipLdapAttribute, LDAPDn requiredParentDn) {
58-
Set<String> allMemberships = LDAPUtils.getExistingMemberships(membershipLdapAttribute, ldapGroup);
57+
protected Set<LDAPDn> getLDAPMembersWithParent(LDAPStorageProvider ldapProvider, LDAPObject ldapGroup, String membershipLdapAttribute, LDAPDn requiredParentDn) {
58+
Set<String> allMemberships = LDAPUtils.getExistingMemberships(ldapProvider, membershipLdapAttribute, ldapGroup);
5959

6060
// Filter and keep just descendants of requiredParentDn
6161
Set<LDAPDn> result = new HashSet<>();
@@ -74,7 +74,7 @@ public List<UserModel> getGroupMembers(RealmModel realm, GroupLDAPStorageMapper
7474
CommonLDAPGroupMapperConfig config = groupMapper.getConfig();
7575

7676
LDAPDn usersDn = LDAPDn.fromString(ldapProvider.getLdapIdentityStore().getConfig().getUsersDn());
77-
Set<LDAPDn> userDns = getLDAPMembersWithParent(ldapGroup, config.getMembershipLdapAttribute(), usersDn);
77+
Set<LDAPDn> userDns = getLDAPMembersWithParent(ldapProvider, ldapGroup, config.getMembershipLdapAttribute(), usersDn);
7878

7979
if (userDns == null) {
8080
return Collections.emptyList();
@@ -139,7 +139,7 @@ public List<UserModel> getGroupMembers(RealmModel realm, GroupLDAPStorageMapper
139139
LDAPConfig ldapConfig = ldapProvider.getLdapIdentityStore().getConfig();
140140

141141
String memberAttrName = groupMapper.getConfig().getMembershipLdapAttribute();
142-
Set<String> memberUids = LDAPUtils.getExistingMemberships(memberAttrName, ldapGroup);
142+
Set<String> memberUids = LDAPUtils.getExistingMemberships(ldapProvider, memberAttrName, ldapGroup);
143143

144144
if (memberUids == null || memberUids.size() <= firstResult) {
145145
return Collections.emptyList();

0 commit comments

Comments
 (0)