Skip to content

[SECURITY] AbstractAuthenticationListener.php error instead info. Rebase of #28462 #31554

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
Jul 8, 2019

Conversation

berezuev
Copy link
Contributor

@berezuev berezuev commented May 20, 2019

Q A
Branch? 4.4
-- --
Bug fix? yes
New feature? no
BC breaks? no I think
Deprecations? no
Tests pass? yes
Fixed tickets ...
License MIT

Rebase of #28462. Origin description:

[2018-09-13 20:43:38] security.INFO: Authentication request failed. {"exception":"[object] (Symfony\\Component\\Security\\Core\\Exception\\AuthenticationServiceException(code: 0): An exception occurred while executing
 ...
 Doctrine\\DBAL\\Driver\\PDOException(code: 42S22): SQLSTATE[42S22]: Column not found: 1054 Unknown column 't0.phone' in 'field list' at

Definitely I think this is NOT info, but error.
And since it's info, it's not logged in production because of fingers_crossed with action_level: error - so to actually see the real error behind Authentication request could not be processed due to a system problem. I had to debug on production. Very bad practice IMHO.

berezuev added a commit to berezuev/symfony that referenced this pull request May 21, 2019
@nicolas-grekas
Copy link
Member

In #28462 (comment), @curry684 suggest to always use error. @linaori WDYT?

@nicolas-grekas nicolas-grekas added this to the next milestone May 21, 2019
@fabpot
Copy link
Member

fabpot commented Jul 8, 2019

IIUC, it should always be an error. If that's the case, @berezuev Can you make the change? And rebase on 4.4 where this would be merged?

@berezuev berezuev changed the base branch from master to 4.4 July 8, 2019 09:16
@berezuev
Copy link
Contributor Author

berezuev commented Jul 8, 2019

@fabpot I've done this change and set PR's destination to 4.4 branch.
Not sure about tests. It fails w/o any warnings or errors.

@fabpot
Copy link
Member

fabpot commented Jul 8, 2019

Thank you @berezuev.

@fabpot fabpot merged commit 867eb78 into symfony:4.4 Jul 8, 2019
fabpot added a commit that referenced this pull request Jul 8, 2019
…stead info. Rebase of #28462 (berezuev)

This PR was merged into the 4.4 branch.

Discussion
----------

[SECURITY] AbstractAuthenticationListener.php error instead info. Rebase of #28462

| Q             | A
| ------------- | ---
| Branch? | 4.4
| -- | --
| Bug fix? | yes
| New feature? | no
| BC breaks? | no I think
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | ...
| License | MIT

Rebase of #28462. Origin description:
> ```
> [2018-09-13 20:43:38] security.INFO: Authentication request failed. {"exception":"[object] (Symfony\\Component\\Security\\Core\\Exception\\AuthenticationServiceException(code: 0): An exception occurred while executing
>  ...
>  Doctrine\\DBAL\\Driver\\PDOException(code: 42S22): SQLSTATE[42S22]: Column not found: 1054 Unknown column 't0.phone' in 'field list' at
> ```
>
> Definitely I think this is NOT info, but error.
> And since it's info, it's not logged in production because of `fingers_crossed` with `action_level: error` - so to actually see the real error behind `Authentication request could not be processed due to a system problem.` I had to debug on production. Very bad practice IMHO.

Commits
-------

867eb78 [SECURITY] AbstractAuthenticationListener.php error instead info. Rebase of #28462
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
@nicolas-grekas
Copy link
Member

Change as been reverted in #34957

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.

5 participants