Do not throw an exception when the samlAuthenticationLogger is used before forAuthentication() is called on it

[pmeulen]

Do not throw an exception when the samlAuthenticationLogger is used before forAuthentication() is called on it.

• This obscures the original log message.
• Exception from Logger are unexpected behaviour.

I've seen this error occur both in selfservice and the gateway, so this does happen in practice.
I changed the behaviour so that the sari is not added to the logging context if it is not set (i.e. when forAuthentication() has not been called ).

I do not understand why this check was added in the first place. There was a test for it too. Please review if I'm missing something here.

[pmeulen]

Rebased on top of #131 to make the integration tests pass

[pablothedude]

This will indeed prevent an exception from being raised, but if this logger is used, it should use forAuthentication first.

If something needs to be logged in situations where we don't have a requestID, another logger should be used.

So I suspect this has something to do with the wrong logger being used after the Symfony upgrade.

It would be safe to merge, but then the sari in the logs might disappear in certain cases were a sari would be helpful.

[pmeulen]

Agree with everything you said, except:

... but then the sari in the logs might disappear in certain cases were a sari would be helpful.

This patch can never make a sari disappear, all it does is not throw when the sari is not there.

E.g. when expectedInResponseTo is null here, for instance because of session troubles,

Stepup-saml-bundle/src/Security/Authentication/Handler/ProcessSamlAuthenticationHandler.php

Line 44 in 61eeee5

$logger = $this->authenticationLogger->forAuthentication($expectedInResponseTo);

the later calls to the logger will throw an exception, hiding the original problem.

Or this could happen because the logger is used before forAuthentication() is called like you said.

These issues in the use of this logger should be addresses to, but the choice to make a log action throw hides valuable info, and does not help at all. I ran into this while debugging such an issue.

[pmeulen]

Discussed with @pablothedude : The point is that without the exception a developer can use this class wrongly (i.e DI'ing it without calling forAuthentication()) and not notice this.