Multiple matching authentication tokens not evaluated as documented

[kdambekalns]

Description

[Description of the bug or feature]

Steps to Reproduce

1. Install Flowpack.Neos.FrontendLogin
2. Create another package with a controller that should use the Neos.Neos:Backend provider. A "reproducer package" is provided at https://github.com/kdambekalns/marvin-test
3. Log in to the Neos backend
4. Call a frontend page that outputs the account identifier

Expected behavior

The authenticated backend account should be output.

Actual behavior

It is not. :)

Affected Versions

Flow: 4.3 - but this seems to be the case since the PR #130.

Details

In https://github.com/neos/flow-development-collection/pull/130/commits/adeca34c50f26c9130af429b27b67c8caa3ee145 the isTokenActive() method is changed, and the docblock states:

Evaluates any RequestPatterns of the given token to determine whether it is active for the current request

• If no RequestPattern is configured for this token, it is active
• Otherwise it is active only if at least one configured RequestPattern per type matches the request

This is done by grouping the patterns by type and then at the end doing return !in_array(false, $requestPatternsByType, true) But… this check is negated! Consider this:

$requestPatternsByType = [
    'Neos\Flow\Security\RequestPattern\ControllerObjectName' => true,
    'Flowpack\Neos\FrontendLogin\Security\NeosRequestPattern' => false
];
!in_array(false, $requestPatternsByType, true));

Clearly one of the patterns matched (thus true in the array). But it evaluates to false. This would be evaluating to true, instead, as expected:

in_array(true, $requestPatternsByType, true));

[kdambekalns]

What puzzles me: this has been in place for quite some time, and nobody noticed… so–is it really the bug I see?

@bwaidelich Could you have a look when you are back? AFAICS the code is yours… @foerthner Maybe you can also say something about this?

[bwaidelich]

@kdambekalns it needs to be discussed whether the current behavior makes complete sense, but it is in sync with the docblock:

Otherwise it is active only if at least one configured RequestPattern per type matches the request

(Note the per type)

The above fails because there is only one pattern of type NeosRequestPattern and that returns false.

So to fix the error you either need to remove the NeosRequestPattern configurations or (preferably) add another one that matches like so:

Neos:
  Flow:
    security:
      authentication:
        providers:
          'Neos.Neos:Backend':
              'Marvin.Test:NeosFrontend':
                pattern: 'Flowpack\Neos\FrontendLogin\Security\NeosRequestPattern'
                patternOptions:
                  matchFrontend: TRUE

PS: The example package uses Security\Context::getAccount() to fetch the account. getAccountByAuthenticationProviderName() would be the safer option if only an authenticated account for a given provider should be returned

[kdambekalns]

(Note the per type)

Ah, that's sneaky. But, yeah, so it does what is says it does…

Maybe an example in the docs/docblock would help. And I do think the behaviour can be discussed… ;)

Thanks for the workaround pointer!

[kdambekalns]

We just came across this again… Would it be an option to have a flag for switching between "all types" or "at least one type"?

[kitsunet]

Not sure I would touch this. IMHO th basic premise of touching these highly interwoven providers (the Backend and Frontend login) is a problem. Roll your own if you want full control over how it works. IMHO such a flag changing low level decision processes will make debugging and reasoning about the code more difficult, especially as there is a solution for the use case.

[kdambekalns]

/cc @franzkugelmann

[kitsunet]

Just came across this again :D

I think it would be a pragmatic and helpful approach to make the path prefix(es) simply configurable via settings (so you don't have to keep it in sync between Frontend and Backend (if it was a patternOption). That would make it much more flexible.

So in the NeosRequestPattern we inject a setting that by default defines:

backendUrlPrefixes:
  neos: 'neos'

And which you could then easily extend in your package.