~migadu/alps-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH] Add From address calculation

Details
Message ID
<20210731195910.18687-1-martin@ashbysoft.com>
DKIM signature
missing
Download raw message
Patch: +81 -19
Try to have sensible inference of From address based on settings, username, and server host.

Add warning message when From could not be calculated.

Add validation message when From address isn't set instead of crashing.

Signed-off-by: Martin Ashby <martin@ashbysoft.com>
---
 plugins/base/routes.go    | 83 +++++++++++++++++++++++++++++++--------
 server.go                 | 15 ++++++-
 themes/alps/settings.html |  2 +-
 3 files changed, 81 insertions(+), 19 deletions(-)

diff --git a/plugins/base/routes.go b/plugins/base/routes.go
index 6a733bb..c8accb3 100644
--- a/plugins/base/routes.go
+++ b/plugins/base/routes.go
@@ -5,8 +5,10 @@ import (
	"fmt"
	"io"
	"io/ioutil"
	"log"
	"mime"
	"net/http"
	netmail "net/mail"
	"net/textproto"
	"net/url"
	"strconv"
@@ -564,22 +566,6 @@ func handleCompose(ctx *alps.Context, msg *OutgoingMessage, options *composeOpti
		return err
	}

	if msg.From == "" && strings.ContainsRune(ctx.Session.Username(), '@') {
		settings, err := loadSettings(ctx.Session.Store())
		if err != nil {
			return err
		}
		if settings.From != "" {
			addr := mail.Address{
				Name:    settings.From,
				Address: ctx.Session.Username(),
			}
			msg.From = addr.String()
		} else {
			msg.From = ctx.Session.Username()
		}
	}

	if ctx.Request().Method == http.MethodPost {
		formParams, err := ctx.FormParams()
		if err != nil {
@@ -594,6 +580,15 @@ func handleCompose(ctx *alps.Context, msg *OutgoingMessage, options *composeOpti
		msg.InReplyTo = ctx.FormValue("in_reply_to")
		msg.MessageID = ctx.FormValue("message_id")

		_, err = netmail.ParseAddress(msg.From)
		if err != nil {
			ibase.Global().Notice = "From address is not valid"
			return ctx.Render(http.StatusOK, "compose.html", &ComposeRenderData{
				IMAPBaseRenderData: *ibase,
				Message:            msg,
			})
		}

		form, err := ctx.MultipartForm()
		if err != nil {
			return fmt.Errorf("failed to get multipart form: %v", err)
@@ -705,6 +700,13 @@ func handleCompose(ctx *alps.Context, msg *OutgoingMessage, options *composeOpti
		} else {
			return submitCompose(ctx, msg, options)
		}
	} else {
		from, err := calculateFrom(ctx)
		if err == nil {
			msg.From = from
		} else {
			ibase.Global().Notice = "From address not set. Add a From address in settings."
		}
	}

	return ctx.Render(http.StatusOK, "compose.html", &ComposeRenderData{
@@ -713,6 +715,55 @@ func handleCompose(ctx *alps.Context, msg *OutgoingMessage, options *composeOpti
	})
}

func calculateFrom(ctx *alps.Context) (string, error) {
	settings, err := loadSettings(ctx.Session.Store())
	if err != nil {
		return "", err
	}
	// Try parsing the setting as a full RFC-5322 email address
	vfrom, err := netmail.ParseAddress(settings.From)
	if err == nil {
		log.Printf("Address in settings is a valid address, using %s", vfrom.String())
		return addFromAndString(vfrom, settings.From), nil
	}

	// Then try the username
	vfrom, err = netmail.ParseAddress(ctx.Session.Username())
	if err == nil {
		log.Printf("Username is valid email, using %s", vfrom.String())
		return addFromAndString(vfrom, settings.From), nil
	}

	// Then if we have the Host, try username@host
	host, err := ctx.Server.Host()
	log.Printf("Host is %s", host)
	if err == nil {
		vfrom, err = netmail.ParseAddress(ctx.Session.Username() + "@" + host)
		if err == nil {
			log.Printf("username@host is a valid email, using %s", vfrom.String())
			return addFromAndString(vfrom, settings.From), nil
		}
	}
	return "", fmt.Errorf("Unable to infer From address")
}

/// From setting might be just the human name, if that's the case
// then compose a full address from whatever email we inferred
// plus the 'From' setting as the name part
func addFromAndString(addr *netmail.Address, from string) string {
	// If from is a valid email it can't also be a name, just return what we were given
	if _, err := netmail.ParseAddress(from); err == nil || from == "" {
		return addr.String()
	}
	// Otherwise From is set and it's not a valid address by itself, interpret
	// it as a Name
	f := &netmail.Address{
		Name:    from,
		Address: addr.Address,
	}
	return f.String()
}

func handleComposeNew(ctx *alps.Context) error {
	text := ctx.QueryParam("body")
	settings, err := loadSettings(ctx.Session.Store())
diff --git a/server.go b/server.go
index c4386d9..159da9d 100644
--- a/server.go
+++ b/server.go
@@ -95,14 +95,20 @@ func (err *NoUpstreamError) Error() string {
// auto-discovery with URL.Host.
func (s *Server) Upstream(schemes ...string) (*url.URL, error) {
	var urls []*url.URL
	for _, scheme := range append(schemes, "") {
	for _, scheme := range schemes {
		u, ok := s.upstreams[scheme]
		if ok {
			urls = append(urls, u)
		}
	}
	if len(urls) == 0 {
		return nil, &NoUpstreamError{schemes}
		// Fallback to empty scheme for discovery
		u, ok := s.upstreams[""]
		if ok {
			return u, nil
		} else {
			return nil, &NoUpstreamError{schemes}
		}
	}
	if len(urls) > 1 {
		return nil, fmt.Errorf("multiple upstream servers are configured for schemes %v", schemes)
@@ -110,6 +116,11 @@ func (s *Server) Upstream(schemes ...string) (*url.URL, error) {
	return urls[0], nil
}

func (s *Server) Host() (string, error) {
	u, err := s.Upstream()
	return u.Host, err
}

func (s *Server) parseIMAPUpstream() error {
	u, err := s.Upstream("imap", "imaps", "imap+insecure")
	if err != nil {
diff --git a/themes/alps/settings.html b/themes/alps/settings.html
index 61e76e6..2c9d370 100644
--- a/themes/alps/settings.html
+++ b/themes/alps/settings.html
@@ -14,7 +14,7 @@
    <main class="settings">
      <form method="post">
        <div class="action-group">
          <label for="from">Full name</label>
          <label for="from">From</label>
          <input
            type="text"
            name="from"
-- 
2.32.0
Details
Message ID
<blf8iUSGjvb75H-BCodV38LemTjw2ft_ynKAWA6O1kYiZDNcvNFmpjdzcdgcXFo2yD8-awh4G1SvTwMCm1dYdLxAz57b_iWLNM-Lcl7-Ip4=@emersion.fr>
In-Reply-To
<20210731195910.18687-1-martin@ashbysoft.com> (view parent)
DKIM signature
pass
Download raw message
I'm not sure about this patch. The upstream is usually something like
mail.example.org, not example.org. This also breaks the use-case where
the username is already a valid e-mail address.

>  func (s *Server) Upstream(schemes ...string) (*url.URL, error) {
>  	var urls []*url.URL
> -	for _, scheme := range append(schemes, "") {
> +	for _, scheme := range schemes {

Sounds like an unrelated change.
Details
Message ID
<CDB1HERYJ067.2W39P8VGJJKH1@martin-n1517rd>
In-Reply-To
<blf8iUSGjvb75H-BCodV38LemTjw2ft_ynKAWA6O1kYiZDNcvNFmpjdzcdgcXFo2yD8-awh4G1SvTwMCm1dYdLxAz57b_iWLNM-Lcl7-Ip4=@emersion.fr> (view parent)
DKIM signature
missing
Download raw message
On Wed Aug 4, 2021 at 10:23 AM BST, Simon Ser wrote:
> I'm not sure about this patch. 

Ah I'm a little unsure of it myself :) Thanks for taking the time to
read and provide feedback.

> The upstream is usually something like
> mail.example.org, not example.org. 

According to the documentation it's the host I need to provide here, 
see https://git.sr.ht/~migadu/alps. This seems to suggest that one 
can either specify imap(s) and smtp(s) upstreams explicitly _or_ 
provide a _domain_ which is used for discovery with SRV records per
RFC-6168.

> This also breaks the use-case where
> the username is already a valid e-mail address.

Ah I must have made an error. I am currently running alps on 2 domains,
one where username is already a valid email, so I will test it there. I
might also add some unit test for the function which derives the From
address.

>
> >  func (s *Server) Upstream(schemes ...string) (*url.URL, error) {
> >  	var urls []*url.URL
> > -	for _, scheme := range append(schemes, "") {
> > +	for _, scheme := range schemes {
>
> Sounds like an unrelated change.

This is to facilitate providing both a host and also specifying the
upstream IMAP / SMTP servers, something like: 

./alps example.com imaps://mail.example.com smtps://mail.example.com

Without this change, the above command line simply crashes due to
thinking it has multiple imap/smtp servers specified.

I think it might actually be better to have explicit flags for these
rather than making assumptions. Especially since only one imap(s) or
smtp(s) upstream is supported. This would make the command line more
like:

# RFC-6168 discovery would be used
./alps --host example.com 

# discovery would be for smtp only
./alps --host example.com --imap imaps://imap.example.com 

# discovery is not used
./alps --host example.com --imap imaps://imap.example.com --smtp
smtps://smtp.example.com 

It might also make the handling simpler, no need to go through arguments
parsing them etc. What do you think?
Details
Message ID
<4EHdaag3LekM7MexlRK5KzGHzdTXWvWuTWCxOVMK6_Cb33kX4rmBQil650T7PtKWxWf98t419SfuN-FFJvpDCT_x2flKkCwNUK2ZsJ6LflY=@emersion.fr>
In-Reply-To
<CDB1HERYJ067.2W39P8VGJJKH1@martin-n1517rd> (view parent)
DKIM signature
pass
Download raw message
On Wednesday, August 4th, 2021 at 23:17, Martin Ashby <martin@ashbysoft.com> wrote:

> This is to facilitate providing both a host and also specifying the
> upstream IMAP / SMTP servers, something like:
>
> ./alps example.com imaps://mail.example.com smtps://mail.example.com
>
> Without this change, the above command line simply crashes due to
> thinking it has multiple imap/smtp servers specified.

Right. I'm not against making this work, but we should keep in mind the
edge cases. For instance specifying two IMAP servers should still be
disallowed.

Additionally, it's a very good practice for administrators to properly
setup discovery. This benefits all e-mail clients, not just alps.

> I think it might actually be better to have explicit flags for these
> rather than making assumptions. Especially since only one imap(s) or
> smtp(s) upstream is supported. This would make the command line more
> like:
>
> # RFC-6168 discovery would be used
> ./alps --host example.com
>
> # discovery would be for smtp only
> ./alps --host example.com --imap imaps://imap.example.com
>
> # discovery is not used
> ./alps --host example.com --imap imaps://imap.example.com --smtp
> smtps://smtp.example.com

(Discovery would still be used for CalDAV/CardDAV.)

> It might also make the handling simpler, no need to go through arguments
> parsing them etc. What do you think?

This seems a bit repetitive because the URL scheme already indicates whether a
host should only be used for IMAP or SMTP. Or am I missing something?
Reply to thread Export thread (mbox)