Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EmbeddedLdapContextConfiguration LdapContextSource bean creation not populating base property #23030

Closed
mdiskin opened this issue Aug 20, 2020 · 12 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@mdiskin
Copy link

mdiskin commented Aug 20, 2020

Drift in behavior between LdapAutoConfiguration and EmbeddedLdapAutoConfiguration.

LdapAutoConfiguration populates the base from properties in LdapContextSource and the EmbeddedLdapAutoConfiguration doesn't which changes how queries, etc.. switching between the two.

I don't mind submitting a PR but wanted confirmation of actual problem or expected behavior.

Tested on 2.4.0-M2

Thanks
Mark

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 20, 2020
@snicoll
Copy link
Member

snicoll commented Aug 21, 2020

@mdiskin sorry, I am not sure I follow. The LdapContextSource is built from properties and so is the embedded auto-configuration as far as I can see.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Aug 21, 2020
@mdiskin
Copy link
Author

mdiskin commented Aug 21, 2020

In the Non-Embedded Autocomplete the there is a property (single string). In the Embedded there is the ability to pass into unboundid an array of dn's but the corresponding LdapContextSource bean create doesn't set this base.

propertyMapper.from(properties.getBase()).to(source::setBase);

So if you are using the embedded for unit/integration testing and then Non-Embedded you have to add additional logic that fully qualifies each LdapTempate search e.g. add "dc=spring,dc=org".

I was playing with options in a forked version and the unboundid support for multiple DNs doesn't seem well supported given it only takes a single base so I wasn't sure whether to remove that array/functionality for the embed-only option. One thought would be for embedded to add another property 'base' (in addition to the dn array) ideally we could derive that from the other but that would not be very easy to determine.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 21, 2020
@bclozel bclozel added this to the 2.2.x milestone Aug 26, 2020
@bclozel bclozel added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Aug 26, 2020
@snicoll
Copy link
Member

snicoll commented Aug 26, 2020

We discussed this one and I can see the problem now. It's largely due to the fact the configuration between the embedded and the non embedded case are duplicated. Rather than creating a completely new LdapContextSource we should customise the one created by the regular auto-configuration (via a LdapContextSourceCustomizer perhaps?).

The fact that there is an .embedded subnamespace with competing configuration keys make it a bit more complex to decide what should be taken into account so perhaps we should refine the structure of those. We've flagged this for resolution in 2.2.x but we may move that if the changes are too involved.

@mdiskin
Copy link
Author

mdiskin commented Aug 27, 2020

That would be awsome but sounds involved refactoring and not something for my first project commit. Let me know if I can help out maybe with testing/snapshot validation.

Additionally with this approach can you also look at the autoconfigure support for BaseLdapPathBeanPostProcessor
and BaseLdapPathAware? I tried that to get around the above issue but again not something the embedded ldap supports out of the box.

Also, would be good to upgrade the unboundid versions to the 5.x at some point.

@philwebb philwebb modified the milestones: 2.2.x, 2.3.x Dec 16, 2020
@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jun 1, 2021
@philwebb
Copy link
Member

philwebb commented Jun 2, 2021

We want to rework EmbeddedLdapContextConfiguration, possibly removing it entirely. Although we do consider this a bug, such a fix would be a little risky for 2.3.x so we're going to look at it in 2.6.x.

@philwebb philwebb modified the milestones: 2.3.x, 2.6.x Jun 2, 2021
@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Jun 2, 2021
@mdiskin
Copy link
Author

mdiskin commented Jun 3, 2021

Makes sense. I can help test snapshot or milestone releases. Also, UnboundID 6.0.0 has since been released and may be good to include in 2.6.x changes

@mdiskin
Copy link
Author

mdiskin commented Aug 17, 2021

Was hoping to get confirmation that will be in the 2.6 initial release (I'm working against the snapshot), but if not I'll start work on a stopgap measure as it's blocking some efforts internally.

@wilkinsona
Copy link
Member

wilkinsona commented Aug 17, 2021

An upgrade of UnboundID is unlikely at this time as Spring Security is still using 4.x. If you'd like to see an upgrade to 6.x, please open a Spring Security issue and we can take things from there.

@mdiskin
Copy link
Author

mdiskin commented Aug 18, 2021

@wilkinsona I'll open that upgrade request (more of a nice to have) but the bugfix to align up the embedded and external ldap is the real blocker for me

@micheljung
Copy link

micheljung commented Aug 18, 2021

UPDATE: This doesn't work, because base needs to be set before afterPropertiesSet() is called

Workaround in Kotlin (FQCNs for clarification):

  @Autowired
  fun populateLdapContextSourceBaseProperty(
    ldapContextSource: org.springframework.ldap.core.support.LdapContextSource,
    ldapProperties: org.springframework.boot.autoconfigure.ldap.LdapProperties,
    inMemoryDirectoryServer: Optional<com.unboundid.ldap.listener.InMemoryDirectoryServer>,
  ) {
    if (!inMemoryDirectoryServer.isPresent) {
      return
    }

    if (ldapContextSource.baseLdapName != org.springframework.ldap.support.LdapUtils.emptyLdapName()) {
      logger.warn("Not setting 'base' property of LdapContextSource as it's already set: ${ldapContextSource.baseLdapName}")
      return
    }

    logger.debug("Setting 'base' property of LdapContextSource to '${ldapContextSource.baseLdapName}'" +
        " as a workaround for https://github.com/spring-projects/spring-boot/issues/23030")
    ldapContextSource.setBase(ldapProperties.base)
  }

@micheljung
Copy link

This one works (Kotlin):

package com.example

import org.springframework.boot.autoconfigure.AutoConfigureBefore
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
import org.springframework.boot.autoconfigure.ldap.LdapProperties
import org.springframework.boot.autoconfigure.ldap.embedded.EmbeddedLdapAutoConfiguration
import org.springframework.boot.autoconfigure.ldap.embedded.EmbeddedLdapProperties
import org.springframework.boot.context.properties.EnableConfigurationProperties
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
import org.springframework.context.annotation.DependsOn
import org.springframework.core.env.Environment
import org.springframework.ldap.core.ContextSource
import org.springframework.ldap.core.support.LdapContextSource

@Configuration(proxyBeanMethods = false)
@AutoConfigureBefore(EmbeddedLdapAutoConfiguration::class)
@EnableConfigurationProperties(LdapProperties::class, EmbeddedLdapProperties::class)
class EmbeddedLdapAutoConfiguration {

  // Workaround for https://github.com/spring-projects/spring-boot/issues/23030
  @Configuration(proxyBeanMethods = false)
  @ConditionalOnClass(ContextSource::class)
  internal class EmbeddedLdapContextConfiguration {
    @Bean
    @DependsOn("directoryServer")
    @ConditionalOnMissingBean
    fun ldapContextSource(
      environment: Environment,
      properties: LdapProperties,
      embeddedProperties: EmbeddedLdapProperties,
    ): LdapContextSource {
      val source = LdapContextSource()
      if (!embeddedProperties.credential.username.isNullOrEmpty() && !embeddedProperties.credential.password.isNullOrEmpty()) {
        source.userDn = embeddedProperties.credential.username
        source.password = embeddedProperties.credential.password
      }
      source.urls = properties.determineUrls(environment)
      source.setBase(properties.base)
      return source
    }
  }
}

spring.factories:

org.springframework.boot.autoconfigure.EnableAutoConfiguration=com.example.EmbeddedLdapAutoConfiguration

@philwebb philwebb modified the milestones: 2.6.x, 2.x Oct 13, 2021
@philwebb philwebb modified the milestones: 2.x, 3.0.x Aug 19, 2022
@philwebb philwebb modified the milestones: 3.0.x, 3.1.x Oct 5, 2022
@mbhave mbhave self-assigned this Oct 6, 2022
@mbhave mbhave closed this as completed in 263433c Oct 7, 2022
@mbhave
Copy link
Contributor

mbhave commented Oct 7, 2022

We decided to fix this by populating the base for the embedded context. In the future, we might want to revisit this to consider the customizer option.

@mbhave mbhave modified the milestones: 3.0.x, 3.0.0-RC1 Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

8 participants