As the title says, 2.x branch of Mailhandler is not checking access to node creations/update.

Maybe this code was working in 1.x thanks to some user_switch, but with feeds, we have to load the user account based on the sender email.

Path attached. I'm afraid this is also valid form comments, global $user is not enough.

CommentFileSizeAuthor
#7 mailhandler-927654-7.patch14.31 KBdagmar
#4 mailhandler-authentication-fix-927654-4.patch9.63 KBIan Ward
#1 mailhandler-927654.patch2.91 KBdagmar
mailhandler-authentication-bypass.patch2.34 KBdagmar

Comments

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
StatusFileSize
new mailhandler-927654.patch2.91 KB

Ok, now that #921774: Support CCK creation using Feeds and Mailhandler is in, I have modified the patch to use an exception to avoid the node creation. Also this fix the @TODO introduced in the patch committed in #921774.

mark trapp’s picture

mark trapp
he/him
Primary language English
commented

Tagging.

mark trapp’s picture

mark trapp
he/him
Primary language English
commented

The patch looks good, but I have a question about the exception thrown:

 if (node_access('create', $target_node, $account)) {
  $this->config['author'] = $account->uid;
}
else {
  throw new Exception(t('User: @personal (@mail) cannot create @node_type nodes.', array('@node_type' => $target_node->type, '@personal' => $value[0]->personal, '@mail' => $mail)));
}

What handles that exception?

Ian Ward’s picture

Ian Ward commented
StatusFileSize
new mailhandler-authentication-fix-927654-4.patch9.63 KB

Attached is a patch which gets MailhandlerNodeProcessor::authenticate working again, which is the mailhandler authentication plugin system. It now works by mapping the 'authenticate' source to the 'authenticate' target. I'm thinking FeedsNodeProcessor should somehow offer a way to bail out of node creation again, instead of defaulting to anonymous author. This could be a setting on the processor which says "allow anon. node creation or require access check."

It may actually be possible to use hook_node_presave to get around doing this hacky mapping stuff in this patch... but will post what I have for now.

Ian Ward’s picture

Ian Ward commented

Noting that hook_node_presave won't work because it's then too late to halt node creation. What may work, and I think dagmar mentioned this on [xxxxx] is to:

* Run authentication in the parser, and if authentication fails, skip over the message so it never gets processed (need to look at feasibility of this)
* Do the mapping, like in the patch above.

This wouldn't get around this hackish way of needing to create a mapping to actually authenticate and map the found uid to the node object, but, it would at least prevent node creation.

Another option is a patch to FeedsNodeProcessor so that the $node can be altered before the node_save is run, and then have the node_save check whether there's still actually a $node or whether its been set to FALSE. I like this third option best because it makes setting up the mapping unnecessary and simplifies how to halt node processing.

Ian Ward’s picture

Ian Ward commented

I meant #921774: Support CCK creation using Feeds and Mailhandler where i had "[xxxxx]"

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
StatusFileSize
new mailhandler-927654-7.patch14.31 KB

I have reviewed the patch attached in #4. I'm afraid Authentication plugin is not being loaded at all. After some research, I think the problem is MailboxNode class is not adding the settings as Mailbox class does, and therefore, $mailbox->getSettings('authenticateplugin', $mailbox) never loads the Authentication plugin.

This patch includes the necessary changes to load the correct Auth plugin, and also provide a UI to define what to do if the Authentication fails.

@Mark Trapp, related to your question, the exception is handled by FeedsProcessor in process() function. I have added a comment explaining this.

@Ian Ward, I think the exception is enough to avoid the creation of the new node. At least it works in my tests.

danepowell’s picture

danepowell commented

Patch in #7 worked for me- it prevented non-authorized users from creating nodes, and assigned the correct author to created nodes

Ian Ward’s picture

Ian Ward commented
Status: Needs review » Fixed

Thanks everyone. I committed a combo of #4 and #7. @dagmar I did not see the issue of the authentication plugin not getting loaded. If you see that issue again you can open a new ticket for it. Closing this one.

Status: Fixed » Closed (fixed)

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