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

        jonathan Jonathan Reissmueller created issue -
        tyson Tyson Phillips (Inactive) made changes -
        Field Original Value New Value
        Rank Ranked higher
        tyson Tyson Phillips (Inactive) made changes -
        Sprint 4.3.0 Sprint 1 [ 49 ]
        tyson Tyson Phillips (Inactive) made changes -
        Rank Ranked lower
        tyson Tyson Phillips (Inactive) made changes -
        Rank Ranked higher
        tyson Tyson Phillips (Inactive) made changes -
        Remaining Estimate 0 minutes [ 0 ]
        Time Spent 1 hour, 39 minutes [ 5940 ]
        Worklog Id 10670 [ 10670 ]
        tyson Tyson Phillips (Inactive) made changes -
        Sprint 4.3.0 Sprint 1 [ 49 ]
        tyson Tyson Phillips (Inactive) made changes -
        Rank Ranked lower
        tyson Tyson Phillips (Inactive) made changes -
        Sprint 4.3.0 Sprint 3 [ 53 ]
        tyson Tyson Phillips (Inactive) made changes -
        Rank Ranked higher
        tyson Tyson Phillips (Inactive) made changes -
        Fix Version/s 4.2.2 [ 11021 ]
        Fix Version/s 4.3.0-b1 [ 11019 ]
        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.
        tyson Tyson Phillips (Inactive) made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 4.3.0-b1 [ 11019 ]
        Fix Version/s 4.2.2 [ 11021 ]
        Resolution Won't Fix [ 2 ]
        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.
        jonathan Jonathan Reissmueller made changes -
        Resolution Won't Fix [ 2 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Assignee Tyson Phillips [ tyson ] Abdy Franco [ abdy ]
        jonathan Jonathan Reissmueller made changes -
        Fix Version/s 4.9.1 [ 11400 ]
        jonathan Jonathan Reissmueller made changes -
        Sprint 4.3.0 Sprint 3 [ 53 ] 4.3.0 Sprint 3, 4.10.0 Sprint 2 [ 53, 106 ]
        jonathan Jonathan Reissmueller made changes -
        Rank Ranked lower
        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
        jonathan Jonathan Reissmueller made changes -
        Assignee Abdy Franco [ abdy ] Jonathan Reissmueller [ jonathan ]
        Automated transition triggered when Jonathan Reissmueller created a branch in Stash -
        Status Reopened [ 4 ] In Progress [ 3 ]
        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.
        jonathan Jonathan Reissmueller made changes -
        Time Spent 1 hour, 39 minutes [ 5940 ] 4 hours, 44 minutes [ 17040 ]
        Worklog Id 13549 [ 13549 ]
        Automated transition triggered when Jonathan Reissmueller created pull request #852 in Stash -
        Status In Progress [ 3 ] In Review [ 5 ]
        Resolution Fixed [ 1 ]
        abdy Abdy Franco made changes -
        Time Spent 4 hours, 44 minutes [ 17040 ] 5 hours, 29 minutes [ 19740 ]
        Worklog Id 13550 [ 13550 ]
        Automated transition triggered when Jonathan Reissmueller merged pull request #852 in Stash -
        Status In Review [ 5 ] Closed [ 6 ]
        jonathan Jonathan Reissmueller made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Automated transition triggered when Jonathan Reissmueller created a branch in Stash -
        Status Reopened [ 4 ] In Progress [ 3 ]
        Automated transition triggered when Jonathan Reissmueller created pull request #854 in Stash -
        Status In Progress [ 3 ] In Review [ 5 ]
        Resolution Fixed [ 1 ]
        jonathan Jonathan Reissmueller made changes -
        Time Spent 5 hours, 29 minutes [ 19740 ] 6 hours, 29 minutes [ 23340 ]
        Worklog Id 13551 [ 13551 ]
        abdy Abdy Franco made changes -
        Time Spent 6 hours, 29 minutes [ 23340 ] 6 hours, 49 minutes [ 24540 ]
        Worklog Id 13552 [ 13552 ]
        Automated transition triggered when Jonathan Reissmueller merged pull request #854 in Stash -
        Status In Review [ 5 ] Closed [ 6 ]
        jonathan Jonathan Reissmueller made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        jonathan Jonathan Reissmueller made changes -
        Fix Version/s 4.10.0-b1 [ 11305 ]
        jonathan Jonathan Reissmueller made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]

          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