Merge "Drop the compatibility password column"

keystone/common/sql/contract_repo/versions/054_contract_drop_old_passoword_column.py

@@ -0,0 +1,21 @@
+#    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
+
+    password_table = sql.Table('password', meta, autoload=True)
+    password_table.c.password.drop()

keystone/common/sql/data_migration_repo/versions/054_migrate_drop_old_passoword_column.py

@@ -0,0 +1,15 @@
+#    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.
+
+
+def upgrade(migrate_engine):
+    pass

keystone/common/sql/expand_repo/versions/054_expand_drop_old_passoword_column.py

@@ -0,0 +1,15 @@
+#    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.
+
+
+def upgrade(migrate_engine):
+    pass

keystone/identity/backends/sql.py

@@ -237,8 +237,7 @@ class Identity(base.IdentityDriverBase):
         if unique_cnt > 0:
             for password_ref in user_ref.local_user.passwords[-unique_cnt:]:
                 if password_hashing.check_password(
-                        password,
-                        password_ref.password_hash or password_ref.password):
+                        password, password_ref.password_hash):
                     raise exception.PasswordHistoryValidationError(
                         unique_count=unique_cnt)
 

keystone/identity/backends/sql_model.py

@@ -105,10 +105,7 @@ class User(sql.ModelBase, sql.ModelDictMixinWithExtras):
     def password(self):
         """Return the current password."""
         if self.password_ref:
-            if self.password_ref.password_hash is not None:
-                return self.password_ref.password_hash
-            else:
-                return self.password_ref.password
+            return self.password_ref.password_hash
         return None
 
     @property
@@ -149,19 +146,17 @@ class User(sql.ModelBase, sql.ModelDictMixinWithExtras):
         new_password_ref = Password()
 
         hashed_passwd = None
-        hashed_compat = None
         if value is not None:
             # NOTE(notmorgan): hash the passwords, never directly bind the
-            # "value" in the unhashed form to hashed_passwd or hashed_compat
-            # to ensure the unhashed password cannot end up in the db. If an
-            # unhashed password ends up in the DB, it cannot be used for auth,
-            # it is however incorrect and could leak user credentials (due to
-            # users doing insecure things such as sharing passwords across
+            # "value" in the unhashed form to hashed_passwd to ensure the
+            # unhashed password cannot end up in the db. If an unhashed
+            # password ends up in the DB, it cannot be used for auth, it is
+            # however incorrect and could leak user credentials (due to users
+            # doing insecure things such as sharing passwords across
             # different systems) to unauthorized parties.
             hashed_passwd = password_hashing.hash_password(value)
 
         new_password_ref.password_hash = hashed_passwd
-        new_password_ref.password = hashed_compat
         new_password_ref.created_at = now
         new_password_ref.expires_at = self._get_password_expires_at(now)
         self.local_user.passwords.append(new_password_ref)
@@ -185,7 +180,7 @@ class User(sql.ModelBase, sql.ModelDictMixinWithExtras):
 
     @password.expression
     def password(cls):
-        return Password.password
+        return Password.password_hash
 
     # NOTE(stevemar): we use a hybrid property here because we leverage the
     # expression method, see `@enabled.expression` and `User._enabled` below.
@@ -289,15 +284,11 @@ class LocalUser(sql.ModelBase, sql.ModelDictMixin):
 
 class Password(sql.ModelBase, sql.ModelDictMixin):
     __tablename__ = 'password'
-    attributes = ['id', 'local_user_id', 'password', 'password_hash',
-                  'created_at', 'expires_at']
+    attributes = ['id', 'local_user_id', 'password_hash', 'created_at',
+                  'expires_at']
     id = sql.Column(sql.Integer, primary_key=True)
     local_user_id = sql.Column(sql.Integer, sql.ForeignKey('local_user.id',
                                ondelete='CASCADE'))
-    # TODO(notmorgan): in the Q release the "password" field can be dropped as
-    # long as data migration exists to move the hashes over to the
-    # password_hash column if no value is in the password_hash column.
-    password = sql.Column(sql.String(128), nullable=True)
     password_hash = sql.Column(sql.String(255), nullable=True)
 
     # TODO(lbragstad): Once Rocky opens for development, the _created_at and

keystone/tests/unit/identity/test_backend_sql.py

@@ -61,18 +61,6 @@ class UserPasswordHashingTestsNoCompat(test_backend_sql.SqlTests):
         self.config_fixture.config(group='identity',
                                    password_hash_algorithm='scrypt')
 
-    def test_password_hashing_compat_not_set_used(self):
-        with sql.session_for_read() as session:
-            user_ref = PROVIDERS.identity_api._get_user(
-                session, self.user_foo['id']
-            )
-        self.assertIsNone(user_ref.password_ref.password)
-        self.assertIsNotNone(user_ref.password_ref.password_hash)
-        self.assertEqual(user_ref.password,
-                         user_ref.password_ref.password_hash)
-        self.assertTrue(password_hashing.check_password(
-            self.user_foo['password'], user_ref.password))
-
     def test_configured_algorithm_used(self):
         with sql.session_for_read() as session:
             user_ref = PROVIDERS.identity_api._get_user(

keystone/tests/unit/test_backend_sql.py

@@ -152,7 +152,6 @@ class SqlModels(SqlTests):
     def test_password_model(self):
         cols = (('id', sql.Integer, None),
                 ('local_user_id', sql.Integer, None),
-                ('password', sql.String, 128),
                 ('password_hash', sql.String, 255),
                 ('created_at', sql.DateTime, None),
                 ('expires_at', sql.DateTime, None),

keystone/tests/unit/test_sql_upgrade.py

@@ -3179,6 +3179,29 @@ class FullMigration(SqlMigrateBase, unit.TestCase):
         }
         role_table.insert().values(role_without_description).execute()
 
+    def test_migration_054_drop_old_password_column(self):
+        self.expand(53)
+        self.migrate(53)
+        self.contract(53)
+
+        password_table = 'password'
+        self.assertTableColumns(
+            password_table,
+            ['id', 'local_user_id', 'password', 'password_hash',
+             'self_service', 'created_at_int', 'created_at', 'expires_at_int',
+             'expires_at']
+        )
+
+        self.expand(54)
+        self.migrate(54)
+        self.contract(54)
+
+        self.assertTableColumns(
+            password_table,
+            ['id', 'local_user_id', 'password_hash', 'self_service',
+             'created_at_int', 'created_at', 'expires_at_int', 'expires_at']
+        )
+
 
 class MySQLOpportunisticFullMigration(FullMigration):
     FIXTURE = db_fixtures.MySQLOpportunisticFixture