Multiple Policy.yaml role merging is broken

[Pingu501]

Description

As Flow supports splitting of your Policy.yaml I wanted to use it and split Policys by parts of the application. After a couple of ours of really strange behaviour I could isolate the problem. The merging of the roles inside a package seams to be more like an override

Steps to Reproduce

1. Inside your SitePackage, create Configuration/Policy.Bar.yaml and fill it with

privilegeTargets:
  'Neos\Flow\Security\Authorization\Privilege\Method\MethodPrivilege':
    'Sandstorm.ProjectX:MyController':
      matcher: 'method(Sandstorm\ProjectX\Controller\Module\MyController->(?!initialize).*Action())'

roles:
  'Neos.Neos:Administrator':
    privileges:
      -
        privilegeTarget: 'Sandstorm.ProjectX:MyController'
        permission: GRANT

2. Inside your SitePackage, create Configuration/Policy.Foo.yaml and fill it with

privilegeTargets:
  'Neos\Neos\Security\Authorization\Privilege\NodeTreePrivilege':
    'Sandstorm.ProjectX:Neos.All':
      matcher: 'TRUE'

roles:
  'Neos.Neos:Administrator':
    privileges:
      -
        privilegeTarget: 'Sandstorm.ProjectX:Neos.All'
        permission: GRANT

3. run ./flow configuration:show --type Policy

Expected behavior

I would expect the Neos.Neos:Administrator role to contain the following entries (plus the defaults from Neos)

roles:
  'Neos.Neos:Administrator':
    privileges:
      -
         privilegeTarget: 'Sandstorm.ProjectX:MyController'
         permission: GRANT
      -
         privilegeTarget: 'Sandstorm.ProjectX:Neos.All'
         permission: GRANT
    # plus configuration from other packages

Actual behavior

Only the privileges from the last Policy.*.yaml inside the site package made it into the configuration

  'Neos.Neos:Administrator':
    privileges:
      -
         privilegeTarget: 'Sandstorm.ProjectX:Neos.All'
         permission: GRANT
    # plus configuration from other packages

Affected Versions

Neos: 5.3.0

Flow: 6.3.4

[albe]

The problem is, that we do overwrite numeric indexes when merging the configurations and that's also documented as such in Arrays::arrayMergeRecursiveOverrule:

Merges two arrays recursively and "binary safe" (integer keys are overridden as well)

So while I see this as buggy behaviour in the specific case of having privilege lists in different policies and expecting them to be joined together, this will not be trivial to fix b/c.

For now, a workaround could be to index the privileges with unique names like so:

privilegeTargets:
  'Neos\Flow\Security\Authorization\Privilege\Method\MethodPrivilege':
    'Sandstorm.ProjectX:MyController':
      matcher: 'method(Sandstorm\ProjectX\Controller\Module\MyController->(?!initialize).*Action())'

roles:
  'Neos.Neos:Administrator':
    privileges:
      'Sandstorm.ProjectX:MyController':
        privilegeTarget: 'Sandstorm.ProjectX:MyController'
        permission: GRANT

privilegeTargets:
  'Neos\Neos\Security\Authorization\Privilege\NodeTreePrivilege':
    'Sandstorm.ProjectX:Neos.All':
      matcher: 'TRUE'

roles:
  'Neos.Neos:Administrator':
    privileges:
      'Sandstorm.ProjectX:Neos.All':
        privilegeTarget: 'Sandstorm.ProjectX:Neos.All'
        permission: GRANT

should lead to:

roles:
  'Neos.Neos:Administrator':
    privileges:
      'Sandstorm.ProjectX:MyController':
         privilegeTarget: 'Sandstorm.ProjectX:MyController'
         permission: GRANT
      'Sandstorm.ProjectX:Neos.All':
         privilegeTarget: 'Sandstorm.ProjectX:Neos.All'
         permission: GRANT
    # plus configuration from other packages

[Pingu501]

Did not know keys can be used for indexing, thank you! Should be a good enough workaround for now ;)

I understand this will be hard to fix, but I think it still would be great some time in the future. Also i think the notation with the - instead of a fully written key is more readable

[sorenmalling]

I understand this will be hard to fix, but I think it still would be great some time in the future. Also i think the notation with the - instead of a fully written key is more readable

Super valid point :) We can look into a "appending" mode, when merging configuration. I believe that we have heard about this in other scenarios (Settings and numeric index)

[kdambekalns]

Major reason we switched from

superTypes: ['Foo', 'Bar']

to

superTypes:
  'Foo': true
  'Bar': true

[kdambekalns]

But obviously accross different packages that works as expected for policy files, so maybe we can adopt the same behaviour for policies within a package, specifically?

[albe]

But obviously accross different packages that works as expected for policy files, so maybe we can adopt the same behaviour for policies within a package, specifically?

Indeed! 😮 We seem to have ConfigurationManager::mergePolicyConfiguration() specifically for that case, so maybe we can use that method for the split merge too. We'd need to somehow pass this merging method down into the YamlSource::load() method though

[sorenmalling]

ping all :)

With the merging of https://github.com/neos/flow-development-collection/pull/2449 being on it's way, this topic can be revisited afterwards, with new fresh and smooth possibilities :)

[albe]

Indeed that change brings us closer, moving the mergePolicyConfiguration() method one step nearer to the YamlSource::load() invocation: https://github.com/neos/flow-development-collection/pull/2449/files#diff-5a2039724ad9c8671d64edaf103bdfccd4ae9d165eee4e7a8c4cf393b9ac8173R61-R69 Solution is to either move the globbing+merging out of YamlSource, or pass in a merging closure (that defaults to Arrays::arrayMergeRecursiveOverrule)