mike-jumper commented on a change in pull request #615:
URL: https://github.com/apache/guacamole-client/pull/615#discussion_r682347276
##########
File path: guacamole-docker/bin/start.sh
##########
@@ -594,6 +594,46 @@ END
}
+## Adds properties to guacamole.properties which select the SAML
+## authentication provider, and configure it to connect to the specified SAML
+## provider.
+##
+associate_saml() {
+
+ # Verify required parameters are present
+ if [ -z "$SAML_IDP_METADATA_URL" ]
+ then
+ cat <<END
+FATAL: Missing required environment variables
+-------------------------------------------------------------------------------
+If using an openid authentication, you must provide each of the following
Review comment:
Oops: "SAML", not "OpenID".
##########
File path: guacamole-docker/bin/start.sh
##########
@@ -830,6 +870,12 @@ if [ -n "$OPENID_AUTHORIZATION_ENDPOINT" ]; then
INSTALLED_AUTH="$INSTALLED_AUTH openid"
fi
+# Use SAML if specified
+if [ -n "$SAML_IDP_METADATA_URL" ]; then
Review comment:
This check will need to be more complicated, since the
`saml-idp-metadata-url` property can be omitted entirely so long as the
required information is given with other properties.
Overall, the user needs either `saml-idp-metadata-url`, or `saml-entity-id`
and `saml-callback-url`, or a combination of these if the metadata XML is
incomplete.
##########
File path: guacamole-docker/bin/start.sh
##########
@@ -594,6 +594,46 @@ END
}
+## Adds properties to guacamole.properties which select the SAML
+## authentication provider, and configure it to connect to the specified SAML
+## provider.
+##
Review comment:
Please correct the style of this comment to match:
```
##
## This somewhat JavaDoc-like format that we've adopted for documenting shell
## script functions.
##
```
rather than:
```
## This, which is missing the leading JavaDoc-like "opening" blank line
##
```
You're just missing that initial empty `##` line.
##########
File path: guacamole-docker/bin/start.sh
##########
@@ -594,6 +594,46 @@ END
}
+## Adds properties to guacamole.properties which select the SAML
+## authentication provider, and configure it to connect to the specified SAML
+## provider.
+##
+associate_saml() {
+
+ # Verify required parameters are present
+ if [ -z "$SAML_IDP_METADATA_URL" ]
+ then
+ cat <<END
+FATAL: Missing required environment variables
+-------------------------------------------------------------------------------
+If using an openid authentication, you must provide each of the following
+environment variables:
+
+ SAML_IDP_METADATA_URL The URI of the XML metadata file that from
the SAML Identity
+ Provider
+END
+ exit 1;
+ fi
+
+ # Update config file
+ set_property "saml-idp-metadata-url"
"$SAML_IDP_METADATA_URL"
Review comment:
I think this has to be optional, as well, unfortunately. Only the SAML
support itself is going to be able to accurately sanity check these variables.
It's possible for the `saml-idp-metadata-url` property to be omitted, and
the required values instead provided through individual properties (like
`saml-entity-id` and `saml-callback-url`), and it's also possible to specify
`saml-idp-metadata-url` _and_ individual properties to make up for any
information not included in the XML pointed to by that URL.
--
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]
