If accounts.conf contains an invalid maildir url, return a nice
error instead of panicing.

Check a couple of different error cases to provide extra
information about the error to the user.
---
 worker/maildir/container.go | 26 +++++++++++++++++++++++---
 worker/maildir/worker.go    | 17 ++++++++++++++++-
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/worker/maildir/container.go b/worker/maildir/container.go
index cd9a447..170e917 100644
--- a/worker/maildir/container.go
+++ b/worker/maildir/container.go
@@ -1,6 +1,7 @@
 package maildir
 
 import (
+	"errors"
 	"fmt"
 	"log"
 	"os"
@@ -21,15 +22,34 @@ type Container struct {
 }
 
 // NewContainer creates a new container at the specified directory
-// TODO: return an error if the provided directory is not accessible
-func NewContainer(dir string, l *log.Logger) *Container {
-	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}
+func NewContainer(dir string, l *log.Logger) (*Container, error) {
+	if len(dir) == 0 {
+		return nil, errors.New("No maildir specified")
+	}
+	f, err := os.Open(dir)
+	if err != nil {
+		if os.IsPermission(err) {
+			return nil, fmt.Errorf("No permissions for '%s'", dir)
+		} else {
+			return nil, fmt.Errorf("Unable to open directory '%s'", dir)
+		}
+	}
+	s, err := f.Stat()
+	f.Close()
+	if !s.IsDir() {
+		return nil, fmt.Errorf("Given maildir '%s' not a directory", dir)
+	}
+	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}, nil
 }
 
 // ListFolders returns a list of maildir folders in the container
 func (c *Container) ListFolders() ([]string, error) {
 	folders := []string{}
 	err := filepath.Walk(c.dir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return fmt.Errorf("Invalid path '%s': error: %v", path, err)
+
+		}
 		if !info.IsDir() {
 			return nil
 		}
diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
index ce548ff..d1ff3c2 100644
--- a/worker/maildir/worker.go
+++ b/worker/maildir/worker.go
@@ -1,6 +1,7 @@
 package maildir
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"net/url"
@@ -221,7 +222,15 @@ func (w *Worker) handleConfigure(msg *types.Configure) error {
 		}
 		dir = filepath.Join(home, u.Path)
 	}
-	w.c = NewContainer(dir, w.worker.Logger)
+	if len(dir) == 0 {
+		return fmt.Errorf("could not resolve maildir from URL '%s'", msg.Config.Source)
+	}
+	c, err := NewContainer(dir, w.worker.Logger)
+	if err != nil {
+		w.worker.Logger.Printf("could not configure maildir: %s", dir)
+		return err
+	}
+	w.c = c
 	w.worker.Logger.Printf("configured base maildir: %s", dir)
 	return nil
 }
@@ -231,6 +240,12 @@ func (w *Worker) handleConnect(msg *types.Connect) error {
 }
 
 func (w *Worker) handleListDirectories(msg *types.ListDirectories) error {
+	// TODO If handleConfigure has returned error, w.c is nil.
+	// It could be better if we skip directory listing completely
+	// when configure fails.
+	if w.c == nil {
+		return errors.New("Incorrect maildir directory")
+	}
 	dirs, err := w.c.ListFolders()
 	if err != nil {
 		w.worker.Logger.Printf("error listing directories: %v", err)
-- 
2.27.0

Hi,
Thanks for the patch, some comments:

On Mon, Jul 20, 2020 at 10:31:10AM +0300, Tero Koskinen wrote:
> If accounts.conf contains an invalid maildir url, return a nice
> error instead of panicing.
typo, panicking
> @@ -21,15 +22,34 @@ type Container struct {
>  }
>  
>  // NewContainer creates a new container at the specified directory
> -// TODO: return an error if the provided directory is not accessible
> -func NewContainer(dir string, l *log.Logger) *Container {
> -	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}
> +func NewContainer(dir string, l *log.Logger) (*Container, error) {
> +	if len(dir) == 0 {
> +		return nil, errors.New("No maildir specified")
> +	}
No need to check for an empty string here, do it in handleConfigure
> +	f, err := os.Open(dir)
> +	if err != nil {
> +		if os.IsPermission(err) {
> +			return nil, fmt.Errorf("No permissions for '%s'", dir)
> +		} else {
> +			return nil, fmt.Errorf("Unable to open directory '%s'", dir)
> +		}
> +	}
> +	s, err := f.Stat()
> +	f.Close()
> +	if !s.IsDir() {
> +		return nil, fmt.Errorf("Given maildir '%s' not a directory", dir)
> +	}

Use os.Stat(dir), you don't need to open the directory to check if it exists.
You can use something like
```
fi, err := os.Stat(dir)
if err != nil {
	return nil, err // no need to check for a permission error...
	                // it's self explaining if it happens
					// output of the error = "stat $dir: permission denied"
}
mode := fi.Mode()
canRW := mode&0600 != 0 // if what you actually want is check r+w access

fi.IsDir() // still works as well
```

> +	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}, nil
>  }
> diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
> index ce548ff..d1ff3c2 100644
> --- a/worker/maildir/worker.go
> +++ b/worker/maildir/worker.go
> @@ -1,6 +1,7 @@
>  package maildir
>  
>  import (
> +	"errors"
>  	"fmt"
>  	"io"
>  	"net/url"
> @@ -221,7 +222,15 @@ func (w *Worker) handleConfigure(msg *types.Configure) error {
>  		}
>  		dir = filepath.Join(home, u.Path)
>  	}
> -	w.c = NewContainer(dir, w.worker.Logger)
> +	if len(dir) == 0 {
> +		return fmt.Errorf("could not resolve maildir from URL '%s'", msg.Config.Source)
> +	}

Please do the check len(dir) != 0 only once, here is fine then NewContainer is
never called with an empty string as input.

Cheers,
Reto

On Wed, Jul 22, 2020 at 09:03:19PM +0200, Reto wrote:
> mode := fi.Mode()
> canRW := mode&0600 != 0 // if what you actually want is check r+w access
> 
> fi.IsDir() // still works as well
> ```

Correction, should be

```
mode := fi.Mode() & os.ModePerm
canRW := mode&0600 == 0600
```

else you get a wrong result if you have have only of the modes... plus I forgot
to mask to only the permission bits, my bad.

Cheers,
Reto

Hi,

Thanks for comments.

On Wed, 22 Jul 2020 21:03:19 +0200
Reto <reto@labrat.space> wrote:

> Hi,
> Thanks for the patch, some comments:
> 
> On Mon, Jul 20, 2020 at 10:31:10AM +0300, Tero Koskinen wrote:
> > error instead of panicing.
> typo, panicking

I'll fix this.

> > +func NewContainer(dir string, l *log.Logger) (*Container, error) {
> > +	if len(dir) == 0 {
> > +		return nil, errors.New("No maildir specified")
> > +	}
> No need to check for an empty string here, do it in handleConfigure

And I'll remove the extra check also from here.

> > +	f, err := os.Open(dir)
> > +	if err != nil {
> > +		if os.IsPermission(err) {
> > +			return nil, fmt.Errorf("No permissions for '%s'", dir)
> > +		} else {
> > +			return nil, fmt.Errorf("Unable to open directory '%s'", dir)
> > +		}
> > +	}
> > +	s, err := f.Stat()
> > +	f.Close()
> > +	if !s.IsDir() {
> > +		return nil, fmt.Errorf("Given maildir '%s' not a directory", dir)
> > +	}
> 
> Use os.Stat(dir), you don't need to open the directory to check if it exists.
> You can use something like
> ```
> fi, err := os.Stat(dir)
> if err != nil {
> 	return nil, err // no need to check for a permission error...
> 	                // it's self explaining if it happens
> 					// output of the error = "stat $dir: permission denied"
> }
> mode := fi.Mode()
> canRW := mode&0600 != 0 // if what you actually want is check r+w access
> 
> fi.IsDir() // still works as well
> ```

I though about this, but ended up to my current solution because:

- stat works for directory 'foo/Mail' when user has rights for 'foo',
  but not for 'foo/Mail' (=> no error even if no permissions)
- stat mode check for user 'rw' rights returns the bits for the owner
  of the file/dir, which might be different from the current user.
- Relying on file/dir permission bits complicates the logic as one
  would need to check the UID of the user invoking aerc and compare
  it to the file/dir owner UID. It is easier to simply try to open
  the file/dir.

My solution doesn't actually check for write permissions (only read).
I am not sure is it a good or bad - in theory user could have his
maildir on read-only media, but that might be quite rare case.

Yours,
 Tero

On Thu, Jul 23, 2020 at 09:00:02PM +0300, Tero Koskinen wrote:
> I though about this, but ended up to my current solution because:
> - stat works for directory 'foo/Mail' when user has rights for 'foo',
>   but not for 'foo/Mail' (=> no error even if no permissions)
> - stat mode check for user 'rw' rights returns the bits for the owner
>   of the file/dir, which might be different from the current user.
> - Relying on file/dir permission bits complicates the logic as one
>   would need to check the UID of the user invoking aerc and compare
>   it to the file/dir owner UID. It is easier to simply try to open
>   the file/dir.

Fair enough.
Let's do it your way, but I'd still remove the check if it's a permission error if
all you do is add more text to the log...
If you special case it then it's a different story naturally.
The error from stat is quite descriptive I think.

Cheers,
Reto

Hi,

On Fri, 24 Jul 2020 09:48:35 +0200
Reto <reto@labrat.space> wrote:

> On Thu, Jul 23, 2020 at 09:00:02PM +0300, Tero Koskinen wrote:
> > I though about this, but ended up to my current solution because:
> > ...
> 
> Fair enough.
> Let's do it your way, but I'd still remove the check if it's a permission error if
> all you do is add more text to the log...

I sent the v2 version of the patch. It should contain the typo fix, have the extra
nil check removed, and logging simplified a little.

When accounts.conf is following:
```
[P1]
source   = maildir:///etc/acme
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P2]
source   = maildir://Maildir
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P3]
source   = maildir:///home/tkoskine/Mail
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P4]
source   = maildir:///home/tkoskine/mbox
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P5]
source   = maildir://Mail
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P6]
source   = maildir:Mail
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent
```

log entries are:
```
~/src/cvs/aerc $ grep -v "(ui)" alog.txt   
2020/07/24 22:41:27 Starting up aerc
2020/07/24 22:41:27 Initializing PGP keyring
2020/07/24 22:41:27 Starting Unix server
2020/07/24 22:41:27 Failed to start Unix server: listen unix /home/tkoskine/.runtime/aerc.sock: bind: address already in use (non-fatal)
2020/07/24 22:41:27 configured base maildir: /home/tkoskine/Mail
2020/07/24 22:41:27 could not configure maildir: /etc/acme
2020/07/24 22:41:27 could not resolve maildir from URL 'maildir:Mail'
2020/07/24 22:41:27 open /etc/acme: permission denied
2020/07/24 22:41:27 could not resolve maildir from URL 'maildir://Maildir'
2020/07/24 22:41:27 could not resolve maildir from URL 'maildir://Mail'
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 could not configure maildir: /home/tkoskine/mbox
2020/07/24 22:41:27 Given maildir '/home/tkoskine/mbox' not a directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Connected.
2020/07/24 22:41:27 opening INBOX
~/src/cvs/aerc $ 
```


Yours,
 Tero