Clarify logic with 418 enabled and raw disabled

Author: jeffw16

includes/ResponseFactory.php

@@ -63,17 +63,27 @@ public function __construct( ServiceOptions $options ) {
 	/**
 	 * Deny access using the configured strategy.
 	 *
+	 * When CrawlerProtectionRawDenial is enabled, a raw HTTP response is
+	 * sent.  If CrawlerProtectionUse418 is *also* enabled the response
+	 * uses the 418 "I'm a teapot" status; otherwise the configured
+	 * header / body are used.
+	 *
+	 * When CrawlerProtectionRawDenial is disabled, a pretty 403 page is
+	 * rendered through OutputPage regardless of the Use418 setting.
+	 *
 	 * @param OutputPage $output Used only for the "pretty" strategy
 	 * @return void
 	 */
 	public function denyAccess( $output ): void {
-		if ( $this->options->get( 'CrawlerProtectionUse418' ) ) {
-			$this->denyAccessWith418();
-		} elseif ( $this->options->get( 'CrawlerProtectionRawDenial' ) ) {
-			$this->denyAccessRaw(
-				$this->options->get( 'CrawlerProtectionRawDenialHeader' ),
-				$this->options->get( 'CrawlerProtectionRawDenialText' )
-			);
+		if ( $this->options->get( 'CrawlerProtectionRawDenial' ) ) {
+			if ( $this->options->get( 'CrawlerProtectionUse418' ) ) {
+				$this->denyAccessWith418();
+			} else {
+				$this->denyAccessRaw(
+					$this->options->get( 'CrawlerProtectionRawDenialHeader' ),
+					$this->options->get( 'CrawlerProtectionRawDenialText' )
+				);
+			}
 		} else {
 			$this->denyAccessPretty( $output );
 		}

tests/phpunit/unit/ResponseFactoryTest.php

@@ -67,12 +67,12 @@ public function testDenyAccessPrettySetStatusCode() {
 	/**
 	 * @covers ::denyAccess
 	 */
-	public function testDenyAccessChooses418WhenConfigured() {
+	public function testDenyAccessChooses418WhenBothRawAndUse418() {
 		$factory = $this->getMockBuilder( ResponseFactory::class )
 			->setConstructorArgs( [
 				new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [
 					'CrawlerProtectionUse418' => true,
-					'CrawlerProtectionRawDenial' => false,
+					'CrawlerProtectionRawDenial' => true,
 					'CrawlerProtectionRawDenialHeader' => '',
 					'CrawlerProtectionRawDenialText' => '',
 				] )
@@ -86,6 +86,35 @@ public function testDenyAccessChooses418WhenConfigured() {
 		$factory->denyAccess( $output );
 	}
 
+	/**
+	 * When RawDenial is false, Use418 should be a no-op and the factory
+	 * must fall through to the pretty 403 page.
+	 *
+	 * @covers ::denyAccess
+	 */
+	public function testDenyAccessIgnoresUse418WhenRawDenialDisabled() {
+		$factory = $this->getMockBuilder( ResponseFactory::class )
+			->setConstructorArgs( [
+				new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [
+					'CrawlerProtectionUse418' => true,
+					'CrawlerProtectionRawDenial' => false,
+					'CrawlerProtectionRawDenialHeader' => '',
+					'CrawlerProtectionRawDenialText' => '',
+				] )
+			] )
+			->onlyMethods( [ 'denyAccessPretty', 'denyAccessWith418' ] )
+			->getMock();
+
+		$factory->expects( $this->never() )->method( 'denyAccessWith418' );
+
+		$output = $this->createMock( self::$outputPageClassName );
+		$factory->expects( $this->once() )
+			->method( 'denyAccessPretty' )
+			->with( $output );
+
+		$factory->denyAccess( $output );
+	}
+
 	/**
 	 * @covers ::denyAccess
 	 */