mike-jumper commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r942655521
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##########
@@ -179,6 +188,30 @@ public class KsmClient {
*/
private final Set<String> cachedAmbiguousUsernames = new HashSet<>();
+ /**
+ * All records retrieved from Keeper Secrets Manager, where each key is the
+ * domain of the corresponding record. The domain of a record is
+ * determined by {@link Login} fields, thus a record may be associated with
+ * multiple users. If a record is associated with multiple users, there
Review Comment:
domains* x2
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##########
@@ -179,6 +188,30 @@ public class KsmClient {
*/
private final Set<String> cachedAmbiguousUsernames = new HashSet<>();
+ /**
+ * All records retrieved from Keeper Secrets Manager, where each key is the
+ * domain of the corresponding record. The domain of a record is
+ * determined by {@link Login} fields, thus a record may be associated with
+ * multiple users. If a record is associated with multiple users, there
+ * will be multiple references to that record within this Map. The contents
+ * of this Map are automatically updated if {@link #validateCache()}
+ * refreshes the cache. This Map must not be accessed without
+ * {@link #cacheLock} acquired appropriately. Before using a value from
+ * this Map, {@link #cachedAmbiguousDomains} must first be checked to
+ * verify that there is indeed only one record associated with that user.
+ */
+ private final Map<String, KeeperRecord> cachedRecordsByDomain = new
HashMap<>();
+
+ /**
+ * The set of all domains that are associated with multiple records, and
+ * thus cannot uniquely identify a record. The contents of this Set are
+ * automatically updated if {@link #validateCache()} refreshes the cache.
+ * This Set must not be accessed without {@link #cacheLock} acquired
+ * appropriately.This Set must be checked before using a value retrieved
Review Comment:
Please add a space between the period and the beginning of the new sentence
that follows.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##########
@@ -250,13 +291,38 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
+ // ... and domain
+ String domain = recordService.getDomain(record);
+ addRecordForDomain(record, domain);
+
+ // Fetch the username
+ String username = recordService.getUsername(record);
+
+ // If domains should be split out from usernames
+ if (username != null &&
confService.getSplitWindowsUsernames()) {
+
+ // Attempt to split the domain of the username
+ WindowsUsername usernameAndDomain = (
+
WindowsUsername.splitWindowsUsernameFromDomain(username));
Review Comment:
A couple points:
* Should this be abstracted away such that the caller need not concern
themselves with whether they need to manually split domain from username?
* As currently written, `username` will be one of the following:
* The raw username containing the domain, which will not match the split
username and domain that would be part of the connection parameters.
* The split username (with the domain stored in a custom field of the
record), which will ambiguously match any user with that username regardless of
domain.
I think we need to somehow take this into account and ensure that indexed
usernames include the domain, and it would be good if the caller need not
manually know whether they need to handle the splitting.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##########
@@ -179,6 +188,30 @@ public class KsmClient {
*/
private final Set<String> cachedAmbiguousUsernames = new HashSet<>();
+ /**
+ * All records retrieved from Keeper Secrets Manager, where each key is the
+ * domain of the corresponding record. The domain of a record is
+ * determined by {@link Login} fields, thus a record may be associated with
+ * multiple users. If a record is associated with multiple users, there
+ * will be multiple references to that record within this Map. The contents
+ * of this Map are automatically updated if {@link #validateCache()}
+ * refreshes the cache. This Map must not be accessed without
+ * {@link #cacheLock} acquired appropriately. Before using a value from
+ * this Map, {@link #cachedAmbiguousDomains} must first be checked to
+ * verify that there is indeed only one record associated with that user.
Review Comment:
domain*
--
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]
