diff --git a/src/conditionals/gradual-rollout-conditional.php b/src/conditionals/gradual-rollout-conditional.php new file mode 100644 index 00000000000..02c5e143fc7 --- /dev/null +++ b/src/conditionals/gradual-rollout-conditional.php @@ -0,0 +1,91 @@ +` constant remains an explicit override: when it is defined + * in wp-config.php it wins outright (`true` forces the feature on, `false` forces it off), + * exactly like a plain {@see Feature_Flag_Conditional}. This is the per-site testing lever. + * + * When the constant is *not* defined, the feature falls back to the gradual-rollout + * heuristic: it is enabled for a slowly widening share of sites. A site's bucket is derived + * from a stable hash of the feature name plus the site URL, so the same site stays in (or out + * of) the rollout consistently across plugin releases. + * + * The share is expressed in per-mille (0-1000), not percent, because at the install base this + * rides on (10M+ sites) a single percent is too coarse for the first rollout steps; per-mille + * lets a rollout start at 0.1% (a share of 1). + * + * The hash input deliberately includes the feature name, so a site that buckets low for one + * feature is not automatically early for every feature - there are no permanently "lucky" sites + * that always receive new features first. + * + * This machinery is temporary by design: once a feature reaches a 100% share with no + * regressions, the concrete conditional reverts to extending {@see Feature_Flag_Conditional} + * directly and this class can be removed. + */ +abstract class Gradual_Rollout_Conditional extends Feature_Flag_Conditional { + + /** + * The number of buckets sites are distributed across. + * + * @var int + */ + private const BUCKET_COUNT = 1000; + + /** + * Returns whether the feature is enabled. + * + * The `YOAST_SEO_` constant, when defined, is an explicit override and wins. + * Otherwise the gradual-rollout share decides. + * + * @return bool Whether the conditional is met. + */ + public function is_met() { + $constant = 'YOAST_SEO_' . \strtoupper( $this->get_feature_flag() ); + + // An explicit constant always wins (true forces on, false forces off). + if ( \defined( $constant ) ) { + return ( \constant( $constant ) === true ); + } + + return $this->is_in_rollout_cohort(); + } + + /** + * Returns the current rollout share in per-mille (0-1000). + * + * 0 means the feature is enabled for no sites, 1000 for all sites. The value is + * raised release over release as the rollout widens. + * + * @return int The rollout share in per-mille. + */ + abstract protected function get_rollout_share(): int; + + /** + * Determines whether this site falls within the current rollout share. + * + * @return bool Whether this site is in the rollout cohort. + */ + private function is_in_rollout_cohort(): bool { + $share = \max( 0, \min( self::BUCKET_COUNT, $this->get_rollout_share() ) ); + + if ( $share <= 0 ) { + return false; + } + + if ( $share >= self::BUCKET_COUNT ) { + return true; + } + + // Hash the feature name together with the site URL so cohorts differ per feature + // (no permanently lucky sites). sprintf( '%u' ) reads crc32's result as unsigned, + // which keeps the modulo correct on 32-bit platforms where crc32 can be negative. + $bucket = ( (int) \sprintf( '%u', \crc32( $this->get_feature_name() . \site_url() ) ) % self::BUCKET_COUNT ); + + return ( $bucket < $share ); + } +} diff --git a/src/conditionals/myyoast-connection-conditional.php b/src/conditionals/myyoast-connection-conditional.php index a0daa51702e..36901f954d7 100644 --- a/src/conditionals/myyoast-connection-conditional.php +++ b/src/conditionals/myyoast-connection-conditional.php @@ -4,18 +4,32 @@ /** * Feature flag conditional for the MyYoast connection (OAuth client, WP-CLI - * commands, and the key-rotation cron). + * commands, and the key-rotation cron, etc.). * * Enable by defining `YOAST_SEO_MYYOAST_CONNECTION` as `true` in wp-config.php. + * On top of the flag, the connection is rolled out gradually to a deterministic + * share of sites — see {@see Gradual_Rollout_Conditional}. */ -class MyYoast_Connection_Conditional extends Feature_Flag_Conditional { +class MyYoast_Connection_Conditional extends Gradual_Rollout_Conditional { /** * Returns the name of the feature flag. * * @return string The name of the feature flag. */ - protected function get_feature_flag() { + protected function get_feature_flag(): string { return 'MYYOAST_CONNECTION'; } + + /** + * The share of sites the connection is rolled out to, in per-mille (0-1000). + * + * Ships at 1%; raised release over release as the rollout widens. + * + * @return int The rollout share in per-mille. + */ + protected function get_rollout_share(): int { + // 1%. + return 10; + } } diff --git a/src/myyoast-client/application/exceptions/registration-temporarily-unavailable-exception.php b/src/myyoast-client/application/exceptions/registration-temporarily-unavailable-exception.php new file mode 100644 index 00000000000..eda66f86184 --- /dev/null +++ b/src/myyoast-client/application/exceptions/registration-temporarily-unavailable-exception.php @@ -0,0 +1,49 @@ +retry_after_seconds = $retry_after_seconds; + } + + /** + * Returns the server-suggested retry delay in seconds, or null when none was provided. + * + * @return int|null The retry delay in seconds, or null. + */ + public function get_retry_after_seconds(): ?int { + return $this->retry_after_seconds; + } +} diff --git a/src/myyoast-client/infrastructure/registration/client-registration.php b/src/myyoast-client/infrastructure/registration/client-registration.php index 92ffd749f51..72c980fb36e 100644 --- a/src/myyoast-client/infrastructure/registration/client-registration.php +++ b/src/myyoast-client/infrastructure/registration/client-registration.php @@ -8,9 +8,11 @@ use Yoast\WP\SEO\Helpers\Lock_Helper; use Yoast\WP\SEO\MyYoast_Client\Application\Exceptions\Discovery_Failed_Exception; use Yoast\WP\SEO\MyYoast_Client\Application\Exceptions\Registration_Failed_Exception; +use Yoast\WP\SEO\MyYoast_Client\Application\Exceptions\Registration_Temporarily_Unavailable_Exception; use Yoast\WP\SEO\MyYoast_Client\Application\Exceptions\Server_Capability_Exception; use Yoast\WP\SEO\MyYoast_Client\Application\Ports\Client_Registration_Interface; use Yoast\WP\SEO\MyYoast_Client\Domain\Auth_Token_Type; +use Yoast\WP\SEO\MyYoast_Client\Domain\HTTP_Response; use Yoast\WP\SEO\MyYoast_Client\Domain\Registered_Client; use Yoast\WP\SEO\MyYoast_Client\Infrastructure\Crypto\Encryption; use Yoast\WP\SEO\MyYoast_Client\Infrastructure\Crypto\Encryption_Exception; @@ -575,7 +577,8 @@ private function get_option_key(): string { * * @return Registered_Client The registration result. * - * @throws Registration_Failed_Exception If registration fails. + * @throws Registration_Temporarily_Unavailable_Exception If the server temporarily refuses new registrations. + * @throws Registration_Failed_Exception If registration fails. */ private function do_register( array $redirect_uris ): Registered_Client { try { @@ -631,6 +634,14 @@ private function do_register( array $redirect_uris ): Registered_Client { throw new Registration_Failed_Exception( 'DCR request failed: ' . $error_message ); } + // The server temporarily refuses new registrations (rollout brake engaged). + // Surface it as a typed transient failure carrying the (display-only) retry hint. + if ( $result->get_status() === 503 && $result->get_body_value( 'error' ) === 'temporarily_unavailable' ) { + $error_message = (string) $result->get_body_value( 'error_description', 'Client registration is temporarily disabled.' ); + // phpcs:ignore WordPress.Security.EscapeOutput.ExceptionNotEscaped -- Internal exception message. + throw new Registration_Temporarily_Unavailable_Exception( $error_message, $this->get_retry_after_seconds( $result ) ); + } + if ( $result->get_status() !== 201 ) { $error_message = (string) $result->get_body_value( 'error_description', $result->get_body_value( 'error', '' ) ); throw new Registration_Failed_Exception( @@ -647,6 +658,30 @@ private function do_register( array $redirect_uris ): Registered_Client { return $this->store_credentials( $body ); } + /** + * Extracts the Retry-After delay (in seconds) from a response, if present and numeric. + * + * Display-only: used solely to tell the user when to try again; the plugin does not + * schedule or enforce a wait based on it. + * + * @param HTTP_Response $result The response to read the Retry-After header from. + * + * @return int|null The retry delay in seconds, or null when absent or non-numeric. + */ + private function get_retry_after_seconds( HTTP_Response $result ): ?int { + foreach ( $result->get_headers() as $name => $value ) { + if ( \strtolower( (string) $name ) !== 'retry-after' ) { + continue; + } + + $value = \is_array( $value ) ? \reset( $value ) : $value; + + return \is_numeric( $value ) ? (int) $value : null; + } + + return null; + } + /** * Strips server-assigned fields from metadata for a RFC 7592 PUT request. * diff --git a/src/myyoast-client/user-interface/auth-command.php b/src/myyoast-client/user-interface/auth-command.php index 2ad361b1f33..9312fa1a4ac 100644 --- a/src/myyoast-client/user-interface/auth-command.php +++ b/src/myyoast-client/user-interface/auth-command.php @@ -10,6 +10,7 @@ use Yoast\WP\SEO\Conditionals\MyYoast_Connection_Conditional; use Yoast\WP\SEO\Loadable_Interface; use Yoast\WP\SEO\Main; +use Yoast\WP\SEO\MyYoast_Client\Application\Exceptions\Registration_Temporarily_Unavailable_Exception; use Yoast\WP\SEO\MyYoast_Client\Application\MyYoast_Client; use Yoast\WP\SEO\MyYoast_Client\Application\MyYoast_Client_Cleanup; use Yoast\WP\SEO\MyYoast_Client\Application\Ports\Client_Registration_Interface; @@ -249,6 +250,11 @@ public function register( $args = null, $assoc_args = null ): void { try { $client = $this->myyoast_client->ensure_registered(); + } catch ( Registration_Temporarily_Unavailable_Exception $e ) { + $retry_after = $e->get_retry_after_seconds(); + $retry_hint = ( $retry_after !== null ) ? \sprintf( ' Try again in %d seconds.', $retry_after ) : ' Try again later.'; + WP_CLI::error( 'Registration is temporarily unavailable.' . $retry_hint ); + return; } catch ( Exception $e ) { WP_CLI::error( 'Registration failed: ' . $e->getMessage() ); return; diff --git a/tests/Unit/Conditionals/Gradual_Rollout_Conditional_Test.php b/tests/Unit/Conditionals/Gradual_Rollout_Conditional_Test.php new file mode 100644 index 00000000000..3e402fa82c8 --- /dev/null +++ b/tests/Unit/Conditionals/Gradual_Rollout_Conditional_Test.php @@ -0,0 +1,209 @@ +assertTrue( $instance->is_met() ); + } + + /** + * Tests that a defined constant set to false forces the feature off, regardless of share. + * + * @covers ::is_met + * + * @return void + */ + public function test_defined_constant_false_forces_off() { + if ( ! \defined( 'YOAST_SEO_OVERRIDE_OFF' ) ) { + \define( 'YOAST_SEO_OVERRIDE_OFF', false ); + } + + // Share 1000 would include every site, but the explicit constant wins. + $instance = new Gradual_Rollout_Conditional_Double( 'OVERRIDE_OFF', 1000 ); + + $this->assertFalse( $instance->is_met() ); + } + + /** + * Tests that, with no constant defined, a share of 0 excludes everyone. + * + * @covers ::is_met + * + * @return void + */ + public function test_cohort_share_zero_excludes_everyone() { + $instance = new Gradual_Rollout_Conditional_Double( self::COHORT_FLAG, 0 ); + + $this->assertFalse( $instance->is_met() ); + } + + /** + * Tests that, with no constant defined, a share of 1000 includes everyone. + * + * @covers ::is_met + * + * @return void + */ + public function test_cohort_share_full_includes_everyone() { + $instance = new Gradual_Rollout_Conditional_Double( self::COHORT_FLAG, 1000 ); + + $this->assertTrue( $instance->is_met() ); + } + + /** + * Tests that an out-of-range share is clamped to the full bucket count. + * + * @covers ::is_met + * + * @return void + */ + public function test_share_above_range_is_clamped_to_full() { + $instance = new Gradual_Rollout_Conditional_Double( self::COHORT_FLAG, 5000 ); + + $this->assertTrue( $instance->is_met() ); + } + + /** + * Tests that a negative share is clamped to zero (nobody). + * + * @covers ::is_met + * + * @return void + */ + public function test_negative_share_is_clamped_to_zero() { + $instance = new Gradual_Rollout_Conditional_Double( self::COHORT_FLAG, -10 ); + + $this->assertFalse( $instance->is_met() ); + } + + /** + * Tests that a site whose bucket is below the share is included. + * + * @covers ::is_met + * + * @return void + */ + public function test_site_inside_share_is_included() { + // Set the share just above this site's bucket. + $bucket = $this->bucket_for( self::COHORT_FLAG, self::SITE_URL ); + $instance = new Gradual_Rollout_Conditional_Double( self::COHORT_FLAG, ( $bucket + 1 ) ); + + $this->assertTrue( $instance->is_met() ); + } + + /** + * Tests that a site whose bucket equals the share is excluded (boundary is exclusive). + * + * @covers ::is_met + * + * @return void + */ + public function test_site_at_share_boundary_is_excluded() { + $bucket = $this->bucket_for( self::COHORT_FLAG, self::SITE_URL ); + + // A bucket of 0 would make the share 0, which the $share <= 0 clamp handles before the + // boundary comparison, so the boundary case is only meaningful for a non-zero bucket. + if ( $bucket === 0 ) { + $this->markTestSkipped( 'Bucket for the test URL is on the clamp boundary.' ); + } + + $instance = new Gradual_Rollout_Conditional_Double( self::COHORT_FLAG, $bucket ); + + $this->assertFalse( $instance->is_met() ); + } + + /** + * Tests that the decision is stable across repeated calls for the same site. + * + * @covers ::is_met + * + * @return void + */ + public function test_decision_is_stable_for_same_site() { + $instance = new Gradual_Rollout_Conditional_Double( self::COHORT_FLAG, 500 ); + + $this->assertSame( $instance->is_met(), $instance->is_met() ); + } + + /** + * Tests that the feature flag name is part of the hash, so the same site can be in the + * cohort for one feature but not for another at the same share (no permanently "lucky" + * sites). Neither constant is defined, so the cohort decides for both. + * + * @covers ::is_met + * + * @return void + */ + public function test_cohort_differs_per_feature_for_same_site() { + $low_bucket = $this->bucket_for( 'FEATURE_LOW', self::SITE_URL ); + $high_bucket = $this->bucket_for( 'FEATURE_HIGH', self::SITE_URL ); + + // Confirm the fixture features bucket differently for this site; if they ever collide, + // the test premise no longer holds and needs different feature names. + $this->assertNotSame( $low_bucket, $high_bucket, 'Fixture features must bucket differently.' ); + + // Pick a share equal to the higher bucket, so the lower-bucket feature is in and the + // higher-bucket one is out (bucket < share). + $share = \max( $low_bucket, $high_bucket ); + + $in_feature = ( $low_bucket < $high_bucket ) ? 'FEATURE_LOW' : 'FEATURE_HIGH'; + $out_feature = ( $low_bucket < $high_bucket ) ? 'FEATURE_HIGH' : 'FEATURE_LOW'; + + $this->assertTrue( ( new Gradual_Rollout_Conditional_Double( $in_feature, $share ) )->is_met() ); + $this->assertFalse( ( new Gradual_Rollout_Conditional_Double( $out_feature, $share ) )->is_met() ); + } +} diff --git a/tests/Unit/Conditionals/MyYoast_Connection_Conditional_Test.php b/tests/Unit/Conditionals/MyYoast_Connection_Conditional_Test.php index 46ea4c8c8a0..85026404a01 100644 --- a/tests/Unit/Conditionals/MyYoast_Connection_Conditional_Test.php +++ b/tests/Unit/Conditionals/MyYoast_Connection_Conditional_Test.php @@ -44,13 +44,15 @@ public function test_get_feature_flag() { } /** - * Tests that the conditional is not met when the constant is not defined. + * Tests that, with no constant defined, the decision falls back to the gradual-rollout + * cohort. For the harness site URL the connection's bucket sits above the current rollout + * share, so the site is not (yet) in the cohort. * * @covers ::is_met * * @return void */ - public function test_is_met_returns_false_when_constant_not_defined() { + public function test_is_met_falls_back_to_cohort_when_constant_not_defined() { $this->assertFalse( $this->instance->is_met() ); } } diff --git a/tests/Unit/Doubles/Gradual_Rollout_Conditional_Double.php b/tests/Unit/Doubles/Gradual_Rollout_Conditional_Double.php new file mode 100644 index 00000000000..be83680a306 --- /dev/null +++ b/tests/Unit/Doubles/Gradual_Rollout_Conditional_Double.php @@ -0,0 +1,55 @@ +feature_flag = $feature_flag; + $this->rollout_share = $rollout_share; + } + + /** + * Returns the feature flag name. + * + * @return string The feature flag name. + */ + protected function get_feature_flag() { + return $this->feature_flag; + } + + /** + * Returns the rollout share in per-mille. + * + * @return int The rollout share. + */ + protected function get_rollout_share(): int { + return $this->rollout_share; + } +} diff --git a/tests/Unit/MyYoast_Client/Infrastructure/Registration/Client_Registration_Test.php b/tests/Unit/MyYoast_Client/Infrastructure/Registration/Client_Registration_Test.php index 294b00aa8b5..5e4d0936dad 100644 --- a/tests/Unit/MyYoast_Client/Infrastructure/Registration/Client_Registration_Test.php +++ b/tests/Unit/MyYoast_Client/Infrastructure/Registration/Client_Registration_Test.php @@ -8,6 +8,8 @@ use Yoast\WP\SEO\Exceptions\Locking\Lock_Timeout_Exception; use Yoast\WP\SEO\Helpers\Lock_Helper; use Yoast\WP\SEO\MyYoast_Client\Application\Exceptions\Registration_Failed_Exception; +use Yoast\WP\SEO\MyYoast_Client\Application\Exceptions\Registration_Temporarily_Unavailable_Exception; +use Yoast\WP\SEO\MyYoast_Client\Domain\Discovery_Document; use Yoast\WP\SEO\MyYoast_Client\Domain\HTTP_Response; use Yoast\WP\SEO\MyYoast_Client\Infrastructure\Crypto\Encryption; use Yoast\WP\SEO\MyYoast_Client\Infrastructure\Crypto\Key_Pair; @@ -180,6 +182,90 @@ public function test_register_throws_when_locked() { $this->instance->register( [] ); } + /** + * Tests that a 503 temporarily_unavailable DCR response throws a typed exception + * carrying the Retry-After value. + * + * @covers ::register + * + * @return void + */ + public function test_register_throws_temporarily_unavailable_with_retry_after() { + $this->mock_do_register_prerequisites(); + + $this->http_client + ->expects( 'request' ) + ->once() + ->andReturn( + new HTTP_Response( + 503, + [ 'retry-after' => '120' ], + [ + 'error' => 'temporarily_unavailable', + 'error_description' => 'Client registration is temporarily disabled', + ], + ), + ); + + try { + $this->instance->register( [ 'https://example.com/callback' ] ); + $this->fail( 'Expected Registration_Temporarily_Unavailable_Exception was not thrown.' ); + } catch ( Registration_Temporarily_Unavailable_Exception $e ) { + $this->assertSame( 120, $e->get_retry_after_seconds() ); + } + } + + /** + * Tests that a 503 temporarily_unavailable DCR response without a Retry-After header + * throws the typed exception with a null retry value. + * + * @covers ::register + * + * @return void + */ + public function test_register_throws_temporarily_unavailable_without_retry_after() { + $this->mock_do_register_prerequisites(); + + $this->http_client + ->expects( 'request' ) + ->once() + ->andReturn( + new HTTP_Response( + 503, + [], + [ 'error' => 'temporarily_unavailable' ], + ), + ); + + try { + $this->instance->register( [ 'https://example.com/callback' ] ); + $this->fail( 'Expected Registration_Temporarily_Unavailable_Exception was not thrown.' ); + } catch ( Registration_Temporarily_Unavailable_Exception $e ) { + $this->assertNull( $e->get_retry_after_seconds() ); + } + } + + /** + * Tests that a non-brake error response still throws the generic registration failure. + * + * @covers ::register + * + * @return void + */ + public function test_register_throws_generic_failure_on_other_errors() { + $this->mock_do_register_prerequisites(); + + $this->http_client + ->expects( 'request' ) + ->once() + ->andReturn( + new HTTP_Response( 503, [], [ 'error' => 'server_error' ] ), + ); + + $this->expectException( Registration_Failed_Exception::class ); + $this->instance->register( [ 'https://example.com/callback' ] ); + } + /** * Tests that delete_registration clears stored data. * @@ -782,4 +868,42 @@ private function mock_get_client( array $metadata = [] ): void { ->expects( 'decrypt' ) ->andReturn( 'decrypted-rat' ); } + + /** + * Sets up the mocks required for do_register() to reach the HTTP request, + * for a fresh (not-yet-registered) site. + * + * @return void + */ + private function mock_do_register_prerequisites(): void { + Functions\expect( 'get_current_blog_id' )->andReturn( 1 ); + $this->lock_helper->allows( 'execute' )->andReturnUsing( + static function ( $key, $callback ) { + return $callback(); + }, + ); + + // Not yet registered, so ensure_registered() proceeds to register(). + Functions\expect( 'get_option' ) + ->with( self::OPTION_KEY, false ) + ->andReturn( false ); + + $discovery_document = Mockery::mock( Discovery_Document::class ); + $discovery_document->allows( 'get_registration_endpoint' )->andReturn( 'https://my.yoast.com/api/oauth/reg' ); + $this->discovery_client->allows( 'get_document' )->andReturn( $discovery_document ); + + $this->issuer_config->allows( 'get_software_statement' )->andReturn( 'ss-jwt' ); + $this->issuer_config->allows( 'get_initial_access_token' )->andReturn( 'iat' ); + + $keypair = \sodium_crypto_sign_keypair(); + $key_pair = new Key_Pair( + \sodium_crypto_sign_publickey( $keypair ), + \sodium_crypto_sign_secretkey( $keypair ), + 'kid', + ); + $this->key_pair_manager->allows( 'get_or_create_key_pair' )->andReturn( $key_pair ); + $this->key_pair_manager->allows( 'get_public_key_jwk' )->andReturn( [ 'kty' => 'OKP' ] ); + + Functions\expect( 'wp_json_encode' )->andReturn( '{}' ); + } }