[Bug]: Get a file with FTP: Return different result with Kettle Pentaho when inputting invalid user name, password or remote directory

[RonnyRen]

Apache Hop version?

2.11

Java version?

17

Operating system

Windows

What happened?

I migrated Pentaho jobs to Hop workflow and found that it behaves differently. Case 1: Input invalid user name and password Pentaho: Error getting files from FTP : User cannot log in. Hop: No error happened and no file was downloaded. Dialog "Connection OK" shows when clicking button "Test Connection".

Case 2: Input correct user name and password, but input a remote directory that doesn't exist Pentaho: Error getting files from FTP : The system cannot find the file specified. Hop: No error happened, it seems that it checks root directory if remote directory doesn't exist based on detailed logs and it downloaded matched files in the root directory.

Is it by design or bug?

I checked source code, it uses different library as Kettle Pentaho, first bug should be related to code as below, it doesn't check if login returns true or false. https://github.com/apache/hop/blob/main/plugins/actions/ftp/src/main/java/org/apache/hop/workflow/actions/util/FtpClientUtil.java#L178

Note: The login method in library EnterpriseDT.Net.Ftp used by Pentaho is void.

The second bug should be also related to different libraries used. Pentaho uses ftpclient.dirDetails( null ) and Hop uses ftpClient.listFiles().

Issue Priority

Priority: 3

Issue Component

Component: Workflows

[hansva]

The originally used library was [EdtFTPj](https://mvnrepository.com/artifact/com.enterprisedt/edtFTPj). This library is LGPL licensed and not compatible with the Apache license (the library also hasn't been maintained since 2006).

This is why we switched to commons-net, the issues sound to not be by design so should be fixed

[rdubois-mel]

I modified the following 2 functions in the plugins/actions/ftp/src/main/java/org/apache/hop/workflow/actions/ftp/ActionFtpDialog.java file to correctly test the authentication and the remote dir.

This seems to work well, it just needs a more human-friendly string to correctly say that the remote dir does not exist.

(...)

  private void checkRemoteFolder(boolean ftpFolder, boolean checkMoveFolder, String foldername) {
    if (!Utils.isEmpty(foldername) && connectToFtp(ftpFolder, checkMoveFolder)) {
      try {
        boolean exists = ftpclient.changeWorkingDirectory(foldername);
        if (exists) {
          MessageBox mb = new MessageBox(shell, SWT.OK | SWT.ICON_INFORMATION);
          mb.setMessage(
              BaseMessages.getString(PKG, "ActionFtp.FolderExists.OK", foldername) + Const.CR);
          mb.setText(BaseMessages.getString(PKG, "ActionFtp.FolderExists.Title.Ok"));
          mb.open();
        } else {
          MessageBox mb = new MessageBox(shell, SWT.OK | SWT.ICON_ERROR);
          mb.setMessage(
              BaseMessages.getString(PKG, "ActionFtp.FolderExists.NOK", foldername) + Const.CR);
          mb.setText(BaseMessages.getString(PKG, "ActionFtp.FolderExists.Title.Bad"));
          mb.open();
        }
      } catch (Exception e) {
        MessageBox mb = new MessageBox(shell, SWT.OK | SWT.ICON_ERROR);
        mb.setMessage("Error checking remote folder: " + e.getMessage());
        mb.setText("Folder Check Failed");
        mb.open();
      }
    }
  }

  private boolean connectToFtp(boolean checkFolder, boolean checkMoveToFolder) {
    try {
      if (ftpclient == null || !ftpclient.isConnected()) {
        ActionFtp actionFtp = new ActionFtp();
        getInfo(actionFtp);

        ftpclient =
            FtpClientUtil.connectAndLogin(LogChannel.UI, variables, actionFtp, wName.getText());

        // Explicit authentication verification
        try {
          if (ftpclient == null || !ftpclient.isConnected()) {
            throw new Exception("FTP client is null or not connected.");
          }
          // Attempting to list files to validate authentication
          ftpclient.listFiles();
        } catch (Exception authEx) {
          throw new Exception("FTP authentication failed: " + authEx.getMessage(), authEx);
        }

      }
      return true;
    } catch (Exception e) {
      MessageBox mb = new MessageBox(shell, SWT.OK | SWT.ICON_ERROR);
      mb.setMessage(
          BaseMessages.getString(
              PKG, "ActionFtp.ErrorConnect.NOK", wServerName.getText(), e.getMessage()));
      mb.setText(BaseMessages.getString(PKG, "ActionFtp.ErrorConnect.Title.Bad"));
      mb.open();
      return false;
    }
  }

(...)

To summarize, I added the return value handling for remote directory changes. And for authentication, I added a check (listFiles) because isConnected() isn't sufficient.

[rdubois-mel]

And to do this correctly, also modify the action itself to throw an error if the folder does not exist.

In the plugins/actions/ftp/src/main/java/org/apache/hop/workflow/actions/ftp/ActionFtp.java file at line 356

      // move to spool dir ...
      if (!Utils.isEmpty(remoteDirectory)) {
        String realFtpDirectory = resolve(remoteDirectory);
        realFtpDirectory = normalizePath(realFtpDirectory);
        boolean exists = ftpClient.changeWorkingDirectory(realFtpDirectory);
        if (!exists) {
          logError(BaseMessages.getString(PKG, "ActionFTP.NonExistentFolder"));
          return result;
        }
        if (isDetailed()) {
          logDetailed(BaseMessages.getString(PKG, "ActionFTP.ChangedDir", realFtpDirectory));
        }
      }

[rdubois-mel]

I've created a pull request with these changes. https://github.com/apache/hop/pull/5342

Please bear with me 😇, I hope I did everything correctly; this is my first pull request for Hop😄