Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow overriding of guacd-[hostname/port/ssl] within the connection's json properties #795

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marvin-enthus
Copy link

Hi,

this is just a simple extension to the guacamole-auth-json module allowing to specify guacd-hostname, guacd-port and guacd-ssl overrides within the connection properties config. Like it is also possible for connections configured via a database.

A config snipplet looks like be:

{
    "username" : "arbitraryUsername",
    "expires" : TIMESTAMP,
    "connections" : {

        "Connection Name" : {
            "protocol" : "PROTOCOL",
            "parameters" : {
                "guacd-hostname" : "my-other-guacd-host",
                "guacd-port" : "4822",
                ...
            }
        },
        ...
    }
}

I'm opening this PR without a JIRA ticket as I don't have an ASF account and it's not stright-forward to register. Maybe someone creates the Issue on behalf of me ;)

@mike-jumper
Copy link
Contributor

@marvin-enthus if you shoot an email to [email protected], I can create a JIRA account for you.

@@ -176,6 +177,22 @@ public GuacamoleTunnel connect(UserData.Connection connection,
String hostname = proxyConfig.getHostname();
int port = proxyConfig.getPort();

// handle guacd-[hostname/port/ssl] overrides
Map<String, String> params = connection.getParameters();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guacd hostname, port, etc. are not connection parameters, which are strictly the protocol-specific key/value pairs for configuring the connection as defined by the protocol plugin that guacd ultimately loads. Additional properties configuring the details for guacd will need to be at a different level of the JSON, perhaps directly within the connection, not within the connection params.

Copy link
Author

@marvin-enthus marvin-enthus Feb 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Mike. I Just pushed an other commit migrating this to connection level.

Currently i'm not sure about the best approach - I've allinged the java class with the current camelCase, so the settings are currently also in camelCase:

{
    "username" : "arbitraryUsername",
    "expires" : TIMESTAMP,
    "connections" : {

        "Connection Name" : {
            "protocol" : "PROTOCOL",
            "guacdHostname" : "my-other-guacd-host",
            "guacdPort" : 4822,
            "guacdSsl" : false,
            "parameters" : {
                "name1" : "value1",
                "name2" : "value2",
                ...
            }
        },
        ...
    }
}

But that's not matching the names in other places. I could set some annotations for the jackson data mapper, but haven't found any json annotation on the code - so i'm not sure about this...

if (overrideGuacdHostname != null && !overrideGuacdHostname.isEmpty()) {
hostname = overrideGuacdHostname;
}
Integer overrideGuacdPort = connection.getGuacdPort();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we prefer int over Integer unless you need the extra functionality of Integer.

port = overrideGuacdPort;
}
EncryptionMethod proxyMethod = proxyConfig.getEncryptionMethod();
Boolean overrideGuacdSSL = connection.getGuacdSsl();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I think we prefer boolean over Boolean, unless specifically requiring Boolean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants