From a608fa274dc09c805310b230d92290b950a3421e Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 11 Mar 2022 18:37:18 +0000 Subject: [PATCH] GUACAMOLE-1550: Allow TOTP key to be cleared by setting its generation status. The previous functionality provided two checkboxes: one for requesting that the TOTP key be cleared, and another for directly managing whether the TOTP key has been confirmed. This is confusing as checkboxes normally represent state, but the "reset" checkbox here is representing an action. Instead, both checkboxes should represent state: whether the key has been generated and whether the generated key has been confirmed. --- .../guacamole/auth/totp/user/TOTPUser.java | 48 ++++++++++--------- .../auth/totp/user/TOTPUserContext.java | 2 +- .../src/main/resources/translations/en.json | 6 +-- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUser.java b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUser.java index d50e3a68c..0c6f567b8 100644 --- a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUser.java +++ b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUser.java @@ -45,17 +45,26 @@ public class TOTPUser extends DelegatingUser { public static final String TOTP_KEY_CONFIRMED_ATTRIBUTE_NAME = "guac-totp-key-confirmed"; /** - * The name of the field used to trigger a reset of the TOTP data. + * The name of the user attribute defines whether the TOTP key has been + * generated for the user, regardless of whether that key has been + * confirmed. This attribute is not stored, but is instead exposed + * dynamically in lieu of exposing the actual TOTP key. */ - public static final String TOTP_KEY_SECRET_RESET_FIELD = "guac-totp-reset"; + public static final String TOTP_KEY_SECRET_GENERATED_ATTRIBUTE_NAME = "guac-totp-key-generated"; + + /** + * The string value used by TOTP user attributes to represent the boolean + * value "true". + */ + private static final String TRUTH_VALUE = "true"; /** * The form which contains all configurable properties for this user. */ - public static final Form TOTP_CONFIG_FORM = new Form("totp-config-form", + public static final Form TOTP_ENROLLMENT_STATUS = new Form("totp-enrollment-status", Arrays.asList( - new BooleanField(TOTP_KEY_SECRET_RESET_FIELD, "true"), - new BooleanField(TOTP_KEY_CONFIRMED_ATTRIBUTE_NAME, "true") + new BooleanField(TOTP_KEY_SECRET_GENERATED_ATTRIBUTE_NAME, TRUTH_VALUE), + new BooleanField(TOTP_KEY_CONFIRMED_ATTRIBUTE_NAME, TRUTH_VALUE) ) ); @@ -86,17 +95,12 @@ public class TOTPUser extends DelegatingUser { // Create independent, mutable copy of attributes Map attributes = new HashMap<>(super.getAttributes()); - - // Protect the secret value by removing it + + // Replace secret key with simple boolean attribute representing + // whether a key has been generated at all String secret = attributes.remove(TOTP_KEY_SECRET_ATTRIBUTE_NAME); - - // If secret is null or empty, mark the reset as true. - if (secret == null || secret.isEmpty()) - attributes.put(TOTP_KEY_SECRET_RESET_FIELD, "true"); - - // If secret has a value, mark the reset as false. - else - attributes.put(TOTP_KEY_SECRET_RESET_FIELD, "false"); + if (secret != null && !secret.isEmpty()) + attributes.put(TOTP_KEY_SECRET_GENERATED_ATTRIBUTE_NAME, TRUTH_VALUE); return attributes; @@ -107,15 +111,13 @@ public class TOTPUser extends DelegatingUser { // Create independent, mutable copy of attributes attributes = new HashMap<>(attributes); - - // Do not expose any TOTP secret attribute outside this extension + + // Do not allow TOTP secret to be directly manipulated attributes.remove(TOTP_KEY_SECRET_ATTRIBUTE_NAME); - - // Pull off the boolean reset field - String reset = attributes.remove(TOTP_KEY_SECRET_RESET_FIELD); - - // If reset has been set to true, clear the secret. - if (reset != null && reset.equals("true")) { + + // Reset TOTP status entirely if requested + String generated = attributes.remove(TOTP_KEY_SECRET_GENERATED_ATTRIBUTE_NAME); + if (generated != null && !generated.equals(TRUTH_VALUE)) { attributes.put(TOTP_KEY_SECRET_ATTRIBUTE_NAME, null); attributes.put(TOTP_KEY_CONFIRMED_ATTRIBUTE_NAME, null); } diff --git a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUserContext.java b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUserContext.java index f4785193c..a7a97b022 100644 --- a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUserContext.java +++ b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/TOTPUserContext.java @@ -68,7 +68,7 @@ public class TOTPUserContext extends DelegatingUserContext { @Override public Collection
getUserAttributes() { Collection userAttrs = new HashSet<>(super.getUserAttributes()); - userAttrs.add(TOTPUser.TOTP_CONFIG_FORM); + userAttrs.add(TOTPUser.TOTP_ENROLLMENT_STATUS); return Collections.unmodifiableCollection(userAttrs); } diff --git a/extensions/guacamole-auth-totp/src/main/resources/translations/en.json b/extensions/guacamole-auth-totp/src/main/resources/translations/en.json index 55bd69ac4..80bdaf06a 100644 --- a/extensions/guacamole-auth-totp/src/main/resources/translations/en.json +++ b/extensions/guacamole-auth-totp/src/main/resources/translations/en.json @@ -33,10 +33,10 @@ "USER_ATTRIBUTES" : { - "FIELD_HEADER_GUAC_TOTP_RESET" : "Clear TOTP secret:", - "FIELD_HEADER_GUAC_TOTP_KEY_CONFIRMED" : "TOTP key confirmed:", + "FIELD_HEADER_GUAC_TOTP_KEY_GENERATED" : "Secret key generated:", + "FIELD_HEADER_GUAC_TOTP_KEY_CONFIRMED" : "Authentication device confirmed:", - "SECTION_HEADER_TOTP_CONFIG_FORM" : "Configure TOTP" + "SECTION_HEADER_TOTP_ENROLLMENT_STATUS" : "TOTP Enrollment Status" }