From b898d2e3c08b50e576ee849fbe8614c66f360c62 Mon Sep 17 00:00:00 2001
From: Slawek Kaplonski <skaplons@redhat.com>
Date: Thu, 12 Sep 2019 22:02:52 +0200
Subject: [PATCH] List SG rules which belongs to tenant's SG

In case when user's security group contains rules created e.g.
by admin, and such rules has got admin's tenant as tenant_id,
owner of security group should be able to see those rules.
Some time ago this was addressed for request:

GET /v2.0/security-groups/<sec_group_id>

But it is also required to behave in same way for

GET /v2.0/security-group-rules

So this patch fixes this behaviour for listing of security
group rules.
To achieve that this patch also adds new policy rule:
ADMIN_OWNER_OR_SG_OWNER which is similar to already existing
ADMIN_OWNER_OR_NETWORK_OWNER used e.g. for listing or creating
ports.

Change-Id: I09114712582d2d38d14cf1683b87a8ce3a8e8c3c
Closes-Bug: #1824248
---
 neutron/conf/policies/security_group.py          | 16 +++++++++++++++-
 neutron/db/securitygroups_db.py                  | 14 ++++++++++++--
 neutron/policy.py                                |  5 ++++-
 ...or-security-group-owner-6635dd3e4c6ab5ee.yaml |  6 ++++++
 4 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/show-all-security-group-rules-for-security-group-owner-6635dd3e4c6ab5ee.yaml

diff --git a/neutron/conf/policies/security_group.py b/neutron/conf/policies/security_group.py
index dbc2d7a6823..f1a74b5b5f5 100644
--- a/neutron/conf/policies/security_group.py
+++ b/neutron/conf/policies/security_group.py
@@ -20,8 +20,22 @@ SG_RESOURCE_PATH = '/security-groups/{id}'
 RULE_COLLECTION_PATH = '/security-group-rules'
 RULE_RESOURCE_PATH = '/security-group-rules/{id}'
 
+RULE_ADMIN_OR_SG_OWNER = 'rule:admin_or_sg_owner'
+RULE_ADMIN_OWNER_OR_SG_OWNER = 'rule:admin_owner_or_sg_owner'
+
 
 rules = [
+    policy.RuleDefault(
+        'admin_or_sg_owner',
+        base.policy_or('rule:context_is_admin',
+                       'tenant_id:%(security_group:tenant_id)s'),
+        description='Rule for admin or security group owner access'),
+    policy.RuleDefault(
+        'admin_owner_or_sg_owner',
+        base.policy_or('rule:owner',
+                       RULE_ADMIN_OR_SG_OWNER),
+        description=('Rule for resource owner, '
+                     'admin or security group owner access')),
     # TODO(amotoki): admin_or_owner is the right rule?
     # Does an empty string make more sense for create_security_group?
     policy.DocumentedRuleDefault(
@@ -88,7 +102,7 @@ rules = [
     ),
     policy.DocumentedRuleDefault(
         'get_security_group_rule',
-        base.RULE_ADMIN_OR_OWNER,
+        RULE_ADMIN_OWNER_OR_SG_OWNER,
         'Get a security group rule',
         [
             {
diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py
index b35fda68db5..5ec25afe638 100644
--- a/neutron/db/securitygroups_db.py
+++ b/neutron/db/securitygroups_db.py
@@ -709,8 +709,13 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
         pager = base_obj.Pager(
             sorts=sorts, marker=marker, limit=limit, page_reverse=page_reverse)
 
+        # NOTE(slaweq): use admin context here to be able to get all rules
+        # which fits filters' criteria. Later in policy engine rules will be
+        # filtered and only those which are allowed according to policy will
+        # be returned
         rule_objs = sg_obj.SecurityGroupRule.get_objects(
-            context, _pager=pager, validate_filters=False, **filters
+            context_lib.get_admin_context(), _pager=pager,
+            validate_filters=False, **filters
         )
         return [
             self._make_security_group_rule_dict(obj.db_obj, fields)
@@ -725,7 +730,12 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
 
     @db_api.retry_if_session_inactive()
     def get_security_group_rule(self, context, id, fields=None):
-        security_group_rule = self._get_security_group_rule(context, id)
+        # NOTE(slaweq): use admin context here to be able to get all rules
+        # which fits filters' criteria. Later in policy engine rules will be
+        # filtered and only those which are allowed according to policy will
+        # be returned
+        security_group_rule = self._get_security_group_rule(
+            context_lib.get_admin_context(), id)
         return self._make_security_group_rule_dict(
             security_group_rule.db_obj, fields)
 
diff --git a/neutron/policy.py b/neutron/policy.py
index 4be1eaeaa21..d97a2768e82 100644
--- a/neutron/policy.py
+++ b/neutron/policy.py
@@ -45,7 +45,10 @@ ADVSVC_CTX_POLICY = 'context_is_advsvc'
 
 # Identify the attribute used by a resource to reference another resource
 _RESOURCE_FOREIGN_KEYS = {
-    net_apidef.COLLECTION_NAME: 'network_id'
+    net_apidef.COLLECTION_NAME: 'network_id',
+    # TODO(slaweq): use SECURITYGROUPS constant from api def when
+    # securitygroups api def will be moved to neutron-lib
+    'security_groups': 'security_group_id'
 }
 
 
diff --git a/releasenotes/notes/show-all-security-group-rules-for-security-group-owner-6635dd3e4c6ab5ee.yaml b/releasenotes/notes/show-all-security-group-rules-for-security-group-owner-6635dd3e4c6ab5ee.yaml
new file mode 100644
index 00000000000..647d0e69af1
--- /dev/null
+++ b/releasenotes/notes/show-all-security-group-rules-for-security-group-owner-6635dd3e4c6ab5ee.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Owners of security groups now see all security group rules which belong to
+    the security group, even if the rule was created by the admin user.
+    Fixes bug `1824248 <https://bugs.launchpad.net/neutron/+bug/1824248>`_.