Problem/Motivation

Now we've fixed #2833562: Installing modules using the ConfigImporter does not work this exposes another interesting issue. Content created during an install occurs before any users exist meaning it is all owned by user 0! This will occur if you are using something like the Config Installer or just have the default content as part of an install profile.

I think this is a major bug because content being owned by the anonymous user can cause very odd security issues if perms are set up oddly.

Proposed resolution

Ideally I think we'd add an extra install step after the site configuration form - which creates user 1 - where default content would be created. Not sure if this is possible. It's not possible.

Create the required user 1 and delete it? A bit hacky but better than the alternative.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#13 2872278-13.patch12.26 KBalexpott
#13 11-13-interdiff.txt528 bytesalexpott
#11 2872278-11.patch12.25 KBalexpott
#10 2872278-10.patch11.81 KBalexpott
#10 8-10-interdiff.txt2.78 KBalexpott
#8 2872278-8.patch11.64 KBalexpott
#5 2872278-5.patch716 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes

There's no way for a module to hook into install_tasks().

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Hmmm but user_install() creates user 1 so why is this not working...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Ah lol...

    "_embedded": {
        "http:\/\/drupal.org\/rest\/relation\/node\/branch\/uid": [
            {
                "_links": {
                    "self": {
                        "href": "http:\/\/default\/user\/0?_format=hal_json"
                    },
                    "type": {
                        "href": "http:\/\/drupal.org\/rest\/type\/user\/user"
                    }
                },
                "uuid": [
                    {
                        "value": "a1882731-fdbb-4164-8c76-0bc67bc483ba"
                    }
                ],
                "lang": "en"
            }
        ],
        "http:\/\/drupal.org\/rest\/relation\/node\/branch\/revision_uid": [
            {
                "_links": {
                    "self": {
                        "href": "http:\/\/default\/user\/0?_format=hal_json"
                    },
                    "type": {
                        "href": "http:\/\/drupal.org\/rest\/type\/user\/user"
                    }
                },
                "uuid": [
                    {
                        "value": "a1882731-fdbb-4164-8c76-0bc67bc483ba"
                    }
                ]
            }
        ]
    },

These UUIDs are not going to be the same - so I guess we're falling back to user 0 because we just don't know and this is the logged in user. This feels wrong. But offers solutions.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2872278-5.patch716 bytes

So we have a problem even in the tests... here's proof... the node we create should be owned by user 2 - it's not.

Status: Needs review » Needs work

The last submitted patch, 5: 2872278-5.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I think this exposes a bug in the denormaliser - if the user doesn't exist it shouldn't fall back to user 0 it should fallback to the logged in user.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2872278-8.patch11.64 KB

Well that's fun. I think user 1 should be portable so I think we should always reference it by ID and not UUID but here's a fix self contained to the default content module.

Status: Needs review » Needs work

The last submitted patch, 8: 2872278-8.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
8-10-interdiff.txt2.78 KB
2872278-10.patch11.81 KB

Fixing the test groups and some coding standards.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2872278-11.patch12.25 KB

Lol after all the messing with normalisers whilst I think that we might consider user 1 to be a special case in the Hal and other core normalisers we don't actually need to do that here.

We can just detect if the entity uses EntityOwnerInterface and default the owner to user 1. Yes this means you can't create content owned by user 0 via default content but I think that that is a super edge case.

This also means that for import #2861591: Export content as user 1 when running in a CLI context is kinda superfluous and we might consider the account switcher for export too.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Thanks @alexpott, looking good to me.

I'm sure Berdir and I came across this issue once before, fairly sure we used to have the assert for $node->uid === 2 in the test, can't recall why we didn't come back to it.

+++ b/src/Importer.php
@@ -119,6 +132,8 @@ class Importer implements ImporterInterface {
+      $root_user = User::load(1);

nit: Please use $this->entityTypeManager->getStorage('user')->load() instead of the singleton in case we want to unit test this in the future.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
11-13-interdiff.txt528 bytes
2872278-13.patch12.26 KB

@larowlan - nice catch

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Assigned: Unassigned » andypost
Status: Needs review » Reviewed & tested by the community

looks good to me, thanks again @alexpott

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

It looks great! Gonna commit soon!

BTW fallback to uid1 is more secure then anonym.

And this needs follow-up to made fallback configurable

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Making the fallback configurable is kinda interesting. It means that the default_content module would have to depend on that user existing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Here 2 examples
- bunch of stage content exported to use as "fixture" but I wanna all owners to be assigned to uid=2 (kinda obfuscation)
- fallback to load user by ID when no uuid found and fallback to `--run-as` for drush operations

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@andypost sure but you still have the problem of ensuring that the user exists when installing. Imo the fallback feature should also include exporting the user :)

  • andypost committed 206190f on 8.x-1.x authored by alexpott 
    Issue #2872278 by alexpott, larowlan: Content created during an...
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Assigned: andypost » Unassigned
Status: Reviewed & tested by the community » Fixed

Commited, thanx for contribution!

@alexpott yes, better use export filters/modifiers

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.