Use 'modern' query for all sqlalchemy datastores (#1007)

Stop using the model.query and convert to select(xx).where(yy) as recommended in sqlalchemy 2. All 3 sqlalchemy based datastores now share a common implementation. the sqlalchemy_session datastore tests now use the new sqla models. Update docs to note that using FsModels.sqla requires pythin 3.10 or higher. To simplify code - the find_user() method now takes a single kwarg (attribute:value) for selecting - that is all FS ever used so supporting multiple added complexity. Added a new low-level way to capture queries during unit tests - converted all tests to use that.
pallets-eco · Jul 10, 2024 · 394e86c · 394e86c
1 parent 5744f9d
commit 394e86c
Show file tree

Hide file tree

Showing 9 changed files with 186 additions and 311 deletions.
diff --git a/CHANGES.rst b/CHANGES.rst
@@ -14,20 +14,24 @@ Features & Improvements
 - (:issue:`944`) Change default password hash to argon2 (was bcrypt). See below for details.
 - (:pr:`990`) Add freshness capability to auth tokens (enables /us-setup to function w/ just auth tokens).
 - (:pr:`991`) Add support /tf-setup to not require sessions (use a state token).
-- (:issue:`xxx`) Add support for Flask-SQLAlchemy-Lite - including new all-inclusive models
-  that confirm to sqlalchemy latest best-practice (type-annotated).
+- (:issue:`994`) Add support for Flask-SQLAlchemy-Lite - including new all-inclusive models
+  that conform to sqlalchemy latest best-practice (type-annotated).
+- (:pr:`xxx`) Convert other sqlalchemy-based datastores from legacy 'model.query' to best-practice 'select'
 
 Fixes
 +++++
 - (:pr:`972`) Set :py:data:`SECURITY_CSRF_COOKIE` at beginning (GET /login) of authentication
   ritual - just as we return the CSRF token. (thanks @e-goto)
 - (:issue:`973`) login and unified sign in should handle GET for authenticated user consistently.
+- (:pr:`995`) Don't show sms options if not defined in US_ENABLED_METHODS. (fredipevcin)
 
 Docs and Chores
 +++++++++++++++
 - (:pr:`979`) Update Russian translations (ademaro)
-- (:pr:`981` and :pr:`956`) Improve docs
-- (:pr:`xx`) The long deprecated `get_token_status` is no longer exported
+- (:pr:`1004`) Update ES and IT translations (gissimo)
+- (:pr:`981` and :pr:`977`) Improve docs
+- (:pr:`992`) The long deprecated `get_token_status` is no longer exported
+- (:pr:`992`) Drop Python 3.8 support.
 
 Backwards Compatibility Concerns
 +++++++++++++++++++++++++++++++++
@@ -38,6 +42,10 @@ Backwards Compatibility Concerns
 - Changes to /tf-setup
     The old path - using state set in the session still works as before. The new path is
     just for the case an authenticated user wants to change their 2FA setup.
+- Changes to sqlalchemy-based datastores
+    Flask-Security no longer uses the legacy model.query - all DB access is done via
+    `select(xx).where(xx)`. As a result the find_user() method now only takes a SINGLE
+    column:value from its kwargs - in prior releases all kwargs were passed into the query.filter.
 
 Version 5.4.3
 -------------

diff --git a/docs/api.rst b/docs/api.rst
@@ -65,6 +65,9 @@ Datastores
 .. autoclass:: flask_security.SQLAlchemyUserDatastore
     :show-inheritance:
 
+.. autoclass:: flask_security.FSQLALiteUserDatastore
+    :show-inheritance:
+
 .. autoclass:: flask_security.SQLAlchemySessionUserDatastore
     :show-inheritance:
 
@@ -112,6 +115,8 @@ Packaged Models
 ---------------
 .. autoclass:: flask_security.models.fsqla.FsModels
    :members:
+.. autoclass:: flask_security.models.sqla.FsModels
+   :members:
 
 Utils
 -----

diff --git a/docs/models.rst b/docs/models.rst
@@ -52,6 +52,9 @@ Your application code should import just the required version e.g.::
 A single method ``sqla.FsModels.set_db_info`` is provided to glue the supplied mixins to your
 models. This is only needed if you use the packaged models.
 
+.. note::
+    This model requires Python >= 3.10.
+
 Model Specification
 -------------------
 
@@ -61,7 +64,7 @@ foreign relationship between `User` and `Role`. The `WebAuthn` model also
 references this primary key (which can be overridden by providing a
 suitable implementation of :py:meth:`flask_security.WebAuthnMixin.get_user_mapping`).
 
-At the bare minimum your `User` and `Role` model should include the following fields:
+At the bare minimum your `User` and `Role` model must include the following fields:
 
 **User**
 

diff --git a/docs/quickstart.rst b/docs/quickstart.rst
@@ -11,17 +11,21 @@ There are some complete (but simple) examples available in the *examples* direct
     and will need to be added to your requirements.txt (or equivalent) file.
     Flask-Security does some configuration validation and will output error messages to the console
     for some missing packages.
+.. note::
+    The sqlalchemy based quickstarts all use the pre-packaged models - because, well, that's
+    the quickest and easiest way to get started. There is NO REQUIREMENT that your application
+    use these - as long as the required fields are in your models.
 
 .. note::
-    The default :data:`SECURITY_PASSWORD_HASH` is "argon2" - so be sure to install `argon_cffi`_.
+    The default :data:`SECURITY_PASSWORD_HASH` is "argon2" - so be sure to install `argon2_cffi`_.
     If you opt for a different hash e.g. "bcrypt" you will need to install the appropriate package.
 .. danger::
    The examples below place secrets in source files. Never do this for your application
    especially if your source code is placed in a public repo. How you pass in secrets
    securely will depend on your deployment model - however in most cases (e.g. docker, lambda)
    using environment variables will be the easiest.
 
-.. _argon_cffi: https://pypi.org/project/argon2-cffi/
+.. _argon2_cffi: https://pypi.org/project/argon2-cffi/
 
 * :ref:`basic-flask-sqlalchemy-application`
 * :ref:`basic-flask-sqlalchemy-lite-application`
@@ -234,7 +238,7 @@ Basic SQLAlchemy Application with session
 SQLAlchemy Install requirements
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-::
+This requires python >= 3.10::
 
      $ python3 -m venv pymyenv
      $ . pymyenv/bin/activate
@@ -312,13 +316,17 @@ and models.py.
     from sqlalchemy import create_engine
     from sqlalchemy.orm import scoped_session, sessionmaker
     from sqlalchemy.ext.declarative import declarative_base
+    from flask_security.models import sqla
 
     engine = create_engine('sqlite:////tmp/test.db')
     db_session = scoped_session(sessionmaker(autocommit=False,
                                              autoflush=False,
                                              bind=engine))
     Base = declarative_base()
-    Base.query = db_session.query_property()
+    # This creates the RolesUser table and is where
+    # you would pass in non-standard tables names.
+    sqla.FsModels.set_db_info(base_model=Base)
+
 
     def init_db():
         # import all modules here that might define models so that
@@ -330,41 +338,13 @@ and models.py.
 - models.py ::
 
     from database import Base
-    from flask_security import UserMixin, RoleMixin, AsaList
-    from sqlalchemy.orm import relationship, backref
-    from sqlalchemy.ext.mutable import MutableList
-    from sqlalchemy import Boolean, DateTime, Column, Integer, \
-                        String, ForeignKey
-
-    class RolesUsers(Base):
-        __tablename__ = 'roles_users'
-        id = Column(Integer(), primary_key=True)
-        user_id = Column('user_id', Integer(), ForeignKey('user.id'))
-        role_id = Column('role_id', Integer(), ForeignKey('role.id'))
-
-    class Role(Base, RoleMixin):
+    from flask_security.models import sqla as sqla
+
+    class Role(Base, sqla.FsRoleMixin):
         __tablename__ = 'role'
-        id = Column(Integer(), primary_key=True)
-        name = Column(String(80), unique=True)
-        description = Column(String(255))
-        permissions = Column(MutableList.as_mutable(AsaList()), nullable=True)
 
-    class User(Base, UserMixin):
+    class User(Base, sqla.FsUserMixin):
         __tablename__ = 'user'
-        id = Column(Integer, primary_key=True)
-        email = Column(String(255), unique=True)
-        username = Column(String(255), unique=True, nullable=True)
-        password = Column(String(255), nullable=False)
-        last_login_at = Column(DateTime())
-        current_login_at = Column(DateTime())
-        last_login_ip = Column(String(100))
-        current_login_ip = Column(String(100))
-        login_count = Column(Integer)
-        active = Column(Boolean())
-        fs_uniquifier = Column(String(64), unique=True, nullable=False)
-        confirmed_at = Column(DateTime())
-        roles = relationship('Role', secondary='roles_users',
-                             backref=backref('users', lazy='dynamic'))
 
 You can run this either with::
 

diff --git a/flask_security/datastore.py b/flask_security/datastore.py
@@ -21,6 +21,7 @@
 
 if t.TYPE_CHECKING:  # pragma: no cover
     import flask_sqlalchemy
+    import flask_sqlalchemy_lite
     import mongoengine
     import sqlalchemy.orm.scoping
 
@@ -223,10 +224,11 @@ def _prepare_create_user_args(self, **kwargs):
 
         return kwargs
 
-    def find_user(self, **kwargs: t.Any) -> User | None:
+    def find_user(self, case_insensitive: bool = False, **kwargs: t.Any) -> User | None:
         """Returns a user matching the provided parameters.
-        Besides keyword arguments used to filter the results,
-        'case_insensitive' can be passed (defaults to False)
+        A single kwarg will be poped and used to filter results. This should
+        be a unique/primary key in the User model since only a single result is
+        returned.
         """
         raise NotImplementedError
 
@@ -760,35 +762,41 @@ def __init__(
         UserDatastore.__init__(self, user_model, role_model, webauthn_model)
 
     def find_user(self, case_insensitive: bool = False, **kwargs: t.Any) -> User | None:
-        from sqlalchemy import func as alchemyFn
-
-        query = self.user_model.query
-        if cv("JOIN_USER_ROLES") and hasattr(self.user_model, "roles"):
-            from sqlalchemy.orm import joinedload
+        from sqlalchemy import func, select
+        from sqlalchemy.orm import joinedload
 
-            query = query.options(joinedload(self.user_model.roles))  # type: ignore
+        attr, value = kwargs.popitem()  # only a single query attribute accepted
+        val = getattr(self.user_model, attr)
+        stmt = select(self.user_model)
 
+        if cv("JOIN_USER_ROLES") and hasattr(self.user_model, "roles"):
+            stmt = stmt.options(joinedload(self.user_model.roles))  # type: ignore
         if case_insensitive:
-            # While it is of course possible to pass in multiple keys to filter on
-            # that isn't the normal use case. If caller asks for case_insensitive
-            # AND gives multiple keys - throw an error.
-            if len(kwargs) > 1:
-                raise ValueError("Case insensitive option only supports single key")
-            attr, identifier = kwargs.popitem()
-            subquery = alchemyFn.lower(
-                getattr(self.user_model, attr)
-            ) == alchemyFn.lower(identifier)
-            return query.filter(subquery).first()
+            stmt = stmt.where(
+                func.lower(val) == func.lower(value)  # type: ignore[arg-type]
+            )
         else:
-            return query.filter_by(**kwargs).first()
+            stmt = stmt.where(val == value)  # type: ignore[arg-type]
+        return self.db.session.scalar(stmt)
 
     def find_role(self, role: str) -> Role | None:
-        return self.role_model.query.filter_by(name=role).first()  # type: ignore
+        from sqlalchemy import select
+
+        return self.db.session.scalar(
+            select(self.role_model).where(self.role_model.name == role)  # type: ignore
+        )
 
     def find_webauthn(self, credential_id: bytes) -> WebAuthn | None:
-        return self.webauthn_model.query.filter_by(  # type: ignore
-            credential_id=credential_id
-        ).first()
+        from sqlalchemy import select
+
+        if not self.webauthn_model:  # pragma: no cover
+            raise NotImplementedError
+
+        return self.db.session.scalar(
+            select(self.webauthn_model).where(
+                self.webauthn_model.credential_id == credential_id  # type: ignore
+            )
+        )
 
     def create_webauthn(
         self,
@@ -859,11 +867,8 @@ def __init__(self, session):
             webauthn_model,
         )
 
-    def commit(self):
-        super().commit()
-
 
-class FSQLALiteUserDatastore(SQLAlchemyDatastore, UserDatastore):
+class FSQLALiteUserDatastore(SQLAlchemyUserDatastore, UserDatastore):
     """A UserDatastore implementation that assumes the use of
     `Flask-SQLAlchemy-Lite <https://pypi.python.org/pypi/flask-sqlalchemy-lite/>`_
     for datastore transactions.
@@ -879,93 +884,18 @@ class FSQLALiteUserDatastore(SQLAlchemyDatastore, UserDatastore):
 
     def __init__(
         self,
-        db: SQLAlchemy,
+        db: flask_sqlalchemy_lite.SQLAlchemy,
         user_model: t.Type[User],
         role_model: t.Type[Role],
         webauthn_model: t.Type[WebAuthn] | None = None,
     ):
-        SQLAlchemyDatastore.__init__(self, db)
-        UserDatastore.__init__(self, user_model, role_model, webauthn_model)
-
-    def find_user(self, case_insensitive: bool = False, **kwargs: t.Any) -> User | None:
-        from sqlalchemy import func, select
-        from sqlalchemy.orm import joinedload
-
-        attr, value = kwargs.popitem()  # only a single query attribute accepted
-        val = getattr(self.user_model, attr)
-        stmt = select(self.user_model)
-
-        if cv("JOIN_USER_ROLES") and hasattr(self.user_model, "roles"):
-            stmt = stmt.options(joinedload(self.user_model.roles))  # type: ignore
-        if case_insensitive:
-            # While it is of course possible to pass in multiple keys to filter on
-            # that isn't the normal use case. If caller asks for case_insensitive
-            # AND gives multiple keys - throw an error.
-            if len(kwargs) > 0:
-                raise ValueError("Case insensitive option only supports single key")
-            stmt = stmt.where(
-                func.lower(val) == func.lower(value)  # type: ignore[arg-type]
-            )
-        else:
-            stmt = stmt.where(val == value)  # type: ignore[arg-type]
-        return self.db.session.scalar(stmt)
-
-    def find_role(self, role: str) -> Role | None:
-        from sqlalchemy import select
-
-        return self.db.session.scalar(
-            select(self.role_model).where(self.role_model.name == role)  # type: ignore
-        )
-
-    def find_webauthn(self, credential_id: bytes) -> WebAuthn | None:
-        from sqlalchemy import select
-
-        if not self.webauthn_model:  # pragma: no cover
-            raise NotImplementedError
-
-        return self.db.session.scalar(
-            select(self.webauthn_model).where(
-                self.webauthn_model.credential_id == credential_id  # type: ignore
-            )
-        )
-
-    def create_webauthn(
-        self,
-        user: User,
-        credential_id: bytes,
-        public_key: bytes,
-        name: str,
-        sign_count: int,
-        usage: str,
-        device_type: str,
-        backup_state: bool,
-        transports: list[str] | None = None,
-        extensions: str | None = None,
-        **kwargs: t.Any,
-    ) -> None:
-        from .proxies import _security
-
-        if (
-            not hasattr(self, "webauthn_model") or not self.webauthn_model
-        ):  # pragma: no cover
-            raise NotImplementedError
-
-        webauthn = self.webauthn_model(
-            credential_id=credential_id,
-            public_key=public_key,
-            name=name,
-            sign_count=sign_count,
-            usage=usage,
-            device_type=device_type,
-            backup_state=backup_state,
-            transports=transports,
-            extensions=extensions,
-            lastuse_datetime=_security.datetime_factory(),
-            **kwargs,
+        SQLAlchemyUserDatastore.__init__(
+            self,
+            db,  # type: ignore
+            user_model,
+            role_model,
+            webauthn_model,
         )
-        user.webauthn.append(webauthn)
-        self.put(webauthn)
-        self.put(user)
 
 
 class MongoEngineUserDatastore(MongoEngineDatastore, UserDatastore):