diff options
| -rw-r--r-- | keystone/assignment/backends/sql.py | 5 | ||||
| -rw-r--r-- | keystone/identity/backends/sql.py | 8 | ||||
| -rw-r--r-- | keystone/identity/id_generators/sha256.py | 10 | ||||
| -rw-r--r-- | keystone/tests/unit/assignment/test_backends.py | 19 | ||||
| -rw-r--r-- | keystone/tests/unit/test_backend_id_mapping_sql.py | 17 | ||||
| -rw-r--r-- | keystone/tests/unit/test_backend_sql.py | 37 | ||||
| -rw-r--r-- | releasenotes/notes/bug-1878938-70ee2af6fdf66004.yaml | 16 | ||||
| -rw-r--r-- | releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml | 6 | ||||
| -rw-r--r-- | releasenotes/notes/bug-1901654-69b9f35d11cd0c75.yaml | 10 |
9 files changed, 126 insertions, 2 deletions
diff --git a/keystone/assignment/backends/sql.py b/keystone/assignment/backends/sql.py index 6822811ca..5eda2b724 100644 --- a/keystone/assignment/backends/sql.py +++ b/keystone/assignment/backends/sql.py @@ -262,6 +262,11 @@ class Assignment(base.AssignmentDriverBase): q = q.filter_by(role_id=role_id) q.delete(False) + with sql.session_for_write() as session: + q = session.query(SystemRoleAssignment) + q = q.filter_by(role_id=role_id) + q.delete(False) + def delete_domain_assignments(self, domain_id): with sql.session_for_write() as session: q = session.query(RoleAssignment) diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index eed349630..5a97c7fd5 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -32,6 +32,10 @@ from keystone.identity.backends import sql_model as model CONF = keystone.conf.CONF +def _stale_data_exception_checker(exc): + return isinstance(exc, sqlalchemy.orm.exc.StaleDataError) + + class Identity(base.IdentityDriverBase): # NOTE(henry-nash): Override the __init__() method so as to take a # config parameter to enable sql to be used as a domain-specific driver. @@ -201,6 +205,10 @@ class Identity(base.IdentityDriverBase): return base.filter_user(user_ref.to_dict()) @sql.handle_conflicts(conflict_type='user') + # Explicitly retry on StaleDataErrors, which can happen if two clients + # update the same user's password and the second client has stale password + # information. + @oslo_db_api.wrap_db_retry(exception_checker=_stale_data_exception_checker) def update_user(self, user_id, user): with sql.session_for_write() as session: user_ref = self._get_user(session, user_id) diff --git a/keystone/identity/id_generators/sha256.py b/keystone/identity/id_generators/sha256.py index d0f4a57ad..dde9c2dd0 100644 --- a/keystone/identity/id_generators/sha256.py +++ b/keystone/identity/id_generators/sha256.py @@ -13,7 +13,6 @@ # under the License. import hashlib - from keystone.identity import generator @@ -22,5 +21,12 @@ class Generator(generator.IDGenerator): def generate_public_ID(self, mapping): m = hashlib.sha256() for key in sorted(mapping.keys()): - m.update(mapping[key].encode('utf-8')) + # python-ldap >3.0 returns bytes data type for attribute values + # except distinguished names, relative distinguished names, + # attribute names, queries on python3. + # Please see Bytes/text management in python-ldap module. + if isinstance(mapping[key], bytes): + m.update(mapping[key]) + else: + m.update(mapping[key].encode('utf-8')) return m.hexdigest() diff --git a/keystone/tests/unit/assignment/test_backends.py b/keystone/tests/unit/assignment/test_backends.py index 589256eff..181c42a54 100644 --- a/keystone/tests/unit/assignment/test_backends.py +++ b/keystone/tests/unit/assignment/test_backends.py @@ -4226,3 +4226,22 @@ class SystemAssignmentTests(AssignmentTestHelperMixin): group_id, role['id'] ) + + def test_delete_role_with_system_assignments(self): + role = unit.new_role_ref() + PROVIDERS.role_api.create_role(role['id'], role) + domain = unit.new_domain_ref() + PROVIDERS.resource_api.create_domain(domain['id'], domain) + user = unit.new_user_ref(domain_id=domain['id']) + user = PROVIDERS.identity_api.create_user(user) + + # creating a system grant for user + PROVIDERS.assignment_api.create_system_grant_for_user( + user['id'], role['id'] + ) + # deleting the role user has on system + PROVIDERS.role_api.delete_role(role['id']) + system_roles = PROVIDERS.assignment_api.list_role_assignments( + role_id=role['id'] + ) + self.assertEqual(len(system_roles), 0) diff --git a/keystone/tests/unit/test_backend_id_mapping_sql.py b/keystone/tests/unit/test_backend_id_mapping_sql.py index e5aa878cd..baee34e99 100644 --- a/keystone/tests/unit/test_backend_id_mapping_sql.py +++ b/keystone/tests/unit/test_backend_id_mapping_sql.py @@ -152,6 +152,23 @@ class SqlIDMapping(test_backend_sql.SqlTests): self.assertEqual( public_id, PROVIDERS.id_mapping_api.get_public_id(local_entity)) + def test_id_mapping_handles_bytes(self): + initial_mappings = len(mapping_sql.list_id_mappings()) + local_id = b'FaKeID' + local_entity = {'domain_id': self.domainA['id'], + 'local_id': local_id, + 'entity_type': mapping.EntityType.USER} + + # 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)) + 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_backend_sql.py b/keystone/tests/unit/test_backend_sql.py index 2ec23c31e..ff37ea739 100644 --- a/keystone/tests/unit/test_backend_sql.py +++ b/keystone/tests/unit/test_backend_sql.py @@ -15,9 +15,11 @@ import datetime import uuid +import fixtures import mock from oslo_db import exception as db_exception from oslo_db import options +from oslo_log import log from six.moves import range import sqlalchemy from sqlalchemy import exc @@ -860,6 +862,41 @@ class SqlIdentity(SqlTests, PROVIDERS.resource_api.check_project_depth, 2) + def test_update_user_with_stale_data_forces_retry(self): + # Capture log output so we know oslo.db attempted a retry + log_fixture = self.useFixture(fixtures.FakeLogger(level=log.DEBUG)) + + # Create a new user + user_dict = unit.new_user_ref( + domain_id=CONF.identity.default_domain_id) + new_user_dict = PROVIDERS.identity_api.create_user(user_dict) + + side_effects = [ + # Raise a StaleDataError simulating that another client has + # updated the user's password while this client's request was + # being processed + sqlalchemy.orm.exc.StaleDataError, + # The oslo.db library will retry the request, so the second + # time this method is called let's return a valid session + # object + sql.session_for_write() + ] + with mock.patch('keystone.common.sql.session_for_write') as m: + m.side_effect = side_effects + + # Update a user's attribute, the first attempt will fail but + # oslo.db will handle the exception and retry, the second attempt + # will succeed + new_user_dict['email'] = uuid.uuid4().hex + PROVIDERS.identity_api.update_user( + new_user_dict['id'], new_user_dict) + + # Make sure oslo.db retried the update by checking the log output + expected_log_message = ( + 'Performing DB retry for function keystone.identity.backends.sql' + ) + self.assertIn(expected_log_message, log_fixture.output) + class SqlTrust(SqlTests, trust_tests.TrustTests): diff --git a/releasenotes/notes/bug-1878938-70ee2af6fdf66004.yaml b/releasenotes/notes/bug-1878938-70ee2af6fdf66004.yaml new file mode 100644 index 000000000..21a53b482 --- /dev/null +++ b/releasenotes/notes/bug-1878938-70ee2af6fdf66004.yaml @@ -0,0 +1,16 @@ +--- +fixes: + - | + [`bug 1878938 <https://bugs.launchpad.net/keystone/+bug/1878938>`_] + Previously when a user used to have system role assignment and tries to delete + the same role, the system role assignments still existed in system_assignment + table. This causes keystone to return `HTTP 404 Not Found` errors when listing + role assignments with names (e.g., `--names` or `?include_names`). + + If you are affected by this bug, you must remove stale role assignments + manually. The following is an example SQL statement you can use to fix the + issue, but you should verify it's applicability to your deployment's SQL + implementation and version. + + SQL: + - delete from system_assignment where role_id not in (select id from role); diff --git a/releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml b/releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml new file mode 100644 index 000000000..f8f2d7f9c --- /dev/null +++ b/releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + [`bug 1885753 <https://bugs.launchpad.net/keystone/+bug/1885753>`_] + Keystone's SQL identity backend now retries update user requests to safely + handle stale data when two clients update a user at the same time. diff --git a/releasenotes/notes/bug-1901654-69b9f35d11cd0c75.yaml b/releasenotes/notes/bug-1901654-69b9f35d11cd0c75.yaml new file mode 100644 index 000000000..0537bb837 --- /dev/null +++ b/releasenotes/notes/bug-1901654-69b9f35d11cd0c75.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + [`bug 1901654 <https://bugs.launchpad.net/keystone/+bug/1901654>`_] + Previously, generate_public_ID() in sha256.py assumed the passed arguments is str data type. + However, python-ldap 3.0 or later returns bytes data type for attribute values except fields + of distinguished names, relative distinguished names, attribute names, queries. + If keystone running on Python3 is integrated with LDAP and the LDAP server has local_id variable + in its attribute, user login operations will fail due to the assumption and modifiation of python-ldap. + By this fix, generate_public_ID() properly handles bytes data type in the parameter. |