Refactors new ACR Handling Code for Better Code Quality

* Fixes some incorrect ACR handling code that incorrectly added the ACR when it wasn't set. * Adds missing settings disable handling when ACR constant is set. * Refactors code for simplicity and code quality.
oidc-wp · Mar 9, 2022 · 63ef4bb · 63ef4bb
1 parent c42bf78
commit 63ef4bb
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 47 deletions.
diff --git a/includes/openid-connect-generic-client-wrapper.php b/includes/openid-connect-generic-client-wrapper.php
@@ -229,29 +229,23 @@ public function get_authentication_url( $atts = array() ) {
 		if ( stripos( $this->settings->endpoint_login, '?' ) !== false ) {
 			$separator = '&';
 		}
-		if ( empty( $this->settings->acr_values ) ) {
-			$url = sprintf(
-				'%1$s%2$sresponse_type=code&scope=%3$s&client_id=%4$s&state=%5$s&redirect_uri=%6$s',
-				$atts['endpoint_login'],
-				$separator,
-				rawurlencode( $atts['scope'] ),
-				rawurlencode( $atts['client_id'] ),
-				$this->client->new_state( $atts['redirect_to'] ),
-				rawurlencode( $atts['redirect_uri'] )
-			);
-		} else {
-			$url = sprintf(
-				'%1$s%2$sresponse_type=code&scope=%3$s&client_id=%4$s&state=%5$s&redirect_uri=%6$s&acr_values=%7$s',
-				$atts['endpoint_login'],
-				$separator,
-				rawurlencode( $atts['scope'] ),
-				rawurlencode( $atts['client_id'] ),
-				$this->client->new_state( $atts['redirect_to'] ),
-				rawurlencode( $atts['redirect_uri'] ),
-				rawurlencode( $atts['acr_values'] )
-			);
+
+		$url_format = '%1$s%2$sresponse_type=code&scope=%3$s&client_id=%4$s&state=%5$s&redirect_uri=%6$s';
+		if ( ! empty( $atts['acr_values'] ) ) {
+			$url_format .= '&acr_values=%7$s';
 		}
 
+		$url = sprintf(
+			$url_format,
+			$atts['endpoint_login'],
+			$separator,
+			rawurlencode( $atts['scope'] ),
+			rawurlencode( $atts['client_id'] ),
+			$this->client->new_state( $atts['redirect_to'] ),
+			rawurlencode( $atts['redirect_uri'] ),
+			rawurlencode( $atts['acr_values'] )
+		);
+
 		$this->logger->log( apply_filters( 'openid-connect-generic-auth-url', $url ), 'make_authentication_url' );
 		return apply_filters( 'openid-connect-generic-auth-url', $url );
 	}

diff --git a/includes/openid-connect-generic-client.php b/includes/openid-connect-generic-client.php
@@ -211,32 +211,20 @@ public function request_authentication_token( $code ) {
 		$parsed_url = parse_url( $this->endpoint_token );
 		$host = $parsed_url['host'];
 
-		if ( $this->acr_values ) {
-			$request = array(
-				'body' => array(
-					'code'          => $code,
-					'client_id'     => $this->client_id,
-					'client_secret' => $this->client_secret,
-					'redirect_uri'  => $this->redirect_uri,
-					'grant_type'    => 'authorization_code',
-					'scope'         => $this->scope,
-					'acr_values'    => $this->acr_values,
-				),
-				'headers' => array( 'Host' => $host ),
-			);
-		} else {
-			$request = array(
-				'body' => array(
-					'code'          => $code,
-					'client_id'     => $this->client_id,
-					'client_secret' => $this->client_secret,
-					'redirect_uri'  => $this->redirect_uri,
-					'grant_type'    => 'authorization_code',
-					'scope'         => $this->scope,
-					'acr_values'    => $this->acr_values,
-				),
-				'headers' => array( 'Host' => $host ),
-			);
+		$request = array(
+			'body' => array(
+				'code'          => $code,
+				'client_id'     => $this->client_id,
+				'client_secret' => $this->client_secret,
+				'redirect_uri'  => $this->redirect_uri,
+				'grant_type'    => 'authorization_code',
+				'scope'         => $this->scope,
+			),
+			'headers' => array( 'Host' => $host ),
+		);
+
+		if ( ! empty( $this->acr_values ) ) {
+			$request['body'] += array( 'acr_values' => $this->acr_values );
 		}
 
 		// Allow modifications to the request.

diff --git a/includes/openid-connect-generic-settings-page.php b/includes/openid-connect-generic-settings-page.php
@@ -278,6 +278,7 @@ private function get_settings_fields() {
 				'title'       => __( 'ACR values', 'daggerhart-openid-connect-generic' ),
 				'description' => __( 'Use a specific defined authentication contract from the IDP - optional.', 'daggerhart-openid-connect-generic' ),
 				'type'        => 'text',
+				'disabled'    => defined( 'OIDC_ACR_VALUES' ),
 				'section'     => 'client_settings',
 			),
 			'identity_key'     => array(