New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Monolog Bridge][DX] Add a Monolog activation strategy for ignoring specific HTTP codes #23707

Merged
merged 2 commits into from Apr 4, 2018

Conversation

Projects
None yet
9 participants
@simshaun
Contributor

simshaun commented Jul 28, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #9712
License MIT
Doc PR symfony/symfony-docs#8235

This PR introduces a Monolog activation strategy that makes it easy to ignore specific HTTP codes. e.g. Stopping logs from being flooded with 403s, 404s, and 405s.

Relevant Symfony integration PR on symfony/monolog-bundle: symfony/monolog-bundle#221

@simshaun simshaun changed the title from Add a Monolog activation strategy for ignoring specific HTTP codes to [Monolog Bridge][DX] Add a Monolog activation strategy for ignoring specific HTTP codes Jul 28, 2017

@chalasr

This comment has been minimized.

Member

chalasr commented Jul 28, 2017

New features should target the 3.4 branch.

@simshaun simshaun changed the base branch from 2.7 to 3.4 Jul 28, 2017

@simshaun

This comment has been minimized.

Contributor

simshaun commented Jul 28, 2017

Sorry about that. Rebased to 3.4.

How do I get AppVeyor to run again? It says "continuous-integration/appveyor/pr — AppVeyor was unable to build non-mergeable "

@chalasr

This comment has been minimized.

Member

chalasr commented Jul 29, 2017

Seems like you changed the PR target but not correctly rebased to the 3.4 branch.

@chalasr chalasr added this to the 3.4 milestone Jul 29, 2017

@simshaun

This comment has been minimized.

Contributor

simshaun commented Jul 29, 2017

I think I fixed the rebase.

The Travis failure seems to be unrelated:

There was 1 failure:
1) Symfony\Component\Lock\Tests\Store\FlockStoreTest::testBlockingLocks
The store saves a locked key.
/home/travis/build/symfony/symfony/src/Symfony/Component/Lock/Tests/Store/BlockingStoreTestTrait.php:57
/home/travis/build/symfony/symfony/.phpunit/phpunit-6.0/phpunit:5
/home/travis/build/symfony/symfony/vendor/symfony/phpunit-bridge/bin/simple-phpunit:192
FAILURES!
Tests: 99, Assertions: 261, Failures: 1.
KO src/Symfony/Component/Lock
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Sep 26, 2017

Just wondering: couldn't/shouldn't this be submitted to monolog instead?

@simshaun

This comment has been minimized.

Contributor

simshaun commented Sep 26, 2017

Maybe. I did it here since the bridge contains the NotFoundActivationStrategy and this is similar.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Sep 26, 2017

OK my bad it's legit, Symfony namespace here :)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Sep 26, 2017

So, ping @lsmith77 I guess, original report of the feature request.

@fabpot

This comment has been minimized.

Member

fabpot commented Oct 1, 2017

Looks useful to me, but what about deprecating the NotFoundActivationStrategy in favor of this one, which is more generic (we should add support for the blacklist option though).

$isActivated
&& isset($record['context']['exception'])
&& $record['context']['exception'] instanceof HttpException
&& ($request = $this->requestStack->getMasterRequest())

This comment has been minimized.

@fabpot

fabpot Oct 1, 2017

Member

$request is not used

This comment has been minimized.

@Soullivaneuh

This comment has been minimized.

@simshaun

simshaun Feb 20, 2018

Contributor

It wasn't being used before. It is now with the addition of URL blacklisting.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 8, 2017

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

*
* @author Shaun Simmons <shaun@envysphere.com>
*/
class HttpCodeActivationStrategy extends ErrorLevelActivationStrategy

This comment has been minimized.

@pimolo

pimolo Dec 18, 2017

It should not extend ErrorLevelActivationStrategy but rather implement ActivationStrategyInterface, since this strategy doesn't deal with the error levels.

This comment has been minimized.

@Soullivaneuh

Soullivaneuh Feb 1, 2018

Contributor

Plus, this class could be final IMO.

This comment has been minimized.

@simshaun

simshaun Feb 1, 2018

Contributor

@pimolo The FingersCrossedHandler leaves it up to the activation strategy object to handle the action level. It defaults to the ErrorLevelActivationStrategy only when it isn't explicitly given an object implementing ActivationStrategyInterface. Because an HttpCodeActivationStrategy object is given to the FingersCrossedHandler when the excluded_http_codes configuration exists, the HttpCodeActivationStrategy becomes responsible for handling the action level, just like the existing NotFoundActivationStrategy does. So, that's why it's extending the ErrorLevelActivationStrategy class.

*
* @author Shaun Simmons <shaun@envysphere.com>
*/
class HttpCodeActivationStrategy extends ErrorLevelActivationStrategy

This comment has been minimized.

@Soullivaneuh

Soullivaneuh Feb 1, 2018

Contributor

Plus, this class could be final IMO.

@Soullivaneuh

This comment has been minimized.

Contributor

Soullivaneuh commented Feb 1, 2018

@nicolas-grekas What is the status of the PR?

@simshaun are you sill handling it? If not, I may rework it.

Looks useful to me, but what about deprecating the NotFoundActivationStrategy in favor of this one, which is more generic (we should add support for the blacklist option though).

@fabpot Makes sense to me. We could imagine something like that:

excluded_http_erros:
    403: ^/    # All 403 errors are excluded
    404: ^/admin # All 404 errors are excluded for /admin/*
    400: # Can be still multiple regex. 
        - ^/csrf/
        - ^/foo/bar/
@simshaun

This comment has been minimized.

Contributor

simshaun commented Feb 1, 2018

I'm working on adding the URL blacklist. @fabpot, does the configuration format @Soullivaneuh showed above look good?

One thing I'm thinking could be improved for user-friendliness: If you want a code to be excluded for all URLs, you just pass true instead of a ^/ regex.

excluded_http_codes:
    403: true           # All 403 errors are excluded
    404: ^/admin        # All 404 errors are excluded for /admin/*
    400:                # Can contain multiple regex
        - ^/csrf/
        - ^/foo/bar/
@Soullivaneuh

This comment has been minimized.

Contributor

Soullivaneuh commented Feb 2, 2018

@simshaun I was thinking about the true option but didn't propose that to simplify the work. But if you think it can be implemented easily then 👍. :-)

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Feb 9, 2018

Just asking: do we really want to allow excluding HTTP status per URL? If we don't do that, this would be really simple:

excluded_http_codes: [400, 403, 404]

If we really must support excluding per URL, do we really want to mix booleans and regexp strings? As proposed above, putting 403: ^/ would exclude 403 for all URLs, without the need to support, explain, learn and maintain forever the true value as an special option.


And now a proposal that mixes both ideas. What if we add support for URLs, but make them optional? Like this:

excluded_http_codes: [403, 404]

excluded_http_codes: [{ 404: '^/some/section' }]

excluded_http_codes: [403, 404, { 400: ['^/csrf', '^/foo/bar' ] }]
@simshaun

This comment has been minimized.

Contributor

simshaun commented Feb 9, 2018

I'm good with that too.

@simshaun

This comment has been minimized.

Contributor

simshaun commented Feb 18, 2018

I've updated this PR to support URL blacklists using the YAML format suggested by @javiereguiluz. The other PRs referenced in the OP (MonologBundle and docs) have been updated as well.

I really struggled with making the Configuration tree work with both YAML and XML. I ended up just normalizing the two into a common format that the new activation strategy understands.

It now supports:

YAML:

excluded_http_codes: [403, 404, { 400: ['^/foo', '^/bar'] }]

XML:

<monolog:excluded-http-code code="403" />
<monolog:excluded-http-code code="404" />
<monolog:excluded-http-code code="400">
  <monolog:url>^/foo</monolog:url>
  <monolog:url>^/bar</monolog:url>
</monolog:excluded-http-code>
@simshaun

This comment has been minimized.

Contributor

simshaun commented Feb 20, 2018

Looks like the Travis failure is unrelated. Lots of problems with FormExtensionBootstrap4HorizontalLayoutTest

@fabpot

fabpot approved these changes Apr 3, 2018

}
$urlBlacklist = null;
if (count($exclusion['url'])) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 3, 2018

Member

If ! empty(...) could prevent an issue when the url index is not defined.

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

Validation added in the constructor

&& ($request = $this->requestStack->getMasterRequest())
) {
foreach ($this->exclusions as $exclusion) {
if ($record['context']['exception']->getStatusCode() !== $exclusion['code']) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 3, 2018

Member

Would it be useful to ensure that the code key exists, and throw otherwise?

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

Done in the constructor

@fabpot fabpot changed the base branch from 3.4 to master Apr 4, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 4, 2018

Thank you @simshaun.

@fabpot fabpot merged commit 6fc1cc3 into symfony:master Apr 4, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 4, 2018

feature #23707 [Monolog Bridge][DX] Add a Monolog activation strategy…
… for ignoring specific HTTP codes (simshaun, fabpot)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Monolog Bridge][DX] Add a Monolog activation strategy for ignoring specific HTTP codes

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9712
| License       | MIT
| Doc PR        | symfony/symfony-docs#8235

This PR introduces a Monolog activation strategy that makes it easy to ignore specific HTTP codes. e.g. Stopping logs from being flooded with 403s, 404s, and 405s.

Relevant Symfony integration PR on symfony/monolog-bundle: symfony/monolog-bundle#221

Commits
-------

6fc1cc3 added some validation
c11a8eb Add a Monolog activation strategy for ignoring specific HTTP codes

fabpot added a commit to symfony/monolog-bundle that referenced this pull request Apr 4, 2018

feature #221 Add configuration for new HttpCodeActivationStrategy (si…
…mshaun, fabpot)

This PR was merged into the 3.x-dev branch.

Discussion
----------

Add configuration for new HttpCodeActivationStrategy

Dependent upon symfony/symfony#23707

This PR introduces configuration to integrate a new `HttpCodeActivationStrategy`, offering an easy way to ignore specific HTTP status codes.

Commits
-------

30a70a2 tweaked code
61a78a6 Add configuration for new HttpCodeActivationStrategy

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 15, 2018

minor #8235 Document new Monolog HTTP code exclusion feature (simshau…
…n, javiereguiluz)

This PR was merged into the 4.1 branch.

Discussion
----------

Document new Monolog HTTP code exclusion feature

I'm submitting PRs on symfony/symfony and symfony/monolog-bundle that introduces an easy way of excluding specific HTTP codes from Monolog.

This PR documents that new feature, pending its acceptance.

Pending PRs:
symfony/symfony#23707
symfony/monolog-bundle#221

Commits
-------

30cc7d4 Added the versionadded directive
c8c19f2 Update to Symfony 4 paths
e19bca2 Fix indentation
17cb416 Update configuration format to support URL blacklist
c9f6ee5 Fix title underline length
a3b6a01 Document new Monolog HTTP code exclusion feature
@fmonts

This comment has been minimized.

fmonts commented Jun 20, 2018

The feature is not working as expected.

I added excluded_http_codes: [403, 404]

and I still got:

[2018-06-20 12:18:05] security.INFO: Populated the TokenStorage with an anonymous Token. [] []
[2018-06-20 12:18:05] request.CRITICAL: Uncaught PHP Exception Symfony\Component\Security\Core\Exception\AccessDeniedException: "Access Denied." at /../vendor/symfony/security/Http/Firewall/AccessListener.php line 68 {"exception":"[object] (Symfony\Component\Security\Core\Exception\AccessDeniedException(code: 403): Access Denied. at /.../vendor/symfony/security/Http/Firewall/AccessListener.php:68)"} []
[2018-06-20 12:18:05] security.DEBUG: Access denied, the user is not fully authenticated; redirecting to authentication entry point. {"exception":"[object] (Symfony\Component\Security\Core\Exception\AccessDeniedException(code: 403): Access Denied. at /.../vendor/symfony/security/Http/Firewall/AccessListener.php:68)"} []
[2018-06-20 12:18:05] security.DEBUG: Calling Authentication entry point. [] []

@fmonts

This comment has been minimized.

fmonts commented Jun 20, 2018

...should I open a new issue?

@simshaun

This comment has been minimized.

Contributor

simshaun commented Jun 20, 2018

@fmonts A new issue would be better. Please @ me and I'll try to take a look.

Also, please include the context of your logging config. (I'm mostly just curious if you're using the fingers_crossed type.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment