mike-jumper commented on code in PR #990:
URL: https://github.com/apache/guacamole-client/pull/990#discussion_r1636840643
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -371,7 +371,13 @@ public int getAuthenticationTimeout() throws
GuacamoleException {
* If the X.509 certificate cannot be parsed.
*/
public File getCertificateFile() throws GuacamoleException {
- return environment.getProperty(SAML_X509_CERT_PATH);
+ File certificate = null;
+ try {
+ certificate =
environment.getProperty(SAML_X509_CERT_PATH).getCanonicalFile();
+ } catch (IOException | GuacamoleException e) {
+ e.printStackTrace();
Review Comment:
If an error should be logged, it should be logged by a `Logger` instance
with the stack trace logged only at the debug level. We typically do this by
logging things twice, with an admin-facing message at the error level and a
developer-facing message at the debug level.
That said, I think the proper thing to do here would be to just rethrow the
`IOException` as a `GuacamoleException`, with the `GuacamoleException` adding a
meaningful message.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -480,7 +492,7 @@ public Saml2Settings getSamlSettings() throws
GuacamoleException {
readFileContentsIntoString(privateKeyFile, "Private Key"));
// If a certificate file is set, load the value into the builder now
- File certificateFile = getCertificateFile();
+ File certificateFile = getCertificateFile();
Review Comment:
It looks like this line was changed only by adding several spaces to the end.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -371,7 +371,13 @@ public int getAuthenticationTimeout() throws
GuacamoleException {
* If the X.509 certificate cannot be parsed.
*/
public File getCertificateFile() throws GuacamoleException {
- return environment.getProperty(SAML_X509_CERT_PATH);
+ File certificate = null;
+ try {
+ certificate =
environment.getProperty(SAML_X509_CERT_PATH).getCanonicalFile();
Review Comment:
This will throw a `NullPointerException` if `getProperty()` returns `null`
(if the property is not present). Same for the other call to
`getCanonicalFile()`.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -371,7 +371,13 @@ public int getAuthenticationTimeout() throws
GuacamoleException {
* If the X.509 certificate cannot be parsed.
*/
public File getCertificateFile() throws GuacamoleException {
- return environment.getProperty(SAML_X509_CERT_PATH);
+ File certificate = null;
+ try {
+ certificate =
environment.getProperty(SAML_X509_CERT_PATH).getCanonicalFile();
Review Comment:
What is the problem you're encountering that is fixed by calling
`getCanonicalFile()`?
--
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]
