I don’t think this makes sense since only authenticated tokens (aka tokens with a user) are serialized into the session, meaning refreshUser should always retrieve a user from the token.

I think this should be covered, the added test shows that the code can fail with a token without a user. Also the issue author gives a reproducer. I suggest adding this check, so even in the case of an unlikely event of this happening, the component code doesn't crash.

A token is always considered authenticated and as such should hold a user IMHO. So we may rather want to enforce this e.g. by throwing an exception in case of null user. Pinging @wouterj and @Spomky for thoughts

/cc @jorrit WDYT?

I thought I was replying on the proposed code change.

Anyway, returning null is best for me as the code is not part of a path that I control. Otherwise, an exception handler in the stack until refreshUser needs to be added or used for this purpose.

This is the stack trace until ContextListener.refreshUser(). In Symfony 6.4, there seem to be no appropriate exception handlers in this stack.

If returning null is not acceptable, I could just try to return a user with a different identifier to get the behavior I want. I just need a way to deauthenticate a user automatically when its underlying token expires.

Listening on CheckPassportEvent to throw a BadCredentialsException when $event->getPassport->getAttribute(OidcToken::AUTH_DATA_ATTR) is expired should do the job.

Couldn’t this be implemented using the underlying library token constraints? https://github.com/Drenso/symfony-oidc?tab=readme-ov-file#additional-token-claim-validation

Looks like there are many alternatives to achieve what's needed. Considering this and PostAuthenticationToken expecting a User instance as constructor argument, I'd say there's nothing to do.

An exception could still be appropriate to inform the developer they made something unexpected?
Like an UnexpectedValueException

The "getUser" method of a token stored in session must not return "null".

?

Looks like there are many alternatives to achieve what's needed

Should a defensive mechanism still be added? In the current state, the underlying code and logic can fail and lead to an unmanaged PHP error

I agree that this would be better than running into a PHP error.

Fair, PR welcome 👍

Nice, I'll take care of it 🙂