Better handling with drafts and fixed some additional issues

[sfer23]

Description of changes:

1. Functionality has been enabled that allows you to save a "custom from" value in drafts. Previously, this functionality worked without saving a "custom from" field value, which could result in the default email address being accidentally revealed.
2. The functionality of personal user settings has been added. Previously, you could either enable or disable the plugin functionality only for all users. A detailed description of the options has been added to the README.md description.
3. Added README.md and added screenshots to make it easier for users to navigate using the plugin’s functionality.

Comments to the code:

Changed the setting of the arbitrary sender variable in the session to another hook. Previously it was message_compose, but the hook has been changed to template_object_composeheaders to manipulate the message fields, "Bcc" and "Reply-To". Because of this, the name of the compose_from_headers method has changed. A hash change has been added to JavaScript functionality, so that the user, after clicking the plugin button, can save the message. In the style file for skins, a style has been added that adds a response icon in the settings item.

Additional changes:

Added a few screenshots to README file for better presentation

[sfer23]

I have applied your suggestions and replace a bit images (my mistake was jpg instead of png for screenshots). As for dedicated orphan branch - you decide, I don't have enough rights to create new branches in your repo, but you able to link images even to my repo if you don't want to keep 53k in versioning store.

PS. IMHO big binary data starts from 1M, and all less can be stored in git :)

Sorry for late response, a bit busy on other projects. As I understand you should also find some time to do local check, do you have some estimate when it's possible?

[r3c]

Hi @sfer23, thanks a lot for the updates! I'll check them out locally for testing. However I won't merge the PR as long as it contains binary files. Maybe the easiest would be for you to remove them and I'll prepare a 2nd commit to reference images stored in a separate branch? Otherwise I can rework the branch on my own, but then I would lose your authorship and would prefer to preserve it if possible 🙂

[edit] I pushed change proposal to https://github.com/sfer23/custom_from/pull/1 ; it can be reviewed commit per commit.

[r3c]

Hello @sfer23! I see you merged a few minor/style changes I suggested on your PR but I still have several open questions about it, and some parts of the code seem wrong to me. Please let me know if you wish to continue iterating on this PR 🙂

[sfer23]

The methods compose_additional_headers, compose_draft_from_headers, and compose_from_headers are triggered if there is already a saved or received message.

In the compose_draft_from_headers method, we add the ability to load a custom sender into the text field in drafts. In the compose_from_headers method, we replace the sender if necessary.

With the option self::RECEIVING_EMAIL_OPTION, we reply from the email to which the message was received. With the option self::RECEIVING_EMAIL_WITH_IDENTITY_OPTION, we reply from the email to which the message was received and use the IDENTITY settings for the other fields. With the option self::RECEIVING_EMAIL_WITH_DEFAULT_IDENTITY_OPTION, we reply from the default identity, and we replace the select field to leave only the default identity in the reply field since the Roundcube JS functionality does not allow otherwise.

In the compose_additional_headers method, we clear or replace fields for two options. Roundcube will independently fill in the fields on the frontend for other options. For our cases, we need to use replacements with our own fields so that the JS functionality does not overwrite our values.

Initially, the field value is set to empty and with the self::RECEIVING_EMAIL_OPTION option it is simply cleared and replaced with our html_textarea. With the self::RECEIVING_EMAIL_WITH_DEFAULT_IDENTITY_OPTION option, we also create our own fields so that the Roundcube JS functionality does not overwrite them, and fill the fields with values from the default identity.

[sfer23]

Hello @r3c, of course I want to continue, but I'm on 2 week vacation right now, soon will proceess request faster

[r3c]

That's starting to be more clear to me, thank you for your answer 🙂 I'll try to summarize what makes me hard for me to understand some parts of the change:

• The overall workflow goes through several method and neither their names nor their arguments give significant hint about what they're doing. For exemple compose_additional_headers doesn't say what headers it touches nor what it does to them.
• Some method expect and/or return broader types than what they should. For exemple compose_additional_headers takes a full opaque $attrib while reading only 2 or 3 fields out of it, and returns a modified $attrib variable while it can replace at most one value in it (so it could just return this value or null).
• Flow seems broken in some places ; I added missing break statements but there is one fall-through case statement in compose_additional_headers and I'm not able to tell if it's on purpose or an oversight.
• New configuration setting REPLY_FROM_SETTING has its value tested in multiple methods, controlling various effects. For example method template_object_composebody is clearing signatures if value is RECEIVING_EMAIL_OPTION when method compose_from_headers is depending on this same value for applying its best-match identity selection. I think each option value should be used in one place only to define a named set of separate effects, e.g. RECEIVING_EMAIL_OPTION could turn some "clear signature" and "match address from recipient", as well as something about bcc & reply-to headers which I didn't understand yet.
• The settings is now stored per user, which is an improvement, but therefore conflicts with previous global configuration system. It seems both are about changing the way "From" & related headers are set, so I don't think we should eventually have two independent settings defining how the plugin should behave.

Also mind the file is formatted with PSR-2 coding style and uses PHP 5-style arrays (so array() keyword, no brackets). Let me know if that's more clear and if I can help in any way 🙂

[MiMoHo]

Hi @r3c Just to make sure: you want to understand the background and use of the entire implementation, but don't argue with the general concept? The idea is to offer a four option reply setting allowing the user to define which (parts of the) identity shall be used in a reply:

• receiving email address
• receiving email address with matching identity
• receiving email address with default identity
• default identity

The first option replies from the receiving email address without adding parameters. The second option replies from the receiving email address adding parameters of a matching identity. The third option replies from the receiving email address adding parameters of the default identity. The fourth option replies from the default identity.

Currently, the plugin loses the modified email address and switched back to the user's default identity when saving a draft, closing the editing window and reopening the draft. This should have been fixed as well.

[r3c]

Hi @MiMoHo, thanks for clarifying, it's indeed more clear to me with these details and probably means we can improve the wording of this new option as well.

Another part that seem blurry to me at the moment is how the new feature interacts with existing matching rule configuration. The former is a user setting while the later is a global configuration, it would probably make sense to move it to a user setting as well but I can take care of it afterwards.

What should be clarified right now however is what replying with "receiving email address" means when the current global configuration changes what headers are taken into account and whether some address is considered matching an existing identity of not. For example I guess the last option ("default identity") completely ignores the global setting, making these two features having an effect one on the other. I would rather try to keep them orthogonal, maybe by having the matching behavior controlled by an option and the "use parts of the identity" flag controlled by another one? (what are these "parts" BTW, signature, real name, anything else?)

[MiMoHo]

Thanks for considering @r3c , but anything coding- or tech-related, @sfer23 would need to address. I can only comment from a user's perspective. What I was told by @sfer23 , there are limitations inside Roundcube that doesn't play nice with the custom_from logic we are extending. Check out related issues here that I commented on.

The reply settings shall overrule the default identity taking its parameters more or less into account: 1st option shall reply from the email address an email is sent to but does not add any identity parameters such as name or signature regardless if there is a matching identity or not. 2nd option does check for a matching identity to the receiving email address and adds all of its parameters to the reply. 3rd and 4th option use the default identity and all of its parameters, the only difference is, that option 3 uses the receiving email address instead of the default identity's email address (in case they are different).

Option 3 is most likely the most useful option for private use when having just one identity but you want to make sure that you can reply from each individual email address you gave to the platforms you registered on without having to set up an identity for them that would be needed for option 2, otherwise you'd reply from the receiving email address without additional information such as name and signature.

In fact, the first 3 out of 4 options partially ignore the default identity, but this is part of the feature. You could decide which of the four reply settings shall be preset to avoid unintended behavior until the user defines the preference. Personally, I would suggest option 4 (default identity) because: In a business scenario where Anna writes to Bob's corporate email who's out of office, Chris can email back but doesn't want to use Bob's email identity, but his own or a general info address. So even when they share the same webmailer setup, he won't accidentally pretend to be Bob. But when they willingly want to do so, they may use the plugin's email address editor to alter the name and email address they reply from. These changes will also be saved in the draft.

[r3c]

Thanks for clarifying. I'm wondering about what would be the best way to have these options play together with the existing matching rules configuration option:

• Let's say you kept default value that specifies "To=de", meaning an email received with "To: anything@yourdomain.com" as header will reply with "From: anything@yourdomain.com" if one of your identities is "me@yourdomain.com" because domain name matches.
• However with the new configuration setting you're introducing, option 2 wouldn't find any exact match, so it wouldn't pick identity parameters from "me@yourdomain.com", resulting in a somewhat inconsistent outcome.

Unless I miss something, options 1-4 have their own matching behavior which is different from the previously existing one, and are incompatible with its flags "d" (match by domain) and "e" (match anything).

This makes me think the two options may be reworked so they're more consistent one with another. Maybe we could agree on the following:

• There is only a single "match behavior" configuration, which can be set either globally or per user
• Match behavior specifies one rule per header (To:, Cc:, X-Original-To:, etc.)
• A rule defines two aspects:
  • Whether some identity can be considered as a valid match for given recipient address (using similar approach than current implementation)
  • What parameters should be used: those from matched identity, from default identity, or no parameter at all

Let's assume two identities "me@domain1.com" (default) and "myself@domain2.com", and following configuration:

• Header "To:"
  • Match: by domain
  • Use parameters from: matched identity
• Header "Cc:"
  • Match: exact
  • Use parameters from: default identity

Then here is how it would behave:

• An e-mail sent with "To: someone@domain1.com" would reply as "someone@domain1.com" (matched by domain) and use parameters from identity "me@domain1.com" (matched identity)
• An e-mail sent with "Cc: myself@domain2.com" would reply as "myself@domain2.com" (matched by exact match) and use parameters from identity "me@domain1.com" (default identity)
• An e-mail sent with "Cc: someone@domain2.com" would reply with default identity as no valid match can be found

Would that make sense to you and answer your needs?

[MiMoHo]

With the four reply options setup, I wanted to take into account all possibilities how users may use the plugin and not just focus on my needs. Let me give an example for each setting:

Option 1: Very basic, just making sure that I reply with the same (correct) email address like "github@mydomain.com" and don't reveal my default identity which would be the main mailbox "contact@mydomain.com" to which all emails are forwarded but that I wish to keep private for privacy reasons. Services receiving an email often work with email management solutions that filter primarily by email address and subjects including a "[ticket ID]". They don't reply with a read receipt and will ignore any specification I put in the "reply to" field in my identity, so I'm not using parameters at all. I also don't want to include my signature in emails sent to "github@mydomain.com" as GitHub Support must not know my personal details that I only wish to include when composing a new email from my main mailbox with default identity.

Option 2: I need to inform my laywer about the emails with GitHub Support. So even though I usually don't set up identities for each individual email address that I used to register on various platforms, I now do create one for GitHub only in order to automatically include my lawyer in CC (so I don't forget one time). For that, I specify my name, email address and CC value in the identity parameters. All other services shall continue to receive a reply from the receiving email address without any other parameters.

Option 3: I set up a default identity with my first and last name and email address contact@example.com. But my main mailbox shall be kept private. I wish to reply to "github@mydomain.com" but make it look neat by including my name specified in the identity. GitHub Support doesn't care about my name preference on the platform. They write to "FirstName SecondName ThirdName Surname ≤github@mydomain.com≥". But I wish to automatically reply from "FirstName Surname ≤github@mydomain.com≥", maybe even include my signature or specify other identity parameters that shall be used in the reply.

Option 4: I don't register with individual email addresses as I'm using trusted platforms only. Hence, usually I'm only using my default identity which is my main mailbox. But now, I advertised on the web that I'm looking to meet up with local strangers and wish to preserve my privacy and protect myself from (romance) scam. So I advertised using a custom email meetup@mydomain.com for people to contact me. When replying, I now need the option to manually change my email address from contact to meetup for which I'm using the plugin. When someone is legit, I will simply continue replying using my default identity.

These are possible scenarios which can be summarized into four general reply settings as far as I can imagine.

I'm not familiar with the technical background of matching rules currently implemented. Again, I'm not coding anything. I cannot even assure our implementation works as it is supposed to be. I can just comment from a user perspective.

My webhosting company removed the custom_from plugin due to other customers complaining about the lack of user customization as the reply setting is currently specified universally by the admin which affects all users whether they reply from their default identity or the receiving email address. However, many webhosting companies use the Roundcube webmailer so offering comprehensive reply settings in combination with the possibility adjust the header when composing an email seems crucial to me to allow people to use individual email addresses and protect themselves against spam and on the other hand, they don't struggle with maintaining identities which is a pain when using just the default setup of Roundcube.

In the end, it is @r3c 's plugin. When you understand the benefit, please find a proper way how this can be combined with your and the general Roundcube concept 🙏

[r3c]

Yes I think I understand how the four options you're willing to add were intended to work and the focus on these being configurable per user rather than globally. We agree on this part 🙂

What I was saying is that current implementation causes an inconsistent overlap with existing matching rules configuration (which only exists as a global setting for now, not per user).

That's why I was proposing something that would, in my opinion, merge them together.

[MiMoHo]

Yes, I think I understood the reason @r3c , but I do not know what this means for the overall implementation and how to proceed. I hired someone to develop what's needed to introduce the desired features but beyond the current proposal, I am afraid, I cannot contribute with coding-related activity. Unless there are substantial concerns about how this was done in the first place, it would require yourself @r3c to refine the tech. Is there anything else I can comment on? Otherwise, please keep us posted about the next steps and future progress.

[r3c]

Then I think we can first focus on improving current contribution, starting with my comment from three weeks ago: https://github.com/r3c/custom_from/pull/26#issuecomment-2268881205

This PR is introducing several separate features so I think it would be best to split it into smaller individual contributions, but if that's too much of an effort then let's keep current PR and iterate until it can be merged.

I understand you cannot contribute yourself directly, but the PR we're commenting about requires a fair amount of fixes and improvements before it can be merged, and none of my original feedback points were addressed so far.

I'll let you decide what you want to do with this PR and get back to me when you believe it's ready for being reviewed again.

[MiMoHo]

Hi @r3c , @bigclumsypanda made a few changes last week. Did that help in some way?

[r3c]

Hi @r3c , @bigclumsypanda made a few changes last week. Did that help in some way?

It helped a bit, but most of my comments are still not addressed nor answered. Just to be clear, I think this PR is still quite far from a state where it can be merged, and having mostly you @MiMoHo answering but not touching the code doesn't really help making things clearer to me.

I'm putting this on hold until some developer answers all previous feedback I already posted and tells me this PR is ready for being read again, as I feel the time spent on this PR is getting to high compared to the reasonably small functional change it was supposed to be.

[sfer23]

Hey! We applied some changes and commented on the code. Here’s a summary and questions:

The overall workflow goes through several method and neither their names nor their arguments give significant hint about what they're doing. For exemple compose_additional_headers doesn't say what headers it touches nor what it does to them.

Changed method name and left a comment.

---

Some method expect and/or return broader types than what they ould. For exemple compose_additional_headers takes a full opaque $attrib while reading only 2 or 3 fields out of it, and returns a modified $attrib variable while it can replace at most one value in it (so it could just return this value or null).

Changed the way of interaction with method. Now it returns value.

---

Flow seems broken in some places ; I added missing break statements but there is one fall-through case statement in compose_additional_headers and I'm not able to tell if it's on purpose or an oversight.

Added comment why “break” is not needed.

---

New configuration setting REPLY_FROM_SETTING has its value tested in multiple methods, controlling various effects. For example method template_object_composebody is clearing signatures if value is RECEIVING_EMAIL_OPTION when method compose_from_headers is depending on this same value for applying its best-match identity selection. I think each option value should be used in one place only to define a named set of separate effects, e.g. RECEIVING_EMAIL_OPTION could turn some "clear signature" and "match address from recipient", as well as something about bcc & reply-to headers which I didn't understand yet.

Moved to a single hook “template_object_composeheaders”

---

The settings is now stored per user, which is an improvement, but therefore conflicts with previous global configuration system. It seems both are about changing the way "From" & related headers are set, so I don't think we should eventually have two independent settings defining how the plugin should behave.

Should we just move settings to User side? For example, theres; a setting: const DEFAULT_HEADER_RULES = 'X-Original-To=deo;To=de;Cc=de;Cci=de;From=de'; It surely won’t be clear to User and he could enter invalid data there. Then it would break the plugin logic.

[r3c]

Hi @sfer23, thank you for your answers.

I have two concerns about the settings:

• New setting is per user while the old one is global. It would make more sense to align everything, and your per-user approach is better. I can take care of upgrading current code so that setting is now user centric. Of course it won't be just a free text field allowing user to specify some opaque string such as 'X-Original-To=deo;To=de;Cc=de;Cci=de;From=de', so it would be replaced by proper UI with backward compatibility for older configurations.
• Both old setting (DEFAULT_HEADER_RULES and your new one) are about matching addresses against identity, but they work independently and in an inconsistent way. I explained why in previous comment here and suggested an alternative which consists in having a single setting to cover both needs. Please let me know if it makes sense to you or if you have an alternative approach.

[sfer23]

Hey @r3c! Thanks for taking care of the user setting part. As for the second one, I need to discuss that with @MiMoHo.

[MiMoHo]

Hi @r3c , regarding your second concern: our approach may provide a larger functionality and allows for a more consistent and secure behavior. Let's assume some real-world use cases with your suggested single match behavior in line with current implementation:

An e-mail sent with "To: someone@domain1.com" would reply as "someone@domain1.com" (matched by domain) and use parameters from identity "me@domain1.com" (matched identity)

The user may not want to use all parameters defined in the matched/default identity that could for example include a signature or "copy to" value when replying from an individual email address "someone@domain1.com". For example, when I provide someone with a dedicated email address to make sure I preserve my privacy without revealing the default identity which is identical to the main mailbox, an automatically attached signature would nonetheless be included in a reply from *@domain1.com, but should actually only be sent to my verified contacts that address my main mailbox me@domain1.com.

An e-mail sent with "Cc: myself@domain2.com" would reply as "myself@domain2.com" (matched by exact match) and use parameters from identity "me@domain1.com" (default identity)

Assuming I've got a personal and business identity in Roundcube and have configured "John Smith" as my personal identity's name for me@domain1.com while using "John Smith │ CEO of Company" for my secondary business email identity myself@domain2.com. When my business email address receives an email CC, then I would reply with my personal domain's parameters which doesn't include my business role "CEO of Company" in the "from" header value.

An e-mail sent with "Cc: someone@domain2.com" would reply with default identity as no valid match can be found

As in the second example: assuming, emails sent to my business colleague are sent in CC to me so that I learn about the progress, then – when engaging in the discussion – I would send an email from my personal default identity which is not at all the intended behavior.

To conclude: I hope, I understood your logic and you agree on the limitations of a single match behavior which specifies only "one rule per header (To:, Cc:, X-Original-To:, etc.)". I would favor our approach and introduce new matching rules replacing the old logic as I explained with different examples before.

[r3c]

Hi @MiMoHo, thanks for your message. I think the new behavior you want to introduce is clear to me, as you already described in this previous message. I don't think we need to go over it again, I personally think current wording could be more explicit but this is really minor and can be discussed over a PR.

The point I was raising is about compatibility with current per-header settings. I understand it doesn't match your need, and I'm open to revisit it. What I was saying however is that it is currently incompatible with your new approach, because both options imply different matching logic:

• Current code matches virtual addresses using a full match or a domain match to decide whether a recipient address should be reused as sender upon reply or not ;
• New code (this PR) matches virtual addresses against identities to decide whether profile should be used or not when writing an e-mail.

I would like both options to stay consistent, so if a virtual address is matched against an identity and should be used for replying, then I want profile information from this same matched identity to be used (name, signature, etc.) if enabled. To my understanding this is quite different from what current PR is implementing. Again, I'm open to dropping current per-header approach and keep a single matching configuration, but I would like to make it consistent with the new feature you're introducing, rather than keeping two orthogonal and inconsistent options.

Let me try another time to explain how I see this could work:

• One "matching behavior" option would define which identity is matched against recipient addresses. This option would replace the existing per-header setting and accept four different values:
  1. Either match an identity by full (exact name + domain) match
  2. or match an identity by domain only (an exact match would still have higher precedence, so if I'm using "by domain" match and am receiving an email sent to "second@domain.com" while having two identities "first@domain.com" and "second@domain.com", I expect the later to be matched),
  3. or always consider a match against default identity regardless of recipient addresses
  4. or always consider a failed match, effectively disabling the plugin
• One "use profile from matched identity" option that would support two different values:
  1. True (use signature, name & other information from matched identity when replying)
  2. False (don't)
  3. We could imagine more fine-tune control on this option, e.g. use the name but ignore the signature

I think this setup covers the 4 options you were describing:

• Your "option 1" would be "matching behavior = 1" + "use profile from matched identity = false"
• Your "option 2" would be "matching behavior = 1" + "use profile from matched identity = true"
• Your "option 3" would be "matching behavior = 3" + "use profile from matched identity = true"
• Your "option 4" would be "matching behavior = 4" + "use profile from matched identity = true"

Other combinations offer more fine-tune control over how identities are matched and whether or not their profile information should be used in outgoing emails.

I hope this makes sense to you.