From 9315ece0181babaf893e764b6c28a2783b7e85e8 Mon Sep 17 00:00:00 2001 From: Paolo Cuffiani Date: Wed, 31 Aug 2022 15:53:46 +0200 Subject: [PATCH] fix: restore correct behavior of `AwsS3CloudFrontAdapter`, which was never invalidating anything --- .../Adapter/AwsS3CloudFrontAdapter.php | 56 +++++++++------ src/Filesystem/Adapter/S3Adapter.php | 4 +- src/Mailer/Transport/SesTransport.php | 12 ++-- src/Mailer/Transport/SnsTransport.php | 21 +++--- src/Plugin.php | 4 +- .../Authenticator/AlbAuthenticatorTest.php | 9 +-- .../Adapter/AwsS3CloudFrontAdapterTest.php | 70 +++---------------- .../Mailer/Transport/SnsTransportTest.php | 8 +-- tests/TestCase/PluginTest.php | 6 +- 9 files changed, 73 insertions(+), 117 deletions(-) diff --git a/src/Filesystem/Adapter/AwsS3CloudFrontAdapter.php b/src/Filesystem/Adapter/AwsS3CloudFrontAdapter.php index 0c82abd..6c5fe68 100644 --- a/src/Filesystem/Adapter/AwsS3CloudFrontAdapter.php +++ b/src/Filesystem/Adapter/AwsS3CloudFrontAdapter.php @@ -32,7 +32,21 @@ class AwsS3CloudFrontAdapter extends AwsS3V3Adapter * * @var \Aws\CloudFront\CloudFrontClient|null */ - protected $cloudfrontClient = null; + protected ?CloudFrontClient $cloudfrontClient = null; + + /** + * CloudFront distribution ID. + * + * @var string|null + */ + protected ?string $distributionId = null; + + /** + * CloudFront distribution path prefix. + * + * @var string|null + */ + protected ?string $cloudfrontPathPrefix = null; /** * Adapter constructor. @@ -44,7 +58,7 @@ class AwsS3CloudFrontAdapter extends AwsS3V3Adapter * @param bool $streamReads Whether reads should be streamed. * @param \Aws\CloudFront\CloudFrontClient|null $cloudfrontClient CloudFront client instance, or `null`. */ - public function __construct(S3ClientInterface $client, $bucket, $prefix = '', array $options = [], $streamReads = true, ?CloudFrontClient $cloudfrontClient = null) + public function __construct(S3ClientInterface $client, string $bucket, string $prefix = '', array $options = [], $streamReads = true, ?CloudFrontClient $cloudfrontClient = null) { parent::__construct($client, $bucket, $prefix, null, null, $options, $streamReads); @@ -52,6 +66,8 @@ public function __construct(S3ClientInterface $client, $bucket, $prefix = '', ar throw new DomainException('When `distributionId` is set, a CloudFront client instance is required'); } $this->cloudfrontClient = $cloudfrontClient; + $this->distributionId = $options['distributionId'] ?? null; + $this->cloudfrontPathPrefix = $options['cloudFrontPathPrefix'] ?? null; } /** @@ -69,12 +85,10 @@ public function getCloudFrontClient(): ?CloudFrontClient * * @return string|null */ - // TODO: `options` attribute is now private in base class, - // and this method doesn't work anymore - see if there's a different way to obtain this ID. - // public function getDistributionId(): ?string - // { - // return $this->options['distributionId'] ?? null; - // } + public function getDistributionId(): ?string + { + return $this->distributionId; + } /** * Check whether CloudFront configuration is set. @@ -89,12 +103,12 @@ public function hasCloudFrontConfig(): bool /** * @inheritDoc */ - public function copy(string $path, string $newpath, Config $config): void + public function copy(string $source, string $destination, Config $config): void { - $existed = $this->hasCloudFrontConfig() && $this->fileExists($newpath); - parent::copy($path, $newpath, $config); + $existed = $this->hasCloudFrontConfig() && $this->fileExists($destination); + parent::copy($source, $destination, $config); if ($existed) { - $this->createCloudFrontInvalidation($newpath); + $this->createCloudFrontInvalidation($destination); } } @@ -113,19 +127,19 @@ public function delete(string $path): void /** * @inheritDoc */ - public function deleteDirectory(string $dirname): void + public function deleteDirectory(string $path): void { - parent::deleteDirectory($dirname); - $this->createCloudFrontInvalidation(rtrim($dirname, '/') . '/*'); + parent::deleteDirectory($path); + $this->createCloudFrontInvalidation(rtrim($path, '/') . '/*'); } /** * @inheritDoc */ - public function write(string $path, string $body, Config $config): void + public function write(string $path, string $contents, Config $config): void { $existed = $this->hasCloudFrontConfig() && $this->fileExists($path); - parent::write($path, $body, $config); + parent::write($path, $contents, $config); if ($existed) { $this->createCloudFrontInvalidation($path); } @@ -140,11 +154,11 @@ public function write(string $path, string $body, Config $config): void protected function applyCloudFrontPathPrefix(string $path): string { $path = '/' . ltrim($path, '/'); - if (empty($this->options['cloudFrontPathPrefix'])) { + if (empty($this->cloudfrontPathPrefix)) { return $path; } - return '/' . trim($this->options['cloudFrontPathPrefix'], '/') . $path; + return '/' . trim($this->cloudfrontPathPrefix, '/') . $path; } /** @@ -155,13 +169,13 @@ protected function applyCloudFrontPathPrefix(string $path): string */ protected function createCloudFrontInvalidation(string $path): void { - if ($this->cloudfrontClient === null || empty($this->options['distributionId'])) { + if ($this->cloudfrontClient === null || empty($this->distributionId)) { return; } try { $this->cloudfrontClient->createInvalidation([ - 'DistributionId' => $this->options['distributionId'], + 'DistributionId' => $this->distributionId, 'InvalidationBatch' => [ 'CallerReference' => uniqid($path), 'Paths' => [ diff --git a/src/Filesystem/Adapter/S3Adapter.php b/src/Filesystem/Adapter/S3Adapter.php index 02f6597..359683d 100644 --- a/src/Filesystem/Adapter/S3Adapter.php +++ b/src/Filesystem/Adapter/S3Adapter.php @@ -47,14 +47,14 @@ class S3Adapter extends FilesystemAdapter * * @var \Aws\S3\S3Client|null */ - protected $client; + protected ?S3Client $client; /** * AWS CloudFront client. * * @var \Aws\CloudFront\CloudFrontClient|null */ - protected $cloudFrontClient; + protected ?CloudFrontClient $cloudFrontClient; /** * @inheritDoc diff --git a/src/Mailer/Transport/SesTransport.php b/src/Mailer/Transport/SesTransport.php index 4d3d769..ce372a1 100644 --- a/src/Mailer/Transport/SesTransport.php +++ b/src/Mailer/Transport/SesTransport.php @@ -49,7 +49,7 @@ class SesTransport extends AbstractTransport * * @var \Aws\Ses\SesClient|null */ - protected $client; + protected ?SesClient $client; /** * @inheritDoc @@ -78,19 +78,19 @@ protected function getClient(): SesClient /** * @inheritDoc */ - public function send(Message $email): array + public function send(Message $message): array { $headerList = ['from', 'sender', 'replyTo', 'readReceipt', 'returnPath', 'to', 'cc', 'bcc', 'subject']; - $headers = $email->getHeadersString($headerList, static::EOL); + $headers = $message->getHeadersString($headerList, static::EOL); - $message = $email->getBodyString(static::EOL); + $body = $message->getBodyString(static::EOL); $this->getClient()->sendRawEmail([ 'RawMessage' => [ - 'Data' => $headers . static::EOL . static::EOL . $message, + 'Data' => $headers . static::EOL . static::EOL . $body, ], ]); - return compact('headers', 'message'); + return ['headers' => $headers, 'message' => $body]; } } diff --git a/src/Mailer/Transport/SnsTransport.php b/src/Mailer/Transport/SnsTransport.php index 8a65720..337cc83 100644 --- a/src/Mailer/Transport/SnsTransport.php +++ b/src/Mailer/Transport/SnsTransport.php @@ -43,7 +43,7 @@ class SnsTransport extends AbstractTransport * * @var \Aws\Sns\SnsClient|null */ - protected $client; + protected ?SnsClient $client; /** * @inheritDoc @@ -70,21 +70,16 @@ protected function getClient(): SnsClient } /** - * Send mail - * - * @param \Cake\Mailer\Message $email Email message. - * @return array + * @inheritDoc */ - public function send(Message $email): array + public function send(Message $message): array { - $from = $email->getFrom(); - $to = $email->getTo(); + $from = $message->getFrom(); + $to = $message->getTo(); $phoneNumber = reset($to); $senderId = trim(reset($from)); - /** @var string $message */ - $message = $email->getBodyText(); - $message = trim($message); + $body = trim($message->getBodyText()); $smsType = $this->getConfig('smsType'); $attributes = []; @@ -102,11 +97,11 @@ public function send(Message $email): array } $this->getClient()->publish([ - 'Message' => $message, + 'Message' => $body, 'PhoneNumber' => $phoneNumber, 'MessageAttributes' => $attributes, ]); - return compact('message') + ['headers' => '']; + return ['headers' => '', 'message' => $body]; } } diff --git a/src/Plugin.php b/src/Plugin.php index 26c9ed3..be59e5a 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -21,7 +21,7 @@ use BEdita\Core\Filesystem\FilesystemRegistry; use Cake\Core\BasePlugin; use Cake\Core\PluginApplicationInterface; -use Cake\Mailer\Email; +use Cake\Mailer\Mailer; /** * Plugin class. @@ -36,7 +36,7 @@ public function bootstrap(PluginApplicationInterface $app): void parent::bootstrap($app); // Register SES (email) and SNS (SMS) transports. - Email::setDsnClassMap([ + Mailer::setDsnClassMap([ 'ses' => SesTransport::class, 'sns' => SnsTransport::class, ]); diff --git a/tests/TestCase/Authenticator/AlbAuthenticatorTest.php b/tests/TestCase/Authenticator/AlbAuthenticatorTest.php index 4c7e99b..c0c36e0 100644 --- a/tests/TestCase/Authenticator/AlbAuthenticatorTest.php +++ b/tests/TestCase/Authenticator/AlbAuthenticatorTest.php @@ -35,6 +35,7 @@ use Lcobucci\JWT\Encoding\JoseEncoder; use Lcobucci\JWT\Signer\Ecdsa\MultibyteStringConverter; use Lcobucci\JWT\Signer\Ecdsa\Sha256; +use Lcobucci\JWT\Signer\Key; use Lcobucci\JWT\Signer\Key\InMemory; use Lcobucci\JWT\Signer\None; use Lcobucci\JWT\Token\Builder; @@ -54,28 +55,28 @@ class AlbAuthenticatorTest extends TestCase * * @var string */ - protected $keyId; + protected string $keyId; /** * Private key material. * * @var \Lcobucci\JWT\Signer\Key */ - protected $privateKey; + protected Key $privateKey; /** * Requests history. * * @var array */ - protected $history = []; + protected array $history = []; /** * Guzzle HTTP handler. * * @var HandlerStack */ - protected $handler; + protected HandlerStack $handler; /** * @inheritDoc diff --git a/tests/TestCase/Filesystem/Adapter/AwsS3CloudFrontAdapterTest.php b/tests/TestCase/Filesystem/Adapter/AwsS3CloudFrontAdapterTest.php index fb6790b..46e24c8 100644 --- a/tests/TestCase/Filesystem/Adapter/AwsS3CloudFrontAdapterTest.php +++ b/tests/TestCase/Filesystem/Adapter/AwsS3CloudFrontAdapterTest.php @@ -22,7 +22,6 @@ use Aws\S3\S3Client; use BEdita\AWS\Filesystem\Adapter\AwsS3CloudFrontAdapter; use DomainException; -use GuzzleHttp\Psr7\Response; use InvalidArgumentException; use League\Flysystem\Config; use PHPUnit\Framework\TestCase; @@ -81,6 +80,7 @@ protected static function cloudFrontClientFactory(?callable $handler = null): Cl * @covers ::__construct() * @covers ::getCloudFrontClient() * @covers ::hasCloudFrontConfig() + * @covers ::getDistributionId() */ public function testConstruct(): void { @@ -88,8 +88,7 @@ public function testConstruct(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', '', [], true, null); static::assertNull($adapter->getCloudFrontClient()); - // `getDistributionId()` method removed for now - // static::assertNull($adapter->getDistributionId()); + static::assertNull($adapter->getDistributionId()); static::assertFalse($adapter->hasCloudFrontConfig()); } @@ -116,6 +115,7 @@ public function testConstructMissingCloudFrontClient(): void * @covers ::__construct() * @covers ::getCloudFrontClient() * @covers ::hasCloudFrontConfig() + * @covers ::getDistributionId() */ public function testConstructWithDistribution(): void { @@ -125,8 +125,7 @@ public function testConstructWithDistribution(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', '', compact('distributionId'), true, $cloudFrontClient); static::assertSame($cloudFrontClient, $adapter->getCloudFrontClient()); - // `getDistributionId()` method removed for now - //static::assertSame($distributionId, $adapter->getDistributionId()); + static::assertSame($distributionId, $adapter->getDistributionId()); static::assertTrue($adapter->hasCloudFrontConfig()); } @@ -194,12 +193,6 @@ public function testCopyCloudFrontNotExistingObject(): void throw new InvalidArgumentException(sprintf('Unexpected Key: %s', $command['Key'])); } - case 'ListObjects': - static::assertSame('example-bucket', $command['Bucket']); - static::assertSame('new.jpg/', $command['Prefix']); - - throw new S3Exception('', $command, ['response' => new Response(404)]); - case 'GetObjectAcl': return new Result([ 'Grants' => [['Grantee' => ['URI' => ''], 'Permission' => 'READ']], @@ -224,8 +217,6 @@ public function testCopyCloudFrontNotExistingObject(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', '', ['distributionId' => 'E2EXAMPLE'], true, $cloudFrontClient); $adapter->copy('old.jpg', 'new.jpg', new Config()); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['HeadObject', 'ListObjects', 'GetObjectAcl', 'HeadObject', 'CopyObject'], $invocations); static::assertSame(['HeadObject', 'GetObjectAcl', 'HeadObject', 'CopyObject'], $invocations); } @@ -248,7 +239,6 @@ public function testCopyCloudFrontExistingObject(): void static::assertSame('example-bucket', $command['Bucket']); switch ($command['Key']) { case 'old.jpg': - return new Result(['ContentLength' => 1]); case 'new.jpg': return new Result(['ContentLength' => 1]); default: @@ -288,9 +278,7 @@ public function testCopyCloudFrontExistingObject(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', '', ['distributionId' => 'E2EXAMPLE'], true, $cloudFrontClient); $adapter->copy('old.jpg', 'new.jpg', new Config()); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['HeadObject', 'GetObjectAcl', 'HeadObject', 'CopyObject', 'CreateInvalidation'], $invocations); - static::assertSame(['HeadObject', 'GetObjectAcl', 'HeadObject', 'CopyObject'], $invocations); + static::assertSame(['HeadObject', 'GetObjectAcl', 'HeadObject', 'CopyObject', 'CreateInvalidation'], $invocations); } /** @@ -311,18 +299,6 @@ public function testDelete(): void static::assertSame('foo/file.txt', $command['Key']); return new Result([]); - - case 'HeadObject': - static::assertSame('example-bucket', $command['Bucket']); - static::assertSame('foo/file.txt', $command['Key']); - - throw new S3Exception('', $command); - - case 'ListObjects': - static::assertSame('example-bucket', $command['Bucket']); - static::assertSame('foo/file.txt/', $command['Prefix']); - - throw new S3Exception('', $command, ['response' => new Response(404)]); } throw new InvalidArgumentException(sprintf('Unexpected command: %s', $name)); @@ -330,8 +306,6 @@ public function testDelete(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', 'foo/', [], true, null); $adapter->delete('file.txt'); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['DeleteObject', 'HeadObject', 'ListObjects'], $invocations); static::assertSame(['DeleteObject'], $invocations); } @@ -359,12 +333,6 @@ public function testDeleteCloudFrontNotExistingObject(): void static::assertSame('foo/file.txt', $command['Key']); throw new S3Exception('', $command); - - case 'ListObjects': - static::assertSame('example-bucket', $command['Bucket']); - static::assertSame('foo/file.txt/', $command['Prefix']); - - throw new S3Exception('', $command, ['response' => new Response(404)]); } throw new InvalidArgumentException(sprintf('Unexpected command: %s', $name)); @@ -378,8 +346,6 @@ public function testDeleteCloudFrontNotExistingObject(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', 'foo/', ['distributionId' => 'E2EXAMPLE', 'cloudFrontPathPrefix' => 'bar/'], true, $cloudFrontClient); $adapter->delete('file.txt'); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['HeadObject', 'ListObjects', 'DeleteObject', 'HeadObject', 'ListObjects'], $invocations); static::assertSame(['HeadObject', 'DeleteObject'], $invocations); } @@ -417,12 +383,6 @@ public function testDeleteCloudFrontExistingObject(): void } throw new S3Exception('', $command); - - case 'ListObjects': - static::assertSame('example-bucket', $command['Bucket']); - static::assertSame('foo/file.txt/', $command['Prefix']); - - throw new S3Exception('', $command, ['response' => new Response(404)]); } throw new InvalidArgumentException(sprintf('Unexpected command: %s', $name)); @@ -445,9 +405,7 @@ public function testDeleteCloudFrontExistingObject(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', 'foo/', ['distributionId' => 'E2EXAMPLE', 'cloudFrontPathPrefix' => 'bar/'], true, $cloudFrontClient); $adapter->delete('file.txt'); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['HeadObject', 'DeleteObject', 'HeadObject', 'ListObjects', 'CreateInvalidation'], $invocations); - static::assertSame(['HeadObject', 'DeleteObject'], $invocations); + static::assertSame(['HeadObject', 'DeleteObject', 'CreateInvalidation'], $invocations); } /** @@ -532,9 +490,7 @@ public function testDeleteDirCloudFront(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', 'foo/', ['distributionId' => 'E2EXAMPLE', 'cloudFrontPathPrefix' => 'bar/'], true, $cloudFrontClient); $adapter->deleteDirectory('my/sub/path'); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['ListObjects', 'DeleteObjects', 'CreateInvalidation'], $invocations); - static::assertSame(['ListObjects', 'DeleteObjects'], $invocations); + static::assertSame(['ListObjects', 'DeleteObjects', 'CreateInvalidation'], $invocations); } /** @@ -591,12 +547,6 @@ public function testWriteCloudFrontNotExistingObject(): void static::assertSame('foo/file.txt', $command['Key']); throw new S3Exception('', $command); - - case 'ListObjects': - static::assertSame('example-bucket', $command['Bucket']); - static::assertSame('foo/file.txt/', $command['Prefix']); - - throw new S3Exception('', $command, ['response' => new Response(404)]); } throw new InvalidArgumentException(sprintf('Unexpected command: %s', $name)); @@ -610,8 +560,6 @@ public function testWriteCloudFrontNotExistingObject(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', 'foo/', ['distributionId' => 'E2EXAMPLE', 'cloudFrontPathPrefix' => 'bar/'], true, $cloudFrontClient); $adapter->write('file.txt', 'data', new Config()); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['HeadObject', 'ListObjects', 'PutObject'], $invocations); static::assertSame(['HeadObject', 'PutObject'], $invocations); } @@ -670,8 +618,6 @@ public function testWriteCloudFrontExistingObject(): void $adapter = new AwsS3CloudFrontAdapter($s3Client, 'example-bucket', 'foo/', ['distributionId' => 'E2EXAMPLE', 'cloudFrontPathPrefix' => 'bar/'], true, $cloudFrontClient); $adapter->write('file.txt', 'data', new Config()); - // $invocations array now differs - is this a problem/bug? - // static::assertSame(['HeadObject', 'PutObject', 'CreateInvalidation'], $invocations); - static::assertSame(['HeadObject', 'PutObject'], $invocations); + static::assertSame(['HeadObject', 'PutObject', 'CreateInvalidation'], $invocations); } } diff --git a/tests/TestCase/Mailer/Transport/SnsTransportTest.php b/tests/TestCase/Mailer/Transport/SnsTransportTest.php index a52f31a..c8ebca7 100644 --- a/tests/TestCase/Mailer/Transport/SnsTransportTest.php +++ b/tests/TestCase/Mailer/Transport/SnsTransportTest.php @@ -84,7 +84,7 @@ public function sendProvider(): array { return [ 'simple' => [ - ['message' => 'Hello, world!', 'headers' => ''], + ['headers' => '', 'message' => 'Hello, world!'], [ 'Message' => 'Hello, world!', 'PhoneNumber' => '+1-202-555-0118', @@ -98,7 +98,7 @@ public function sendProvider(): array ->setBodyText('Hello, world!'), ], 'with sender' => [ - ['message' => 'Hello, world!', 'headers' => ''], + ['headers' => '', 'message' => 'Hello, world!'], [ 'Message' => 'Hello, world!', 'PhoneNumber' => '+1-202-555-0118', @@ -114,7 +114,7 @@ public function sendProvider(): array ->setBodyText('Hello, world!'), ], 'with SMS type' => [ - ['message' => 'Hello, world!', 'headers' => ''], + ['headers' => '', 'message' => 'Hello, world!'], [ 'Message' => 'Hello, world!', 'PhoneNumber' => '+1-202-555-0118', @@ -130,7 +130,7 @@ public function sendProvider(): array ->setBodyText('Hello, world!'), ], 'with sender and SMS type' => [ - ['message' => 'Hello, world!', 'headers' => ''], + ['headers' => '', 'message' => 'Hello, world!'], [ 'Message' => 'Hello, world!', 'PhoneNumber' => '+1-202-555-0118', diff --git a/tests/TestCase/PluginTest.php b/tests/TestCase/PluginTest.php index ec37c82..9e53114 100644 --- a/tests/TestCase/PluginTest.php +++ b/tests/TestCase/PluginTest.php @@ -34,7 +34,7 @@ class PluginTest extends TestCase * * @var \BEdita\AWS\Plugin */ - protected $plugin; + protected Plugin $plugin; /** * @inheritDoc @@ -55,9 +55,9 @@ protected function setUp(): void public function testBootstrap(): void { $app = new class (CONFIG) extends BaseApplication { - public function middleware(MiddlewareQueue $middleware): MiddlewareQueue + public function middleware(MiddlewareQueue $middlewareQueue): MiddlewareQueue { - return $middleware; + return $middlewareQueue; } };