[microservice] Add auth-mode ldap to enable authentication based on LDAP
#35
Conversation
eidottermihi
changed the title
ldap to enable authentication based on LDAP
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes in this pull request primarily involve modifications to the authentication configuration within a Spring application. Key updates include renaming methods in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (9)
microservice/src/main/java/de/muenchen/oss/ezldap/config/AppConfigurationProperties.java (1)
Line range hint
42-56: Consider adding LDAP configuration properties.While there's a
@ValidbasicAuthproperty for BASIC authentication, there's no corresponding configuration property for LDAP settings (URL, base DN, etc.). Consider adding anLdapAuthConfigurationPropertiesclass to encapsulate LDAP-specific configuration.Example implementation:
@Data public class LdapAuthConfigurationProperties { @NotNull private String url; @NotNull private String baseDn; // Optional settings private Boolean useStartTls; private String bindDn; private String bindPassword; } // Add to AppConfigurationProperties @NestedConfigurationProperty @Valid private LdapAuthConfigurationProperties ldapAuth;microservice/src/main/java/de/muenchen/oss/ezldap/security/GlobalSecurityExceptionHandler.java (1)
32-34: Enhance class-level documentationThe current documentation only includes the author. Consider adding comprehensive Javadoc that describes:
- The class's purpose in handling security exceptions
- The types of exceptions handled
- The expected response status codes
- Integration with Spring's exception handling mechanism
/** + * Global exception handler for security-related exceptions in the application. + * Provides centralized handling of authentication failures and related security errors. + * + * @see org.springframework.web.bind.annotation.ControllerAdvice + * @see javax.naming.AuthenticationException * @author michael.prankl */microservice/src/test/java/de/muenchen/oss/ezldap/CorsConfigurationTest.java (1)
Line range hint
66-79: Consider adding CORS tests for LDAP authentication endpoints.While the current test covers CORS for the user endpoint, consider adding test cases for the new LDAP authentication endpoints introduced in this PR. This would ensure CORS is properly configured for the new authentication flows.
Example scenarios to consider:
- CORS behavior for LDAP authentication attempts
- CORS headers on authentication failure responses
- CORS pre-flight requests (OPTIONS) for authenticated endpoints
lib-spring/src/main/java/de/muenchen/oss/ezldap/spring/LdapConfiguration.java (2)
Line range hint
49-58: Consider masking sensitive information in logsWhile logging LDAP connection details is useful for debugging, consider masking sensitive parts of the userDn to prevent potential security risks in log files.
- log.info("Initiating LDAP connection with url='{}' and user-dn='{}'.", props.getLdap().getUrl(), - props.getLdap().getUserDn()); + log.info("Initiating LDAP connection with url='{}' and user-dn='{}'.", props.getLdap().getUrl(), + maskSensitiveInfo(props.getLdap().getUserDn()));Consider adding a utility method:
private String maskSensitiveInfo(String userDn) { return userDn.replaceAll("(?<=cn=)[^,]+", "****"); }
Line range hint
49-58: Consider implementing LDAP connection poolingFor production environments, consider implementing LDAP connection pooling to improve performance and resource utilization. Spring LDAP supports this through
PoolingContextSource.Example implementation:
@Bean @ConditionalOnMissingBean(name = "ezldapQueryContextSource") LdapContextSource ezldapQueryContextSource(final EzLdapConfigurationProperties props) { final LdapContextSource contextSource = new LdapContextSource(); contextSource.setUrl(props.getLdap().getUrl()); contextSource.setUserDn(props.getLdap().getUserDn()); contextSource.setPassword(props.getLdap().getPassword()); // Add pooling configuration contextSource.setPooled(true); contextSource.getBaseEnvironmentProperties().put("com.sun.jndi.ldap.connect.pool", "true"); contextSource.getBaseEnvironmentProperties().put("com.sun.jndi.ldap.connect.pool.maxsize", "8"); return contextSource; }microservice/src/main/java/de/muenchen/oss/ezldap/config/SpringdocsSwaggerConfig.java (1)
76-79: Enhance LDAP authentication descriptionThe current description "Authenticate using LDAP" could be more informative for API consumers.
Consider updating the description to include:
- new SecurityScheme().type(SecurityScheme.Type.HTTP).scheme("basic").description("Authenticate using LDAP"))); + new SecurityScheme().type(SecurityScheme.Type.HTTP).scheme("basic") + .description("Authenticate using LDAP. Provide LDAP Distinguished Name (DN) as username (e.g., 'cn=admin,dc=example,dc=org') and corresponding password.")));microservice/src/test/java/de/muenchen/oss/ezldap/LdapAuthPassthroughTest.java (1)
84-113: Improve test structure and coverage.Consider the following improvements:
- Extract the common URL pattern to a constant or helper method
- Add tests for edge cases:
- Malformed DN
- Empty password
- Special characters in credentials
- Consider adding cleanup between tests if the LDAP state is modified
Example refactor:
+ private static final String TEST_ENDPOINT = "/v1/ldap/search/findByUidWildcard"; + + private String buildUrl(String uid) { + return String.format("http://localhost:%d%s?uid=%s&size=10", + webPort, TEST_ENDPOINT, uid); + } @Test void unauthenticated() { - ResponseEntity<String> responseEntity = testRestTemplate - .getForEntity("http://localhost:" + webPort + "/v1/ldap/search/findByUidWildcard?uid=erika.%2A&size=10", String.class); + ResponseEntity<String> responseEntity = testRestTemplate + .getForEntity(buildUrl("erika.%2A"), String.class); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED); }🧰 Tools
🪛 GitHub Check: JUnit Test Report
[failure] 88-88: LdapAuthPassthroughTest.unauthenticated
expected: 401 UNAUTHORIZED
but was: 403 FORBIDDEN
[failure] 96-96: LdapAuthPassthroughTest.basic_auth_invalid_dn_as_username
expected: 401 UNAUTHORIZED
but was: 403 FORBIDDENmicroservice/src/main/java/de/muenchen/oss/ezldap/security/BasicAuthPassthroughFilter.java (2)
49-52: Rename parameterfiltertofilterProcessUrlfor clarityThe parameter
filterrepresents the processing URL for the authentication filter. Renaming it tofilterProcessUrlenhances readability and aligns with common conventions in Spring Security.Apply this diff to rename the parameter:
-public BasicAuthPassthroughFilter(AuthenticationManager authenticationManager, String filter) { - super(filter); +public BasicAuthPassthroughFilter(AuthenticationManager authenticationManager, String filterProcessUrl) { + super(filterProcessUrl);
85-99: Utilize Spring'sBasicAuthenticationConverterfor decoding credentialsManually decoding the Basic Authentication header increases the risk of errors and security issues. Spring Security provides
BasicAuthenticationConverterto handle this functionality securely and efficiently.Apply this diff to use
BasicAuthenticationConverter:-import java.util.Base64; ... -private String[] extractAndDecodeHeader(String header) { - byte[] base64Token = header.substring(6).getBytes(StandardCharsets.UTF_8); - byte[] decoded; - try { - decoded = Base64.getDecoder().decode(base64Token); - } catch (IllegalArgumentException e) { - throw new BadCredentialsException("Failed to decode basic authentication token"); - } - String token = new String(decoded, StandardCharsets.UTF_8); - int delim = token.indexOf(":"); - if (delim == -1) { - throw new BadCredentialsException("Invalid basic authentication token"); - } - return new String[] { token.substring(0, delim), token.substring(delim + 1) }; -} +import org.springframework.security.web.authentication.www.BasicAuthenticationConverter; + +private UsernamePasswordAuthenticationToken extractAndDecodeHeader(HttpServletRequest request) { + BasicAuthenticationConverter converter = new BasicAuthenticationConverter(); + UsernamePasswordAuthenticationToken authRequest = converter.convert(request); + if (authRequest == null) { + throw new BadCredentialsException("Invalid Basic Authentication token"); + } + return authRequest; }Then, modify the
attemptAuthenticationmethod:... -String[] tokens = extractAndDecodeHeader(header); -String username = tokens[0]; ... -String password = tokens[1]; ... -UsernamePasswordAuthenticationToken authRequest = new UsernamePasswordAuthenticationToken(username, password); +UsernamePasswordAuthenticationToken authRequest = extractAndDecodeHeader(request); +String username = authRequest.getName(); ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
lib-spring/src/main/java/de/muenchen/oss/ezldap/spring/LdapConfiguration.java(3 hunks)microservice/pom.xml(3 hunks)microservice/src/main/java/de/muenchen/oss/ezldap/config/AppConfigurationProperties.java(1 hunks)microservice/src/main/java/de/muenchen/oss/ezldap/config/SecurityConfiguration.java(2 hunks)microservice/src/main/java/de/muenchen/oss/ezldap/config/SpringdocsSwaggerConfig.java(2 hunks)microservice/src/main/java/de/muenchen/oss/ezldap/security/BasicAuthPassthroughFilter.java(1 hunks)microservice/src/main/java/de/muenchen/oss/ezldap/security/GlobalSecurityExceptionHandler.java(1 hunks)microservice/src/test/java/de/muenchen/oss/ezldap/CorsConfigurationTest.java(2 hunks)microservice/src/test/java/de/muenchen/oss/ezldap/LdapAuthPassthroughTest.java(1 hunks)microservice/src/test/resources/application.yml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: JUnit Test Report
microservice/src/test/java/de/muenchen/oss/ezldap/LdapAuthPassthroughTest.java
[failure] 88-88: LdapAuthPassthroughTest.unauthenticated
expected: 401 UNAUTHORIZED
but was: 403 FORBIDDEN
[failure] 96-96: LdapAuthPassthroughTest.basic_auth_invalid_dn_as_username
expected: 401 UNAUTHORIZED
but was: 403 FORBIDDEN
🔇 Additional comments (16)
microservice/src/test/resources/application.yml (3)
15-17: LGTM: Appropriate logging configuration for LDAP testing
The debug logging configuration for Spring LDAP and security packages will help in troubleshooting during testing.
Line range hint 4-7: Verify CORS patterns for test environment
The CORS configuration allows specific patterns for localhost and example.org domains, which is appropriate for testing. However, we should ensure these patterns don't accidentally make it to production.
✅ Verification successful
CORS configuration is consistent across environments
The CORS patterns in the test configuration match the main application configuration, and they are appropriately restrictive:
- Only allows specific subdomains of example.org over HTTPS
- Only allows localhost access over HTTP for local development
- Port wildcards are properly constrained to these specific domains
These patterns are suitable for both test and production environments as they follow security best practices by limiting access to specific trusted domains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare test and main CORS configurations
# Look for main application.yml
echo "Checking main application.yml for CORS patterns..."
rg "allowed-origin-patterns:" "microservice/src/main"
Length of output: 162
Script:
#!/bin/bash
# Let's try a broader search for application.yml files and their content
echo "Looking for all application.yml files..."
fd application.yml
echo -e "\nChecking content of all application.yml files..."
fd application.yml --exec cat {}
Length of output: 1392
Line range hint 10-10: Verify test configuration for auth-mode
The auth-mode is set to 'none' in the test configuration. While this might be intentional for specific test cases, we should ensure there are separate test configurations that verify the new LDAP authentication mode.
✅ Verification successful
LDAP authentication mode is properly tested
The concern is unfounded as there is comprehensive test coverage for LDAP authentication:
LdapAuthPassthroughTest.javaexplicitly tests LDAP authentication mode by settingapp.auth-mode=ldapvia test properties- The test class includes multiple test cases covering:
- Unauthenticated access
- Invalid DN authentication
- Invalid password authentication
- Successful LDAP authentication
- The test uses a real LDAP container via TestContainers for integration testing
The auth-mode: none in application.yml is just the default test configuration, but it's correctly overridden in the LDAP-specific test class using @SpringBootTest properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test configurations that cover LDAP auth mode
# Look for test configurations with LDAP auth mode
echo "Searching for test configurations with LDAP auth mode..."
rg -l "auth-mode:\s*(ldap|LDAP)" "microservice/src/test"
# Look for LDAP-related test classes
echo "Searching for LDAP-related test classes..."
fd -e java -e kotlin . "microservice/src/test" --exec rg -l "(?i)ldap.*test|test.*ldap"
Length of output: 441
Script:
#!/bin/bash
# Let's examine the content of the LDAP test file and related configuration
echo "Content of LdapAuthPassthroughTest.java:"
cat "microservice/src/test/java/de/muenchen/oss/ezldap/LdapAuthPassthroughTest.java"
echo -e "\nSearching for all test configuration files:"
fd -e yml -e yaml . "microservice/src/test" --exec echo "=== {} ===" \; --exec cat {}
echo -e "\nSearching for auth-mode configurations in any YAML file:"
rg -p "auth-mode:" "microservice/src/test"
Length of output: 6581
microservice/src/main/java/de/muenchen/oss/ezldap/config/AppConfigurationProperties.java (1)
53-53: Verify enum order impact.
Adding LDAP between existing enum values could break code that relies on ordinal values. While enum ordinals shouldn't typically be used for business logic, it's worth verifying.
✅ Verification successful
Enum order change is safe to proceed
The codebase uses proper enum comparison with equals() method throughout and does not rely on ordinal values. All AuthMode comparisons are found in:
SpringdocsSwaggerConfig.java: UsesAuthMode.BASIC.equals()andAuthMode.LDAP.equals()SecurityConfiguration.java: UsesAuthMode.NONE.equals()andAuthMode.LDAP.equals()
No usage of ordinal values, arrays, or switch statements with AuthMode was found, making the enum order change safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential enum ordinal usage
# Look for .ordinal() calls or array/list indexing with AuthMode
rg -g '*.java' -A 3 'AuthMode.*\.ordinal\(\)|AuthMode\[\]|List<AuthMode>|ArrayList<AuthMode>'
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for AuthMode usage patterns
rg -g '*.java' -A 5 'AuthMode'
# Also search for array operations or comparisons that might use ordinal values indirectly
rg -g '*.java' -A 5 'switch.*AuthMode|if.*AuthMode|AuthMode.*compareTo|Arrays.*AuthMode|Collections.*AuthMode'
# Use AST to find switch statements involving AuthMode
ast-grep --pattern 'switch ($expr) { $$$ }' | rg -A 5 'AuthMode'
Length of output: 10110
microservice/src/main/java/de/muenchen/oss/ezldap/security/GlobalSecurityExceptionHandler.java (2)
35-36: Verify integration with Spring Security chain
Please ensure this exception handler is properly integrated with Spring Security:
- Verify the order of execution relative to Spring Security filters
- Confirm it doesn't interfere with Spring Security's own exception handling
Let's check the security configuration:
✅ Verification successful
Let's gather more information about the security configuration and exception handling setup:
Let's get one final piece of information about the security configuration and exception handling:
GlobalSecurityExceptionHandler is properly integrated with Spring Security
The exception handler is correctly implemented and won't interfere with Spring Security's chain because:
- It only handles LDAP-specific AuthenticationException (error code 49 and Invalid Credentials)
- The security configuration uses stateless sessions and properly configures the filter chain
- The handler is placed after security filters as it's a @ControllerAdvice, which processes exceptions after the security chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Spring Security configuration
ast-grep --pattern 'class $_ extends WebSecurityConfigurerAdapter {
$$$
}'
# Check for other security-related configurations
rg -t java "SecurityFilterChain|@SecurityScheme|@EnableWebSecurity" --no-filename
Length of output: 348
Script:
#!/bin/bash
# Get the SecurityFilterChain configuration
ast-grep --pattern 'SecurityFilterChain $_($$$) {
$$$
}'
# Check for exception handling order configuration
rg -t java "@Order|Ordered" -A 2
# Find usage of our GlobalSecurityExceptionHandler
rg -t java "GlobalSecurityExceptionHandler" -A 2
Length of output: 529
Script:
#!/bin/bash
# Get the full content of GlobalSecurityExceptionHandler
cat microservice/src/main/java/de/muenchen/oss/ezldap/security/GlobalSecurityExceptionHandler.java
# Get the security configuration class content
rg -t java "SecurityFilterChain securityFilterChain" -A 20
Length of output: 5456
38-38: Consider handling additional security exceptions
The current implementation only handles javax.naming.AuthenticationException. Consider adding handlers for other relevant security exceptions:
org.springframework.ldap.AuthenticationExceptionorg.springframework.security.core.AuthenticationExceptionorg.springframework.security.access.AccessDeniedException
Let's check if these exceptions are used in the codebase:
microservice/src/test/java/de/muenchen/oss/ezldap/CorsConfigurationTest.java (1)
51-51: LGTM! Class name and visibility changes improve clarity.
The rename from AppTest to CorsConfigurationTest better reflects the test's purpose, and package-private visibility is appropriate for test classes.
lib-spring/src/main/java/de/muenchen/oss/ezldap/spring/LdapConfiguration.java (2)
60-63: LGTM! Well-structured bean configuration
The changes improve clarity and maintainability by:
- Using explicit bean naming
- Properly qualifying dependencies
- Following Spring best practices for dependency injection
66-73: Verify LDAP search base configuration usage
The implementation looks good with proper dependency qualification. Let's verify the usage of LDAP search base configurations across the codebase to ensure consistency.
✅ Verification successful
LDAP search base configuration is properly used
The verification shows that:
- Search base configurations are only used in
LdapConfiguration.javathrough the properties object, maintaining consistent configuration-driven approach - All other LDAP paths found are in test files (
LdapAuthPassthroughTest.java,LdapServiceIntegrationTest.java) where hardcoded values are expected - The core
LdapService.javacorrectly uses the injected search base configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of LDAP search base configurations
# Check for other uses of search base configurations
rg -A 2 "getUserSearchBase|getOuSearchBase" --type java
# Check for any hardcoded LDAP search paths that should use configuration
rg -A 2 "dc=|ou=" --type java
Length of output: 4586
microservice/src/main/java/de/muenchen/oss/ezldap/config/SpringdocsSwaggerConfig.java (2)
52-53: LGTM!
The reformatting improves readability while maintaining the same functionality.
52-53: Verify API documentation completeness
Let's ensure the API documentation properly reflects the LDAP authentication requirements.
Also applies to: 76-79
✅ Verification successful
Let me check the security configuration mentioned in the README to verify the LDAP authentication documentation.
API documentation properly reflects LDAP authentication
The OpenAPI/Swagger configuration already includes comprehensive authentication documentation:
- Basic authentication scheme is properly configured
- LDAP authentication mode is explicitly documented with a dedicated security scheme and description
- Global response codes for authentication (401, 403) are included
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if LDAP authentication is properly documented
# Search for API documentation or README files
fd -e md -e adoc . | while read -r file; do
echo "=== Checking $file ==="
rg -A 5 -i "authentication|ldap|auth" "$file"
done
Length of output: 4188
Script:
#!/bin/bash
# Check the security configuration and API documentation
echo "=== Checking SecurityConfiguration.java ==="
rg -A 10 "class SecurityConfiguration" microservice/src/main/java/de/muenchen/oss/ezldap/config/SecurityConfiguration.java
echo -e "\n=== Checking OpenAPI annotations for auth ==="
ast-grep --pattern 'annotation { "@$_" }' microservice/src/main/java/de/muenchen/oss/ezldap/spring/rest/v1/
echo -e "\n=== Checking Swagger customizations ==="
rg -A 5 "OpenApiCustomizer|SecurityScheme|SecurityRequirement" microservice/src/main/java/de/muenchen/oss/ezldap/config/
Length of output: 6747
microservice/pom.xml (2)
83-87: LGTM! Test dependency properly scoped.
The addition of spring-boot-testcontainers with test scope is appropriate for integration testing with LDAP.
66-69: LGTM! Verify LDAP dependency compatibility.
The addition of spring-security-ldap dependency is essential for implementing LDAP authentication support.
Let's verify the dependency compatibility and check for any known vulnerabilities:
✅ Verification successful
LGTM! Spring Security LDAP dependency is properly integrated.
The codebase shows proper integration of the LDAP dependency:
- Security configuration includes LDAP authentication mode with appropriate beans
- Uses Spring Boot 3.3.0 which is compatible with spring-security-ldap
- Implements proper LDAP context source configuration
- Contains necessary security filter chain setup for LDAP authentication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and compatibility issues with spring-security-ldap
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "spring-security-ldap") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check effective version and its compatibility
mvn dependency:tree -Dincludes=org.springframework.security:spring-security-ldap
Length of output: 532
Script:
#!/bin/bash
# Let's try a different approach to verify LDAP dependency usage and compatibility
# Check parent POM for spring boot version and other version management
fd "pom.xml" --exec grep -A 5 -B 5 "<parent>"
# Find LDAP configuration and usage in the codebase
rg -l "LdapAuthentication|LdapAuthenticator|LdapAuthenticationProvider|@EnableLdapRepositories" --type java
# Look for LDAP related beans or configuration classes
ast-grep --pattern 'class $_ implements LdapAuthenticationProvider { $$$ }'
ast-grep --pattern '@Configuration
class $_ {
$$$
@Bean
$$$LdapAuthentication$$$
$$$
}'
Length of output: 2451
Script:
#!/bin/bash
# Let's check for LDAP configuration files and Spring Security setup
# Look for LDAP properties in configuration files
rg -l "ldap\.|spring\.ldap\.|spring\.security\.ldap" --type properties --type yaml
# Search for Spring Security configuration classes
rg -l "WebSecurityConfigurerAdapter|SecurityFilterChain|@EnableWebSecurity" --type java
# Check for any LDAP-related test files
fd -e java -e groovy -e kotlin test --exec grep -l "LDAP|ldap" {}
Length of output: 363
Script:
#!/bin/bash
# Let's examine the security configuration and application properties
# Check the content of SecurityConfiguration.java
cat microservice/src/main/java/de/muenchen/oss/ezldap/config/SecurityConfiguration.java
# Look for application properties/yaml files with different extensions
fd -e yml -e yaml -e properties --exec grep -l "ldap"
# Check for any application.yml or application.yaml
fd "application.yml|application.yaml" --exec cat {}
Length of output: 9044
microservice/src/test/java/de/muenchen/oss/ezldap/LdapAuthPassthroughTest.java (3)
43-49: LGTM! Well-structured test configuration.
The test configuration is properly set up with:
- Appropriate LDAP properties
- Random port allocation for test isolation
- Clear search base configurations
63-76: Verify test resource paths and consider environment variable handling.
While the container configuration is generally good, there are a few considerations:
- The hardcoded paths to test resources might break if the project structure changes
- Sensitive credentials are exposed in environment variables
Let's verify the test resources exist:
✅ Verification successful
Test resources are properly configured and accessible
The verification shows that both required resources exist and are accessible:
- Schema file:
lib-core/src/test/resources/ldap/schema/lhm.schema - Data directory:
lib-core/src/test/resources/ldap/datacontainingdata.ldif
The paths in the test configuration are correct and match the actual project structure. While the credentials are exposed in environment variables, this is acceptable for test containers as they are only used in the test environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the LDAP schema and data files exist
fd "lhm.schema" --type f
fd "data" --type d --base-directory "../lib-core/src/test/resources/ldap/"
Length of output: 247
Script:
#!/bin/bash
# Let's verify the directory structure and data files
fd "data" --type d
ls -la lib-core/src/test/resources/ldap/data 2>/dev/null || echo "Data directory not found"
Length of output: 274
84-89:
Fix failing authentication tests and verify security configuration.
The test failures indicate a security configuration issue:
- Tests expect 401 UNAUTHORIZED but receive 403 FORBIDDEN
- This suggests the authentication filter might be processing the request incorrectly
This needs to be addressed as it affects the core authentication behavior.
Let's verify the security configuration:
Also applies to: 91-97
🧰 Tools
🪛 GitHub Check: JUnit Test Report
[failure] 88-88: LdapAuthPassthroughTest.unauthenticated
expected: 401 UNAUTHORIZED
but was: 403 FORBIDDEN
| @ExceptionHandler(AuthenticationException.class) | ||
| public ResponseEntity<Object> handleAuthenticationException(AuthenticationException ex) { | ||
| String message = ex.getMessage(); | ||
| if (message.contains("error code 49") || message.contains("Invalid Credentials")) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); | ||
| } | ||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling robustness and security
The current implementation has several areas for improvement:
- String matching of error messages is fragile and could break if LDAP error messages change
- No logging of authentication failures for security monitoring
- No structured error response to help API clients
Consider implementing these improvements:
@ExceptionHandler(AuthenticationException.class)
public ResponseEntity<Object> handleAuthenticationException(AuthenticationException ex) {
+ // Log authentication failure with request details but without credentials
+ log.warn("Authentication failed: {}", ex.getMessage());
+
String message = ex.getMessage();
- if (message.contains("error code 49") || message.contains("Invalid Credentials")) {
+ // Use enum or constants for LDAP error codes
+ if (isInvalidCredentialsError(message)) {
+ // Return structured error response
+ ErrorResponse error = new ErrorResponse(
+ "AUTHENTICATION_FAILED",
+ "Invalid credentials provided"
+ );
- return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
+ return ResponseEntity
+ .status(HttpStatus.UNAUTHORIZED)
+ .body(error);
}
- return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build();
+ // Return generic error for other authentication issues
+ ErrorResponse error = new ErrorResponse(
+ "AUTHENTICATION_ERROR",
+ "An authentication error occurred"
+ );
+ return ResponseEntity
+ .status(HttpStatus.INTERNAL_SERVER_ERROR)
+ .body(error);
}
+
+ private boolean isInvalidCredentialsError(String message) {
+ return message.contains(LdapErrorCodes.INVALID_CREDENTIALS) ||
+ message.contains(LdapErrorCodes.ERROR_CODE_49);
+ }Don't forget to add these imports:
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import lombok.extern.slf4j.Slf4j; // If using Lombok| .components(new Components().addSecuritySchemes("basicAuth", | ||
| new SecurityScheme().type(SecurityScheme.Type.HTTP).scheme("basic"))); | ||
| } | ||
| if (AuthMode.LDAP.equals(appProps.getAuthMode())) { | ||
| openApi.addSecurityItem(new SecurityRequirement().addList("basicAuthLdap")) | ||
| .components(new Components().addSecuritySchemes("basicAuthLdap", | ||
| new SecurityScheme().type(SecurityScheme.Type.HTTP).scheme("basic").description("Authenticate using LDAP"))); |
There was a problem hiding this comment.
💡 Codebase verification
Add handling for NONE and default auth modes in security configuration
The verification confirms that AuthMode enum has three values: NONE, LDAP, and BASIC, but the SpringdocsSwaggerConfig only handles BASIC and LDAP explicitly. The NONE mode and potential unknown modes (through enum extension) are not handled. Add explicit handling:
microservice/src/main/java/de/muenchen/oss/ezldap/config/SpringdocsSwaggerConfig.java: Add an else branch after the BASIC and LDAP checks to handle NONE and any future auth modes, ensuring security configuration is appropriately set or explicitly disabled.
🔗 Analysis chain
Consider handling default/unknown auth modes
The current implementation only handles BASIC and LDAP modes explicitly. Consider adding a default case to maintain security when an unexpected auth mode is configured.
Let's verify all possible auth modes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all possible AuthMode enum values to ensure complete coverage
# Search for AuthMode enum definition
ast-grep --pattern 'enum AuthMode {
$$$
}'
Length of output: 46
Script:
#!/bin/bash
# Let's try a different approach to find the AuthMode enum using ripgrep
rg "enum.*AuthMode" -A 5
Length of output: 637
Script:
#!/bin/bash
# Let's check how AuthMode is handled in SpringdocsSwaggerConfig
rg "AuthMode" microservice/src/main/java/de/muenchen/oss/ezldap/config/SpringdocsSwaggerConfig.java -B 2 -A 2
Length of output: 951
| protected void successfulAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain, | ||
| Authentication authResult) throws IOException, ServletException { | ||
|
|
||
| SecurityContextHolder.getContext().setAuthentication(authResult); | ||
| chain.doFilter(request, response); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the security context is cleared upon authentication failure
If an exception occurs during chain.doFilter(request, response);, the security context might retain the previous authentication. Consider adding a finally block to clear the security context to prevent potential security issues.
Apply this diff to clear the security context on failure:
@Override
protected void successfulAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain,
Authentication authResult) throws IOException, ServletException {
SecurityContextHolder.getContext().setAuthentication(authResult);
try {
chain.doFilter(request, response);
} finally {
SecurityContextHolder.clearContext();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected void successfulAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain, | |
| Authentication authResult) throws IOException, ServletException { | |
| SecurityContextHolder.getContext().setAuthentication(authResult); | |
| chain.doFilter(request, response); | |
| } | |
| protected void successfulAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain, | |
| Authentication authResult) throws IOException, ServletException { | |
| SecurityContextHolder.getContext().setAuthentication(authResult); | |
| try { | |
| chain.doFilter(request, response); | |
| } finally { | |
| SecurityContextHolder.clearContext(); | |
| } | |
| } |
| public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response) { | ||
|
|
||
| String header = request.getHeader("Authorization"); | ||
|
|
||
| if (header != null && header.startsWith("Basic ")) { | ||
| String[] tokens = extractAndDecodeHeader(header); | ||
| String username = tokens[0]; | ||
| // Validate username as a valid DN | ||
| try { | ||
| new LdapName(username); | ||
| } catch (InvalidNameException e) { | ||
| throw new BadCredentialsException("Invalid username format: " + e.getMessage(), e); | ||
| } | ||
| String password = tokens[1]; | ||
|
|
||
| UsernamePasswordAuthenticationToken authRequest = new UsernamePasswordAuthenticationToken(username, password); | ||
| return getAuthenticationManager().authenticate(authRequest); | ||
| } else { | ||
| throw new BadCredentialsException("Missing basic auth header"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle missing or invalid Authorization header appropriately
Throwing a BadCredentialsException when the Authorization header is missing may not be the best approach. Instead, consider initiating the authentication entry point to prompt for credentials without exposing the internal exception.
Apply this diff to handle the missing header:
-} else {
- throw new BadCredentialsException("Missing basic auth header");
-}
+} else {
+ authenticationEntryPoint.commence(request, response, new InsufficientAuthenticationException("Authorization header missing"));
+ return null;
+}Ensure that an AuthenticationEntryPoint is configured to handle unauthorized access responses appropriately.
Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| new LdapName(username); | ||
| } catch (InvalidNameException e) { | ||
| throw new BadCredentialsException("Invalid username format: " + e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
Avoid exposing internal error details in exceptions
Throwing a BadCredentialsException with the underlying exception message may reveal sensitive information. It's safer to provide a generic error message to the client and log the detailed exception on the server side.
Apply this diff to modify the exception handling:
try {
new LdapName(username);
} catch (InvalidNameException e) {
- throw new BadCredentialsException("Invalid username format: " + e.getMessage(), e);
+ logger.warn("Invalid username format for user DN", e);
+ throw new BadCredentialsException("Invalid credentials");
}Committable suggestion skipped: line range outside the PR's diff.
| @Bean | ||
| @ConditionalOnProperty(name = "app.auth-mode", havingValue = "ldap") | ||
| AuthenticationManager authenticationManager() { | ||
| ProviderManager providerManager = new ProviderManager(Collections.singletonList(new AuthenticationProvider() { | ||
|
|
||
| @Override | ||
| public boolean supports(Class<?> authentication) { | ||
| return UsernamePasswordAuthenticationToken.class.isAssignableFrom(authentication); | ||
| } | ||
|
|
||
| @Override | ||
| public Authentication authenticate(Authentication authentication) throws AuthenticationException { | ||
| UsernamePasswordAuthenticationToken token = (UsernamePasswordAuthenticationToken) authentication; | ||
| return new UsernamePasswordAuthenticationToken(token.getPrincipal(), token.getCredentials(), Collections.emptyList()); | ||
| } | ||
| })); | ||
| providerManager.setEraseCredentialsAfterAuthentication(false); | ||
| return providerManager; | ||
| } |
There was a problem hiding this comment.
Security Vulnerability: Authentication provider does not validate credentials
The AuthenticationProvider in the authenticationManager() method returns an authenticated UsernamePasswordAuthenticationToken without verifying the user's credentials. This means any username and password combination will be accepted, posing a critical security risk.
Proposed Fix: Implement LDAP authentication using Spring Security
Utilize Spring Security's LDAP authentication mechanisms to validate user credentials against your LDAP server. Here's how you can modify the authenticationManager() method:
@Bean
@ConditionalOnProperty(name = "app.auth-mode", havingValue = "ldap")
-AuthenticationManager authenticationManager() {
- ProviderManager providerManager = new ProviderManager(Collections.singletonList(new AuthenticationProvider() {
-
- @Override
- public boolean supports(Class<?> authentication) {
- return UsernamePasswordAuthenticationToken.class.isAssignableFrom(authentication);
- }
-
- @Override
- public Authentication authenticate(Authentication authentication) throws AuthenticationException {
- UsernamePasswordAuthenticationToken token = (UsernamePasswordAuthenticationToken) authentication;
- return new UsernamePasswordAuthenticationToken(token.getPrincipal(), token.getCredentials(), Collections.emptyList());
- }
- }));
- providerManager.setEraseCredentialsAfterAuthentication(false);
- return providerManager;
+AuthenticationManager authenticationManager(LdapContextSource contextSource) {
+ BindAuthenticator bindAuthenticator = new BindAuthenticator(contextSource);
+ // Configure search settings if necessary
+ LdapAuthenticationProvider ldapAuthenticationProvider = new LdapAuthenticationProvider(bindAuthenticator);
+ ProviderManager providerManager = new ProviderManager(Collections.singletonList(ldapAuthenticationProvider));
+ return providerManager;
}- Explanation:
- BindAuthenticator: Authenticates users by binding to the LDAP directory with the provided credentials.
- LdapAuthenticationProvider: Processes authentication requests using the
BindAuthenticator. - Configure additional settings like user DN patterns or search filters if required.
Committable suggestion skipped: line range outside the PR's diff.
| SecurityFilterChain securityFilterChain(HttpSecurity http, EzLdapConfigurationProperties configProps, | ||
| AppConfigurationProperties appProps, @Autowired(required = false) AuthenticationManager authenticationManager) throws Exception { | ||
| if (AuthMode.NONE.equals(appProps.getAuthMode())) { | ||
| log.info("Bootstrapping Spring Security filter chain for auth-mode 'none' ..."); | ||
| http | ||
| .authorizeHttpRequests(authz -> authz.anyRequest().permitAll()); | ||
| http.sessionManagement(sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | ||
| http.authorizeHttpRequests(authz -> authz.anyRequest().permitAll()); | ||
| http.sessionManagement( | ||
| sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | ||
| } else if (AuthMode.LDAP.equals(appProps.getAuthMode())) { | ||
| log.info("Bootstrapping Spring Security filter chain for auth-mode 'ldap' ..."); | ||
| configureMatchers(http); | ||
| http.sessionManagement( | ||
| sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | ||
| http.addFilterBefore(new BasicAuthPassthroughFilter(authenticationManager, "/v1/**"), AnonymousAuthenticationFilter.class); |
There was a problem hiding this comment.
Ensure AuthenticationManager is properly injected for LDAP authentication
In the securityFilterChain method, during LDAP authentication mode, the authenticationManager is essential for adding the BasicAuthPassthroughFilter. However, it is autowired with required = false, which may result in a null value if no AuthenticationManager bean is available. This could lead to a NullPointerException at runtime.
Suggested Improvement:
- Remove
required = falseto ensureAuthenticationManageris always injected when LDAP authentication is enabled. - Ensure that an
AuthenticationManagerbean is defined and available in the application context whenapp.auth-modeis set toldap.
Apply this change to enforce the presence of AuthenticationManager:
SecurityFilterChain securityFilterChain(HttpSecurity http, EzLdapConfigurationProperties configProps,
- AppConfigurationProperties appProps, @Autowired(required = false) AuthenticationManager authenticationManager) throws Exception {
+ AppConfigurationProperties appProps, AuthenticationManager authenticationManager) throws Exception {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SecurityFilterChain securityFilterChain(HttpSecurity http, EzLdapConfigurationProperties configProps, | |
| AppConfigurationProperties appProps, @Autowired(required = false) AuthenticationManager authenticationManager) throws Exception { | |
| if (AuthMode.NONE.equals(appProps.getAuthMode())) { | |
| log.info("Bootstrapping Spring Security filter chain for auth-mode 'none' ..."); | |
| http | |
| .authorizeHttpRequests(authz -> authz.anyRequest().permitAll()); | |
| http.sessionManagement(sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | |
| http.authorizeHttpRequests(authz -> authz.anyRequest().permitAll()); | |
| http.sessionManagement( | |
| sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | |
| } else if (AuthMode.LDAP.equals(appProps.getAuthMode())) { | |
| log.info("Bootstrapping Spring Security filter chain for auth-mode 'ldap' ..."); | |
| configureMatchers(http); | |
| http.sessionManagement( | |
| sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | |
| http.addFilterBefore(new BasicAuthPassthroughFilter(authenticationManager, "/v1/**"), AnonymousAuthenticationFilter.class); | |
| SecurityFilterChain securityFilterChain(HttpSecurity http, EzLdapConfigurationProperties configProps, | |
| AppConfigurationProperties appProps, AuthenticationManager authenticationManager) throws Exception { | |
| if (AuthMode.NONE.equals(appProps.getAuthMode())) { | |
| log.info("Bootstrapping Spring Security filter chain for auth-mode 'none' ..."); | |
| http.authorizeHttpRequests(authz -> authz.anyRequest().permitAll()); | |
| http.sessionManagement( | |
| sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | |
| } else if (AuthMode.LDAP.equals(appProps.getAuthMode())) { | |
| log.info("Bootstrapping Spring Security filter chain for auth-mode 'ldap' ..."); | |
| configureMatchers(http); | |
| http.sessionManagement( | |
| sessionManagement -> sessionManagement.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); | |
| http.addFilterBefore(new BasicAuthPassthroughFilter(authenticationManager, "/v1/**"), AnonymousAuthenticationFilter.class); |
| @Bean(name = "ezldapQueryContextSource") | ||
| @ConditionalOnProperty(name = "app.auth-mode", havingValue = "ldap") | ||
| LdapContextSource ezldapQueryContextSource(final EzLdapConfigurationProperties props) { | ||
| final LdapContextSource contextSource = new LdapContextSource(); | ||
| contextSource.setUrl(props.getLdap().getUrl()); | ||
| contextSource.setAuthenticationSource(new AuthenticationSource() { | ||
|
|
||
| @Override | ||
| public String getPrincipal() { | ||
| UsernamePasswordAuthenticationToken auth = (UsernamePasswordAuthenticationToken) SecurityContextHolder.getContext().getAuthentication(); | ||
| return auth.getPrincipal().toString(); | ||
| } | ||
|
|
||
| @Override | ||
| public String getCredentials() { | ||
| UsernamePasswordAuthenticationToken auth = (UsernamePasswordAuthenticationToken) SecurityContextHolder.getContext().getAuthentication(); | ||
| return auth.getCredentials().toString(); | ||
| } | ||
| }); | ||
| log.info( | ||
| "Initiating LDAP context-source with url='{}' and SpringSecurityAuthenticationSource for Web LDAP authentication credentials passthrough to LDAP queries.", | ||
| props.getLdap().getUrl()); | ||
| return contextSource; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential NullPointerException in AuthenticationSource
The custom AuthenticationSource in the ezldapQueryContextSource bean retrieves the authentication from the SecurityContextHolder. If the security context does not contain an authentication (e.g., if the user is not authenticated), calling getPrincipal() or getCredentials() will result in a NullPointerException.
Suggested Improvement:
Add null checks and type checks to safely handle missing or invalid authentication:
contextSource.setAuthenticationSource(new AuthenticationSource() {
@Override
public String getPrincipal() {
- UsernamePasswordAuthenticationToken auth = (UsernamePasswordAuthenticationToken) SecurityContextHolder.getContext().getAuthentication();
- return auth.getPrincipal().toString();
+ Authentication auth = SecurityContextHolder.getContext().getAuthentication();
+ if (auth instanceof UsernamePasswordAuthenticationToken) {
+ Object principal = auth.getPrincipal();
+ return principal != null ? principal.toString() : null;
+ } else {
+ log.warn("No authentication principal found in SecurityContext");
+ return null;
+ }
}
@Override
public String getCredentials() {
- UsernamePasswordAuthenticationToken auth = (UsernamePasswordAuthenticationToken) SecurityContextHolder.getContext().getAuthentication();
- return auth.getCredentials().toString();
+ Authentication auth = SecurityContextHolder.getContext().getAuthentication();
+ if (auth instanceof UsernamePasswordAuthenticationToken) {
+ Object credentials = auth.getCredentials();
+ return credentials != null ? credentials.toString() : null;
+ } else {
+ log.warn("No authentication credentials found in SecurityContext");
+ return null;
+ }
}
});- Explanation:
- Checks if
authis an instance ofUsernamePasswordAuthenticationToken. - Logs a warning if authentication is missing or of an unexpected type.
- Prevents
NullPointerExceptionby handlingnullvalues gracefully.
- Checks if
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Bean(name = "ezldapQueryContextSource") | |
| @ConditionalOnProperty(name = "app.auth-mode", havingValue = "ldap") | |
| LdapContextSource ezldapQueryContextSource(final EzLdapConfigurationProperties props) { | |
| final LdapContextSource contextSource = new LdapContextSource(); | |
| contextSource.setUrl(props.getLdap().getUrl()); | |
| contextSource.setAuthenticationSource(new AuthenticationSource() { | |
| @Override | |
| public String getPrincipal() { | |
| UsernamePasswordAuthenticationToken auth = (UsernamePasswordAuthenticationToken) SecurityContextHolder.getContext().getAuthentication(); | |
| return auth.getPrincipal().toString(); | |
| } | |
| @Override | |
| public String getCredentials() { | |
| UsernamePasswordAuthenticationToken auth = (UsernamePasswordAuthenticationToken) SecurityContextHolder.getContext().getAuthentication(); | |
| return auth.getCredentials().toString(); | |
| } | |
| }); | |
| log.info( | |
| "Initiating LDAP context-source with url='{}' and SpringSecurityAuthenticationSource for Web LDAP authentication credentials passthrough to LDAP queries.", | |
| props.getLdap().getUrl()); | |
| return contextSource; | |
| } | |
| @Bean(name = "ezldapQueryContextSource") | |
| @ConditionalOnProperty(name = "app.auth-mode", havingValue = "ldap") | |
| LdapContextSource ezldapQueryContextSource(final EzLdapConfigurationProperties props) { | |
| final LdapContextSource contextSource = new LdapContextSource(); | |
| contextSource.setUrl(props.getLdap().getUrl()); | |
| contextSource.setAuthenticationSource(new AuthenticationSource() { | |
| @Override | |
| public String getPrincipal() { | |
| Authentication auth = SecurityContextHolder.getContext().getAuthentication(); | |
| if (auth instanceof UsernamePasswordAuthenticationToken) { | |
| Object principal = auth.getPrincipal(); | |
| return principal != null ? principal.toString() : null; | |
| } else { | |
| log.warn("No authentication principal found in SecurityContext"); | |
| return null; | |
| } | |
| } | |
| @Override | |
| public String getCredentials() { | |
| Authentication auth = SecurityContextHolder.getContext().getAuthentication(); | |
| if (auth instanceof UsernamePasswordAuthenticationToken) { | |
| Object credentials = auth.getCredentials(); | |
| return credentials != null ? credentials.toString() : null; | |
| } else { | |
| log.warn("No authentication credentials found in SecurityContext"); | |
| return null; | |
| } | |
| } | |
| }); | |
| log.info( | |
| "Initiating LDAP context-source with url='{}' and SpringSecurityAuthenticationSource for Web LDAP authentication credentials passthrough to LDAP queries.", | |
| props.getLdap().getUrl()); | |
| return contextSource; | |
| } |
Description
Adds a new configuration option
ldapforapp.auth-mode.This auth-mode essentially forces API clients to pass valid credentials constisting of
userDn/passwordof an LDAP user as HTTP Basic auth, e.g.: username =cn=admin,dc=example,dc=org/ password =s3cretThose credentials are then used for the LdapContextSource/LdapTemplate of Spring LDAP.
Reference
Issues #4
Summary by CodeRabbit
Release Notes
New Features
BasicAuthPassthroughFilterfor handling Basic Authentication.Improvements
Bug Fixes
Tests