Adds inherited column to RoleAssignment PK

It should be possible to add both inherited and
non-inherited role assignments for the same actor
and target with the same role.

However, this was not currently possible since
the inherited column was not part of the PK of
the assignment table.

This patch alters the table definition and adds a
migration script and tests for it.

Closes-Bug: #1403539

Change-Id: I1ba4935934b0dc6b6077d18761023ad50462c8b8

keystone/assignment/backends/sql.py

@@ -404,7 +404,8 @@ class RoleAssignment(sql.ModelBase, sql.DictBase):
     role_id = sql.Column(sql.String(64), nullable=False)
     inherited = sql.Column(sql.Boolean, default=False, nullable=False)
     __table_args__ = (
-        sql.PrimaryKeyConstraint('type', 'actor_id', 'target_id', 'role_id'),
+        sql.PrimaryKeyConstraint('type', 'actor_id', 'target_id', 'role_id',
+                                 'inherited'),
         sql.Index('ix_actor_id', 'actor_id'),
     )
 

keystone/common/sql/migrate_repo/versions/073_insert_assignment_inherited_pk.py

@@ -0,0 +1,114 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import migrate
+import sqlalchemy as sql
+from sqlalchemy.orm import sessionmaker
+
+from keystone.assignment.backends import sql as assignment_sql
+
+
+def upgrade(migrate_engine):
+    """Inserts inherited column to assignment table PK contraints.
+
+    For non-SQLite databases, it changes the constraint in the existing table.
+
+    For SQLite, since changing constraints is not supported, it recreates the
+    assignment table with the new PK constraint and migrates the existing data.
+
+    """
+
+    ASSIGNMENT_TABLE_NAME = 'assignment'
+
+    metadata = sql.MetaData()
+    metadata.bind = migrate_engine
+
+    # Retrieve the existing assignment table
+    assignment_table = sql.Table(ASSIGNMENT_TABLE_NAME, metadata,
+                                 autoload=True)
+
+    if migrate_engine.name == 'sqlite':
+        ACTOR_ID_INDEX_NAME = 'ix_actor_id'
+        TMP_ASSIGNMENT_TABLE_NAME = 'tmp_assignment'
+
+        # Define the new assignment table with a temporary name
+        new_assignment_table = sql.Table(
+            TMP_ASSIGNMENT_TABLE_NAME, metadata,
+            sql.Column('type', sql.Enum(
+                assignment_sql.AssignmentType.USER_PROJECT,
+                assignment_sql.AssignmentType.GROUP_PROJECT,
+                assignment_sql.AssignmentType.USER_DOMAIN,
+                assignment_sql.AssignmentType.GROUP_DOMAIN,
+                name='type'),
+                nullable=False),
+            sql.Column('actor_id', sql.String(64), nullable=False),
+            sql.Column('target_id', sql.String(64), nullable=False),
+            sql.Column('role_id', sql.String(64), sql.ForeignKey('role.id'),
+                       nullable=False),
+            sql.Column('inherited', sql.Boolean, default=False,
+                       nullable=False),
+            sql.PrimaryKeyConstraint('type', 'actor_id', 'target_id',
+                                     'role_id', 'inherited'),
+            mysql_engine='InnoDB',
+            mysql_charset='utf8')
+
+        # Create the new assignment table
+        new_assignment_table.create(migrate_engine, checkfirst=True)
+
+        # Change the index from the existing assignment table to the new one
+        sql.Index(ACTOR_ID_INDEX_NAME, assignment_table.c.actor_id).drop()
+        sql.Index(ACTOR_ID_INDEX_NAME,
+                  new_assignment_table.c.actor_id).create()
+
+        # Instantiate session
+        maker = sessionmaker(bind=migrate_engine)
+        session = maker()
+
+        # Migrate existing data
+        insert = new_assignment_table.insert().from_select(
+            assignment_table.c, select=session.query(assignment_table))
+        session.execute(insert)
+        session.commit()
+
+        # Drop the existing assignment table, in favor of the new one
+        assignment_table.deregister()
+        assignment_table.drop()
+
+        # Finally, rename the new table to the original assignment table name
+        new_assignment_table.rename(ASSIGNMENT_TABLE_NAME)
+    elif migrate_engine.name == 'ibm_db_sa':
+        # Recreate the existing constraint, marking the inherited column as PK
+        # for DB2.
+
+        # This is a workaround to the general case in the else statement below.
+        # Due to a bug in the DB2 sqlalchemy dialect, Column.alter() actually
+        # creates a primary key over only the "inherited" column. This is wrong
+        # because the primary key for the table actually covers other columns
+        # too, not just the "inherited" column. Since the primary key already
+        # exists for the table after the Column.alter() call, it causes the
+        # next line to fail with an error that the primary key already exists.
+
+        # The workaround here skips doing the Column.alter(). This causes a
+        # warning message since the metadata is out of sync. We can remove this
+        # workaround once the DB2 sqlalchemy dialect is fixed.
+        # DB2 Issue: https://code.google.com/p/ibm-db/issues/detail?id=173
+
+        migrate.PrimaryKeyConstraint(table=assignment_table).drop()
+        migrate.PrimaryKeyConstraint(
+            assignment_table.c.type, assignment_table.c.actor_id,
+            assignment_table.c.target_id, assignment_table.c.role_id,
+            assignment_table.c.inherited).create()
+    else:
+        # Recreate the existing constraint, marking the inherited column as PK
+        migrate.PrimaryKeyConstraint(table=assignment_table).drop()
+        assignment_table.c.inherited.alter(primary_key=True)
+        migrate.PrimaryKeyConstraint(table=assignment_table).create()

keystone/tests/unit/test_backend.py

@@ -5212,12 +5212,10 @@ class InheritanceTests(object):
         grants = self.assignment_api.list_role_assignments_for_role(role['id'])
         self.assertEqual([], grants)
 
-    @test_utils.wip('Waiting on bug #1403539')
     def test_crud_inherited_and_direct_assignment_for_user_on_domain(self):
         self._test_crud_inherited_and_direct_assignment(
             user_id=self.user_foo['id'], domain_id=DEFAULT_DOMAIN_ID)
 
-    @test_utils.wip('Waiting on bug #1403539')
     def test_crud_inherited_and_direct_assignment_for_group_on_domain(self):
         group = {'name': uuid.uuid4().hex, 'domain_id': DEFAULT_DOMAIN_ID}
         group = self.identity_api.create_group(group)
@@ -5225,12 +5223,10 @@ class InheritanceTests(object):
         self._test_crud_inherited_and_direct_assignment(
             group_id=group['id'], domain_id=DEFAULT_DOMAIN_ID)
 
-    @test_utils.wip('Waiting on bug #1403539')
     def test_crud_inherited_and_direct_assignment_for_user_on_project(self):
         self._test_crud_inherited_and_direct_assignment(
             user_id=self.user_foo['id'], project_id=self.tenant_baz['id'])
 
-    @test_utils.wip('Waiting on bug #1403539')
     def test_crud_inherited_and_direct_assignment_for_group_on_project(self):
         group = {'name': uuid.uuid4().hex, 'domain_id': DEFAULT_DOMAIN_ID}
         group = self.identity_api.create_group(group)

keystone/tests/unit/test_sql_upgrade.py

@@ -430,6 +430,80 @@ class SqlUpgradeTests(SqlMigrateBase):
             # sqlite does not support FK deletions (or enforcement)
             self.assertFalse(self.does_fk_exist('assignment', 'role_id'))
 
+    def test_insert_assignment_inherited_pk(self):
+        ASSIGNMENT_TABLE_NAME = 'assignment'
+        INHERITED_COLUMN_NAME = 'inherited'
+        ROLE_TABLE_NAME = 'role'
+
+        self.upgrade(72)
+
+        # Check that the 'inherited' column is not part of the PK
+        self.assertFalse(self.does_pk_exist(ASSIGNMENT_TABLE_NAME,
+                                            INHERITED_COLUMN_NAME))
+
+        session = self.Session()
+
+        role = {'id': uuid.uuid4().hex,
+                'name': uuid.uuid4().hex}
+        self.insert_dict(session, ROLE_TABLE_NAME, role)
+
+        # Create both inherited and noninherited role assignments
+        inherited = {'type': 'UserProject',
+                     'actor_id': uuid.uuid4().hex,
+                     'target_id': uuid.uuid4().hex,
+                     'role_id': role['id'],
+                     'inherited': True}
+
+        noninherited = inherited.copy()
+        noninherited['inherited'] = False
+
+        # Create another inherited role assignment as a spoiler
+        spoiler = inherited.copy()
+        spoiler['actor_id'] = uuid.uuid4().hex
+
+        self.insert_dict(session, ASSIGNMENT_TABLE_NAME, inherited)
+        self.insert_dict(session, ASSIGNMENT_TABLE_NAME, spoiler)
+
+        # Since 'inherited' is not part of the PK, we can't insert noninherited
+        self.assertRaises(db_exception.DBDuplicateEntry,
+                          self.insert_dict,
+                          session,
+                          ASSIGNMENT_TABLE_NAME,
+                          noninherited)
+
+        session.close()
+
+        self.upgrade(73)
+
+        session = self.Session()
+        self.metadata.clear()
+
+        # Check that the 'inherited' column is now part of the PK
+        self.assertTrue(self.does_pk_exist(ASSIGNMENT_TABLE_NAME,
+                                           INHERITED_COLUMN_NAME))
+
+        # The noninherited role assignment can now be inserted
+        self.insert_dict(session, ASSIGNMENT_TABLE_NAME, noninherited)
+
+        assignment_table = sqlalchemy.Table(ASSIGNMENT_TABLE_NAME,
+                                            self.metadata,
+                                            autoload=True)
+
+        assignments = session.query(assignment_table).all()
+        for assignment in (inherited, spoiler, noninherited):
+            self.assertIn((assignment['type'], assignment['actor_id'],
+                           assignment['target_id'], assignment['role_id'],
+                           assignment['inherited']),
+                          assignments)
+
+    def does_pk_exist(self, table, pk_column):
+        """Checks whether a column is primary key on a table."""
+
+        inspector = reflection.Inspector.from_engine(self.engine)
+        pk_columns = inspector.get_pk_constraint(table)['constrained_columns']
+
+        return pk_column in pk_columns
+
     def does_fk_exist(self, table, fk_column):
         inspector = reflection.Inspector.from_engine(self.engine)
         for fk in inspector.get_foreign_keys(table):

keystone/tests/unit/test_v3_assignment.py

@@ -20,7 +20,6 @@ from keystone.common import controller
 from keystone import exception
 from keystone.tests import unit as tests
 from keystone.tests.unit import test_v3
-from keystone.tests.unit import utils as test_utils
 
 
 CONF = cfg.CONF
@@ -2297,12 +2296,10 @@ class AssignmentInheritanceTestCase(test_v3.RestfulTestCase):
         self.head(direct_url, expected_status=404)
         self.head(inherited_url, expected_status=404)
 
-    @test_utils.wip('Waiting on bug #1403539')
     def test_crud_inherited_and_direct_assignment_on_domains(self):
         self._test_crud_inherited_and_direct_assignment_on_target(
             '/domains/%s' % self.domain_id)
 
-    @test_utils.wip('Waiting on bug #1403539')
     def test_crud_inherited_and_direct_assignment_on_projects(self):
         self._test_crud_inherited_and_direct_assignment_on_target(
             '/projects/%s' % self.project_id)