Merge pull request #292 from databasus/develop

FIX (postgresql): Do not throw an error over read-only user creation …

backend/internal/features/databases/databases/postgresql/model.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/jackc/pgx/v5"
+	"github.com/jackc/pgx/v5/pgconn"
 	"gorm.io/gorm"
 )
 
@@ -394,10 +395,13 @@ func (p *PostgresqlDatabase) IsUserReadOnly(
 //
 // This method performs the following operations atomically in a single transaction:
 // 1. Creates a PostgreSQL user with a UUID-based password
-// 2. Grants CONNECT privilege on the database
-// 3. Grants USAGE on all non-system schemas
-// 4. Grants SELECT on all existing tables and sequences
-// 5. Sets default privileges for future tables and sequences
+// 2. Revokes CREATE privilege on public schema from PUBLIC role
+// 3. Grants CONNECT privilege on the database
+// 4. Discovers all user-created schemas
+// 5. Grants USAGE on all non-system schemas
+// 6. Grants SELECT on all existing tables and sequences
+// 7. Sets default privileges for future tables and sequences
+// 8. Verifies user creation before committing
 //
 // Security features:
 // - Username format: "databasus-{8-char-uuid}" for uniqueness
@@ -487,33 +491,56 @@ func (p *PostgresqlDatabase) CreateReadOnlyUser(
 			return "", "", fmt.Errorf("failed to create user: %w", err)
 		}
 
-		// Step 1.5: Revoke CREATE privilege from PUBLIC role on public schema
+		// Step 2: Check if public schema exists and revoke CREATE privilege if it does
 		// This is necessary because all PostgreSQL users inherit CREATE privilege on the
 		// public schema through the PUBLIC role. This is a one-time operation that affects
 		// the entire database, making it more secure by default.
 		// Note: This only affects the public schema; other schemas are unaffected.
-		_, err = tx.Exec(ctx, `REVOKE CREATE ON SCHEMA public FROM PUBLIC`)
-		if err != nil {
-			logger.Error("Failed to revoke CREATE on public from PUBLIC", "error", err)
-			if !strings.Contains(err.Error(), "schema \"public\" does not exist") &&
-				!strings.Contains(err.Error(), "permission denied") {
-				return "", "", fmt.Errorf("failed to revoke CREATE from PUBLIC: %w", err)
-			}
-		}
-
-		// Now revoke from the specific user as well (belt and suspenders)
-		_, err = tx.Exec(ctx, fmt.Sprintf(`REVOKE CREATE ON SCHEMA public FROM "%s"`, baseUsername))
-		if err != nil {
-			logger.Error(
-				"Failed to revoke CREATE on public schema from user",
-				"error",
-				err,
-				"username",
-				baseUsername,
+		var publicSchemaExists bool
+		err = tx.QueryRow(ctx, `
+			SELECT EXISTS(
+				SELECT 1 FROM information_schema.schemata 
+				WHERE schema_name = 'public'
 			)
+		`).Scan(&publicSchemaExists)
+		if err != nil {
+			return "", "", fmt.Errorf("failed to check if public schema exists: %w", err)
 		}
 
-		// Step 2: Grant database connection privilege and revoke TEMP
+		if publicSchemaExists {
+			// Revoke CREATE from PUBLIC role (affects all users)
+			_, err = tx.Exec(ctx, `REVOKE CREATE ON SCHEMA public FROM PUBLIC`)
+			if err != nil {
+				if strings.Contains(err.Error(), "permission denied") {
+					logger.Warn(
+						"Failed to revoke CREATE on public from PUBLIC (permission denied)",
+						"error",
+						err,
+					)
+				} else {
+					return "", "", fmt.Errorf("failed to revoke CREATE from PUBLIC on existing public schema: %w", err)
+				}
+			}
+
+			// Now revoke from the specific user as well (belt and suspenders)
+			_, err = tx.Exec(
+				ctx,
+				fmt.Sprintf(`REVOKE CREATE ON SCHEMA public FROM "%s"`, baseUsername),
+			)
+			if err != nil {
+				logger.Warn(
+					"Failed to revoke CREATE on public schema from user",
+					"error",
+					err,
+					"username",
+					baseUsername,
+				)
+			}
+		} else {
+			logger.Info("Public schema does not exist, skipping CREATE privilege revocation")
+		}
+
+		// Step 3: Grant database connection privilege and revoke TEMP
 		_, err = tx.Exec(
 			ctx,
 			fmt.Sprintf(`GRANT CONNECT ON DATABASE "%s" TO "%s"`, *p.Database, baseUsername),
@@ -537,7 +564,7 @@ func (p *PostgresqlDatabase) CreateReadOnlyUser(
 			logger.Warn("Failed to revoke TEMP privilege", "error", err, "username", baseUsername)
 		}
 
-		// Step 3: Discover all user-created schemas
+		// Step 4: Discover all user-created schemas
 		rows, err := tx.Query(ctx, `
 			SELECT schema_name
 			FROM information_schema.schemata
@@ -562,7 +589,7 @@ func (p *PostgresqlDatabase) CreateReadOnlyUser(
 			return "", "", fmt.Errorf("error iterating schemas: %w", err)
 		}
 
-		// Step 4: Grant USAGE on each schema and explicitly prevent CREATE
+		// Step 5: Grant USAGE on each schema and explicitly prevent CREATE
 		for _, schema := range schemas {
 			// Revoke CREATE specifically (handles inheritance from PUBLIC role)
 			_, err = tx.Exec(
@@ -591,7 +618,7 @@ func (p *PostgresqlDatabase) CreateReadOnlyUser(
 			}
 		}
 
-		// Step 5: Grant SELECT on ALL existing tables and sequences
+		// Step 6: Grant SELECT on ALL existing tables and sequences
 		grantSelectSQL := fmt.Sprintf(`
 			DO $$
 			DECLARE
@@ -613,7 +640,7 @@ func (p *PostgresqlDatabase) CreateReadOnlyUser(
 			return "", "", fmt.Errorf("failed to grant select on tables: %w", err)
 		}
 
-		// Step 6: Set default privileges for FUTURE tables and sequences
+		// Step 7: Set default privileges for FUTURE tables and sequences
 		defaultPrivilegesSQL := fmt.Sprintf(`
 			DO $$
 			DECLARE
@@ -635,7 +662,7 @@ func (p *PostgresqlDatabase) CreateReadOnlyUser(
 			return "", "", fmt.Errorf("failed to set default privileges: %w", err)
 		}
 
-		// Step 7: Verify user creation before committing
+		// Step 8: Verify user creation before committing
 		var verifyUsername string
 		err = tx.QueryRow(ctx, fmt.Sprintf(`SELECT rolname FROM pg_roles WHERE rolname = '%s'`, baseUsername)).
 			Scan(&verifyUsername)
@@ -851,7 +878,15 @@ func checkBackupPermissions(
 		}
 
 		if err != nil {
-			return fmt.Errorf("cannot check SELECT privileges: %w", err)
+			// If the user doesn't have USAGE on the schema, has_table_privilege will fail
+			// with "permission denied for schema". This means they definitely don't have
+			// SELECT privileges, so treat this as missing permissions rather than an error.
+			var pgErr *pgconn.PgError
+			if errors.As(err, &pgErr) && pgErr.Code == "42501" { // insufficient_privilege
+				selectableTableCount = 0
+			} else {
+				return fmt.Errorf("cannot check SELECT privileges: %w", err)
+			}
 		}
 		if selectableTableCount == 0 {
 			missingPrivileges = append(missingPrivileges, "SELECT on tables")

backend/internal/features/databases/databases/postgresql/model_test.go

@@ -709,6 +709,344 @@ func Test_CreateReadOnlyUser_Supabase_UserCanReadButNotWrite(t *testing.T) {
 	assert.Contains(t, err.Error(), "permission denied")
 }
 
+func Test_CreateReadOnlyUser_WithPublicSchema_Success(t *testing.T) {
+	env := config.GetEnv()
+	cases := []struct {
+		name    string
+		version string
+		port    string
+	}{
+		{"PostgreSQL 12", "12", env.TestPostgres12Port},
+		{"PostgreSQL 13", "13", env.TestPostgres13Port},
+		{"PostgreSQL 14", "14", env.TestPostgres14Port},
+		{"PostgreSQL 15", "15", env.TestPostgres15Port},
+		{"PostgreSQL 16", "16", env.TestPostgres16Port},
+		{"PostgreSQL 17", "17", env.TestPostgres17Port},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			container := connectToPostgresContainer(t, tc.port)
+			defer container.DB.Close()
+
+			_, err := container.DB.Exec(`
+				DROP TABLE IF EXISTS public_schema_test CASCADE;
+				CREATE TABLE public_schema_test (
+					id SERIAL PRIMARY KEY,
+					data TEXT NOT NULL
+				);
+				INSERT INTO public_schema_test (data) VALUES ('test1'), ('test2');
+			`)
+			assert.NoError(t, err)
+
+			pgModel := createPostgresModel(container)
+			logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
+			ctx := context.Background()
+
+			username, password, err := pgModel.CreateReadOnlyUser(ctx, logger, nil, uuid.New())
+			assert.NoError(t, err)
+			assert.NotEmpty(t, username)
+			assert.NotEmpty(t, password)
+			assert.True(t, strings.HasPrefix(username, "databasus-"))
+
+			readOnlyModel := &PostgresqlDatabase{
+				Version:  pgModel.Version,
+				Host:     pgModel.Host,
+				Port:     pgModel.Port,
+				Username: username,
+				Password: password,
+				Database: pgModel.Database,
+				IsHttps:  false,
+			}
+
+			isReadOnly, privileges, err := readOnlyModel.IsUserReadOnly(
+				ctx,
+				logger,
+				nil,
+				uuid.New(),
+			)
+			assert.NoError(t, err)
+			assert.True(t, isReadOnly, "User should be read-only")
+			assert.Empty(t, privileges, "Read-only user should have no write privileges")
+
+			readOnlyDSN := fmt.Sprintf(
+				"host=%s port=%d user=%s password=%s dbname=%s sslmode=disable",
+				container.Host,
+				container.Port,
+				username,
+				password,
+				container.Database,
+			)
+			readOnlyConn, err := sqlx.Connect("postgres", readOnlyDSN)
+			assert.NoError(t, err)
+			defer readOnlyConn.Close()
+
+			var count int
+			err = readOnlyConn.Get(&count, "SELECT COUNT(*) FROM public_schema_test")
+			assert.NoError(t, err)
+			assert.Equal(t, 2, count)
+
+			_, err = readOnlyConn.Exec(
+				"INSERT INTO public_schema_test (data) VALUES ('should-fail')",
+			)
+			assert.Error(t, err)
+			assert.Contains(t, err.Error(), "permission denied")
+
+			_, err = readOnlyConn.Exec("CREATE TABLE public.hack_table (id INT)")
+			assert.Error(t, err)
+			assert.Contains(t, err.Error(), "permission denied")
+
+			_, err = container.DB.Exec(fmt.Sprintf(`DROP OWNED BY "%s" CASCADE`, username))
+			if err != nil {
+				t.Logf("Warning: Failed to drop owned objects: %v", err)
+			}
+			_, err = container.DB.Exec(fmt.Sprintf(`DROP USER IF EXISTS "%s"`, username))
+			assert.NoError(t, err)
+		})
+	}
+}
+
+func Test_CreateReadOnlyUser_WithoutPublicSchema_Success(t *testing.T) {
+	env := config.GetEnv()
+	cases := []struct {
+		name    string
+		version string
+		port    string
+	}{
+		{"PostgreSQL 12", "12", env.TestPostgres12Port},
+		{"PostgreSQL 13", "13", env.TestPostgres13Port},
+		{"PostgreSQL 14", "14", env.TestPostgres14Port},
+		{"PostgreSQL 15", "15", env.TestPostgres15Port},
+		{"PostgreSQL 16", "16", env.TestPostgres16Port},
+		{"PostgreSQL 17", "17", env.TestPostgres17Port},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			container := connectToPostgresContainer(t, tc.port)
+			defer container.DB.Close()
+
+			_, err := container.DB.Exec(`
+				DROP SCHEMA IF EXISTS public CASCADE;
+				DROP SCHEMA IF EXISTS app_schema CASCADE;
+				DROP SCHEMA IF EXISTS data_schema CASCADE;
+				CREATE SCHEMA app_schema;
+				CREATE SCHEMA data_schema;
+				CREATE TABLE app_schema.users (
+					id SERIAL PRIMARY KEY,
+					username TEXT NOT NULL
+				);
+				CREATE TABLE data_schema.records (
+					id SERIAL PRIMARY KEY,
+					info TEXT NOT NULL
+				);
+				INSERT INTO app_schema.users (username) VALUES ('user1'), ('user2');
+				INSERT INTO data_schema.records (info) VALUES ('record1'), ('record2');
+			`)
+			assert.NoError(t, err)
+
+			pgModel := createPostgresModel(container)
+			logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
+			ctx := context.Background()
+
+			username, password, err := pgModel.CreateReadOnlyUser(ctx, logger, nil, uuid.New())
+			assert.NoError(t, err, "CreateReadOnlyUser should succeed without public schema")
+			assert.NotEmpty(t, username)
+			assert.NotEmpty(t, password)
+			assert.True(t, strings.HasPrefix(username, "databasus-"))
+
+			readOnlyModel := &PostgresqlDatabase{
+				Version:  pgModel.Version,
+				Host:     pgModel.Host,
+				Port:     pgModel.Port,
+				Username: username,
+				Password: password,
+				Database: pgModel.Database,
+				IsHttps:  false,
+			}
+
+			isReadOnly, privileges, err := readOnlyModel.IsUserReadOnly(
+				ctx,
+				logger,
+				nil,
+				uuid.New(),
+			)
+			assert.NoError(t, err)
+			assert.True(t, isReadOnly, "User should be read-only")
+			assert.Empty(t, privileges, "Read-only user should have no write privileges")
+
+			readOnlyDSN := fmt.Sprintf(
+				"host=%s port=%d user=%s password=%s dbname=%s sslmode=disable",
+				container.Host,
+				container.Port,
+				username,
+				password,
+				container.Database,
+			)
+			readOnlyConn, err := sqlx.Connect("postgres", readOnlyDSN)
+			assert.NoError(t, err)
+			defer readOnlyConn.Close()
+
+			var userCount int
+			err = readOnlyConn.Get(&userCount, "SELECT COUNT(*) FROM app_schema.users")
+			assert.NoError(t, err)
+			assert.Equal(t, 2, userCount)
+
+			var recordCount int
+			err = readOnlyConn.Get(&recordCount, "SELECT COUNT(*) FROM data_schema.records")
+			assert.NoError(t, err)
+			assert.Equal(t, 2, recordCount)
+
+			_, err = readOnlyConn.Exec(
+				"INSERT INTO app_schema.users (username) VALUES ('should-fail')",
+			)
+			assert.Error(t, err)
+			assert.Contains(t, err.Error(), "permission denied")
+
+			_, err = readOnlyConn.Exec("CREATE TABLE app_schema.hack_table (id INT)")
+			assert.Error(t, err)
+			assert.Contains(t, err.Error(), "permission denied")
+
+			_, err = readOnlyConn.Exec("CREATE TABLE data_schema.hack_table (id INT)")
+			assert.Error(t, err)
+			assert.Contains(t, err.Error(), "permission denied")
+
+			_, err = container.DB.Exec(fmt.Sprintf(`DROP OWNED BY "%s" CASCADE`, username))
+			if err != nil {
+				t.Logf("Warning: Failed to drop owned objects: %v", err)
+			}
+			_, err = container.DB.Exec(fmt.Sprintf(`DROP USER IF EXISTS "%s"`, username))
+			assert.NoError(t, err)
+
+			_, err = container.DB.Exec(`
+				DROP SCHEMA IF EXISTS app_schema CASCADE;
+				DROP SCHEMA IF EXISTS data_schema CASCADE;
+				CREATE SCHEMA IF NOT EXISTS public;
+			`)
+			assert.NoError(t, err)
+		})
+	}
+}
+
+func Test_CreateReadOnlyUser_PublicSchemaExistsButNoPermissions_ReturnsError(t *testing.T) {
+	env := config.GetEnv()
+	cases := []struct {
+		name    string
+		version string
+		port    string
+	}{
+		{"PostgreSQL 12", "12", env.TestPostgres12Port},
+		{"PostgreSQL 13", "13", env.TestPostgres13Port},
+		{"PostgreSQL 14", "14", env.TestPostgres14Port},
+		{"PostgreSQL 15", "15", env.TestPostgres15Port},
+		{"PostgreSQL 16", "16", env.TestPostgres16Port},
+		{"PostgreSQL 17", "17", env.TestPostgres17Port},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			container := connectToPostgresContainer(t, tc.port)
+			defer container.DB.Close()
+
+			limitedAdminUsername := fmt.Sprintf("limited_admin_%s", uuid.New().String()[:8])
+			limitedAdminPassword := "limited_password_123"
+
+			_, err := container.DB.Exec(`
+				CREATE SCHEMA IF NOT EXISTS public;
+				DROP TABLE IF EXISTS public.permission_test_table CASCADE;
+				CREATE TABLE public.permission_test_table (
+					id SERIAL PRIMARY KEY,
+					data TEXT NOT NULL
+				);
+				INSERT INTO public.permission_test_table (data) VALUES ('test1');
+			`)
+			assert.NoError(t, err)
+
+			_, err = container.DB.Exec(`GRANT CREATE ON SCHEMA public TO PUBLIC`)
+			assert.NoError(t, err)
+
+			_, err = container.DB.Exec(fmt.Sprintf(
+				`CREATE USER "%s" WITH PASSWORD '%s' LOGIN CREATEROLE`,
+				limitedAdminUsername,
+				limitedAdminPassword,
+			))
+			assert.NoError(t, err)
+
+			_, err = container.DB.Exec(fmt.Sprintf(
+				`GRANT CONNECT ON DATABASE "%s" TO "%s"`,
+				container.Database,
+				limitedAdminUsername,
+			))
+			assert.NoError(t, err)
+
+			defer func() {
+				_, _ = container.DB.Exec(
+					fmt.Sprintf(`DROP OWNED BY "%s" CASCADE`, limitedAdminUsername),
+				)
+				_, _ = container.DB.Exec(
+					fmt.Sprintf(`DROP USER IF EXISTS "%s"`, limitedAdminUsername),
+				)
+				_, _ = container.DB.Exec(`REVOKE CREATE ON SCHEMA public FROM PUBLIC`)
+			}()
+
+			limitedAdminDSN := fmt.Sprintf(
+				"host=%s port=%d user=%s password=%s dbname=%s sslmode=disable",
+				container.Host,
+				container.Port,
+				limitedAdminUsername,
+				limitedAdminPassword,
+				container.Database,
+			)
+			limitedAdminConn, err := sqlx.Connect("postgres", limitedAdminDSN)
+			assert.NoError(t, err)
+			defer limitedAdminConn.Close()
+
+			pgModel := &PostgresqlDatabase{
+				Version:  tools.GetPostgresqlVersionEnum(tc.version),
+				Host:     container.Host,
+				Port:     container.Port,
+				Username: limitedAdminUsername,
+				Password: limitedAdminPassword,
+				Database: &container.Database,
+				IsHttps:  false,
+			}
+
+			logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
+			ctx := context.Background()
+
+			username, password, err := pgModel.CreateReadOnlyUser(ctx, logger, nil, uuid.New())
+			assert.Error(
+				t,
+				err,
+				"CreateReadOnlyUser should fail when admin lacks permissions to secure public schema",
+			)
+			if err != nil {
+				errorMsg := err.Error()
+				hasExpectedError := strings.Contains(
+					errorMsg,
+					"failed to revoke CREATE from PUBLIC on existing public schema",
+				) ||
+					strings.Contains(errorMsg, "permission denied for schema public") ||
+					strings.Contains(errorMsg, "failed to grant")
+				assert.True(
+					t,
+					hasExpectedError,
+					"Error should indicate permission issues with public schema, got: %s",
+					errorMsg,
+				)
+			}
+			assert.Empty(t, username)
+			assert.Empty(t, password)
+		})
+	}
+}
+
 func Test_Validate_WhenLocalhostAndDatabasus_ReturnsError(t *testing.T) {
 	testCases := []struct {
 		name     string

backend/internal/features/notifiers/models/email_notifier/model.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"mime"
 	"net"
 	"net/smtp"
 	"time"
@@ -115,16 +116,34 @@ func (e *EmailNotifier) EncryptSensitiveData(encryptor encryption.FieldEncryptor
 	return nil
 }
 
+// encodeRFC2047 encodes a string using RFC 2047 MIME encoding for email headers
+// This ensures compatibility with SMTP servers that don't support SMTPUTF8
+func encodeRFC2047(s string) string {
+	// mime.QEncoding handles UTF-8 → =?UTF-8?Q?...?= encoding
+	// This allows non-ASCII characters (emojis, accents, etc.) in email headers
+	// while maintaining compatibility with all SMTP servers
+	return mime.QEncoding.Encode("UTF-8", s)
+}
+
 func (e *EmailNotifier) buildEmailContent(heading, message, from string) []byte {
-	subject := fmt.Sprintf("Subject: %s\r\n", heading)
-	mime := fmt.Sprintf(
+	// Encode Subject header using RFC 2047 to avoid SMTPUTF8 requirement
+	// This ensures compatibility with SMTP servers that don't support SMTPUTF8
+	encodedSubject := encodeRFC2047(heading)
+	subject := fmt.Sprintf("Subject: %s\r\n", encodedSubject)
+
+	mimeHeaders := fmt.Sprintf(
 		"MIME-version: 1.0;\nContent-Type: %s; charset=\"%s\";\n\n",
 		MIMETypeHTML,
 		MIMECharsetUTF8,
 	)
-	fromHeader := fmt.Sprintf("From: %s\r\n", from)
+
+	// Encode From header display name if it contains non-ASCII
+	encodedFrom := encodeRFC2047(from)
+	fromHeader := fmt.Sprintf("From: %s\r\n", encodedFrom)
+
 	toHeader := fmt.Sprintf("To: %s\r\n", e.TargetEmail)
-	return []byte(fromHeader + toHeader + subject + mime + message)
+
+	return []byte(fromHeader + toHeader + subject + mimeHeaders + message)
 }
 
 func (e *EmailNotifier) sendImplicitTLS(

backend/internal/features/storages/models/ftp/model.go

@@ -285,30 +285,30 @@ func (f *FTPStorage) ensureDirectory(conn *ftp.ServerConn, path string) error {
 	}
 
 	parts := strings.Split(path, "/")
-	currentPath := ""
+
+	currentDir, err := conn.CurrentDir()
+	if err != nil {
+		return fmt.Errorf("failed to get current directory: %w", err)
+	}
+	defer func() {
+		_ = conn.ChangeDir(currentDir)
+	}()
 
 	for _, part := range parts {
 		if part == "" || part == "." {
 			continue
 		}
 
-		if currentPath == "" {
-			currentPath = part
-		} else {
-			currentPath = currentPath + "/" + part
-		}
-
-		err := conn.ChangeDir(currentPath)
+		err := conn.ChangeDir(part)
 		if err != nil {
-			err = conn.MakeDir(currentPath)
+			err = conn.MakeDir(part)
 			if err != nil {
-				return fmt.Errorf("failed to create directory '%s': %w", currentPath, err)
+				return fmt.Errorf("failed to create directory '%s': %w", part, err)
+			}
+			err = conn.ChangeDir(part)
+			if err != nil {
+				return fmt.Errorf("failed to change into directory '%s': %w", part, err)
 			}
-		}
-
-		err = conn.ChangeDirToParent()
-		if err != nil {
-			return fmt.Errorf("failed to change to parent directory: %w", err)
 		}
 	}