Fix to retire ProFTPD virtual FTP feature

[iliaross]

Hello, Jamie!

As you asked, creating a PR for this change so we can discuss it.

In particular, this change breaks other FTP-related features such as chroot directories, which we do want to preserve.

Thanks, I’ll take a look soon! I just want to finish something first.

[iliaross]

In particular, this change breaks other FTP-related features such as chroot directories, which we do want to preserve.

Thanks, I’ll take a look soon! I just want to finish something first.

Jamie, please see this patch 0e336da.

[jcameron]

So is the goal here to remove FTP support entirely, or just the virtual FTP feature? Because I see PR is deleting things like the ability to stop and start the FTP server, which I think we should keep. Also support for chroot'd FTP.

[iliaross]

Because I see PR is deleting things like the ability to stop and start the FTP server, which I think we should keep. Also support for chroot'd FTP.

You're right! Sorry about that! The fix is here e79471f.

[github-actions]

Code review failed

Repository: virtualmin/virtualmin-gpl
Commit: e79471fd0

The virtual FTP removal mostly updates feature lists and callers, but the now-unconditional chroot backup entry can break Virtualmin backups when ProFTPd is not available.

Fatal	Attention
1	0

Commit | Reviewed diff | Patch | GitHub run

Findings

• [fatal] virtual-server-lib.pl:185 - chroot is now always included in @virtualmin_backups, but virtualmin_backup_chroot returns 0 when ProFTPd chroot support is unavailable; because the restore path treats this unsupported case as success, the backup path is likely to mark Virtualmin configuration backups as failed on systems without ProFTPd. Suggested fix: Either keep chroot conditional on has_ftp_chroot()/ProFTPd support, or make virtualmin_backup_chroot return success when it intentionally skips because ProFTPd chroot support is unavailable.

Reviewed

• Removal of the retired virtual FTP feature and related feature lists
• ProFTPd helper loading and chroot backup/restore handling
• CLI feature validation changes for backup and restore commands

Passed checks

• Feature validation now checks core feature membership before accepting enabled config keys, which avoids accepting stale removed feature settings.
• The deleted per-domain FTP log option in get-logs.pl is rejected explicitly instead of silently selecting an unsupported log type.

[iliaross]

Because I see PR is deleting things like the ability to stop and start the FTP server, which I think we should keep. Also support for chroot'd FTP.

You're right! Sorry about that! The fix is here e79471f.

Jamie, code review is wrong, right?

The virtual FTP removal mostly updates feature lists and callers, but the now-unconditional chroot backup entry can break Virtualmin backups when ProFTPd is not available.

[jcameron]

We could remove the need for this extra check of @features by just removing ftp from the module config in postinstall.pl

[iliaross]

We could remove the need for this extra check of @features by just removing ftp from the module config in postinstall.pl

Probably better to keep the check here so we don’t break anything on existing systems?

[jcameron]

But that wouldn't be possible if we removed ftp=1 from /etc/webmin/virtual-server/config at upgrade time.

[jcameron]

Or alternately, create a common function like is_enabled_feature and move the current logic there.

[jcameron]

Yeah I don't get what that code review is thinking, sorry. The change seems pretty much OK to me, except for one comment I added.

[iliaross]

Yeah I don't get what that code review is thinking, sorry. The change seems pretty much OK to me, except for one comment I added.

There was a bug back then in the code review code for PRs that should be fixed now. Thanks.