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

Issue #5486 PropertyFileLoginModule retains PropertyUserStores #5518

Merged
merged 3 commits into from Nov 11, 2020

Conversation

janbartel
Copy link
Contributor

Closes #5486

In pursuing the original problem, which was that the PropertyFileLoginModule ignored the hotReload setting option for PropertyUserStores, I've realized there was a bigger problem, which is that the PropertyFileLoginModule was retaining instances of the PropertyUserStore in a static map and never removing them. Thus, the map would remain populated across restarts, possibly even pinning a classloader.

This fix ensures the the map of PropertyUserStores is retained on an instance of the JAASLoginService instead, and is cleared whenever the login service is stopped.

A few other code cleanups along the way, and added extra tests.

Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel added this to In progress in Jetty 9.4.34 via automation Oct 27, 2020
@janbartel janbartel self-assigned this Oct 27, 2020
Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

Seems to be some test failures, so maybe it needs a merge from 9.4.x.

{
private static final Logger LOG = Log.getLogger(JAASLoginService.class);

public static final String DEFAULT_ROLE_CLASS_NAME = "org.eclipse.jetty.jaas.JAASRole";
public static final String[] DEFAULT_ROLE_CLASS_NAMES = {DEFAULT_ROLE_CLASS_NAME};

public static final ThreadLocal<JAASLoginService> INSTANCE = new ThreadLocal<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an unusual way to share the LoginService with the LoginModule.
Would it be better to do this with a Callback instead? similar how we would use ServletRequestCallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we allow the user to supply their own CallbackHandler class, which might not know anything about the special jetty Callbacks, therefore couldn't supply a value for the JAASLoginService.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that seems strange to me, how would you ever use ServletRequestCallback or RequestParameterCallback inside a login module because they only ever get set on the DefaultCallbackHandler? Seems like using this would also just break if they supplied a custom CallbackHandler.

@gregw gregw removed this from In progress in Jetty 9.4.34 Nov 2, 2020
Signed-off-by: Jan Bartel <janb@webtide.com>
@joakime joakime added this to In progress in Jetty 9.4.35 via automation Nov 2, 2020
@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.35 Nov 4, 2020
Jetty 9.4.35 automation moved this from Review in progress to Reviewer approved Nov 9, 2020
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

LGTM.
If the threadlocal is a problem we could use it only as a fallback if the callbackhandler is not a type we know.... but that feels like complexity we don't need if the threadlocal works.

@janbartel janbartel merged commit 3a99c89 into jetty-9.4.x Nov 11, 2020
Jetty 9.4.35 automation moved this from Reviewer approved to Done Nov 11, 2020
@janbartel janbartel deleted the jetty-9.4.x-5486-propertyfileloginmodule branch November 11, 2020 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants