Do not allow drupal/core-composer-scaffold to be used by packages other than core

👏 Great find, and thanks for the detailed write-up! 🙏🤩

Issue summary update: improved markup and issue links; no material changes.

This definitely improves reliability, arguably also security. Added to roadmap at #3319030: Drupal Core Roadmap for Package Manager and Update Manager because you tagged it core-mvp.

Composer and the
composer/installers plugin have a limitation that one project cannot be moved
inside of another project.

Woah, this very much sounds like the "inter-package symlinks should indeed not be allowed, but intra-package symlinks should be!" problem that is described in #3319507: Add symlink support to Composer Stager 2.0, require that version, and simplify UX & tests accordingly and being solved in https://github.com/php-tuf/composer-stager/issues/58. i.e. composer/installers makes sure that each installed package remains one atomic unit. That is exactly what the symlink support is aiming to do going forward!

(I bet they did that for composer/installers to allow parallellization in the future: if each package is an atomic unit, operations can be run in parallel. If that is not the case, they must be run serially. Clearly, the drupal/core-composer-scaffold plugin must execute after all other packages have been installed/updated!)

For instance what if a plugin moves a scaffold file into a directory that we are excluding from the apply phase?

🤔I don't understand yet how this can happen; isn't that exactly what the code in #3331310: Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows is proposing to do? (Take into account the targets of the scaffold plugin, and not exclude those?)

We also might want to consider at some supporting any package if drupal/core-composer-scaffold provided a UI like getAllScaffoldfiles() that would return all files across all packages that are allowed. Without this I don't think we can support it.

Very interesting! Tagging needs follow-up for that: that plugin is part of Drupal core, and so we can just file a Drupal core issue that blocks general Automatic Updates/Package Manager support for this plugin 👍

Couldn't that be prevented by ensuring that a new validator + excluder run last? 🤔

1. 👍 — but we should document explicitly that this could be supported in the future

Doesn't make sense to do this until #3342227: ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility is done, otherwise we're doing double work.

#3342227: ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility landed!

Discussed this with @omkar.podey, who'd already started looking into this! 👍 He has a pretty clear idea of how to approach this, very curious to see what he comes up with! 😄

Using composer/Plugin/Scaffold/AllowedPackages.php makes it easier to manage which packages are allowed to scaffold files.

sample code -

"extra": {
    "drupal-scaffold": {
      "allowed-packages": [
        "fixtures/drupal-core-fixture",
        "fixtures/scaffold-override-fixture"
      ],
      "locations": {
        "web-root": "./docroot"
      },
      "symlink": __SYMLINK__,
      "file-mapping": {
        "[web-root]/.htaccess": false,
        "[web-root]/robots.txt": "assets/robots-default.txt"
      }
    }

@Wim.leers i talked with @phenaproxima yesterday and he confirmed that any modules not in the allowed packages list except "drupal/legacy-scaffold-assets", "drupal/core", will not be allowed to scaffold files by default, He also helped writing the documentation for drupal/core-composer-scaffold and has vouched for it's implementation (also agreed on the documentation needs to be clearer). So I was able to simplify the validator as well as the tests.

Looking good! And thanks for catching "strict types" just in time before my review! 😁👍

Please just mark Needs review going forward — I will have reduced capacity for reviews. @tedbow or @phenaproxima can also review! 😊

This is looking very good. There is one choice here that I strongly object to, though.

This looks really good. Thanks for addressing my feedback! A few more things and then IMHO this is good to go.

Looks great to me! This is still tagged for a follow-up, originating in #3. Once that's filed, this is RTBC!

Created #3350172: [PP-1] Support any package to scaffold files in Automatic Updates + blocked that issue on a core feature request #3350176: Create something like getAllScaffoldfiles() in core-composer-scaffold.

Beautiful end result! 👏😄