Skip to content

Conversation

@tbrugz
Copy link
Contributor

@tbrugz tbrugz commented Oct 17, 2025

Fixes #2308

By creating a new branch and resetting the old one (main), I've unintentionally closed the old PR (#2309)

Thanks for the suggestions, @lprimak !

@fpapon
Copy link
Member

fpapon commented Oct 18, 2025

@tbrugz thank you for your contribution! Is it possible for you to add a test for this issue?

@lprimak lprimak added this to the 2.0.6 milestone Oct 18, 2025
@lprimak lprimak added java Pull requests that update Java code core Core Modules labels Oct 18, 2025
@lprimak
Copy link
Contributor

lprimak commented Oct 20, 2025

@fpapon It looks like the modified code is already covered by existing test.
Net new code is just a setter. I am not sure it's worth holding this up for a setter-type test.
What do you think?

@lprimak lprimak requested a review from bmarwell October 20, 2025 15:25
Copy link
Contributor

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

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

I haven't touched this code in a while. It looks as if an oversight was fixed (suffix for the login username ONLY if it was not given by the user), but I cannot say much more about this.

I am not sure how this could or could not break things. I might have done something similar in my apps, but this seems safe enough. I'd say give it a try.

@bmarwell bmarwell requested a review from Copilot October 20, 2025 19:12

This comment was marked as resolved.

@lprimak lprimak requested a review from fpapon October 23, 2025 07:13
@tbrugz
Copy link
Contributor Author

tbrugz commented Oct 23, 2025

@fpapon, I've added a new test to check ActiveDirectoryRealm's property settings here: tbrugz@396c532

I've based the test on https://shiro.apache.org/testing.html (class ExampleShiroIntegrationTest). It uses the deprecated IniSecurityManagerFactory class, I hope that's ok.

Should I merge with this (PR) branch?

@fpapon
Copy link
Member

fpapon commented Oct 24, 2025

@tbrugz thank you!

@fpapon fpapon merged commit 21e8c2d into apache:main Oct 24, 2025
50 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core Modules java Pull requests that update Java code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Error authenticating with AD

4 participants