mike-jumper commented on code in PR #1087:
URL: https://github.com/apache/guacamole-client/pull/1087#discussion_r2111094537
##########
guacamole-ext/src/main/java/org/apache/guacamole/properties/GuacamoleProperty.java:
##########
@@ -39,7 +39,7 @@ public interface GuacamoleProperty<Type> {
* input string which match this pattern will not be included in the parsed
* result.
*/
- static final Pattern DELIMITER_PATTERN = Pattern.compile(";\\s*");
+ static final Pattern DELIMITER_PATTERN = Pattern.compile("[;,]\\s*");
Review Comment:
I'm a little worried about this vs. the rationale cited in #490:
> Also, note that I switched over to semicolon delimiting for these - that's
worth some discussion, but my rationale for that is that some URIs - LDAP ones,
for instance - rely on the ability to use commas to separate LDAP DNs within
the URI, so, one way or the other we need to address that.
Looking at this again now with fresh eyes, it also seems a bit odd to have
this declared here on the base `GuacamoleProperty` interface - that should
probably either be part of an abstract base class or defined as needed within
implementations.
Perhaps `URIGuacamoleProperty` should use semicolons, while
`StringGuacamoleProperty` (and everything else) should use commas? Extensions
can provide their own more specific `GuacamoleProperty` implementations if the
default delimiters don't work.
Based on the changes from #490, I think the established properties that we
need to consider when deciding this are:
* `json-trusted-networks`
* `ldap-group-name-attribute`
* `ldap-user-attributes`
* `ldap-username-attribute`
* `openid-attributes-claim-type`
* `quickconnect-allowed-parameters`
* `quickconnect-denied-parameters`
* `ssl-subject-username-attribute`
--
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]
