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

Allow custom authenticator to be added via configuration #115

Merged
merged 6 commits into from Sep 27, 2013
Merged

Allow custom authenticator to be added via configuration #115

merged 6 commits into from Sep 27, 2013

Conversation

georgy
Copy link
Contributor

@georgy georgy commented Sep 15, 2013

Introduce new configuration parameter (authenticatorClass) that is used to specify custom authenticator class. This authenticator class will be found, instance will be created (if class has a constructor that takes Configuration as parameter, jolokia runtime configuration will be provided, allowing authenticator to take it into account). Newly created instance of authenticator will be set for Jolokia server to use. If authenticatorClass config parameter was not provided, code will fall back to old logic (i.e. check if user/password is set and create default authenticator).

This pull request also adds a ugly looking code into copyResourceToTemp method of JvmAgentConfigTest with the main goal of properly escaping paths used in tests when they are run win (tested on win7).

@georgy
Copy link
Contributor Author

georgy commented Sep 23, 2013

Any chance this can make next minor release :)? How can I help this to get integrated?

@rhuss
Copy link
Member

rhuss commented Sep 24, 2013

Looks fine, thanks a lot ! I'll apply the pull request ASAP, assuming that APL as license is ok for you.

BTW, in the initial description it is mentioned, that a customer authenticator gets a Configuration object as argument for its constructor, but as far as I see, your code uses only the default constructor. That's no problem for me, just wanted to mention it.

@rhuss
Copy link
Member

rhuss commented Sep 24, 2013

BTW, sorry for the delay, I'm super busy nowadays. But I will push out a 1.1.4-SNAPSHOT this week with this change applied.

@georgy
Copy link
Contributor Author

georgy commented Sep 24, 2013

No worries, just checking in :).

Re Configuration: Implementation will try two constructors, first it will try to find constructor that takes Configuration, if there is none it will fall back to default (no-arg) constructor.

@rhuss
Copy link
Member

rhuss commented Sep 24, 2013

Ah, ok. Overlooked this.

Could you do me a favor and add this configuration parameter to src/docbkx/agents/jvm.xml and add this as a new feature to src/changes/changes.xml 'would speed up things, but I can do it, too later on.

@georgy
Copy link
Contributor Author

georgy commented Sep 24, 2013

Sure thing, I will add this in couple of hours

@georgy
Copy link
Contributor Author

georgy commented Sep 25, 2013

Done

@rhuss
Copy link
Member

rhuss commented Sep 26, 2013

Thanks a lot, I will apply the pull request ASAP. You can expect a 1.1.4 release this weekend probably, worst case is end of next week.

@georgy
Copy link
Contributor Author

georgy commented Sep 26, 2013

Cool, thanks

rhuss added a commit that referenced this pull request Sep 27, 2013
Allow custom authenticator to be added via configuration
@rhuss rhuss merged commit 9218aac into jolokia:master Sep 27, 2013
@rhuss
Copy link
Member

rhuss commented Sep 27, 2013

Thanks again. Hopefully this weekend I've more time in order to prepare a release.

@rhuss
Copy link
Member

rhuss commented May 28, 2015

FYI, I'm about to release 1.3.1 today and for consistencies sake I renamed 'authenticationClass' to 'authClass' as parameter. I hope this does not provide you any issues when doing upgrades. If this is a stumbling block, though, I can introduce this parameter back as a fallback (but would like to avoid this to keep the code base clean).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants