You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by "abhiramaab (via GitHub)" <gi...@apache.org> on 2026/08/12 06:49:12 UTC
[PR] Fix Active Directory DN authentication [shiro]
abhiramaab opened a new pull request, #2854:
URL: https://github.com/apache/shiro/pull/2854
## Description
Fixes Active Directory authentication when the supplied username is already a distinguished name (DN).
The authentication path now preserves valid LDAP DNs instead of escaping them and appending the configured principal suffix.
Normal usernames continue to use the existing `Rdn.escapeValue()` handling.
## Tests
Added regression tests covering:
- authentication with an existing LDAP DN
- authentication with a regular username requiring LDAP escaping
- LDAP authentication using the resolved username
All tests pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [#2841] Fix Active Directory DN authentication [shiro]
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak merged PR #2854:
URL: https://github.com/apache/shiro/pull/2854
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [#2841] Fix Active Directory DN authentication [shiro]
Posted by "steinarb (via GitHub)" <gi...@apache.org>.
steinarb commented on code in PR #2854:
URL: https://github.com/apache/shiro/pull/2854#discussion_r3781312270
##########
core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java:
##########
@@ -235,6 +236,43 @@ protected Collection<String> getRoleNamesForGroups(Collection<String> groupNames
return roleNames;
}
+ /**
+ * Returns the username to use for authentication.
+ * If {@link #principalSuffix} is configured, the sanitized username with appended suffix will be returned.
+ * If {@link #principalSuffix} is not configured, the method will check if the username is a valid LDAP DN.
+ * If it is a valid LDAP DN, the username will be returned as-is,
+ * otherwise the sanitized username with appended suffix will be returned.
+ *
+ * @param username input to check and sanitize
+ * @return the sanitized username with optional suffix to use for authentication
+ */
+ protected String getUsernameWithSuffixOrFullDN(String username) {
+ if (!StringUtils.hasText(principalSuffix)) {
+ try {
+ LdapName ldapName = new LdapName(username);
+ // Full LDAP DN needs to have more than one RDN, so we can return the username as-is
+ if (ldapName.size() > 1) {
+ return username;
+ }
+ } catch (javax.naming.InvalidNameException e) {
+ // Not a valid LDAP DN, so treat it as a regular username.
+ }
+ }
+
+ return getUsernameWithSuffix(username);
+ }
+
+ /**
+ * Returns the sanitized username with appended suffix if {@link #principalSuffix} is configured
+ * and the username does not already end with it.
+ * If {@link #principalSuffix} is not configured, the sanitized username will be returned
+ * <p>
+ * NOTE: {@link #getUsernameWithSuffixOrFullDN(String)} should be used instead of this method
+ * to handle full LDAP DNs correctly.
Review Comment:
@lprimak Out of curiousity: why keep getUsernameWithSuffix as a non-private method with javadoc if getUsernameWithSuffixOrFullDN is the one that should be used? Is getUsernameWithSuffix used elsewhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Active Directory DN authentication [shiro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2854:
URL: https://github.com/apache/shiro/pull/2854#discussion_r3764217760
##########
core/src/test/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealmTest.java:
##########
@@ -149,6 +152,42 @@ void testInitialization() {
SecurityUtils.setSecurityManager(null);
}
+ @Test
+ void testUsernameForAuthenticationWithDn() {
+ ActiveDirectoryRealm activeDirectoryRealm = new ActiveDirectoryRealm();
Review Comment:
This DN-preservation test currently exercises the default realm where `principalSuffix` is null. Since the reported bug involves escaping/appending the configured suffix, it would be good to set a non-null `principalSuffix` here to ensure DN inputs are still preserved even when suffixing is enabled.
This issue also appears on line 182 of the same file.
##########
core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java:
##########
@@ -109,7 +110,7 @@ protected AuthenticationInfo queryForAuthenticationInfo(AuthenticationToken toke
// Binds using the username and password provided by the user.
LdapContext ctx = null;
try {
- ctx = ldapContextFactory.getLdapContext(getUsernameWithSuffix(upToken.getUsername()),
+ ctx = ldapContextFactory.getLdapContext(getUsernameForAuthentication(upToken.getUsername()),
String.valueOf(upToken.getPassword()));
Review Comment:
`queryForAuthenticationInfo` now binds with `getUsernameForAuthentication(...)`, but DN-preserving logic is not applied when resolving group memberships: `queryForAuthorizationInfo` passes the (raw) principal into `getRoleNamesForUser`, which still calls `getUsernameWithSuffix(username)`.
If a user authenticates with a DN while `principalSuffix` is configured, the authorization path will escape/append the suffix to the DN and the LDAP search will likely fail (roles won’t resolve for DN-authenticated users). Consider reusing the same DN-detection logic when building search arguments (or otherwise ensure the resolved/bind username is used consistently for both authentication and authorization).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Active Directory DN authentication [shiro]
Posted by "boris-petrov (via GitHub)" <gi...@apache.org>.
boris-petrov commented on code in PR #2854:
URL: https://github.com/apache/shiro/pull/2854#discussion_r3770083782
##########
core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java:
##########
@@ -237,10 +238,24 @@ protected Collection<String> getRoleNamesForGroups(Collection<String> groupNames
protected String getUsernameWithSuffix(String username) {
String sanitizedUsername = Rdn.escapeValue(username);
if (principalSuffix != null
- && !sanitizedUsername.toLowerCase(Locale.ROOT).endsWith(principalSuffix.toLowerCase(Locale.ROOT))) {
+ && !sanitizedUsername.toLowerCase(Locale.ROOT)
Review Comment:
This whitespace-only change is unrelated so please revert it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Active Directory DN authentication [shiro]
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on PR #2854:
URL: https://github.com/apache/shiro/pull/2854#issuecomment-5288310861
Looks good. I have pushed an additional commit into your branch to add some comments / renamed methods and fixed tests. I also made full-DN passthrough more strict and made it contingent of null `principalSuffix` configuration.
Please check the appropriate box to attribute license and copyright to Apache.
Thank you for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [#2841] Fix Active Directory DN authentication [shiro]
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #2854:
URL: https://github.com/apache/shiro/pull/2854#discussion_r3781325413
##########
core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java:
##########
@@ -235,6 +236,43 @@ protected Collection<String> getRoleNamesForGroups(Collection<String> groupNames
return roleNames;
}
+ /**
+ * Returns the username to use for authentication.
+ * If {@link #principalSuffix} is configured, the sanitized username with appended suffix will be returned.
+ * If {@link #principalSuffix} is not configured, the method will check if the username is a valid LDAP DN.
+ * If it is a valid LDAP DN, the username will be returned as-is,
+ * otherwise the sanitized username with appended suffix will be returned.
+ *
+ * @param username input to check and sanitize
+ * @return the sanitized username with optional suffix to use for authentication
+ */
+ protected String getUsernameWithSuffixOrFullDN(String username) {
+ if (!StringUtils.hasText(principalSuffix)) {
+ try {
+ LdapName ldapName = new LdapName(username);
+ // Full LDAP DN needs to have more than one RDN, so we can return the username as-is
+ if (ldapName.size() > 1) {
+ return username;
+ }
+ } catch (javax.naming.InvalidNameException e) {
+ // Not a valid LDAP DN, so treat it as a regular username.
+ }
+ }
+
+ return getUsernameWithSuffix(username);
+ }
+
+ /**
+ * Returns the sanitized username with appended suffix if {@link #principalSuffix} is configured
+ * and the username does not already end with it.
+ * If {@link #principalSuffix} is not configured, the sanitized username will be returned
+ * <p>
+ * NOTE: {@link #getUsernameWithSuffixOrFullDN(String)} should be used instead of this method
+ * to handle full LDAP DNs correctly.
Review Comment:
Simply because it was part of the API for a long time. Who knows how users are using it?
In order to remove an API, we would have to deprecated it for one major release at least,
and then remove it in another (today, it would be 5.0) so it better be worth it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org