Uploaded image for project: 'Blesta Core'
  1. Blesta Core
  2. CORE-2567

Upgrading a plugin adds permissions for only one company

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.9.1, 4.10.0-b1
    • Component/s: None
    • Labels:
      None

      Description

      When a plugin is upgraded, all instances of the plugin on other companies are also upgraded since they share database tables. New permissions however are only added for the instance upgraded. This should be updated to add the permission for all plugin instances.

        Activity

        Hide
        tyson Tyson Phillips (Inactive) added a comment -

        The only backward compatible integration I see here is to assume the plugin will set permissions for all copies of the plugin during upgrade. So the solution would be for each plugin to implement any permissions across all installed plugins. The plugin system would have to be refactored to change the way that plugins handle multiple instances of a single copy and that would require a major version change to accomplish, and likely updates to all plugins.

        Show
        tyson Tyson Phillips (Inactive) added a comment - The only backward compatible integration I see here is to assume the plugin will set permissions for all copies of the plugin during upgrade. So the solution would be for each plugin to implement any permissions across all installed plugins. The plugin system would have to be refactored to change the way that plugins handle multiple instances of a single copy and that would require a major version change to accomplish, and likely updates to all plugins.
        Hide
        jonathan Jonathan Reissmueller added a comment -

        I'm curious your thoughts on this Abdy. Tyson seemed to think that the burden should be on the plugins to ensure that all installation correctly added permission. Personally I think that the plugin manager could be updated to make this easier for them. Regardless, we have a lot of missing plugin permissions. I'd like to see an upgrade in the core the searches for these missing permissions and adds them.

        Show
        jonathan Jonathan Reissmueller added a comment - I'm curious your thoughts on this Abdy. Tyson seemed to think that the burden should be on the plugins to ensure that all installation correctly added permission. Personally I think that the plugin manager could be updated to make this easier for them. Regardless, we have a lot of missing plugin permissions. I'd like to see an upgrade in the core the searches for these missing permissions and adds them.
        Hide
        abdy Abdy Franco added a comment - - edited

        Jonathan Reissmueller I see that the plugins have the permissions inside upgrade() and install(), maybe we could save the permissions inside a function in the plugin class? This way the core would have access to this function and the core would be in charge of verifying and adding the permissions of the plugins to all the companies, this way the plugin would not have to add the permissions in upgrade() or install() and the core would be in charge of managing the permissions of the plugins.

        <?php
        class OrderPlugin extends Plugin
        {
            public function permissions($plugin_id) {
                $permissions = [];
        
                $group = $this->Permissions->getGroupByAlias('admin_packages');
                $permissions[] = [
                    'plugin_id' => $plugin_id,
                    'group_id' => $group->id,
                    'name' => Language::_('OrderPlugin.admin_forms.name', true),
                    'alias' => 'order.admin_forms',
                    'action' => '*'
                ];
        
                // ...
        
                return $permissions;
            }
        }
        
        Show
        abdy Abdy Franco added a comment - - edited Jonathan Reissmueller I see that the plugins have the permissions inside upgrade() and install(), maybe we could save the permissions inside a function in the plugin class? This way the core would have access to this function and the core would be in charge of verifying and adding the permissions of the plugins to all the companies, this way the plugin would not have to add the permissions in upgrade() or install() and the core would be in charge of managing the permissions of the plugins. <?php class OrderPlugin extends Plugin { public function permissions($plugin_id) { $permissions = []; $group = $this->Permissions->getGroupByAlias('admin_packages'); $permissions[] = [ 'plugin_id' => $plugin_id, 'group_id' => $group->id, 'name' => Language::_('OrderPlugin.admin_forms.name', true), 'alias' => 'order.admin_forms', 'action' => '*' ]; // ... return $permissions; } }
        Hide
        abdy Abdy Franco added a comment -

        However, this would mean updating all the plugins to add this new function, and I don't know to what extent it could break the backwards compatibility

        Show
        abdy Abdy Franco added a comment - However, this would mean updating all the plugins to add this new function, and I don't know to what extent it could break the backwards compatibility
        Hide
        jonathan Jonathan Reissmueller added a comment -

        Backward incompatibility? You're speaking of adding a method to the Plugin abstract class that accept permission parameters and ensures the are added for all instances of a plugin, correct? Simply adding a protected method would not make it backward incompatible. You are correct in saying that we would need to update plugins to use this new method though.

        Show
        jonathan Jonathan Reissmueller added a comment - Backward incompatibility? You're speaking of adding a method to the Plugin abstract class that accept permission parameters and ensures the are added for all instances of a plugin, correct? Simply adding a protected method would not make it backward incompatible. You are correct in saying that we would need to update plugins to use this new method though.
        Hide
        abdy Abdy Franco added a comment - - edited

        This new method will be similar to install() or upgrade() it will only receive plugin_id as the parameter and will return an array of all the plugin permissions, the plugin manager should be updated as well, as will need to add the permissions during installation and check what new permissions should be added on upgrade as well for all companies.

        Show
        abdy Abdy Franco added a comment - - edited This new method will be similar to install() or upgrade() it will only receive plugin_id as the parameter and will return an array of all the plugin permissions, the plugin manager should be updated as well, as will need to add the permissions during installation and check what new permissions should be added on upgrade as well for all companies.
        Hide
        jonathan Jonathan Reissmueller added a comment -

        I see. Well in that case I don't think there is any need to make this backward incompatible. It is simple enough to create a default method on the Plugin class that returns an empty array. Then all plugins that don't implement the method will continue to function in the current fashion.

        Is there a need for the $plugin_id parameter? It is just getting a list of permissions, yes?

        We'll also probably want to remove the permissions on uninstall. Upgrade should check permissions on every instance of the plugin, while install and uninstall should only effect the particular plugin the action is being taken on.

        Do we have what we need to get started on this? I'd love to get 4.9.1 out by the end of the week.

        Show
        jonathan Jonathan Reissmueller added a comment - I see. Well in that case I don't think there is any need to make this backward incompatible. It is simple enough to create a default method on the Plugin class that returns an empty array. Then all plugins that don't implement the method will continue to function in the current fashion. Is there a need for the $plugin_id parameter? It is just getting a list of permissions, yes? We'll also probably want to remove the permissions on uninstall. Upgrade should check permissions on every instance of the plugin, while install and uninstall should only effect the particular plugin the action is being taken on. Do we have what we need to get started on this? I'd love to get 4.9.1 out by the end of the week.
        Hide
        abdy Abdy Franco added a comment -

        I think we can remove the $plugin_id parameter as the method will only return a list of permissions, I see it possible to have it ready by the end of the week.

        Show
        abdy Abdy Franco added a comment - I think we can remove the $plugin_id parameter as the method will only return a list of permissions, I see it possible to have it ready by the end of the week.
        Hide
        jonathan Jonathan Reissmueller added a comment -

        I've got some time this afternoon so I'll go ahead and pick this one up and let you focus on filtering

        Show
        jonathan Jonathan Reissmueller added a comment - I've got some time this afternoon so I'll go ahead and pick this one up and let you focus on filtering
        Hide
        abdy Abdy Franco added a comment - - edited

        If you need help on the task, just let me know.

        Show
        abdy Abdy Franco added a comment - - edited If you need help on the task, just let me know.

          People

          • Assignee:
            jonathan Jonathan Reissmueller
            Reporter:
            jonathan Jonathan Reissmueller
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Fix Release Date:
              23/Apr/20

              Time Tracking

              Estimated:
              Original Estimate - Not Specified
              Not Specified
              Remaining:
              Remaining Estimate - 0 minutes
              0m
              Logged:
              Time Spent - 6 hours, 49 minutes
              6h 49m

                Agile