You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by ceharris <gi...@git.apache.org> on 2017/08/16 20:59:35 UTC
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
GitHub user ceharris opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/184
GUACAMOLE-364: authentication and tunnel listener extension support
This PR adds support for extensions that include listeners that implement the interfaces declared in the `org.apache.guacamole.ext.net.event.listener` package of the guacamole-ext module of the client.
The fully-qualified names of classes that implement one or more of these listener interfaces are configured in the extension manifest using the `listeners` property. Listener classes are wrapped in a facade in a fashion similar to that used for authentication providers. A `ListenerService` is registered as a Guice service (in `RESTServiceModule`) and injected into the `AuthenticationService` and `TunnelRequestService`. The `ListenerService` is used to notify all bound listeners in each of the situations in which a listener is to be notified.
I made some assumptions about the desired semantics of the boolean return value of some of the listener methods in the presence of multiple listeners, since the expected behavior isn't really expressed in the contract of the listener interfaces. Similarly, I made assumptions about the effect of listener exceptions. I describe the behavior in the `ListenerService` Javadoc, although it should probably be documented as part of the extension API. My assumptions could obviously be revisited and the implementation adjusted, as well.
I used a fairly simplistic approach to loading and binding listeners. The facade that wraps an extension listener class implements all of the existing listener interfaces, and uses `instanceof` to determine whether the delegate listener actually implements a given interface. This probably works well enough for a small number of event types with a reasonably small number of listeners, but should be revisited if either of these assumptions changes.
I was a little uncertain of what should happen WRT authentication listener notification on a re-authentication. I decided that notifying listeners of the re-authentication success was what was intended. See `AuthenticationService.getAuthenticatedUser` around line 290.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/soulwing/incubator-guacamole-client GUACAMOLE-364
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/184.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 #184
----
commit 287ab56f0f793190522e9d173f3fbd61c9155919
Author: Carl Harris <ce...@vt.edu>
Date: 2017-08-16T10:52:10Z
GUACAMOLE-364: factor out common provider class instantiation support
This will allow the same error and debug logging to be used both
for the AuthenticationProviderFacade and a new ListenerFacade.
commit 6f89a0b53047dd1cc9fac8264639c30067bf6cb6
Author: Carl Harris <ce...@vt.edu>
Date: 2017-08-16T10:54:16Z
GUACAMOLE-364: listener interfaces now extend a common marker interface
commit dca7862351fd4c27e82ecaf1cf540180501f48ec
Author: Carl Harris <ce...@vt.edu>
Date: 2017-08-16T10:54:55Z
GUACAMOLE-364: add facade used to wrap extension listeners
commit 109d57ecb372c3fc8114792086f339e9269eaf98
Author: Carl Harris <ce...@vt.edu>
Date: 2017-08-16T10:55:28Z
GUACAMOLE-364: add extension module support for event listeners
commit cfb879b763b4254ba1c44b38d7fb5993b3e62187
Author: Carl Harris <ce...@vt.edu>
Date: 2017-08-16T10:57:16Z
GUACAMOLE-364: add injectable ListenerService
commit 5a232f6825deb9b73f89473f941d92d012e36f67
Author: Carl Harris <ce...@vt.edu>
Date: 2017-08-16T10:58:18Z
GUACAMOLE-364: notify authentication listeners in AuthenticationService
commit 6b6340ac464e03cc2a7bb8e9f72f8044e79beed6
Author: Carl Harris <ce...@vt.edu>
Date: 2017-08-16T10:59:12Z
GUACAMOLE-364: notify tunnel listeners in TunnelRequestService
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143106229
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationSuccessEvent) {
+ if (!delegate.authenticationSucceeded((AuthenticationSuccessEvent) event)) {
+ throw new GuacamoleSecurityException(
+ "listener vetoed successful authentication");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationFailureListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationFailureListenerAdapter implements Listener {
+
+ private final AuthenticationFailureListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationFailureListenerAdapter(AuthenticationFailureListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationFailureEvent by passing the event to the delegate
+ * listener. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationFailureEvent) {
+ delegate.authenticationFailed((AuthenticationFailureEvent) event);
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows a TunnelConnectListener to be used as an ordinary
+ * Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class TunnelConnectListenerAdapter implements Listener {
+
+ private final TunnelConnectListener delegate;
+
+ /**
+ * Constructs a new adapter.
--- End diff --
See above - comment should be more informative.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105636
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ExtensionManifest.java ---
@@ -356,6 +361,32 @@ public void setAuthProviders(Collection<String> authProviders) {
}
/**
+ * Returns the classnames of all listener classes within the extension.
+ * These classnames are defined within the manifest by the "listeners"
+ * property as an array of strings, where each string is a listener
+ * class name.
+ *
+ * @return
+ * a collection of classnames for all listeners within the extension
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/184
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105548
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/Listener.java ---
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.event.listener;
+
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * A listener for events that occur in handing various Guacamole requests
+ * such as authentication, tunnel connect/close, etc. Listeners are registered
+ * through the extension manifest mechanism. When an event occurs, listeners
+ * are notified in the order in which they are declared in the manifest and
+ * continues until either all listeners have been notified or with the first
+ * listener that throws a GuacamoleException or other runtime exception.
+ */
+public interface Listener {
+
+ /**
+ * Notifies the recipient that an event has occurred.
+ * <p>
+ * Throwing an exception from an event listener can act to veto an action in
+ * progress for some event types. See the Javadoc for specific event types for
+ * details.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * If the listener wishes to stop notification of the event to subsequent
+ * listeners. For some event types, this acts to veto an action in progress;
+ * e.g. treating a successful authentication as though it failed
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104561
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/TunnelConnectListener.java ---
@@ -25,7 +25,11 @@
/**
* A listener whose tunnelConnected() hook will fire immediately after a new
* tunnel is connected.
+ *
+ * @deprecated
+ * Listeners should instead implement the {@link Listener} interface
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133656227
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole;
+
+/**
+ * An exception thrown when a successful authentication is rejected by a
+ * AuthenticationSuccessListener in an extension.
+ */
+public class GuacamoleAuthenticationRejectedException
--- End diff --
It was a judgement call. I subtyped existing exceptions to get status codes that should be appropriate for these cases, but was uncomfortable making assumptions about how the existing exception types might be interpreted in `catch` clauses.
I'd argue that there is something fundamentally different about an API user providing an incorrect credential vs. an extension deciding that a successful authentication should be rejected. For that reason, I think a different exception type is appropriate. But I'm happy to change the code to reuse existing exceptions if that's preferred.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105704
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
--- End diff --
Missing JavaDoc.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133610412
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/Listener.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole.net.event.listener;
+
+/**
+ * A marker interface extended by all listener types. This interface is used
+ * simply to validate that a listener class identified in an extension
+ * actually implements some listener interface.
+ */
+public interface Listener {
+}
--- End diff --
Style: probably should have a blank line here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133655326
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java ---
@@ -232,11 +286,17 @@ private AuthenticatedUser getAuthenticatedUser(GuacamoleSession existingSession,
try {
// Re-authenticate user if session exists
- if (existingSession != null)
- return updateAuthenticatedUser(existingSession.getAuthenticatedUser(), credentials);
+ if (existingSession != null) {
+ AuthenticatedUser updatedUser = updateAuthenticatedUser(
+ existingSession.getAuthenticatedUser(), credentials);
+ notifyAuthenticationSuccessListeners(updatedUser, existingSession);
+ return updatedUser;
+ }
// Otherwise, attempt authentication as a new user
- AuthenticatedUser authenticatedUser = AuthenticationService.this.authenticateUser(credentials);
+ AuthenticatedUser authenticatedUser = authenticateUser(credentials);
--- End diff --
I suspect that my IDE noticed that the explicit `this` wasn't needed and removed it. I haven't looked at the history of this piece of code, but I suspect that it was previously refactored from a state in which defererencing `authenticateUser` in this way was necessary. I'm happy to revert this change if desired.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143106248
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationSuccessEvent) {
+ if (!delegate.authenticationSucceeded((AuthenticationSuccessEvent) event)) {
+ throw new GuacamoleSecurityException(
+ "listener vetoed successful authentication");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationFailureListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationFailureListenerAdapter implements Listener {
+
+ private final AuthenticationFailureListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationFailureListenerAdapter(AuthenticationFailureListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationFailureEvent by passing the event to the delegate
+ * listener. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationFailureEvent) {
+ delegate.authenticationFailed((AuthenticationFailureEvent) event);
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows a TunnelConnectListener to be used as an ordinary
+ * Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class TunnelConnectListenerAdapter implements Listener {
+
+ private final TunnelConnectListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ TunnelConnectListenerAdapter(TunnelConnectListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles a TunnelConnectEvent by passing the event to the delegate listener.
+ * If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the tunnel connect event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105757
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104765
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ProviderFactory.java ---
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+
+/**
+ * Static factory method for creating provider instances and logging unexpected outcomes
+ * with sufficient detail to allow user/developer debugging.
+ */
+class ProviderFactory {
+
+ private static final Logger logger = LoggerFactory.getLogger(ProviderFactory.class);
--- End diff --
Missing JavaDoc.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133610458
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/Extension.java ---
@@ -35,6 +35,8 @@
import java.util.zip.ZipEntry;
import java.util.zip.ZipException;
import java.util.zip.ZipFile;
+
--- End diff --
Style: remove extra line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105353
--- Diff: guacamole/src/main/java/org/apache/guacamole/tunnel/TunnelRequestService.java ---
@@ -58,6 +62,58 @@
private AuthenticationService authenticationService;
/**
+ * A service for notifying listeners about tunnel connect/closed events.
+ */
+ @Inject
+ private ListenerService listenerService;
+
+ /**
+ * Notifies bound listeners that a new tunnel has been connected.
+ * Listeners may veto a connected tunnel by throwing any GuacamoleException.
+ *
+ * @param userContext
+ * The UserContext associated with the user for whom the tunnel is
+ * being created.
+ *
+ * @param credentials
+ * Credentials that authenticate the user
+ *
+ * @param tunnel
+ * The tunnel that was connected
+ *
+ * @throws GuacamoleException
+ * If thrown by a listener or if any listener vetoes the connected tunnel
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105779
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105628
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ExtensionManifest.java ---
@@ -356,6 +361,32 @@ public void setAuthProviders(Collection<String> authProviders) {
}
/**
+ * Returns the classnames of all listener classes within the extension.
+ * These classnames are defined within the manifest by the "listeners"
+ * property as an array of strings, where each string is a listener
+ * class name.
+ *
+ * @return
+ * a collection of classnames for all listeners within the extension
+ */
+ public Collection<String> getListeners() {
+ return listeners;
+ }
+
+ /**
+ * Sets the classnames of all listener classes within the extension.
+ * These classnames are defined within the manifest by the "listeners"
+ * property as an array of strings, where each string is a listener
+ * class name.
+ *
+ * @param listeners
+ * a collection of classnames for all listeners within the extension
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105313
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.rest.event;
+
+import java.util.List;
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.net.event.listener.Listener;
+
+/**
+ * A service used to notify listeners registered by extensions when events of
+ * interest occur.
+ */
+public class ListenerService implements Listener {
+
+ @Inject
+ private List<Listener> listeners;
+
+ /**
+ * Notifies registered listeners than an event has occurred. Notification continues
+ * until a given listener throws a GuacamoleException or other runtime exception, or
+ * until all listeners have been notified.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException if a registered listener throws a GuacamoleException
--- End diff --
The portion following the `@throws ExceptionClass` should be on the next line, indented, sentence case, plus period (same as the other JavaDoc `@things`).
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104862
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ProviderFactory.java ---
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+
+/**
+ * Static factory method for creating provider instances and logging unexpected outcomes
--- End diff --
This is a class, not a method.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133609341
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java ---
@@ -66,58 +65,8 @@
* The AuthenticationProvider subclass to instantiate.
*/
public AuthenticationProviderFacade(Class<? extends AuthenticationProvider> authProviderClass) {
-
- AuthenticationProvider instance = null;
-
- try {
- // Attempt to instantiate the authentication provider
- instance = authProviderClass.getConstructor().newInstance();
- }
- catch (NoSuchMethodException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("AuthenticationProvider is missing a default constructor.", e);
- }
- catch (SecurityException e) {
- logger.error("The Java security mananager is preventing authentication extensions "
- + "from being loaded. Please check the configuration of Java or your "
- + "servlet container.");
- logger.debug("Creation of AuthenticationProvider disallowed by security manager.", e);
- }
- catch (InstantiationException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("AuthenticationProvider cannot be instantiated.", e);
- }
- catch (IllegalAccessException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("Default constructor of AuthenticationProvider is not public.", e);
- }
- catch (IllegalArgumentException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("Default constructor of AuthenticationProvider cannot accept zero arguments.", e);
- }
- catch (InvocationTargetException e) {
-
- // Obtain causing error - create relatively-informative stub error if cause is unknown
- Throwable cause = e.getCause();
- if (cause == null)
- cause = new GuacamoleException("Error encountered during initialization.");
-
- logger.error("Authentication extension failed to start: {}", cause.getMessage());
- logger.debug("AuthenticationProvider instantiation failed.", e);
-
- }
-
- // Associate instance, if any
- authProvider = instance;
-
+ authProvider = ProviderFactory.newInstance("authentication provider",
+ authProviderClass);
--- End diff --
Not sure I understand the reason for getting rid of all of this stuff and replacing it with the single line? I believe I see the commit log associated with this, but not sure why I understand that it's important that the debug logging be done in there vs. in here.
I'm relatively new to Java programming, so there may be a good reason for it...just asking :-).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133610235
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
--- End diff --
Extra line...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133611902
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole.rest.event;
+
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.extension.ListenerProvider;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.AuthenticationFailureListener;
+import org.apache.guacamole.net.event.listener.AuthenticationSuccessListener;
+import org.apache.guacamole.net.event.listener.TunnelCloseListener;
+import org.apache.guacamole.net.event.listener.TunnelConnectListener;
+
+import java.util.List;
--- End diff --
This should go in alphabetical order, like the rest of them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133612836
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole.rest.event;
+
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.extension.ListenerProvider;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.AuthenticationFailureListener;
+import org.apache.guacamole.net.event.listener.AuthenticationSuccessListener;
+import org.apache.guacamole.net.event.listener.TunnelCloseListener;
+import org.apache.guacamole.net.event.listener.TunnelConnectListener;
+
+import java.util.List;
+
+/**
+ * A service used to notify listeners registered by extensions when events of
+ * interest occur.
+ *
+ * @author Carl Harris
+ */
+public class ListenerService implements ListenerProvider {
+
+ @Inject
+ private List<ListenerProvider> listeners;
+
+ /**
+ * Notifies all bound listeners of an authentication success event. Listeners
+ * are allowed to veto a successful authentication by returning false from the
+ * listener method. Regardless of whether a particular listener rejects the
+ * successful authentication, all listeners are notified.
+ * @param e
+ * The AuthenticationSuccessEvent describing the successful authentication
+ * that just occurred.
+ *
+ * @return
+ * false if any bound listener returns false, else true
+ *
+ * @throws GuacamoleException
+ * If any bound listener throws this exception. If a listener throws an exception
+ * some listeners may not receive the authentication success event notification.
+ */
+ @Override
+ public boolean authenticationSucceeded(AuthenticationSuccessEvent e)
+ throws GuacamoleException {
+ boolean result = true;
+ for (AuthenticationSuccessListener listener : listeners) {
+ result = result && listener.authenticationSucceeded(e);
+ }
+ return result;
--- End diff --
Any reason not to do something like:
```
for (AuthenticationSuccessListener listener : listeners)
if (!listener.authenticationSucceeded(e))
return false;
return true;
```
here, and get rid of the extra boolean variable? Same below in the tunnelConnected() and tunnelClosed() methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133608587
--- Diff: guacamole/src/main/java/org/apache/guacamole/tunnel/TunnelRequestService.java ---
@@ -58,6 +64,74 @@
private AuthenticationService authenticationService;
/**
+ * A service for notifying listeners about tunnel connect/closed events.
+ */
+ @Inject
+ private ListenerService listenerService;
+
+ /**
+ * Notifies bound TunnelConnectListeners that a new tunnel has been connected.
+ * Listeners are allowed to veto a connected tunnel by returning false from the
+ * listener method. If the ListenerService indicates that any listener rejected
+ * the tunnel, the tunnel is closed an GuacamoleTunnelRejectedException is thrown.
+ *
+ * @param userContext
+ * The UserContext associated with the user for whom the tunnel is
+ * being created.
+ *
+ * @param credentials
+ * Credentials that authenticate the user
+ *
+ * @param tunnel
+ * The tunnel that was connected
+ *
+ * @throws GuacamoleException
+ * If thrown by a listener or if any listener vetoes the connected tunnel
+ */
+ private void notifyTunnelConnectListeners(UserContext userContext,
+ Credentials credentials, GuacamoleTunnel tunnel) throws GuacamoleException {
+ TunnelConnectEvent event = new TunnelConnectEvent(userContext, credentials, tunnel);
+ boolean ok = listenerService.tunnelConnected(event);
+ if (!ok) {
--- End diff --
Why not just:
`if (!listenerService.tunnelConnected(event)) {`
??
In fact, you did this in the next method down...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143106271
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationSuccessEvent) {
+ if (!delegate.authenticationSucceeded((AuthenticationSuccessEvent) event)) {
+ throw new GuacamoleSecurityException(
+ "listener vetoed successful authentication");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationFailureListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationFailureListenerAdapter implements Listener {
+
+ private final AuthenticationFailureListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationFailureListenerAdapter(AuthenticationFailureListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationFailureEvent by passing the event to the delegate
+ * listener. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationFailureEvent) {
+ delegate.authenticationFailed((AuthenticationFailureEvent) event);
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows a TunnelConnectListener to be used as an ordinary
+ * Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class TunnelConnectListenerAdapter implements Listener {
+
+ private final TunnelConnectListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ TunnelConnectListenerAdapter(TunnelConnectListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles a TunnelConnectEvent by passing the event to the delegate listener.
+ * If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the tunnel connect event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof TunnelConnectEvent) {
+ if (!delegate.tunnelConnected((TunnelConnectEvent) event)) {
+ throw new GuacamoleException("listener vetoed tunnel connection");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows a TunnelCloseListener to be used as an ordinary
+ * Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class TunnelCloseListenerAdapter implements Listener {
+
+ private final TunnelCloseListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104442
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/AuthenticationSuccessListener.java ---
@@ -27,7 +27,11 @@
* authentication attempt succeeds. If a user successfully authenticates,
* the authenticationSucceeded() hook has the opportunity to cancel the
* authentication and force it to fail.
+ *
+ * @deprecated
+ * Listeners should instead implement the {@link Listener} interface
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r134059181
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole;
+
+/**
+ * An exception thrown when a successful authentication is rejected by a
+ * AuthenticationSuccessListener in an extension.
+ */
+public class GuacamoleAuthenticationRejectedException
--- End diff --
For the authentication case, there is no need for any sort of listener. This sort of veto/monitoring functionality is already present in the auth system, and is the mechanism by which things like multi-factor authentication have been implemented.
Take the Duo extension, for example:
https://github.com/apache/incubator-guacamole-client/blob/a5ebc07349bcf43c80f404836d9964ac894bed26/extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/DuoAuthenticationProvider.java#L82-L96
https://github.com/apache/incubator-guacamole-client/blob/a5ebc07349bcf43c80f404836d9964ac894bed26/extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java#L53-L107
In the above, when the extension determines that the user needs to be verified via Duo's service, it throws an exception requesting additional credentials, implicitly halting the authentication process. This result trickles out in the response from the token REST endpoint, and the client renders a prompt requesting that they enter their code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133610357
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/AuthenticationFailureListener.java ---
@@ -27,8 +27,7 @@
* after a user's authentication attempt fails. Note that this hook cannot
* be used to cancel the authentication failure.
*/
-public interface AuthenticationFailureListener {
-
+public interface AuthenticationFailureListener extends Listener {
/**
--- End diff --
Style: probably should leave the line spacing here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r134061382
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/Listener.java ---
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.event.listener;
+
+/**
+ * A marker interface extended by all listener types. This interface is used
--- End diff --
I would fall on the "marker interface is an anti-pattern" side of the fence. If there is no shared functional interface between these listeners, then we shouldn't be defining a contract which states that there is.
That said, if it feels wrong to not have some common ancestor, then perhaps there *should* be a shared functional interface? At the moment, the only barrier I see preventing a common function for handling events is the variation on whether a handler is `void` or `boolean`, but there may be a better approach than the way things were done in the early guac days. Doing so would simplify the way these events get fired, and would avoid all the `instanceof` checks and casting.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105673
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104433
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/AuthenticationFailureListener.java ---
@@ -26,8 +26,12 @@
* A listener whose authenticationFailed() hook will fire immediately
* after a user's authentication attempt fails. Note that this hook cannot
* be used to cancel the authentication failure.
+ *
+ * @deprecated
+ * Listeners should instead implement the {@link Listener} interface
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143204123
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/TunnelCloseListener.java ---
@@ -25,11 +25,15 @@
/**
* A listener whose tunnelClosed() hook will fire immediately after an
* existing tunnel is closed.
+ *
+ * @deprecated
+ * Listeners should instead implement the {@link Listener} interface
*/
+@Deprecated
public interface TunnelCloseListener {
/**
- * Event hook which fires immediately after an existing tunnel is closed.
+ * Event hook which fires immediately before an existing tunnel is closed.
--- End diff --
Should be before. Fixed at the interface level comment.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105666
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133608373
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java ---
@@ -232,11 +286,17 @@ private AuthenticatedUser getAuthenticatedUser(GuacamoleSession existingSession,
try {
// Re-authenticate user if session exists
- if (existingSession != null)
- return updateAuthenticatedUser(existingSession.getAuthenticatedUser(), credentials);
+ if (existingSession != null) {
+ AuthenticatedUser updatedUser = updateAuthenticatedUser(
+ existingSession.getAuthenticatedUser(), credentials);
+ notifyAuthenticationSuccessListeners(updatedUser, existingSession);
+ return updatedUser;
+ }
// Otherwise, attempt authentication as a new user
- AuthenticatedUser authenticatedUser = AuthenticationService.this.authenticateUser(credentials);
+ AuthenticatedUser authenticatedUser = authenticateUser(credentials);
--- End diff --
What's the reason for this change? The original line shouldn't impact your addition of the notifyAuthenticationSuccessListeners() call, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133659431
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole.rest.event;
+
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.extension.ListenerProvider;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.AuthenticationFailureListener;
+import org.apache.guacamole.net.event.listener.AuthenticationSuccessListener;
+import org.apache.guacamole.net.event.listener.TunnelCloseListener;
+import org.apache.guacamole.net.event.listener.TunnelConnectListener;
+
+import java.util.List;
+
+/**
+ * A service used to notify listeners registered by extensions when events of
+ * interest occur.
+ *
+ * @author Carl Harris
+ */
+public class ListenerService implements ListenerProvider {
+
+ @Inject
+ private List<ListenerProvider> listeners;
+
+ /**
+ * Notifies all bound listeners of an authentication success event. Listeners
+ * are allowed to veto a successful authentication by returning false from the
+ * listener method. Regardless of whether a particular listener rejects the
+ * successful authentication, all listeners are notified.
+ * @param e
+ * The AuthenticationSuccessEvent describing the successful authentication
+ * that just occurred.
+ *
+ * @return
+ * false if any bound listener returns false, else true
+ *
+ * @throws GuacamoleException
+ * If any bound listener throws this exception. If a listener throws an exception
+ * some listeners may not receive the authentication success event notification.
+ */
+ @Override
+ public boolean authenticationSucceeded(AuthenticationSuccessEvent e)
+ throws GuacamoleException {
+ boolean result = true;
+ for (AuthenticationSuccessListener listener : listeners) {
+ result = result && listener.authenticationSucceeded(e);
+ }
+ return result;
--- End diff --
Doing so would result in some listeners not being notified when some listener determines that an authentication should be rejected. That seems inconsistent with the the concept of `listener`. For example, you might have listeners that are passing messages to external systems when an authentication success occurs and want/need to get that notification even if a some filter decides to reject the successful authentication. I documented the behavior in the preceding doc comment, and noted in the PR that the listener API doesn't really discuss the expected behavior.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105340
--- Diff: guacamole/src/main/java/org/apache/guacamole/tunnel/TunnelRequestService.java ---
@@ -58,6 +62,58 @@
private AuthenticationService authenticationService;
/**
+ * A service for notifying listeners about tunnel connect/closed events.
+ */
+ @Inject
+ private ListenerService listenerService;
+
+ /**
+ * Notifies bound listeners that a new tunnel has been connected.
+ * Listeners may veto a connected tunnel by throwing any GuacamoleException.
+ *
+ * @param userContext
+ * The UserContext associated with the user for whom the tunnel is
+ * being created.
+ *
+ * @param credentials
+ * Credentials that authenticate the user
+ *
+ * @param tunnel
+ * The tunnel that was connected
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105788
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105138
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.rest.event;
+
+import java.util.List;
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.net.event.listener.Listener;
+
+/**
+ * A service used to notify listeners registered by extensions when events of
+ * interest occur.
+ */
+public class ListenerService implements Listener {
+
+ @Inject
+ private List<Listener> listeners;
+
+ /**
+ * Notifies registered listeners than an event has occurred. Notification continues
+ * until a given listener throws a GuacamoleException or other runtime exception, or
+ * until all listeners have been notified.
+ *
+ * @param event
+ * an object that describes the subject event
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133682138
--- Diff: guacamole/src/main/java/org/apache/guacamole/tunnel/TunnelRequestService.java ---
@@ -226,7 +300,7 @@ protected GuacamoleTunnel createConnectedTunnel(UserContext context,
* @throws GuacamoleException
* If an error occurs while obtaining the tunnel.
*/
- protected GuacamoleTunnel createAssociatedTunnel(GuacamoleTunnel tunnel,
+ protected GuacamoleTunnel createAssociatedTunnel(final GuacamoleTunnel tunnel,
--- End diff --
Ah, okay, got it!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133680595
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,34 @@
+/*
--- End diff --
Yes, you are correct. Sorry about that...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104981
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ProviderFactory.java ---
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+
+/**
+ * Static factory method for creating provider instances and logging unexpected outcomes
+ * with sufficient detail to allow user/developer debugging.
+ */
+class ProviderFactory {
+
+ private static final Logger logger = LoggerFactory.getLogger(ProviderFactory.class);
+
+ /**
+ * Creates an instance of the specified provider class using the no-arg constructor.
+ *
+ * @param typeName
+ * The provider type name used for log messages (e.g. "authentication provider")
--- End diff --
Missing period (same for the other `@param` and `@return`).
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105648
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ExtensionModule.java ---
@@ -188,6 +195,49 @@ private void bindAuthenticationProviders(Collection<Class<AuthenticationProvider
}
/**
+ * Binds the given provider class such that a listener is bound for each
+ * listener interface implemented by the provider and such that all bound
+ * listener instances can be obtained via injection.
+ * *
+ * @param providerClass
+ * The listener provider class to bind
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133610715
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java ---
@@ -208,6 +218,50 @@ private AuthenticatedUser updateAuthenticatedUser(AuthenticatedUser authenticate
}
/**
+ * Notify all bound AuthenticationSuccessListeners that a successful authentication
+ * has occurred. If any of the bound listeners returns false (indicating that the
+ * authentication should be rejected) a GuacamoleRejectedAuthenticationException is
+ * thrown.
+ *
+ * @param authenticatedUser
+ * The user that was successfully authenticated
+ * @param session
+ * Existing session for the user (if any)
+ * @throws GuacamoleException
+ * If a filter throws an exception or if any filter rejects the authentication
+ */
+ private void notifyAuthenticationSuccessListeners(
+ AuthenticatedUser authenticatedUser, GuacamoleSession session)
+ throws GuacamoleException {
+ UserContext userContext = null;
--- End diff --
Style nit-pick: Probably good to add a space above this line between the header and body of the method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133609931
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole.rest.event;
+
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.extension.ListenerProvider;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.AuthenticationFailureListener;
+import org.apache.guacamole.net.event.listener.AuthenticationSuccessListener;
+import org.apache.guacamole.net.event.listener.TunnelCloseListener;
+import org.apache.guacamole.net.event.listener.TunnelConnectListener;
+
+import java.util.List;
+
+/**
+ * A service used to notify listeners registered by extensions when events of
+ * interest occur.
+ *
+ * @author Carl Harris
--- End diff --
We don't do @author tags, anymore.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143106160
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationSuccessEvent) {
+ if (!delegate.authenticationSucceeded((AuthenticationSuccessEvent) event)) {
+ throw new GuacamoleSecurityException(
+ "listener vetoed successful authentication");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationFailureListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationFailureListenerAdapter implements Listener {
+
+ private final AuthenticationFailureListener delegate;
--- End diff --
Missing JavaDoc.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143106189
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationSuccessEvent) {
+ if (!delegate.authenticationSucceeded((AuthenticationSuccessEvent) event)) {
+ throw new GuacamoleSecurityException(
+ "listener vetoed successful authentication");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationFailureListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationFailureListenerAdapter implements Listener {
+
+ private final AuthenticationFailureListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104478
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/Listener.java ---
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.event.listener;
+
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * A listener for events that occur in handing various Guacamole requests
+ * such as authentication, tunnel connect/close, etc. Listeners are registered
+ * through the extension manifest mechanism. When an event occurs, listeners
+ * are notified in the order in which they are declared in the manifest and
+ * continues until either all listeners have been notified or with the first
+ * listener that throws a GuacamoleException or other runtime exception.
+ */
+public interface Listener {
+
+ /**
+ * Notifies the recipient that an event has occurred.
+ * <p>
+ * Throwing an exception from an event listener can act to veto an action in
+ * progress for some event types. See the Javadoc for specific event types for
+ * details.
+ *
+ * @param event
+ * an object that describes the subject event
--- End diff --
Should be sentence case (plus a period).
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133608633
--- Diff: guacamole/src/main/java/org/apache/guacamole/tunnel/TunnelRequestService.java ---
@@ -226,7 +300,7 @@ protected GuacamoleTunnel createConnectedTunnel(UserContext context,
* @throws GuacamoleException
* If an error occurs while obtaining the tunnel.
*/
- protected GuacamoleTunnel createAssociatedTunnel(GuacamoleTunnel tunnel,
+ protected GuacamoleTunnel createAssociatedTunnel(final GuacamoleTunnel tunnel,
--- End diff --
Any particular reason for changing this to final, here? Might be a good reason, just asking...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105739
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
--- End diff --
Missing JavaDoc.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133608995
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,34 @@
+/*
--- End diff --
Style issue here - should start with
`/**`
(two asterisks).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105098
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java ---
@@ -208,6 +218,44 @@ private AuthenticatedUser updateAuthenticatedUser(AuthenticatedUser authenticate
}
/**
+ * Notify all bound listeners that a successful authentication
+ * has occurred.
+ *
+ * @param authenticatedUser
+ * The user that was successfully authenticated
+ * @param session
+ * Existing session for the user (if any)
+ * @throws GuacamoleException
+ * If thrown by a listener
+ */
+ private void fireAuthenticationSuccessEvent(
+ AuthenticatedUser authenticatedUser, GuacamoleSession session)
+ throws GuacamoleException {
+
+ UserContext userContext = null;
+ if (session != null) {
+ userContext = session.getUserContext(
+ authenticatedUser.getAuthenticationProvider().getIdentifier());
+ }
+
+ listenerService.handleEvent(new AuthenticationSuccessEvent(
+ userContext, authenticatedUser.getCredentials()));
+ }
+
+ /**
+ * Notify all bound listeners that an authentication attempt has failed.
+ *
+ * @param credentials
+ * The credentials that failed to authenticate
+ * @throws GuacamoleException
--- End diff --
Missing blank lines between the various `@param` and `@throws, as well as the periods.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133654470
--- Diff: guacamole/src/main/java/org/apache/guacamole/tunnel/TunnelRequestService.java ---
@@ -226,7 +300,7 @@ protected GuacamoleTunnel createConnectedTunnel(UserContext context,
* @throws GuacamoleException
* If an error occurs while obtaining the tunnel.
*/
- protected GuacamoleTunnel createAssociatedTunnel(GuacamoleTunnel tunnel,
+ protected GuacamoleTunnel createAssociatedTunnel(final GuacamoleTunnel tunnel,
--- End diff --
It gets referenced in the anonymous inner `UserTunnel` type. Previously, it wasn't referenced, so this didn't need to be final.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104494
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/TunnelCloseListener.java ---
@@ -25,11 +25,15 @@
/**
* A listener whose tunnelClosed() hook will fire immediately after an
* existing tunnel is closed.
+ *
+ * @deprecated
+ * Listeners should instead implement the {@link Listener} interface
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104963
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ProviderFactory.java ---
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+
+/**
+ * Static factory method for creating provider instances and logging unexpected outcomes
+ * with sufficient detail to allow user/developer debugging.
+ */
+class ProviderFactory {
+
+ private static final Logger logger = LoggerFactory.getLogger(ProviderFactory.class);
+
+ /**
+ * Creates an instance of the specified provider class using the no-arg constructor.
+ *
+ * @param typeName
+ * The provider type name used for log messages (e.g. "authentication provider")
+ * @param providerClass
--- End diff --
Missing blank lines between the various `@param`.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133610612
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerProvider.java ---
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.net.event.listener.AuthenticationFailureListener;
+import org.apache.guacamole.net.event.listener.AuthenticationSuccessListener;
+import org.apache.guacamole.net.event.listener.TunnelCloseListener;
+import org.apache.guacamole.net.event.listener.TunnelConnectListener;
+
+/**
+ * A provider of an event listener. While an implementation of this interface
+ * must implement all of the specified listener interfaces, an implementation
+ * may selectively deliver event notifications to an underlying delegate based
+ * on the specific listener interfaces implemented by the delegate.
+ */
+public interface ListenerProvider extends AuthenticationSuccessListener,
+ AuthenticationFailureListener, TunnelConnectListener,
+ TunnelCloseListener {
+}
--- End diff --
Style: add a space here, I think....
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105024
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java ---
@@ -75,6 +79,12 @@
private AuthTokenGenerator authTokenGenerator;
/**
+ * The service to use to notify registered authentication listeners
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143106213
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationSuccessEvent) {
+ if (!delegate.authenticationSucceeded((AuthenticationSuccessEvent) event)) {
+ throw new GuacamoleSecurityException(
+ "listener vetoed successful authentication");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationFailureListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationFailureListenerAdapter implements Listener {
+
+ private final AuthenticationFailureListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationFailureListenerAdapter(AuthenticationFailureListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationFailureEvent by passing the event to the delegate
+ * listener. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationFailureEvent) {
+ delegate.authenticationFailed((AuthenticationFailureEvent) event);
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows a TunnelConnectListener to be used as an ordinary
+ * Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class TunnelConnectListenerAdapter implements Listener {
+
+ private final TunnelConnectListener delegate;
--- End diff --
Missing JavaDoc.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r134059609
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleTunnelConnectedException.java ---
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+/**
+ * An exception thrown when a request to close a tunnel is vetoed by a
+ * TunnelCloseListener in an extension.
+ */
+public class GuacamoleTunnelConnectedException extends GuacamoleClientException {
+
+ public GuacamoleTunnelConnectedException() {
--- End diff --
I would similarly question the utility of the tunnel-specific exceptions. In my view, simply specifying that throwing any `GuacamoleException` will veto the in-progress operation (as is currently done with auth) would be cleaner, as well as in-line with established patterns.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105331
--- Diff: guacamole/src/main/java/org/apache/guacamole/tunnel/TunnelRequestService.java ---
@@ -58,6 +62,58 @@
private AuthenticationService authenticationService;
/**
+ * A service for notifying listeners about tunnel connect/closed events.
+ */
+ @Inject
+ private ListenerService listenerService;
+
+ /**
+ * Notifies bound listeners that a new tunnel has been connected.
+ * Listeners may veto a connected tunnel by throwing any GuacamoleException.
+ *
+ * @param userContext
+ * The UserContext associated with the user for whom the tunnel is
+ * being created.
+ *
+ * @param credentials
+ * Credentials that authenticate the user
--- End diff --
Missing period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143104552
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/TunnelCloseListener.java ---
@@ -25,11 +25,15 @@
/**
* A listener whose tunnelClosed() hook will fire immediately after an
* existing tunnel is closed.
+ *
+ * @deprecated
+ * Listeners should instead implement the {@link Listener} interface
*/
+@Deprecated
public interface TunnelCloseListener {
/**
- * Event hook which fires immediately after an existing tunnel is closed.
+ * Event hook which fires immediately before an existing tunnel is closed.
--- End diff --
The comment at the interface level still says "after". If it's "before" that's correct, and the top-level comment should be updated, as well.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143105120
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.rest.event;
+
+import java.util.List;
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.net.event.listener.Listener;
+
+/**
+ * A service used to notify listeners registered by extensions when events of
+ * interest occur.
+ */
+public class ListenerService implements Listener {
+
+ @Inject
+ private List<Listener> listeners;
--- End diff --
Missing JavaDoc.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133609979
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/event/ListenerService.java ---
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole.rest.event;
+
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.extension.ListenerProvider;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.AuthenticationFailureListener;
+import org.apache.guacamole.net.event.listener.AuthenticationSuccessListener;
+import org.apache.guacamole.net.event.listener.TunnelCloseListener;
+import org.apache.guacamole.net.event.listener.TunnelConnectListener;
+
+import java.util.List;
+
+/**
+ * A service used to notify listeners registered by extensions when events of
+ * interest occur.
+ *
+ * @author Carl Harris
+ */
+public class ListenerService implements ListenerProvider {
+
+ @Inject
+ private List<ListenerProvider> listeners;
+
+ /**
+ * Notifies all bound listeners of an authentication success event. Listeners
+ * are allowed to veto a successful authentication by returning false from the
+ * listener method. Regardless of whether a particular listener rejects the
+ * successful authentication, all listeners are notified.
+ * @param e
+ * The AuthenticationSuccessEvent describing the successful authentication
+ * that just occurred.
+ *
+ * @return
+ * false if any bound listener returns false, else true
+ *
+ * @throws GuacamoleException
+ * If any bound listener throws this exception. If a listener throws an exception
+ * some listeners may not receive the authentication success event notification.
+ */
+ @Override
+ public boolean authenticationSucceeded(AuthenticationSuccessEvent e)
+ throws GuacamoleException {
+ boolean result = true;
+ for (AuthenticationSuccessListener listener : listeners) {
+ result = result && listener.authenticationSucceeded(e);
+ }
+ return result;
+ }
+
+ /**
+ * Notifies all bound listeners of an authentication failure event.
+ *
+ * @param e
+ * The AuthenticationSuccessEvent describing the authentication failure
+ * that just occurred.
+ *
+ * @throws GuacamoleException
+ * If any bound listener throws this exception. If a listener throws an exception
+ * some listeners may not receive the authentication failure event notification.
+ */
+ @Override
+ public void authenticationFailed(AuthenticationFailureEvent e)
+ throws GuacamoleException {
+ for (AuthenticationFailureListener listener : listeners) {
+ listener.authenticationFailed(e);
+ }
+ }
+
+ /**
+ * Notifies all bound listeners of an tunnel connected event. Listeners
+ * are allowed to veto a tunnel connection by returning false from the
+ * listener method. Regardless of whether a particular listener rejects the
+ * tunnel connection, all listeners are notified.
+ * @param e
--- End diff --
Style: add line spacing here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133656761
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,34 @@
+/*
--- End diff --
This is not a javadoc comment and should not start with two asterisks. Checking the other license headers in the project, I found none that were written with two leading asterisks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133654410
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java ---
@@ -66,58 +65,8 @@
* The AuthenticationProvider subclass to instantiate.
*/
public AuthenticationProviderFacade(Class<? extends AuthenticationProvider> authProviderClass) {
-
- AuthenticationProvider instance = null;
-
- try {
- // Attempt to instantiate the authentication provider
- instance = authProviderClass.getConstructor().newInstance();
- }
- catch (NoSuchMethodException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("AuthenticationProvider is missing a default constructor.", e);
- }
- catch (SecurityException e) {
- logger.error("The Java security mananager is preventing authentication extensions "
- + "from being loaded. Please check the configuration of Java or your "
- + "servlet container.");
- logger.debug("Creation of AuthenticationProvider disallowed by security manager.", e);
- }
- catch (InstantiationException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("AuthenticationProvider cannot be instantiated.", e);
- }
- catch (IllegalAccessException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("Default constructor of AuthenticationProvider is not public.", e);
- }
- catch (IllegalArgumentException e) {
- logger.error("The authentication extension in use is not properly defined. "
- + "Please contact the developers of the extension or, if you "
- + "are the developer, turn on debug-level logging.");
- logger.debug("Default constructor of AuthenticationProvider cannot accept zero arguments.", e);
- }
- catch (InvocationTargetException e) {
-
- // Obtain causing error - create relatively-informative stub error if cause is unknown
- Throwable cause = e.getCause();
- if (cause == null)
- cause = new GuacamoleException("Error encountered during initialization.");
-
- logger.error("Authentication extension failed to start: {}", cause.getMessage());
- logger.debug("AuthenticationProvider instantiation failed.", e);
-
- }
-
- // Associate instance, if any
- authProvider = instance;
-
+ authProvider = ProviderFactory.newInstance("authentication provider",
+ authProviderClass);
--- End diff --
Pulled it out so that the same logic could be reused by `ListenerFacade` rather than duplicating it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r143106204
--- Diff: guacamole/src/main/java/org/apache/guacamole/extension/ListenerFactory.java ---
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.extension;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleSecurityException;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.TunnelCloseEvent;
+import org.apache.guacamole.net.event.TunnelConnectEvent;
+import org.apache.guacamole.net.event.listener.*;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * A factory that reflectively instantiates Listener objects for a given
+ * provider class.
+ */
+class ListenerFactory {
+
+ /**
+ * Creates all listeners represented by an instance of the given provider class.
+ * <p>
+ * If a provider class implements the simple Listener interface, that is the
+ * only listener type that will be returned. Otherwise, a list of Listener
+ * objects that adapt the legacy listener interfaces will be returned.
+ *
+ * @param providerClass
+ * a class that represents a listener provider
+ *
+ * @return
+ * list of listeners represented by the given provider class
+ */
+ static List<Listener> createListeners(Class<?> providerClass) {
+
+ Object provider = ProviderFactory.newInstance("listener", providerClass);
+
+ if (provider instanceof Listener) {
+ return Collections.singletonList((Listener) provider);
+ }
+
+ return createListenerAdapters(provider);
+
+ }
+
+ @SuppressWarnings("deprecation")
+ private static List<Listener> createListenerAdapters(Object provider) {
+
+ final List<Listener> listeners = new ArrayList<Listener>();
+
+ if (provider instanceof AuthenticationSuccessListener) {
+ listeners.add(new AuthenticationSuccessListenerAdapter(
+ (AuthenticationSuccessListener) provider));
+ }
+
+ if (provider instanceof AuthenticationFailureListener) {
+ listeners.add(new AuthenticationFailureListenerAdapter(
+ (AuthenticationFailureListener) provider));
+ }
+
+ if (provider instanceof TunnelConnectListener) {
+ listeners.add(new TunnelConnectListenerAdapter(
+ (TunnelConnectListener) provider));
+ }
+
+ if (provider instanceof TunnelCloseListener) {
+ listeners.add(new TunnelCloseListenerAdapter(
+ (TunnelCloseListener) provider));
+ }
+
+ return listeners;
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationSuccessListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationSuccessListenerAdapter implements Listener {
+
+ private final AuthenticationSuccessListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationSuccessListenerAdapter(AuthenticationSuccessListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationSuccessEvent by passing the event to the delegate
+ * listener. If the delegate returns false, the adapter throws a GuacamoleException
+ * to veto the authentication success event. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
+ */
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+ if (event instanceof AuthenticationSuccessEvent) {
+ if (!delegate.authenticationSucceeded((AuthenticationSuccessEvent) event)) {
+ throw new GuacamoleSecurityException(
+ "listener vetoed successful authentication");
+ }
+ }
+ }
+
+ }
+
+ /**
+ * An adapter the allows an AuthenticationFailureListener to be used
+ * as an ordinary Listener.
+ */
+ @SuppressWarnings("deprecation")
+ private static class AuthenticationFailureListenerAdapter implements Listener {
+
+ private final AuthenticationFailureListener delegate;
+
+ /**
+ * Constructs a new adapter.
+ *
+ * @param delegate
+ * the delegate listener
+ */
+ AuthenticationFailureListenerAdapter(AuthenticationFailureListener delegate) {
+ this.delegate = delegate;
+ }
+
+ /**
+ * Handles an AuthenticationFailureEvent by passing the event to the delegate
+ * listener. All other event types are ignored.
+ *
+ * @param event
+ * an object that describes the subject event
+ *
+ * @throws GuacamoleException
+ * if thrown by the delegate listener
--- End diff --
Should be sentence case, plus a period.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r134061675
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/listener/Listener.java ---
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.event.listener;
+
+/**
+ * A marker interface extended by all listener types. This interface is used
--- End diff --
In fact ... if things move toward the style of veto used by the rest of the auth system (throw a `GuacamoleException` to abort the operation), then there definitely could be a common event handler, as there would be no need to rely on a `boolean` return value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r134056483
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+/**
+ * An exception thrown when a successful authentication is rejected by a
+ * AuthenticationSuccessListener in an extension.
+ */
+public class GuacamoleAuthenticationRejectedException
+ extends GuacamoleSecurityException {
+
+ public GuacamoleAuthenticationRejectedException() {
--- End diff --
I haven't yet checked whether these exceptions make sense as API changes overall, but beware that the constructors of all classes need to be documented with javadoc, including exceptions.
It is also standard convention to provide the message, message/cause, and cause constructors. It would be odd if these exceptions are the only exceptions in the Guacamole API which deviate from that convention, instead providing their own default message.
See:
https://github.com/apache/incubator-guacamole-client/blob/a5ebc07349bcf43c80f404836d9964ac894bed26/guacamole-common/src/main/java/org/apache/guacamole/GuacamoleResourceClosedException.java#L30-L62
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #184: GUACAMOLE-364: authentication ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/184#discussion_r133606749
--- Diff: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleAuthenticationRejectedException.java ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.guacamole;
+
+/**
+ * An exception thrown when a successful authentication is rejected by a
+ * AuthenticationSuccessListener in an extension.
+ */
+public class GuacamoleAuthenticationRejectedException
--- End diff --
Any particular reason you're implementing a new exception instead of using one of the existing ones, like GuacamoleInvalidCredentialsException? You can set custom messages for the existing exceptions, so if all you're doing is calling the exception and putting in a custom message, would probably be best to just call the existing exceptions. I glanced at the existing exceptions, and most of them have at least custom status codes that get passed over to the web application.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---