From 0768c79459149422c69cc44ac4403e64e4857da4 Mon Sep 17 00:00:00 2001 From: Sam Stevens Date: Fri, 24 Mar 2017 21:27:10 +0000 Subject: [PATCH 1/3] implement custom callbacks and refactor the user setting code into a callback --- BugsnagBundle.php | 14 +++ Callbacks/UserSettingCallback.php | 75 ++++++++++++++ DependencyInjection/ClientFactory.php | 98 +++---------------- .../Compiler/CallbackRegisteringPass.php | 41 ++++++++ Resources/config/services.yml | 12 ++- Tests/Callbacks/UserSettingCallbackTest.php | 90 +++++++++++++++++ .../Compiler/CallbackRegisteringPassTest.php | 38 +++++++ example/symfony27/app/config/services.yml | 8 +- .../AppBundle/Bugsnag/MetadataCallback.php | 21 ++++ example/symfony28/app/config/services.yml | 8 +- .../AppBundle/Bugsnag/MetadataCallback.php | 21 ++++ example/symfony31/app/config/services.yml | 8 +- .../AppBundle/Bugsnag/MetadataCallback.php | 21 ++++ 13 files changed, 361 insertions(+), 94 deletions(-) create mode 100644 Callbacks/UserSettingCallback.php create mode 100644 DependencyInjection/Compiler/CallbackRegisteringPass.php create mode 100644 Tests/Callbacks/UserSettingCallbackTest.php create mode 100644 Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php create mode 100644 example/symfony27/src/AppBundle/Bugsnag/MetadataCallback.php create mode 100644 example/symfony28/src/AppBundle/Bugsnag/MetadataCallback.php create mode 100644 example/symfony31/src/AppBundle/Bugsnag/MetadataCallback.php diff --git a/BugsnagBundle.php b/BugsnagBundle.php index d4956f6..00bb0a2 100644 --- a/BugsnagBundle.php +++ b/BugsnagBundle.php @@ -2,6 +2,8 @@ namespace Bugsnag\BugsnagBundle; +use Bugsnag\BugsnagBundle\DependencyInjection\Compiler\CallbackRegisteringPass; +use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\HttpKernel\Bundle\Bundle; class BugsnagBundle extends Bundle @@ -12,4 +14,16 @@ class BugsnagBundle extends Bundle * @return string */ const VERSION = '1.0.0'; + + /** + * {@inheritdoc} + * + * @param \Symfony\Component\DependencyInjection\ContainerBuilder $container + * + * @return void + */ + public function build(ContainerBuilder $container) + { + $container->addCompilerPass(new CallbackRegisteringPass()); + } } diff --git a/Callbacks/UserSettingCallback.php b/Callbacks/UserSettingCallback.php new file mode 100644 index 0000000..f01742c --- /dev/null +++ b/Callbacks/UserSettingCallback.php @@ -0,0 +1,75 @@ +tokens = $tokens; + $this->checker = $checker; + $this->setUser = $setUser; + } + + /** + * @param \Bugsnag\Report $report + * + * @return void + */ + public function registerCallback(Report $report) + { + // If told to not set the user, or the security services were not passed in + // (not registered in the container), then exit early + if (!$this->setUser || is_null($this->tokens) || is_null($this->checker)) { + return; + } + + $token = $this->tokens->getToken(); + + if (!$token || !$this->checker->isGranted('IS_AUTHENTICATED_REMEMBERED')) { + return; + } + + $user = $token->getUser(); + + if ($user instanceof UserInterface) { + $bugsnagUser = ['id' => $user->getUsername()]; + } else { + $bugsnagUser = ['id' => (string) $user]; + } + + $report->setUser($bugsnagUser); + } +} diff --git a/DependencyInjection/ClientFactory.php b/DependencyInjection/ClientFactory.php index 6e236b7..253618f 100644 --- a/DependencyInjection/ClientFactory.php +++ b/DependencyInjection/ClientFactory.php @@ -4,12 +4,8 @@ use Bugsnag\BugsnagBundle\BugsnagBundle; use Bugsnag\BugsnagBundle\Request\SymfonyResolver; -use Bugsnag\Callbacks\CustomUser; use Bugsnag\Client; use Bugsnag\Configuration as Config; -use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; -use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; -use Symfony\Component\Security\Core\User\UserInterface; class ClientFactory { @@ -20,20 +16,6 @@ class ClientFactory */ protected $resolver; - /** - * The token resolver. - * - * @var \Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface|null - */ - protected $tokens; - - /** - * The auth checker. - * - * @var \Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface|null - */ - protected $checker; - /** * The api key. * @@ -55,13 +37,6 @@ class ClientFactory */ protected $callbacks; - /** - * User detection enabled. - * - * @var bool - */ - protected $user; - /** * The type. * @@ -149,36 +124,30 @@ class ClientFactory /** * Create a new client factory instance. * - * @param \Bugsnag\BugsnagBundle\Request\SymfonyResolver $resolver - * @param \Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface|null $tokens - * @param \Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface|null $checker - * @param string|null $key - * @param string|null $endpoint - * @param bool $callbacks - * @param bool $user - * @param string|null $type - * @param string|null $version - * @param bool $batch - * @param string|null $hostname - * @param bool $code - * @param string|null $strip - * @param string|null $project - * @param string|null $root - * @param string|null $env - * @param string|null $stage - * @param string[]|null $stages - * @param string[]|null $filters + * @param \Bugsnag\BugsnagBundle\Request\SymfonyResolver $resolver + * @param string|null $key + * @param string|null $endpoint + * @param bool $callbacks + * @param string|null $type + * @param string|null $version + * @param bool $batch + * @param string|null $hostname + * @param bool $code + * @param string|null $strip + * @param string|null $project + * @param string|null $root + * @param string|null $env + * @param string|null $stage + * @param string[]|null $stages + * @param string[]|null $filters * * @return void */ public function __construct( SymfonyResolver $resolver, - TokenStorageInterface $tokens = null, - AuthorizationCheckerInterface $checker = null, $key = null, $endpoint = null, $callbacks = true, - $user = true, $type = null, $version = true, $batch = null, @@ -193,12 +162,9 @@ public function __construct( array $filters = null ) { $this->resolver = $resolver; - $this->tokens = $tokens; - $this->checker = $checker; $this->key = $key; $this->endpoint = $endpoint; $this->callbacks = $callbacks; - $this->user = $user; $this->type = $type; $this->version = $version; $this->batch = $batch; @@ -228,10 +194,6 @@ public function make() $client->registerDefaultCallbacks(); } - if ($this->tokens && $this->checker && $this->user) { - $this->setupUserDetection($client, $this->tokens, $this->checker); - } - $this->setupPaths($client, $this->strip, $this->project, $this->root); $client->setReleaseStage($this->stage ?: ($this->env === 'prod' ? 'production' : $this->env)); @@ -261,34 +223,6 @@ public function make() return $client; } - /** - * Setup user detection. - * - * @param \Bugsnag\Client $client - * @param \Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface $tokens - * @param \Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface $checker - * - * @return void - */ - protected function setupUserDetection(Client $client, TokenStorageInterface $tokens, AuthorizationCheckerInterface $checker) - { - $client->registerCallback(new CustomUser(function () use ($tokens, $checker) { - $token = $tokens->getToken(); - - if (!$token || !$checker->isGranted('IS_AUTHENTICATED_REMEMBERED')) { - return; - } - - $user = $token->getUser(); - - if ($user instanceof UserInterface) { - return ['id' => $user->getUsername()]; - } - - return ['id' => (string) $user]; - })); - } - /** * Setup the client paths. * diff --git a/DependencyInjection/Compiler/CallbackRegisteringPass.php b/DependencyInjection/Compiler/CallbackRegisteringPass.php new file mode 100644 index 0000000..5fea414 --- /dev/null +++ b/DependencyInjection/Compiler/CallbackRegisteringPass.php @@ -0,0 +1,41 @@ +has(self::BUGSNAG_SERVICE_NAME)) { + return; + } + + // Get the Bugsnag service + $bugsnag = $container->findDefinition(self::BUGSNAG_SERVICE_NAME); + + // Get all services tagged as a callback + $callbackServices = $container->findTaggedServiceIds(self::TAG_NAME); + + // Register each callback on the bugsnag service + foreach ($callbackServices as $id => $tags) { + foreach ($tags as $attributes) { + // Get the method name to call on the service from the tag definition, + // defaulting to registerCallback + $method = isset($attributes['method']) ? $attributes['method'] : 'registerCallback'; + $bugsnag->addMethodCall('registerCallback', [[new Reference($id), $method]]); + } + } + } +} diff --git a/Resources/config/services.yml b/Resources/config/services.yml index a17cde5..e7920cc 100644 --- a/Resources/config/services.yml +++ b/Resources/config/services.yml @@ -6,12 +6,9 @@ services: class: '%bugsnag.factory%' arguments: - '@bugsnag.resolver' - - '@?security.token_storage' - - '@?security.authorization_checker' - '%bugsnag.api_key%' - '%bugsnag.endpoint%' - '%bugsnag.callbacks%' - - '%bugsnag.user%' - '%bugsnag.app_type%' - '%bugsnag.app_version%' - '%bugsnag.batch_sending%' @@ -39,3 +36,12 @@ services: - { name: kernel.event_listener, event: kernel.request, method: onKernelRequest, priority: 256 } - { name: kernel.event_listener, event: kernel.exception, method: onKernelException, priority: 128 } - { name: kernel.event_listener, event: console.exception, method: onConsoleException, priority: 128 } + + bugsnag.callbacks.user_setting: + class: Bugsnag\BugsnagBundle\Callbacks\UserSettingCallback + arguments: + - '@?security.token_storage' + - '@?security.authorization_checker' + - '%bugsnag.user%' + tags: + - { name: 'bugsnag.callback' } diff --git a/Tests/Callbacks/UserSettingCallbackTest.php b/Tests/Callbacks/UserSettingCallbackTest.php new file mode 100644 index 0000000..19ddc51 --- /dev/null +++ b/Tests/Callbacks/UserSettingCallbackTest.php @@ -0,0 +1,90 @@ +getMockBuilder(TokenStorageInterface::class) + ->getMock(); + + $authorizationChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class) + ->getMock(); + + $reportMock = $this->getBugsnagReportMock(); + $reportMock + ->expects($this->never()) + ->method('setUser'); + + $callback = new UserSettingCallback($tokenStorageMock, $authorizationChecker, false); + $callback->registerCallback($reportMock); + } + + public function testUserNotSetWhenServicesNotPassed() + { + $reportMock = $this->getBugsnagReportMock(); + $reportMock + ->expects($this->never()) + ->method('setUser'); + + $callback = new UserSettingCallback(null, null, true); + $callback->registerCallback($reportMock); + } + + public function testUserIsSetWhenLoggedIn() + { + $user = new User('example', 'example'); + + $tokenMock = $this->getMockBuilder(TokenInterface::class) + ->getMock(); + + $tokenMock + ->expects($this->once()) + ->method('getUser') + ->willReturn($user); + + $tokenStorageMock = $this->getMockBuilder(TokenStorageInterface::class) + ->getMock(); + + $tokenStorageMock + ->expects($this->once()) + ->method('getToken') + ->willReturn($tokenMock); + + $authorizationChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class) + ->getMock(); + + $authorizationChecker + ->expects($this->once()) + ->method('isGranted') + ->with('IS_AUTHENTICATED_REMEMBERED') + ->willReturn(true); + + $reportMock = $this->getBugsnagReportMock(); + $reportMock + ->expects($this->once()) + ->method('setUser') + ->with([ + 'id' => $user->getUsername(), + ]); + + $callback = new UserSettingCallback($tokenStorageMock, $authorizationChecker, true); + $callback->registerCallback($reportMock); + } + + private function getBugsnagReportMock() + { + return $this->getMockBuilder(Report::class) + ->disableOriginalConstructor() + ->getMock(); + } +} diff --git a/Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php b/Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php new file mode 100644 index 0000000..e0b840c --- /dev/null +++ b/Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php @@ -0,0 +1,38 @@ +setDefinition(CallbackRegisteringPass::BUGSNAG_SERVICE_NAME, $bugsnag); + + $taggedCallbackOne = new Definition(); + $taggedCallbackOne->addTag(CallbackRegisteringPass::TAG_NAME); + $containerBuilder->setDefinition('callback_1', $taggedCallbackOne); + + $taggedCallbackTwo = new Definition(); + $taggedCallbackTwo->addTag(CallbackRegisteringPass::TAG_NAME, ['method' => 'customMethod']); + $containerBuilder->setDefinition('callback_2', $taggedCallbackTwo); + + $pass = new CallbackRegisteringPass(); + $pass->process($containerBuilder); + + $this->assertSame(2, count($bugsnag->getMethodCalls())); + $this->assertSame('registerCallback', $bugsnag->getMethodCalls()[0][0]); + $this->assertEquals([new Reference('callback_1'), 'registerCallback'], $bugsnag->getMethodCalls()[0][1][0]); + $this->assertSame('registerCallback', $bugsnag->getMethodCalls()[1][0]); + $this->assertEquals([new Reference('callback_2'), 'customMethod'], $bugsnag->getMethodCalls()[1][1][0]); + } +} diff --git a/example/symfony27/app/config/services.yml b/example/symfony27/app/config/services.yml index 5c76fc5..2fc0933 100755 --- a/example/symfony27/app/config/services.yml +++ b/example/symfony27/app/config/services.yml @@ -4,6 +4,8 @@ parameters: # parameter_name: value services: -# service_name: -# class: AppBundle\Directory\ClassName -# arguments: ["@another_service_name", "plain_value", "%parameter_name%"] + + bugsnag_metadata_callback: + class: AppBundle\Bugsnag\MetadataCallback + tags: + - { name: 'bugsnag.callback' } diff --git a/example/symfony27/src/AppBundle/Bugsnag/MetadataCallback.php b/example/symfony27/src/AppBundle/Bugsnag/MetadataCallback.php new file mode 100644 index 0000000..b95d90a --- /dev/null +++ b/example/symfony27/src/AppBundle/Bugsnag/MetadataCallback.php @@ -0,0 +1,21 @@ +setMetaData([ + 'custom_data_1' => 'custom_value_1', + 'custom_data_2' => 'custom_value_2', + ]); + } +} diff --git a/example/symfony28/app/config/services.yml b/example/symfony28/app/config/services.yml index 5c76fc5..2fc0933 100755 --- a/example/symfony28/app/config/services.yml +++ b/example/symfony28/app/config/services.yml @@ -4,6 +4,8 @@ parameters: # parameter_name: value services: -# service_name: -# class: AppBundle\Directory\ClassName -# arguments: ["@another_service_name", "plain_value", "%parameter_name%"] + + bugsnag_metadata_callback: + class: AppBundle\Bugsnag\MetadataCallback + tags: + - { name: 'bugsnag.callback' } diff --git a/example/symfony28/src/AppBundle/Bugsnag/MetadataCallback.php b/example/symfony28/src/AppBundle/Bugsnag/MetadataCallback.php new file mode 100644 index 0000000..b95d90a --- /dev/null +++ b/example/symfony28/src/AppBundle/Bugsnag/MetadataCallback.php @@ -0,0 +1,21 @@ +setMetaData([ + 'custom_data_1' => 'custom_value_1', + 'custom_data_2' => 'custom_value_2', + ]); + } +} diff --git a/example/symfony31/app/config/services.yml b/example/symfony31/app/config/services.yml index 5c76fc5..2fc0933 100755 --- a/example/symfony31/app/config/services.yml +++ b/example/symfony31/app/config/services.yml @@ -4,6 +4,8 @@ parameters: # parameter_name: value services: -# service_name: -# class: AppBundle\Directory\ClassName -# arguments: ["@another_service_name", "plain_value", "%parameter_name%"] + + bugsnag_metadata_callback: + class: AppBundle\Bugsnag\MetadataCallback + tags: + - { name: 'bugsnag.callback' } diff --git a/example/symfony31/src/AppBundle/Bugsnag/MetadataCallback.php b/example/symfony31/src/AppBundle/Bugsnag/MetadataCallback.php new file mode 100644 index 0000000..b95d90a --- /dev/null +++ b/example/symfony31/src/AppBundle/Bugsnag/MetadataCallback.php @@ -0,0 +1,21 @@ +setMetaData([ + 'custom_data_1' => 'custom_value_1', + 'custom_data_2' => 'custom_value_2', + ]); + } +} From fc6bf1edd2a97254c315bc8350829bd68474abae Mon Sep 17 00:00:00 2001 From: Sam Stevens Date: Thu, 15 Jun 2017 12:06:50 +0100 Subject: [PATCH 2/3] use legacy phpunit test classes for php 5.5.9 --- Tests/Callbacks/UserSettingCallbackTest.php | 2 +- .../Compiler/CallbackRegisteringPassTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Callbacks/UserSettingCallbackTest.php b/Tests/Callbacks/UserSettingCallbackTest.php index 19ddc51..426cd3b 100644 --- a/Tests/Callbacks/UserSettingCallbackTest.php +++ b/Tests/Callbacks/UserSettingCallbackTest.php @@ -4,7 +4,7 @@ use Bugsnag\BugsnagBundle\Callbacks\UserSettingCallback; use Bugsnag\Report; -use PHPUnit\Framework\TestCase; +use PHPUnit_Framework_TestCase as TestCase; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; diff --git a/Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php b/Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php index e0b840c..2b1f4da 100644 --- a/Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php +++ b/Tests/DependencyInjection/Compiler/CallbackRegisteringPassTest.php @@ -4,7 +4,7 @@ use Bugsnag\BugsnagBundle\DependencyInjection\Compiler\CallbackRegisteringPass; use Bugsnag\Client; -use PHPUnit\Framework\TestCase; +use PHPUnit_Framework_TestCase as TestCase; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Reference; From 849a2afc6ab17f7bcae2e2d8db4fbda344a905c0 Mon Sep 17 00:00:00 2001 From: Sam Stevens Date: Thu, 15 Jun 2017 12:46:15 +0100 Subject: [PATCH 3/3] CS requests --- BugsnagBundle.php | 2 +- Callbacks/UserSettingCallback.php | 5 +++++ example/symfony27/app/config/services.yml | 3 +-- example/symfony28/app/config/services.yml | 1 - example/symfony31/app/config/services.yml | 1 - 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/BugsnagBundle.php b/BugsnagBundle.php index 00bb0a2..6b93e1c 100644 --- a/BugsnagBundle.php +++ b/BugsnagBundle.php @@ -16,7 +16,7 @@ class BugsnagBundle extends Bundle const VERSION = '1.0.0'; /** - * {@inheritdoc} + * Setup the callback registering pass. * * @param \Symfony\Component\DependencyInjection\ContainerBuilder $container * diff --git a/Callbacks/UserSettingCallback.php b/Callbacks/UserSettingCallback.php index f01742c..d2c9db0 100644 --- a/Callbacks/UserSettingCallback.php +++ b/Callbacks/UserSettingCallback.php @@ -32,6 +32,8 @@ class UserSettingCallback * @param null|\Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface $tokens * @param null|\Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface $checker * @param bool $setUser + * + * @return void */ public function __construct( TokenStorageInterface $tokens = null, @@ -44,6 +46,9 @@ public function __construct( } /** + * Define a callback to set the currently authenticated user as the user + * on any Bugsnag reports that are sent. + * * @param \Bugsnag\Report $report * * @return void diff --git a/example/symfony27/app/config/services.yml b/example/symfony27/app/config/services.yml index 2fc0933..31f6aee 100755 --- a/example/symfony27/app/config/services.yml +++ b/example/symfony27/app/config/services.yml @@ -4,8 +4,7 @@ parameters: # parameter_name: value services: - bugsnag_metadata_callback: class: AppBundle\Bugsnag\MetadataCallback tags: - - { name: 'bugsnag.callback' } + - { name: 'bugsnag.callback' } \ No newline at end of file diff --git a/example/symfony28/app/config/services.yml b/example/symfony28/app/config/services.yml index 2fc0933..9d1c70f 100755 --- a/example/symfony28/app/config/services.yml +++ b/example/symfony28/app/config/services.yml @@ -4,7 +4,6 @@ parameters: # parameter_name: value services: - bugsnag_metadata_callback: class: AppBundle\Bugsnag\MetadataCallback tags: diff --git a/example/symfony31/app/config/services.yml b/example/symfony31/app/config/services.yml index 2fc0933..9d1c70f 100755 --- a/example/symfony31/app/config/services.yml +++ b/example/symfony31/app/config/services.yml @@ -4,7 +4,6 @@ parameters: # parameter_name: value services: - bugsnag_metadata_callback: class: AppBundle\Bugsnag\MetadataCallback tags: