Fixed crash in when requesting recovery for a non registered email

README.md

@@ -31,7 +31,7 @@ There are two possibilities for building Coreander from source:
 * Otherwise, a simple `go build` or `go install` will do, although no version information will be added to the executable.
 
 ## How to use
-Coreander is designed to be run as a service managed by systemd or any other service manager. For example, in Raspberry Pi OS, just create a file called `/etc/systemd/system/coreander.service` with the following contents:
+Coreander is designed to be run as a service managed by [systemd](https://systemd.io) or any other service manager. For example, in Raspberry Pi OS, just create a file called `/etc/systemd/system/coreander.service` with the following contents:
 
 ```
 [Unit]
@@ -107,5 +107,7 @@ On first run, Coreander creates an admin user with the following credentials:
 * `MIN_PASSWORD_LENGTH`: Minimum length acceptable for passwords. Defaults to 5.
 * `WORDS_PER_MINUTE`: Defines a default words per minute reading speed that will be used for not logged-in users. Defaults to 250.
 * `SESSION_TIMEOUT`: Specifies the maximum time a user session may last, in hours. Floating-point values are allowed. Defaults to 24 hours.
+* `RECOVERY_TIMEOUT`: Specifies the maximum time a user recovery link may last, in hours. Floating-point values are allowed. Defaults to 2 hours.
 * `UPLOAD_DOCUMENT_MAX_SIZE`: Maximum document size allowed to be uploaded to the library, in megabytes. Set this to 0 to unlimit upload size. Defaults to 20 megabytes.
-* `HOSTNAME`: The name of the host the server is running on.
+* `HOSTNAME`: **Deprecated, use FQDN instead**.
+* `FQDN`: Domain name of the server. If Coreander is listening to a non-standard HTTP / HTTPS port, include it using a colon (e. g. example.com:3000). Defaults to `localhost`.

config.go

@@ -4,8 +4,10 @@ package main
 type Config struct {
 	// LibPath holds the absolute path to the folder containing the documents
 	LibPath string `env:"LIB_PATH" env-required:"true"`
-	// Hostname stores the name of the host the server is running on
+	// Deprecated. Use FQDN instead
 	Hostname string `env:"HOSTNAME" env-default:"localhost"`
+	// FQDN stores the domain name of the server. If the server is listening on a non-standard HTTP / HTTPS port, include it using a colon (e. g. :3000)
+	FQDN string `env:"FQDN" env-default:"localhost"`
 	// Port defines the port number in which the webserver listens for requests
 	Port int `env:"PORT" env-default:"3000"`
 	// BatchSize indicates the number of documents persisted by the indexer in one operation
@@ -32,6 +34,8 @@ type Config struct {
 	WordsPerMinute float64 `env:"WORDS_PER_MINUTE" env-default:"250"`
 	// SessionTimeout specifies the maximum time a user session may last in hours
 	SessionTimeout float64 `env:"SESSION_TIMEOUT" env-default:"24"`
+	// RecoveryTimeout specifies the maximum time a user recovery link may last in hours
+	RecoveryTimeout float64 `env:"RECOVERY_TIMEOUT" env-default:"2"`
 	// UploadDocumentMaxSize is the maximum document size allowed to be uploaded to the library, in megabytes.
 	// Set this to 0 to unlimit upload size. Defaults to 20 megabytes.
 	UploadDocumentMaxSize int `env:"UPLOAD_DOCUMENT_MAX_SIZE" env-default:"20"`

internal/webserver/authentication_test.go

@@ -1,21 +1,26 @@
 package webserver_test
 
 import (
+	"fmt"
 	"net/http"
 	"net/url"
 	"reflect"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/PuerkitoBio/goquery"
+	"github.com/gofiber/fiber/v2"
 	"github.com/spf13/afero"
+	"github.com/svera/coreander/v3/internal/webserver"
 	"github.com/svera/coreander/v3/internal/webserver/infrastructure"
 	"github.com/svera/coreander/v3/internal/webserver/model"
+	"gorm.io/gorm"
 )
 
 func TestAuthentication(t *testing.T) {
 	db := infrastructure.Connect("file::memory:", 250)
-	app := bootstrapApp(db, &infrastructure.SMTP{}, afero.NewMemMapFs())
+	app := bootstrapApp(db, &infrastructure.SMTP{}, afero.NewMemMapFs(), webserver.Config{})
 
 	data := url.Values{
 		"email":    {"admin@example.com"},
@@ -78,7 +83,7 @@ func TestAuthentication(t *testing.T) {
 
 func TestRecoverNoEmailService(t *testing.T) {
 	db := infrastructure.Connect("file::memory:?cache=shared", 250)
-	app := bootstrapApp(db, &infrastructure.NoEmail{}, afero.NewMemMapFs())
+	app := bootstrapApp(db, &infrastructure.NoEmail{}, afero.NewMemMapFs(), webserver.Config{})
 
 	req, err := http.NewRequest(http.MethodGet, "/en/recover", nil)
 	if err != nil {
@@ -94,15 +99,34 @@ func TestRecoverNoEmailService(t *testing.T) {
 }
 
 func TestRecover(t *testing.T) {
-	db := infrastructure.Connect("file::memory:?cache=shared", 250)
-	smtpMock := &SMTPMock{}
-	app := bootstrapApp(db, smtpMock, afero.NewMemMapFs())
+	var (
+		db       *gorm.DB
+		app      *fiber.App
+		data     url.Values
+		smtpMock *infrastructure.SMTPMock
+	)
+
+	reset := func(recoveryTimeout time.Duration) {
+		t.Helper()
+
+		webserverConfig := webserver.Config{
+			SessionTimeout:        24 * time.Hour,
+			RecoveryTimeout:       recoveryTimeout,
+			LibraryPath:           "fixtures/library",
+			UploadDocumentMaxSize: 1,
+		}
+		db = infrastructure.Connect("file::memory:?cache=shared", 250)
+		smtpMock = &infrastructure.SMTPMock{}
+		app = bootstrapApp(db, smtpMock, afero.NewMemMapFs(), webserverConfig)
 
-	data := url.Values{
-		"email": {"admin@example.com"},
+		data = url.Values{
+			"email": {"admin@example.com"},
+		}
 	}
 
-	t.Run("Check that login page is accessible", func(t *testing.T) {
+	t.Run("Check that recover page is accessible", func(t *testing.T) {
+		reset(2 * time.Hour)
+
 		req, err := http.NewRequest(http.MethodGet, "/en/recover", nil)
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
@@ -117,6 +141,8 @@ func TestRecover(t *testing.T) {
 	})
 
 	t.Run("Check that not posting an email returns an error", func(t *testing.T) {
+		reset(2 * time.Hour)
+
 		req, err := http.NewRequest(http.MethodPost, "/en/recover", nil)
 		req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
 		if err != nil {
@@ -147,15 +173,19 @@ func TestRecover(t *testing.T) {
 		}
 	})
 
-	t.Run("Check that posting an existing email sends a recovery email", func(t *testing.T) {
+	t.Run("Check that posting a non-existing email does not send an email", func(t *testing.T) {
+		reset(2 * time.Hour)
+
+		data = url.Values{
+			"email": {"unknown@example.com"},
+		}
+
 		req, err := http.NewRequest(http.MethodPost, "/en/recover", strings.NewReader(data.Encode()))
 		req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
-		smtpMock.wg.Add(1)
 		response, err := app.Test(req)
-		smtpMock.wg.Wait()
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
@@ -164,12 +194,14 @@ func TestRecover(t *testing.T) {
 			t.Errorf("Expected status %d, received %d", expectedStatus, response.StatusCode)
 		}
 
-		if !smtpMock.calledSend {
-			t.Error("Email service 'send' method not called")
+		if smtpMock.CalledSend() {
+			t.Error("Email service 'send' method called")
 		}
 	})
 
-	t.Run("Try to access the update password without the recovery ID from previous step", func(t *testing.T) {
+	t.Run("Try to access the update password without the recovery ID", func(t *testing.T) {
+		reset(2 * time.Hour)
+
 		req, err := http.NewRequest(http.MethodGet, "/en/reset-password", nil)
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
@@ -183,29 +215,30 @@ func TestRecover(t *testing.T) {
 		}
 	})
 
-	t.Run("Try to access the reset password page with a recovery ID", func(t *testing.T) {
-		adminUser := model.User{}
-		db.Where("email = ?", "admin@example.com").First(&adminUser)
+	t.Run("Check that posting an existing email sends a recovery email and resetting the password successfully redirects to the login page", func(t *testing.T) {
+		reset(2 * time.Hour)
 
-		req, err := http.NewRequest(http.MethodGet, "/en/reset-password", nil)
+		req, err := http.NewRequest(http.MethodPost, "/en/recover", strings.NewReader(data.Encode()))
+		req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
-
-		q := req.URL.Query()
-		q.Add("id", adminUser.RecoveryUUID)
-		req.URL.RawQuery = q.Encode()
-
+		smtpMock.Wg.Add(1)
 		response, err := app.Test(req)
+		smtpMock.Wg.Wait()
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
+
 		if expectedStatus := http.StatusOK; response.StatusCode != expectedStatus {
 			t.Errorf("Expected status %d, received %d", expectedStatus, response.StatusCode)
 		}
-	})
 
-	t.Run("Check that resetting the password successfully redirects to the login page", func(t *testing.T) {
+		if !smtpMock.CalledSend() {
+			t.Error("Email service 'send' method not called")
+		}
+
+		// resetting the password successfully redirects to the login page
 		adminUser := model.User{}
 		db.Where("email = ?", "admin@example.com").First(&adminUser)
 
@@ -215,12 +248,12 @@ func TestRecover(t *testing.T) {
 			"id":               {adminUser.RecoveryUUID},
 		}
 
-		req, err := http.NewRequest(http.MethodPost, "/en/reset-password", strings.NewReader(data.Encode()))
+		req, err = http.NewRequest(http.MethodPost, "/en/reset-password", strings.NewReader(data.Encode()))
 		req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
-		response, err := app.Test(req)
+		response, err = app.Test(req)
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
@@ -237,14 +270,11 @@ func TestRecover(t *testing.T) {
 		if expectedURL := "/en/login"; url.Path != expectedURL {
 			t.Errorf("Expected location %s, received %s", expectedURL, url.Path)
 		}
-	})
 
-	// Try to access again to the reset password page with the same recovery ID leads to an error
-	t.Run("Try to access again to the reset password page with the same recovery ID leads to an error", func(t *testing.T) {
-		adminUser := model.User{}
+		// Try to access again to the reset password page with the same recovery ID leads to an error
 		db.Where("email = ?", "admin@example.com").First(&adminUser)
 
-		req, err := http.NewRequest(http.MethodGet, "/en/reset-password", nil)
+		req, err = http.NewRequest(http.MethodGet, "/en/reset-password", nil)
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
@@ -253,10 +283,69 @@ func TestRecover(t *testing.T) {
 		q.Add("id", adminUser.RecoveryUUID)
 		req.URL.RawQuery = q.Encode()
 
+		response, err = app.Test(req)
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err.Error())
+		}
+		if expectedStatus := http.StatusBadRequest; response.StatusCode != expectedStatus {
+			t.Errorf("Expected status %d, received %d", expectedStatus, response.StatusCode)
+		}
+	})
+
+	t.Run("Check that using a timed out link returns an error", func(t *testing.T) {
+		reset(0 * time.Hour)
+
+		req, err := http.NewRequest(http.MethodPost, "/en/recover", strings.NewReader(data.Encode()))
+		req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err.Error())
+		}
+		smtpMock.Wg.Add(1)
 		response, err := app.Test(req)
+		smtpMock.Wg.Wait()
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err.Error())
+		}
+
+		if expectedStatus := http.StatusOK; response.StatusCode != expectedStatus {
+			t.Errorf("Expected status %d, received %d", expectedStatus, response.StatusCode)
+		}
+
+		// trying to access the reset password page using a time out ID returns an error
+		adminUser := model.User{}
+		db.Where("email = ?", "admin@example.com").First(&adminUser)
+
+		req, err = http.NewRequest(http.MethodGet, fmt.Sprintf("/en/reset-password?id=%s", adminUser.RecoveryUUID), nil)
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err.Error())
+		}
+
+		response, err = app.Test(req)
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err.Error())
 		}
+
+		if expectedStatus := http.StatusBadRequest; response.StatusCode != expectedStatus {
+			t.Errorf("Expected status %d, received %d", expectedStatus, response.StatusCode)
+		}
+
+		// trying to reset the password using a time out ID returns an error
+		data = url.Values{
+			"password":         {"newPass"},
+			"confirm-password": {"newPass"},
+			"id":               {adminUser.RecoveryUUID},
+		}
+
+		req, err = http.NewRequest(http.MethodPost, "/en/reset-password", strings.NewReader(data.Encode()))
+		req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err.Error())
+		}
+		response, err = app.Test(req)
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err.Error())
+		}
+
 		if expectedStatus := http.StatusBadRequest; response.StatusCode != expectedStatus {
 			t.Errorf("Expected status %d, received %d", expectedStatus, response.StatusCode)
 		}

internal/webserver/controller.go

@@ -29,6 +29,7 @@ func SetupControllers(cfg Config, db *gorm.DB, metadataReaders map[string]metada
 		Hostname:          cfg.Hostname,
 		Port:              cfg.Port,
 		SessionTimeout:    cfg.SessionTimeout,
+		RecoveryTimeout:   cfg.RecoveryTimeout,
 	}
 
 	usersCfg := user.Config{

internal/webserver/controller/auth/controller.go

@@ -30,6 +30,7 @@ type Config struct {
 	Hostname          string
 	Port              int
 	SessionTimeout    time.Duration
+	RecoveryTimeout   time.Duration
 }
 
 func NewController(repository authRepository, sender recoveryEmail, cfg Config, printers map[string]*message.Printer) *Controller {

internal/webserver/controller/auth/request.go

@@ -3,6 +3,7 @@ package auth
 import (
 	"fmt"
 	"net/mail"
+	"strconv"
 	"time"
 
 	"github.com/gofiber/fiber/v2"
@@ -22,9 +23,14 @@ func (a *Controller) Request(c *fiber.Ctx) error {
 		}, "layout")
 	}
 
-	if user, err := a.repository.FindByEmail(c.FormValue("email")); err == nil {
+	user, err := a.repository.FindByEmail(c.FormValue("email"))
+	if err != nil {
+		return fiber.ErrInternalServerError
+	}
+
+	if user != nil {
 		user.RecoveryUUID = uuid.NewString()
-		user.RecoveryValidUntil = time.Now().Add(a.config.SessionTimeout)
+		user.RecoveryValidUntil = time.Now().UTC().Add(a.config.RecoveryTimeout)
 		if err := a.repository.Update(user); err != nil {
 			return fiber.ErrInternalServerError
 		}
@@ -36,11 +42,12 @@ func (a *Controller) Request(c *fiber.Ctx) error {
 			user.RecoveryUUID,
 		)
 		c.Render("auth/email", fiber.Map{
-			"Lang":         c.Params("lang"),
-			"RecoveryLink": recoveryLink,
+			"Lang":            c.Params("lang"),
+			"RecoveryLink":    recoveryLink,
+			"RecoveryTimeout": strconv.FormatFloat(a.config.RecoveryTimeout.Hours(), 'f', -1, 64),
 		})
 
-		go a.sender.Send(
+		return a.sender.Send(
 			c.FormValue("email"),
 			a.printers[c.Params("lang")].Sprintf("Password recovery request"),
 			string(c.Response().Body()),