You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@netbeans.apache.org by phansson <gi...@git.apache.org> on 2017/09/18 21:41:58 UTC
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
GitHub user phansson opened a pull request:
https://github.com/apache/incubator-netbeans/pull/2
Allow custom authenticator
Fixes [NETBEANS-61](https://issues.apache.org/jira/browse/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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/phansson/incubator-netbeans AllowCustomAuthenticator
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-netbeans/pull/2.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2
----
----
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by emilianbold <gi...@git.apache.org>.
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139602545
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -71,7 +74,19 @@ private NbAuthenticator() {
static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
- setDefault(new NbAuthenticator());
+ // Look for custom authenticator
+ Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class);
+ if (authenticator == null) {
+ authenticator = new NbAuthenticator();
+ }
+ if (authenticator.getClass().equals(NbAuthenticator.class)) {
--- End diff --
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.
---
[GitHub] incubator-netbeans issue #2: Allow custom authenticator
Posted by JaroslavTulach <gi...@git.apache.org>.
Github user JaroslavTulach commented on the issue:
https://github.com/apache/incubator-netbeans/pull/2
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.
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by emilianbold <gi...@git.apache.org>.
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139596801
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -71,7 +74,19 @@ private NbAuthenticator() {
static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
- setDefault(new NbAuthenticator());
--- End diff --
... 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.
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by JaroslavTulach <gi...@git.apache.org>.
Github user JaroslavTulach commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r141021368
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -61,6 +63,7 @@
*/
final class NbAuthenticator extends java.net.Authenticator {
--- End diff --
`java.util.ServiceLoader` requires classes with public constructor so does `Lookup.getDefault()`. Is the class in public or non-public package? If it is in non-public, feel free to make its default constructor public - it won't be considered an API change anyway.
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by emilianbold <gi...@git.apache.org>.
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139607093
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -71,7 +74,19 @@ private NbAuthenticator() {
static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
- setDefault(new NbAuthenticator());
+ // Look for custom authenticator
+ Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class);
+ if (authenticator == null) {
+ authenticator = new NbAuthenticator();
+ }
+ if (authenticator.getClass().equals(NbAuthenticator.class)) {
--- End diff --
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.
---
[GitHub] incubator-netbeans issue #2: Allow custom authenticator
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/incubator-netbeans/pull/2
Can one of the admins verify this patch?
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by phansson <gi...@git.apache.org>.
Github user phansson commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139602369
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -71,7 +74,19 @@ private NbAuthenticator() {
static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
- setDefault(new NbAuthenticator());
+ // Look for custom authenticator
+ Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class);
+ if (authenticator == null) {
+ authenticator = new NbAuthenticator();
+ }
+ if (authenticator.getClass().equals(NbAuthenticator.class)) {
--- End diff --
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?
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by emilianbold <gi...@git.apache.org>.
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139596689
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -61,6 +63,7 @@
*/
final class NbAuthenticator extends java.net.Authenticator {
--- End diff --
Why not just add `@ServiceProvider(service= Authenticator.class)` here?
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by emilianbold <gi...@git.apache.org>.
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139602349
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -61,6 +63,7 @@
*/
final class NbAuthenticator extends java.net.Authenticator {
--- End diff --
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
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by emilianbold <gi...@git.apache.org>.
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139596868
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -71,7 +74,19 @@ private NbAuthenticator() {
static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
- setDefault(new NbAuthenticator());
+ // Look for custom authenticator
+ Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class);
+ if (authenticator == null) {
+ authenticator = new NbAuthenticator();
+ }
+ if (authenticator.getClass().equals(NbAuthenticator.class)) {
--- End diff --
Everything else, including the log call doesn't seem necessary.
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by phansson <gi...@git.apache.org>.
Github user phansson commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139614856
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -71,7 +74,19 @@ private NbAuthenticator() {
static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
- setDefault(new NbAuthenticator());
+ // Look for custom authenticator
+ Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class);
+ if (authenticator == null) {
+ authenticator = new NbAuthenticator();
+ }
+ if (authenticator.getClass().equals(NbAuthenticator.class)) {
--- End diff --
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.
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by phansson <gi...@git.apache.org>.
Github user phansson commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139604336
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -71,7 +74,19 @@ private NbAuthenticator() {
static void install() {
if (Boolean.valueOf(NbBundle.getMessage(GuiRunLevel.class, "USE_Authentication"))) {
- setDefault(new NbAuthenticator());
+ // Look for custom authenticator
+ Authenticator authenticator = Lookup.getDefault().lookup(Authenticator.class);
+ if (authenticator == null) {
+ authenticator = new NbAuthenticator();
+ }
+ if (authenticator.getClass().equals(NbAuthenticator.class)) {
--- End diff --
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](https://bitbucket.org/phansson/netbeansnetworkauthenticator).
---
[GitHub] incubator-netbeans pull request #2: Allow custom authenticator
Posted by phansson <gi...@git.apache.org>.
Github user phansson commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/2#discussion_r139601775
--- Diff: o.n.core/src/org/netbeans/core/NbAuthenticator.java ---
@@ -61,6 +63,7 @@
*/
final class NbAuthenticator extends java.net.Authenticator {
--- End diff --
I actually did that initially. (i.e. registering self as a service). 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.
---