Allow custom authenticator #2
Conversation
| @@ -61,6 +63,7 @@ | |||
| */ | |||
| final class NbAuthenticator extends java.net.Authenticator { | |||
There was a problem hiding this comment.
Why not just add @ServiceProvider(service= Authenticator.class) here?
There was a problem hiding this comment.
I actually did that initially. But it requires a public no-arg constructor and I didn't have time to analyze why the original author insisted on a private constructor and since it wouldn't actually bring any benefit - besides a bit cleaner code - I then discarded the idea. But I have no objections to do this change.
There was a problem hiding this comment.
Ah, I see, and NbAuthenticator is package private too. I guess this is just standard NetBeans architecture of not over-exposing more than necessary.
I guess:
- we could either make
NbAuthenticatorclass and constructor public or - introduce a
AuthenticatorFactorywhich will be registered in the lookup with a default factory producingNbAuthenticatoror - use your current solution
| @@ -71,7 +74,19 @@ private NbAuthenticator() { | |||
|
|
|||
| static void install() { | |||
| if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) { | |||
| setDefault(new NbAuthenticator()); | |||
There was a problem hiding this comment.
... then just call setDefault(Lookup.getDefault().lookup(Authenticator.class)) here.
There will always be at least one instance in the lookup and the other implementations can jump in front with the position attribute.
| if (authenticator == null) { | ||
| authenticator = new NbAuthenticator(); | ||
| } | ||
| if (authenticator.getClass().equals(NbAuthenticator.class)) { |
There was a problem hiding this comment.
Everything else, including the log call doesn't seem necessary.
There was a problem hiding this comment.
After analyzing hundreds of messages files (NB log files) from my customers there's one thing I'm certain of: you can never have too much logging. :-)
Why do you think the logging calls are unnecessary?
There was a problem hiding this comment.
Because it's not usual to log which instance you are using from the Lookup. Since this is a static situation, you know at build-time which instance is going to be used, unless somebody yanks your whole module with the other implementation out.
There was a problem hiding this comment.
Why is it static? The idea is that people can add their own Authenticators. Heck, I might even publish my own version as a plugin in the Plugin Portal and ask users to use my plugin if I believe I've come up with something brilliant which is better than the standard NbAuthenticator. And that situation already exists.
There was a problem hiding this comment.
I assumed only a Platform application would need that. I didn't think that people might want to install a 3rd party plugin in their existing IDE/Platform app.
There was a problem hiding this comment.
I can can think of a dozen reason why you might want to override the Authenticator on a existing application, i.e. use of Plugin. It is actually quite difficult to design an Authenticator which fits everyone's taste. For example imagine you are on a site where you have an alternative way that you can obtain credentials, one that cannot be anticipated in the std Authenticator. This also includes integration with all kinds of SSO systems. There's simply no way we can cater for all.
There was a problem hiding this comment.
I (and all my colleagues in my company) have serious trouble with NB platform based applications such as the IDE itself during startup, because we are sitting behind a corporate proxy that requires authentication. Please have a look at https://issues.apache.org/jira/browse/NETBEANS-58 for a detailed discussion. Therefore phansson solution should absolutely be included in NB otherwise users might have a very bad experience using NB IDE and will just get rid off in favor for alternatives. Bummer.
There was a problem hiding this comment.
Specifically, users on Windows, right?
There was a problem hiding this comment.
Yes, but as said below, if NB reuses phansson's plugins (proxy and authenticator) code, NB will benefit from a much better proxy and authentication implementation anyway... This plugins deserve to be included in NB and not remain separate.
|
Can one of the admins verify this patch? |
|
This is an API change (one can register |
|
On hold for now. |
|
I think you can proceed now, when beta of 9.0 is out and most of the licensing issues are resolved. |
|
I would like to stress that it is of highest importance to bundle phansson's plugins: in NB platform, due to this issue very critical issue https://issues.apache.org/jira/browse/NETBEANS-58 |
|
@jmborer, isn't that what this issue is about? |
|
Hi all, are we ready to merge this or do we need more discussion? |
|
@geertjanw: yes it is, but it seems incomplete. As far as I understand, it just allows the installation of a custom Authenticator, but when I requested the integration of phansson plugins, my query was motivated by the fact that the plugins also include advanced proxy PAC support and provide an implementation of an Authenticator that handles much better SPNEGO as the default NBAuthenticator. |
| Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class); | ||
| if (authenticator == null) { | ||
| authenticator = new NbAuthenticator(); | ||
| } |
There was a problem hiding this comment.
Add setDefault(authenticator); here instead of line 73
|
I am very interested for integration of netbeansproxy2 because I have since a couple of yeas I run into problems with my platform applications running in the windows worls of some universities. Suddenly an authentification window opens but authentification does nor work. In some case I cauld find the origin of the problem in bugs in the proxies but this bug only affects my nb platform apps so they dont wont to fix it and I have to workaound it. Using the netbeansproxy2 solves this problem. So it would be really great if this can be integrated. |
|
Is the original author still working on this? |
|
Am Freitag, 21. September 2018, 22:10:55 CEST
schrieb darkyellow:
Is the original author still working on this?
I don't know:-(
|
|
@phansson what needs to get it done? Any big issues? |
|
@phansson can you be so kind pleaes and fix what @emilianbold requested? Otherwise we should close it, maybe. |
|
I was mostly commenting on top of it back then. Nothing from what I've said was a hard -1 so to speak. Please carry on. |
Update to latest remote/master
Adding fixes to let tests pass after nb-javac upgrade.
|
Any plans to finish this pull request? If so, feel free to reopen. |
Fixes NETBEANS-61.
Allows a Platform user to install his own version of an Authenticator which may be more appropriate for the given application than the one provided by default by the Platform.
In no implementation of Authenticator is found in the Global Lookup then everything will work as before.