You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "necouchman (via GitHub)" <gi...@apache.org> on 2024/10/12 20:22:52 UTC
[PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
necouchman opened a new pull request, #1028:
URL: https://github.com/apache/guacamole-client/pull/1028
Next round of fixes for case-sensitivity stuff in the JDBC module.
Opening as a draft for the moment while I verify that the changes I've made will actually work consistently for username case-sensitivity.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2421872998
Hm ... maybe what I was thinking is actually backwards, and we should instead:
* Use the `*-case-sensitive-usernames` properties to dictate only whether users authenticating _via that extension_ have case-sensitive usernames.
* Extensions that consume usernames from other extensions _should_ honor the `isCaseSensitive()` value when comparing identities.
* In the equality comparison for `AbstractIdentifiable`, we need some sane way to resolve what happens if you compare a case-sensitive identifier with a case-insensitive identifier.
Otherwise things stop making sense. The JSON auth has a `json-case-sensitive-usernames` property but does not consume external identifiers, so the intent of that property _has_ to be declaring the intent of what other extensions should do with identifiers from JSON auth. For that to be useful, we would need to meaningfully consume `isCaseSensitive()`, but most of the logic around consuming external identifiers on the database side depends solely on the database's `*-case-sensitive-usernames` property.
I'm starting to thing these changes have part of the right idea, and we need to change the rest to match.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2424691511
Yes and no. That would be an alternative and consistent approach. Taking a mammoth step back to the beginning, I think we have two mutually-exclusive possible approaches:
1. Identifier case sensitivity is defined by the extension _consuming_ the identifier.
- This is what you're describing above and what I was describing initially.
- This is invitingly simple, particularly for cases where there's only one thing consuming identifiers.
- This feels a little odd, since it's the producer and not the consumer that determines the intent of the identifier, but it does mean there is no ambiguity in handling identifier comparisons.
- This might make sense only in light of a single, global property for case sensitivity ... which is admittedly also invitingly simple.
2. Identifier case sensitivity is defined by the extension _producing_ the identifier.
- This is what I was describing most recently.
- This is more complex, but allows the intent of an identifier to be communicated.
- This feels cleaner, but introduces ambiguity around how case-sensitive and case-insensitive identifiers could be compared. Then again, encountering that explicit ambiguity might be an opportunity to log a warning that makes identifier case sensitivity mismatches easier to notice and remedy.
Migrating to the former would mean getting rid of `isCaseSensitive()`, instead relying purely on configuration properties determining how usernames should be handled, and considering whether there should be any extension-specific case sensitivity properties (since having intentionally mixed case sensitivity would be bad practice).
Migrating to the latter would mean keeping `isCaseSensitive()`, clarifying the properties as controlling the case sensitivity of the usernames of users authenticated by and/or stored by that extension.
I find myself flipping back and forth between these without a clear answer as to which is truly best. I suspect that's probably also why the current approach is a sort of mix of these - they're both reasonable approaches.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "corentin-soriano (via GitHub)" <gi...@apache.org>.
corentin-soriano commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2410163377
@necouchman I found another issue related to the case sensitive with the quick connect extension.
It is still present on your branch:
```log
Oct 14 08:28:46 guacamole.host server[3886265]: 08:28:46.647 [http-nio-8080-exec-4] DEBUG o.a.g.event.EventLoggingListener - User "corentin" (authenticated by "openid") successfully accessed/retrieved connection "0" within "quickconnect" (currently named "ssh://127.0.0.1/")
Oct 14 08:28:46 guacamole.host server[3886265]: 14-Oct-2024 08:28:46.852 SEVERE [http-nio-8080-exec-14] org.apache.coyote.AbstractProtocol$ConnectionHandler.process Error reading request, ignored
Oct 14 08:28:46 guacamole.host server[3886265]: java.lang.NullPointerException
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.auth.jdbc.HistoryTrackingConnection.connect(HistoryTrackingConnection.java:109)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:160)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:171)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.net.auth.TokenInjectingConnection.connect(TokenInjectingConnection.java:115)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:160)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:171)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.tunnel.TunnelRequestService.createConnectedTunnel(TunnelRequestService.java:216)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.tunnel.TunnelRequestService.createTunnel(TunnelRequestService.java:352)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.tunnel.websocket.RestrictedGuacamoleWebSocketTunnelEndpoint.createTunnel(RestrictedGuacamoleWebSocketTunnelEndpoint.java:113)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.guacamole.websocket.GuacamoleWebSocketTunnelEndpoint.onOpen(GuacamoleWebSocketTunnelEndpoint.java:200)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.tomcat.websocket.server.WsHttpUpgradeHandler.init(WsHttpUpgradeHandler.java:131)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:970)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1786)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659)
Oct 14 08:28:46 guacamole.host server[3886265]: at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:63)
Oct 14 08:28:46 guacamole.host server[3886265]: at java.base/java.lang.Thread.run(Thread.java:829)
Oct 14 08:28:46 guacamole.host server[3886265]: 08:28:46.989 [http-nio-8080-exec-10] WARN o.a.g.a.mysql.conf.MySQLEnvironment - You have enabled case-sensitive usernames; however, MySQL's default collations do not support case-sensitive string comparisons. If you really want case-sensitive usernames you will need to configure your database appropriately.
```
HistoryTrackingConnection.java:109
```java
connectionRecordMapper.insert(connectionRecordModel,
environment.getCaseSensitiveUsernames());
```
The extension works on an instance without auth-jdbc.
Can you take a look please?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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<ModeledConnection> 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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2416625312
Thanks @jmuehlner and @mike-jumper - a question comes to mind before I pursue a particular solution: In these situations, which environment should the case-(in)sensitivity be pulled from? Since the `HistoryTrackingConnection` is decorating another Connection via the `HistoryTrackingUserContext` (in the case of Corentin's example, the QuickConnect user context and connections), should the case sensitivity be pulled from the 1) the JDBC extension, 2) the QuickConnect extension, or 3) the global setting? My inclination is to say 2 - it should come from the _decorated_ class(es)/extension, and not from the _decorating_ extension, but curious 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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2424692328
Then again ... it's hard to argue against the sheer simplicity of losing `isCaseSensitive()` and just having `case-sensitive-usernames` ...
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
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<String> getEffectiveUserGroups() {
return Collections.<String>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<CaseSensitivity> 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<String> retrieveEffectiveGroups(ModeledPermissions<? extends EntityModel> entity,
Collection<String> 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<ModeledConnection> 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:
The `e` parameter here will result in the stack trace being included in the logs. We should instead use a `{}` placeholder and `e.getMessage()` for messages that are not at the debug level.
##########
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:
I think this should be `group-names` (since it's a "group name" and not a "groupname").
##########
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<PermissionSetType extends PermissionSet<PermissionType>,
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:
Is this function used? I don't see any calls in the diff.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2415941122
I think it could potentially be problematic to leverage `isCaseSensitive()` in this way, since the meaning of `isCaseSensitive()` ("is the identifier of this object case sensitive?") is different from the meaning of the configuration property ("should identifiers be considered case sensitive?"), even if their values will usually be the same.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2411160085
@corentin-soriano It looks like maybe you're not using the latest git staging/1.6.0 version after the merge of the latest round of changes? Specifically, the warning about having enabled case-sensitive usernames was moved so that it only occurs during module loading, and not at each call, so you shouldn't have that warning right below the connection attempt, which indicates you may have an older version. I'll still test it out and see what happens with that extension in my environment.
Thank you for the continued testing and reporting these issues!!
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2418168816
> The current approach with the various `*-case-sensitive-usernames` properties is that the property dictates how the relevant extension handles username comparisons, regardless of how the authenticating extension may handle username comparisons. I think the only way to be consistent with that logic would be for `HistoryTrackingConnection` to honor only the value from the `*-case-sensitive-usernames` property of the extension that defines that instance of `HistoryTrackingConnection`.
>
So, if I'm understanding what you're saying correctly, given the current way these properties are interpreted, `HistoryTrackingConnection` should pull the value from the `JDBCEnvironment`?
> I also think it would make sense to allow the authenticating extension to dictate how the identifiers it presents should be compared (`isCaseSensitive()`), but that would be different semantics from what we currently have here.
>
> To switch over to that approach, the other cases where case sensitivity is handled would need to be updated to honor `isCaseSensitive()` instead of relying purely on the configuration property, and care would need to be taken to make sure a case-insensitive authentication provider can't be used to escalate privileges (for example: by creating an unprivileged `GuAcAdMiN` user in some case-insensitive auth system, logging in as that user, and inheriting the permissions of `guacadmin`).
>
> The current approach is less automatic, but I think that's a Good Thing, since any change from the default, strict behavior must be explicitly requested by the admin.
Yeah, seems like this might be a future enhancement with a lot more thought and effort putting into doing it safely and with as little surprise as possible.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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<PermissionSetType extends PermissionSet<PermissionType>,
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "corentin-soriano (via GitHub)" <gi...@apache.org>.
corentin-soriano commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2411652402
@necouchman I no longer encounter the issue, thank you!
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2415191432
> @mike-jumper You might take a look and make sure that what I did is sane - I couldn't pull the `JDBCEnvironment` injection into the `HistoryTrackingConnection` class, so I ended up using the `isCaseSensitive()` method from the `currentUser`. I _think_ this is sane, but let me know if anything seems off about that...
It looks like this should work, but it's a little goofy to be reaching through the user to get access to the environment. Consider just injecting the environment into the `UserContext`, which you can do by using a factory when it's constructed.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2422258034
> Hm ... maybe what I was thinking is actually backwards, and we should instead:
>
> * Use the `*-case-sensitive-usernames` properties to dictate only whether users authenticating _via that extension_ have case-sensitive usernames.
> * Extensions that consume usernames from other extensions _should_ honor the `isCaseSensitive()` value when comparing identities.
I'll go back through the previous changes and see what needs to be done to rework things using this logic.
> * In the equality comparison for `AbstractIdentifiable`, we need some sane way to resolve what happens if you compare a case-sensitive identifier with a case-insensitive identifier.
I've pushed a commit that I think tackles this particular issue without too much hassle - let me know if that looks okay to you.
>
> Otherwise things stop making sense. The JSON auth has a `json-case-sensitive-usernames` property but does not consume external identifiers, so the intent of that property _has_ to be declaring the intent of what other extensions should do with identifiers from JSON auth. For that to be useful, we would need to meaningfully consume `isCaseSensitive()`, but most of the logic around consuming external identifiers on the database side depends solely on the database's `*-case-sensitive-usernames` property.
>
> I'm starting to thing these changes have part of the right idea, and we need to change the rest to match.
Yeah, this makes sense to me. I'll review and see what changes need to be made to implement this logic.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2418178997
> > The current approach with the various `*-case-sensitive-usernames` properties is that the property dictates how the relevant extension handles username comparisons, regardless of how the authenticating extension may handle username comparisons. I think the only way to be consistent with that logic would be for `HistoryTrackingConnection` to honor only the value from the `*-case-sensitive-usernames` property of the extension that defines that instance of `HistoryTrackingConnection`.
>
> So, if I'm understanding what you're saying correctly, given the current way these properties are interpreted, `HistoryTrackingConnection` should pull the value from the `JDBCEnvironment`?
>
Actually, maybe I'm just confused, here. Given Corentin's example:
* `guacamole-auth-jdbc` is the _Authenticating_ extension.
* `guacamole-auth-quickconnect` is defining the `Connection` that is ultimately wrapped inside the `HistoryTrackingConnection`
So, you're saying that the extension that defines the `Connection` - `quickconnect` in this case - would be the one that defines the case-sensitivity for that particular wrapped object, regardless of the `jdbc` extension's setting?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2417700558
The current approach with the various `*-case-sensitive-usernames` properties is that the property dictates how the relevant extension handles username comparisons, regardless of how the authenticating extension may handle username comparisons. I think the only way to be consistent with that logic would be for `HistoryTrackingConnection` to honor only the value from the `*-case-sensitive-usernames` property of the extension that defines that instance of `HistoryTrackingConnection`.
I also think it would make sense to allow the authenticating extension to dictate how the identifiers it presents should be compared (`isCaseSensitive()`), but that would be different semantics from what we currently have here.
To switch over to that approach, the other cases where case sensitivity is handled would need to be updated to honor `isCaseSensitive()` instead of relying purely on the configuration property, and care would need to be taken to make sure a case-insensitive authentication provider can't be used to escalate privileges (for example: by creating an unprivileged `GuAcAdMiN` user in some case-insensitive auth system, logging in as that user, and inheriting the permissions of `guacadmin`).
The current approach is less automatic, but I think that's a Good Thing, since any change from the default, strict behavior must be explicitly requested by the admin.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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<CaseSensitivity> 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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2411576863
@corentin-soriano Okay, I've corrected the latest issue you identified with non-JDBC connections inserting history records into the database in this pull request.
@mike-jumper You might take a look and make sure that what I did is sane - I couldn't pull the `JDBCEnvironment` injection into the `HistoryTrackingConnection` class, so I ended up using the `isCaseSensitive()` method from the `currentUser`. I _think_ this is sane, but let me know if anything seems off about that...
I've also tested out things like creating users, creating connections, assigning permissions, etc., and I'm not seeing any issues, so I'm going to mark this as ready to review. I'll continue testing a few other scenarios - enabling/disabling case sensitivity, add some usernames with mixed case, etc., and see if I can break anything else.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Add case-sensitivity settings to permissions mappers and services [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1028:
URL: https://github.com/apache/guacamole-client/pull/1028#issuecomment-2422300196
> > Otherwise things stop making sense. The JSON auth has a `json-case-sensitive-usernames` property but does not consume external identifiers, so the intent of that property _has_ to be declaring the intent of what other extensions should do with identifiers from JSON auth. For that to be useful, we would need to meaningfully consume `isCaseSensitive()`, but most of the logic around consuming external identifiers on the database side depends solely on the database's `*-case-sensitive-usernames` property.
> > I'm starting to thing these changes have part of the right idea, and we need to change the rest to match.
>
> Yeah, this makes sense to me. I'll review and see what changes need to be made to implement this logic.
So, if I'm thinking about this correctly, what this looks like a little more concretely is:
* The only reference to the various `Environment` calls for things like `postgresql-case-sensitive-usernames` would be in the JDBC `User` classes, to determine if the `User` identifiers should be treated as case-sensitive.
* In all of the various `Service` instances in the JDBC module (e.g. `ModeledDirectoryObjectService`), the call should not be to the `Environment`, but to the `User` object and whether it is case-(in)sensitive.
Does that sound right?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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<String> getEffectiveUserGroups() {
return Collections.<String>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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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<String> retrieveEffectiveGroups(ModeledPermissions<? extends EntityModel> entity,
Collection<String> 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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings. [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org