[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954390299
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -250,13 +290,26 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
-// Store based on username ONLY if no hostname (will otherwise
+// ... and domain
+String domain = recordService.getDomain(record);
+addRecordForDomain(record, domain);
+
+// Get the username off of the record
+String username = recordService.getUsername(record);
+
+// If domain matching is not enabled for user records,
+// explicitly set all domains to null to allow matching
+// on username only
+if (!confService.getMatchUserRecordsByDomain())
+domain = null;
Review Comment:
Sorry, I'm a little unclear exactly what you mean by "It sounds like the
minor performance hit and loss of nifty lambda isn't worth the added
complexity.".
Are you suggesting I get rid of the refactor that moved the conf parsing
into the record service?
I think that would be my preference - to just read all the configs once
before this here `forEach` and get rid of all the exception-throwing stuff in
the domain service.
We could go back to using a lambda that way too.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954372186
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -250,13 +290,26 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
-// Store based on username ONLY if no hostname (will otherwise
+// ... and domain
+String domain = recordService.getDomain(record);
+addRecordForDomain(record, domain);
+
+// Get the username off of the record
+String username = recordService.getUsername(record);
+
+// If domain matching is not enabled for user records,
+// explicitly set all domains to null to allow matching
+// on username only
+if (!confService.getMatchUserRecordsByDomain())
+domain = null;
Review Comment:
I know that we read from a config file to parse the config so it's not going
to change but it still feels weird to pull the config once at service creation
time when there's no actual guarantee in the API that the config will never
change.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954367425
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -250,13 +290,26 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
-// Store based on username ONLY if no hostname (will otherwise
+// ... and domain
+String domain = recordService.getDomain(record);
+addRecordForDomain(record, domain);
+
+// Get the username off of the record
+String username = recordService.getUsername(record);
+
+// If domain matching is not enabled for user records,
+// explicitly set all domains to null to allow matching
+// on username only
+if (!confService.getMatchUserRecordsByDomain())
+domain = null;
Review Comment:
Well, that's not the only config parsing that's going on now - `getUsername`
and `getDomain` now also parse the config every time they're called - a
consequence of the refactor where that logic got shoved into the record
service. I guess I could read the config once on service startup or something,
or we could just undo that refactor and read all the configs once at the start
of the loop here.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954215185
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -250,13 +292,38 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
-// Store based on username ONLY if no hostname (will otherwise
-// result in ambiguous entries for servers tied to identical
-// accounts)
-if (hostname == null)
-addRecordForLogin(record,
recordService.getUsername(record));
+// ... and domain
+String domain = recordService.getDomain(record);
+addRecordForDomain(record, domain);
+
+// Get the username off of the record
+String username = recordService.getUsername(record);
+
+// If we have a username, and there isn't already a domain
explicitly defined
+if (username != null && domain == null
+&& confService.getSplitWindowsUsernames()) {
-});
+// Attempt to split out the domain of the username
+WindowsUsername usernameAndDomain = (
+
WindowsUsername.splitWindowsUsernameFromDomain(username));
+
+// Use the username-split domain if not already set
explicitly
+if (usernameAndDomain.hasDomain())
+domain = usernameAndDomain.getDomain();
+addRecordForDomain(record, domain);
+
+}
+
+// If domain matching is not enabled for user records,
+// explicitly set all domains to null to allow matching
+// on username only
+if (!confService.getMatchUserRecordsByDomain())
+domain = null;
+
+// Store the login by username and domain
+addRecordForLogin(record, username, domain);
Review Comment:
Sure, how does this look?
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954188756
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -250,13 +292,40 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
-// Store based on username ONLY if no hostname (will otherwise
-// result in ambiguous entries for servers tied to identical
-// accounts)
-if (hostname == null)
-addRecordForLogin(record,
recordService.getUsername(record));
Review Comment:
Oops, I realize that I completely removed this logic for no good reason.
I'll restore it.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954183050
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -250,13 +292,38 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
-// Store based on username ONLY if no hostname (will otherwise
-// result in ambiguous entries for servers tied to identical
-// accounts)
-if (hostname == null)
-addRecordForLogin(record,
recordService.getUsername(record));
+// ... and domain
+String domain = recordService.getDomain(record);
+addRecordForDomain(record, domain);
+
+// Get the username off of the record
+String username = recordService.getUsername(record);
+
+// If we have a username, and there isn't already a domain
explicitly defined
+if (username != null && domain == null
+&& confService.getSplitWindowsUsernames()) {
-});
+// Attempt to split out the domain of the username
+WindowsUsername usernameAndDomain = (
+
WindowsUsername.splitWindowsUsernameFromDomain(username));
+
+// Use the username-split domain if not already set
explicitly
+if (usernameAndDomain.hasDomain())
+domain = usernameAndDomain.getDomain();
+addRecordForDomain(record, domain);
+
+}
+
+// If domain matching is not enabled for user records,
+// explicitly set all domains to null to allow matching
+// on username only
+if (!confService.getMatchUserRecordsByDomain())
+domain = null;
+
+// Store the login by username and domain
+addRecordForLogin(record, username, domain);
Review Comment:
Where did you see it indexing by both the raw username, and domain+username?
I only see a single call to `addRecordForLogin`, which should do either one of
those depending on `getSplitWindowsUsernames()`.
There was a bug here where I didn't correctly remove the username from the
domain if splitting is enabled, but that should be fixed now.
Everything should be working the way you suggest above.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954172328
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -250,13 +292,38 @@ private void validateCache() throws GuacamoleException {
String hostname = recordService.getHostname(record);
addRecordForHost(record, hostname);
-// Store based on username ONLY if no hostname (will otherwise
-// result in ambiguous entries for servers tied to identical
-// accounts)
-if (hostname == null)
-addRecordForLogin(record,
recordService.getUsername(record));
+// ... and domain
+String domain = recordService.getDomain(record);
+addRecordForDomain(record, domain);
+
+// Get the username off of the record
+String username = recordService.getUsername(record);
+
+// If we have a username, and there isn't already a domain
explicitly defined
+if (username != null && domain == null
+&& confService.getSplitWindowsUsernames()) {
-});
+// Attempt to split out the domain of the username
+WindowsUsername usernameAndDomain = (
+
WindowsUsername.splitWindowsUsernameFromDomain(username));
+
+// Use the username-split domain if not already set
explicitly
+if (usernameAndDomain.hasDomain())
+domain = usernameAndDomain.getDomain();
+addRecordForDomain(record, domain);
+
+}
+
+// If domain matching is not enabled for user records,
+// explicitly set all domains to null to allow matching
+// on username only
+if (!confService.getMatchUserRecordsByDomain())
+domain = null;
+
+// Store the login by username and domain
+addRecordForLogin(record, username, domain);
Review Comment:
Where do you see it indexing the record twice, with `DOMAIN\Username` and
domain+username? As far as I can tell, the code is already doing what you're
suggesting above.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954164720
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##
@@ -338,12 +332,59 @@ public Map> getTokens(UserContext
userContext, Connectabl
addRecordTokens(tokens, "KEEPER_GATEWAY_",
ksm.getRecordByHost(filter.filter(gatewayHostname)));
+// Retrieve and define domain tokens, if any
+String domain = parameters.get("domain");
+String filteredDomain = null;
+if (domain != null && !domain.isEmpty()) {
+filteredDomain = filter.filter(domain);
+addRecordTokens(tokens, "{KEEPER_DOMAIN_",
Review Comment:
Oops!
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954162903
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserLogin.java:
##
@@ -0,0 +1,121 @@
+/*
+ * 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.vault.ksm.secret;
+
+import java.util.Objects;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+/**
+ * A class intended for use as a key in KSM user record client cache. This
+ * class contains both a username and a password. When identifying a KSM
+ * record using token syntax like "KEEPER_USER_*", the user record will
+ * actually be identified by both the user and domain, if the appropriate
+ * settings are enabled.
+ */
+class UserLogin {
+
+/**
+ * The username associated with the user record.
+ * This field should never be null.
+ */
+private final String username;
+
+/**
+ * The domain associated with the user record.
+ * This field can be null.
+ */
+private final String domain;
+
+/**
+ * Create a new UserLogin instance with the provided username and
+ * domain. The domain may be null, but the username should never be.
+ *
+ * @param username
+ *The username to create the UserLogin instance with. This should
+ *never be null.
+ *
+ * @param domain
+ *The domain to create the UserLogin instance with. This can be null.
+ */
+UserLogin(@Nonnull String username, @Nullable String domain) {
+this.username = username;
+this.domain = domain;
+}
+
+@Override
+public int hashCode() {
+
+final int prime = 31;
+
+int result = 1;
+result = prime * result + Objects.hashCode(domain);
+result = prime * result + Objects.hashCode(username);
+
+return result;
+
+}
Review Comment:
Sure
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r954122423
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -399,32 +497,75 @@ public KeeperRecord getRecordByHost(String hostname)
throws GuacamoleException {
}
/**
- * Returns the record associated with the given username. If no such record
- * exists, or there are multiple such records, null is returned.
+ * Returns the record associated with the given username and domain. If no
+ * such record exists, or there are multiple such records, null is
returned.
*
* @param username
* The username of the record to return.
*
+ * @param domain
+ * The domain of the record to return.
Review Comment:
Fair - added.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r948268136
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java:
##
@@ -0,0 +1,129 @@
+/*
+ * 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.vault.ksm.secret;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+/**
+ * A class intended for use as a key in KSM user record client cache. This
+ * class contains both a username and a password. When identifying a KSM
+ * record using token syntax like "KEEPER_USER_*", the user record will
+ * actually be identified by both the user and domain, if the appropriate
+ * settings are enabled.
+ */
+class UserDomain {
+
+/**
+ * The username associated with the user record.
+ * This field should never be null.
+ */
+private final String username;
+
+/**
+ * The domain associated with the user record.
+ * This field can be null.
+ */
+private final String domain;
+
+/**
+ * Create a new UserDomain instance with the provided username and
+ * domain. The domain may be null, but the username should never be.
+ *
+ * @param username
+ *The username to create the UserDomain instance with. This should
+ *never be null.
+ *
+ * @param domain
+ *The domain to create the UserDomain instance with. This can be null.
+ */
+UserDomain(@Nonnull String username, @Nullable String domain) {
+this.username = username;
+this.domain = domain;
+}
+
+@Override
+public int hashCode() {
+
+final int prime = 31;
+
+int result = 1;
+result = prime * result + ((domain == null) ? 0 : domain.hashCode());
+result = prime * result + ((username == null) ? 0 :
username.hashCode());
+
+return result;
+
+}
+
+@Override
+public boolean equals(Object obj) {
+
+// Check if the other object is this exact object
+if (this == obj)
+return true;
+
+// Check if the other object is null
+if (obj == null)
+return false;
+
+// Check if the other object is also a UserDomain
+if (getClass() != obj.getClass())
+return false;
+
+// If it is a UserDomain, it must have the same username...
+UserDomain other = (UserDomain) obj;
+if (username == null) {
+if (other.username != null)
+return false;
+} else if (!username.equals(other.username))
+return false;
+
+// .. and the same domain
+if (domain == null) {
+if (other.domain != null)
+return false;
+} else if (!domain.equals(other.domain))
+return false;
+
+return true;
Review Comment:
While I'm at it, I also switched to the simplified `Objects.hashCode` method.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r948251965
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -399,32 +497,75 @@ public KeeperRecord getRecordByHost(String hostname)
throws GuacamoleException {
}
/**
- * Returns the record associated with the given username. If no such record
- * exists, or there are multiple such records, null is returned.
+ * Returns the record associated with the given username and domain. If no
+ * such record exists, or there are multiple such records, null is
returned.
*
* @param username
* The username of the record to return.
*
+ * @param domain
+ * The domain of the record to return.
Review Comment:
Hmm, what specifically would you request that I add? All it means if this
parameter is null is that it will fetch records for which the domain is null.
It's just regular equality, nothing special.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r948245722
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##
@@ -236,12 +270,20 @@ private void validateCache() throws GuacamoleException {
cachedRecordsByHost.clear();
// Clear cache of login-based records
-cachedAmbiguousUsernames.clear();
-cachedRecordsByUsername.clear();
+cachedAmbiguousUsers.clear();
+cachedRecordsByUser.clear();
-// Store all records, sorting each into host-based and login-based
-// buckets
-records.forEach(record -> {
+// Clear cache of domain-based records
+cachedAmbiguousDomains.clear();
+cachedRecordsByDomain.clear();
+
+// Store all records, sorting each into host-based, login-based,
+// and domain-based buckets
+Iterator recordIterator = records.iterator();
+while(recordIterator.hasNext()) {
+
+// Go through records one at a time
+KeeperRecord record = recordIterator.next();
Review Comment:
Because it can now throw a `GuacamoleException`, and you can't have a java
lambda that throws a checked exception. My level of frustration with the whole
java lambda ecosystem is quite high right now.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r948244377
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java:
##
@@ -0,0 +1,129 @@
+/*
+ * 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.vault.ksm.secret;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+/**
+ * A class intended for use as a key in KSM user record client cache. This
+ * class contains both a username and a password. When identifying a KSM
+ * record using token syntax like "KEEPER_USER_*", the user record will
+ * actually be identified by both the user and domain, if the appropriate
+ * settings are enabled.
+ */
+class UserDomain {
Review Comment:
Yeah, that would probably make more sense.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r948244194
##
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java:
##
@@ -0,0 +1,129 @@
+/*
+ * 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.vault.ksm.secret;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+/**
+ * A class intended for use as a key in KSM user record client cache. This
+ * class contains both a username and a password. When identifying a KSM
+ * record using token syntax like "KEEPER_USER_*", the user record will
+ * actually be identified by both the user and domain, if the appropriate
+ * settings are enabled.
+ */
+class UserDomain {
+
+/**
+ * The username associated with the user record.
+ * This field should never be null.
+ */
+private final String username;
+
+/**
+ * The domain associated with the user record.
+ * This field can be null.
+ */
+private final String domain;
+
+/**
+ * Create a new UserDomain instance with the provided username and
+ * domain. The domain may be null, but the username should never be.
+ *
+ * @param username
+ *The username to create the UserDomain instance with. This should
+ *never be null.
+ *
+ * @param domain
+ *The domain to create the UserDomain instance with. This can be null.
+ */
+UserDomain(@Nonnull String username, @Nullable String domain) {
+this.username = username;
+this.domain = domain;
+}
+
+@Override
+public int hashCode() {
+
+final int prime = 31;
+
+int result = 1;
+result = prime * result + ((domain == null) ? 0 : domain.hashCode());
+result = prime * result + ((username == null) ? 0 :
username.hashCode());
+
+return result;
+
+}
+
+@Override
+public boolean equals(Object obj) {
+
+// Check if the other object is this exact object
+if (this == obj)
+return true;
+
+// Check if the other object is null
+if (obj == null)
+return false;
+
+// Check if the other object is also a UserDomain
+if (getClass() != obj.getClass())
+return false;
+
+// If it is a UserDomain, it must have the same username...
+UserDomain other = (UserDomain) obj;
+if (username == null) {
+if (other.username != null)
+return false;
+} else if (!username.equals(other.username))
+return false;
+
+// .. and the same domain
+if (domain == null) {
+if (other.domain != null)
+return false;
+} else if (!domain.equals(other.domain))
+return false;
+
+return true;
Review Comment:
Hallelujah! I shall switch it.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r943010166
##
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:
Ah, yes good call. The functionality was a bit goofed, but it should be
better now. The `KEEPER_USER_` tokens will now ALSO match on domain (if
configured to do so. false by default).
The behavior seems a lot more sane now - let me know what you think.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r942739520
##
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:
What caller are you referring to, specifically?
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r942724668
##
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 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:
gah the copypasta
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #753: GUACAMOLE-1661: Add domain search support for KSM vault extension.
jmuehlner commented on code in PR #753:
URL: https://github.com/apache/guacamole-client/pull/753#discussion_r940747804
##
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));
+
+if (usernameAndDomain.hasDomain()) {
+
+// Update the username if a domain has been stripped
off
+username = usernameAndDomain.getUsername();
Review Comment:
This probably should have been done before, even if we weren't using the
domain here.
--
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]
