feat: add ability to force email verification (#1837)

backend/internal/usersignup/cleanup_job.go

@@ -0,0 +1,113 @@
+package usersignup
+
+import (
+	"context"
+	"fmt"
+	"log/slog"
+	"time"
+
+	"github.com/italypaleale/francis/builtin/cronjob"
+	"gorm.io/gorm"
+
+	"github.com/pocket-id/pocket-id/backend/internal/appconfig"
+	"github.com/pocket-id/pocket-id/backend/internal/model"
+	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
+)
+
+const (
+	// cleanupJobInterval is how often overdue signups are deleted, which bounds how long an account can outlive its deadline
+	cleanupJobInterval = time.Hour
+	// cleanupJobJitter spreads each occurrence around its scheduled time, so it doesn't coincide with the other cleanup jobs
+	cleanupJobJitter = 5 * time.Minute
+)
+
+// UserDeleter deletes a user together with everything that references it
+type UserDeleter interface {
+	DeleteUser(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string, allowLdapDelete bool) error
+}
+
+type cleanupJob struct {
+	db        *gorm.DB
+	appConfig appconfig.AppConfigResolver
+	users     UserDeleter
+}
+
+func newCleanupJob(db *gorm.DB, appConfig appconfig.AppConfigResolver, users UserDeleter) (*cronjob.CronJob, error) {
+	job := &cleanupJob{
+		db:        db,
+		appConfig: appConfig,
+		users:     users,
+	}
+
+	cronActor, err := cronjob.New(
+		"DeleteUnverifiedSignups",
+		cronjob.WithJob(job.deleteUnverifiedSignups),
+		cronjob.WithInterval(cleanupJobInterval),
+		cronjob.WithJitter(cleanupJobJitter),
+		// Also run right after the job is first registered, so accounts that missed their deadline while Pocket ID wasn't running are removed at startup
+		cronjob.WithImmediate(),
+		cronjob.WithLogger(slog.Default()),
+	)
+	if err != nil {
+		return nil, fmt.Errorf("error creating unverified signup cleanup cron job: %w", err)
+	}
+
+	return cronActor, nil
+}
+
+// deleteUnverifiedSignups deletes self-signed-up accounts that did not verify their email before their deadline
+func (j *cleanupJob) deleteUnverifiedSignups(ctx context.Context) error {
+	dbConfig, err := j.appConfig.GetConfig(ctx)
+	if err != nil {
+		return fmt.Errorf("error loading app config: %w", err)
+	}
+
+	// Deadlines only apply while verification is required
+	// Clearing them otherwise keeps accounts that were used in the meantime from being deleted once verification is required again
+	if dbConfig.EmailVerification.String() != appconfig.EmailVerificationRequired {
+		st := j.db.
+			WithContext(ctx).
+			Model(&model.User{}).
+			Where("email_verification_deadline IS NOT NULL").
+			Update("email_verification_deadline", nil)
+		if st.Error != nil {
+			return fmt.Errorf("failed to clear email verification deadlines: %w", st.Error)
+		}
+		if st.RowsAffected > 0 {
+			slog.InfoContext(ctx, "Cleared email verification deadlines because verification is no longer required", slog.Int64("count", st.RowsAffected))
+		}
+		return nil
+	}
+
+	// Load the overdue accounts
+	// Admins are exempt from email verification, so an account promoted to admin before its deadline is kept
+	var userIDs []string
+	err = j.db.
+		WithContext(ctx).
+		Model(&model.User{}).
+		Where("email_verification_deadline < ? AND email_verified = ? AND is_admin = ?", datatype.DateTime(time.Now()), false, false).
+		Pluck("id", &userIDs).
+		Error
+	if err != nil {
+		return fmt.Errorf("failed to list accounts that missed their email verification deadline: %w", err)
+	}
+	if len(userIDs) == 0 {
+		return nil
+	}
+
+	// Delete each account through the user service so its client sessions, SCIM peers and profile picture are cleaned up as well
+	// A failure is logged and the remaining accounts are still processed
+	deleted := 0
+	for _, userID := range userIDs {
+		err = j.users.DeleteUser(ctx, dbConfig, userID, false)
+		if err != nil {
+			slog.ErrorContext(ctx, "Failed to delete an account that missed its email verification deadline", slog.String("userID", userID), slog.Any("error", err))
+			continue
+		}
+		deleted++
+	}
+
+	slog.InfoContext(ctx, "Deleted accounts that missed their email verification deadline", slog.Int("count", deleted))
+
+	return nil
+}

backend/internal/usersignup/cleanup_job_test.go

@@ -0,0 +1,76 @@
+package usersignup
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+
+	"github.com/pocket-id/pocket-id/backend/internal/appconfig"
+	"github.com/pocket-id/pocket-id/backend/internal/model"
+	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
+	testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing"
+)
+
+type testUserDeleter struct {
+	db      *gorm.DB
+	deleted []string
+}
+
+func (d *testUserDeleter) DeleteUser(ctx context.Context, _ *appconfig.AppConfigModel, userID string, _ bool) error {
+	d.deleted = append(d.deleted, userID)
+	return d.db.WithContext(ctx).Delete(&model.User{}, "id = ?", userID).Error
+}
+
+func TestCleanupJobDeletesUnverifiedSignups(t *testing.T) {
+	// seed creates users whose deadline lies in the past or future, covering every condition the job checks
+	seed := func(t *testing.T, db *gorm.DB) {
+		t.Helper()
+
+		past := new(datatype.DateTime(time.Now().Add(-time.Minute)))
+		future := new(datatype.DateTime(time.Now().Add(time.Hour)))
+		users := []model.User{
+			{Base: model.Base{ID: "overdue"}, Username: "overdue", EmailVerificationDeadline: past},
+			{Base: model.Base{ID: "not-yet-due"}, Username: "not-yet-due", EmailVerificationDeadline: future},
+			{Base: model.Base{ID: "promoted-admin"}, Username: "promoted-admin", IsAdmin: true, EmailVerificationDeadline: past},
+			{Base: model.Base{ID: "verified"}, Username: "verified", EmailVerified: true, EmailVerificationDeadline: past},
+			{Base: model.Base{ID: "existing"}, Username: "existing"},
+		}
+		require.NoError(t, db.Create(&users).Error)
+	}
+
+	newJob := func(db *gorm.DB, mode string) (*cleanupJob, *testUserDeleter) {
+		cfg := appconfig.NewTestConfig(nil)
+		cfg.EmailVerification = appconfig.AppConfigValue(mode)
+		deleter := &testUserDeleter{db: db}
+		return &cleanupJob{db: db, appConfig: appconfig.StaticResolver{Config: cfg}, users: deleter}, deleter
+	}
+
+	t.Run("deletes only overdue unverified regular users when verification is required", func(t *testing.T) {
+		db := testutils.NewDatabaseForTest(t)
+		seed(t, db)
+		job, deleter := newJob(db, appconfig.EmailVerificationRequired)
+
+		require.NoError(t, job.deleteUnverifiedSignups(t.Context()))
+
+		require.Equal(t, []string{"overdue"}, deleter.deleted)
+		var remaining []string
+		require.NoError(t, db.Model(&model.User{}).Order("id").Pluck("id", &remaining).Error)
+		require.Equal(t, []string{"existing", "not-yet-due", "promoted-admin", "verified"}, remaining)
+	})
+
+	t.Run("clears every deadline instead of deleting when verification is not required", func(t *testing.T) {
+		db := testutils.NewDatabaseForTest(t)
+		seed(t, db)
+		job, deleter := newJob(db, appconfig.EmailVerificationOptional)
+
+		require.NoError(t, job.deleteUnverifiedSignups(t.Context()))
+
+		require.Empty(t, deleter.deleted)
+		var withDeadline int64
+		require.NoError(t, db.Model(&model.User{}).Where("email_verification_deadline IS NOT NULL").Count(&withDeadline).Error)
+		require.Zero(t, withDeadline)
+	})
+}

backend/internal/usersignup/module.go

@@ -29,6 +29,10 @@ type UserCreator interface {
 	CreateUserInternal(ctx context.Context, dbConfig *appconfig.AppConfigModel, input dto.UserCreateDto, isLdapSync bool, tx *gorm.DB) (model.User, error)
 }
 
+type EmailVerificationSender interface {
+	Send(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string) error
+}
+
 // ScimSyncScheduler schedules SCIM after the signup transaction has committed
 type ScimSyncScheduler interface {
 	ScheduleSync(ctx context.Context)
@@ -43,6 +47,12 @@ type Dependencies struct {
 	UserCreator UserCreator
 	AppConfig   appconfig.AppConfigResolver
 	ScimSync    ScimSyncScheduler
+	UserDeleter UserDeleter
+
+	EmailVerification EmailVerificationSender
+
+	// CleanupDisabled skips registering the cron job that deletes signups that missed their email verification deadline, for example in tests
+	CleanupDisabled bool
 }
 
 type Module struct {
@@ -58,6 +68,19 @@ func New(deps Dependencies) (*Module, error) {
 		return nil, fmt.Errorf("error registering the %s actor: %w", SignupTokenActorType, err)
 	}
 
+	// Register the job that deletes self-signed-up accounts that did not verify their email in time
+	if !deps.CleanupDisabled {
+		job, err := newCleanupJob(deps.DB, deps.AppConfig, deps.UserDeleter)
+		if err != nil {
+			return nil, err
+		}
+
+		err = deps.Actors.RegisterBuiltInActor(job)
+		if err != nil {
+			return nil, fmt.Errorf("error registering unverified signup cleanup cron actor: %w", err)
+		}
+	}
+
 	service := newService(deps, deps.Actors.Service())
 	return &Module{
 		service: service,

backend/internal/usersignup/service.go

@@ -17,6 +17,7 @@ import (
 	"github.com/pocket-id/pocket-id/backend/internal/auditlogs"
 	"github.com/pocket-id/pocket-id/backend/internal/common"
 	"github.com/pocket-id/pocket-id/backend/internal/dto"
+	"github.com/pocket-id/pocket-id/backend/internal/emailverification"
 	"github.com/pocket-id/pocket-id/backend/internal/model"
 	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
 	"github.com/pocket-id/pocket-id/backend/internal/utils"
@@ -26,23 +27,28 @@ import (
 // It must match the value emitted by the JWT service in the access token's "amr" claim
 const authenticationMethodOneTimePassword = "otp"
 
+// signupVerificationPeriod is how long a self-signed-up account has to verify its email before it is deleted
+const signupVerificationPeriod = emailverification.TokenLifetime
+
 type Service struct {
-	db           *gorm.DB
-	actorService *actor.Service
-	userCreator  UserCreator
-	signer       TokenService
-	auditLog     AuditLogger
-	scimSync     ScimSyncScheduler
+	db                *gorm.DB
+	actorService      *actor.Service
+	userCreator       UserCreator
+	signer            TokenService
+	auditLog          AuditLogger
+	scimSync          ScimSyncScheduler
+	emailVerification EmailVerificationSender
 }
 
 func newService(deps Dependencies, actorService *actor.Service) *Service {
 	return &Service{
-		db:           deps.DB,
-		actorService: actorService,
-		userCreator:  deps.UserCreator,
-		signer:       deps.Signer,
-		auditLog:     deps.AuditLog,
-		scimSync:     deps.ScimSync,
+		db:                deps.DB,
+		actorService:      actorService,
+		userCreator:       deps.UserCreator,
+		signer:            deps.Signer,
+		auditLog:          deps.AuditLog,
+		scimSync:          deps.ScimSync,
+		emailVerification: deps.EmailVerification,
 	}
 }
 
@@ -94,6 +100,13 @@ func (s *Service) SignUp(ctx context.Context, config *appconfig.AppConfigModel,
 		return model.User{}, "", err
 	}
 
+	if emailverification.IsPending(config, &user) {
+		err = s.emailVerification.Send(ctx, config, user.ID)
+		if err != nil {
+			slog.WarnContext(ctx, "Failed to send the email verification message after signup", slog.String("userID", user.ID), slog.Any("error", err))
+		}
+	}
+
 	return user, accessToken, nil
 }
 
@@ -110,6 +123,16 @@ func (s *Service) createSignedUpUser(ctx context.Context, config *appconfig.AppC
 		return model.User{}, "", err
 	}
 
+	// Give the account a deadline to verify its email, after which it is deleted so abandoned signups cannot squat an address
+	if emailverification.IsPending(config, &user) {
+		deadline := datatype.DateTime(time.Now().Add(signupVerificationPeriod))
+		err = tx.WithContext(ctx).Model(&user).Update("email_verification_deadline", deadline).Error
+		if err != nil {
+			return model.User{}, "", err
+		}
+		user.EmailVerificationDeadline = &deadline
+	}
+
 	accessToken, err := s.signer.GenerateAccessToken(user, "", config.SessionDuration.AsDurationMinutes())
 	if err != nil {
 		return model.User{}, "", err

backend/internal/usersignup/service_test.go

@@ -30,6 +30,25 @@ func (f fakeUserCreator) CreateUserInternal(_ context.Context, _ *appconfig.AppC
 	return f.user, nil
 }
 
+// dbUserCreator stores the user in the signup transaction, so changes made later in that transaction can be asserted
+type dbUserCreator struct{}
+
+func (dbUserCreator) CreateUserInternal(_ context.Context, _ *appconfig.AppConfigModel, input dto.UserCreateDto, _ bool, tx *gorm.DB) (model.User, error) {
+	user := model.User{Username: input.Username, Email: input.Email, EmailVerified: input.EmailVerified}
+	err := tx.Create(&user).Error
+	return user, err
+}
+
+type fakeEmailVerificationSender struct {
+	err     error
+	userIDs []string
+}
+
+func (f *fakeEmailVerificationSender) Send(_ context.Context, _ *appconfig.AppConfigModel, userID string) error {
+	f.userIDs = append(f.userIDs, userID)
+	return f.err
+}
+
 type fakeSigner struct{}
 
 func (fakeSigner) GenerateAccessToken(_ model.User, _ string, _ time.Duration) (string, error) {
@@ -164,3 +183,56 @@ func listAllOptions() utils.ListRequestOptions {
 	opts.Pagination.Limit = 100
 	return opts
 }
+
+func TestSignUpRequiresEmailVerification(t *testing.T) {
+	signUp := func(t *testing.T, mode string, sender *fakeEmailVerificationSender) (*gorm.DB, model.User) {
+		t.Helper()
+
+		db := testutils.NewDatabaseForTest(t)
+		svc := newSignupServiceForTest(t, db, dbUserCreator{})
+		svc.emailVerification = sender
+
+		config := appconfig.NewTestConfig(nil)
+		config.AllowUserSignups = "open"
+		config.EmailVerification = appconfig.AppConfigValue(mode)
+		user, _, err := svc.SignUp(t.Context(), config, signUpDto{
+			Username: "newuser",
+			Email:    new("newuser@example.com"),
+		}, "1.2.3.4", "test-agent")
+		require.NoError(t, err)
+
+		return db, user
+	}
+
+	t.Run("sets a deadline and sends the verification email when verification is required", func(t *testing.T) {
+		sender := &fakeEmailVerificationSender{}
+		before := time.Now()
+		db, user := signUp(t, appconfig.EmailVerificationRequired, sender)
+
+		require.NotNil(t, user.EmailVerificationDeadline)
+		require.WithinDuration(t, before.Add(24*time.Hour), user.EmailVerificationDeadline.ToTime(), time.Minute)
+
+		var stored model.User
+		require.NoError(t, db.First(&stored, "id = ?", user.ID).Error)
+		require.NotNil(t, stored.EmailVerificationDeadline)
+		require.Equal(t, []string{user.ID}, sender.userIDs)
+	})
+
+	t.Run("keeps the account without a deadline when verification is optional", func(t *testing.T) {
+		sender := &fakeEmailVerificationSender{}
+		db, user := signUp(t, appconfig.EmailVerificationOptional, sender)
+
+		var stored model.User
+		require.NoError(t, db.First(&stored, "id = ?", user.ID).Error)
+		require.Nil(t, stored.EmailVerificationDeadline)
+		require.Empty(t, sender.userIDs)
+	})
+
+	t.Run("a failed delivery does not fail the signup", func(t *testing.T) {
+		sender := &fakeEmailVerificationSender{err: errors.New("smtp unavailable")}
+		_, user := signUp(t, appconfig.EmailVerificationRequired, sender)
+
+		require.NotNil(t, user.EmailVerificationDeadline)
+		require.Equal(t, []string{user.ID}, sender.userIDs)
+	})
+}