Skip to content

[#2308] Adds setPrincipalSuffix() to AbstractLdapRealm & updates ActiveDirectoryRealm #2309

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[#2308] Adds setPrincipalSuffix() to AbstractLdapRealm & updates ActiveDirectoryRealm #2309

wants to merge 1 commit into from

Conversation

tbrugz
Copy link
Contributor

@tbrugz tbrugz commented Oct 14, 2025 •
edited
Loading

Fixes #2308

lprimak
lprimak requested changes Oct 15, 2025
View reviewed changes
Copy link
Contributor

@lprimak lprimak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some code suggestions added (to pass the build)
Rest LGTM.

core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java
LdapContext ctx = null;
try {
ctx = ldapContextFactory.getLdapContext(upToken.getUsername(), String.valueOf(upToken.getPassword()));
ctx = ldapContextFactory.getLdapContext(getUsernameWithSuffix(upToken.getUsername()), String.valueOf(upToken.getPassword()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ctx = ldapContextFactory.getLdapContext(getUsernameWithSuffix(upToken.getUsername()), String.valueOf(upToken.getPassword()));
ctx = ldapContextFactory.getLdapContext(getUsernameWithSuffix(upToken.getUsername()),
String.valueOf(upToken.getPassword()));

core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java
protected String getUsernameWithSuffix(String username) {
if (principalSuffix != null
&& !username.toLowerCase(Locale.ROOT).endsWith(principalSuffix.toLowerCase(Locale.ROOT))) {
return username += principalSuffix;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return username += principalSuffix;
return username + principalSuffix;

@lprimak
Copy link
Contributor

lprimak commented Oct 15, 2025

Thank you Telmo for your contribution.
Before we can merge it, can you do a couple of things?

  • Can you fix the code (or accept suggestions) to make it pass the build?
  • Can you make the branch in your fork something other than "main"
  • Can you squash the commits (so it's only one commit)

@lprimak lprimak added java Pull requests that update Java code CLA labels Oct 15, 2025
@lprimak lprimak added this to the 2.0.6 milestone Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lprimak lprimak lprimak requested changes

Assignees

No one assigned

Labels

CLA java Pull requests that update Java code

Projects

None yet

Milestone

2.0.6

Development

Successfully merging this pull request may close these issues.

[Bug] Error authenticating with AD

2 participants

@tbrugz @lprimak