Validation failure for body text filter format due to lack of permission

This is quite a WTF to be honest.

\Drupal\filter\Plugin\DataType\FilterFormat::getPossibleOptions and filter_formats depend on the current user, so the API we have validates access of the current user.

By default switching the user in D8 is less scary than in D7, because we don't save that by default:

\Drupal::service('account_switcher')->switchTo($account);

I agree with @dawehner and @moonray this seems an important bug to fix. @moonray told me that the user that creates the nodes on cron is configured in the UI. It should be switch to that user whilst it is doing that.

1. +++ src/Feeds/Processor/EntityProcessorBase.php	(revision )
   @@ -119,6 +121,17 @@
   +    else {
   +      $account = new AnonymousUserSession();
   +    }
   

   Originally whilst reviewing this patch I wondered if this was correct by reading the UI text it makes it clear that blank is anon so +1. This means that the user whilst creating content from feeds is always predictable. Nice.

2. +++ src/Feeds/Processor/EntityProcessorBase.php	(revision )
   @@ -119,6 +121,17 @@
   +    $switcher = \Drupal::service('account_switcher');
   

   The service should be injected in the constructor.

I'm not completely sure about the proposed fix, though it seems to be a step in the right direction. Ideally, the import would be owned by the one that triggered the import (thus the person who created the feed on the site). But perhaps that is an other issue.

Also note that in case of nodes, "owner" is the author set on the nodes. But the one that imports the content and the author may not always be the same. It is possible that the one that imports the content also sets "private" fields that the author may not set. For example, I have a site where the content has a field that contains an ID from the external source. The author of the content is not allowed to change the value of that field.

@MegaChriz - then what is the point of this setting...

      $form['owner_id'] = [
        '#type' => 'entity_autocomplete',
        '#title' => $this->t('Owner'),
        '#description' => $this->t('Select the owner of the entities to be created. Leave blank for %anonymous.', ['%anonymous' => \Drupal::config('user.settings')->get('anonymous')]),
        '#target_type' => 'user',
        '#default_value' => User::load($this->configuration['owner_id']),
      ];

If we switch to this owner then if said owner edits them then everything should work as expected. Anything else doesn't make sense.

@alexpott
I think we crossposted, see #9.

I think that the switch to the configured owner should only be made when the option "Authorize" is checked. That option is now used to check if the owner has permission to create or update the entity (see EntityProcessorBase::entitySaveAccess()). It probably makes sense to extend that with permissions checks on the entity's fields.

Perhaps in the other case it should switch to user 1? I'm not sure what the security implications of that are. As I said earlier, it would probably be better if the switch was made to the one that triggered the import.

So this pr

+++ src/Feeds/Processor/EntityProcessorBase.php	(revision )
@@ -119,6 +121,17 @@
+    // To prevent validation errors during node creation, switch to configured user.
+    if (!empty($this->configuration['owner_id'])) {
+      $account = new UserSession(['uid' => $this->configuration['owner_id']]);
+    }
+    else {
+      $account = new AnonymousUserSession();
+    }

So here's a thing. The cron service switches to the anonymous user. So I think this is sensible behaviour as it ensure consistent result regardless of whether the process was kicked off by a user or by cron. If this doesn't do this you can have the situation where it works when you run interactively through the UI but not on cron. That'd be truly frustrating.

I do think it is worth somehow trying to check that a feeds current 'owner_id' can successfully create a node of the desired type with the fields you want.

If it is a required feature that the feed processor user can set things that the owner can't then we need a new setting that allows a different user to be the one that does the creating.

Alternatively we could just switch to user 1 here. But that feels really icky.

A separate setting for the switch account also makes sense in case the entity does not implement EntityOwnerInterface.

One thing that is kind of weird here it that by setting this field you can do something as another user. Perhaps you should only be able to change it yourself or leave it as the existing user. Not sure.

Perhaps any kind of permission validation should be bypassed unless "Authorize" is checked. My assumption is that validation of the input is mainly there to catch invalid values. For example: an item should be rejected if it tries to reference a non-existing taxonomy term.
Also at least in the D7 version, with only the permission to administer feeds, you are able to configure an importer that imports users. It is questionable if that is right too.

I would like twistor's view on this. He is much longer around as maintainer of Feeds than I do. And he wrote pretty much all of the D8 code of Feeds.

This is the issue where the "Authorize" option was added (D6 version of Feeds): #1041646: Check permissions when creating nodes (FeedsNodeProcessor)

As a start, we should switch to the Feed owner. That's the closest way to emulate how the batch import runs.

We should also do it during the actual cron run, no in the processor.

Quick refactor. Botched in interdiff.

This needs to be backported to the D7 version as well. See #2541944: Switch to feed author or user 1 during imports (taxonomy mapping does not work with cron).

Hello,

Rerolling the patch from comment 19.

Hello,

When applying the rerolled patch, I still got the same error as the issue summary one.

When importing manually on feed/1/import, no problem.
When using drush cron or triggering the cron using admin/config/system/cron, I obtain:

The content .................. failed to validate with the error: The value you selected is not a valid choice. Please check your mappings.

I just did some quick manual testing and it works.

@Grimreaper, is the user you use to import via cron the same user that authored the feed?

Gonna commit this, and then work on the test case.

Tracking the user that executed the feed manually is not a good idea. For one, what about manually created feeds. Second, user 1 can import a feed at any time, essentially raising the privileges of the feed. Third, users of various roles can execute the same feed, essentially changing the permissions of the feed at random.

I've created #2811429: Switch to feed owner during manual import. to lower the WTF factor for manually importing feeds.

Hello,

Sorry for the delay.

@twistor: The user used to run cron is the admin (user 1), the same that has created the feed.

I have just restested feed. And now even when I import manually I have no element imported. And no error on the logs.

I will retest some months later.

Hi.
this one will solve the problem with validations for feeds which are running under cron.
@twistor please review

Hi there,

Just wondering if there's any plans for a fix soon? I've tried to import on cron with the latest alpha release and still experiencing the problem:

Importing by manually clicking 'import' in the feed works fine, just get this error on cron.

Note: feed user/author is set as an admin user

Thanks,

Hello,

On Feeds version 8.x-3.0-alpha1, I have the same issue as @telegraph when launching import with drush cron.

The feed's author is an authenticated user. And I have the option to set the author of feed items to the feed's author.

I had changed my filter format to an autorized one for anonymous users.

Drupal core 8.5.6
Feeds 8.x-3.0-alpha2

Importing RSS feeds, I've seen similar issues. And errors don't seem to bubble up. I can run the import manually with no issues, but cron fails. When I run cron from the devel menu I sometimes, but not always, see warnings.

Happy to provide any other information you need.

Patch #28 fixes this issue for me, can we get this RTBC? This seems pretty important as surely most people are running their feeds with Drupal Console / Drush?

It would be useful to have a functional test for this that demonstrates the issue. The test should perform the steps described in the issue summary and run cron.

(I removed the 'backport' tag, as the backport already happened in #2541944: Switch to feed author or user 1 during imports (taxonomy mapping does not work with cron).)

The patch in #28 fixes this for me too.

I observe that the import runs as the feed creator, and not the owner defined on the feed. So it works, but isn't perfect for my use case.

Here is a patch with a test that follows the steps provided in the issue summary. The patch is also a reroll as the patch from #28 no longer applies.

Finally understood why AccountProxy works better than UserSession. With UserSession you have still an anonymous user which just happens to have an ID, with AccountProxy you have access to a full user entity.

Committed #35. @alphawebgroup gets credit for the fix.

thank you for committing that.. i knew one day it will happen))