Skip to content

Allow custom authenticator #2

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.

Sign up for GitHub

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

Closed
phansson wants to merge 2 commits into from
Closed

Allow custom authenticator #2

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea655cf
Improve Javadoc
Sep 18, 2017
a9c77fd
Allow custom Authenticator
Sep 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions o.n.core/src/org/netbeans/core/NbAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,27 @@
*/
package org.netbeans.core;

import java.net.Authenticator;
import java.net.PasswordAuthentication;
import java.net.URL;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.prefs.Preferences;
import org.openide.DialogDescriptor;
import org.openide.DialogDisplayer;
import org.openide.util.Lookup;
import org.openide.util.NbBundle;
import org.openide.util.NbPreferences;
import org.openide.util.NetworkSettings;

/** Global password protected sites Authenticator for IDE
/** Global password protected sites/proxies Authenticator for IDE
* or Platform apps.
*
* @author Jiri Rechtacek
*/
final class NbAuthenticator extends java.net.Authenticator {
Copy link
Member

@emilianbold emilianbold Sep 19, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@phansson phansson Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@emilianbold emilianbold Sep 19, 2017

Choose a reason for hiding this comment

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

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


private static final Logger LOGGER = Logger.getLogger(NbAuthenticator.class.getName());
private static final long TIMEOUT = 3000;
private static long lastTry = 0;

Expand All @@ -70,7 +74,19 @@ private NbAuthenticator() {

static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
setDefault(new NbAuthenticator());
Copy link
Member

@emilianbold emilianbold Sep 19, 2017

Choose a reason for hiding this comment

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

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

// Look for custom authenticator
Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class);
if (authenticator == null) {
authenticator = new NbAuthenticator();
}
Copy link
Contributor

@jmborer jmborer Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Add setDefault(authenticator); here instead of line 73

if (authenticator.getClass().equals(NbAuthenticator.class)) {
Copy link
Member

@emilianbold emilianbold Sep 19, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@phansson phansson Sep 19, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

@emilianbold emilianbold Sep 19, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Author

@phansson phansson Sep 19, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@emilianbold emilianbold Sep 19, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Author

@phansson phansson Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jmborer jmborer Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@geertjanw geertjanw Apr 25, 2018

Choose a reason for hiding this comment

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

Specifically, users on Windows, right?

Copy link
Contributor

@jmborer jmborer Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

LOGGER.log(Level.FINE, "Standard Authenticator, {0}, will be used as Authenticator.",
authenticator.getClass().getName());
} else {
LOGGER.log(Level.FINE, "Custom Authenticator, {0}, was found in Global Lookup and will be used as Authenticator.",
authenticator.getClass().getName());
}
setDefault(authenticator);
}
}

Expand Down