salesagility / SuiteCRM

SuiteCRM - Open source CRM for the world
https://www.suitecrm.com
GNU Affero General Public License v3.0
4.52k stars 2.09k forks source link

installdef `administration` may overwrite other plugins' admindefs #8569

Open strangedev opened 4 years ago

strangedev commented 4 years ago

Issue

The module loader overwrites existing admindefs when using the administration installdef.

Expected Behavior

The name of the admindef file should not matter, since a plugin vendor has no way of knowing which names were chosen by other plugin vendors. At the very least, this caveat should be included in the documentation.

Actual Behavior

When installing admindefs through the administration installdef, the file provided in from is simply copied to custom/Extension/modules/Administration/Ext/Administration/, regardless of whether a file of the same name exists. When multiple plugins use the same filename in the administration installdef, this causes the module loader to overwrite the previously installed admindefs.

Possible Fix

The module loader should either rename the file on conflict or use a random file name when copying the admindef file.

Steps to Reproduce

I have assembled a minimal example

  1. Build the two plugins by running build.sh.
  2. Install pluginA through the module loader.
  3. Install pluginB through the module loader.
  4. Navigate to the admin section and observe that only pluginB appears.

Context

I am a plugin vendor and chose using the plugin infrastructure to avoid conflicts between plugins. Since I'm using a plugin template, the admindefs file was named the same for each plugin. I was not able to figure out why the admin panel would not appear for several days. This discourages plugin development.

Your Environment

pgorod commented 4 years ago

Thanks for the detailed report (and the "minimal example").

Do you happen to know the place in core code where this is handled?

strangedev commented 4 years ago

A quick grep through the codebase leads me to believe it is handled in ModuleInstall/ModuleInstaller.php. It seems the administration installdef is handled by the ModuleInstaller::install_extensions() method, which in turn calls ModuleInstaller::installExt(). I see some filesystem operations happening in this method, which suggests to me that the issue lies there.

This would also suggest that the administration installdef is not the only one affected, as this method handles installation of all extensions which do not have a separate ModuleInstaller::install_<module_name>() method.

strangedev commented 4 years ago

Also, I have a hunch (because of some issues when trying to uninstall plugins when the upload directory was reset) that the uninstall procedure uses the manifest file to remove the plugin files from Suite. This would make renaming the files difficult, because we'd lose track of what files belong to which plugin. An easy "fix" would be to just document this caveat.