Merge pull request #3629 from AlanCoding/nt_read_enable

Fix RBAC bugs with notification attachment

Reviewed-by: https://github.com/softwarefactory-project-zuul[bot]

awx/main/access.py

@@ -460,6 +460,42 @@ class BaseAccess(object):
         return False
 
 
+class NotificationAttachMixin(BaseAccess):
+    '''For models that can have notifications attached
+
+    I can attach a notification template when
+    - I have notification_admin_role to organization of the NT
+    - I can read the object I am attaching it to
+
+    I can unattach when those same critiera are met
+    '''
+    notification_attach_roles = None
+
+    def _can_attach(self, notification_template, resource_obj):
+        if not NotificationTemplateAccess(self.user).can_change(notification_template, {}):
+            return False
+        if self.notification_attach_roles is None:
+            return self.can_read(resource_obj)
+        return any(self.user in getattr(resource_obj, role) for role in self.notification_attach_roles)
+
+    @check_superuser
+    def can_attach(self, obj, sub_obj, relationship, data, skip_sub_obj_read_check=False):
+        if isinstance(sub_obj, NotificationTemplate):
+            # reverse obj and sub_obj
+            return self._can_attach(notification_template=sub_obj, resource_obj=obj)
+        return super(NotificationAttachMixin, self).can_attach(
+            obj, sub_obj, relationship, data, skip_sub_obj_read_check=skip_sub_obj_read_check)
+
+    @check_superuser
+    def can_unattach(self, obj, sub_obj, relationship, data=None):
+        if isinstance(sub_obj, NotificationTemplate):
+            # due to this special case, we use symmetrical logic with attach permission
+            return self._can_attach(notification_template=sub_obj, resource_obj=obj)
+        return super(NotificationAttachMixin, self).can_unattach(
+            obj, sub_obj, relationship, relationship, data=data
+        )
+
+
 class InstanceAccess(BaseAccess):
 
     model = Instance
@@ -715,7 +751,7 @@ class OAuth2TokenAccess(BaseAccess):
         return True
 
 
-class OrganizationAccess(BaseAccess):
+class OrganizationAccess(NotificationAttachMixin, BaseAccess):
     '''
     I can see organizations when:
      - I am a superuser.
@@ -729,6 +765,8 @@ class OrganizationAccess(BaseAccess):
 
     model = Organization
     prefetch_related = ('created_by', 'modified_by',)
+    # organization admin_role is not a parent of organization auditor_role
+    notification_attach_roles = ['admin_role', 'auditor_role']
 
     def filtered_queryset(self):
         return self.model.accessible_objects(self.user, 'read_role')
@@ -966,7 +1004,7 @@ class GroupAccess(BaseAccess):
         return False
 
 
-class InventorySourceAccess(BaseAccess):
+class InventorySourceAccess(NotificationAttachMixin, BaseAccess):
     '''
     I can see inventory sources whenever I can see their inventory.
     I can change inventory sources whenever I can change their inventory.
@@ -1282,7 +1320,7 @@ class TeamAccess(BaseAccess):
                                                     *args, **kwargs)
 
 
-class ProjectAccess(BaseAccess):
+class ProjectAccess(NotificationAttachMixin, BaseAccess):
     '''
     I can see projects when:
      - I am a superuser.
@@ -1301,6 +1339,7 @@ class ProjectAccess(BaseAccess):
 
     model = Project
     select_related = ('modified_by', 'credential', 'current_job', 'last_job',)
+    notification_attach_roles = ['admin_role']
 
     def filtered_queryset(self):
         return self.model.accessible_objects(self.user, 'read_role')
@@ -1363,7 +1402,7 @@ class ProjectUpdateAccess(BaseAccess):
         return obj and self.user in obj.project.admin_role
 
 
-class JobTemplateAccess(BaseAccess):
+class JobTemplateAccess(NotificationAttachMixin, BaseAccess):
     '''
     I can see job templates when:
      - I have read role for the job template.
@@ -1514,8 +1553,6 @@ class JobTemplateAccess(BaseAccess):
 
     @check_superuser
     def can_attach(self, obj, sub_obj, relationship, data, skip_sub_obj_read_check=False):
-        if isinstance(sub_obj, NotificationTemplate):
-            return self.check_related('organization', Organization, {}, obj=sub_obj, mandatory=True)
         if relationship == "instance_groups":
             if not obj.project.organization:
                 return False
@@ -1913,7 +1950,7 @@ class WorkflowJobNodeAccess(BaseAccess):
 
 
 # TODO: notification attachments?
-class WorkflowJobTemplateAccess(BaseAccess):
+class WorkflowJobTemplateAccess(NotificationAttachMixin, BaseAccess):
     '''
     I can only see/manage Workflow Job Templates if I'm a super user
     '''

awx/main/tests/functional/test_rbac_notifications.py

@@ -1,5 +1,6 @@
 import pytest
 
+from awx.main.models import Organization, Project
 from awx.main.access import (
     NotificationTemplateAccess,
     NotificationAccess,
@@ -137,6 +138,106 @@ def test_system_auditor_JT_attach(system_auditor, job_template, notification_tem
         {'id': notification_template.id})
 
 
+@pytest.mark.django_db
+@pytest.mark.parametrize("org_role,expect", [
+    ('admin_role', True),
+    ('notification_admin_role', True),
+    ('workflow_admin_role', False),
+    ('auditor_role', False),
+    ('member_role', False)
+])
+def test_org_role_JT_attach(rando, job_template, project, workflow_job_template, inventory_source,
+                            notification_template, org_role, expect):
+    nt_organization = Organization.objects.create(name='organization just for the notification template')
+    notification_template.organization = nt_organization
+    notification_template.save()
+    getattr(notification_template.organization, org_role).members.add(rando)
+    kwargs = dict(
+        sub_obj=notification_template,
+        relationship='notification_templates_success',
+        data={'id': notification_template.id}
+    )
+    permissions = {}
+    expected_permissions = {}
+    organization = Organization.objects.create(name='objective organization')
+
+    for resource in (organization, job_template, project, workflow_job_template, inventory_source):
+        permission_resource = resource
+        if resource == inventory_source:
+            permission_resource = inventory_source.inventory
+        getattr(permission_resource, 'admin_role').members.add(rando)
+        model_name = resource.__class__.__name__
+        permissions[model_name] = rando.can_access(resource.__class__, 'attach', resource, **kwargs)
+        expected_permissions[model_name] = expect
+
+    assert permissions == expected_permissions
+
+
+@pytest.mark.django_db
+def test_organization_NT_attach_permission(rando, notification_template):
+    notification_template.organization.notification_admin_role.members.add(rando)
+    target_organization = Organization.objects.create(name='objective organization')
+    target_organization.workflow_admin_role.members.add(rando)
+    assert not rando.can_access(Organization, 'attach', obj=target_organization, sub_obj=notification_template,
+                                relationship='notification_templates_success', data={})
+    target_organization.auditor_role.members.add(rando)
+    assert rando.can_access(Organization, 'attach', obj=target_organization, sub_obj=notification_template,
+                            relationship='notification_templates_success', data={})
+
+
+@pytest.mark.django_db
+def test_project_NT_attach_permission(rando, notification_template):
+    notification_template.organization.notification_admin_role.members.add(rando)
+    project = Project.objects.create(
+        name='objective project',
+        organization=Organization.objects.create(name='foo')
+    )
+    project.update_role.members.add(rando)
+    assert not rando.can_access(Project, 'attach', obj=project, sub_obj=notification_template,
+                                relationship='notification_templates_success', data={})
+    project.admin_role.members.add(rando)
+    assert rando.can_access(Project, 'attach', obj=project, sub_obj=notification_template,
+                            relationship='notification_templates_success', data={})
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize("res_role,expect", [
+    ('read_role', True),
+    (None, False)
+])
+def test_object_role_JT_attach(rando, job_template, workflow_job_template, inventory_source,
+                               notification_template, res_role, expect):
+    nt_organization = Organization.objects.create(name='organization just for the notification template')
+    nt_organization.notification_admin_role.members.add(rando)
+    notification_template.organization = nt_organization
+    notification_template.save()
+    kwargs = dict(
+        sub_obj=notification_template,
+        relationship='notification_templates_success',
+        data={'id': notification_template.id}
+    )
+    permissions = {}
+    expected_permissions = {}
+
+    for resource in (job_template, workflow_job_template, inventory_source):
+        permission_resource = resource
+        if resource == inventory_source:
+            permission_resource = inventory_source.inventory
+        model_name = resource.__class__.__name__
+        if res_role is None or hasattr(permission_resource, res_role):
+            if res_role is not None:
+                getattr(permission_resource, res_role).members.add(rando)
+            permissions[model_name] = rando.can_access(
+                resource.__class__, 'attach', resource, **kwargs
+            )
+            expected_permissions[model_name] = expect
+        else:
+            permissions[model_name] = None
+            expected_permissions[model_name] = None
+
+    assert permissions == expected_permissions
+
+
 @pytest.mark.django_db
 def test_notification_access_org_admin(notification, org_admin):
     access = NotificationAccess(org_admin)

awx/ui/client/src/inventories-hosts/inventories/related/sources/edit/sources-edit.controller.js

@@ -17,9 +17,14 @@ export default ['$state', '$scope', 'ParseVariableString', 'ParseTypeChange',
 
         const inventorySourceData = inventorySource.get();
 
+        // To toggle notifications a user needs to have a read role on the inventory
+        // _and_ have at least a notification template admin role on an org.
+        // If the user has gotten this far it's safe to say they have
+        // at least read access to the inventory
+        $scope.sufficientRoleForNotifToggle = isNotificationAdmin;
+        $scope.sufficientRoleForNotif =  isNotificationAdmin || $scope.user_is_system_auditor;
         $scope.projectBasePath = GetBasePath('projects') + '?not__status=never updated';
         $scope.canAdd = inventorySourcesOptions.actions.POST;
-        $scope.isNotificationAdmin = isNotificationAdmin || false;
         const virtualEnvs = ConfigData.custom_virtualenvs || [];
         $scope.custom_virtualenvs_options = virtualEnvs;
         // instantiate expected $scope values from inventorySourceData

awx/ui/client/src/inventories-hosts/inventories/related/sources/sources.form.js

@@ -9,7 +9,7 @@ export default ['NotificationsList', 'i18n', function(NotificationsList, i18n){
     var notifications_object = {
         generateList: true,
         include: "NotificationsList",
-        ngIf: "(current_user.is_superuser || isOrgAdmin || isNotificationAdmin) && !(inventory_source_obj.source === undefined || inventory_source_obj.source === '')",
+        ngIf: "(sufficientRoleForNotif) && !(inventory_source_obj.source === undefined || inventory_source_obj.source === '')",
         ngClick: "$state.go('inventories.edit.inventory_sources.edit.notifications')"
     };
     let clone = _.clone(NotificationsList);

awx/ui/client/src/notifications/notifications.list.js

@@ -20,7 +20,7 @@ export default ['i18n', 'templateUrl', function(i18n, templateUrl){
         hover: false,
         emptyListText: i18n.sprintf(i18n._("This list is populated by notification templates added from the %sNotifications%s section"), "&nbsp;<a ui-sref='notifications.add'>", "</a>&nbsp;"),
         basePath: 'notification_templates',
-        ngIf: 'current_user.is_superuser || isOrgAdmin || isNotificationAdmin',
+        ngIf: 'sufficientRoleForNotif',
         fields: {
             name: {
                 key: true,
@@ -40,7 +40,7 @@ export default ['i18n', 'templateUrl', function(i18n, templateUrl){
                 flag: 'notification_templates_success',
                 type: "toggle",
                 ngClick: "toggleNotification($event, notification.id, \"notification_templates_success\")",
-                ngDisabled: "!(current_user.is_superuser || isOrgAdmin)",
+                ngDisabled: "!sufficientRoleForNotifToggle",
                 awToolTip: "{{ schedule.play_tip }}",
                 dataTipWatch: "schedule.play_tip",
                 dataPlacement: "right",
@@ -53,7 +53,7 @@ export default ['i18n', 'templateUrl', function(i18n, templateUrl){
                 flag: 'notification_templates_error',
                 type: "toggle",
                 ngClick: "toggleNotification($event, notification.id, \"notification_templates_error\")",
-                ngDisabled: "!(current_user.is_superuser || isOrgAdmin)",
+                ngDisabled: "!sufficientRoleForNotifToggle",
                 awToolTip: "{{ schedule.play_tip }}",
                 dataTipWatch: "schedule.play_tip",
                 dataPlacement: "right",
@@ -64,7 +64,7 @@ export default ['i18n', 'templateUrl', function(i18n, templateUrl){
             add: {
                 type: 'template',
                 template: templateUrl('notifications/notification-templates-list/add-notifications-action'),
-                ngShow: 'current_user.is_superuser || (current_user_admin_orgs && current_user_admin_orgs.length > 0)'
+                ngShow: 'isNotificationAdmin'
             }
         }
 

awx/ui/client/src/notifications/shared/notification-list-init.factory.js

@@ -22,14 +22,6 @@ export default ['Wait', 'GetBasePath', 'ProcessErrors', 'Rest', 'GetChoices',
             url = params.url,
             id = params.id;
 
-        scope.current_user_admin_orgs = [];
-
-        Rest.setUrl($rootScope.current_user.related.admin_of_organizations);
-        Rest.get()
-            .then(({data}) => {
-                scope.current_user_admin_orgs = data.results.map(i => i.name);
-            });
-
         scope.addNotificationTemplate = function() {
             var org_id;
             if($stateParams.hasOwnProperty('project_id')){

awx/ui/client/src/organizations/edit/organizations-edit.controller.js

@@ -4,11 +4,11 @@
  * All Rights Reserved
  *************************************************/
 
-export default ['$scope', '$location', '$stateParams', 'OrgAdminLookup',
+export default ['$scope', '$location', '$stateParams', 'isOrgAdmin', 'isNotificationAdmin',
-    'OrganizationForm', 'Rest', 'ProcessErrors', 'Prompt', '$rootScope', 'i18n',
+    'OrganizationForm', 'Rest', 'ProcessErrors', 'Prompt', 'i18n', 'isOrgAuditor',
     'GetBasePath', 'Wait', '$state', 'ToggleNotification', 'CreateSelect2', 'InstanceGroupsService', 'InstanceGroupsData', 'ConfigData',
-    function($scope, $location, $stateParams, OrgAdminLookup,
+    function($scope, $location, $stateParams, isOrgAdmin, isNotificationAdmin,
-        OrganizationForm, Rest, ProcessErrors, Prompt, $rootScope, i18n,
+        OrganizationForm, Rest, ProcessErrors, Prompt, i18n, isOrgAuditor,
         GetBasePath, Wait, $state, ToggleNotification, CreateSelect2, InstanceGroupsService, InstanceGroupsData, ConfigData) {
 
         let form = OrganizationForm(),
@@ -18,34 +18,22 @@ export default ['$scope', '$location', '$stateParams', 'OrgAdminLookup',
             id = $stateParams.organization_id,
             instance_group_url = defaultUrl + id + '/instance_groups/';
 
-        init();
+        $scope.isOrgAuditor = isOrgAuditor;
+        $scope.isOrgAdmin = isOrgAdmin;
+        $scope.isNotificationAdmin = isNotificationAdmin;
 
-        function init() {
+        $scope.$watch('organization_obj.summary_fields.user_capabilities.edit', function(val) {
-            OrgAdminLookup.checkForAdminAccess({organization: id})
+            if (val === false) {
-                .then(function(isOrgAdmin){
+                $scope.canAdd = false;
-                    $scope.isOrgAdmin = isOrgAdmin;
+            }
-                });
+        });
-
-            Rest.setUrl(GetBasePath('users') + $rootScope.current_user.id + '/roles/?role_field=notification_admin_role');
-            Rest.get()
-                .then(({data}) => {
-                    $scope.isNotificationAdmin = (data.count && data.count > 0);
-                });
-
-            $scope.$watch('organization_obj.summary_fields.user_capabilities.edit', function(val) {
-                if (val === false) {
-                    $scope.canAdd = false;
-                }
-            });
-
-            $scope.instance_groups = InstanceGroupsData;
-            const virtualEnvs = ConfigData.custom_virtualenvs || [];
-            $scope.custom_virtualenvs_visible = virtualEnvs.length > 1;
-            $scope.custom_virtualenvs_options = virtualEnvs.filter(
-                v => !/\/ansible\/$/.test(v)
-            );
-        }
 
+        $scope.instance_groups = InstanceGroupsData;
+        const virtualEnvs = ConfigData.custom_virtualenvs || [];
+        $scope.custom_virtualenvs_visible = virtualEnvs.length > 1;
+        $scope.custom_virtualenvs_options = virtualEnvs.filter(
+            v => !/\/ansible\/$/.test(v)
+        );
 
         // Retrieve detail record and prepopulate the form
         Wait('start');
@@ -54,6 +42,15 @@ export default ['$scope', '$location', '$stateParams', 'OrgAdminLookup',
         .then(({data}) => {
             let fld;
 
+            $scope.sufficientRoleForNotifToggle = 
+                isNotificationAdmin && (
+                    $scope.is_system_auditor || 
+                    isOrgAuditor ||
+                    isOrgAdmin
+                );
+
+            $scope.sufficientRoleForNotif =  isNotificationAdmin || isOrgAuditor || $scope.user_is_system_auditor;
+            
             $scope.organization_name = data.name;
             for (fld in form.fields) {
                 if (typeof data[fld] !== 'undefined') {
@@ -169,4 +166,4 @@ export default ['$scope', '$location', '$stateParams', 'OrgAdminLookup',
 
         };
     }
-];
+];

awx/ui/client/src/organizations/main.js

@@ -98,7 +98,49 @@ angular.module('Organizations', [
                                                     'status: ' + status
                                             });
                                     });
-                                }]
+                            }],
+                            isOrgAuditor: ['Rest', 'ProcessErrors', 'GetBasePath', 'i18n', '$stateParams',
+                                function(Rest, ProcessErrors, GetBasePath, i18n, $stateParams) {
+                                    Rest.setUrl(`${GetBasePath('organizations')}?role_level=auditor_role&id=${$stateParams.organization_id}`);
+                                    return Rest.get()
+                                        .then(({data}) => {
+                                            return data.count > 0;
+                                        })
+                                        .catch(({data, status}) => {
+                                            ProcessErrors(null, data, status, null, {
+                                                hdr: i18n._('Error!'),
+                                                msg: i18n._('Failed while checking to see if user is a notification administrator of this organization. GET returned ') + status
+                                            });
+                                    });
+                            }],
+                            isOrgAdmin: ['ProcessErrors', 'i18n', '$stateParams', 'OrgAdminLookup',
+                                function(ProcessErrors, i18n, $stateParams, OrgAdminLookup) {
+                                    return OrgAdminLookup.checkForAdminAccess({organization: $stateParams.organization_id})
+                                    .then(function(isOrgAdmin){
+                                        return isOrgAdmin;
+                                    })
+                                    .catch(({data, status}) => {
+                                        ProcessErrors(null, data, status, null, {
+                                            hdr: i18n._('Error!'),
+                                            msg: i18n._('Failed while checking to see if user is an administrator of this organization. GET returned ') + status
+                                        });
+                                    });
+                                    
+                            }],
+                            isNotificationAdmin: ['Rest', 'ProcessErrors', 'GetBasePath', 'i18n',
+                                function(Rest, ProcessErrors, GetBasePath, i18n) {
+                                    Rest.setUrl(`${GetBasePath('organizations')}?role_level=notification_admin_role&page_size=1`);
+                                    return Rest.get()
+                                        .then(({data}) => {
+                                            return data.count > 0;
+                                        })
+                                        .catch(({data, status}) => {
+                                            ProcessErrors(null, data, status, null, {
+                                                hdr: i18n._('Error!'),
+                                                msg: i18n._('Failed to get organizations for which this user is a notification admin. GET returned ') + status
+                                            });
+                                    });
+                            }],
                         }
                     }
                     // concat manually-defined state definitions with generated defintions

awx/ui/client/src/projects/edit/projects-edit.controller.js

@@ -20,15 +20,10 @@ export default ['$scope', '$rootScope', '$stateParams', 'ProjectsForm', 'Rest',
             master = {},
             id = $stateParams.project_id;
 
-        init();
+        $scope.project_local_paths = [];
-
+        $scope.base_dir = '';
-        function init() {
+        const virtualEnvs = ConfigData.custom_virtualenvs || [];
-            $scope.project_local_paths = [];
+        $scope.custom_virtualenvs_options = virtualEnvs;
-            $scope.base_dir = '';
-            const virtualEnvs = ConfigData.custom_virtualenvs || [];
-            $scope.custom_virtualenvs_options = virtualEnvs;
-            $scope.isNotificationAdmin = isNotificationAdmin || false;
-        }
 
         $scope.$watch('project_obj.summary_fields.user_capabilities.edit', function(val) {
             if (val === false) {
@@ -144,6 +139,12 @@ export default ['$scope', '$rootScope', '$stateParams', 'ProjectsForm', 'Rest',
                     });
 
                     $scope.project_obj = data;
+                    // To toggle notifications a user needs to have an admin role on the project
+                    // _and_ have at least a notification template admin role on an org.
+                    // Only users with admin role on the project can edit it which is why we
+                    // look at that user_capability
+                    $scope.sufficientRoleForNotifToggle = isNotificationAdmin && data.summary_fields.user_capabilities.edit;
+                    $scope.sufficientRoleForNotif =  isNotificationAdmin || $scope.user_is_system_auditor;
                     $scope.name = data.name;
                     $scope.breadcrumb.project_name = data.name;
                     $scope.$emit('projectLoaded');

awx/ui/client/src/templates/job_templates/edit-job-template/job-template-edit.controller.js

@@ -54,6 +54,12 @@ export default
 
                 CallbackHelpInit({ scope: $scope });
 
+                // To toggle notifications a user needs to have a read role on the JT
+                // _and_ have at least a notification template admin role on an org.
+                // If the user has gotten this far it's safe to say they have
+                // at least read access to the JT
+                $scope.sufficientRoleForNotifToggle = isNotificationAdmin;
+                $scope.sufficientRoleForNotif =  isNotificationAdmin || $scope.user_is_system_auditor;
                 $scope.playbook_options = null;
                 $scope.playbook = null;
                 $scope.mode = 'edit';
@@ -66,7 +72,6 @@ export default
                 $scope.skip_tag_options = [];
                 const virtualEnvs = ConfigData.custom_virtualenvs || [];
                 $scope.custom_virtualenvs_options = virtualEnvs;
-                $scope.isNotificationAdmin = isNotificationAdmin || false;
 
                 SurveyControllerInit({
                     scope: $scope,

awx/ui/client/src/templates/workflows/edit-workflow/workflow-edit.controller.js

@@ -18,6 +18,12 @@ export default [
         workflowLaunch, $transitions, WorkflowJobTemplate, Inventory, isNotificationAdmin
     ) {
 
+        // To toggle notifications a user needs to have a read role on the WFJT
+        // _and_ have at least a notification template admin role on an org.
+        // If the user has gotten this far it's safe to say they have
+        // at least read access to the WFJT
+        $scope.sufficientRoleForNotifToggle = isNotificationAdmin;
+        $scope.sufficientRoleForNotif =  isNotificationAdmin || $scope.user_is_system_auditor;
         $scope.missingTemplates = _.has(workflowLaunch, 'node_templates_missing') && workflowLaunch.node_templates_missing.length > 0 ? true : false;
 
         $scope.$watch('workflow_job_template_obj.summary_fields.user_capabilities.edit', function(val) {
@@ -26,8 +32,6 @@ export default [
             }
         });
 
-        $scope.isNotificationAdmin = isNotificationAdmin || false;
-
         const criteriaObj = {
             from: (state) => state.name === 'templates.editWorkflowJobTemplate.workflowMaker',
             to: (state) => state.name === 'templates.editWorkflowJobTemplate'