diff --git a/doc/source/admin/domain-specific-config.inc b/doc/source/admin/domain-specific-config.inc index 3797e30782..2d8f9936af 100644 --- a/doc/source/admin/domain-specific-config.inc +++ b/doc/source/admin/domain-specific-config.inc @@ -146,6 +146,12 @@ then the same public ID will be created. This is useful if you are running multiple keystones and want to ensure the same ID would be generated whichever server you hit. +.. NOTE:: + + In case of the LDAP backend, the names of users and groups are not hashed. + As a result, these are length limited to 255 characters. Longer names + will result in an error. + While keystone will dynamically maintain the identity mapping, including removing entries when entities are deleted via the keystone, for those entities in backends that are managed outside of keystone (e.g. a read-only LDAP), diff --git a/keystone/common/sql/contract_repo/versions/079_contract_update_local_id_limit.py b/keystone/common/sql/contract_repo/versions/079_contract_update_local_id_limit.py new file mode 100644 index 0000000000..2b09cbc99a --- /dev/null +++ b/keystone/common/sql/contract_repo/versions/079_contract_update_local_id_limit.py @@ -0,0 +1,18 @@ +# 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. + +# This is a placeholder for Ussuri backports. Do not use this number for new +# Victoria work. New Victoria work starts after all the placeholders. + + +def upgrade(migrate_engine): + pass diff --git a/keystone/common/sql/data_migration_repo/versions/079_migrate_update_local_id_limit.py b/keystone/common/sql/data_migration_repo/versions/079_migrate_update_local_id_limit.py new file mode 100644 index 0000000000..2b09cbc99a --- /dev/null +++ b/keystone/common/sql/data_migration_repo/versions/079_migrate_update_local_id_limit.py @@ -0,0 +1,18 @@ +# 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. + +# This is a placeholder for Ussuri backports. Do not use this number for new +# Victoria work. New Victoria work starts after all the placeholders. + + +def upgrade(migrate_engine): + pass diff --git a/keystone/common/sql/expand_repo/versions/079_expand_update_local_id_limit.py b/keystone/common/sql/expand_repo/versions/079_expand_update_local_id_limit.py new file mode 100644 index 0000000000..20db838515 --- /dev/null +++ b/keystone/common/sql/expand_repo/versions/079_expand_update_local_id_limit.py @@ -0,0 +1,24 @@ +# 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 sqlalchemy as sql + + +def upgrade(migrate_engine): + + meta = sql.MetaData() + meta.bind = migrate_engine + + id_mapping_table = sql.Table( + 'id_mapping', meta, autoload=True + ) + id_mapping_table.c.local_id.alter(type=sql.String(255)) diff --git a/keystone/identity/mapping_backends/sql.py b/keystone/identity/mapping_backends/sql.py index 676d144922..6fadd6a0b0 100644 --- a/keystone/identity/mapping_backends/sql.py +++ b/keystone/identity/mapping_backends/sql.py @@ -21,7 +21,7 @@ class IDMapping(sql.ModelBase, sql.ModelDictMixin): __tablename__ = 'id_mapping' public_id = sql.Column(sql.String(64), primary_key=True) domain_id = sql.Column(sql.String(64), nullable=False) - local_id = sql.Column(sql.String(64), nullable=False) + local_id = sql.Column(sql.String(255), nullable=False) # NOTE(henry-nash): Postgres requires a name to be defined for an Enum entity_type = sql.Column( sql.Enum(identity_mapping.EntityType.USER, diff --git a/keystone/tests/unit/test_backend_id_mapping_sql.py b/keystone/tests/unit/test_backend_id_mapping_sql.py index baee34e99f..7729d53c63 100644 --- a/keystone/tests/unit/test_backend_id_mapping_sql.py +++ b/keystone/tests/unit/test_backend_id_mapping_sql.py @@ -33,7 +33,7 @@ class SqlIDMappingTable(test_backend_sql.SqlModels): def test_id_mapping(self): cols = (('public_id', sql.String, 64), ('domain_id', sql.String, 64), - ('local_id', sql.String, 64), + ('local_id', sql.String, 255), ('entity_type', sql.Enum, None)) self.assertExpectedSchema('id_mapping', cols) @@ -169,6 +169,26 @@ class SqlIDMapping(test_backend_sql.SqlTests): self.assertEqual( public_id, PROVIDERS.id_mapping_api.get_public_id(local_entity)) + def test_id_mapping_handles_ids_greater_than_64_characters(self): + initial_mappings = len(mapping_sql.list_id_mappings()) + local_id = 'Aa' * 100 + local_entity = {'domain_id': self.domainA['id'], + 'local_id': local_id, + 'entity_type': mapping.EntityType.GROUP} + + # Check no mappings for the new local entity + self.assertIsNone(PROVIDERS.id_mapping_api.get_public_id(local_entity)) + + # Create the new mapping and then read it back + public_id = PROVIDERS.id_mapping_api.create_id_mapping(local_entity) + self.assertThat(mapping_sql.list_id_mappings(), + matchers.HasLength(initial_mappings + 1)) + self.assertEqual( + public_id, PROVIDERS.id_mapping_api.get_public_id(local_entity)) + self.assertEqual( + local_id, + PROVIDERS.id_mapping_api.get_id_mapping(public_id)['local_id']) + def test_delete_public_id_is_silent(self): # Test that deleting an invalid public key is silent PROVIDERS.id_mapping_api.delete_id_mapping(uuid.uuid4().hex) diff --git a/keystone/tests/unit/test_sql_banned_operations.py b/keystone/tests/unit/test_sql_banned_operations.py index 9916657dad..2207140753 100644 --- a/keystone/tests/unit/test_sql_banned_operations.py +++ b/keystone/tests/unit/test_sql_banned_operations.py @@ -263,7 +263,12 @@ class TestKeystoneExpandSchemaMigrations( # timestamp to datetime and updates the initial value in the contract # phase. Adding an exception here to pass expand banned tests, # otherwise fails. - 4 + 4, + + # Migration 79 changes a varchar column length, doesn't + # convert the data within that column/table and doesn't rebuild + # indexes. + 79 ] def setUp(self): diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py index 50c28707a4..502032f66b 100644 --- a/keystone/tests/unit/test_sql_upgrade.py +++ b/keystone/tests/unit/test_sql_upgrade.py @@ -3499,6 +3499,25 @@ class FullMigration(SqlMigrateBase, unit.TestCase): ['id', 'domain_id', 'enabled', 'description', 'authorization_ttl']) + def test_migration_079_expand_update_local_id_limit(self): + self.expand(78) + self.migrate(78) + self.contract(78) + + id_mapping_table = sqlalchemy.Table('id_mapping', + self.metadata, autoload=True) + # assert local_id column is a string of 64 characters (before) + self.assertEqual('VARCHAR(64)', str(id_mapping_table.c.local_id.type)) + + self.expand(79) + self.migrate(79) + self.contract(79) + + id_mapping_table = sqlalchemy.Table('id_mapping', + self.metadata, autoload=True) + # assert local_id column is a string of 255 characters (after) + self.assertEqual('VARCHAR(255)', str(id_mapping_table.c.local_id.type)) + class MySQLOpportunisticFullMigration(FullMigration): FIXTURE = db_fixtures.MySQLOpportunisticFixture diff --git a/releasenotes/notes/bug-1929066-6e741c9182620a37.yaml b/releasenotes/notes/bug-1929066-6e741c9182620a37.yaml new file mode 100644 index 0000000000..0acd1abc96 --- /dev/null +++ b/releasenotes/notes/bug-1929066-6e741c9182620a37.yaml @@ -0,0 +1,7 @@ +--- +upgrade: + - | + [`bug 1929066 `_] + Increase the length of the `local_id` column in the `id_mapping` table + to accommodate LDAP group names that result in names greater than + 64 characters.