Skip to content

[Security] Revert "AbstractAuthenticationListener.php error instead info" #34957

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

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

MolloKhan
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #34836
License MIT

This reverts commit 867eb78.

After that change, every failed login attempt is logged as an error. It's flooding our errors log channel

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the new error() level has a very practical negative effect (all login attempts are going into high priority log handlers, commonly Slack) and there is no easy way to work around this (that I know of), I think we should merge this and revisit how this could be done in a more flexible way. I get why it was done, but our logs are going crazy :)

Note: should be merged into 4.4

@nicolas-grekas nicolas-grekas changed the title [SECURITY] Revert "AbstractAuthenticationListener.php error instead i… [Security] Revert "AbstractAuthenticationListener.php error instead i… Dec 12, 2019
@nicolas-grekas nicolas-grekas changed the title [Security] Revert "AbstractAuthenticationListener.php error instead i… [Security] Revert "AbstractAuthenticationListener.php error instead info. Dec 12, 2019
@nicolas-grekas nicolas-grekas changed the title [Security] Revert "AbstractAuthenticationListener.php error instead info. [Security] Revert "AbstractAuthenticationListener.php error instead info Dec 12, 2019
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Dec 12, 2019
@nicolas-grekas nicolas-grekas changed the title [Security] Revert "AbstractAuthenticationListener.php error instead info [Security] Revert "AbstractAuthenticationListener.php error instead info" Dec 12, 2019
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, for 4.4

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 December 13, 2019 12:11
@nicolas-grekas
Copy link
Member

Thank you @larzuk91.

nicolas-grekas added a commit that referenced this pull request Dec 13, 2019
…r instead info" (larzuk91)

This PR was submitted for the master branch but it was merged into the 4.4 branch instead.

Discussion
----------

[Security] Revert "AbstractAuthenticationListener.php error instead info"

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34836
| License       | MIT

This reverts commit 867eb78.

After that change, every failed login attempt is logged as an error. It's flooding our errors log channel

Commits
-------

9d306f9 [SECURITY] Revert "AbstractAuthenticationListener.php error instead info. Rebase of #28462"
@nicolas-grekas nicolas-grekas merged commit 9d306f9 into symfony:4.4 Dec 13, 2019
@MolloKhan MolloKhan deleted the fix-34836 branch December 14, 2019 02:19
This was referenced Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants