Please use a single PR per issue next time, easier to review.

Fixes themself are fine.

Status: reviewed

Actually it shouldn't be based onmaster as it's a bugfix. I'll see if I can rebase it on 3.4.

@chalasr I just discussed with@michaelcullum that I think we should consider merging it intomaster anyway despite being a bugfix, as older applications may actually be accidentally relying on the broken behavior.

So I'd prefer putting it intomaster instead and documenting it as a B/C breaking bugfix in the upgrade guide.

Let's add some notes in the CHANGELOG.md file of the component, and give some instructions in UPGRADE-4.3.md then

@dimabory can you pick that up?

@curry684,@nicolas-grekas I'm not sure about B/C breaking. Can you guide me on what exactly can be broken with this change?

The change may cause the AccessDeniedHandler to be executed now in cases where it wasn't before, and applications may depend on the previous erroneous behavior to some degree.

Technically everything that changes existing behavior is to be considered BC breakage, but in this case the old behavior was technically wrong and out of spec, hence why we're accepting the fix in a minor release, but not backporting it. But the PR should mention it in the upgrade guide.

Looking at the patch and the fixed ticket, I think we should merge in 3.4, every bugfix breaks someone's behavior.
Calling the access denied handler doesn't sound harmful, is it?

I'm mainly hesitant because it'll be a patch level change in low level security behavior. I'm hard pressed to find a harmful scenario but I prefer to err on the side of caution where access control is involved... 😉

So what's the base branch3.4 ormaster?

3.4.

Technically it's correct for a bugfix, I won't object to it and@chalasr is the boss of security component 😉

Thank you@dimabory.

Excuse me, guys, but I didn't updateCHANGELOG.md andUPGRADE.md (like mentioned@curry684).

@dimabory As this is a bug fix, there is no need to update the CHANGELOG/UPGRADE files.

@fabpot There probably is a need, see#31136

These changes make log with message "Access denied, the user is neither anonymous, nor remember-me."(https://github.com/chalasr/symfony/blob/fd1408b13869a381fbebf9b9967f7a80e8b141d3/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php#L137) lying, because token may be anonymous on!$this->authenticationTrustResolver->isFullFledged($token) (https://github.com/chalasr/symfony/blob/fd1408b13869a381fbebf9b9967f7a80e8b141d3/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php#L121).

Also with this PR symfony call access denied handler without any context, that response already generated for anonymous token.

In my case my handler doesn't expect calling for anonymous token and I got an infinite redirect.

I think it's BC break.

@andrew-demb fix is coming, see#31142