[BUGS#1235] fix: hardening plugin installer utility class

[miurahr]

Hardening plugin installer class

Pull request type

• Bug fix -> [bug]

Which ticket is resolved?

• PluginInstaller throws NPE in certain condition
• https://sourceforge.net/p/omegat/bugs/1235/

dev-ml https://sourceforge.net/p/omegat/mailman/omegat-development/thread/78b810a2-34f6-4416-b28e-6e1dda659f4b%40northside.tokyo/#msg58712660

What does this PR change?

• Refactor PluginUtils#loadPlugins to register "jarFile!className" URL even when it is main jar file.
• Check nullity of currentInfo.getUrl() and skip retrieving of jarFile object when null.
• Check jar file path that is not under installDir system directory, when try to remove it.

Other information

[miurahr]

I hope this change solve the issue @brandelune experienced

[brandelune]

The install process works (the plugin is copied to the configuration folder / plugins subfolder) but the weird error dialog is still displayed.

[miurahr]

The install process works (the plugin is copied to the configuration folder / plugins subfolder) but the weird error dialog is still displayed.

This is not an error dialog, but confirmation dialog. no problem and looks working. Thank you for feedback.

[brandelune]

But the meaning of this dialog is not clear. What is its purpose? What is its meaning?

[miurahr]

You override existing "GUI extensions" plugin. It confirm a replacement, means remove old one and install new one.

[miurahr]

FYI @brandelune you examine the dialog when the feature was proposed and merged in 5.8 development cycle. https://github.com/omegat-org/omegat/pull/109

[brandelune]

FYI @brandelune you examine the dialog when the feature was proposed and merged in 5.8 development cycle. #109

I don't remember seeing this dialogue. If I had seen it in the context of a plugin install, I would have equally complained about its lack of clarity.

here is what I saw then:

https://github.com/omegat-org/omegat/pull/109#issuecomment-1280002278

It's very different from the new dialog.

In any case, if the plugin version is the same, the message should not be "upgraded".

[miurahr]

In any case, if the plugin version is the same, the message should not be "upgraded".

So it said "upgrade/override"

[brandelune]

In any case, if the plugin version is the same, the message should not be "upgraded".

So it said "upgrade/override"

Which is confusing.

There are 3 cases:

• no previous version of a plugin is installed the message should be "Version ... of the ... plugin was properly copied to the configuration folder."
• an older version of the plugin is installed, there should be a validation first: "OmegaT has detected version ... of the ... plugin. Do you want to replace it with version ...?" and when the new version is installed we should have the same message as above.
• the same version of the plugin is installed : "The plugin is already installed. Do you want to reinstall it?" and when the plugin is reinstalled we should have the same bessage as above.

[miurahr]

In any case, if the plugin version is the same, the message should not be "upgraded".

So it said "upgrade/override"

Which is confusing.

There are 3 cases:

• no previous version of a plugin is installed the message should be "Version ... of the ... plugin was properly copied to the configuration folder."

There is no dialog shown.

• an older version of the plugin is installed, there should be a validation first: "OmegaT has detected version ... of the ... plugin. Do you want to replace it with version ...?" and when the new version is installed we should have the same message as above.
• the same version of the plugin is installed : "The plugin is already installed. Do you want to reinstall it?" and when the plugin is reinstalled we should have the same bessage as above.

There is no logic to compare versions, because there is no rule in plugin versions, that means plugin author can give arbitrary version string. Only user can decide it.

[miurahr]

https://github.com/omegat-org/omegat/blob/release/5.7.1/docs_devel/HowToCreateFilterPlugin.md#manifest

You can find the fact in OmegaT V5.7.1 developer document;

There must be a manifest file that indicates that it is an OmegaT plugin.

There are two flavors, see below. Omegat 5.3.0 also supports to provide additional information (valid for both flavors) that can be displayed in the UI. You can optionally provide name, version, author and description.

There can be no version string in plugin.

[brandelune]

In any case, if the plugin version is the same, the message should not be "upgraded".

So it said "upgrade/override"

Which is confusing. There are 3 cases:

• no previous version of a plugin is installed the message should be "Version ... of the ... plugin was properly copied to the configuration folder."

There is no dialog shown.

There should be one to indicate that the action ended with success.

• an older version of the plugin is installed, there should be a validation first: "OmegaT has detected version ... of the ... plugin. Do you want to replace it with version ...?" and when the new version is installed we should have the same message as above.
• the same version of the plugin is installed : "The plugin is already installed. Do you want to reinstall it?" and when the plugin is reinstalled we should have the same bessage as above.

There is no logic to compare versions, because there is no rule in plugin versions, that means plugin author can give arbitrary version string. Only user can decide it.

If the version is optional, you can have a different case for plugins that don't have a version.

The point of this discussion is that the current dialogue does not mean anything.

If you don't want to deal with cases, just have one dialogue that covers everything : "The ... plugin was successfully copied to the configuration folder", and we're done.

[miurahr]

In any case, if the plugin version is the same, the message should not be "upgraded".

So it said "upgrade/override"

Which is confusing. There are 3 cases:

• no previous version of a plugin is installed the message should be "Version ... of the ... plugin was properly copied to the configuration folder."

There is no dialog shown.

There should be one to indicate that the action ended with success.

• an older version of the plugin is installed, there should be a validation first: "OmegaT has detected version ... of the ... plugin. Do you want to replace it with version ...?" and when the new version is installed we should have the same message as above.
• the same version of the plugin is installed : "The plugin is already installed. Do you want to reinstall it?" and when the plugin is reinstalled we should have the same bessage as above.

There is no logic to compare versions, because there is no rule in plugin versions, that means plugin author can give arbitrary version string. Only user can decide it.

If the version is optional, you can have a different case for plugins that don't have a version.

What do you mean? I don't understand what is "case". I have already show you the fact, OmegT team have instructed developers it is optional. Whether you want to change a specifications or not, you should keep compatibility.

The point of this discussion is that the current dialogue does not mean anything.

The dialog is informative purpose. If message is not good, you can improve it.

If you don't want to deal with cases, just have one dialogue that covers everything : "The ... plugin was successfully copied to the configuration folder", and we're done.

This dialog is shown before copying plugin file in the folder. You request to show some dialog after copying jar file and removing old one. This is last chance to stop the process before losing old plugin file from users disk.

[brandelune]

This dialog is shown before copying plugin file in the folder.

The user has no idea where she is in the process when that dialog shows up. This dialogue reads like an error message after the process, not like an alert before the process.

It should be written in plain English with a confirmation question.

[miurahr]

The message is not a scope of the proposal here. Are any issues in the fix? If not, I'd like to merge a logic hardening and wait @Kazephil to send PR for message improvement.

[Kazephil]

I've found time to test the plugin installation from disk in both the current master branch and this one, using the same ODT Review plugin as Jean-Christophe .

For reference, this is on Arch Linux, where the standard OmegaT installation is in /usr/share/java/omegat. I've set up ~/.omegat-test and ~/.omegat-master configuration directories, and I start the application with the --config-dir option.

With the latest master (using the ~/.omegat-master configuration directory), the installation fails silently. (The plugin installer seems to be trying to install the plugin to the plugins directory in the /usr/share/java/omegat/plugins OmegaT installation directory, but fails because it doesn't have write permission for that directory.) The installation does not create a plugins directory under ~/.omegat-master.

With the branch for this PR (using the ~/.omegat-test configuration directory), a plugins directory is created under ~/.omegat-test and the plugin seems to be installed correctly. In that respect, the fix seems to be correct in terms of functionality.

One thing I haven't been able to test:

• What happens if a user inadvertently installs different versions of the plugin, one in the application directory (/usr/share/java/omegat/plugins in my example) manually, and one under the configuration directory (~/.omegat-test/plugins in my example) using the Install from file option?

The dialog will need more than just changing the messages. I see several issues that need to be solved:

• The icon definitely makes it look like an error or (strong) warning message, not a confirmation one.
• I don't know if it's a problem in the plugin itself or in the installer logic, but the dialog should show the actual plugin name ("ODT Review") rather than the category (or whatever "GUI extensions" represents).
• The user should be show a message box to indicate whether the installation succeeded or failed.
• Having the same version shown (for plugins that have one) for the current and new versions is very confusing (especially under a phrase that starts with "Upgrade". Wording concerns aside, if these lines are shown, it should only be when the currently installed plugin and the plugin the user is trying to install are different. If the user is installing a plugin for the first time, the dialog should either show no version, or only the version being installed.
• If (and only if) the plugin is already installed, the dialog should add additional information about versions and make it clear that installing the chosen plugin file will overwrite the existing one.
• The dialog should show the user where the plugin will be installed, and probably give the option to change the location. (On an unrelated note, the plugins section of the Preferences should probably let the user set the location of the plugin directory (defaulting to the configuration directory) in a way similar to how users can set the location of the scripts directory. The plugin installer could then default to the directory set by the user.

I think some of these issues probably affect the code logic (although perhaps logic for a separate PR) as well, at least to some extent.

I'll handle the message keys in a separate PR, but I think we need to agree on the above first. My own rough proposal:

• Title bar: "Install plugin" (or better, if possible, "Install <name> plugin")
• Icon: Either an information type icon or none.
• Main message: "The <name> plugin will be installed in <directory name>". If possible, there should be a Browse button to let the user choose a different directory, or a drop down list of available plugins directories recognized by OmegaT. If a system directory is chosen, the user should ideally be asked to enter the administrator password, but that might be handled at the system level rather than by OmegaT itself.
• (Secondary message, if the same plugin already exists in the plugins directory:
  1. If the versions are different: "This will install version <version string for file to install> of plugin name over <version string of already installed plugin>")
  2. If the version strings are identical: "This plugin is already installed."
• Buttons: Install and Cancel
• Message telling the user if the installation was successful (or not).

It think it's worth taking the time to make sure the hardening logic and the dialog are in sync before proceeding with merging, even if several small PRs are the best way to implement everything.

@brandelune: Any more feedback on the overall workflow?

@miurahr: Which part of the differences in dialog logic fit with the logic for this fix? Which ones can (or should) be a separate PR?

[brandelune]

@brandelune: Any more feedback on the overall workflow?

I think you got everything right.

[miurahr]

Here use JOptionPane for message dialog, then we have several choices.

Method Name	Description
showConfirmDialog	Asks a confirming question, like yes/no/cancel.
showInputDialog	Prompt for some input.
showMessageDialog	Tell the user about something that has happened.
showOptionDialog	The Grand Unification of the above three.

Here we use a showConfirmDialog

messageType

Defines the style of the message. The Look and Feel manager may lay out the dialog differently depending on this value, and will often provide a default icon. The possible values are:

• ERROR_MESSAGE
• INFORMATION_MESSAGE
• WARNING_MESSAGE
• QUESTION_MESSAGE
• PLAIN_MESSAGE

Here we use ERROR_MESSAGE when installation failed, and WARNING_MESSAGE before actual copy.

optionType

Defines the set of option buttons that appear at the bottom of the dialog box:

• DEFAULT_OPTION
• YES_NO_OPTION
• YES_NO_CANCEL_OPTION
• OK_CANCEL_OPTION

When we want to show "install" or something other than "yes/no", we cannot use the method showConfirmDialog.

[miurahr]

• I don't know if it's a problem in the plugin itself or in the installer logic, but the dialog should show the actual plugin name ("ODT Review") rather than the category (or whatever "GUI extensions" represents).

We use pluginName for message. I don't know why you see category.

        String pluginName = info.getName();
       String version = info.getVersion();
        // detect current installation
        PluginInformation currentInfo = getInstalledPlugins().getOrDefault(info.getClassName(), null);
        String message;
        if (currentInfo != null) {
            message = StringUtil.format(OStrings.getString("PREFS_PLUGINS_CONFIRM_UPGRADE"), pluginName,
                    currentInfo.getVersion(), version);
        } else {
            message = StringUtil.format(OStrings.getString("PREFS_PLUGINS_CONFIRM_INSTALL"), pluginName,
                    version);
        }

As a fact, "GUI extensions" is a plugin-description of bundled plugins that class in under org.omegat.util.gui, which is configured in build.gradle. It should not be appeared for third party plugins.

[miurahr]

I found that jar file loader, URLClassLoader, does not work on Java 11 (9) and later. PluginInstaller#parsePluginJarFileManifest returns wrong information for the jar file specified.

[brandelune]

I found that jar file loader, URLClassLoader, does not work on Java 11 (9) and later. PluginInstaller#parsePluginJarFileManifest returns wrong information for the jar file specified.

Thank you for your finding. Let's not forget that 6.0 targets Java 11.

[miurahr]

PluginInstaller#parsePluginJarFileManifest method depends on classloader instrument external jar file manifest entries. It is impossible with standard URLClassLoader in Java9 and later, we have already had a custom class loader MainClassLoader class but PluginInstaller don't use it. The above fix is to use MainClassLoader instead of URLClassLoader. We also need to change MainClassLoader#addJarToClasspath method to be public.

[miurahr]

All the review feedback are adjusted. I'd like to wait for comments for dialog messages.

[Kazephil]

I'll try to test this latest version and propose updated messages in the next day or two.

[Kazephil]

I had a little time to test the latest version of this branch.

This is the dialog I see now:

It no longer looks like an error message, but I don't think everything works as intended yet.

I get the exact same dialog when I try to install a new plugin and when I choose a plugin that is already installed.

If I understand the code in lines 100 to 111 of PluginInstaller.java correctly, I should see a different dialog depending on whether or not a version of the plugin is already installed.

However, even if I delete the contents of my test configuration folder and try to install the plugin, I still get the upgrade dialog. I also tried with a plugin I had never installed in my test configuration, just in case information about the plugin having been installed in the past came from elsewhere, but I got the same results.

It looks like the plugin installer does not recognize that the plugin is not installed (at least when installing from a file), and assumes it is an update every time.

Also, it looks like the plugin information is not taken from the plugin's manifest. As you can see in the screenshot, the dialog shows "GUI extensions" for the name, and "6.1.0" for the version, but neither matches the information in the manifest:

Manifest-Version: 1.0
Plugin-Description: Export/Import translations for external reviews. S
 ponsored by California State University, Long Beach.
OmegaT-Plugins: net.briac.omegat.plugin.odtreview.ODTReviewPlugin
Plugin-Category: plugin
Plugin-Author: Briac Pilpré
Plugin-Version: 0.4.0
Plugin-Link: https://github.com/briacp/plugin-odt-review
Created-By: 1.8.0_332 (Temurin)
Plugin-Date: 2023-06-26T10:25:35+0200
Plugin-Name: ODT Review

Getting the correct information from the plugin appears to be tricky.

[Kazephil]

@miurahr

Thanks for working on this some more.

I'll try to test it sometime this week. (Vacation is over, and I expect to be busy in the first few days back to work.)

[miurahr]

I would like to port some improvement to raise robustness of plugin management from a proposed update #906 to here.

[Kazephil]

I have finally been able to test the latest version of this branch. The code seems to work as intended now.

If I install a new plugin, I get the following dialog:

The dialog now shows the correct plugin name. Excellent! :tada:

I didn't bother to take a screenshot, but I got the dialog with the "Installation successful. Restart OmegaT" message after installing the plugin.

If I try to install the same plugin again, I get a different dialog:

Question: Would it be possible to show different messages when the already installed plugin and the plugin to install:

• have different version numbers in the META-INF/MANIFEST.MF file, and
• have identical version numbers in the META-INF/MANIFEST.MF file.

Ideally, in the example above where the version of the plugin is the same, the dialog should show something like:

This version of the plugin is already installed. Reinstall?"

When the versions are different, the message would show both versions:

Update the ABC plugin?
Current version: 0.x.0
Updated version: 0.x.1

(I think the Cancel button is sufficiently self-explanatory and that there is no need for the last line about clicking it to abort.)

Also, for good measure, I also created a fake .jar (I changed the extension of a shell script to .jar) to test for failure, and got this message:

In terms of functionality, it looks pretty good now. I'll work on the messages and dialog titles later this evening.

[brandelune]

Thank you @Kazephil for checking. The behavior seems much improved. Thank you @miurahr. I'll let the two of you discuss further details.

[Kazephil]

Replying to myself as I start to work on the messages themselves...

Question: Would it be possible to show different messages when the already installed plugin and the plugin to install:

• have different version numbers in the META-INF/MANIFEST.MF file, and
• have identical version numbers in the META-INF/MANIFEST.MF file.

Ideally, in the example above where the version of the plugin is the same, the dialog should show something like:

This version of the plugin is already installed. Reinstall?"

I just realized that maybe the PREFS_PLUGINS_CONFIRM_OVERRIDE key may be intended to cover cases where the existing plugin and the plugin to install have the same version. If that's the case, the question is moot, and it's only a matter of changing that message.

(Minor note: it should be "overwrite", not "override", so I'll be changing the key name as well.)