Skip to content
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

BUG: Authentication is broken with new Proxy Class Building #3062

Closed
1 task done
bwaidelich opened this issue May 25, 2023 · 7 comments
Closed
1 task done

BUG: Authentication is broken with new Proxy Class Building #3062

bwaidelich opened this issue May 25, 2023 · 7 comments

Comments

@bwaidelich
Copy link
Member

Is there an existing issue for this?

  • 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

bwaidelich added a commit to neos/neos-development-distribution that referenced this issue May 25, 2023
Pins the version of Flow to commit ff724597bdf1832625c02aba317577324251a7a7 that contains a working version before the Proxy Class Refactoring.

*Note:* This is just a temporary work around until Flow 9.0 is stable again

Related: neos/flow-development-collection#3062
@robertlemke robertlemke self-assigned this May 26, 2023
@robertlemke
Copy link
Member

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 (6cfe7d8)
  • 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 : #3032

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

Image

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
Copy link
Member

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:

Image

@robertlemke
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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?

robertlemke added a commit to robertlemke/flow-development-collection that referenced this issue May 26, 2023
Due to a recent optimization, Flow was not generating `__sleep()` methods for classes which are not either entities or were configured with a session scope. This led to errors in classes which were using the `@Transient` annotation to exclude certain properties from serialization. Therefore, Flow now also generates proxy classes with `__sleep()` methods if the original class contains such annotations.

Resolves: neos#3062
@github-project-automation github-project-automation bot moved this from Developing to Closed in Flow 9.0 - Proxy Class Refactoring May 26, 2023
@robertlemke robertlemke moved this from Closed to Merged in Flow 9.0 - Proxy Class Refactoring May 26, 2023
@kdambekalns
Copy link
Member

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… 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

When branches are created from issues, their pull requests are automatically linked.

4 participants