From 5a0811f355671c94e985aa888be5472a7920afa4 Mon Sep 17 00:00:00 2001 From: Samuel de Medeiros Queiroz Date: Wed, 17 Dec 2014 12:52:40 -0300 Subject: [PATCH] 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 | 3 +- .../073_insert_assignment_inherited_pk.py | 114 ++++++++++++++++++ keystone/tests/unit/test_backend.py | 4 - keystone/tests/unit/test_sql_upgrade.py | 74 ++++++++++++ keystone/tests/unit/test_v3_assignment.py | 3 - 5 files changed, 190 insertions(+), 8 deletions(-) create mode 100644 keystone/common/sql/migrate_repo/versions/073_insert_assignment_inherited_pk.py diff --git a/keystone/assignment/backends/sql.py b/keystone/assignment/backends/sql.py index 766b0a123a..ae434de9de 100644 --- a/keystone/assignment/backends/sql.py +++ b/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'), ) diff --git a/keystone/common/sql/migrate_repo/versions/073_insert_assignment_inherited_pk.py b/keystone/common/sql/migrate_repo/versions/073_insert_assignment_inherited_pk.py new file mode 100644 index 0000000000..ffa210c4de --- /dev/null +++ b/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() diff --git a/keystone/tests/unit/test_backend.py b/keystone/tests/unit/test_backend.py index 903afc68e8..e7a15a3758 100644 --- a/keystone/tests/unit/test_backend.py +++ b/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) diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py index 35fd95388b..6a2338e4a2 100644 --- a/keystone/tests/unit/test_sql_upgrade.py +++ b/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): diff --git a/keystone/tests/unit/test_v3_assignment.py b/keystone/tests/unit/test_v3_assignment.py index 555486bc8a..ebf4850f04 100644 --- a/keystone/tests/unit/test_v3_assignment.py +++ b/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)