mike-jumper commented on code in PR #780: URL: https://github.com/apache/guacamole-client/pull/780#discussion_r1040031742
########## extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/RestrictedDirectory.java: ########## @@ -0,0 +1,132 @@ +package org.apache.guacamole.auth.jdbc.base; +/* + * 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. + */ + +import java.util.Collection; +import java.util.Set; + +import javax.annotation.Nonnull; + +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.GuacamoleUnauthorizedException; +import org.apache.guacamole.auth.jdbc.user.ModeledAuthenticatedUser; +import org.apache.guacamole.auth.jdbc.user.ModeledUser; +import org.apache.guacamole.net.auth.Directory; +import org.apache.guacamole.net.auth.Identifiable; + +/** + * A directory that will wrap another provided directory, and automatically + * perform user access window restrictions before every directory operation, + * delegating the operation itself to the wrapped directory. + * + * If the user access check fails, an exception will be thrown to block access + * to the wrapped directory. + */ +public class RestrictedDirectory<T extends Identifiable> implements Directory<T> { Review Comment: May save some trouble to `extend DelegatingDirectory<T>`. ########## extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AbstractGuacamoleTunnelService.java: ########## @@ -736,7 +778,10 @@ public GuacamoleTunnel getGuacamoleTunnel(RemoteAuthenticatedUser user, user, definition.getActiveConnection(), definition.getSharingProfile()); // Connect to shared connection described by the created record - GuacamoleTunnel tunnel = assignGuacamoleTunnel(connectionRecord, info, tokens, false); + GuacamoleTunnel tunnel = assignGuacamoleTunnel( + connectionRecord, userService.retrieveAuthenticatedUser( + user.getAuthenticationProvider(), user.getCredentials()), + info, tokens, false); Review Comment: Why re-retrieve the `AuthenticatedUser`? ########## extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/RestrictedDirectory.java: ########## @@ -0,0 +1,132 @@ +package org.apache.guacamole.auth.jdbc.base; +/* + * 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. + */ + +import java.util.Collection; +import java.util.Set; + +import javax.annotation.Nonnull; + +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.GuacamoleUnauthorizedException; +import org.apache.guacamole.auth.jdbc.user.ModeledAuthenticatedUser; +import org.apache.guacamole.auth.jdbc.user.ModeledUser; +import org.apache.guacamole.net.auth.Directory; +import org.apache.guacamole.net.auth.Identifiable; + +/** + * A directory that will wrap another provided directory, and automatically + * perform user access window restrictions before every directory operation, + * delegating the operation itself to the wrapped directory. + * + * If the user access check fails, an exception will be thrown to block access + * to the wrapped directory. + */ +public class RestrictedDirectory<T extends Identifiable> implements Directory<T> { + + /** + * The user whose access window restrictions are being applied to the + * wrapped directory. + */ + private ModeledAuthenticatedUser currentUser; + + /** + * The wrapped directory to which all operations will be delegated. + */ + private Directory<T> wrappedDirectory; + + /** + * Create a new RestrictedDirectory that will automatically enforce user + * access window restrictions for all directory operations, delegating the + * results of the operations to the wrapped directory. + * + * @param wrappedDirectory; + * The directory to delegate all directory operations to. + * + * @param user + * The user whose access window restrictions will be applied to the + * directory. + */ + public RestrictedDirectory( + @Nonnull Directory<T> wrappedDirectory, + @Nonnull ModeledAuthenticatedUser currentUser) { + + this.currentUser = currentUser; + this.wrappedDirectory = wrappedDirectory; + } + + @Override + public T get(String identifier) throws GuacamoleException { + validateUserAccess(); + return wrappedDirectory.get(identifier); + } + + @Override + public Collection<T> getAll(Collection<String> identifiers) throws GuacamoleException { + validateUserAccess(); + return wrappedDirectory.getAll(identifiers); + } + + @Override + public Set<String> getIdentifiers() throws GuacamoleException { + validateUserAccess(); + return wrappedDirectory.getIdentifiers(); + } + + @Override + public void add(T object) throws GuacamoleException { + validateUserAccess(); + wrappedDirectory.add(object); + } + + @Override + public void update(T object) throws GuacamoleException { + validateUserAccess(); + wrappedDirectory.update(object); + } + + @Override + public void remove(String identifier) throws GuacamoleException { + validateUserAccess(); + wrappedDirectory.remove(identifier); + } + + /** + * Validate that the current user is within a valid access time window + * and not disabled. If the user account is disabled or not within a + * valid access window, a GuacamoleUnauthorizedException will be thrown. + * + * @throws GuacamoleException + * If the user is outside of a valid access window, the user is + * disabled, or an error occurs while trying to determine access time + * restriction configuration. + */ + protected void validateUserAccess() throws GuacamoleException { + + // If the user is outside of a valid access time window or disabled, + // throw an exception to immediately log them out + ModeledUser modeledUser = currentUser.getUser(); + if ( + !modeledUser.isAccountAccessible() + || !modeledUser.isAccountValid() + || modeledUser.isDisabled() + ) + throw new GuacamoleUnauthorizedException("Permission Denied."); Review Comment: Except for perhaps `isDisabled()`, which is supposed to behave as if the user does not exist, I think we should use `TranslatableGuacamoleUnauthorizedException` and the user-facing messages created for this purpose: https://github.com/apache/guacamole-client/blob/6c43611f5198a18cfaf582fcbc1fbcfb1efbda1f/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java#L113-L123 ########## extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AccessEnforcingDelegatingTunnel.java: ########## @@ -0,0 +1,196 @@ +/* + * 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.jdbc.tunnel; + +import java.util.concurrent.atomic.AtomicReference; + +import javax.annotation.Nonnull; + +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.GuacamoleUnauthorizedException; +import org.apache.guacamole.auth.jdbc.user.ModeledAuthenticatedUser; +import org.apache.guacamole.auth.jdbc.user.ModeledUser; +import org.apache.guacamole.auth.jdbc.user.UserService; +import org.apache.guacamole.io.GuacamoleReader; +import org.apache.guacamole.net.DelegatingGuacamoleTunnel; +import org.apache.guacamole.net.GuacamoleTunnel; +import org.apache.guacamole.protocol.FilteredGuacamoleReader; +import org.apache.guacamole.protocol.GuacamoleFilter; +import org.apache.guacamole.protocol.GuacamoleInstruction; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.google.inject.Inject; +import com.google.inject.assistedinject.Assisted; +import com.google.inject.assistedinject.AssistedInject; + +/** + * A tunnel implementation that enforces access window restriction for the + * provided ModeledUser, throwing a GuacamoleUnauthorizedException if the + * user's configured access window has closed, or if the user has become + * disabled. All other tunnel implementation is delegated to the underlying + * tunnel object. + */ +public class AccessEnforcingDelegatingTunnel extends DelegatingGuacamoleTunnel { + + /** + * Logger for this class. + */ + private static final Logger logger = LoggerFactory.getLogger( + AccessEnforcingDelegatingTunnel.class); + + /** + * The number of milliseconds between subsequent refreshes of the user + * from the DB. + */ + private static final long USER_MODEL_REFRESH_INTERVAL = 10000; + + /** + * The user who's access window restrictions should be applied for the + * wrapped tunnel. + */ + private final AtomicReference<ModeledUser> user; + + /** + * A thread that will continously refresh the user + */ + private final Thread userRefreshThread; + + /** + * A service to use for refreshing the user from the DB. + */ + @Inject + private UserService userService; + + /** + * Create a new tunnel that will enforce the access window restrictions of + * the provided user, during usage of the provided tunnel. + * + * @param tunnel + * The tunnel to delegate to. + * + * @param modeledAuthenticatedUser + * The user whose access restrictions should be applied. + * + */ + @AssistedInject + public AccessEnforcingDelegatingTunnel( + @Nonnull @Assisted GuacamoleTunnel tunnel, + @Nonnull @Assisted ModeledAuthenticatedUser modeledAuthenticatedUser) { + + super(tunnel); + this.user = new AtomicReference<>(modeledAuthenticatedUser.getUser()); + + this.userRefreshThread = new Thread(() -> { + while (true) { + + try { + + // Fetch an up-to-date user record from the DB to ensure + // that any access restrictions modified while this tunnel + // is open will be taken into account + this.user.set(userService.retrieveUser( + modeledAuthenticatedUser.getAuthenticationProvider(), + modeledAuthenticatedUser)); + } + + // If an error occurs while trying to fetch the updated user, + // log the warning / exception and stop the refresh thread + catch (GuacamoleException e) { + + logger.warn( + "Aborting user refresh thread due to error: {}", + e.getMessage()); + logger.debug( + "Exception caught while attempting to refresh user.", e); + + return; + } + + try { + + // Wait a bit before refreshing the user record again + Thread.sleep(USER_MODEL_REFRESH_INTERVAL); + } + + // If interrupted by the tunnel, exit immediately + catch (InterruptedException e) { + return; + } + } + }); + } + + @Override + public GuacamoleReader acquireReader() { + + // Start periodically refreshing the user record + userRefreshThread.start(); + + // Filter received instructions, checking if the user's login + // is still valid for each one. If the login is invalid, + // log them out immediately and close the tunnel. + return new FilteredGuacamoleReader( + super.acquireReader(), + new GuacamoleFilter() { Review Comment: Rather than override `acquireReader()`, etc., it would be better to override `getSocket()` and use a `FilteredGuacamoleSocket`. The same filter implementation could be used for both reads and writes. ########## extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AccessEnforcingDelegatingTunnel.java: ########## @@ -0,0 +1,196 @@ +/* + * 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.jdbc.tunnel; + +import java.util.concurrent.atomic.AtomicReference; + +import javax.annotation.Nonnull; + +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.GuacamoleUnauthorizedException; +import org.apache.guacamole.auth.jdbc.user.ModeledAuthenticatedUser; +import org.apache.guacamole.auth.jdbc.user.ModeledUser; +import org.apache.guacamole.auth.jdbc.user.UserService; +import org.apache.guacamole.io.GuacamoleReader; +import org.apache.guacamole.net.DelegatingGuacamoleTunnel; +import org.apache.guacamole.net.GuacamoleTunnel; +import org.apache.guacamole.protocol.FilteredGuacamoleReader; +import org.apache.guacamole.protocol.GuacamoleFilter; +import org.apache.guacamole.protocol.GuacamoleInstruction; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.google.inject.Inject; +import com.google.inject.assistedinject.Assisted; +import com.google.inject.assistedinject.AssistedInject; + +/** + * A tunnel implementation that enforces access window restriction for the + * provided ModeledUser, throwing a GuacamoleUnauthorizedException if the + * user's configured access window has closed, or if the user has become + * disabled. All other tunnel implementation is delegated to the underlying + * tunnel object. + */ +public class AccessEnforcingDelegatingTunnel extends DelegatingGuacamoleTunnel { + + /** + * Logger for this class. + */ + private static final Logger logger = LoggerFactory.getLogger( + AccessEnforcingDelegatingTunnel.class); + + /** + * The number of milliseconds between subsequent refreshes of the user + * from the DB. + */ + private static final long USER_MODEL_REFRESH_INTERVAL = 10000; + + /** + * The user who's access window restrictions should be applied for the + * wrapped tunnel. + */ + private final AtomicReference<ModeledUser> user; + + /** + * A thread that will continously refresh the user + */ + private final Thread userRefreshThread; + + /** + * A service to use for refreshing the user from the DB. + */ + @Inject + private UserService userService; + + /** + * Create a new tunnel that will enforce the access window restrictions of + * the provided user, during usage of the provided tunnel. + * + * @param tunnel + * The tunnel to delegate to. + * + * @param modeledAuthenticatedUser + * The user whose access restrictions should be applied. + * + */ + @AssistedInject + public AccessEnforcingDelegatingTunnel( + @Nonnull @Assisted GuacamoleTunnel tunnel, + @Nonnull @Assisted ModeledAuthenticatedUser modeledAuthenticatedUser) { + + super(tunnel); + this.user = new AtomicReference<>(modeledAuthenticatedUser.getUser()); + + this.userRefreshThread = new Thread(() -> { + while (true) { + + try { + + // Fetch an up-to-date user record from the DB to ensure + // that any access restrictions modified while this tunnel + // is open will be taken into account + this.user.set(userService.retrieveUser( + modeledAuthenticatedUser.getAuthenticationProvider(), + modeledAuthenticatedUser)); Review Comment: I think this might cause problems at scale if we're executing a SQL query every 10 seconds for every connection across every user. It should instead occur once across all logged-in users (ie: at the `AuthenticationProvider` level), presumably at most once per minute. -- 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]
