Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails,read this.

try

Build succeeded:

• CI-Done

Since new elements are added to the UI (extra buttons for a user on the user list), it must be added to the documentation.
webadminstration.rst.

Thank you for the suggestion. I know added the information on the new buttons to the documentation there.
(I also amended the a previous commit to only show the webmail button only if webmail is actually enabled)

When using roundcube a banner is displayed in top with the information that you are impersonating an user. However this is missing for snappymaill.

Yes, I had skipped snappymail since I was unable to correctly configure it for local testing (see#3162).

Having had a look again now into snappymail, it seems to me that snappymail does not provide a straight-forward API to easily add a simple banner when logged in as another user the same way as with roundcibe. So I'm a bit skeptical whether the effort required to implement this analogously to roundcube is really worth it...

@nextgens is adding the same banner to snappymail a hard requirement for this PR? Would there be maybe another way than a banner to notify the user which may be easier to implement in snappymail?

And besides this: is there anything else blocking this PR from being merged?

Can't we just frame the webmails when impersonation is in use?

@nextgens I (rebased and) added a PoC for a framed webmail with impersonation ina8dbc5c.

But in my opinion framing does not provide the best user experience and can be confusing. Because the webmailer will use the same session cookie with and without framing. So having an open tab with the admin user's normal mailbox while opening another user's mailbox with impersonation will also switch the session in the open tab to the other user's mailbox - but without the alert.

Also, having the impersonated mailbox opened in a frame (especially when keeping the mailu admin navigation as in this PoC, but that can easily be removed, though) could imply to the user that impersonation is only active in that window...

So, I'd rather let the webmailer itself handle everything instead.

But let me know which route you want to go here, and I can update the PR accordingly.

@nextgens anything I can do to move this PR forward?

bors try

try

Build succeeded:

• CI-Done

But in my opinion framing does not provide the best user experience and can be confusing. Because the webmailer will use the same session cookie with and without framing. So having an open tab with the admin user's normal mailbox while opening another user's mailbox with impersonation will also switch the session in the open tab to the other user's mailbox - but without the alert.

Also, having the impersonated mailbox opened in a frame (especially when keeping the mailu admin navigation as in this PoC, but that can easily be removed, though) could imply to the user that impersonation is only active in that window...

Those are good points; We do not want that.

Would it be doable/better to have a separate 'virtual' path for each webmail instance? So that one could open several tabs with different inboxes at the same time without having things breaking.

Say redirect to/webmail-as/test@example.com/ and have nginx proxy that too? If the path is different we may have different cookies too (so completely different parallel webmail sessions).

So, I'd rather let the webmailer itself handle everything instead.

I wanted to try out framing as that is what would be maintenance free; I am not opposed to doing it differently. If the visual indication is in the URL maybe we don't need the UX to change at all.

But let me know which route you want to go here, and I can update the PR accordingly.

As is snappymail is broken (it sets some anti-framing headers).

I'd love if you could try to make the parallel webmail session thing to work. In the future we would/could even offer all webmails by default (/webmail would redirect to either/roundcube or/snappymail)

Would it be doable/better to have a separate 'virtual' path for each webmail instance? So that one could open several tabs with different inboxes at the same time without having things breaking.

Say redirect to /webmail-as/test@example.com/ and have nginx proxy that too?

I implemented this with2eea2a4. It is working but I'm not sure if it really is worth it because it requires a lot of additional complexity for the integration:

First, for the webmail to be available under multiple URLs we not only need to make it available with different base URLs but even underdynamic base URLs to include the e-mail address.
Since thefront nginx rewrites the request URI the original/dynamic URL gets lost. I "solved" this by passing the correct base URL as an additionalX-Remote-Base-Url header to the webmail nginx:

This then has to be handled by nginxas well as the PHP code:

If the path is different we may have different cookies too (so completely different parallel webmail sessions).

Yes, having different path for each mailbox this is possible by usingproxy_cookie_path to limit the cookies set by the webmail to the proxied path infront.
But this may cause unexpected issues if the path overlap, e.g., withWEB_WEBMAIL=/.

Another problem was thatauth_request does not allow to use variables in the auth URL. So to pass the impersonated user e-mail from the request I again had to add an additionalX-User-Email header:

All of this already adds much complexity. Additionally it probably also still requires some refactoring on how the valid webmail token(s) are stored in the session. Because up until now there is always only one valid token in the session and previously we differentiated the user (original user or impersonated user) by storing the e-mail in the session as well. But now there could be multiple valid e-mail/token combinations.
Should we re-use the same token in the session for all e-mails? Or should we rather use a new token when impersonating? I guess, both would work...

If this solution should be used, I can try to polish up the commits again, so what do you think?

@nextgens after thinking about it a bit more: having a dedicated proxied path for impersonated webmail is actually quite neat and provides the best UX be even allowing to have two different mailboxes open side by side in the same browser session.
Implementing that cleanly though still requires some refactoring of the admin service in how the tokens are stored in the session. I can take a shot at it if wanted.

The issue with conflicting cookies in case whenWEB_WEBMAIL overlaps with the impersonating path can not really be resolved though (it would probably required some kind of cookie-rewriting middleware, which would blow up complexity...).

So it stand to reason whether the added complexity is really worth the result since it'll probably still be a rarely used feature...

How best to proceed here?

@nextgens after thinking about it a bit more: having a dedicated proxied path for impersonated webmail is actually quite neat and provides the best UX be even allowing to have two different mailboxes open side by side in the same browser session. Implementing that cleanly though still requires some refactoring of the admin service in how the tokens are stored in the session. I can take a shot at it if wanted.

The issue with conflicting cookies in case whenWEB_WEBMAIL overlaps with the impersonating path can not really be resolved though (it would probably required some kind of cookie-rewriting middleware, which would blow up complexity...).

I am not sure I am following regarding overlapping paths; We could useproxy_cookie_path to ensure we maintain independent cookies with the same names but different paths, no?

So it stand to reason whether the added complexity is really worth the result since it'll probably still be a rarely used feature...

How best to proceed here?

I am not sure either

I am not sure I am following regarding overlapping paths; We could useproxy_cookie_path to ensure we maintain independent cookies with the same names but different paths, no?

Yes, this works and is utilized by this PR, see e.g.

But there is an edge case withWEB_WEBMAIL=/ and e.g.WEB_ADMIN=/admin. With that the regular webmail will be available at path/ while an impersonated webmail is at path/admin/webmail-as/$user_email. Since the latter is a subpath of the former the browser can mix up the session cookies for the normal and the impersonated webmails.

With something likeWEB_WEBMAIL=/webmail andWEB_ADMIN=/admin this works without issues, though.

I don't see that as a problem: we won't be backporting the feature and personally I am okay with removingWEB_* variables in the name of simplicity for next release.

We are already instructing those that do run reverse proxies to forwardeverything for Mailu's FQDNs to Mailu.

personally I am okay with removingWEB_* variables in the name of simplicity for next release.

Is this already on the roadmap? Because I see these config options as a valuable feature and would want to see them gone - especially not for this 😅

I don't see that as a problem

Aside from this single edge case me neither. Even keeping theWEB_* variables this could be "fixed" by adding a warning to the documentation or maybe a sanity check into the admin scripts...

So anyway, should I then proceed with the necessary changes to cleanup the storage of tokens in the session so we can bring this PR to a merge?