From 7a54a83c8d2ed42738b7e54cb416545fcff1c515 Mon Sep 17 00:00:00 2001 From: StarFleetCPTN Date: Sun, 6 Apr 2025 23:11:41 -0700 Subject: [PATCH 1/6] fix: Improve migration process by handling potential leftover temporary tables - Added logic to drop any existing temporary tables from previous failed migration runs before renaming the current table. - Enhanced error logging to provide warnings if the drop operation fails, ensuring smoother migration execution. https://github.com/StarFleetCPTN/GoMFT/issues/72 --- internal/db/migrations/012_alter_boolean_defaults.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/db/migrations/012_alter_boolean_defaults.go b/internal/db/migrations/012_alter_boolean_defaults.go index cfcbd42..b71d02c 100644 --- a/internal/db/migrations/012_alter_boolean_defaults.go +++ b/internal/db/migrations/012_alter_boolean_defaults.go @@ -178,6 +178,12 @@ func AlterBooleanDefaults() *gormigrate.Migration { fmt.Printf("Recreating table %s...\n", tableName) oldTableName := fmt.Sprintf("_%s_old", tableName) + // Drop the old temp table if it exists from a previous failed run + if err := tx.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %s", oldTableName)).Error; err != nil { + // Log the error but proceed, as the rename might still work or fail for the intended reason + fmt.Printf("Warning: failed to drop potential leftover table %s: %v\n", oldTableName, err) + } + // Rename old table if err := tx.Exec(fmt.Sprintf("ALTER TABLE %s RENAME TO %s", tableName, oldTableName)).Error; err == nil { fmt.Printf("Renamed %s to %s.\n", tableName, oldTableName) From 105d223be42ec1928c912b40be7afdb612fda1c4 Mon Sep 17 00:00:00 2001 From: StarFleetCPTN Date: Sun, 6 Apr 2025 23:27:00 -0700 Subject: [PATCH 2/6] refactor: Simplify job scheduling validation logic - Removed manual parsing and validation of cron expressions, relying on the cron instance's AddFunc for validation. - Enhanced error logging to provide more informative messages when scheduling fails, including the original schedule string. - Streamlined the scheduling process by using the original schedule directly, improving code clarity and maintainability. https://github.com/StarFleetCPTN/GoMFT/issues/71 --- internal/scheduler/scheduler.go | 37 +++++++-------------------------- 1 file changed, 8 insertions(+), 29 deletions(-) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 085a8d4..85f99b4 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -3,7 +3,6 @@ package scheduler import ( "context" "fmt" - "strings" "sync" // Needed for Job.NextRun update @@ -162,38 +161,18 @@ func (s *Scheduler) ScheduleJob(job *db.Job) error { return nil } - // Use a 6-field parser for validation and determine the schedule string to use. - scheduleToUse := job.Schedule - parser := cron.NewParser(cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) - _, err := parser.Parse(scheduleToUse) - - // If initial parse fails AND it was a 5-field schedule, try prepending seconds - if err != nil && len(strings.Fields(job.Schedule)) == 5 { - scheduleWithSeconds := "0 " + job.Schedule - _, errSeconds := parser.Parse(scheduleWithSeconds) - if errSeconds == nil { - scheduleToUse = scheduleWithSeconds // Use the 6-field version - err = nil // Clear the original error - s.logger.LogDebug("Converted 5-field schedule '%s' to 6-field '%s'", job.Schedule, scheduleToUse) - } - } - - // If error still exists after trying conversion, return it - if err != nil { - return fmt.Errorf("invalid cron expression '%s': %w", job.Schedule, err) - } - - s.logger.LogDebug("Validated cron expression '%s' for job %d", scheduleToUse, jobID) - - // Schedule the job using the validated scheduleToUse + // Rely on the cron instance's AddFunc for validation based on its configuration (5 or 6 fields) + scheduleToUse := job.Schedule // Use the original schedule string + s.logger.LogDebug("Using schedule '%s' for job %d", scheduleToUse, jobID) + // Schedule the job using the original schedule string. AddFunc will validate it. entryID, err := s.cron.AddFunc(scheduleToUse, func() { // Calls interface method s.executor.executeJob(jobID) // Calls interface method }) - if err != nil { - s.logger.LogError("Error scheduling job %d: %v", jobID, err) - return err - } // <-- Added missing closing brace + // Log and return a more informative error if AddFunc fails validation + s.logger.LogError("Error scheduling job %d with schedule '%s': %v", jobID, scheduleToUse, err) + return fmt.Errorf("invalid cron expression '%s' for the configured scheduler: %w", scheduleToUse, err) + } s.logger.LogDebug("Scheduled job %d with cron entry ID %d", jobID, entryID) // Store mapping of job ID to cron entry ID From 0f9a4cffd285df5f9bd5535f7d75db64c491d552 Mon Sep 17 00:00:00 2001 From: StarFleetCPTN Date: Sun, 6 Apr 2025 23:35:48 -0700 Subject: [PATCH 3/6] fix: Improve UID/GID management in entrypoint script - Enhanced the logic for updating user and group IDs by deleting and recreating the user/group if the current IDs do not match the specified values. - Added verification steps to ensure the UID/GID changes were successful, with appropriate error handling and logging for failures. - Streamlined the process for handling Alpine Linux user management, improving clarity and maintainability of the script. https://github.com/StarFleetCPTN/GoMFT/issues/68 --- entrypoint.sh | 52 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/entrypoint.sh b/entrypoint.sh index 4305494..bf2f734 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -16,15 +16,49 @@ if [ -n "${PUID}" ] && [ -n "${PGID}" ]; then echo "Detected Alpine Linux, using busybox usermod/groupmod..." # Update group ID first - if [ "$(getent group ${USERNAME} | cut -d: -f3)" != "${PGID}" ]; then - echo "Updating GID to ${PGID}..." - groupmod -g ${PGID} ${USERNAME} || echo "⚠️ Failed to change GID" - fi - - # Update user ID - if [ "$(id -u ${USERNAME})" != "${PUID}" ]; then - echo "Updating UID to ${PUID}..." - usermod -u ${PUID} ${USERNAME} || echo "⚠️ Failed to change UID" + CURRENT_GID=$(getent group ${USERNAME} | cut -d: -f3) + CURRENT_UID=$(id -u ${USERNAME}) + + if [ "${CURRENT_GID}" != "${PGID}" ] || [ "${CURRENT_UID}" != "${PUID}" ]; then + echo "Attempting to update UID/GID to ${PUID}:${PGID} using delete/recreate..." + + # Delete existing user and group, ignoring errors + deluser ${USERNAME} > /dev/null 2>&1 || true + delgroup ${USERNAME} > /dev/null 2>&1 || true + + # Add group with the specified GID + echo "Adding group ${USERNAME} with GID ${PGID}" + if ! addgroup -g ${PGID} ${USERNAME}; then + echo "⚠️ Failed to add group ${USERNAME} with GID ${PGID}." + # Exiting because user creation will likely fail + exit 1 + fi + + # Add user with the specified UID and GID + # Use -G for primary group with adduser in BusyBox + # Use -h /app for home directory (consistent with expectations) + # Use -s /bin/sh for shell + # Use -D for no password (system user) + echo "Adding user ${USERNAME} with UID ${PUID}" + if ! adduser -u ${PUID} -G ${USERNAME} -h /app -s /bin/sh -D ${USERNAME}; then + echo "⚠️ Failed to add user ${USERNAME} with UID ${PUID} and group ${USERNAME}." + # Exiting because the application cannot run as the correct user + exit 1 + fi + + # Verify the change + FINAL_UID=$(id -u ${USERNAME} 2>/dev/null || echo "error") + FINAL_GID=$(getent group ${USERNAME} | cut -d: -f3 2>/dev/null || echo "error") + + if [ "${FINAL_UID}" = "${PUID}" ] && [ "${FINAL_GID}" = "${PGID}" ]; then + echo "✅ Successfully updated UID/GID to ${PUID}:${PGID}" + else + echo "⚠️ Verification failed after update. Target: ${PUID}:${PGID}, Actual: ${FINAL_UID}:${FINAL_GID}" + # Exiting because the UID/GID is not correct + exit 1 + fi + else + echo "UID/GID ${PUID}:${PGID} already set." fi else echo "Non-Alpine system, using standard user management..." From 53cf4bc3c5f9bde25401f41ae614cbc8c6feb6a0 Mon Sep 17 00:00:00 2001 From: StarFleetCPTN Date: Sun, 6 Apr 2025 23:52:46 -0700 Subject: [PATCH 4/6] feat: Add migration to clean up invalid boolean values in database - Introduced a new migration (013_cleanup_invalid_booleans) to update boolean columns represented as integers in the transfer_configs, notification_services, and auth_providers tables. - Ensured that only valid values (0, 1, or NULL) are retained, setting any invalid values to 0. - Added error handling for potential issues during the migration process, including checks for table existence before attempting updates. This migration improves data integrity by standardizing boolean representations across the database. https://github.com/StarFleetCPTN/GoMFT/issues/69 --- .../013_cleanup_invalid_booleans.go | 91 +++++++++++++++++++ internal/db/migrations/migrations.go | 1 + 2 files changed, 92 insertions(+) create mode 100644 internal/db/migrations/013_cleanup_invalid_booleans.go diff --git a/internal/db/migrations/013_cleanup_invalid_booleans.go b/internal/db/migrations/013_cleanup_invalid_booleans.go new file mode 100644 index 0000000..bbde9e6 --- /dev/null +++ b/internal/db/migrations/013_cleanup_invalid_booleans.go @@ -0,0 +1,91 @@ +package migrations + +import ( + "fmt" + + "github.com/go-gormigrate/gormigrate/v2" + "gorm.io/gorm" +) + +// CleanupInvalidBooleans updates boolean columns represented as integers +// to ensure they only contain valid values (0, 1, or NULL). +func CleanupInvalidBooleans() *gormigrate.Migration { + return &gormigrate.Migration{ + ID: "013_cleanup_invalid_booleans", + Migrate: func(tx *gorm.DB) error { + fmt.Println("Running migration 013: Cleaning up invalid boolean values...") + + // Target: transfer_configs.delete_after_transfer + // Set any non-NULL value that is not 0 or 1 to 0 (false) + sql := `UPDATE transfer_configs + SET delete_after_transfer = 0 + WHERE delete_after_transfer IS NOT NULL AND delete_after_transfer NOT IN (0, 1);` + + if err := tx.Exec(sql).Error; err != nil { + return fmt.Errorf("failed to cleanup delete_after_transfer in transfer_configs: %w", err) + } + fmt.Println("Cleaned up invalid values in transfer_configs.delete_after_transfer.") + + // Target: transfer_configs.archive_enabled + sql = `UPDATE transfer_configs + SET archive_enabled = 0 + WHERE archive_enabled IS NOT NULL AND archive_enabled NOT IN (0, 1);` + if err := tx.Exec(sql).Error; err != nil { + return fmt.Errorf("failed to cleanup archive_enabled in transfer_configs: %w", err) + } + fmt.Println("Cleaned up invalid values in transfer_configs.archive_enabled.") + + // Target: transfer_configs.skip_processed_files + sql = `UPDATE transfer_configs + SET skip_processed_files = 0 + WHERE skip_processed_files IS NOT NULL AND skip_processed_files NOT IN (0, 1);` + if err := tx.Exec(sql).Error; err != nil { + return fmt.Errorf("failed to cleanup skip_processed_files in transfer_configs: %w", err) + } + fmt.Println("Cleaned up invalid values in transfer_configs.skip_processed_files.") + + // Target: notification_services.is_enabled + sql = `UPDATE notification_services + SET is_enabled = 0 + WHERE is_enabled IS NOT NULL AND is_enabled NOT IN (0, 1);` + if err := tx.Exec(sql).Error; err != nil { + // Check if the table exists before failing hard + var tableExists int + tx.Raw("SELECT count(*) FROM sqlite_master WHERE type='table' AND name='notification_services'").Scan(&tableExists) + if tableExists == 0 { + fmt.Println("Skipping cleanup for notification_services.is_enabled: table does not exist.") + } else { + return fmt.Errorf("failed to cleanup is_enabled in notification_services: %w", err) + } + } else { + fmt.Println("Cleaned up invalid values in notification_services.is_enabled.") + } + + // Target: auth_providers.enabled + sql = `UPDATE auth_providers + SET enabled = 0 + WHERE enabled IS NOT NULL AND enabled NOT IN (0, 1);` + if err := tx.Exec(sql).Error; err != nil { + // Check if the table exists before failing hard + var tableExists int + tx.Raw("SELECT count(*) FROM sqlite_master WHERE type='table' AND name='auth_providers'").Scan(&tableExists) + if tableExists == 0 { + fmt.Println("Skipping cleanup for auth_providers.enabled: table does not exist.") + } else { + return fmt.Errorf("failed to cleanup enabled in auth_providers: %w", err) + } + } else { + fmt.Println("Cleaned up invalid values in auth_providers.enabled.") + } + + fmt.Println("Migration 013 completed successfully.") + return nil + }, + Rollback: func(tx *gorm.DB) error { + // This migration cleans up data. Rolling back doesn't make sense + // as we don't know the original invalid values. + fmt.Println("Rollback for migration 013_cleanup_invalid_booleans is not applicable.") + return nil + }, + } +} diff --git a/internal/db/migrations/migrations.go b/internal/db/migrations/migrations.go index 4bfbaa2..be953b6 100644 --- a/internal/db/migrations/migrations.go +++ b/internal/db/migrations/migrations.go @@ -23,6 +23,7 @@ func GetMigrations(db *gorm.DB) *gormigrate.Gormigrate { AddRcloneCommandToConfig(), // 010 AddAuthProviders(), // 011 AlterBooleanDefaults(), // 012 + CleanupInvalidBooleans(), // 013 ) return gormigrate.New(db, gormigrate.DefaultOptions, migrations) From 71ec298db53a1bb38e6ba9ae05992b20fbd694a0 Mon Sep 17 00:00:00 2001 From: StarFleetCPTN Date: Sun, 6 Apr 2025 23:58:56 -0700 Subject: [PATCH 5/6] fix: Update admin user deletion logic to prevent deletion of the last administrator - Changed the logic in the HandleDeleteUser function to check for other administrators using the actual 'is_admin' column instead of counting all admins. - Prevented deletion if no other administrators exist, ensuring at least one admin remains in the system. https://github.com/StarFleetCPTN/GoMFT/issues/67 --- internal/web/handlers/admin_handlers.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/internal/web/handlers/admin_handlers.go b/internal/web/handlers/admin_handlers.go index 3523c9c..a3f71b5 100644 --- a/internal/web/handlers/admin_handlers.go +++ b/internal/web/handlers/admin_handlers.go @@ -1181,15 +1181,18 @@ func (h *Handlers) HandleDeleteUser(c *gin.Context) { // Check if this is an admin user if user.GetIsAdmin() { - // Count how many admins there are - var adminCount int64 - if err := h.DB.Model(&db.User{}).Where("metadata->>'is_admin' = 'true'").Count(&adminCount).Error; err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to check admin count"}) + // Check if other administrators exist + var otherAdminCount int64 + // Use the actual 'is_admin' column, comparing against true + if err := h.DB.Model(&db.User{}). + Where("is_admin = ? AND id != ?", true, user.ID). + Count(&otherAdminCount).Error; err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to check for other administrators"}) return } - // If this is the last admin, prevent deletion - if adminCount <= 1 { + // If no other administrators exist, prevent deletion + if otherAdminCount == 0 { c.JSON(http.StatusBadRequest, gin.H{"error": "Cannot delete the last administrator"}) return } From e14d90e37e1f01b3edb0c61a991be3d8d8829936 Mon Sep 17 00:00:00 2001 From: StarFleetCPTN Date: Mon, 7 Apr 2025 00:24:10 -0700 Subject: [PATCH 6/6] feat: Add recovery migration for transfer_configs table rename - Introduced a new migration (011a_recover_transfer_configs_rename) to check and recover the transfer_configs table if it was incorrectly renamed to _transfer_configs_old during a previous migration. - Implemented logic to handle various states of the tables, including logging warnings for potential issues. - Ensured that the migration can be safely added to the existing migration sequence. This migration enhances the robustness of the database migration process by addressing potential inconsistencies. --- .../011a_recover_transfer_configs_rename.go | 47 +++++++++++++++++++ internal/db/migrations/migrations.go | 27 ++++++----- 2 files changed, 61 insertions(+), 13 deletions(-) create mode 100644 internal/db/migrations/011a_recover_transfer_configs_rename.go diff --git a/internal/db/migrations/011a_recover_transfer_configs_rename.go b/internal/db/migrations/011a_recover_transfer_configs_rename.go new file mode 100644 index 0000000..cd6dcf3 --- /dev/null +++ b/internal/db/migrations/011a_recover_transfer_configs_rename.go @@ -0,0 +1,47 @@ +package migrations + +import ( + "fmt" + + "github.com/go-gormigrate/gormigrate/v2" + "gorm.io/gorm" +) + +// RecoverTransferConfigsRename checks for and corrects a specific inconsistent state +// left by a potentially failed run of migration 012, where the transfer_configs +// table might have been left renamed as _transfer_configs_old. +func RecoverTransferConfigsRename() *gormigrate.Migration { + return &gormigrate.Migration{ + ID: "011a_recover_transfer_configs_rename", + Migrate: func(tx *gorm.DB) error { + fmt.Println("Running migration 011a: Checking for transfer_configs rename recovery...") + + var oldTableExists int + tx.Raw("SELECT count(*) FROM sqlite_master WHERE type='table' AND name='_transfer_configs_old'").Scan(&oldTableExists) + + var newTableExists int + tx.Raw("SELECT count(*) FROM sqlite_master WHERE type='table' AND name='transfer_configs'").Scan(&newTableExists) + + if oldTableExists > 0 && newTableExists == 0 { + fmt.Println("Found _transfer_configs_old table but not transfer_configs. Attempting recovery rename...") + if err := tx.Exec("ALTER TABLE _transfer_configs_old RENAME TO transfer_configs").Error; err != nil { + return fmt.Errorf("failed to rename _transfer_configs_old back to transfer_configs: %w", err) + } + fmt.Println("Successfully renamed _transfer_configs_old to transfer_configs.") + } else if oldTableExists > 0 && newTableExists > 0 { + // This state shouldn't ideally happen if migration 012 followed its logic, + // but indicates a potential issue. Maybe drop the old one? For now, just log. + fmt.Println("Warning: Both transfer_configs and _transfer_configs_old tables exist. Manual inspection might be needed.") + } else { + fmt.Println("No recovery needed for transfer_configs rename.") + } + + return nil + }, + Rollback: func(tx *gorm.DB) error { + // Rollback doesn't make sense for a recovery step. + fmt.Println("Rollback for migration 011a_recover_transfer_configs_rename is not applicable.") + return nil + }, + } +} diff --git a/internal/db/migrations/migrations.go b/internal/db/migrations/migrations.go index be953b6..956e58b 100644 --- a/internal/db/migrations/migrations.go +++ b/internal/db/migrations/migrations.go @@ -11,19 +11,20 @@ var migrations []*gormigrate.Migration func GetMigrations(db *gorm.DB) *gormigrate.Gormigrate { // Add all migrations in order migrations = append(migrations, - InitialSchema(), // 001 - UpdateGDriveType(), // 002 - Add2FA(), // 003 - AddAuditLogs(), // 004 - AddDefaultRoles(), // 005 - AddTimestampsToJobHistories(), // 006 - AddNotificationServices(), // 007 - AddUserNotifications(), // 008 - AddRcloneTables(), // 009 - AddRcloneCommandToConfig(), // 010 - AddAuthProviders(), // 011 - AlterBooleanDefaults(), // 012 - CleanupInvalidBooleans(), // 013 + InitialSchema(), // 001 + UpdateGDriveType(), // 002 + Add2FA(), // 003 + AddAuditLogs(), // 004 + AddDefaultRoles(), // 005 + AddTimestampsToJobHistories(), // 006 + AddNotificationServices(), // 007 + AddUserNotifications(), // 008 + AddRcloneTables(), // 009 + AddRcloneCommandToConfig(), // 010 + AddAuthProviders(), // 011 + RecoverTransferConfigsRename(), // 011a + AlterBooleanDefaults(), // 012 + CleanupInvalidBooleans(), // 013 ) return gormigrate.New(db, gormigrate.DefaultOptions, migrations)