Problem/Motivation

In order to allow handlers to manipulate with the current user data we could introduce user context switching.

Proposed resolution

By default, the "current" user should be an Anonymous user. Each analyzer could update it with their (user) findings.
For example, UserAnalyzer could be implemented to extract the mail address from "From" mail header field and find a corresponding user in the system. Thus, the user context could be updated.
Mailhandler currently supports PGP-signed emails which could allow PGPAnalyzer to update user context as well.

Comments

mbovan created an issue. See original summary.

miro_dietiker’s picture

Priority: Normal » Critical

Yeah, this is important and i'd say critical to contextual processing.
I was hoping to find a patch here... :-)

Arla’s picture

Sounds like an appropriate way to handle certain things.

Note that account switching can have unwanted effects. For example, with Feeds, we had a problem with automatically setting a text field because anonymous did not have access to the necessary filter format.

Could this be applicable also by the current bounce analyzers? They now do BounceAnalyzerResult::setRecipient (where "recipient" means "recipient of the prior message", thus = sender of the current message). Or is that scope separate from how account switching is intended to be used? Do we have example use cases?

mbovan’s picture

Status: Active » Needs review
FileSize
3.3 KB

Thanks for the feedback.

Note that account switching can have unwanted effects. For example, with Feeds, we had a problem with automatically setting a text field because anonymous did not have access to the necessary filter format.

Agree this could be a problem... For this case, we could assert user has access to the specific format before assuming the filter format access is granted.

Could this be applicable also by the current bounce analyzers? They now do BounceAnalyzerResult::setRecipient (where "recipient" means "recipient of the prior message", thus = sender of the current message). Or is that scope separate from how account switching is intended to be used? Do we have example use cases?

I think it can apply for bounce analyzers too. The idea would be that each analyzer can update the "identified user" = sender (or recipient in bounce messages language).

I'm attaching the first patch. At this point, I think we are overlapping with #2754253: Lack of standard result in collaboration of analyzers.

In case we introduce a standardized analyzer result, do you think identified user should be set on InmailAnalyzerResult or ProcessorResult? The problem can occur if some analyzers use their own result object (e.g. BounceAnalyzerResult) and still want to set the global (Inmail) user. On the other side, I am not sure we want to allow plugins (analyzers) to make changes on ProcessorResult?

Status: Needs review » Needs work

The last submitted patch, 4: support_switching_the-2754261-4.patch, failed testing.

miro_dietiker’s picture

+++ b/src/MessageProcessor.php
@@ -114,6 +124,11 @@ class MessageProcessor implements MessageProcessorInterface {
+    $this->accountSwitcher->switchTo(User::getAnonymousUser());

@@ -124,6 +139,9 @@ class MessageProcessor implements MessageProcessorInterface {
+    $this->accountSwitcher->switchBack();

IMHO the code should be clear in that being anonymous is the default behaviour. We only want to switch away if the target user is non-anonymous. Similarly, switching back is optional only.

Agree that committing this as is, is useless as it doesn't do anything. At the same time we need the result with a user context.
The only way to generalise it on this level is by introducing an account switcher result interface and checking for that.
And indeed, the bounce doesn't need to implement that.

if ($result implements MessageResultAccountSwitcherInterface && $result->getAccount->id() != User::getAnonymousUser()->id()) {
miro_dietiker’s picture

Status: Needs work » Postponed

Functional code will follow once the standard result is in.

miro_dietiker’s picture

Arla’s picture

Status: Postponed » Needs work
mbovan’s picture

Updated the patch according to the latest changes and #6.
Added tests as well.

Since we have $account on default result and as a "current user" too, I guess it would be preferable to use \Drupal::curentUser() rather than $default_result->getAccount() on handler's level?

Arla’s picture

Some suggestions, but nothing serious. I'm fine with committing as is.

  1. +++ b/src/MessageProcessor.php
    @@ -117,6 +127,11 @@ class MessageProcessor implements MessageProcessorInterface {
    +    // Conditionally switch to the account identified by analyzers.
    +    if ($default_result->getAccount() != $anonymous_user) {
    +      $this->accountSwitcher->switchTo($default_result->getAccount());
    +    }
    
    @@ -127,6 +142,11 @@ class MessageProcessor implements MessageProcessorInterface {
    +    if ($default_result->getAccount() != $anonymous_user) {
    +      // Switch back to a previous account.
    +      $this->accountSwitcher->switchBack();
    +    }
    

    This could be a place to use $default_result->isUserAuthenticated(). We wouldn't have to use $anonymous_user then.

    Vaguely related: DefaultResult could lazily call getAnonymousUser() in getAccount(), then MessageProcessor doesn't have to setAccount().

  2. +++ b/tests/src/Kernel/HandlerTest.php
    @@ -0,0 +1,63 @@
    +/**
    + * Tests handlers.
    + *
    + * @group inmail
    + */
    +class HandlerTest extends KernelTestBase {
    

    This test doesn't really test any handler. It's just handler-related. Maybe consider moving the method to ProcessorTest?

miro_dietiker’s picture

Status: Needs review » Needs work

It's very important that we guarantee that if we switch the account, we call switch back.
This should not call twice an API. (Handlers could accidentally alter values...)
Instead, we should determine a local variable for it in MessageProcessor.php and check it.

mbovan’s picture

Re #11.1: Replaced with $default_result->isUserAuthenticated().
Re #11.2: I missed ProcessorTest at first. Moved it there.

Re #12: Added a local $has_account_changed variable.

Status: Needs review » Needs work

The last submitted patch, 13: support_switching_the-2754261-13.patch, failed testing.

mbovan’s picture

Fixed AnalyzerTest.

miro_dietiker’s picture

Status: Needs review » Fixed

Very nice. :-)

  • miro_dietiker committed def0e5a on 8.x-1.x authored by mbovan
    Issue #2754261 by mbovan, miro_dietiker: Support switching the user...

Status: Fixed » Closed (fixed)

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