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 #2

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@phansson

phansson commented Sep 18, 2017

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.

@@ -61,6 +63,7 @@
*/
final class NbAuthenticator extends java.net.Authenticator {

This comment has been minimized.

@emilianbold

emilianbold Sep 19, 2017

Contributor

Why not just add @ServiceProvider(service= Authenticator.class) here?

@emilianbold

emilianbold Sep 19, 2017

Contributor

Why not just add @ServiceProvider(service= Authenticator.class) here?

This comment has been minimized.

@phansson

phansson Sep 19, 2017

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.

@phansson

phansson Sep 19, 2017

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.

This comment has been minimized.

@emilianbold

emilianbold Sep 19, 2017

Contributor

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 NbAuthenticator class and constructor public or
  • introduce a AuthenticatorFactory which will be registered in the lookup with a default factory producing NbAuthenticator or
  • use your current solution
@emilianbold

emilianbold Sep 19, 2017

Contributor

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 NbAuthenticator class and constructor public or
  • introduce a AuthenticatorFactory which will be registered in the lookup with a default factory producing NbAuthenticator or
  • 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());

This comment has been minimized.

@emilianbold

emilianbold Sep 19, 2017

Contributor

... 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.

@emilianbold

emilianbold Sep 19, 2017

Contributor

... 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)) {

This comment has been minimized.

@emilianbold

emilianbold Sep 19, 2017

Contributor

Everything else, including the log call doesn't seem necessary.

@emilianbold

emilianbold Sep 19, 2017

Contributor

Everything else, including the log call doesn't seem necessary.

This comment has been minimized.

@phansson

phansson Sep 19, 2017

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?

@phansson

phansson Sep 19, 2017

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?

This comment has been minimized.

@emilianbold

emilianbold Sep 19, 2017

Contributor

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.

@emilianbold

emilianbold Sep 19, 2017

Contributor

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.

This comment has been minimized.

@phansson

phansson Sep 19, 2017

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.

@phansson

phansson Sep 19, 2017

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.

This comment has been minimized.

@emilianbold

emilianbold Sep 19, 2017

Contributor

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.

@emilianbold

emilianbold Sep 19, 2017

Contributor

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.

This comment has been minimized.

@phansson

phansson Sep 19, 2017

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.

@phansson

phansson Sep 19, 2017

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.

This comment has been minimized.

@jmborer

jmborer Apr 25, 2018

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.

@jmborer

jmborer Apr 25, 2018

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.

This comment has been minimized.

@geertjanw

geertjanw Apr 25, 2018

Member

Specifically, users on Windows, right?

@geertjanw

geertjanw Apr 25, 2018

Member

Specifically, users on Windows, right?

This comment has been minimized.

@jmborer

jmborer Apr 25, 2018

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.

@jmborer

jmborer Apr 25, 2018

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.

@asfgit

This comment has been minimized.

Show comment
Hide comment
@asfgit

asfgit Sep 25, 2017

Can one of the admins verify this patch?

asfgit commented Sep 25, 2017

Can one of the admins verify this patch?

@JaroslavTulach

This comment has been minimized.

Show comment
Hide comment
@JaroslavTulach

JaroslavTulach Sep 26, 2017

Contributor

This is an API change (one can register Authenticator which wasn't possible). You should also change apichanges.xml and arch.xml files. The later one to describe the API - use <api group="lookup"....> tag, the first file to refer to description of the change. You should also increase the version number of the module and use it in apichanges.xml file. Please run ant javadoc and make sure your documentation looks acceptable.

Contributor

JaroslavTulach commented Sep 26, 2017

This is an API change (one can register Authenticator which wasn't possible). You should also change apichanges.xml and arch.xml files. The later one to describe the API - use <api group="lookup"....> tag, the first file to refer to description of the change. You should also increase the version number of the module and use it in apichanges.xml file. Please run ant javadoc and make sure your documentation looks acceptable.

@phansson

This comment has been minimized.

Show comment
Hide comment
@phansson

phansson Nov 26, 2017

On hold for now.

phansson commented Nov 26, 2017

On hold for now.

@JaroslavTulach

This comment has been minimized.

Show comment
Hide comment
@JaroslavTulach

JaroslavTulach Mar 6, 2018

Contributor

I think you can proceed now, when beta of 9.0 is out and most of the licensing issues are resolved.

Contributor

JaroslavTulach commented Mar 6, 2018

I think you can proceed now, when beta of 9.0 is out and most of the licensing issues are resolved.

@jmborer

This comment has been minimized.

Show comment
Hide comment
@jmborer

jmborer Apr 25, 2018

I would like to stress that it is of highest importance to bundle phansson's plugins:
https://bitbucket.org/phansson/netbeansproxy2
https://bitbucket.org/phansson/netbeansnetworkauthenticator

in NB platform, due to this issue very critical issue https://issues.apache.org/jira/browse/NETBEANS-58

jmborer commented Apr 25, 2018

I would like to stress that it is of highest importance to bundle phansson's plugins:
https://bitbucket.org/phansson/netbeansproxy2
https://bitbucket.org/phansson/netbeansnetworkauthenticator

in NB platform, due to this issue very critical issue https://issues.apache.org/jira/browse/NETBEANS-58

@geertjanw

This comment has been minimized.

Show comment
Hide comment
@geertjanw

geertjanw Apr 25, 2018

Member

@jmborer, isn't that what this issue is about?

Member

geertjanw commented Apr 25, 2018

@jmborer, isn't that what this issue is about?

@geertjanw

This comment has been minimized.

Show comment
Hide comment
@geertjanw

geertjanw Apr 25, 2018

Member

Hi all, are we ready to merge this or do we need more discussion?

Member

geertjanw commented Apr 25, 2018

Hi all, are we ready to merge this or do we need more discussion?

@jmborer

This comment has been minimized.

Show comment
Hide comment
@jmborer

jmborer Apr 25, 2018

@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.

jmborer commented Apr 25, 2018

@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();
}

This comment has been minimized.

@jmborer

jmborer Apr 25, 2018

Add setDefault(authenticator); here instead of line 73

@jmborer

jmborer Apr 25, 2018

Add setDefault(authenticator); here instead of line 73

@orat

This comment has been minimized.

Show comment
Hide comment
@orat

orat Sep 21, 2018

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.

orat commented Sep 21, 2018

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.

@darkyellow

This comment has been minimized.

Show comment
Hide comment
@darkyellow

darkyellow Sep 22, 2018

Is the original author still working on this?

darkyellow commented Sep 22, 2018

Is the original author still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment