From 45ee895044cd7a4e5489ed0d4bd818368522a8ca Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 4 Jun 2017 13:32:52 -0700 Subject: [PATCH 1/5] GUACAMOLE-284: Veto authentication result if a database account is required but unavailable. --- .../auth/jdbc/JDBCAuthenticationProviderService.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java index dd39f245b..2e85e788c 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java @@ -104,8 +104,16 @@ public class JDBCAuthenticationProviderService implements AuthenticationProvider } - // Update password if password is expired + // Veto authentication result if account is required but unavailable + // due to account restrictions UserModel userModel = user.getModel(); + if (environment.isUserRequired() + && (userModel.isDisabled() || !user.isAccountValid() || !user.isAccountAccessible())) { + throw new GuacamoleInvalidCredentialsException("Invalid login", + CredentialsInfo.USERNAME_PASSWORD); + } + + // Update password if password is expired if (userModel.isExpired() || passwordPolicyService.isPasswordExpired(user)) userService.resetExpiredPassword(user, authenticatedUser.getCredentials()); From 0eef629a9dad12ad6d60a0d045e845236761be88 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 4 Jun 2017 13:42:28 -0700 Subject: [PATCH 2/5] GUACAMOLE-284: Move enforcement of account restrictions into AuthenticationProviderService. --- .../JDBCAuthenticationProviderService.java | 21 +++++++++++++++---- .../guacamole/auth/jdbc/user/UserService.java | 19 ++++------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java index 2e85e788c..a5cc164ea 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java @@ -21,9 +21,11 @@ package org.apache.guacamole.auth.jdbc; import com.google.inject.Inject; import com.google.inject.Provider; +import org.apache.guacamole.GuacamoleClientException; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.auth.jdbc.security.PasswordPolicyService; import org.apache.guacamole.auth.jdbc.sharing.user.SharedAuthenticatedUser; +import org.apache.guacamole.auth.jdbc.user.ModeledAuthenticatedUser; import org.apache.guacamole.auth.jdbc.user.ModeledUser; import org.apache.guacamole.auth.jdbc.user.ModeledUserContext; import org.apache.guacamole.auth.jdbc.user.UserModel; @@ -104,13 +106,24 @@ public class JDBCAuthenticationProviderService implements AuthenticationProvider } - // Veto authentication result if account is required but unavailable - // due to account restrictions + // Apply account restrictions if this extension authenticated the user + // OR if an account from this extension is explicitly required UserModel userModel = user.getModel(); - if (environment.isUserRequired() - && (userModel.isDisabled() || !user.isAccountValid() || !user.isAccountAccessible())) { + if (authenticatedUser instanceof ModeledAuthenticatedUser || environment.isUserRequired()) { + + // If user is disabled, pretend user does not exist + if (userModel.isDisabled()) throw new GuacamoleInvalidCredentialsException("Invalid login", CredentialsInfo.USERNAME_PASSWORD); + + // Verify user account is still valid as of today + if (!user.isAccountValid()) + throw new GuacamoleClientException("LOGIN.ERROR_NOT_VALID"); + + // Verify user account is allowed to be used at the current time + if (!user.isAccountAccessible()) + throw new GuacamoleClientException("LOGIN.ERROR_NOT_ACCESSIBLE"); + } // Update password if password is expired diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserService.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserService.java index 7935f864d..3dc025fcd 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserService.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserService.java @@ -312,9 +312,10 @@ public class UserService extends ModeledDirectoryObjectService Date: Sun, 4 Jun 2017 14:04:56 -0700 Subject: [PATCH 3/5] GUACAMOLE-284: Reverse structure of restriction enforcement such that the default action is to deny access. --- .../JDBCAuthenticationProviderService.java | 79 ++++++++++--------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java index a5cc164ea..37ff3bcae 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java @@ -88,52 +88,55 @@ public class JDBCAuthenticationProviderService implements AuthenticationProvider // Retrieve user account for already-authenticated user ModeledUser user = userService.retrieveUser(authenticationProvider, authenticatedUser); - if (user == null) { + if (user != null) { - // Do not invalidate the authentication result of users who were - // authenticated via our own connection sharing links - if (authenticatedUser instanceof SharedAuthenticatedUser) - return null; + // User data only exists for purposes of retrieval if the account + // is not disabled + UserModel userModel = user.getModel(); + if (!userModel.isDisabled()) { - // Simply return no data if a database user account is not required - if (!environment.isUserRequired()) - return null; + // Apply account restrictions if this extension authenticated + // the user OR if an account from this extension is explicitly + // required + if (authenticatedUser instanceof ModeledAuthenticatedUser + || environment.isUserRequired()) { - // Otherwise, invalidate the authentication result, as database user - // accounts are absolutely required - throw new GuacamoleInvalidCredentialsException("Invalid login", - CredentialsInfo.USERNAME_PASSWORD); + // Verify user account is still valid as of today + if (!user.isAccountValid()) + throw new GuacamoleClientException("LOGIN.ERROR_NOT_VALID"); + + // Verify user account is allowed to be used at the current time + if (!user.isAccountAccessible()) + throw new GuacamoleClientException("LOGIN.ERROR_NOT_ACCESSIBLE"); + + // Update password if password is expired + if (userModel.isExpired() || passwordPolicyService.isPasswordExpired(user)) + userService.resetExpiredPassword(user, authenticatedUser.getCredentials()); + + } + + // Link to user context + ModeledUserContext context = userContextProvider.get(); + context.init(user.getCurrentUser()); + return context; + + } } - // Apply account restrictions if this extension authenticated the user - // OR if an account from this extension is explicitly required - UserModel userModel = user.getModel(); - if (authenticatedUser instanceof ModeledAuthenticatedUser || environment.isUserRequired()) { + // Do not invalidate the authentication result of users who were + // authenticated via our own connection sharing links + if (authenticatedUser instanceof SharedAuthenticatedUser) + return null; - // If user is disabled, pretend user does not exist - if (userModel.isDisabled()) - throw new GuacamoleInvalidCredentialsException("Invalid login", - CredentialsInfo.USERNAME_PASSWORD); + // Simply return no data if a database user account is not required + if (!environment.isUserRequired()) + return null; - // Verify user account is still valid as of today - if (!user.isAccountValid()) - throw new GuacamoleClientException("LOGIN.ERROR_NOT_VALID"); - - // Verify user account is allowed to be used at the current time - if (!user.isAccountAccessible()) - throw new GuacamoleClientException("LOGIN.ERROR_NOT_ACCESSIBLE"); - - } - - // Update password if password is expired - if (userModel.isExpired() || passwordPolicyService.isPasswordExpired(user)) - userService.resetExpiredPassword(user, authenticatedUser.getCredentials()); - - // Link to user context - ModeledUserContext context = userContextProvider.get(); - context.init(user.getCurrentUser()); - return context; + // Otherwise, invalidate the authentication result, as database user + // accounts are absolutely required + throw new GuacamoleInvalidCredentialsException("Invalid login", + CredentialsInfo.USERNAME_PASSWORD); } From f4fce6a07a94a6a4f8919df5bffe171e82aa3081 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 4 Jun 2017 14:08:49 -0700 Subject: [PATCH 4/5] GUACAMOLE-284: Add convenience methods for determining whether a user account is disabled/expired. --- .../JDBCAuthenticationProviderService.java | 48 ++++++++----------- .../guacamole/auth/jdbc/user/ModeledUser.java | 24 ++++++++++ 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java index 37ff3bcae..b753ff865 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java @@ -28,7 +28,6 @@ import org.apache.guacamole.auth.jdbc.sharing.user.SharedAuthenticatedUser; import org.apache.guacamole.auth.jdbc.user.ModeledAuthenticatedUser; import org.apache.guacamole.auth.jdbc.user.ModeledUser; import org.apache.guacamole.auth.jdbc.user.ModeledUserContext; -import org.apache.guacamole.auth.jdbc.user.UserModel; import org.apache.guacamole.auth.jdbc.user.UserService; import org.apache.guacamole.net.auth.AuthenticatedUser; import org.apache.guacamole.net.auth.AuthenticationProvider; @@ -88,40 +87,33 @@ public class JDBCAuthenticationProviderService implements AuthenticationProvider // Retrieve user account for already-authenticated user ModeledUser user = userService.retrieveUser(authenticationProvider, authenticatedUser); - if (user != null) { + if (user != null && !user.isDisabled()) { - // User data only exists for purposes of retrieval if the account - // is not disabled - UserModel userModel = user.getModel(); - if (!userModel.isDisabled()) { + // Apply account restrictions if this extension authenticated + // the user OR if an account from this extension is explicitly + // required + if (authenticatedUser instanceof ModeledAuthenticatedUser + || environment.isUserRequired()) { - // Apply account restrictions if this extension authenticated - // the user OR if an account from this extension is explicitly - // required - if (authenticatedUser instanceof ModeledAuthenticatedUser - || environment.isUserRequired()) { + // Verify user account is still valid as of today + if (!user.isAccountValid()) + throw new GuacamoleClientException("LOGIN.ERROR_NOT_VALID"); - // Verify user account is still valid as of today - if (!user.isAccountValid()) - throw new GuacamoleClientException("LOGIN.ERROR_NOT_VALID"); + // Verify user account is allowed to be used at the current time + if (!user.isAccountAccessible()) + throw new GuacamoleClientException("LOGIN.ERROR_NOT_ACCESSIBLE"); - // Verify user account is allowed to be used at the current time - if (!user.isAccountAccessible()) - throw new GuacamoleClientException("LOGIN.ERROR_NOT_ACCESSIBLE"); - - // Update password if password is expired - if (userModel.isExpired() || passwordPolicyService.isPasswordExpired(user)) - userService.resetExpiredPassword(user, authenticatedUser.getCredentials()); - - } - - // Link to user context - ModeledUserContext context = userContextProvider.get(); - context.init(user.getCurrentUser()); - return context; + // Update password if password is expired + if (user.isExpired() || passwordPolicyService.isPasswordExpired(user)) + userService.resetExpiredPassword(user, authenticatedUser.getCredentials()); } + // Link to user context + ModeledUserContext context = userContextProvider.get(); + context.init(user.getCurrentUser()); + return context; + } // Do not invalidate the authentication result of users who were diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java index 418ffad81..745fe5f7f 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java @@ -766,4 +766,28 @@ public class ModeledUser extends ModeledDirectoryObject implements Us return isActive(getAccessWindowStart(), getAccessWindowEnd()); } + /** + * Returns whether the user has been disabled. Disabled users are not + * allowed to login. Although their account data exists, all login attempts + * will fail as if the account does not exist. + * + * @return + * true if the account is disabled, false otherwise. + */ + public boolean isDisabled() { + return getModel().isDisabled(); + } + + /** + * Returns whether the user's password has expired. If a user's password is + * expired, it must be immediately changed upon login. A user account with + * an expired password cannot be used until the password has been changed. + * + * @return + * true if the user's password has expired, false otherwise. + */ + public boolean isExpired() { + return getModel().isExpired(); + } + } From 862e2c398aaa346d59766f7cc3bec61c2e4a4639 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 4 Jun 2017 14:15:47 -0700 Subject: [PATCH 5/5] GUACAMOLE-284: Clarify semantics of disabled user accounts. --- .../JDBCAuthenticationProviderService.java | 6 ++-- .../guacamole/auth/jdbc/user/ModeledUser.java | 18 ++++++----- .../guacamole/auth/jdbc/user/UserModel.java | 31 ++++++++++--------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java index b753ff865..284a5aaae 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCAuthenticationProviderService.java @@ -89,9 +89,9 @@ public class JDBCAuthenticationProviderService implements AuthenticationProvider ModeledUser user = userService.retrieveUser(authenticationProvider, authenticatedUser); if (user != null && !user.isDisabled()) { - // Apply account restrictions if this extension authenticated - // the user OR if an account from this extension is explicitly - // required + // Account restrictions specific to this extension apply if this + // extension authenticated the user OR if an account from this + // extension is explicitly required if (authenticatedUser instanceof ModeledAuthenticatedUser || environment.isUserRequired()) { diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java index 745fe5f7f..0ed115ff4 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java @@ -767,24 +767,26 @@ public class ModeledUser extends ModeledDirectoryObject implements Us } /** - * Returns whether the user has been disabled. Disabled users are not - * allowed to login. Although their account data exists, all login attempts - * will fail as if the account does not exist. + * Returns whether this user account has been disabled. The credentials of + * disabled user accounts are treated as invalid, effectively disabling + * that user's access to data for which they would otherwise have + * permission. * * @return - * true if the account is disabled, false otherwise. + * true if this user account has been disabled, false otherwise. */ public boolean isDisabled() { return getModel().isDisabled(); } /** - * Returns whether the user's password has expired. If a user's password is - * expired, it must be immediately changed upon login. A user account with - * an expired password cannot be used until the password has been changed. + * Returns whether this user's password has expired. If a user's password + * is expired, it must be immediately changed upon login. A user account + * with an expired password cannot be used until the password has been + * changed. * * @return - * true if the user's password has expired, false otherwise. + * true if this user's password has expired, false otherwise. */ public boolean isExpired() { return getModel().isExpired(); diff --git a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserModel.java b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserModel.java index 2376caed9..afaeb5521 100644 --- a/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserModel.java +++ b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserModel.java @@ -194,48 +194,51 @@ public class UserModel extends ObjectModel { } /** - * Returns whether the user has been disabled. Disabled users are not - * allowed to login. Although their account data exists, all login attempts - * will fail as if the account does not exist. + * Returns whether this user account has been disabled. The credentials of + * disabled user accounts are treated as invalid, effectively disabling + * that user's access to data for which they would otherwise have + * permission. * * @return - * true if the account is disabled, false otherwise. + * true if this user account is disabled, false otherwise. */ public boolean isDisabled() { return disabled; } /** - * Sets whether the user is disabled. Disabled users are not allowed to - * login. Although their account data exists, all login attempts will fail - * as if the account does not exist. + * Sets whether this user account has been disabled. The credentials of + * disabled user accounts are treated as invalid, effectively disabling + * that user's access to data for which they would otherwise have + * permission. * * @param disabled - * true if the account should be disabled, false otherwise. + * true if this user account should be disabled, false otherwise. */ public void setDisabled(boolean disabled) { this.disabled = disabled; } /** - * Returns whether the user's password has expired. If a user's password is - * expired, it must be immediately changed upon login. A user account with - * an expired password cannot be used until the password has been changed. + * Returns whether this user's password has expired. If a user's password + * is expired, it must be immediately changed upon login. A user account + * with an expired password cannot be used until the password has been + * changed. * * @return - * true if the user's password has expired, false otherwise. + * true if this user's password has expired, false otherwise. */ public boolean isExpired() { return expired; } /** - * Sets whether the user's password is expired. If a user's password is + * Sets whether this user's password is expired. If a user's password is * expired, it must be immediately changed upon login. A user account with * an expired password cannot be used until the password has been changed. * * @param expired - * true to expire the user's password, false otherwise. + * true if this user's password has expired, false otherwise. */ public void setExpired(boolean expired) { this.expired = expired;