[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #615: GUACAMOLE-1322: Include SAML Plugin in Docker Image

[GitBox <gi...@apache.org>]

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: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org