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)