Treat review comments

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/AuthenticationProviderService.java

@@ -146,14 +146,15 @@ public class AuthenticationProviderService implements SSOAuthenticationProviderS
         if (confService.isImplicitFlow()) {
             String token = credentials.getParameter(IMPLICIT_TOKEN_PARAMETER_NAME);
             if (token != null) {
-                JwtClaims claims = tokenService.validateToken(token);
+                JwtClaims claims = tokenService.validateTokenOrCode(token, "");
                 if (claims != null) {
                     username = tokenService.processUsername(claims);
                     groups = tokenService.processGroups(claims);
                     tokens = tokenService.processAttributes(claims);
                 }
             }
-        } else {
+        }
+        else {
             String verifier = null;
             if (confService.isPKCERequired()) {
                 // Recover session
@@ -164,7 +165,7 @@ public class AuthenticationProviderService implements SSOAuthenticationProviderS
             }
             String code = credentials.getParameter("code");
             if (code != null && (confService.isPKCERequired() == false || verifier != null)) {
-                JwtClaims claims = tokenService.validateCode(code, verifier);
+                JwtClaims claims = tokenService.validateTokenOrCode(code, verifier);
                 if (claims != null) {
                     username = tokenService.processUsername(claims);
                     groups = tokenService.processGroups(claims);
@@ -203,7 +204,8 @@ public class AuthenticationProviderService implements SSOAuthenticationProviderS
 
         if (confService.isImplicitFlow()) {
             builder.queryParam("nonce", nonceService.generate(confService.getMaxNonceValidity() * 60000L));
-        } else  {
+        }
+        else  {
             if (confService.isPKCERequired()) {
                 String codeVerifier = PKCEUtil.generateCodeVerifier();
                 String codeChallenge;

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/OpenIDAuthenticationProviderModule.java

@@ -46,6 +46,7 @@ public class OpenIDAuthenticationProviderModule extends AbstractModule {
         bind(NonceService.class).in(Scopes.SINGLETON);
         bind(TokenValidationService.class);
         bind(OpenIDAuthenticationSessionManager.class);
+
         bind(Environment.class).toInstance(environment);
     }
 

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/OpenIDAuthenticationSession.java

@@ -24,6 +24,7 @@ import org.apache.guacamole.net.auth.AuthenticationSession;
 /**
  * Representation of an in-progress OpenID authentication attempt.
  */
+
 public class OpenIDAuthenticationSession extends AuthenticationSession {
     /**
      * The PKCE challenge verifier.

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java

@@ -20,7 +20,6 @@
 package org.apache.guacamole.auth.openid.conf;
 
 import com.google.inject.Inject;
-
 import java.net.URI;
 import java.util.Collection;
 import java.util.Collections;
@@ -125,10 +124,9 @@ public class ConfigurationService {
      */
     private static final URIGuacamoleProperty OPENID_TOKEN_ENDPOINT =
             new URIGuacamoleProperty() {
+
         @Override
-        public String getName() {
+        public String getName() { return "openid-token-endpoint"; }
-            return "openid-token-endpoint";
-        }
     };
 
     /**
@@ -148,10 +146,9 @@ public class ConfigurationService {
      */
     private static final StringGuacamoleProperty OPENID_CLIENT_SECRET =
             new StringGuacamoleProperty() {
+
         @Override
-        public String getName() {
+        public String getName() { return "openid-client-secret"; }
-            return "openid-client-secret";
-        }
     };
 
     /**
@@ -159,10 +156,9 @@ public class ConfigurationService {
      */
     private static final BooleanGuacamoleProperty OPENID_PKCE_REQUIRED =
             new BooleanGuacamoleProperty() {
+
         @Override
-        public String getName() {
+        public String getName() { return "openid-pkce-required"; }
-            return "openid-pkce-required";
-        }
     };
 
     /**
@@ -174,9 +170,9 @@ public class ConfigurationService {
      */
     private static final IntegerGuacamoleProperty OPENID_AUTH_TIMEOUT =
             new IntegerGuacamoleProperty() {
+
         @Override
         public String getName() { return "openid-auth-timeout"; }
-
     };
 
 
@@ -448,22 +444,6 @@ public class ConfigurationService {
         return token_endpoint;
     }
 
-    /**
-     * Returns the well-known endpoint (URI) of the OIDC service as
-     * configured with guacamole.properties.
-     *
-     * @return
-     *     The well-known endpoint of the OIDC service, as configured with
-     *     guacamole.properties.
-     *
-     * @throws GuacamoleException
-     *     If guacamole.properties cannot be parsed, or if the well-known
-     *     endpoint property is missing.
-     */
-    public URI getWellKnownEndpoint() throws GuacamoleException {
-        return confWellKnown.getWellKnownEndpoint();
-    }    
-
     /**
      * Returns the reponse type of the OpenID service as configured with guacamole.properties.
      *

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/OpenIDWellKnown.java

@@ -46,7 +46,7 @@ public class OpenIDWellKnown {
     /**
      * Logger for this class.
      */
-    private final Logger logger = LoggerFactory.getLogger(OpenIDWellKnown.class);
+    private static final Logger logger = LoggerFactory.getLogger(OpenIDWellKnown.class);
 
 
     /**
@@ -62,32 +62,37 @@ public class OpenIDWellKnown {
     /**
      * The detected issuer
      */
-     private static String issuer = null;
+    private static String issuer = null;
 
     /**
      * The detected authorization edpoint
      */
-     private static URI authorization_endpoint = null;
+    private static URI authorization_endpoint = null;
 
     /**
      * The detected token edpoint
      */
-     private static URI token_endpoint = null;
+    private static URI token_endpoint = null;
 
     /**
      * The detected jwks_uri
      */
-     private static URI jwks_uri = null;
+    private static URI jwks_uri = null;
+    
+    /**
+     * Empty constructor of the class to populate data recovered from a OIDC
+     * well-known URL. The class will be populated on injection by Guice
+     */
+    public OpenIDWellKnown() { }
 
     /**
      * The well-known endpoint (URI) of the OIDC service.
      */
     private static final URIGuacamoleProperty OPENID_WELL_KNOWN_ENDPOINT =
             new URIGuacamoleProperty() {
+
         @Override
-        public String getName() {
+        public String getName() { return "openid-well-known-endpoint"; }
-            return "openid-well-known-endpoint";
-        }
     };
 
     /**
@@ -102,7 +107,7 @@ public class OpenIDWellKnown {
      *     If guacamole.properties cannot be parsed, or if the authorization
      *     endpoint property is missing.
      */
-    public URI getWellKnownEndpoint() throws GuacamoleException {
+    private URI getWellKnownEndpoint() throws GuacamoleException {
         return environment.getProperty(OPENID_WELL_KNOWN_ENDPOINT);
     }
 
@@ -163,15 +168,22 @@ public class OpenIDWellKnown {
     private Environment environment;
 
     /*
-     * Creates an OpenIDWellKnown class that reads the json from an OIDC
+     * On injection, when the environment is non null, populates the OpenIDWellKnown 
-     * well-known endpoint and saves these values for later use. Use Guice
+     * class by reading the json from an OIDC well-known endpoint and saves these values
-     * to ensure environment exists before initializing.
+     * for later use. Use Guice to ensure environment exists before initializing.
      */
-    public OpenIDWellKnown() {
-    }
-
     @Inject
     public void init() {
+        // Fast return if there is no well-known endpoint or its unreadable
+        try {
+            if (getWellKnownEndpoint() == null) {
+                return;
+            }
+        }
+        catch (Exception e) {
+            return;
+        }
+    
         // Call to well-known endpoint might fail, so allow several tries before giving up
         ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor();
         
@@ -198,7 +210,8 @@ public class OpenIDWellKnown {
 
                     scheduler.shutdown();
                     return;
-                } catch (Exception e) {
+                }
+                catch (Exception e) {
                     logger.debug("Rejecting well-known endpoint : {}", e.getMessage());
                 }
 

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java

@@ -30,7 +30,6 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-
 import javax.ws.rs.core.UriBuilder;
 import org.apache.guacamole.GuacamoleException;
 import org.apache.guacamole.auth.openid.conf.ConfigurationService;
@@ -77,20 +76,26 @@ public class TokenValidationService {
     private NonceService nonceService;
 
     /**
-     * Validates the given ID token, using implicit flow, returning the JwtClaims
+     * Validates the given ID token, using implicit flow. Also validates codes,
-     * contained therein. If the ID token is invalid, null is returned.
+     * exchanging them for id_tokens before validation. If the id_token or
+     * code is invalid, null is returned, otherwise the JwtClaims in the
+     * id_token are returned.
      *
      * @param token
-     *     The ID token to validate.
+     *     The ID token to validate if implicit flow or the code to exchange for
+     *     an id_token and then validate.
+     *
+     * @param verifier
+     *     A PKCE verifier or null if not used. Only used with code flow
      *
      * @return
-     *     The JWT claims contained within the given ID token if it passes tests,
+     *     The JWT claims contained within the id_token if it passes tests,
-     *     or null if the token is not valid.
+     *     or null if the id_token is not valid.
      *
      * @throws GuacamoleException
      *     If guacamole.properties could not be parsed.
      */
-    public JwtClaims validateToken(String token) throws GuacamoleException {
+    public JwtClaims validateTokenOrCode(String token, String verifier) throws GuacamoleException {
         // Validating the token requires a JWKS key resolver
         HttpsJwks jwks = new HttpsJwks(confService.getJWKSEndpoint().toString());
         HttpsJwksVerificationKeyResolver resolver = new HttpsJwksVerificationKeyResolver(jwks);
@@ -106,25 +111,35 @@ public class TokenValidationService {
                 .setVerificationKeyResolver(resolver)
                 .build();
                 
+        /* Exchange code → token */
+        if (! confService.isImplicitFlow()) {
+            token = exchangeCode(token, verifier);                
+        }
+        
         try {
             // Validate JWT
             JwtClaims claims = jwtConsumer.processToClaims(token);
 
-            // Verify a nonce is present
+            if (confService.isImplicitFlow()) {
-            String nonce = claims.getStringClaimValue("nonce");
+                // Verify a nonce is present
-            if (nonce != null) {
+                String nonce = claims.getStringClaimValue("nonce");
-                // Verify that we actually generated the nonce, and that it has not
+                if (nonce != null) {
-                // already been used
+                    // Verify that we actually generated the nonce, and that it has not
-                if (nonceService.isValid(nonce)) {
+                    // already been used
-                    // nonce is valid, consider claims valid
+                    if (nonceService.isValid(nonce)) {
-                    return claims;
+                        // nonce is valid, consider claims valid
+                        return claims;
+                    }
+                    else {
+                        logger.info("Rejected OpenID token with invalid/old nonce.");
+                    }
                 }
                 else {
-                    logger.info("Rejected OpenID token with invalid/old nonce.");
+                    logger.info("Rejected OpenID token without nonce.");
                 }
             }
             else {
-                logger.info("Rejected OpenID token without nonce.");
+                return claims;
             }
         }
         // Log any failures to validate/parse the JWT
@@ -138,54 +153,6 @@ public class TokenValidationService {
         return null;
     }
 
-    /**
-     * Validates the given ID token, using code flow, returning the JwtClaims
-     * contained therein. If the ID token is invalid, null is returned.
-     *
-     * @param code
-     *     The code to validate and receive the id_token.
-     *
-     * @param verifier
-     *     A PKCE verifier or null if not used.
-     *
-     * @return
-     *     The JWT claims contained within the given ID token if it passes tests,
-     *     or null if the token is not valid.
-     *
-     * @throws GuacamoleException
-     *     If guacamole.properties could not be parsed.
-     */
-    public JwtClaims validateCode(String code, String verifier) throws GuacamoleException {
-        // Validating the token requires a JWKS key resolver
-        HttpsJwks jwks = new HttpsJwks(confService.getJWKSEndpoint().toString());
-        HttpsJwksVerificationKeyResolver resolver = new HttpsJwksVerificationKeyResolver(jwks);
-
-        /* Exchange code → token */
-        String token = exchangeCode(code, verifier);
-
-        // Create JWT consumer for validating received token
-        JwtConsumer jwtConsumer = new JwtConsumerBuilder()
-                .setRequireExpirationTime()
-                .setMaxFutureValidityInMinutes(confService.getMaxTokenValidity())
-                .setAllowedClockSkewInSeconds(confService.getAllowedClockSkew())
-                .setRequireSubject()
-                .setExpectedIssuer(confService.getIssuer())
-                .setExpectedAudience(confService.getClientID())
-                .setVerificationKeyResolver(resolver)
-                .build();
-
-        try {
-            // Validate JWT
-            return jwtConsumer.processToClaims(token);
-        }
-        // Log any failures to validate/parse the JWT
-        catch (InvalidJwtException e) {
-            logger.info("Rejected invalid OpenID token: {}", e.getMessage(), e);
-        }
-
-        return null;
-    }
-    
     /**
      * URLEncodes a key/value pair
      *
@@ -211,6 +178,7 @@ public class TokenValidationService {
      *
      * @param code
      *     The authorization code received from the IdP.
+     *
      * @param codeVerifier
      *     The PKCE verifier (or null if PKCE is disabled).
      *
@@ -245,7 +213,8 @@ public class TokenValidationService {
 
             return (String) json.get("id_token");
 
-        } catch (Exception e) {
+        }
+        catch (Exception e) {
             logger.info("Rejected invalid OpenID code exchange: {}", e.getMessage(), e);
         }
         return null;

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/util/JsonUrlReader.java

@@ -20,13 +20,13 @@
 package org.apache.guacamole.auth.openid.util;
 
 import java.io.BufferedReader;
+import java.io.IOException;
 import java.io.OutputStream;
 import java.io.InputStreamReader;
 import java.net.HttpURLConnection;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
-import org.apache.guacamole.GuacamoleException;
 import org.jose4j.json.JsonUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -35,17 +35,38 @@ import org.slf4j.LoggerFactory;
  * Utility class to open a http connection to a URL, send a body
  * and receive a response in the form of a parsed JSON
  */
+
 public final class JsonUrlReader {
     /**
      * Logger for this class.
      */
     private static final Logger logger = LoggerFactory.getLogger(JsonUrlReader.class);
 
+    /**
+     * Class to GET and POST to a URL and read the returned JSON. This class should
+     * not be instantiated.
+     */
     private JsonUrlReader() {}
 
-    public static Map<String,Object> fetch(String method, URL url, String body) throws GuacamoleException {
+    /**
-        if (url == null) {
+     * Method to POST or GET to a URL and recover the JSON in the form of a Map
-            throw new GuacamoleException("JsonUrlReader : Missing URL");
+     *
+     * @param String method
+     *      The htpp method to use. Should be "GET", "POST" or "PATCH".
+     *
+     * @param URL url
+     *      A URL value giving the address where to recover the JSON
+     *
+     * @param String body
+     *      A pre-encoded body string to be sent to the address if the method is
+     *      "POST" or "PATCH". Ignored if the method is "GET".
+     *
+     * @return
+     *      A Map<String,Object> containing the decoded json values.
+     */
+    public static Map<String,Object> fetch(String method, URL url, String body) throws IOException {
+        if (url == null || url.toString() == "") {
+            throw new IOException("JsonUrlReader : Missing URL");
         }
         
         try {
@@ -55,7 +76,7 @@ public final class JsonUrlReader {
             conn.setRequestMethod(method);
             conn.setRequestProperty("Content-Type", "application/x-www-form-urlencoded; charset=UTF-8");
 
-            if (! method.equalsIgnoreCase("GET")) {
+            if (! method.equals("GET")) {
                 conn.setDoOutput(true);
                 try (OutputStream out = conn.getOutputStream()) {
                     byte [] bodyBytes = body.getBytes(StandardCharsets.UTF_8);
@@ -87,12 +108,13 @@ public final class JsonUrlReader {
             Map<String,Object> json = JsonUtil.parseJson(responseBody.toString());
 
             if (status < 200 || status >= 300) {
-                throw new GuacamoleException("(status: " + status + "): " + json.toString());
+                throw new IOException("(status: " + status + "): " + json.toString());
             }
 
             return json;
-        } catch (Exception e) {
+        }
-            throw new GuacamoleException("JsonUrlreader error : " + method + " : " + e.getMessage());
+        catch (Exception e) {
+            throw new IOException("JsonUrlreader error : " + e.getMessage());
         }
     }
 }

extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/util/PKCEUtil.java

@@ -30,13 +30,21 @@ import java.security.SecureRandom;
  *   - code_challenge (S256)
  */
 public final class PKCEUtil {
-
+    /**
+     * Get the verifier data from a secure random source
+     */
     private static final SecureRandom RANDOM = new SecureRandom();
 
+    /*
+     * Class to create PKCE challenges and verifiers. This class should not be instantiated
+     */
     private PKCEUtil() {}
 
     /**
      * Generates a high-entropy PKCE code_verifier.
+     *
+     * @return
+     *      A 256bit or 64 byte random Base64 URL encode string
      */
     public static String generateCodeVerifier() {
         byte[] bytes = new byte[64];
@@ -46,6 +54,13 @@ public final class PKCEUtil {
 
     /**
      * Computes the PKCE code_challenge = BASE64URL(SHA256(code_verifier)).
+     *
+     * @param String verifier
+     *      A string containing the S56 verifier calculated bu generateCodeVerifier
+     *
+     * @return
+     *      The generated S256 code challenge used for the PKCE request encoded 
+     *      in Base64 URL format.
      */
     public static String generateCodeChallenge(String verifier) throws Exception {
         MessageDigest sha = MessageDigest.getInstance("SHA-256");
@@ -55,6 +70,12 @@ public final class PKCEUtil {
 
     /**
      * Base64URL encoding without padding.
+     *
+     * @param bytes
+     *      The bytes to be Base54 URL encoded
+     *
+     * @return
+     *      The Base64 URL encoded string value corresponding to the bytes 
      */
     public static String base64Url(byte[] bytes) {
         return java.util.Base64.getUrlEncoder()