From 2700adaadcd19baf4ee6edf9b41ff9e6e4009edc Mon Sep 17 00:00:00 2001 From: Grzegorz Grasza Date: Thu, 20 May 2021 21:07:02 +0200 Subject: Update local_id limit to 255 characters This avoids the "String length exceeded." error, when using LDAP domain specific backend in case the user uses a user id attribute, which can exceed the previous constraint of 64 chars. Change-Id: I923a2a2a5e79c8f265ff436e96258288dddb867b Closes-Bug: #1929066 Resolves: rhbz#1959345 (cherry picked from commit ce6031ca12156620cec214a49d162ec7bb30752f) --- doc/source/admin/domain-specific-config.inc | 6 ++++++ .../versions/079_contract_update_local_id_limit.py | 18 ++++++++++++++++ .../versions/079_migrate_update_local_id_limit.py | 18 ++++++++++++++++ .../versions/079_expand_update_local_id_limit.py | 24 ++++++++++++++++++++++ keystone/identity/mapping_backends/sql.py | 2 +- keystone/tests/unit/test_backend_id_mapping_sql.py | 22 +++++++++++++++++++- keystone/tests/unit/test_sql_banned_operations.py | 7 ++++++- keystone/tests/unit/test_sql_upgrade.py | 19 +++++++++++++++++ .../notes/bug-1929066-6e741c9182620a37.yaml | 7 +++++++ 9 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 keystone/common/sql/contract_repo/versions/079_contract_update_local_id_limit.py create mode 100644 keystone/common/sql/data_migration_repo/versions/079_migrate_update_local_id_limit.py create mode 100644 keystone/common/sql/expand_repo/versions/079_expand_update_local_id_limit.py create mode 100644 releasenotes/notes/bug-1929066-6e741c9182620a37.yaml diff --git a/doc/source/admin/domain-specific-config.inc b/doc/source/admin/domain-specific-config.inc index 3797e3078..2d8f9936a 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 000000000..2b09cbc99 --- /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 000000000..2b09cbc99 --- /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 000000000..20db83851 --- /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 676d14492..6fadd6a0b 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 baee34e99..7729d53c6 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 9916657da..220714075 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 50c28707a..502032f66 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 000000000..0acd1abc9 --- /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. -- cgit v1.2.1