Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
mike-jumper merged PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028 -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835410771
##
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticatedUser.java:
##
@@ -29,13 +34,39 @@
public abstract class AbstractAuthenticatedUser extends AbstractIdentifiable
implements AuthenticatedUser {
+/**
+ * The logger for this class.
+ */
+private static final Logger LOGGER =
LoggerFactory.getLogger(AbstractAuthenticatedUser.class);
+
+/**
+ * The server environment in which this Guacamole Client instance is
+ * running.
+ */
+private final Environment environment = LocalEnvironment.getInstance();
+
// Prior functionality now resides within AbstractIdentifiable
@Override
public Set getEffectiveUserGroups() {
return Collections.emptySet();
}
+@Override
+public boolean isCaseSensitive() {
+try {
+return environment.getCaseSensitivity().caseSensitiveUsernames();
+}
+catch (GuacamoleException e) {
+LOGGER.warn("Exception attempting to read the Guacamole
configuration, "
+ + "usernames will be treated as case-sensitive.",
e.getMessage());
Review Comment:
Added via rebase.
--
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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835409378
##
guacamole-ext/src/main/java/org/apache/guacamole/properties/CaseSensitivity.java:
##
@@ -0,0 +1,92 @@
+/*
+ * 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.properties;
+
+import org.apache.guacamole.properties.EnumGuacamoleProperty.PropertyValue;
+
+/**
+ * An enum that supports configuring various user and group case-sensitivity
+ * settings.
+ */
+public enum CaseSensitivity {
+
+/**
+ * Case-sensitivity enabled for both usernames and group names.
+ */
+@PropertyValue("enabled")
+ENABLED(true, true),
+
+/**
+ * Case-sensitivity enabled for usernames but disabled for group names.
+ */
+@PropertyValue("usernames")
+USERS(true, false),
+
+/**
+ * Case-sensitivity disabled for usernames but enabled for group names.
+ */
+@PropertyValue("groupnames")
Review Comment:
Works for me. Updated via rebase.
--
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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835409285
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/usergroup/ModeledUserGroup.java:
##
@@ -187,5 +202,20 @@ public RelatedObjectSet getMemberUserGroups() throws
GuacamoleException {
memberUserGroupSet.init(getCurrentUser(), this);
return memberUserGroupSet;
}
+
+@Override
+public boolean isCaseSensitive() {
+try {
+return environment.getCaseSensitivity().caseSensitiveGroupNames();
+}
+catch (GuacamoleException e) {
+LOGGER.warn("Unable to retrieve environment setting for "
+ + "case-sensitivity, group names will default to being "
+ + "case-sensitive.", e.getMessage());
Review Comment:
Added via rebase.
--
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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835409153
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AbstractGuacamoleTunnelService.java:
##
@@ -637,8 +638,20 @@ private List
getBalancedConnections(ModeledAuthenticatedUser
if (connectionGroup.isSessionAffinityEnabled())
identifiers = getPreferredConnections(user, identifiers);
+CaseSensitivity caseSensitivity = CaseSensitivity.ENABLED;
+try {
+caseSensitivity = environment.getCaseSensitivity();
+}
+catch (GuacamoleException e) {
+logger.warn("Error trying to retrieve case-sensitivity
configuration, "
+ + "usernames and group names will be treated as case-"
+ + "sensitive.", e);
Review Comment:
I've fixed this one - I'll go back through and try to make sure I caught it
everywhere...
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java:
##
@@ -792,11 +792,12 @@ public boolean isSkeleton() {
@Override
public boolean isCaseSensitive() {
try {
-return environment.getCaseSensitiveUsernames();
+return environment.getCaseSensitivity().caseSensitiveUsernames();
}
catch (GuacamoleException e) {
-logger.error("Failed to retrieve the configuration for
case-sensitive usernames: {}."
-+ " Usernames comparisons will be case-sensitive.",
e.getMessage());
+logger.error("Failed to retrieve the configuration for
case-sensitivity, "
+ + "username comparisons will be case-sensitive.",
+ e.getMessage());
Review Comment:
Added via rebase.
--
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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835408672
##
guacamole-ext/src/main/java/org/apache/guacamole/environment/Environment.java:
##
@@ -70,16 +72,12 @@ public interface Environment {
};
-/**
- * A property that configures whether or not Guacamole will take case
- * into account when comparing and processing usernames.
- */
-public static final BooleanGuacamoleProperty CASE_SENSITIVE_USERNAMES =
-new BooleanGuacamoleProperty() {
-
+public static final EnumGuacamoleProperty
CASE_SENSITIVITY =
Review Comment:
Oops. Comment added via rebase.
--
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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835408613
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/EntityService.java:
##
@@ -76,9 +85,22 @@ public class EntityService {
public Set retrieveEffectiveGroups(ModeledPermissions entity,
Collection effectiveGroups) {
+CaseSensitivity caseSensitivity = CaseSensitivity.ENABLED;
+try {
+caseSensitivity = environment.getCaseSensitivity();
+}
+catch (GuacamoleException e) {
+LOGGER.warn("Unable to retrieve configuration setting for group "
+ + "name case-sensitivity, group names will be treated "
+ + "as case-sensitive.", e.getMessage());
+LOGGER.debug("Got exception while trying to get group name case-"
+ + "sensitivity configuration.", e);
Review Comment:
I believe I've caught/fixed all of these inconsistencies via rebase.
--
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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835408758
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/permission/PermissionService.java:
##
@@ -43,6 +44,41 @@
public interface PermissionService,
PermissionType extends Permission> {
+/**
+ * Return the current case-sensitivity setting, allowing the system to
+ * determine if usernames and/or group names should be treated as case-
+ * sensitive.
+ *
+ * @return
+ * The current case-sensitivity configuration.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving configuration information related to
+ * case-sensitivity.
+ */
+default CaseSensitivity getCaseSensitivity() throws GuacamoleException {
+
+// By default identifiers are case-sensitive.
+return CaseSensitivity.ENABLED;
+}
+
+/**
+ * Return "true" if group names should be treated as case-sensitive,
+ * otherwise "false".
+ *
+ * @return
+ * "true" if group names should be treated as case-sensitive, otherwise
+ * "false".
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving configuration information related to
+ * case-sensitivity.
+ */
+default boolean getCaseSensitiveGroupNames() throws GuacamoleException {
+// By default group names are case-sensitive.
+return true;
+}
Review Comment:
Must have been a left-over from the previous implementation iterations.
Removed via rebase.
--
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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
mike-jumper commented on code in PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1835316554
##
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticatedUser.java:
##
@@ -29,13 +34,39 @@
public abstract class AbstractAuthenticatedUser extends AbstractIdentifiable
implements AuthenticatedUser {
+/**
+ * The logger for this class.
+ */
+private static final Logger LOGGER =
LoggerFactory.getLogger(AbstractAuthenticatedUser.class);
+
+/**
+ * The server environment in which this Guacamole Client instance is
+ * running.
+ */
+private final Environment environment = LocalEnvironment.getInstance();
+
// Prior functionality now resides within AbstractIdentifiable
@Override
public Set getEffectiveUserGroups() {
return Collections.emptySet();
}
+@Override
+public boolean isCaseSensitive() {
+try {
+return environment.getCaseSensitivity().caseSensitiveUsernames();
+}
+catch (GuacamoleException e) {
+LOGGER.warn("Exception attempting to read the Guacamole
configuration, "
+ + "usernames will be treated as case-sensitive.",
e.getMessage());
Review Comment:
`{}` placeholder for `e.getMessage()` missing here.
##
guacamole-ext/src/main/java/org/apache/guacamole/environment/Environment.java:
##
@@ -70,16 +72,12 @@ public interface Environment {
};
-/**
- * A property that configures whether or not Guacamole will take case
- * into account when comparing and processing usernames.
- */
-public static final BooleanGuacamoleProperty CASE_SENSITIVE_USERNAMES =
-new BooleanGuacamoleProperty() {
-
+public static final EnumGuacamoleProperty
CASE_SENSITIVITY =
Review Comment:
Please document.
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/EntityService.java:
##
@@ -76,9 +85,22 @@ public class EntityService {
public Set retrieveEffectiveGroups(ModeledPermissions entity,
Collection effectiveGroups) {
+CaseSensitivity caseSensitivity = CaseSensitivity.ENABLED;
+try {
+caseSensitivity = environment.getCaseSensitivity();
+}
+catch (GuacamoleException e) {
+LOGGER.warn("Unable to retrieve configuration setting for group "
+ + "name case-sensitivity, group names will be treated "
+ + "as case-sensitive.", e.getMessage());
+LOGGER.debug("Got exception while trying to get group name case-"
+ + "sensitivity configuration.", e);
Review Comment:
Unlike "case-sensitive" and "case-insensitive", "case sensitivity" shouldn't
have a hyphen.
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/usergroup/ModeledUserGroup.java:
##
@@ -187,5 +202,20 @@ public RelatedObjectSet getMemberUserGroups() throws
GuacamoleException {
memberUserGroupSet.init(getCurrentUser(), this);
return memberUserGroupSet;
}
+
+@Override
+public boolean isCaseSensitive() {
+try {
+return environment.getCaseSensitivity().caseSensitiveGroupNames();
+}
+catch (GuacamoleException e) {
+LOGGER.warn("Unable to retrieve environment setting for "
+ + "case-sensitivity, group names will default to being "
+ + "case-sensitive.", e.getMessage());
Review Comment:
`{}` placeholder for `e.getMessage()` missing here.
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUser.java:
##
@@ -792,11 +792,12 @@ public boolean isSkeleton() {
@Override
public boolean isCaseSensitive() {
try {
-return environment.getCaseSensitiveUsernames();
+return environment.getCaseSensitivity().caseSensitiveUsernames();
}
catch (GuacamoleException e) {
-logger.error("Failed to retrieve the configuration for
case-sensitive usernames: {}."
-+ " Usernames comparisons will be case-sensitive.",
e.getMessage());
+logger.error("Failed to retrieve the configuration for
case-sensitivity, "
+ + "username comparisons will be case-sensitive.",
+ e.getMessage());
Review Comment:
The `{}` placeholder that will receive `e.getMessage()` is missing here.
##
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/AbstractGuacamoleTunnelService.java:
##
@@ -637,8 +638,20 @@ private List
getBalancedConnections(ModeledAuthenticatedUser
if
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2457902830 I've removed this from draft state, as I've done several rounds of testing, fixed several items, and am cautiously optimistic that it is working as intended and that I've corrected the typos. If others are inclined to test, that would be welcome - so far I've only tested with PostgreSQL, as that is my DB of choice. I'll try to spin up MySQL and SQL Server at some point, here, soon, and test with those. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2457773071 > > my eyes are crossing from SO. MUCH. XML > > Oof, I don't blame them, but thanks for doing this. I think we're on the right path. Sounds good. I'm working through testing, now, and have already corrected a couple of issues, which I'll push, here, shortly. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
mike-jumper commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2457755836 > my eyes are crossing from SO. MUCH. XML Oof, I don't blame them, but thanks for doing this. I think we're on the right path. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2456039156 > > > I'll see if I can knock the `UserGroup` portions out real quick - shouldn't be too terribly difficult. > > > And then...testing... > > > > > > Working through the `UserGroup` stuff this weekend. It's a little more involved that I anticipated... > > Almost there. Wrapping things up today. 🤞 Okay...here are the changes. I am :100: certain that I have made multiple mistakes. I have done :zero: testing after finishing these changes - my eyes are crossing from SO. MUCH. XML - while it compiles right now, I very much doubt that it will actually work out of the gate. There are also several style issues that need fixing. But, any overall concerns or comments about the approach, let me know. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
mike-jumper commented on code in PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1828584626 ## guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Identifiable.java: ## Review Comment: Awesome - sounds reasonable to me. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on code in PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1828583459 ## guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Identifiable.java: ## Review Comment: Okay - I'm almost done with the changes - hoping to at least get the draft out tonight ahead of doing some testing, and you can see what I've done, there. I left `isCaseSensitive()` in, for now, because it helps with the comparisons between identifiers. But you can see what you think after I've pushed the remaining changes... -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
mike-jumper commented on code in PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#discussion_r1828581893 ## guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Identifiable.java: ## Review Comment: I think we should probably remove the `isCaseSensitive()` function, at least from guacamole-ext, so that it does not potentially result in conflicting information between an implementation's `isCaseSensitive()` vs. the global configuration option. If it makes things easier to plumb within the JDBC auth, I don't see any issue with having a `isCaseSensitive()` function that's strictly internal to JDBC. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2454921154 > > I'll see if I can knock the `UserGroup` portions out real quick - shouldn't be too terribly difficult. > > And then...testing... > > Working through the `UserGroup` stuff this weekend. It's a little more involved that I anticipated... Almost there. Wrapping things up today. :crossed_fingers: -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2453247739 > > I'll see if I can knock the `UserGroup` portions out real quick - shouldn't be too terribly difficult. > > And then...testing... Working through the `UserGroup` stuff this weekend. It's a little more involved that I anticipated... -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2450468883 > > ... Thoughts on expanding the scope from just `User` objects to include `UserGroup` objects in this? > > I agree. I'm sure this will be necessary and it makes sense to include this as part of the general configurable case sensitivity effort. Okay - I _think_ I've completed the re-work of the user code to only pull from the global configuration value, and removed all of the extension-based options. I have left the bits in place, mainly in the JDBC module, that pull from the `user.isCaseSensitive()` method, rather than pulling the environment directly - this tends to be a bit easier than injecting the environment everywhere it might be required, and also keeps the concern of whether or not a `User` identifier is case sensitive as part of the `User` object. I'll see if I can knock the `UserGroup` portions out real quick - shouldn't be too terribly difficult. And then...testing... -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
mike-jumper commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2450412512 > ... Thoughts on expanding the scope from just `User` objects to include `UserGroup` objects in this? I agree. I'm sure this will be necessary and it makes sense to include this as part of the general configurable case sensitivity effort. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2449699263 > I think we do need to switch over to the monolithic enable/disable option (and +1 on the 😩). > > As I review, I'm running into things like handling of usernames within history records, and there doesn't seem to be any way we can handle differences in case sensitivity sanely unless there is only the platform-wide option that states how all usernames are to be interpreted. > > For example, let's say there are two authentication systems installed: > > 1. LDAP (configured to be case-insensitive) > 2. PostgreSQL (configured to be case-sensitive) > > If a user `SOMEONE` logs in with LDAP and accesses a connection, and then a user `someone` logs into PostgreSQL and accesses another connection, are they the same user? If a group in PostgreSQL includes the PostgreSQL user `someone`, does that include the LDAP user `SOMEONE`? > > With a platform-wide option, things are completely unambiguous and everything is in agreement. With extension-specific options, things make sense as long as everything is manually configured to be in agreement, but stop making sense once there is any disagreement between extensions. Okay, I'll switch it up and go that route. It pains me a little bit to take away the flexibility of being able to choose on a per-extension basis, but the example you give above highlights the challenges with even making that available and the confusion that might arise were someone to try to debug that sort of situation. The other thing that has continued to nag at the back of my mind is that I think we (ultimately) need to make the same option available for group names. This effort has focused almost exclusively on usernames, but I cannot help but think that the expectation of making _usernames_ case-insensitive is going to easily make someone think that _group names_ also ought to be case-insensitive. Thoughts on expanding the scope from just `User` objects to include `UserGroup` objects in this? -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
mike-jumper commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2448651425 I think we do need to switch over to the monolithic enable/disable option. As I review, I'm running into things like handling of usernames within history records, and there doesn't seem to be any way we can handle differences in case sensitivity sanely unless there is a platform-wide option that states how all installed extensions are expected to behave. For example, let's say there are two authentication systems installed: 1. LDAP (configured to be case-insensitive) 2. PostgreSQL (configured to be case-sensitive) If a user `SOMEONE` logs in with LDAP and accesses a connection, and then a user `someone` logs into PostgreSQL and accesses another connection, are they the same user? If a group in PostgreSQL includes the PostgreSQL user `someone`, does that include the LDAP user `SOMEONE`? With a platform-wide option, things are completely unambiguous and everything is in agreement. With extension-specific options, things make sense as long as everything is manually configured to be in agreement, but stop making sense once there is any disagreement between extensions. -- 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]
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
necouchman commented on PR #1028: URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2440415730 @mike-jumper @jmuehlner : I've taken a stab at an approach that should make the source for case-sensitivity more consistent across different login modules - essentially, what I've tried to do, is make sure that, whenever and wherever possible, particularly within the JDBC module, the source for the case sensitivity setting comes from the logged in user. The logged in user, in turn will pull from the environment for the current authentication module, which will either be that particular extension's configuration property or the global configuration property. I just finished this up and have verified that it compiles - tomorrow I'll run it through some testing to make sure that the various use-cases that others identified that were broken are still functional. If you spot any flaws in my logic, please give a shout. And, if we need to close it out and just go to a monolithic enable/disable option and scrap the whole per-extension setting, I can give that a run, too. :weary: -- 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]
