BUG: Authentication is broken with new Proxy Class Building

bwaidelich commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current Behavior

Authenicating (with a session bound auth token) leads to an exception

Serialization of 'Closure' is not allowed

Expected Behavior

no exception :)

Steps To Reproduce

1. Configure an authentication provider (e.g. using the `UsernamePassword` token) with an entry point

for example

Neos:
  Flow:
    security:
      authentication:
        providers:
          'Acme.SomePackage:Default':
            provider: PersistedUsernamePasswordProvider
            token: UsernamePassword
            entryPoint: WebRedirect
            entryPointOptions:
              routeValues:
                '@package': Acme.SomePackage
                '@controller': Authentication
                '@action': login

2. Authenticate

Alternatively, the Flow version can be upgraded in a running Neos installation

Environment

- Flow: 9.0-dev
- PHP: 8.2

Anything else?

Slack debugging session: https://neos-project.slack.com/archives/C3MCBK6S2/p1685021294539839

robertlemke commented 1 year ago

It's not directly to be reproduced like you mentioned, you first need to apply another patch. Here's what I did:

composer create-project neos/neos-base-distribution
modify composer.json to use 9.0.x-dev: "neos/flow-development-collection": "9.0.x-dev as 8.3.1"
composer update, check that Framework indeed contains 9.0.x-dev (6cfe7d8d14f304a1a8d3b65a93dd1ff154fb768d)
doctrine:migrate, create a user etc
open Neos in a browser

What I see then, using PHP 8.2, are a lot of errors like:

Deprecated: Creation of dynamic property Neos\Flow\ResourceManagement\Streams\ResourceStreamWrapper::$Flow_Injected_Properties is deprecated in /application/Data/Temporary/Development/SubContextBeach/SubContextInstance/Cache/Code/Flow_Object_Classes/Neos_Flow_ResourceManagement_Streams_ResourceStreamWrapper.php on line 567

To solve that, cherry-pick and merge conflicts of this PR by @kitsunet : https://github.com/neos/flow-development-collection/pull/3032

Then try to open the Neos backend via https://thedomain/neos – you will end up on this page:

The System.log contains the following error:

23-05-26 11:13:49 1276 CRITICAL Exception in line 72 of /application/Packages/Framework/Neos.Cache/Classes/Frontend/VariableFrontend.php: Serialization of 'Closure' is not allowed - See also: 20230526111347d6f749.txt

The stacktrace is:

Exception in line 72 of /application/Packages/Framework/Neos.Cache/Classes/Frontend/VariableFrontend.php: Serialization of 'Closure' is not allowed

15 serialize(array|3|)
14 Neos\Cache\Frontend\VariableFrontend::set("15368616-eac2-407a-8814-913b5cdf1dd0d1993ff050a2c957f03196d5a89730c9", array|3|, array|1|, 0)
13 Neos\Flow\Session\Session_Original::putData("Neos_Flow_Object_ObjectManager", array|3|)
12 Neos\Flow\Session\Session_Original::shutdownObject()
11 Neos\Flow\ObjectManagement\ObjectManager::callShutdownMethods(SplObjectStorage)
10 Neos\Flow\ObjectManagement\ObjectManager::Neos\Flow\ObjectManagement\{closure}()
9 Closure::__invoke()
8 Neos\Flow\Security\Context_Original::withoutAuthorizationChecks(Closure)
7 Neos\Flow\ObjectManagement\ObjectManager::shutdown("Runtime", "Neos\Flow\Core\Bootstrap::bootstrapShuttingDown")
6 call_user_func_array(array|2|, array|2|)
5 Neos\Flow\SignalSlot\Dispatcher::dispatch("Neos\Flow\Core\Bootstrap", "bootstrapShuttingDown", array|1|)
4 Neos\Flow\Core\Bootstrap::emitBootstrapShuttingDown("Runtime")
3 Neos\Flow\Core\Bootstrap::shutdown("Runtime")
2 Neos\Flow\Http\RequestHandler::handleRequest()
1 Neos\Flow\Core\Bootstrap::run()

robertlemke commented 1 year ago

Further hints to how to reproduce this:

if you already have an authenticated session cookie, this error might not appear in the logs anymore; use a fresh private browser window

In Slack, @bwaidelich mentioned the WebRedirect entry point as a possible cause for the serialization error. Comparing the proxy class of WebRedirect with Flow 8.3, I see that the __sleep() method is missing:

robertlemke commented 1 year ago

When I add the following code to WebRedirect, the error is gone:

public function __sleep(): array
    {
        return ['options'];
    }
}

The problem seems to be this:

proxy class building was recently optimized to not add support for "related entities" serialization into proxy classes, if the class doesn't have related entities
WebRedirect doesn't have related entities, so the code (sleep method) is not generated
however, WebRedirect uses the @Transient annotation, to make sure that the two properties are not serialized. The "related entities" code in proxy classes deals with such annotations

So, if that is the root problem, we have to decide how to solve this. We can either:

generate proxy classes / code if we detect a @Transient annotation – maximum backwards-compatibility
or we decide that developers should add a sleep method themselves to control which properties should be ignored during serialization – the cleanest approach

kitsunet commented 1 year ago

I am very much for having proxies if you used transient as this is mostly in relation to (session) serialisation so framework magic instead of userland necessities.

robertlemke commented 1 year ago

My opinion: If there was no legacy, I'd not introduce that magic and let devs use __sleep() as it was supposed to be used. However, as this can break existing code in an ugly way (see Basti's debug session), we rather should play safe and create the necessary proxy code.

kitsunet commented 1 year ago

devs totally can do __sleep as supposed to but in that case there is no need or use for transient annotations, so I guess that works out. I woudl rather suggest via future deprecation or at least documentation that we suggest you take care of this yourself instead of relying on the transient annotation?

kdambekalns commented 1 year ago

in that case there is no need or use for transient annotations

@Transient is about persistence, that it influences serialization is rather a sad side-effect… 🤷‍♂️

neos / flow-development-collection