Merge pull request #171 from daggerhart/90-state-race

Fix race condition by replacing states option array with individual transients

includes/openid-connect-generic-client.php

@@ -232,53 +232,31 @@ class OpenID_Connect_Generic_Client {
 	}
 
 	/**
-	 * Generate a new state, save it to the states option with a timestamp,
-	 *  and return it.
+	 * Generate a new state, save it as a transient,
+	 *  and return the state hash.
 	 *
 	 * @return string
 	 */
 	function new_state() {
-		$states = get_option( 'openid-connect-generic-valid-states', array() );
-
 		// new state w/ timestamp
-		$new_state            = md5( mt_rand() . microtime( true ) );
-		$states[ $new_state ] = time();
-
-		// save state
-		update_option( 'openid-connect-generic-valid-states', $states );
+		$state = md5( mt_rand() . microtime( true ) );
+		$expire = time() + $this->state_time_limit;
+		set_transient( 'openid-connect-generic-state--' . $state, $state, $expire );
 
-		return $new_state;
+		return $state;
 	}
 
 	/**
-	 * Check the validity of a given state
+	 * Check the existence of a given state transient.
 	 *
 	 * @param $state
 	 * 
 	 * @return bool
 	 */
 	function check_state( $state ) {
-		$states = get_option( 'openid-connect-generic-valid-states', array() );
-		$valid  = false;
-
-		// remove any expired states
-		foreach ( $states as $code => $timestamp ) {
-			if ( ( $timestamp + $this->state_time_limit ) < time() ) {
-				unset( $states[ $code ] );
-			}
-		}
-
-		// see if the current state is still within the list of valid states
-		if ( isset( $states[ $state ] ) ) {
-			// state is valid, remove it
-			unset( $states[ $state ] );
-			$valid = true;
-		}
-
-		// save our altered states
-		update_option( 'openid-connect-generic-valid-states', $states );
+		$valid = get_transient( 'openid-connect-generic-state--' . $state );
 
-		return $valid;
+		return !!$valid;
 	}
 
 	/**

openid-connect-generic.php

@@ -3,7 +3,7 @@
 Plugin Name: OpenID Connect Generic
 Plugin URI: https://github.com/daggerhart/openid-connect-generic
 Description:  Connect to an OpenID Connect generic client using Authorization Code Flow
-Version: 3.5.0
+Version: 3.5.1
 Author: daggerhart
 Author URI: http://www.daggerhart.com
 License: GPLv2 Copyright (c) 2015 daggerhart
@@ -30,6 +30,7 @@ Notes
   - openid-connect-generic-update-user-using-current-claim - 2 args: fires every time an existing user logs
   - openid-connect-generic-redirect-user-back - 2 args: $redirect_url, $user. Allows interruption of redirect during login.
   - openid-connect-generic-user-logged-in     - 1 arg: $user, fires when user is logged in.
+  - openid-connect-generic-cron-daily         - daily cron action
 
   User Meta
   - openid-connect-generic-subject-identity    - the identity of the user provided by the idp
@@ -45,7 +46,7 @@ Notes
 
 class OpenID_Connect_Generic {
 	// plugin version
-	const VERSION = '3.5.0';
+	const VERSION = '3.5.1';
 
 	// plugin settings
 	private $settings;
@@ -110,6 +111,9 @@ class OpenID_Connect_Generic {
 		// add a shortcode to get the auth url
 		add_shortcode( 'openid_connect_generic_auth_url', array( $this->client_wrapper, 'get_authentication_url' ) );
 
+		// add actions to our scheduled cron jobs
+		add_action( 'openid-connect-generic-cron-daily', [ $this, 'cron_states_garbage_collection'] );
+
 		$this->upgrade();
 
 		if ( is_admin() ){
@@ -153,6 +157,7 @@ class OpenID_Connect_Generic {
 
 		if ( version_compare( self::VERSION, $last_version, '>' ) ) {
 			// upgrade required
+			self::setup_cron_jobs();
 
 			// @todo move this to another file for upgrade scripts
 			if ( isset( $settings->ep_login ) ) {
@@ -169,6 +174,44 @@ class OpenID_Connect_Generic {
 		}
 	}
 
+	/**
+	 * Expire state transients by attempting to access them and allowing the
+	 * transient's own mechanisms to delete any that have expired.
+	 */
+	function cron_states_garbage_collection() {
+		global $wpdb;
+		$states = $wpdb->get_col( "SELECT `option_name` FROM {$wpdb->options} WHERE `option_name` LIKE 'openid-connect-generic-state--%'" );
+
+		if ( !empty( $states ) ) {
+			foreach ( $states as $state ) {
+				get_transient( $state );
+			}
+		}
+	}
+
+	/**
+	 * Ensure cron jobs are added to the schedule.
+	 */
+	static public function setup_cron_jobs() {
+		if ( ! wp_next_scheduled( 'openid-connect-generic-cron-daily' ) ) {
+			wp_schedule_event( time(), 'daily', 'openid-connect-generic-cron-daily' );
+		}
+	}
+
+	/**
+	 * Activation hook.
+	 */
+	static public function activation() {
+		self::setup_cron_jobs();
+	}
+
+	/**
+	 * Deactivation hook.
+	 */
+	static public function deactivation() {
+		wp_clear_scheduled_hook( 'openid-connect-generic-cron-daily' );
+	}
+
 	/**
 	 * Simple autoloader
 	 *
@@ -253,3 +296,6 @@ class OpenID_Connect_Generic {
 }
 
 OpenID_Connect_Generic::bootstrap();
+
+register_activation_hook( __FILE__, [ 'OpenID_Connect_Generic', 'activation' ] );
+register_deactivation_hook( __FILE__, [ 'OpenID_Connect_Generic', 'deactivation' ] );

readme.txt

@@ -50,6 +50,10 @@ On the settings page for this plugin (Dashboard > Settings > OpenID Connect Gene
 
 == Changelog ==
 
+= 3.5.1 =
+
+* Fix: @daggerhart - New approach to state management using transients.
+
 = 3.5.0 =
 
 * Readme fix: @thijskh - Fix syntax error in example openid-connect-generic-login-button-text