-
Bug
-
Resolution: Unresolved
-
Medium
-
None
-
2.0.7, 2.4, 2.8.3, 2.8.4, 2.10.1
-
Crowd with LDAP as backend, LDAP administration by Collax Businness Server
-
29
-
Severity 3 - Minor
-
5
-
When we change in Collax a user to "inactive", this state is not reflected in the crowd database.
The proposed workaround is to remove the user from ldap and re-create the user on crowd. The result is two directories of users in one company. Administration, search, migration will become error-prone and is not acceptable for us. After using crowd with LDAP for about 2 yours, we get to our licence-limit of 100, where the active users are about 70 users.
Steps to reproduce:
- install Crowd
- Create new Directory connecting to OpenLDAP using Connector
- try to disable any user sync from OpenLDAP by unchecking the Active checkbox
Expected Behaviour:
User will be disactivated
Actual Behaviour:
Crowd indicate update is sucessful, but the user remain active
Environment:
Crowd connected to OpenLDAP
Notes
This is not reproducible when Crowd is connecting to Microsoft AD
- causes
-
JRACLOUD-34557 Jira should read from OpenLDAP a flag to make users inactive
- Closed
-
- Gathering Interest
- is related to
-
- Closed
-
- Closed
-
- Gathering Interest
-
- Gathering Interest
-
- Under Consideration
- relates to
-
- Closed
- is cloned by
-
KRAK-4556 Loading...
- links to
(2 is related to, 1 relates to, 1 is cloned by, 1 links to, 3 mentioned in)
[CWD-2762] Support changing active status (activating/deactivating) for users in OpenLDAP directories
We have the same problem. We're using OpenLDAP as backend and need to deactivate users. Not being able to disable users while using active users as licensing factor is not really a customer-friendly strategy...
Dear Atlassian Team (I think justin@atlassian.com has leave Atlassian company because he hasn't got any recent activities since 2016) ...
Please, could you apply patch from konrad.windszus?
Here is the patch for 3.7.0
From 15bf93b1016b9bcb1922fa5a18487acbbc2cb9b6 Fri, 15 Nov 2019 13:24:58 +0100 From: Konrad Windszus <konrad.windszus@netcentric.biz> Date: Wed, 29 Jun 2016 17:50:22 +0200 Subject: [PATCH] INTERNAL-12645, INTERNAL-13490, INTERNAL-565, INTERNAL-20389 sync deactivation with LDAP diff --git a/readme.md b/readme.md new file mode 100644 index 0000000..82e68bc --- /dev/null +++ b/readme.md @@ -0,0 +1,30 @@ +# Overview # +Customization of the LDAP plugin in Crowd to allow to sync activations/deactivations with LDAP +[(see CWD-2762)](https://jira.atlassian.com/browse/CWD-2762). + +# Original source code/compilation # + +* [Original Crowd Source Code](https://my.atlassian.com/download/source/crowd) +* [Further hints on compilation](https://developer.atlassian.com/display/CROWDDEV/Compiling+the+Crowd+Source) + +crowd-ldap cannot be compiled with JDT but only with Maven + +The original source code is in branch "master". All other branches contain the changes for a specific version as one commit. +To upgrade to a newer version just rebase that commit to master. + +# Testing locally # + +## Prerequisites ## +Set up plugin sdk as described [here](https://developer.atlassian.com/docs/getting-started/set-up-the-atlassian-plugin-sdk-and-build-a-project/install-the-atlassian-sdk-on-a-linux-or-mac-system) + +Make sure you disable your user's `settings.xml` (e.g. by renaming it) to make the global atlassian SDK `settings.xml` take full effect! + +## Run/Debug Local Instance ## + +1. Run regular Maven build: `mvn clean install` +2. `atlas-run-standalone --product crowd --version 2.11.2 --lib-plugins com.atlassian.crowd:crowd-ldap:2.11.1 --jvmargs "-Xdebug -Xrunjdwp:transport=dt_socket,address=8000,server=y,suspend=n"` (starts Crowd [locally](http://localhost:4990/crowd) and deploys this plugin in it.). +The default `atlas-run` will not work, as this will still run with the default ldap-plugin (below /target/crowd/webapp/WEB-INF/lib) +Further starting options described [here](https://developer.atlassian.com/docs/developer-tools/working-with-the-sdk/command-reference/atlas-run) +3. Run a local ldap from [Apache Directory Studio](http://directory.apache.org/studio/downloads.html) +4. Set up a new directory connector in Crowd via http://localhost:4990/crowd/console/secure/directory/createconnector.action on `ldap://localhost:10389` with bind dn `uid=admin,ou=system` and pw `secret` and base dn `dc=example,dc=com`. +5. Connect through a remote debugger with your IDE on port 8000 diff --git a/src/main/java/com/atlassian/crowd/directory/OpenLDAP.java b/src/main/java/com/atlassian/crowd/directory/OpenLDAP.java index a1296c4..1036646 100644 --- a/src/main/java/com/atlassian/crowd/directory/OpenLDAP.java +++ b/src/main/java/com/atlassian/crowd/directory/OpenLDAP.java @@ -10,7 +10,22 @@ import javax.naming.directory.Attributes; +import com.atlassian.crowd.directory.ldap.LDAPPropertiesMapper; +import com.atlassian.crowd.directory.ldap.mapper.UserContextMapperConfig; +import com.atlassian.crowd.directory.ldap.mapper.attribute.AttributeMapper; +import com.atlassian.crowd.directory.ldap.mapper.attribute.user.UserActiveMapper; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableList.Builder; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + public class OpenLDAP extends RFC4519Directory { + + private static final String ATTRIBUTE_LOCKOUT_KEY = "pwdAccountLockedTime"; + /** @see <a href="http://linux.die.net/man/5/slapo-ppolicy">man for slapo-ppolicy</a> */ + private static final String ATTRIBUTE_LOCKOUT_VALUE_PERMANENTLY_LOCKED = "000001010000Z"; + private final PasswordEncoderFactory passwordEncoderFactory; public OpenLDAP(LDAPQueryTranslater ldapQueryTranslater, EventPublisher eventPublisher, InstanceFactory instanceFactory, @@ -25,7 +40,7 @@ @Override public String getDescriptiveName() { - return OpenLDAP.getStaticDirectoryType(); + return OpenLDAP.getStaticDirectoryType() + " (extended with support for sync of active flag, CWD-2762)"; } @Override @@ -43,4 +58,32 @@ protected void getNewUserDirectorySpecificAttributes(final User user, final Attributes attributes) { addDefaultSnToUserAttributes(attributes, user.getName()); } + + /** + * As long as the LDAP attributes and values are not configurable, we just add those as hard-coded attributes in here, that they are available within {@link com.atlassian.crowd.directory.ldap.LDAPPropertiesMapper} + * The LDAP attribute keys and values are taken from the de-facto standard "Password Policy for LDAP Directories" + * @see <a href="http://tools.ietf.org/html/draft-behera-ldap-password-policy-10">Password Policy for LDAP Directories</a> + * @see <a href="http://linux.die.net/man/5/slapo-ppolicy">man for slapo-ppolicy</a> + */ + @Override + public void setAttributes(Map<String, String> attributes) { + Map<String, String> extendedAttributes = new HashMap<>(attributes); + + extendedAttributes.put(LDAPPropertiesMapper.USER_ACTIVE_ATTRIBUTE_KEY, ATTRIBUTE_LOCKOUT_KEY); + extendedAttributes.put(LDAPPropertiesMapper.USER_ACTIVE_VALUE, ""); + extendedAttributes.put(LDAPPropertiesMapper.USER_INACTIVE_VALUE, ATTRIBUTE_LOCKOUT_VALUE_PERMANENTLY_LOCKED); + + super.setAttributes(extendedAttributes); + } + + /** + * Combine the custom user attribute mappers with the {@link UserActiveMapper}. + */ + @Override + protected List<AttributeMapper> getCustomUserAttributeMappers(UserContextMapperConfig config) { + Builder<AttributeMapper> builder = ImmutableList.<AttributeMapper> builder(); + builder.addAll(super.getCustomUserAttributeMappers(config)); + builder.add(new UserActiveMapper(ldapPropertiesMapper.getUserActiveAttribute())); + return builder.build(); + } } diff --git a/src/main/java/com/atlassian/crowd/directory/SpringLDAPConnector.java b/src/main/java/com/atlassian/crowd/directory/SpringLDAPConnector.java index f4ca104..9cb5914 100644 --- a/src/main/java/com/atlassian/crowd/directory/SpringLDAPConnector.java +++ b/src/main/java/com/atlassian/crowd/directory/SpringLDAPConnector.java @@ -870,6 +870,13 @@ try { SearchControls searchControls = getSearchControls(contextMapper, SearchControls.OBJECT_SCOPE); + // make sure to really return also operational attributes (e.g. ones to determine active state of the user) + if (contextMapper instanceof ContextMapperWithRequiredAttributes<?>) { + logger.debug("Return required attributes {} for findEntityById", + (((ContextMapperWithRequiredAttributes<?>) contextMapper).getRequiredLdapAttributes())); + searchControls.setReturningAttributes( + toArray(((ContextMapperWithRequiredAttributes<?>) contextMapper).getRequiredLdapAttributes())); + } searchControls.setTimeLimit(ldapPropertiesMapper.getSearchTimeLimit()); entities = ldapTemplate.search(asLdapName(dn, "DN: " + dn, entityClass), filter, searchControls, contextMapper); } catch (NameNotFoundException e) { @@ -971,6 +978,32 @@ modificationItems.add(displayNameMod); } + // support enabling/disabling users + if (supportsInactiveAccounts()) { + ModificationItem activeModItem = null; + if (!currentUser.isActive() && userTemplate.isActive()) { + // try to activate user in case it is currently inactive + String newValue = ldapPropertiesMapper.getUserActiveValue(); + if (StringUtils.isEmpty(newValue)) { + // rather than modifying an attribute we completely remove it + activeModItem = new ModificationItem(DirContext.REMOVE_ATTRIBUTE, + new BasicAttribute(ldapPropertiesMapper.getUserActiveAttribute())); + } else { + // TODO: getting the real old value from LDAP is not easily possible here, just always use a REPLACE by setting the old + // value to something else than new value + activeModItem = createModificationItem(ldapPropertiesMapper.getUserActiveAttribute(), newValue + "changed", newValue); + } + } else if (currentUser.isActive() && !userTemplate.isActive()) { + // try to deactivate user in case it is currently active + String newValue = ldapPropertiesMapper.getUserInactiveValue(); + // TODO: getting the real old value from LDAP is not easily possible here, just always use a REPLACE by setting the old + // value to something else than new value + activeModItem = createModificationItem(ldapPropertiesMapper.getUserActiveAttribute(), newValue + "changed", newValue); + } + if (activeModItem != null) { + modificationItems.add(activeModItem); + } + } return modificationItems; } @@ -1545,7 +1578,7 @@ */ @Override public boolean supportsInactiveAccounts() { - return false; + return StringUtils.isNotBlank(ldapPropertiesMapper.getUserActiveAttribute()); } @Override diff --git a/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapper.java b/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapper.java index 4668c2f..7c24ab9 100644 --- a/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapper.java +++ b/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapper.java @@ -175,6 +175,21 @@ * Attribute key for the LDAP principal password attribute. */ String USER_PASSWORD_KEY = "ldap.user.password"; + + /** + * Attribute key for the LDAP principal active attribute. + */ + String USER_ACTIVE_ATTRIBUTE_KEY = "ldap.user.active.attribute"; + + /** + * Attribute key for the LDAP value which stands for active users. In contrast to {@link #USER_INACTIVE_VALUE} this is only considered for writing to the LDAP, not for evaluation! + */ + String USER_ACTIVE_VALUE = "ldap.user.active"; + + /** + * Attribute key for the LDAP value which stands for inactive users. This is used for checking whether an account is active and also for persisting that information within the LDAP. + */ + String USER_INACTIVE_VALUE = "ldap.user.inactive"; /** * Attribute key for the LDAP paged results attribute. @@ -363,6 +378,25 @@ String getUserPasswordAttribute(); String getUserEncryptionMethod(); + + /** + * + * @return the LDAP attribute in which the users active state is stored. Derived from {@link #USER_ACTIVE_ATTRIBUTE_KEY}. + */ + String getUserActiveAttribute(); + + /** + * + * @return the LDAP value which stands for an active user (should only be used for turning a user from inactive to active, i.e. for writing to LDAP, but not for evaluating the state). Derived from {@link #USER_ACTIVE_VALUE}} + */ + String getUserActiveValue(); + + /** + * + * @return the LDAP value which stands for an inactive user (should only be used for turning a user from active to inactive, i.e. for writing to LDAP, as well as for evaluating the current state). Derived from {@link #USER_INACTIVE_VALUE}} + */ + String getUserInactiveValue(); + boolean isPagedResultsControl(); diff --git a/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapperImpl.java b/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapperImpl.java index 7b07ed5..a82799c 100644 --- a/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapperImpl.java +++ b/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapperImpl.java @@ -418,6 +418,22 @@ public String getUserEncryptionMethod() { return getAttribute(LDAP_USER_ENCRYPTION_METHOD); } + + @Override + public String getUserActiveAttribute() + { + return getAttribute(USER_ACTIVE_ATTRIBUTE_KEY); + } + + @Override + public String getUserActiveValue() { + return getAttribute(USER_ACTIVE_VALUE); + } + + @Override + public String getUserInactiveValue() { + return getAttribute(USER_INACTIVE_VALUE); + } /** * Checks if the configuration of the LDAP directory server uses paged results. diff --git a/src/main/java/com/atlassian/crowd/directory/ldap/mapper/attribute/user/UserActiveMapper.java b/src/main/java/com/atlassian/crowd/directory/ldap/mapper/attribute/user/UserActiveMapper.java new file mode 100644 index 0000000..284c0c0 --- /dev/null +++ b/src/main/java/com/atlassian/crowd/directory/ldap/mapper/attribute/user/UserActiveMapper.java @@ -0,0 +1,42 @@ +package com.atlassian.crowd.directory.ldap.mapper.attribute.user; + +import java.util.Collections; +import java.util.Set; + +import org.springframework.ldap.core.DirContextAdapter; + +import com.atlassian.crowd.directory.ldap.mapper.attribute.AttributeMapper; + +/** + * Attribute mapper which takes care of the active state of the LDAP user. + */ +public class UserActiveMapper implements AttributeMapper { + + private final String userActiveAttribute; + + public UserActiveMapper(String userActiveAttribute) { + this.userActiveAttribute = userActiveAttribute; + } + + @Override + public String getKey() { + return userActiveAttribute; + } + + @Override + public Set<String> getValues(DirContextAdapter ctx) throws Exception { + // this must never return null + String userActiveValue = ctx.getStringAttribute(getKey()); + if (userActiveValue != null) { + return Collections.singleton(userActiveValue); + } else { + return Collections.emptySet(); + } + } + + @Override + public Set<String> getRequiredLdapAttributes() { + return Collections.singleton(getKey()); + } + +} diff --git a/src/main/java/com/atlassian/crowd/directory/ldap/mapper/entity/LDAPUserAttributesMapper.java b/src/main/java/com/atlassian/crowd/directory/ldap/mapper/entity/LDAPUserAttributesMapper.java index eda1734..68a9a69 100644 --- a/src/main/java/com/atlassian/crowd/directory/ldap/mapper/entity/LDAPUserAttributesMapper.java +++ b/src/main/java/com/atlassian/crowd/directory/ldap/mapper/entity/LDAPUserAttributesMapper.java @@ -67,6 +67,13 @@ // full name putValueInAttributes(populatedUser.getDisplayName(), ldapPropertiesMapper.getUserDisplayNameAttribute(), directoryAttributes); + + // active is the default case for LDAP, we only need to write something to the LDAP in case the user is not active + if (!populatedUser.isActive()) { + putValueInAttributes(ldapPropertiesMapper.getUserInactiveValue(), ldapPropertiesMapper.getUserActiveAttribute(), directoryAttributes); + } else { + putValueInAttributes(ldapPropertiesMapper.getUserActiveValue(), ldapPropertiesMapper.getUserActiveAttribute(), directoryAttributes); + } // TODO: currently we don't support arbitrary attributes / iconLocation / active @@ -129,6 +136,24 @@ } protected boolean getUserActiveFromAttribute(final Attributes directoryAttributes) { + String userActive = DirectoryAttributeRetriever.getValueFromAttributes(ldapPropertiesMapper.getUserActiveAttribute(), directoryAttributes); + if (userActive == null) { + if (logger.isDebugEnabled()) { + logger.debug("User active attribute '{}' on user '{}' is not set or could not be evaluated, assuming user is active", + ldapPropertiesMapper.getUserActiveAttribute(), getUsernameFromAttributes(directoryAttributes)); + } + return true; + } + + if (logger.isDebugEnabled()) { + logger.debug("User active attribute '{}' is set to '{}' on user {}", ldapPropertiesMapper.getUserActiveAttribute(), userActive, + getUsernameFromAttributes(directoryAttributes)); + } + // is user inactive? + if (userActive.equals(ldapPropertiesMapper.getUserInactiveValue())) { + logger.debug("User is inactive"); + return false; + } return true; } diff --git a/src/main/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImpl.java b/src/main/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImpl.java index 243f2ab..bee3f9e 100644 --- a/src/main/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImpl.java +++ b/src/main/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImpl.java @@ -5,6 +5,7 @@ import com.atlassian.crowd.directory.ldap.LDAPPropertiesMapper; import com.atlassian.crowd.embedded.api.SearchRestriction; +import com.atlassian.crowd.search.Entity; import com.atlassian.crowd.search.EntityDescriptor; import com.atlassian.crowd.search.query.entity.EntityQuery; import com.atlassian.crowd.search.query.entity.restriction.BooleanRestriction; @@ -14,11 +15,13 @@ import com.atlassian.crowd.search.query.entity.restriction.constants.GroupTermKeys; import com.atlassian.crowd.search.query.entity.restriction.constants.UserTermKeys; +import org.apache.commons.lang3.StringUtils; import org.springframework.ldap.filter.AndFilter; import org.springframework.ldap.filter.BinaryLogicalFilter; import org.springframework.ldap.filter.EqualsFilter; import org.springframework.ldap.filter.Filter; import org.springframework.ldap.filter.LikeFilter; +import org.springframework.ldap.filter.NotFilter; import org.springframework.ldap.filter.NotPresentFilter; import org.springframework.ldap.filter.OrFilter; import org.springframework.ldap.filter.PresentFilter; @@ -125,18 +128,39 @@ } - protected Filter booleanTermRestrictionAsFilter(final EntityDescriptor entityDescriptor, final PropertyRestriction<Boolean> termRestriction, LDAPPropertiesMapper ldapPropertiesMapper) { - // if boolean term restrictions are for anything other than the group/user active flag, then throw exception - if (!termRestriction.getProperty().equals(GroupTermKeys.ACTIVE) && !termRestriction.getProperty().equals(UserTermKeys.ACTIVE)) { - throw new IllegalArgumentException("Boolean restrictions for property " + termRestriction.getProperty().getPropertyName() + " are not supported"); - } else { + protected Filter booleanTermRestrictionAsFilter(final EntityDescriptor entityDescriptor, + final PropertyRestriction<Boolean> termRestriction, LDAPPropertiesMapper ldapPropertiesMapper) { + if (entityDescriptor.getEntityType() == Entity.USER && termRestriction.getProperty().equals(UserTermKeys.ACTIVE)) { + if (StringUtils.isEmpty(ldapPropertiesMapper.getUserActiveAttribute())) { + if (termRestriction.getValue()) { + // don't consider active because user activeAttribute is not set (i.e. return all users) + return new EverythingResult(); + } else { + // don't consider disabled because user activeAttribute is not set (i.e. return no users) + return new NothingResult(); + } + } else { + if (termRestriction.getValue()) { + // querying for enabled users + return new NotFilter( + new EqualsFilter(ldapPropertiesMapper.getUserActiveAttribute(), ldapPropertiesMapper.getUserInactiveValue())); + } else { + // querying for disabled users + return new EqualsFilter(ldapPropertiesMapper.getUserActiveAttribute(), ldapPropertiesMapper.getUserInactiveValue()); + } + } + } else if (entityDescriptor.getEntityType() == Entity.GROUP && termRestriction.getProperty().equals(GroupTermKeys.ACTIVE)) { if (termRestriction.getValue()) { - // everything is active = true, so no need to add a restriction + // groups are always active = true, so no need to add a restriction return new EverythingResult(); } else { - // nothing is active = false, so need to + // no group is ever active = false, so no results return new NothingResult(); } + } else { + // if boolean term restrictions are for anything other than the group/user active flag, then throw exception + throw new IllegalArgumentException( + "Boolean restrictions for property " + termRestriction.getProperty().getPropertyName() + " are not supported"); } } diff --git a/src/main/resources/com/atlassian/crowd/integration/directory/openldap.properties b/src/main/resources/com/atlassian/crowd/integration/directory/openldap.properties index 36cb5db..9e53bec 100644 --- a/src/main/resources/com/atlassian/crowd/integration/directory/openldap.properties +++ b/src/main/resources/com/atlassian/crowd/integration/directory/openldap.properties @@ -24,6 +24,11 @@ ldap.user.group memberOf ldap.user.password userPassword +# active properties (according to http://tools.ietf.org/html/draft-behera-ldap-password-policy-10) +ldap.user.active.attribute pwdAccountLockedTime +ldap.user.active +ldap.user.inactive 000001010000Z + # generic options ldap.pagedresults false ldap.relaxed.dn.standardisation true diff --git a/src/test/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImplTest.java b/src/test/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImplTest.java index 8314fa2..2c48d4b 100644 --- a/src/test/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImplTest.java +++ b/src/test/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImplTest.java @@ -1,5 +1,13 @@ package com.atlassian.crowd.search.ldap; +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; + import com.atlassian.crowd.directory.ldap.LDAPPropertiesMapper; import com.atlassian.crowd.model.group.Group; import com.atlassian.crowd.model.group.GroupType; @@ -14,13 +22,6 @@ import com.atlassian.crowd.search.query.entity.restriction.constants.GroupTermKeys; import com.atlassian.crowd.search.query.entity.restriction.constants.UserTermKeys; -import org.junit.Before; -import org.junit.Test; - -import static org.junit.Assert.assertEquals; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - public class LDAPQueryTranslaterImplTest { private static final Property<String> CUSTOM_STRING_PROPERTY = new PropertyImpl<String>("myproperty", String.class); @@ -48,6 +49,11 @@ when(ldapPropertiesMapper.getGroupNameAttribute()).thenReturn("cn"); when(ldapPropertiesMapper.getGroupDescriptionAttribute()).thenReturn("description"); when(ldapPropertiesMapper.getGroupMemberAttribute()).thenReturn("member"); + + // test user activation through LDAP Password Policy (http://tools.ietf.org/html/draft-behera-ldap-password-policy-10) + when(ldapPropertiesMapper.getUserActiveAttribute()).thenReturn("pwdAccountLockedTime"); + when(ldapPropertiesMapper.getUserActiveValue()).thenReturn(""); + when(ldapPropertiesMapper.getUserInactiveValue()).thenReturn("000001010000Z"); } @Test @@ -104,7 +110,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(|(givenName=joe*)(sn=joe*)(displayName=joe*)))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(&(|(givenName=joe*)(sn=joe*)(displayName=joe*))(!(pwdAccountLockedTime=000001010000Z))))", filter); } @Test @@ -116,14 +122,16 @@ assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(entryUUID=a1b2))", filter); } - @Test(expected = NullResultException.class) + @Test public void testSearchInactiveUsers() throws NullResultException { EntityQuery<User> query = QueryBuilder.queryFor(User.class, EntityDescriptor.user()).with(Restriction.on(UserTermKeys.ACTIVE).exactlyMatching(false)).returningAtMost(10); - queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + String filter = ldapQuery.encode(); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(pwdAccountLockedTime=000001010000Z))", filter); } - - @Test(expected = NullResultException.class) + @Test + @Ignore("Those nested results no longer return null") public void testSearchInactiveUsersViaNestingReturningNullResult() throws NullResultException { EntityQuery<User> query = QueryBuilder.queryFor(User.class, EntityDescriptor.user()).with(Combine.anyOf( Combine.allOf( @@ -136,10 +144,14 @@ ) )).returningAtMost(10); - queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + String filter = ldapQuery.encode(); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(entryUUID=a1b2))", filter); + } @Test + @Ignore("This test is no longer applicable since the nested conditions don't return everything") public void testSearchActiveUsersViaNestingReturningEverything() throws NullResultException { EntityQuery<User> query = QueryBuilder.queryFor(User.class, EntityDescriptor.user()).with(Combine.allOf( Combine.anyOf( @@ -174,7 +186,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(&(sn=b*)(givenName=b*)))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(|(&(pwdAccountLockedTime=000001010000Z)(displayName=b*))(&(sn=b*)(givenName=b*))))", filter); } @Test @@ -203,7 +215,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(objectCategory=Person)(sAMAccountName=*))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(!(pwdAccountLockedTime=000001010000Z)))", filter); } @Test @@ -216,7 +228,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(sAMAccountName=*bob*))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(&(!(pwdAccountLockedTime=000001010000Z))(sAMAccountName=*bob*)))", filter); } @Test
I would love to see this feature, It is quite annoying having to remove groups for a user that has been removed instead of Jira simply disabling a user based on an attribute.
This patch (from https://jira.atlassian.com/browse/CWD-2762?focusedCommentId=1993254&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-1993254 ) works fine for us.
Is it possible to integrate this one into core product?
We really need this one into Jira, Confluence and Bitbucket!
I find it a bit disappointing that this issue is not addresses yet. I created a patch based on the previous patches for a more recent version 3.4.4
The add attachment button is gone so i added the patch is het in a code block:
-3.4.4.patch
Index: atlassian-crowd/components/crowd-ldap/src/test/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImplTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/test/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImplTest.java (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/test/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImplTest.java (date 1560350448000) @@ -13,8 +13,8 @@ import com.atlassian.crowd.search.query.entity.restriction.PropertyImpl; import com.atlassian.crowd.search.query.entity.restriction.constants.GroupTermKeys; import com.atlassian.crowd.search.query.entity.restriction.constants.UserTermKeys; - import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import static org.junit.Assert.assertEquals; @@ -48,6 +48,11 @@ when(ldapPropertiesMapper.getGroupNameAttribute()).thenReturn("cn"); when(ldapPropertiesMapper.getGroupDescriptionAttribute()).thenReturn("description"); when(ldapPropertiesMapper.getGroupMemberAttribute()).thenReturn("member"); + + // test user activation through LDAP Password Policy (http://tools.ietf.org/html/draft-behera-ldap-password-policy-10) + when(ldapPropertiesMapper.getUserActiveAttribute()).thenReturn("pwdAccountLockedTime"); + when(ldapPropertiesMapper.getUserActiveValue()).thenReturn(""); + when(ldapPropertiesMapper.getUserInactiveValue()).thenReturn("000001010000Z"); } @Test @@ -104,7 +109,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(|(givenName=joe*)(sn=joe*)(displayName=joe*)))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(&(|(givenName=joe*)(sn=joe*)(displayName=joe*))(!(pwdAccountLockedTime=000001010000Z))))", filter); } @Test @@ -116,14 +121,16 @@ assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(entryUUID=a1b2))", filter); } - @Test(expected = NullResultException.class) + @Test public void testSearchInactiveUsers() throws NullResultException { EntityQuery<User> query = QueryBuilder.queryFor(User.class, EntityDescriptor.user()).with(Restriction.on(UserTermKeys.ACTIVE).exactlyMatching(false)).returningAtMost(10); - queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + String filter = ldapQuery.encode(); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(pwdAccountLockedTime=000001010000Z))", filter); } - @Test(expected = NullResultException.class) + @Test public void testSearchInactiveUsersViaNestingReturningNullResult() throws NullResultException { EntityQuery<User> query = QueryBuilder.queryFor(User.class, EntityDescriptor.user()).with(Combine.anyOf( Combine.allOf( @@ -136,7 +143,11 @@ ) )).returningAtMost(10); - queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); + String filter = ldapQuery.encode(); + //assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(entryUUID=a1b2))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(|(&(pwdAccountLockedTime=000001010000Z)(displayName=b*))(&(pwdAccountLockedTime=000001010000Z)(givenName=b*))))", filter); + } @Test @@ -155,7 +166,8 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(objectCategory=Person)(sAMAccountName=*))", filter); +// assertEquals("(&(objectCategory=Person)(sAMAccountName=*))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(&(|(!(pwdAccountLockedTime=000001010000Z))(displayName=b*))(|(!(pwdAccountLockedTime=000001010000Z))(givenName=b*))))", filter); } @Test @@ -174,7 +186,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(&(sn=b*)(givenName=b*)))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(|(&(pwdAccountLockedTime=000001010000Z)(displayName=b*))(&(sn=b*)(givenName=b*))))", filter); } @Test @@ -203,7 +215,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(objectCategory=Person)(sAMAccountName=*))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(!(pwdAccountLockedTime=000001010000Z)))", filter); } @Test @@ -216,7 +228,7 @@ LDAPQuery ldapQuery = queryTranslater.asLDAPFilter(query, ldapPropertiesMapper); String filter = ldapQuery.encode(); - assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(sAMAccountName=*bob*))", filter); + assertEquals("(&(&(objectCategory=Person)(sAMAccountName=*))(&(!(pwdAccountLockedTime=000001010000Z))(sAMAccountName=*bob*)))", filter); } @Test @@ -281,7 +293,8 @@ assertEquals("(&(objectCategory=Role)(cn=admins))", filter); } - @Test(expected = NullResultException.class) + @Test + @Ignore("Those nested results no longer return null") public void testSearchInactiveGroups() throws NullResultException { EntityQuery<Group> query = QueryBuilder.queryFor(Group.class, EntityDescriptor.group(GroupType.GROUP)).with(Restriction.on(UserTermKeys.ACTIVE).exactlyMatching(false)).returningAtMost(10); Index: atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/mapper/entity/LDAPUserAttributesMapper.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/mapper/entity/LDAPUserAttributesMapper.java (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/mapper/entity/LDAPUserAttributesMapper.java (date 1560348813000) @@ -68,6 +68,13 @@ // full name putValueInAttributes(populatedUser.getDisplayName(), ldapPropertiesMapper.getUserDisplayNameAttribute(), directoryAttributes); + // active is the default case for LDAP, we only need to write something to the LDAP in case the user is not active + if (!populatedUser.isActive()) { + putValueInAttributes(ldapPropertiesMapper.getUserInactiveValue(), ldapPropertiesMapper.getUserActiveAttribute(), directoryAttributes); + } else { + putValueInAttributes(ldapPropertiesMapper.getUserActiveValue(), ldapPropertiesMapper.getUserActiveAttribute(), directoryAttributes); + } + // TODO: currently we don't support arbitrary attributes / iconLocation / active return directoryAttributes; @@ -129,6 +136,22 @@ } protected boolean getUserActiveFromAttribute(final Attributes directoryAttributes) { + String userActive = DirectoryAttributeRetriever.getValueFromAttributes(ldapPropertiesMapper.getUserActiveAttribute(), directoryAttributes); + if (userActive == null) { + if (logger.isDebugEnabled()) { + logger.debug("User active attribute '{}' on user '{}' is not set or could not be evaluated, assuming user is active", ldapPropertiesMapper.getUserActiveAttribute(), getUsernameFromAttributes(directoryAttributes)); + } + return true; + } + + if (logger.isDebugEnabled()) { + logger.debug("User active attribute '{}' is set to '{}' on user {}", ldapPropertiesMapper.getUserActiveAttribute(), userActive, getUsernameFromAttributes(directoryAttributes)); + } + // is user inactive? + if (userActive.equals(ldapPropertiesMapper.getUserInactiveValue())) { + logger.debug("User is inactive"); + return false; + } return true; } Index: atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/OpenLDAP.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/OpenLDAP.java (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/OpenLDAP.java (date 1560350511000) @@ -1,16 +1,30 @@ package com.atlassian.crowd.directory; +import com.atlassian.crowd.directory.ldap.LDAPPropertiesMapper; import com.atlassian.crowd.directory.ldap.credential.EncryptingCredentialEncoder; import com.atlassian.crowd.directory.ldap.credential.LDAPCredentialEncoder; +import com.atlassian.crowd.directory.ldap.mapper.UserContextMapperConfig; +import com.atlassian.crowd.directory.ldap.mapper.attribute.AttributeMapper; +import com.atlassian.crowd.directory.ldap.mapper.attribute.user.UserActiveMapper; import com.atlassian.crowd.model.user.User; import com.atlassian.crowd.password.factory.PasswordEncoderFactory; import com.atlassian.crowd.search.ldap.LDAPQueryTranslater; import com.atlassian.crowd.util.InstanceFactory; import com.atlassian.event.api.EventPublisher; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableList.Builder; import javax.naming.directory.Attributes; +import java.util.HashMap; +import java.util.List; +import java.util.Map; public class OpenLDAP extends RFC4519Directory { + + private static final String ATTRIBUTE_LOCKOUT_KEY = "pwdAccountLockedTime"; + /** @see <a href="http://linux.die.net/man/5/slapo-ppolicy">man for slapo-ppolicy</a> */ + private static final String ATTRIBUTE_LOCKOUT_VALUE_PERMANENTLY_LOCKED = "000001010000Z"; + private final PasswordEncoderFactory passwordEncoderFactory; public OpenLDAP(LDAPQueryTranslater ldapQueryTranslater, EventPublisher eventPublisher, InstanceFactory instanceFactory, @@ -25,7 +39,7 @@ @Override public String getDescriptiveName() { - return OpenLDAP.getStaticDirectoryType(); + return OpenLDAP.getStaticDirectoryType() + " (extended with support for sync of active flag, CWD-2762)"; } @Override @@ -43,4 +57,31 @@ protected void getNewUserDirectorySpecificAttributes(final User user, final Attributes attributes) { addDefaultSnToUserAttributes(attributes, user.getName()); } + + + /** + * As long as the LDAP attributes and values are not configurable, we just add those as hard-coded attributes in here, that they are available within {@link com.atlassian.crowd.directory.ldap.LDAPPropertiesMapper} + * The LDAP attribute keys and values are taken from the de-facto standard "Password Policy for LDAP Directories" + * @see <a href="http://tools.ietf.org/html/draft-behera-ldap-password-policy-10">Password Policy for LDAP Directories</a> + * @see <a href="http://linux.die.net/man/5/slapo-ppolicy">man for slapo-ppolicy</a> + */ + @Override + public void setAttributes(Map<String, String> attributes) { + Map<String, String> extendedAttributes = new HashMap<String, String>(attributes); + + extendedAttributes.put(LDAPPropertiesMapper.USER_ACTIVE_ATTRIBUTE_KEY, ATTRIBUTE_LOCKOUT_KEY); + extendedAttributes.put(LDAPPropertiesMapper.USER_ACTIVE_VALUE, ""); + extendedAttributes.put(LDAPPropertiesMapper.USER_INACTIVE_VALUE, ATTRIBUTE_LOCKOUT_VALUE_PERMANENTLY_LOCKED); + + super.setAttributes(extendedAttributes); + } + + @Override + protected List<AttributeMapper> getCustomUserAttributeMappers(UserContextMapperConfig config) { + + Builder<AttributeMapper> builder = ImmutableList.<AttributeMapper>builder(); + builder.addAll(super.getCustomUserAttributeMappers(config)); + builder.add(new UserActiveMapper(ldapPropertiesMapper.getUserActiveAttribute())); + return builder.build(); + } } Index: atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapperImpl.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapperImpl.java (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapperImpl.java (date 1557236920000) @@ -419,6 +419,22 @@ return getAttribute(LDAP_USER_ENCRYPTION_METHOD); } + @Override + public String getUserActiveAttribute() + { + return getAttribute(USER_ACTIVE_ATTRIBUTE_KEY); + } + + @Override + public String getUserActiveValue() { + return getAttribute(USER_ACTIVE_VALUE); + } + + @Override + public String getUserInactiveValue() { + return getAttribute(USER_INACTIVE_VALUE); + } + /** * Checks if the configuration of the LDAP directory server uses paged results. * Index: atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/mapper/attribute/user/UserActiveMapper.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/mapper/attribute/user/UserActiveMapper.java (date 1557236975000) +++ atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/mapper/attribute/user/UserActiveMapper.java (date 1557236975000) @@ -0,0 +1,38 @@ +package com.atlassian.crowd.directory.ldap.mapper.attribute.user; + +import com.atlassian.crowd.directory.ldap.mapper.attribute.AttributeMapper; +import org.springframework.ldap.core.DirContextAdapter; + +import java.util.Collections; +import java.util.Set; + +public class UserActiveMapper implements AttributeMapper { + + private final String userActiveAttribute; + + public UserActiveMapper(String userActiveAttribute) { + this.userActiveAttribute = userActiveAttribute; + } + + @Override + public String getKey() { + return userActiveAttribute; + } + + @Override + public Set<String> getValues(DirContextAdapter ctx) throws Exception { + // this must never return null + String userActiveValue = ctx.getStringAttribute(getKey()); + if (userActiveValue != null) { + return Collections.singleton(userActiveValue); + } else { + return Collections.emptySet(); + } + } + + @Override + public Set<String> getRequiredLdapAttributes() { + return Collections.singleton(getKey()); + } + +} Index: atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImpl.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImpl.java (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/search/ldap/LDAPQueryTranslaterImpl.java (date 1560350791000) @@ -5,6 +5,7 @@ import com.atlassian.crowd.directory.ldap.LDAPPropertiesMapper; import com.atlassian.crowd.embedded.api.SearchRestriction; +import com.atlassian.crowd.search.Entity; import com.atlassian.crowd.search.EntityDescriptor; import com.atlassian.crowd.search.query.entity.EntityQuery; import com.atlassian.crowd.search.query.entity.restriction.BooleanRestriction; @@ -13,12 +14,13 @@ import com.atlassian.crowd.search.query.entity.restriction.PropertyRestriction; import com.atlassian.crowd.search.query.entity.restriction.constants.GroupTermKeys; import com.atlassian.crowd.search.query.entity.restriction.constants.UserTermKeys; - +import org.apache.commons.lang3.StringUtils; import org.springframework.ldap.filter.AndFilter; import org.springframework.ldap.filter.BinaryLogicalFilter; import org.springframework.ldap.filter.EqualsFilter; import org.springframework.ldap.filter.Filter; import org.springframework.ldap.filter.LikeFilter; +import org.springframework.ldap.filter.NotFilter; import org.springframework.ldap.filter.NotPresentFilter; import org.springframework.ldap.filter.OrFilter; import org.springframework.ldap.filter.PresentFilter; @@ -127,16 +129,35 @@ protected Filter booleanTermRestrictionAsFilter(final EntityDescriptor entityDescriptor, final PropertyRestriction<Boolean> termRestriction, LDAPPropertiesMapper ldapPropertiesMapper) { // if boolean term restrictions are for anything other than the group/user active flag, then throw exception - if (!termRestriction.getProperty().equals(GroupTermKeys.ACTIVE) && !termRestriction.getProperty().equals(UserTermKeys.ACTIVE)) { - throw new IllegalArgumentException("Boolean restrictions for property " + termRestriction.getProperty().getPropertyName() + " are not supported"); - } else { + if (entityDescriptor.getEntityType() == Entity.USER && termRestriction.getProperty().equals(UserTermKeys.ACTIVE)) { + if (StringUtils.isEmpty(ldapPropertiesMapper.getUserActiveAttribute())) { + if (termRestriction.getValue()) { + // groups are always active = true, so no need to add a restriction + return new EverythingResult(); + } else { + // no group is ever active = false, so no results + return new NothingResult(); + } + } else { + if (termRestriction.getValue()) { + // querying for enabled users + return new NotFilter(new EqualsFilter(ldapPropertiesMapper.getUserActiveAttribute(), ldapPropertiesMapper.getUserInactiveValue())); + } else { + // querying for disabled users + return new EqualsFilter(ldapPropertiesMapper.getUserActiveAttribute(), ldapPropertiesMapper.getUserInactiveValue()); + } + } + } else if (entityDescriptor.getEntityType() == Entity.GROUP && termRestriction.getProperty().equals(GroupTermKeys.ACTIVE)) { if (termRestriction.getValue()) { - // everything is active = true, so no need to add a restriction + // groups are always active = true, so no need to add a restriction return new EverythingResult(); } else { - // nothing is active = false, so need to + // no group is ever active = false, so no results return new NothingResult(); } + } else { + // if boolean term restrictions are for anything other than the group/user active flag, then throw exception + throw new IllegalArgumentException("Boolean restrictions for property " + termRestriction.getProperty().getPropertyName() + " are not supported"); } } Index: atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/SpringLDAPConnector.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/SpringLDAPConnector.java (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/SpringLDAPConnector.java (date 1560349863000) @@ -870,6 +870,10 @@ try { SearchControls searchControls = getSearchControls(contextMapper, SearchControls.OBJECT_SCOPE); + if (contextMapper instanceof ContextMapperWithRequiredAttributes<?>) { + logger.debug("Return required attributes {} for findEntityById", (((ContextMapperWithRequiredAttributes<?>)contextMapper).getRequiredLdapAttributes())); + searchControls.setReturningAttributes(toArray(((ContextMapperWithRequiredAttributes<?>)contextMapper).getRequiredLdapAttributes())); + } searchControls.setTimeLimit(ldapPropertiesMapper.getSearchTimeLimit()); entities = ldapTemplate.search(asLdapName(dn, "DN: " + dn, entityClass), filter, searchControls, contextMapper); } catch (NameNotFoundException e) { @@ -971,6 +975,30 @@ modificationItems.add(displayNameMod); } + // support enabling/disabling users + if (supportsInactiveAccounts()) { + ModificationItem activeModItem = null; + if (!currentUser.isActive() && userTemplate.isActive()) { + // try to activate user in case it is currently inactive + String newValue = ldapPropertiesMapper.getUserActiveValue(); + if (StringUtils.isEmpty(newValue)) { + // rather than modifying an attribute we completely remove it + activeModItem = new ModificationItem(DirContext.REMOVE_ATTRIBUTE, new BasicAttribute(ldapPropertiesMapper.getUserActiveAttribute())); + } else { + // TODO: getting the real old value from LDAP is not easily possible here, just always use a REPLACE by setting the old value to something else than new value + activeModItem = createModificationItem(ldapPropertiesMapper.getUserActiveAttribute(), newValue + "changed", newValue); + } + } else if (currentUser.isActive() && !userTemplate.isActive()) { + // try to deactivate user in case it is currently active + String newValue = ldapPropertiesMapper.getUserInactiveValue(); + // TODO: getting the real old value from LDAP is not easily possible here, just always use a REPLACE by setting the old value to something else than new value + activeModItem = createModificationItem(ldapPropertiesMapper.getUserActiveAttribute(), newValue + "changed", newValue); + } + if (activeModItem != null) + { + modificationItems.add(activeModItem); + } + } return modificationItems; } @@ -1545,7 +1573,7 @@ */ @Override public boolean supportsInactiveAccounts() { - return false; + return StringUtils.isNotBlank(ldapPropertiesMapper.getUserActiveAttribute()); } @Override Index: atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapper.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapper.java (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/main/java/com/atlassian/crowd/directory/ldap/LDAPPropertiesMapper.java (date 1560350010000) @@ -176,6 +176,21 @@ */ String USER_PASSWORD_KEY = "ldap.user.password"; + /** + * Attribute key for the LDAP principal active attribute. + */ + String USER_ACTIVE_ATTRIBUTE_KEY = "ldap.user.active.attribute"; + + /** + * Attribute key for the LDAP value which stands for active users. In contrast to {@link #USER_INACTIVE_VALUE} this is only considered for writing to the LDAP, not for evaluation! + */ + String USER_ACTIVE_VALUE = "ldap.user.active"; + + /** + * Attribute key for the LDAP value which stands for inactive users. This is used for checking whether an account is active and also for persisting that information within the LDAP. + */ + String USER_INACTIVE_VALUE = "ldap.user.inactive"; + /** * Attribute key for the LDAP paged results attribute. */ @@ -380,6 +395,22 @@ String getUserEncryptionMethod(); + /** + * @return the LDAP attribute in which the users active state is stored. Derived from {@link #USER_ACTIVE_ATTRIBUTE_KEY}. + */ + String getUserActiveAttribute(); + + /** + * @return the LDAP value which stands for an active user (should only be used for turning a user from inactive to active, i.e. for writing to LDAP, but not for evaluating the state). Derived from {@link #USER_ACTIVE_VALUE}} + */ + String getUserActiveValue(); + + /** + * @return the LDAP value which stands for an inactive user (should only be used for turning a user from active to inactive, i.e. for writing to LDAP, as well as for evaluating the current state). Derived from {@link #USER_INACTIVE_VALUE}} + */ + String getUserInactiveValue(); + + boolean isPagedResultsControl(); int getPagedResultsSize(); @@ -431,7 +462,7 @@ * compatible method for standardising DNs when mapping object DNs and * and memberDNs (value = <code>false</code>); or if we can use a more * efficient but relaxed form of standardisation (value = <code>true</code>). - * + * <p> * See <code>DNStandardiser</code> for more information. * * @return <code>false</code> if proper standardisation is required. Index: atlassian-crowd/components/crowd-ldap/src/main/resources/com/atlassian/crowd/integration/directory/openldap.properties IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- atlassian-crowd/components/crowd-ldap/src/main/resources/com/atlassian/crowd/integration/directory/openldap.properties (date 1557236232000) +++ atlassian-crowd/components/crowd-ldap/src/main/resources/com/atlassian/crowd/integration/directory/openldap.properties (date 1560346799000) @@ -24,6 +24,11 @@ ldap.user.group memberOf ldap.user.password userPassword +# active properties (according to http://tools.ietf.org/html/draft-behera-ldap-password-policy-10) +ldap.user.active.attribute pwdAccountLockedTime +ldap.user.active +ldap.user.inactive 000001010000Z + # generic options ldap.pagedresults false ldap.relaxed.dn.standardisation true
Deleted Account (Inactive)
added a comment - I added the updated patch for a more recent version: CWD-2762-3.1.2.patch
Deleted Account (Inactive)
added a comment - I added the updated patch for a more recent version: CWD-2762-3.1.2.patch I attached the modified patch for 2.11.1 which supports OpenLDAP: CWD-2762-2.11.1.patch
Jason @ FPC
added a comment - Thanks @gviana! Jason @ FPC
added a comment - Thank you for updating the status of this issue to bug, reflecting the true nature of the issue. In that same light, I would request we update the list of "affected versions", as I know 2.8.4 is also affected, and I have been led to believe the bug is present all the way up to the current stable release.
Jason @ FPC
added a comment - Thank you for updating the status of this issue to bug, reflecting the true nature of the issue. In that same light, I would request we update the list of "affected versions", as I know 2.8.4 is also affected, and I have been led to believe the bug is present all the way up to the current stable release. Jason @ FPC
added a comment - There are a number of things about this bug that make it really troubling:
1) Crowd tells the user the operation is successful, yet Crowd also throws an exception:
2016-11-05 20:43:43,042 http-bio-8095-exec-174 ERROR [crowd.manager.token.RecoveryModeAwareTokenAuthenticationManager] Operation to update last active for user 'foobar' faile d com.atlassian.crowd.exception.OperationNotSupportedException: Custom user attributes are not yet supported for LDAP directories at com.atlassian.crowd.directory.SpringLDAPConnector.storeUserAttributes(SpringLDAPConnector.java:748) at com.atlassian.crowd.manager.directory.DirectoryManagerGeneric.storeUserAttributes(DirectoryManagerGeneric.java:415) at sun.reflect.GeneratedMethodAccessor397.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) ...
This is misleading to the user, and problematic because some users do not realize the action actually failed (which is not just a licensing issue, but also a security implication as well - we're talking about deactivating users).
2) Given that the action fails, we're unable to properly disable/deactivate the target user, and must take other steps to do so (all of which are cumbersome and cost each of your users additional time to research and implement, none of which are tested/documented/supported by Atlassian).
3) Crowd does authentication/authorization, it is a critical component in an organization's security fabric. If the organization is running a suite of Atlassian applications, and leveraging CrowdID, we have compounded the problem. Allowing this bug to persist another day/week/month/year leaves us less secure and more prone to creating holes in our security (thinking we have deactivated a user that is still active, requiring users patch this issue themselves, and/or requiring users find another way to deactivate the user without impacting details like ensuring our JIRA tickets still include the deactivated user for regulatory purposes).
4) None of this is documented, to the contrary, the documentation tells you using OpenLDAP as a directory backend is a supported configuration, yet this core feature/action (disabling/deactivating a user) is not only broken, but misleading the user to believe the action succeeded.
5) No one should have to apply a patch as is offered here, for this type of problem. It should be acknowledged as a critical bug and fixed expeditiously and with care to the nature of the bug's implications on security-conscious organizations. Atlassian should not ask their users to apply a random and unsupported patch for an application that is critical to an organization's application security. While I may be qualified to manage a secure network, and while I may have developers available to me, that doesn't mean either of us are fit for properly reviewing that patch, and it would be irresponsible of me to knowingly apply that patch without getting a proper code review. This is especially true for Crowd as a application critical to security.
I call on Atlassian to address your customer's concerns in one of the following ways:
1) Properly fix this bug and make a release sooner, not later. You already have a patch in the works, and you can easily test/release a polished fix to this rather simple but critical bug.
2) Be honest with your users, and either document the known problems or completely drop OpenLDAP as a supported configurations. Ensure the action which throws and exception does not claim success when it failed - accurately report those results to the user.
For each day/week/month/year that passes with a bug like this that is ignored and left idle, your users (current and potential) lose faith in Atlassian. I want to be able to vouch for the quality in your software and attention to security-critical details like this, yet you instead give me reasons to tell my management and architects to refuse a solution which includes Atlassian's software.
Please do the right thing and address this bug ASAP.
Jason @ FPC
added a comment - There are a number of things about this bug that make it really troubling:
1) Crowd tells the user the operation is successful, yet Crowd also throws an exception:
2016-11-05 20:43:43,042 http-bio-8095-exec-174 ERROR [crowd.manager.token.RecoveryModeAwareTokenAuthenticationManager] Operation to update last active for user 'foobar' faile d com.atlassian.crowd.exception.OperationNotSupportedException: Custom user attributes are not yet supported for LDAP directories at com.atlassian.crowd.directory.SpringLDAPConnector.storeUserAttributes(SpringLDAPConnector.java:748) at com.atlassian.crowd.manager.directory.DirectoryManagerGeneric.storeUserAttributes(DirectoryManagerGeneric.java:415) at sun.reflect.GeneratedMethodAccessor397.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606)
...
This is misleading to the user, and problematic because some users do not realize the action actually failed (which is not just a licensing issue, but also a security implication as well - we're talking about deactivating users).
2) Given that the action fails, we're unable to properly disable/deactivate the target user, and must take other steps to do so (all of which are cumbersome and cost each of your users additional time to research and implement, none of which are tested/documented/supported by Atlassian).
3) Crowd does authentication/authorization, it is a critical component in an organization's security fabric. If the organization is running a suite of Atlassian applications, and leveraging CrowdID, we have compounded the problem. Allowing this bug to persist another day/week/month/year leaves us less secure and more prone to creating holes in our security (thinking we have deactivated a user that is still active, requiring users patch this issue themselves, and/or requiring users find another way to deactivate the user without impacting details like ensuring our JIRA tickets still include the deactivated user for regulatory purposes).
4) None of this is documented, to the contrary, the documentation tells you using OpenLDAP as a directory backend is a supported configuration, yet this core feature/action (disabling/deactivating a user) is not only broken, but misleading the user to believe the action succeeded.
5) No one should have to apply a patch as is offered here, for this type of problem. It should be acknowledged as a critical bug and fixed expeditiously and with care to the nature of the bug's implications on security-conscious organizations. Atlassian should not ask their users to apply a random and unsupported patch for an application that is critical to an organization's application security. While I may be qualified to manage a secure network, and while I may have developers available to me, that doesn't mean either of us are fit for properly reviewing that patch, and it would be irresponsible of me to knowingly apply that patch without getting a proper code review. This is especially true for Crowd as a application critical to security.
I call on Atlassian to address your customer's concerns in one of the following ways:
1) Properly fix this bug and make a release sooner, not later. You already have a patch in the works, and you can easily test/release a polished fix to this rather simple but critical bug.
2) Be honest with your users, and either document the known problems or completely drop OpenLDAP as a supported configurations. Ensure the action which throws and exception does not claim success when it failed - accurately report those results to the user.
For each day/week/month/year that passes with a bug like this that is ignored and left idle, your users (current and potential) lose faith in Atlassian. I want to be able to vouch for the quality in your software and attention to security-critical details like this, yet you instead give me reasons to tell my management and architects to refuse a solution which includes Atlassian's software.
Please do the right thing and address this bug ASAP. I just attached an updated patch for Crowd 2.9.1 where this is still not fixed. Would you please reconsider applying the patch?
David Yu
added a comment - I applied this kludgey workaround:
1. Add a new Crowd internal directory
2. Make some sync mechanism to populate it with inactive users. We use the crowd API to add to this directory on a nightly basis. Mark each of these users inactive.
3. Place this directory above the position of your normal OpenLDAP directory
David Yu
added a comment - I applied this kludgey workaround:
1. Add a new Crowd internal directory
2. Make some sync mechanism to populate it with inactive users. We use the crowd API to add to this directory on a nightly basis. Mark each of these users inactive.
3. Place this directory above the position of your normal OpenLDAP directory
Brian Topping
added a comment - I didn't realize @konrad.windszus had posted a patch for this over a year ago. Can't imagine why it's not fixed yet. Doing my best to contain my snark here... :/
Brian Topping
added a comment - I didn't realize @konrad.windszus had posted a patch for this over a year ago. Can't imagine why it's not fixed yet. Doing my best to contain my snark here... :/ We use OpenDJ as the external directory for our Crowd instance. Much like user "jh" in the comment thread above, we are unable to deactivate users in Crowd (unchecking the "Active" checkbox for a given user does not persist the change). We are forced to use the Remove User feature in Crowd in order to stay under our licensing limitations in Crowd as well as in other Atlassian products integrated through Crowd. Removing users has an impact on the change history of Issues in JIRA and of pages in Confluence.
To me, this is a "defect", not a missing feature or a new feature request. This is basic, and somewhat essential, Crowd functionality that is broken. It seems many users experience the problem, and it would be great to hear an update.
Michał Staruch
added a comment - Any chance for progress? 3 years passed, and still no improvements here.
Michał Staruch
added a comment - Any chance for progress? 3 years passed, and still no improvements here. Brian Topping
added a comment - Please add this to the generic Crowd connector in JIRA. Compared to FreeIPA, Crowd isn't very attractive and isn't necessary in advanced deployments nowadays.
Brian Topping
added a comment - Please add this to the generic Crowd connector in JIRA. Compared to FreeIPA, Crowd isn't very attractive and isn't necessary in advanced deployments nowadays. 
Yuri Gorshkov
added a comment - I contacted the FreeIPA developers with this issue and they told me you should also implement relying on nsAccountLock attribute for user lockouts, because it is widely supported by NS-derived LDAP servers.
Shouldn't be that hard, I think?
< ab> nsaccountlock is operational attribute OID 2.16.840.1.113730.3.1.610 that exists for more than 10 years (if not more)
<ab> all Netscape DIrectory-derived servers support it
Yuri Gorshkov
added a comment - I contacted the FreeIPA developers with this issue and they told me you should also implement relying on nsAccountLock attribute for user lockouts, because it is widely supported by NS-derived LDAP servers.
Shouldn't be that hard, I think?
< ab> nsaccountlock is operational attribute OID 2.16.840.1.113730.3.1.610 that exists for more than 10 years (if not more)
<ab> all Netscape DIrectory-derived servers support it Yuri Gorshkov
added a comment - Could we also implement this for Generic LDAP directories?
We have a FreeIPA instance that we'd like to use with Crowd, and we need a reliable way to detect if the user is inactive.
I've filed a feature request against FreeIPA's bug tracker here: https://fedorahosted.org/freeipa/ticket/4222 so I hope FreeIPA will support the 'standard' way too.
Yuri Gorshkov
added a comment - Could we also implement this for Generic LDAP directories?
We have a FreeIPA instance that we'd like to use with Crowd, and we need a reliable way to detect if the user is inactive.
I've filed a feature request against FreeIPA's bug tracker here: https://fedorahosted.org/freeipa/ticket/4222 so I hope FreeIPA will support the 'standard' way too. Any chance you would be able to explore merging Konrad's patch in a future release of Crowd?
Thanks mate, have a great week,
Nick
The attached version of the patch defines three new crowd attributes (which are not yet maintainable via the web console). Those define which LDAP attribute should be taken to evaluate whether a user is active or inactive and the values for active and inactive. For the RFC draft listed above which is implemented by OpenLDAP those attributes are set to pwdAccountLockedTime (attributeName), "" (value for active users, means the attribute is removed if users was turned active) and "000001010000Z" (value for permanently locked users).
The patch supports reading the flags from LDAP, writing it to LDAP and also searching for active or inactive users. Currently it is only enabled for the OpenLDAP connector but the implementation can be used for almost any LDAP as it is very flexible. I would love to see it integrated in the next version of Crowd.
Patched version of the LDAP connector to support pushing of active flag. Only active for OpenLDAP for now.
By setting the attribute pwdAccountLockedTime to the value 000001010000Z you mark an account as permanently locked. This is not yet a standard, but defined in a RFC draft (http://tools.ietf.org/html/draft-behera-ldap-password-policy-10), which is currently implemented by:
- OpenLDAP (http://www.openldap.org/doc/admin24/overlays.html#Password%20Policies)
- ApacheDS (http://directory.apache.org/apacheds/advanced-ug/4.3-password-policy.html)
- IBM Tivoli Directory Server (http://publib.boulder.ibm.com/infocenter/tivihelp/v2r1/index.jsp?topic=%2Fcom.ibm.IBMDS.doc_6.2%2Fadmin_gd39.htm)
It would be good to extend the LDAP connector to optionally support locking accounts through that attribute. If pwdAccountLockedTime is set to anything else than 000001010000Z the account should be considered enabled (as in that case, the attribute value might be changed by the LDAP server, e.g. after some time elapsed). Such an implementation would cover CWD-995 as well.
Would be very handy to have a way to filter out inactive users in LDAP.
Or even as a start, provide the same option given to Active Directory with the feature Manage User Status Locally to the other connector types.
The inability to do this is making mentions, and user auto-completion very tedious.
jh
added a comment - the numbers of users in openldap is growing constantly. Due to the fact, that we can't deactivate users that have left, we are comming close to the limitation of 100 users.
As an alternative to some bugfixing we would also kindly accept some user upgrade. Don't get me wrong, but having no way to dismiss deactivated users is no good implementation for customers like us.
jh
added a comment - the numbers of users in openldap is growing constantly. Due to the fact, that we can't deactivate users that have left, we are comming close to the limitation of 100 users.
As an alternative to some bugfixing we would also kindly accept some user upgrade. Don't get me wrong, but having no way to dismiss deactivated users is no good implementation for customers like us. jh
added a comment - is there some activity going on?
we observe more and more problems with this missing feature. when users leave the company, they still get mails from the projects they participate, as only deactivating them on the LDAP-directory doesn't remove them as active users.
jh
added a comment - is there some activity going on?
we observe more and more problems with this missing feature. when users leave the company, they still get mails from the projects they participate, as only deactivating them on the LDAP-directory doesn't remove them as active users. jh
added a comment - The directory used is OpenLDAP. 
Justin Koke
added a comment - Is Collax Businness Server, just an administration interface on top of Microsoft Active Directory?
Looking at: http://www.collax.com/produkte/detail/produkt/Die-Komplettloesung-fuer-kleine-Unternehmen
On the above link it mentions putting it over a Windows domain, if at the end of the day this is just Active Directory, there is already a issue open here, that would satisfy this feature request CWD-995
Justin Koke
added a comment - Is Collax Businness Server, just an administration interface on top of Microsoft Active Directory?
Looking at: http://www.collax.com/produkte/detail/produkt/Die-Komplettloesung-fuer-kleine-Unternehmen
On the above link it mentions putting it over a Windows domain, if at the end of the day this is just Active Directory, there is already a issue open here, that would satisfy this feature request CWD-995 
Below the patch for Crowd 4.1.2