mike-jumper commented on code in PR #846:
URL: https://github.com/apache/guacamole-client/pull/846#discussion_r1244020898
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-base/src/main/java/org/apache/guacamole/auth/sso/AuthenticationSessionManager.java:
##########
@@ -97,9 +149,21 @@ public String generateInvalid() {
* value was returned by defer().
*/
public T resume(String identifier) {
-
if (identifier != null) {
- T session = sessions.remove(identifier);
+ T session = sessions.get(identifier);
+
+ // If this session is already pending, invalidate and return null
+ if (pendingSessions.contains(identifier)) {
+ invalidateSession(identifier);
+ return null;
+ }
+
+ // Otherwise, mark the session as pending. NOTE: Unless explicitly
+ // removed from pending status via a call to reactivateSession(),
+ // the next attempt to resume this session will fail
+ else
+ pendingSessions.add(identifier);
Review Comment:
Though the underlying collections are threadsafe, this access pattern is
not. The intent appears to be that, given two separate calls to
`resume("someValue")`, the first call succeeds and returns a non-null session
while the second call returns `null`, but this may not happen. Consider:
| Thread 1 | Thread 2
|
| ----------------------------------------------------- |
------------------------------------------------------ |
|`resume("someValue")`||
| |
`resume("someValue")` |
| `if (identifier != null) {` (true) |
|
| | `if (identifier !=
null) {` (true) |
| `T session = sessions.get(identifier);` |
|
| | `T session =
sessions.get(identifier);` |
| `if (pendingSessions.contains(identifier)) {` (false) |
|
| | `if
(pendingSessions.contains(identifier)) {` (false) |
| `pendingSessions.add(identifier);` | |
| |
`pendingSessions.add(identifier);` |
| `if (session != null && session.isValid())` (true) | |
| | `if (session !=
null && session.isValid())` (true) |
| `return session;` | |
| | `return session;`
|
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-base/src/main/java/org/apache/guacamole/auth/sso/AuthenticationSessionManager.java:
##########
@@ -97,9 +149,21 @@ public String generateInvalid() {
* value was returned by defer().
*/
public T resume(String identifier) {
-
if (identifier != null) {
- T session = sessions.remove(identifier);
+ T session = sessions.get(identifier);
Review Comment:
Indentation seems to have gotten wonky here (misaligned by one space).
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-ssl/src/main/java/org/apache/guacamole/auth/ssl/AuthenticationEventListener.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.auth.ssl;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.auth.ssl.SSLAuthenticationSessionManager;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.listener.Listener;
+
+/**
+ * A Listener that will reactivate or invalidate SSL auth sessions depending on
+ * overall auth success or failure.
+ */
+@Singleton
+public class AuthenticationEventListener implements Listener {
+
+ /**
+ * Session manager for generating and maintaining unique tokens to
+ * represent the authentication flow of a user who has only partially
+ * authenticated.
+ *
+ * Requires static injection due to the fact that the webapp just calls the
+ * constructor directly when creating new Listeners. The instances will not
+ * be constructed by guice.
+ */
+ @Inject
+ private static SSLAuthenticationSessionManager sessionManager;
+
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+
+ if (event instanceof AuthenticationSuccessEvent)
+
+ // After an auth attempt has fully succeeded, invalidate the
session
+ // associated with the successful login event so it can't be reused
+ sessionManager.invalidateSession(
+ AuthenticationProviderService.getSessionIdentifier(
+ ((AuthenticationSuccessEvent) event).getCredentials()));
+
+ else if (event instanceof AuthenticationFailureEvent)
+
+ // If the SSL auth succeeded, but other auth providers failed to
+ // authenticate the user associated with the credentials in this
+ // failure event, they may wish to make another login attempt. To
+ // avoid an infinite login attempt loop, re-enable the session
+ // associated with these credentials, allowing the auth attempt to
be
+ // resumed without requiring another round trip to the SSL service.
+ sessionManager.reactivateSession(
+ AuthenticationProviderService.getSessionIdentifier(
+ ((AuthenticationFailureEvent) event).getCredentials()));
+
+ }
+
+}
Review Comment:
Could this and the SAML implementation perhaps be brought up into
`guacamole-auth-sso-base` as a base class that both implementations might
leverage? It looks like they apply the same logic.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-ssl/src/main/java/org/apache/guacamole/auth/ssl/AuthenticationEventListener.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.auth.ssl;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.auth.ssl.SSLAuthenticationSessionManager;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.listener.Listener;
+
+/**
+ * A Listener that will reactivate or invalidate SSL auth sessions depending on
+ * overall auth success or failure.
+ */
+@Singleton
+public class AuthenticationEventListener implements Listener {
+
+ /**
+ * Session manager for generating and maintaining unique tokens to
+ * represent the authentication flow of a user who has only partially
+ * authenticated.
+ *
+ * Requires static injection due to the fact that the webapp just calls the
+ * constructor directly when creating new Listeners. The instances will not
+ * be constructed by guice.
+ */
+ @Inject
+ private static SSLAuthenticationSessionManager sessionManager;
+
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+
+ if (event instanceof AuthenticationSuccessEvent)
+
+ // After an auth attempt has fully succeeded, invalidate the
session
+ // associated with the successful login event so it can't be reused
+ sessionManager.invalidateSession(
+ AuthenticationProviderService.getSessionIdentifier(
+ ((AuthenticationSuccessEvent) event).getCredentials()));
+
+ else if (event instanceof AuthenticationFailureEvent)
+
+ // If the SSL auth succeeded, but other auth providers failed to
+ // authenticate the user associated with the credentials in this
+ // failure event, they may wish to make another login attempt. To
+ // avoid an infinite login attempt loop, re-enable the session
+ // associated with these credentials, allowing the auth attempt to
be
+ // resumed without requiring another round trip to the SSL service.
+ sessionManager.reactivateSession(
+ AuthenticationProviderService.getSessionIdentifier(
+ ((AuthenticationFailureEvent) event).getCredentials()));
Review Comment:
Same here - this should probably only happen for specific exceptions, with
other exceptions invalidating the session.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java:
##########
@@ -69,6 +69,26 @@ public class AuthenticationProviderService implements
SSOAuthenticationProviderS
@Inject
private SAMLService saml;
+ /**
+ * Return the value of the session identifier associated with the given
+ * credentials, or null if no session identifier is found in the
+ * credentials.
+ *
+ * @param credentials The credentials from which to extract the session
+ * identifier.
+ *
+ * @return The session identifier associated with the given credentials, or
+ * null if no identifier is found.
Review Comment:
Please reformat to match style of other JavaDoc.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationEventListener.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.auth.saml;
+
+import com.google.inject.Inject;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.auth.saml.acs.SAMLAuthenticationSessionManager;
+import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
+import org.apache.guacamole.net.event.listener.Listener;
+
+/**
+ * A Listener that will reactivate or invalidate SAML auth sessions depending
on
+ * overall auth success or failure.
+ */
+public class AuthenticationEventListener implements Listener {
+
+ /**
+ * Manager of active SAML authentication attempts.
+ *
+ * Requires static injection due to the fact that the webapp just calls the
+ * constructor directly when creating new Listeners. The instances will not
+ * be constructed by guice.
+ */
+ @Inject
+ private static SAMLAuthenticationSessionManager sessionManager;
+
+ @Override
+ public void handleEvent(Object event) throws GuacamoleException {
+
+ if (event instanceof AuthenticationSuccessEvent)
+
+ // After an auth attempt has fully succeeded, invalidate the
session
+ // associated with the successful login event so it can't be reused
+ sessionManager.invalidateSession(
+ AuthenticationProviderService.getSessionIdentifier(
+ ((AuthenticationSuccessEvent) event).getCredentials()));
+
+ else if (event instanceof AuthenticationFailureEvent)
+
+ // If the SSL auth succeeded, but other auth providers failed to
+ // authenticate the user associated with the credentials in this
+ // failure event, they may wish to make another login attempt. To
+ // avoid an infinite login attempt loop, re-enable the session
+ // associated with these credentials, allowing the auth attempt to
be
+ // resumed without requiring another round trip to the SAML
provider.
+ sessionManager.reactivateSession(
+ AuthenticationProviderService.getSessionIdentifier(
+ ((AuthenticationFailureEvent) event).getCredentials()));
Review Comment:
This should probably only happen for specific exceptions, namely:
* `GuacamoleInsufficientCredentialsException`
* A `GuacamoleClientException` that isn't a `GuacamoleSecurityException`
Other failures like `GuacamoleInvalidCredentialsException`,
`GuacamoleUnauthorizedException`, or some unknown internal failure, should
instead fully invalidate the current flow as they did previously.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
