diff --git a/README.rst b/README.rst index 4a2f5ee6..6a82ece8 100644 --- a/README.rst +++ b/README.rst @@ -175,13 +175,19 @@ Technology choices: DB Models --------- -.. image:: https://user-images.githubusercontent.com/786326/32836181-43de9248-ca09-11e7-99c5-7b877272aef4.png - :alt: screen shot 2017-11-15 at 1 30 31 pm +.. image:: https://user-images.githubusercontent.com/786326/33126887-7b6c746e-cf86-11e7-9176-1d500739adf7.png + :alt: screen shot 2017-11-22 at 12 24 37 pm Changelog ========= +v. 0.6.1 +-------- + +* Under-the-hood traceability updates (`#78 `_) + + v. 0.6.0 -------- diff --git a/migrations/versions/b984311d26d7_add_modified_by_created_by_fields_for_.py b/migrations/versions/b984311d26d7_add_modified_by_created_by_fields_for_.py new file mode 100644 index 00000000..b4575910 --- /dev/null +++ b/migrations/versions/b984311d26d7_add_modified_by_created_by_fields_for_.py @@ -0,0 +1,489 @@ +"""Add modified_by/created_by fields for traceability + make sure libris@kb.se admin user exists. + +Revision ID: b984311d26d7 +Revises: 072487c42d98 +Create Date: 2017-11-21 15:50:10.745763 + +""" + +from __future__ import absolute_import, division, print_function, unicode_literals + +from binascii import hexlify +from codecs import getencoder +from datetime import datetime +from os import urandom + +import sqlalchemy as sa +from alembic import op +from flask_bcrypt import Bcrypt +from flask_login import UserMixin +from six import string_types +from sqlalchemy import (Binary, Boolean, Column, DateTime, ForeignKey, Integer, String, Text, + UniqueConstraint) +from sqlalchemy.ext.declarative import declarative_base +from sqlalchemy.ext.hybrid import hybrid_property +from sqlalchemy.orm import relationship, sessionmaker + +# Revision identifiers, used by Alembic. +revision = 'b984311d26d7' +down_revision = '072487c42d98' +branch_labels = None +depends_on = None + + +try: + # noinspection PyCompatibility,PyUnresolvedReferences,PyUnboundLocalVariable + basestring # py2 +except NameError: + # noinspection PyShadowingBuiltins + basestring = str # py3 + +Session = sessionmaker() + +Base = declarative_base() + + +def upgrade(): + """Add 'modified_by'/'created_by' cols and backfill clients/collections/users/permissions.""" + bind = op.get_bind() + session = Session(bind=bind) + + class CRUDMixin(object): + """Mixin that adds convenience methods for CRUD (create, read, update, delete) ops.""" + + @classmethod + def create_as(cls, current_user, **kwargs): + """Create a new record and save it to the database as 'current_user'.""" + assert hasattr(cls, 'modified_by') and hasattr(cls, 'created_by') + instance = cls(**kwargs) + return instance.save_as(current_user) + + @classmethod + def create(cls, **kwargs): + """Create a new record and save it to the database.""" + instance = cls(**kwargs) + return instance.save() + + def update_as(self, current_user, commit=True, preserve_modified=False, **kwargs): + """Update specific fields of the record and save as 'current_user'.""" + for attr, value in kwargs.items(): + setattr(self, attr, value) + return self.save_as(current_user, commit=commit, preserve_modified=preserve_modified) + + def update(self, commit=True, preserve_modified=False, **kwargs): + """Update specific fields of a record.""" + for attr, value in kwargs.items(): + setattr(self, attr, value) + return self.save(commit=commit, preserve_modified=preserve_modified) + + def save_as(self, current_user, commit=True, preserve_modified=False): + """Save instance as 'current_user'.""" + assert hasattr(self, 'modified_by') and hasattr(self, 'created_by') + # noinspection PyUnresolvedReferences + if current_user and not self.created_at: + # noinspection PyAttributeOutsideInit + self.created_by = current_user + if current_user and not preserve_modified: + # noinspection PyAttributeOutsideInit + self.modified_by = current_user + return self.save(commit=commit, preserve_modified=preserve_modified) + + def save(self, commit=True, preserve_modified=False): + """Save the record.""" + session.add(self) + if commit: + if preserve_modified and hasattr(self, 'modified_at'): + modified_dt = self.modified_at + session.commit() + # noinspection PyAttributeOutsideInit + self.modified_at = modified_dt + session.commit() + return self + + def delete(self, commit=True): + """Remove the record from the database.""" + session.delete(self) + return commit and session.commit() + + class Model(CRUDMixin, Base): + """Base model class that includes CRUD convenience methods.""" + + __abstract__ = True + + @staticmethod + def _get_rand_hex_str(length=32): + """Create random hex string.""" + return getencoder('hex')(urandom(length // 2))[0].decode('utf-8') + + class SurrogatePK(object): + """A mixin that adds a surrogate integer primary key column to declarative-mapped class.""" + + __table_args__ = {'extend_existing': True} + + id = Column(Integer, primary_key=True) + + @classmethod + def get_by_id(cls, record_id): + """Get record by ID.""" + if any((isinstance(record_id, basestring) and record_id.isdigit(), + isinstance(record_id, (int, float))), ): + # noinspection PyUnresolvedReferences + return session.query(cls).get(int(record_id)) + else: + return None + + def reference_col(tablename, nullable=False, pk_name='id', ondelete=None, **kwargs): + """Column that adds primary key foreign key reference. + + Usage :: + + category_id = reference_col('category') + category = relationship('Category', backref='categories') + + """ + return Column(ForeignKey('{0}.{1}'.format(tablename, pk_name), ondelete=ondelete), + nullable=nullable, **kwargs) + + class User(UserMixin, SurrogatePK, Model): + """A user of the app.""" + + __tablename__ = 'users' + id = Column(Integer, primary_key=True) + email = Column(String(255), unique=True, nullable=False) + full_name = Column(String(255), unique=False, nullable=False) + password = Column(Binary(128), nullable=False) + last_login_at = Column(DateTime, default=None) + is_active = Column(Boolean(), default=False, nullable=False) + is_admin = Column(Boolean(), default=False, nullable=False) + permissions = relationship('Permission', back_populates='user', + foreign_keys='Permission.user_id') + + modified_at = Column(DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=False) + modified_by_id = reference_col('users', nullable=True) + modified_by = relationship('User', remote_side=id, foreign_keys=modified_by_id) + + created_at = Column(DateTime, default=datetime.utcnow, nullable=False) + created_by_id = reference_col('users', nullable=True) + created_by = relationship('User', remote_side=id, foreign_keys=created_by_id) + + def __init__(self, email, full_name, password=None, **kwargs): + """Create instance.""" + # noinspection PyArgumentList + super(User, self).__init__(email=email, full_name=full_name, **kwargs) + if password: + self.set_password(password) + else: + self.set_password(hexlify(urandom(16))) + + @staticmethod + def get_by_email(email): + """Get by email.""" + return session.query(User).filter(User.email.ilike(email)).first() + + def set_password(self, password): + """Set password.""" + self.password = Bcrypt().generate_password_hash(password) + + def save_as(self, current_user, commit=True, preserve_modified=False): + """Save instance as 'current_user'.""" + if current_user and not self.created_at: + self.created_by = current_user + if current_user and not preserve_modified: + self.modified_by_id = current_user.id + # Using ``self.modified_by = current_user`` yields error when user modifies itself: + # "sqlalchemy.exc.CircularDependencyError: Circular dependency detected." + return self.save(commit=commit, preserve_modified=preserve_modified) + + class Permission(SurrogatePK, Model): + """A permission on a Collection, granted to a User.""" + + __table_args__ = (UniqueConstraint('user_id', 'collection_id'), SurrogatePK.__table_args__) + + __tablename__ = 'permissions' + user_id = reference_col('users', nullable=False) + user = relationship('User', back_populates='permissions', foreign_keys=user_id) + collection_id = reference_col('collections', nullable=False) + collection = relationship('Collection', back_populates='permissions') + + registrant = Column(Boolean(), default=False, nullable=False) + cataloger = Column(Boolean(), default=False, nullable=False) + cataloging_admin = Column(Boolean(), default=False, nullable=False) + + modified_at = Column(DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=False) + modified_by_id = reference_col('users', nullable=False) + modified_by = relationship('User', foreign_keys=modified_by_id) + + created_at = Column(DateTime, default=datetime.utcnow, nullable=False) + created_by_id = reference_col('users', nullable=False) + created_by = relationship('User', foreign_keys=created_by_id) + + def __init__(self, **kwargs): + """Create instance.""" + # noinspection PyArgumentList + super(Permission, self).__init__(**kwargs) + + class Collection(SurrogatePK, Model): + """A collection of library stuff, a.k.a. 'a sigel'.""" + + __tablename__ = 'collections' + code = Column(String(255), unique=True, nullable=False) + friendly_name = Column(String(255), unique=False, nullable=False) + category = Column(String(255), nullable=False) + is_active = Column(Boolean(), default=True) + permissions = relationship('Permission', back_populates='collection') + replaces = Column(String(255)) + replaced_by = Column(String(255)) + + modified_at = Column(DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=True) + modified_by_id = reference_col('users', nullable=True) + modified_by = relationship('User', foreign_keys=modified_by_id) + + created_at = Column(DateTime, default=datetime.utcnow, nullable=False) + created_by_id = reference_col('users', nullable=True) + created_by = relationship('User', foreign_keys=created_by_id) + + def __init__(self, code, friendly_name, category, **kwargs): + """Create instance.""" + # noinspection PyArgumentList + super(Collection, self).__init__(code=code, friendly_name=friendly_name, + category=category, **kwargs) + + class Client(Model): + """An OAuth2 Client.""" + + __tablename__ = 'clients' + client_id = Column(String(32), primary_key=True) + client_secret = Column(String(256), unique=True, nullable=False) + + is_confidential = Column(Boolean(), default=True, nullable=False) + + _redirect_uris = Column(Text(), nullable=False) + _default_scopes = Column(Text(), nullable=False) + + # Human readable info fields + name = Column(String(64), nullable=True) + description = Column(String(400), nullable=True) + + modified_at = Column(DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=True) + modified_by_id = reference_col('users', nullable=True) + modified_by = relationship('User', foreign_keys=modified_by_id) + + created_at = Column(DateTime, default=datetime.utcnow, nullable=True) + created_by_id = reference_col('users', nullable=True) + created_by = relationship('User', foreign_keys=created_by_id) + + def __init__(self, redirect_uris=None, default_scopes=None, **kwargs): + """Create instance.""" + client_id = Client._get_rand_hex_str(32) + client_secret = Client._get_rand_hex_str(256) + # noinspection PyArgumentList + super(Client, self).__init__(client_id=client_id, client_secret=client_secret, + **kwargs) + self.redirect_uris = redirect_uris + self.default_scopes = default_scopes + + @classmethod + def get_by_id(cls, record_id): + """Get record by ID.""" + return session.query(Client).filter_by(client_id=record_id).first() + + @hybrid_property + def client_type(self): + """Return client type.""" + if self.is_confidential: + return 'confidential' + else: + return 'public' + + @hybrid_property + def redirect_uris(self): + """Return redirect URIs list.""" + return self._redirect_uris.split(' ') + + @redirect_uris.setter + def redirect_uris(self, value): + """Store redirect URIs list as string.""" + if isinstance(value, string_types): + self._redirect_uris = value + elif isinstance(value, list): + self._redirect_uris = ' '.join(value) + else: + self._redirect_uris = value + + @hybrid_property + def default_redirect_uri(self): + """Return default redirect URI.""" + return self.redirect_uris[0] + + @hybrid_property + def default_scopes(self): + """Return default scopes list.""" + return self._default_scopes.split(' ') + + @default_scopes.setter + def default_scopes(self, value): + """Store default scopes list as string.""" + if isinstance(value, string_types): + self._default_scopes = value + elif isinstance(value, list): + self._default_scopes = ' '.join(value) + else: + self._default_scopes = value + + with op.batch_alter_table('clients', schema=None) as batch_op: + batch_op.alter_column('created_by', new_column_name='created_by_id') + batch_op.add_column(sa.Column('modified_by_id', sa.Integer(), nullable=True)) + batch_op.create_foreign_key('clients_modified_by_id_fkey', 'users', + ['modified_by_id'], ['id']) + batch_op.add_column(sa.Column('created_at', sa.DateTime(), nullable=True)) + batch_op.add_column(sa.Column('modified_at', sa.DateTime(), nullable=True)) + + with op.batch_alter_table('collections', schema=None) as batch_op: + batch_op.add_column(sa.Column('created_by_id', sa.Integer(), nullable=True)) + batch_op.create_foreign_key('collections_created_by_id_fkey', 'users', + ['created_by_id'], ['id']) + batch_op.add_column(sa.Column('modified_by_id', sa.Integer(), nullable=True)) + batch_op.create_foreign_key('collections_modified_by_id_fkey', 'users', + ['modified_by_id'], ['id']) + + with op.batch_alter_table('permissions', schema=None) as batch_op: + batch_op.add_column(sa.Column('created_by_id', sa.Integer(), nullable=True)) + batch_op.create_foreign_key('permissions_created_by_id_fkey', 'users', + ['created_by_id'], ['id']) + batch_op.add_column(sa.Column('modified_by_id', sa.Integer(), nullable=True)) + batch_op.create_foreign_key('permissions_modified_by_id_fkey', 'users', + ['modified_by_id'], ['id']) + + with op.batch_alter_table('users', schema=None) as batch_op: + batch_op.add_column(sa.Column('created_by_id', sa.Integer(), nullable=True)) + batch_op.create_foreign_key('users_created_by_id_fkey', 'users', + ['created_by_id'], ['id']) + batch_op.add_column(sa.Column('modified_by_id', sa.Integer(), nullable=True)) + batch_op.create_foreign_key('users_modified_by_id_fkey', 'users', + ['modified_by_id'], ['id']) + + # Assert admin user 'libris@kb.se' exists. + admin_email = 'libris@kb.se' + superuser = User.get_by_email(admin_email) + if superuser: + if not all([superuser.is_active, superuser.is_admin]): + superuser.is_active = True + superuser.is_admin = True + superuser.save() + else: + superuser = User(admin_email, 'SuperAdmin', is_active=True, is_admin=True).save() + superuser.created_by_id = superuser.id + superuser.modified_by_id = superuser.id + superuser.save(preserve_modified=True) + + # Backfill 'modified_by' and 'created_by' fields. + for client in session.query(Client).all(): + client.created_at = datetime.utcnow() + client.save_as(superuser) + + for collection in session.query(Collection).all(): + collection.created_by = superuser + collection.modified_by = superuser + if not collection.modified_at: + collection.modified_at = collection.created_at + if collection.modified_at == collection.created_at: + if collection.replaced_by: + replacement = session.query(Collection).filter_by( + code=collection.replaced_by).first() + if replacement.created_at > collection.modified_at: + collection.modified_at = replacement.created_at + collection.save_as(superuser, preserve_modified=True) + + for permission in session.query(Permission).all(): + permission.created_by = superuser + permission.modified_by = superuser + if not permission.modified_at: + permission.modified_at = permission.created_at + permission.save_as(superuser, preserve_modified=True) + + for user in session.query(User).all(): + user.created_by_id = superuser.id + user.modified_by_id = superuser.id + if not user.modified_at: + user.modified_at = user.created_at + user.save_as(superuser, preserve_modified=True) + + # Strictify NOT NULL constraints. + with op.batch_alter_table('clients', schema=None) as batch_op: + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('created_at', existing_type=sa.DateTime(), nullable=False) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=False) + + batch_op.alter_column('name', existing_type=sa.String(length=64), nullable=False) + batch_op.alter_column('description', existing_type=sa.String(length=400), nullable=False) + + with op.batch_alter_table('collections', schema=None) as batch_op: + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=False) + + with op.batch_alter_table('permissions', schema=None) as batch_op: + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=False) + + with op.batch_alter_table('users', schema=None) as batch_op: + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=False) + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=False) + + +def downgrade(): + """Drop 'modified_by'/'created_by' cols and 'laxify NOT NULL constraints.""" + with op.batch_alter_table('users', schema=None) as batch_op: + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=True) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=True) + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=True) + + with op.batch_alter_table('permissions', schema=None) as batch_op: + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=True) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=True) + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=True) + + with op.batch_alter_table('collections', schema=None) as batch_op: + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=True) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=True) + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=True) + + with op.batch_alter_table('clients', schema=None) as batch_op: + batch_op.alter_column('description', existing_type=sa.String(length=400), nullable=True) + batch_op.alter_column('name', existing_type=sa.String(length=64), nullable=True) + batch_op.alter_column('modified_at', existing_type=sa.DateTime(), nullable=True) + batch_op.alter_column('created_at', existing_type=sa.DateTime(), nullable=True) + batch_op.alter_column('modified_by_id', existing_type=sa.Integer(), nullable=True) + batch_op.alter_column('created_by_id', existing_type=sa.Integer(), nullable=True) + + with op.batch_alter_table('users', schema=None) as batch_op: + batch_op.drop_constraint('users_modified_by_id_fkey', type_='foreignkey') + batch_op.drop_column('modified_by_id') + batch_op.drop_constraint('users_created_by_id_fkey', type_='foreignkey') + batch_op.drop_column('created_by_id') + + with op.batch_alter_table('permissions', schema=None) as batch_op: + batch_op.drop_constraint('permissions_modified_by_id_fkey', type_='foreignkey') + batch_op.drop_column('modified_by_id') + batch_op.drop_constraint('permissions_created_by_id_fkey', type_='foreignkey') + batch_op.drop_column('created_by_id') + + with op.batch_alter_table('collections', schema=None) as batch_op: + batch_op.drop_constraint('collections_modified_by_id_fkey', type_='foreignkey') + batch_op.drop_column('modified_by_id') + batch_op.drop_constraint('collections_created_by_id_fkey', type_='foreignkey') + batch_op.drop_column('created_by_id') + + with op.batch_alter_table('clients', schema=None) as batch_op: + batch_op.drop_column('modified_at') + batch_op.drop_column('created_at') + batch_op.drop_constraint('clients_modified_by_id_fkey', type_='foreignkey') + batch_op.drop_column('modified_by_id') + batch_op.alter_column('created_by_id', new_column_name='created_by') diff --git a/package-lock.json b/package-lock.json index 9c833895..7a9b38d4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "xl_auth", - "version": "0.6.0", + "version": "0.6.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 10cda8b1..5d9842c0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "xl_auth", - "version": "0.6.0", + "version": "0.6.1", "author": "National Library of Sweden", "license": "Apache-2.0", "description": "OAuth2 authorization for LibrisXL, replacing BibDB counterpart", diff --git a/tests/end2end/test_collection_editing.py b/tests/end2end/test_collection_editing.py index e96fcc08..1087f572 100644 --- a/tests/end2end/test_collection_editing.py +++ b/tests/end2end/test_collection_editing.py @@ -14,6 +14,10 @@ def test_superuser_can_edit_existing_collection(superuser, collection, testapp): """Edit an existing collection.""" + # Check expected premises + collection_creator = collection.created_by + initial_modified_by = collection.modified_by + assert collection_creator != superuser and initial_modified_by != superuser # Goes to homepage res = testapp.get('/') # Fills out login form @@ -39,6 +43,9 @@ def test_superuser_can_edit_existing_collection(superuser, collection, testapp): edited_collection = Collection.query.filter(Collection.code == collection.code).first() assert edited_collection.friendly_name == form['friendly_name'].value assert edited_collection.category == form['category'].value + # 'modified_by' is updated to reflect change, with 'created_by' intact + assert edited_collection.created_by == collection_creator + assert edited_collection.modified_by == superuser # The edited collection is listed under existing collections collection_id = 'collection-{0}'.format(form['code'].value) collection_anchor_xpath = "//a[@class='anchor' and @id='{0}']".format(collection_id) @@ -53,8 +60,7 @@ def test_superuser_can_edit_existing_collection(superuser, collection, testapp): _(form['category'].value.capitalize())))) == 1 else: assert len(res.lxml.xpath("//td[contains(., '{0}')]/ancestor::tr/td[contains(., '{1}')]" - .format(form['code'].value, - _('No category')))) == 1 + .format(form['code'].value, _('No category')))) == 1 def test_superuser_sees_error_message_if_code_is_changed(superuser, collection, testapp): diff --git a/tests/end2end/test_collection_registering.py b/tests/end2end/test_collection_registering.py index 7caef767..48cadc57 100644 --- a/tests/end2end/test_collection_registering.py +++ b/tests/end2end/test_collection_registering.py @@ -39,9 +39,13 @@ def test_superuser_can_register_new_collection(superuser, testapp): assert res.status_code == 200 # A new collection was created assert len(Collection.query.all()) == old_count + 1 + registered_collection = Collection.query.filter(Collection.code == form['code'].value).first() + # Keeping track of who created what + assert registered_collection.created_by == superuser + assert registered_collection.modified_by == superuser # The new collection is listed under existing collections - collection_id = 'collection-{0}'.format(form['code'].value) - collection_anchor_xpath = "//a[@class='anchor' and @id='{0}']".format(collection_id) + collection_anchor_xpath = "//a[@class='anchor' and @id='{0}']".format( + 'collection-' + registered_collection.code) assert len(res.lxml.xpath(collection_anchor_xpath)) == 1 assert len(res.lxml.xpath("//td[contains(., '{0}')]".format(form['code'].value))) == 1 @@ -52,8 +56,7 @@ def test_superuser_can_register_new_collection(superuser, testapp): _(form['category'].value.capitalize())))) == 1 else: assert len(res.lxml.xpath("//td[contains(., '{0}')]/ancestor::tr/td[contains(., '{1}')]" - .format(form['code'].value, - _('No category')))) == 1 + .format(form['code'].value, _('No category')))) == 1 # noinspection PyUnusedLocal diff --git a/tests/end2end/test_oauth_editing_client.py b/tests/end2end/test_oauth_editing_client.py index a2c1c1f9..450766bb 100644 --- a/tests/end2end/test_oauth_editing_client.py +++ b/tests/end2end/test_oauth_editing_client.py @@ -11,6 +11,10 @@ def test_superuser_can_edit_existing_client(superuser, client, testapp): """Edit an existing client.""" old_count = len(Client.query.all()) + # Check expected premises + client_creator = client.created_by + initial_modified_by = client.modified_by + assert client_creator != superuser and initial_modified_by != superuser # Goes to homepage res = testapp.get('/') # Fills out login form @@ -40,6 +44,16 @@ def test_superuser_can_edit_existing_client(superuser, client, testapp): res = form.submit().follow() assert res.status_code == 200 + # The client was edited + edited_client = Client.get_by_id(client.client_id) + assert edited_client.name == form['name'].value + assert edited_client.description == form['description'].value + assert edited_client.redirect_uris == ['http://localhost/'] + assert edited_client.default_scopes == ['read', 'write'] + # 'modified_by' is updated to reflect change, with 'created_by' intact + assert edited_client.created_by == client_creator + assert edited_client.modified_by == superuser + # No new client was created assert len(Client.query.all()) == old_count diff --git a/tests/end2end/test_oauth_registering_client.py b/tests/end2end/test_oauth_registering_client.py index 9bbcc371..b4d11c73 100644 --- a/tests/end2end/test_oauth_registering_client.py +++ b/tests/end2end/test_oauth_registering_client.py @@ -43,6 +43,10 @@ def test_superuser_can_register_new_client(superuser, testapp): # A new client was created assert len(Client.query.all()) == old_count + 1 + registered_client = Client.query.filter(Client.name == form['name'].value).first() + # Keeping track of who created what + assert registered_client.created_by == superuser + assert registered_client.modified_by == superuser # The new client is listed under existing clients assert len(res.lxml.xpath("//td[contains(., '{0}')]".format(form['name'].value))) == 1 diff --git a/tests/end2end/test_public_logging_in.py b/tests/end2end/test_public_logging_in.py index d0112e41..7a772317 100644 --- a/tests/end2end/test_public_logging_in.py +++ b/tests/end2end/test_public_logging_in.py @@ -38,10 +38,13 @@ def test_unauthorized_leads_to_login_which_follows_next_redirect_param_on_succes assert '/oauth/authorize?param1=value1¶m2=value2' in res.location -def test_successful_login_updates_last_login_at(user, testapp): - """Successful login sets 'last_login_at' to current UTC datetime.""" +def test_successful_login_updates_last_login_at_only(user, testapp): + """Successful login sets 'last_login_at' to current UTC datetime (but not 'modified_*').""" + # Check expected premises. assert user.last_login_at is None - + initial_modified_at = user.modified_at + initial_modified_by = user.modified_by + assert initial_modified_by != user # Goes to homepage. res = testapp.get('/') # Fills out login form. @@ -52,10 +55,12 @@ def test_successful_login_updates_last_login_at(user, testapp): res = form.submit().follow() assert res.status_code == 200 - # Fetch user from database, now with login timestamp. + # Fetch user from database, now with login timestamp but no modified at/by changes. updated_user = User.get_by_id(user.id) assert isinstance(updated_user.last_login_at, datetime) assert (datetime.utcnow() - updated_user.last_login_at).total_seconds() < 10 + assert updated_user.modified_at == initial_modified_at + assert updated_user.modified_by == initial_modified_by def test_sees_alert_on_log_out(user, testapp): diff --git a/tests/end2end/test_public_reset_password.py b/tests/end2end/test_public_reset_password.py index 0afae867..1491d623 100644 --- a/tests/end2end/test_public_reset_password.py +++ b/tests/end2end/test_public_reset_password.py @@ -14,11 +14,16 @@ from ..factories import UserFactory -# noinspection PyUnusedLocal def test_can_complete_password_reset_flow(db, testapp): """Successfully request password reset, use code to change password and activate account.""" inactive_user = UserFactory(is_active=False) + db.session.commit() assert inactive_user.is_active is False + # Check expected premises. + user_creator = inactive_user.created_by + initial_modified_at = inactive_user.modified_at + initial_modified_by = inactive_user.modified_by + assert initial_modified_by != inactive_user # Goes to homepage. res = testapp.get('/') # Clicks on 'Forgot password'. @@ -35,6 +40,7 @@ def test_can_complete_password_reset_flow(db, testapp): password_reset = PasswordReset.query.filter_by(user=inactive_user).first() assert password_reset.is_active is True assert password_reset.expires_at > (datetime.utcnow() + timedelta(seconds=3600)) + assert password_reset.user.modified_at == initial_modified_at # URL sent to user email. reset_password_url = url_for('public.reset_password', email=password_reset.user.email, @@ -54,6 +60,10 @@ def test_can_complete_password_reset_flow(db, testapp): assert updated_password_reset.is_active is False assert updated_password_reset.user.check_password('unicorns are real') is True assert updated_password_reset.user.is_active is True + # 'modified_by' is updated to reflect change, with 'created_by' intact. + assert updated_password_reset.user.created_by == user_creator + assert updated_password_reset.user.modified_at != initial_modified_at + assert updated_password_reset.user.modified_by == updated_password_reset.user # noinspection PyUnusedLocal diff --git a/tests/end2end/test_user_editing.py b/tests/end2end/test_user_editing.py index 83feed3c..58ae9509 100644 --- a/tests/end2end/test_user_editing.py +++ b/tests/end2end/test_user_editing.py @@ -9,8 +9,12 @@ from xl_auth.user.models import User -def test_superuser_can_administer_existing_user(superuser, testapp): +def test_superuser_can_administer_existing_user(superuser, user, testapp): """Administer user details for an existing user.""" + # Check expected premises. + user_creator = user.created_by + initial_modified_by = user.modified_by + assert user_creator != superuser and initial_modified_by != superuser # Goes to homepage. res = testapp.get('/') # Fills out login form. @@ -22,20 +26,23 @@ def test_superuser_can_administer_existing_user(superuser, testapp): # Clicks Users button. res = res.click(_('Users')) # Clicks Edit Details button. - res = res.click(href='/users/administer/{0}'.format(superuser.id)) + res = res.click(href='/users/administer/{0}'.format(user.id)) # Fills out the form. form = res.forms['administerForm'] - form['username'] = superuser.email + form['username'] = user.email form['full_name'] = 'A new name' - form['is_active'].checked = not superuser.is_active + form['is_active'].checked = not user.is_active # Submits. res = form.submit().follow() assert res.status_code == 200 # The user was edited. - edited_user = User.query.filter(User.email == superuser.email).first() + edited_user = User.get_by_email(user.email) assert edited_user.full_name == form['full_name'].value assert edited_user.is_active == form['is_active'].checked assert edited_user.is_admin == form['is_admin'].checked + # 'modified_by' is updated to reflect change, with 'created_by' intact + assert edited_user.created_by == user_creator + assert edited_user.modified_by == superuser # The edited user is listed under existing users. assert len(res.lxml.xpath("//td[contains(., '{0}')]".format(form['username'].value))) == 1 @@ -48,8 +55,12 @@ def test_superuser_can_administer_existing_user(superuser, testapp): assert len(res.lxml.xpath(admin_xpath)) == 1 -def test_superuser_can_change_password_for_existing_user(superuser, testapp): +def test_superuser_can_change_password_for_existing_user(superuser, user, testapp): """Change password for an existing user.""" + # Check expected premises. + user_creator = user.created_by + initial_modified_by = user.modified_by + assert user_creator != superuser and initial_modified_by != superuser # Goes to homepage. res = testapp.get('/') # Fills out login form. @@ -61,20 +72,23 @@ def test_superuser_can_change_password_for_existing_user(superuser, testapp): # Clicks Users button. res = res.click(_('Users')) # Clicks Change Password button. - res = res.click(href='/users/change_password/{0}'.format(superuser.id)) + res = res.click(href='/users/change_password/{0}'.format(user.id)) # Fills out the form. form = res.forms['changePasswordForm'] - form['username'] = superuser.email + form['username'] = user.email form['password'] = 'newSecrets13' form['confirm'] = 'newSecrets13' # Submits. res = form.submit().follow() assert res.status_code == 200 # The user was edited. - edited_user = User.query.filter(User.email == superuser.email).first() + edited_user = User.query.filter(User.email == user.email).first() # Verify the new password is considered valid, not the old one. assert edited_user.check_password('myPrecious') is False assert edited_user.check_password('newSecrets13') is True + # 'modified_by' is updated to reflect change, with 'created_by' intact + assert edited_user.created_by == user_creator + assert edited_user.modified_by == superuser def test_superuser_sees_error_message_if_username_is_changed_from_administer(superuser, testapp): @@ -197,6 +211,8 @@ def test_user_cannot_administer_other_user(superuser, user, testapp): def test_user_can_edit_own_details(user, testapp): """Change details for self.""" + user_creator = user.created_by + assert user_creator != user # Goes to homepage. res = testapp.get('/') # Fills out login form. @@ -218,6 +234,10 @@ def test_user_can_edit_own_details(user, testapp): form = res.forms['editDetailsForm'] form['full_name'] = 'New Name' res = form.submit().follow() + # 'modified_by' is updated to reflect change, with 'created_by' intact. + edited_user = User.get_by_email(user.email) + assert edited_user.created_by == user_creator + assert edited_user.modified_by == user # Make sure name has been updated assert len(res.lxml.xpath("//h1[contains(., '{0} {1}')]".format(_('Welcome'), old_name))) == 0 diff --git a/tests/end2end/test_user_registering.py b/tests/end2end/test_user_registering.py index cf330ae2..867fc075 100644 --- a/tests/end2end/test_user_registering.py +++ b/tests/end2end/test_user_registering.py @@ -29,7 +29,7 @@ def test_superuser_can_register_not_triggering_password_reset(superuser, testapp # Fills out the form form = res.forms['registerUserForm'] form['username'] = 'foo@bar.com' - form['full_name'] = 'Mr End2End' + form['full_name'] = 'End2End' form['send_password_reset_email'].checked = False # Submits res = form.submit().follow() @@ -38,6 +38,9 @@ def test_superuser_can_register_not_triggering_password_reset(superuser, testapp new_user = User.get_by_email('foo@bar.com') assert isinstance(new_user, User) assert new_user.is_active is False + # Keeping track of who created what + assert new_user.created_by == superuser + assert new_user.modified_by == superuser # A password reset was not created password_reset = PasswordReset.query.filter_by(user=new_user).first() assert password_reset is None @@ -61,7 +64,7 @@ def test_superuser_can_register_with_password_reset(superuser, testapp): # Fills out the form form = res.forms['registerUserForm'] form['username'] = 'foo@bar.com' - form['full_name'] = 'Mr End2End' + form['full_name'] = 'End2End' form['send_password_reset_email'].checked = True # Submits res = form.submit().follow() @@ -70,6 +73,8 @@ def test_superuser_can_register_with_password_reset(superuser, testapp): new_user = User.get_by_email('foo@bar.com') assert isinstance(new_user, User) assert new_user.is_active is False + assert new_user.created_by == superuser + assert new_user.modified_by == superuser # A password reset was created password_resets = PasswordReset.query.filter_by(user=new_user).all() assert len(password_resets) == 1 @@ -115,7 +120,7 @@ def test_user_sees_error_message_if_user_already_registered(superuser, user, tes # Fills out form, but username is already registered. form = res.forms['registerUserForm'] form['username'] = user.email.upper() # Default would be `userN@example.com`, not upper-cased. - form['full_name'] = 'Mr End2End' + form['full_name'] = 'End2End' # Submits. res = form.submit() # Sees error. diff --git a/tests/factories.py b/tests/factories.py index dfb555e4..6c007e9a 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -27,13 +27,17 @@ class Meta: sqlalchemy_session = db.session -class UserFactory(BaseFactory): - """User factory.""" +class SuperUserFactory(BaseFactory): + """Super user factory.""" - email = Sequence(lambda _: 'user{0}@example.com'.format(_)) + email = Sequence(lambda _: 'admin{0}@example.com'.format(_)) full_name = Sequence(lambda _: 'full_name{0}'.format(_)) password = PostGenerationMethodCall('set_password', 'example') is_active = True + is_admin = True + + modified_by_id = '1' + created_by_id = '1' class Meta: """Factory configuration.""" @@ -41,14 +45,16 @@ class Meta: model = User -class SuperUserFactory(BaseFactory): - """Super user factory.""" +class UserFactory(BaseFactory): + """User factory.""" - email = Sequence(lambda _: 'admin{0}@example.com'.format(_)) + email = Sequence(lambda _: 'user{0}@example.com'.format(_)) full_name = Sequence(lambda _: 'full_name{0}'.format(_)) password = PostGenerationMethodCall('set_password', 'example') is_active = True - is_admin = True + + modified_by = LazyFunction(SuperUserFactory) + created_by = LazyFunction(SuperUserFactory) class Meta: """Factory configuration.""" @@ -75,6 +81,9 @@ class CollectionFactory(BaseFactory): category = choice(['bibliography', 'library', 'uncategorized']) is_active = True + modified_by = LazyFunction(UserFactory) + created_by = LazyFunction(UserFactory) + class Meta: """Factory configuration.""" @@ -87,6 +96,9 @@ class PermissionFactory(BaseFactory): user = LazyFunction(UserFactory) collection = LazyFunction(CollectionFactory) + modified_by = LazyFunction(UserFactory) + created_by = LazyFunction(UserFactory) + class Meta: """Factory configuration.""" @@ -101,7 +113,9 @@ class ClientFactory(BaseFactory): is_confidential = True redirect_uris = 'https://libris.kb.se http://example.com' default_scopes = 'read write' - created_by = '1' + + modified_by = LazyFunction(UserFactory) + created_by = LazyFunction(UserFactory) class Meta: """Factory configuration.""" diff --git a/tests/forms/test_user_administer.py b/tests/forms/test_user_administer.py index f4eba11f..9b177f02 100644 --- a/tests/forms/test_user_administer.py +++ b/tests/forms/test_user_administer.py @@ -25,7 +25,7 @@ def test_validate_success(superuser): def test_validate_username_does_not_exist(db, superuser): """Attempt to edit user details with a username that is not registered.""" form = AdministerForm(superuser, 'missing@nowhere.com', username='missing@nowhere.com', - full_name='Mr Foo', is_active=choice([True, False]), + full_name='FooBar', is_active=choice([True, False]), is_admin=choice([True, False])) assert form.validate() is False diff --git a/tests/forms/test_user_register.py b/tests/forms/test_user_register.py index e60f8ee4..e63566f9 100644 --- a/tests/forms/test_user_register.py +++ b/tests/forms/test_user_register.py @@ -22,7 +22,7 @@ def test_validate_success(db, superuser): # noinspection PyUnusedLocal def test_validate_without_full_name(superuser, db): """Attempt registering user with name shorter than 3 chars.""" - form = RegisterForm(superuser, username='mr.librarian@kb.se', full_name='01') + form = RegisterForm(superuser, username='librarian@kb.se', full_name='01') assert form.validate() is False assert _('Field must be between 3 and 255 characters long.') in form.full_name.errors diff --git a/tests/models/test_client.py b/tests/models/test_client.py index e291bf5d..8eef6df2 100644 --- a/tests/models/test_client.py +++ b/tests/models/test_client.py @@ -3,36 +3,84 @@ from __future__ import absolute_import, division, print_function, unicode_literals +from datetime import datetime + import pytest from xl_auth.oauth.client.models import Client +from xl_auth.user.models import User -from ..factories import ClientFactory +from ..factories import ClientFactory, SuperUserFactory -@pytest.mark.usefixtures('db', 'user') -def test_get_by_id(user): +def test_get_by_id(superuser): """Get client by ID.""" - client = Client(created_by=user.id, redirect_uris='http://example.com', default_scopes='fake') - client.save() + client = Client(name='favv', description='favorite client', redirect_uris='http://example.com', + default_scopes='fake') + client.save_as(superuser) retrieved = Client.get_by_id(client.client_id) assert retrieved == client -@pytest.mark.usefixtures('db', 'user') -def test_factory(db, user): +def test_created_by_and_modified_by_is_updated(superuser): + """Test created/modified by.""" + client = Client(name='favv', description='favorite client', redirect_uris='http://example.com', + default_scopes='fake') + client.save_as(superuser) + assert client.created_by_id == superuser.id + assert client.created_by == superuser + assert client.modified_by_id == superuser.id + assert client.modified_by == superuser + + # Another superuser updates something in the client. + another_superuser = SuperUserFactory() + client.update_as(another_superuser, commit=True, default_scopes='another') + assert client.created_by == superuser + assert client.modified_by == another_superuser + + +def test_created_at_defaults_to_datetime(superuser): + """Test creation date.""" + client = Client(name='favv', description='favorite client', redirect_uris='http://example.com', + default_scopes='fake') + client.save_as(superuser) + + assert bool(client.created_at) + assert isinstance(client.created_at, datetime) + + +def test_modified_at_defaults_to_current_datetime(superuser): + """Test modified date.""" + client = Client(name='favv', description='favorite client', redirect_uris='http://example.com', + default_scopes='fake') + client.save_as(superuser) + first_modified_at = client.modified_at + + assert abs((first_modified_at - client.created_at).total_seconds()) < 10 + + client.is_confidential = not client.is_confidential + client.save() + + assert first_modified_at != client.modified_at + + +def test_factory(db): """Test user factory.""" - client = ClientFactory(created_by=user.id) + client = ClientFactory() db.session.commit() assert bool(client.client_id) assert bool(client.client_secret) - assert bool(client.created_by) assert bool(client.is_confidential) - assert bool(client.name) - assert bool(client.description) assert client.default_scopes == ['read', 'write'] assert client.redirect_uris == ['https://libris.kb.se', 'http://example.com'] + assert bool(client.name) + assert bool(client.description) + + assert isinstance(client.modified_at, datetime) + assert isinstance(client.modified_by, User) + assert isinstance(client.created_at, datetime) + assert isinstance(client.created_by, User) @pytest.mark.usefixtures('db') diff --git a/tests/models/test_collection.py b/tests/models/test_collection.py index 1961507a..43441a72 100644 --- a/tests/models/test_collection.py +++ b/tests/models/test_collection.py @@ -3,7 +3,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import datetime as dt +from datetime import datetime import pytest from flask_babel import gettext as _ @@ -11,35 +11,49 @@ from xl_auth.collection.models import Collection from xl_auth.permission.models import Permission +from xl_auth.user.models import User -from ..factories import CollectionFactory +from ..factories import CollectionFactory, SuperUserFactory -@pytest.mark.usefixtures('db') -def test_get_by_id(): +def test_get_by_id(superuser): """Get collection by ID.""" collection = Collection(code='SKB', friendly_name='Literature by Strindberg', category='bibliography') - collection.save() + collection.save_as(superuser) retrieved = Collection.get_by_id(collection.id) assert retrieved == collection -@pytest.mark.usefixtures('db') -def test_created_at_defaults_to_datetime(): +def test_created_by_and_modified_by_is_updated(superuser): + """Test created/modified by.""" + collection = Collection('KBX', 'Secret books', 'library') + collection.save_as(superuser) + assert collection.created_by_id == superuser.id + assert collection.created_by == superuser + assert collection.modified_by_id == superuser.id + assert collection.modified_by == superuser + + # Another superuser updates something in the collection. + another_superuser = SuperUserFactory() + collection.update_as(another_superuser, commit=True, is_active=not collection.is_active) + assert collection.created_by == superuser + assert collection.modified_by == another_superuser + + +def test_created_at_defaults_to_datetime(superuser): """Test creation date.""" collection = Collection('KBX', 'Secret books', 'library') - collection.save() + collection.save_as(superuser) assert bool(collection.created_at) - assert isinstance(collection.created_at, dt.datetime) + assert isinstance(collection.created_at, datetime) -@pytest.mark.usefixtures('db') -def test_modified_at_defaults_to_current_datetime(): +def test_modified_at_defaults_to_current_datetime(superuser): """Test modified date.""" collection = Collection('KBU', 'Outdated name', 'library') - collection.save() + collection.save_as(superuser) first_modified_at = collection.modified_at assert abs((first_modified_at - collection.created_at).total_seconds()) < 10 @@ -50,7 +64,6 @@ def test_modified_at_defaults_to_current_datetime(): assert first_modified_at != collection.modified_at -@pytest.mark.usefixtures('db') def test_factory(db): """Test collection factory.""" collection = CollectionFactory() @@ -59,31 +72,32 @@ def test_factory(db): assert isinstance(collection.friendly_name, string_types) assert collection.category in {'bibliography', 'library', 'uncategorized'} assert collection.is_active is True + assert isinstance(collection.permissions, list) assert collection.replaces is None assert collection.replaced_by is None - assert isinstance(collection.permissions, list) - assert bool(collection.modified_at) - assert bool(collection.created_at) + assert isinstance(collection.modified_at, datetime) + assert isinstance(collection.modified_by, User) + assert isinstance(collection.created_at, datetime) + assert isinstance(collection.created_by, User) -@pytest.mark.usefixtures('db') -def test_adding_permissions(user): + +def test_adding_permissions(superuser, user): """Add a permission on the collection.""" collection = CollectionFactory() collection.save() permission = Permission(user=user, collection=collection) - permission.save() + permission.save_as(superuser) assert permission in collection.permissions -@pytest.mark.usefixtures('db') -def test_removing_permissions(user): +def test_removing_permissions(superuser, user): """Remove the permissions an a collection.""" collection = CollectionFactory() collection.save() permission = Permission(user=user, collection=collection) - permission.save() + permission.save_as(superuser) permission.delete() assert permission not in collection.permissions diff --git a/tests/models/test_grant.py b/tests/models/test_grant.py index 78d09e56..05e8c22d 100644 --- a/tests/models/test_grant.py +++ b/tests/models/test_grant.py @@ -10,7 +10,6 @@ from ..factories import GrantFactory -@pytest.mark.usefixtures('db', 'user', 'client') def test_get_by_id(user, client): """Get grant by ID.""" grant = Grant(user=user, client=client, code='temp-secret', redirect_uri='http://example.com', @@ -21,7 +20,6 @@ def test_get_by_id(user, client): assert retrieved == grant -@pytest.mark.usefixtures('db') def test_factory(db): """Test grant factory.""" grant = GrantFactory() diff --git a/tests/models/test_password_reset.py b/tests/models/test_password_reset.py index 5f1f4f0e..283a0950 100644 --- a/tests/models/test_password_reset.py +++ b/tests/models/test_password_reset.py @@ -14,7 +14,6 @@ from ..factories import PasswordResetFactory, UserFactory -@pytest.mark.usefixtures('db') def test_get_by_id(user): """Get password reset by ID.""" password_reset = PasswordReset(user) @@ -24,7 +23,6 @@ def test_get_by_id(user): assert retrieved == password_reset -@pytest.mark.usefixtures('db') def test_created_at_defaults_to_datetime(user): """Test creation date.""" password_reset = PasswordReset(user) @@ -33,7 +31,6 @@ def test_created_at_defaults_to_datetime(user): assert isinstance(password_reset.created_at, dt.datetime) -@pytest.mark.usefixtures('db') def test_modified_at_defaults_to_current_datetime(user): """Test modified date.""" password_reset = PasswordReset(user) @@ -49,7 +46,6 @@ def test_modified_at_defaults_to_current_datetime(user): assert first_modified_at != password_reset.modified_at -@pytest.mark.usefixtures('db') def test_code_defaults_to_a_random_one_with_length_32(user): """Test code field is automatically assigned some 32-char random string.""" password_reset_1 = PasswordReset(user) @@ -63,7 +59,6 @@ def test_code_defaults_to_a_random_one_with_length_32(user): assert len(password_reset_2.code) == 32 -@pytest.mark.usefixtures('db') def test_factory(db): """Test password reset factory.""" password_reset = PasswordResetFactory() @@ -92,7 +87,6 @@ def test_repr(): assert repr(password_reset) == ''.format(password_reset.user.email) -@pytest.mark.usefixtures('db') def test_unique_constraint(user): """Test uniqueness constraint for user-code pairs.""" password_reset = PasswordResetFactory(user=user) diff --git a/tests/models/test_permission.py b/tests/models/test_permission.py index 8a57d27f..32d1893c 100644 --- a/tests/models/test_permission.py +++ b/tests/models/test_permission.py @@ -3,7 +3,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import datetime as dt +from datetime import datetime import pytest from sqlalchemy.exc import IntegrityError @@ -12,34 +12,47 @@ from xl_auth.permission.models import Permission from xl_auth.user.models import User -from ..factories import PermissionFactory +from ..factories import PermissionFactory, SuperUserFactory -@pytest.mark.usefixtures('db') -def test_get_by_id(user, collection): +def test_get_by_id(superuser, user, collection): """Get permission by ID.""" permission = Permission(user=user, collection=collection) - permission.save() + permission.save_as(superuser) retrieved = Permission.get_by_id(permission.id) assert retrieved == permission -@pytest.mark.usefixtures('db') -def test_created_at_defaults_to_datetime(user, collection): +def test_created_by_and_modified_by_is_updated(superuser, user, collection): + """Test created/modified by.""" + permission = Permission(user=user, collection=collection) + permission.save_as(superuser) + assert permission.created_by_id == superuser.id + assert permission.created_by == superuser + assert permission.modified_by_id == superuser.id + assert permission.modified_by == superuser + + # Another superuser updates something in the permission. + another_superuser = SuperUserFactory() + permission.update_as(another_superuser, commit=True, cataloger=not permission.cataloger) + assert permission.created_by == superuser + assert permission.modified_by == another_superuser + + +def test_created_at_defaults_to_datetime(superuser, user, collection): """Test creation date.""" permission = Permission(user=user, collection=collection) - permission.save() + permission.save_as(superuser) assert bool(permission.created_at) - assert isinstance(permission.created_at, dt.datetime) + assert isinstance(permission.created_at, datetime) -@pytest.mark.usefixtures('db') -def test_modified_at_defaults_to_current_datetime(user, collection): +def test_modified_at_defaults_to_current_datetime(superuser, user, collection): """Test modified date.""" permission = Permission(user=user, collection=collection) - permission.save() + permission.save_as(superuser) first_modified_at = permission.modified_at assert abs((first_modified_at - permission.created_at).total_seconds()) < 10 @@ -50,7 +63,6 @@ def test_modified_at_defaults_to_current_datetime(user, collection): assert first_modified_at != permission.modified_at -@pytest.mark.usefixtures('db') def test_factory(db): """Test permission factory.""" permission = PermissionFactory() @@ -58,21 +70,23 @@ def test_factory(db): assert isinstance(permission.user, User) assert isinstance(permission.collection, Collection) + assert permission.registrant is False assert permission.cataloger is False assert permission.cataloging_admin is False - assert bool(permission.modified_at) - assert bool(permission.created_at) + + assert isinstance(permission.modified_at, datetime) + assert isinstance(permission.modified_by, User) + assert isinstance(permission.created_at, datetime) + assert isinstance(permission.created_by, User) -@pytest.mark.usefixtures('db') def test_repr(user, collection): """Check repr output.""" permission = PermissionFactory(user=user, collection=collection) assert repr(permission) == ''.format(user, collection) -@pytest.mark.usefixtures('db') def test_unique_constraint(user, collection): """Test uniqueness constraint for user-collection pairs.""" permission = PermissionFactory(user=user, collection=collection) diff --git a/tests/models/test_user.py b/tests/models/test_user.py index 63c17a30..1d9611df 100644 --- a/tests/models/test_user.py +++ b/tests/models/test_user.py @@ -3,7 +3,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import datetime as dt +from datetime import datetime import pytest @@ -13,46 +13,70 @@ from ..factories import UserFactory -@pytest.mark.usefixtures('db') -def test_get_by_id(): +def test_get_by_id(superuser): """Get user by ID.""" - user = User('foo@bar.com', 'Mr. Foo Bar') - user.save() + user = User('foo@bar.com', 'Foo Bar') + user.save_as(superuser) retrieved = User.get_by_id(user.id) assert retrieved == user -@pytest.mark.usefixtures('db') -def test_created_at_defaults_to_datetime(): +def test_created_by_and_modified_by_is_updated(superuser): + """Test created/modified by.""" + user = User(email='foo@bar.com', full_name='Foo Bar') + user.save_as(superuser) + assert user.created_by_id == superuser.id + assert user.created_by == superuser + assert user.modified_by_id == superuser.id + assert user.modified_by == superuser + + # User updates something in their profile. + user.update_as(user, commit=True, full_name='Bar Foo') + assert user.created_by == superuser + assert user.modified_by == user + + +def test_created_at_defaults_to_datetime(superuser): """Test creation date.""" - user = User(email='foo@bar.com', full_name='Mr. Foo Bar') - user.save() + user = User(email='foo@bar.com', full_name='Foo Bar') + user.save_as(superuser) assert bool(user.created_at) - assert isinstance(user.created_at, dt.datetime) + assert isinstance(user.created_at, datetime) -@pytest.mark.usefixtures('db') -def test_modified_at_defaults_to_current_datetime(): +def test_modified_at_defaults_to_current_datetime(superuser): """Test modified date.""" user = User('foo@kb.se', 'Wrong Name') - user.save() + user.save_as(superuser) first_modified_at = user.modified_at assert abs((first_modified_at - user.created_at).total_seconds()) < 10 user.full_name = 'Correct Name' - user.save() + user.save_as(user) # Initial 'modified_at' has been overwritten. assert first_modified_at != user.modified_at -@pytest.mark.usefixtures('db') -def test_password_defaults_to_a_random_one(): - """Test empty password field is assigned some random password, instead of being set to tull.""" - user = User(email='foo@bar.com', full_name='Mr. Foo Bar') - user.save() +def test_update_last_login_does_not_update_modified_at(superuser): + """Test modified date.""" + user = User('foo@kb.se', 'Hello World') + user.save_as(superuser) + first_modified_at = user.modified_at + + # Update 'last_login_at' timestamp. + user.update_last_login(commit=True) + + # Initial 'modified_at' is still the same. + assert first_modified_at == user.modified_at + + +def test_password_defaults_to_a_random_one(superuser): + """Test empty password field is assigned some random password, instead of being set to null.""" + user = User(email='foo@bar.com', full_name='Foo Bar') + user.save_as(superuser) assert user.password is not None @@ -61,22 +85,28 @@ def test_factory(db): """Test user factory.""" user = UserFactory(password='myPrecious') db.session.commit() + assert bool(user.email) assert bool(user.full_name) - assert bool(user.modified_at) - assert bool(user.created_at) - assert isinstance(user.permissions, list) - assert isinstance(user.roles, list) - assert user.is_admin is False - assert user.is_active is True assert user.check_password('myPrecious') assert user.last_login_at is None + assert user.is_active is True + assert user.is_admin is False + assert isinstance(user.permissions, list) + assert isinstance(user.roles, list) + assert isinstance(user.password_resets, list) -@pytest.mark.usefixtures('db') -def test_check_password(): + assert isinstance(user.modified_at, datetime) + assert isinstance(user.modified_by, User) + assert isinstance(user.created_at, datetime) + assert isinstance(user.created_by, User) + + +def test_check_password(superuser): """Check password.""" - user = User.create(email='foo@bar.com', full_name='Mr. Foo Bar', password='fooBarBaz123') + user = User.create_as(superuser, email='foo@bar.com', full_name='Foo Bar', + password='fooBarBaz123') assert user.check_password('fooBarBaz123') is True assert user.check_password('barFooBaz') is False @@ -94,7 +124,7 @@ def test_adding_permissions(collection): user = UserFactory() user.save() permission = Permission(user=user, collection=collection) - permission.save() + permission.save_as(user) assert permission in user.permissions @@ -105,7 +135,7 @@ def test_removing_permissions(collection): user = UserFactory() user.save() permission = Permission(user=user, collection=collection) - permission.save() + permission.save_as(user) permission.delete() assert permission not in user.permissions @@ -140,7 +170,7 @@ def test_roles(): @pytest.mark.usefixtures('db') -def test_adding_password_reset(collection): +def test_adding_password_reset(): """Associate user with a password reset code.""" user = UserFactory() user.save() @@ -157,7 +187,7 @@ def test_adding_password_reset(collection): @pytest.mark.usefixtures('db') -def test_removing_password_reset(collection): +def test_removing_password_reset(): """Remove password reset from a user.""" user = UserFactory() user.save() diff --git a/xl_auth/collection/models.py b/xl_auth/collection/models.py index 073b1a7d..e3cde133 100644 --- a/xl_auth/collection/models.py +++ b/xl_auth/collection/models.py @@ -3,11 +3,11 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import datetime as dt +from datetime import datetime from flask_babel import lazy_gettext as _ -from ..database import Column, Model, SurrogatePK, db, relationship +from ..database import Column, Model, SurrogatePK, db, reference_col, relationship class Collection(SurrogatePK, Model): @@ -22,18 +22,20 @@ class Collection(SurrogatePK, Model): replaces = Column(db.String(255)) replaced_by = Column(db.String(255)) - modified_at = Column(db.DateTime, default=dt.datetime.utcnow, onupdate=dt.datetime.utcnow) - created_at = Column(db.DateTime, nullable=False, default=dt.datetime.utcnow) + modified_at = Column(db.DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=False) + modified_by_id = reference_col('users', nullable=False) + modified_by = relationship('User', foreign_keys=modified_by_id) + + created_at = Column(db.DateTime, default=datetime.utcnow, nullable=False) + created_by_id = reference_col('users', nullable=False) + created_by = relationship('User', foreign_keys=created_by_id) def __init__(self, code, friendly_name, category, **kwargs): """Create instance.""" db.Model.__init__(self, code=code, friendly_name=friendly_name, category=category, **kwargs) - def __repr__(self): - """Represent instance as a unique string.""" - return ''.format(code=self.code) - def get_replaces_and_replaced_by_str(self): """Build string with replaces/replaced-by info.""" if self.replaced_by and self.replaces: @@ -45,3 +47,7 @@ def get_replaces_and_replaced_by_str(self): return _('Replaced by %(replaced_by_code)s', replaced_by_code=self.replaced_by) else: return '' + + def __repr__(self): + """Represent instance as a unique string.""" + return ''.format(code=self.code) diff --git a/xl_auth/collection/views.py b/xl_auth/collection/views.py index 48afa0be..b717197b 100644 --- a/xl_auth/collection/views.py +++ b/xl_auth/collection/views.py @@ -34,10 +34,11 @@ def register(): register_collection_form = RegisterForm(current_user, request.form) if register_collection_form.validate_on_submit(): - Collection.create(code=register_collection_form.code.data, - friendly_name=register_collection_form.friendly_name.data, - category=register_collection_form.category.data, - is_active=True) + Collection.create_as(current_user, + code=register_collection_form.code.data, + friendly_name=register_collection_form.friendly_name.data, + category=register_collection_form.category.data, + is_active=True) flash(_('Thank you for registering a new collection.'), 'success') return redirect(url_for('collection.home')) else: @@ -60,8 +61,9 @@ def edit(collection_code): edit_collection_form = EditForm(current_user, collection_code, request.form) if edit_collection_form.validate_on_submit(): - collection.update(friendly_name=edit_collection_form.friendly_name.data, - category=edit_collection_form.category.data).save() + collection.update_as(current_user, + friendly_name=edit_collection_form.friendly_name.data, + category=edit_collection_form.category.data).save() flash(_('Thank you for editing collection "%(code)s".', code=collection.code), 'success') return redirect(url_for('collection.home')) else: diff --git a/xl_auth/commands.py b/xl_auth/commands.py index 41661f82..5afd0030 100644 --- a/xl_auth/commands.py +++ b/xl_auth/commands.py @@ -102,26 +102,28 @@ def clean(): @click.command() @click.option('-e', '--email', required=True, default=None, help='Email for user') @click.option('-n', '--full-name', default=None, help='Full name for user (default: None)') -@click.option('-p', '--password', default='password', - help='Password for user (default: "password")') -@click.option('--is_active', default=False, is_flag=True, help='Activate account (default: False)') +@click.option('-p', '--password', default=None, help='Password for user (default: None)') +@click.option('--admin-email', default='libris@kb.se', help='Related admin (default: None)') +@click.option('--is-active', default=False, is_flag=True, help='Activate account (default: False)') @click.option('--is-admin', default=False, is_flag=True, help='Create admin user (default: False)') @click.option('-f', '--force', default=False, is_flag=True, help='Force overwrite existing account (default: False)') @with_appcontext -def create_user(email, full_name, password, is_active, is_admin, force): +def create_user(email, full_name, password, admin_email, is_active, is_admin, force): """Create or overwrite user account.""" user = User.get_by_email(email) + op_admin = User.get_by_email(admin_email) if force and user: if full_name: user.full_name = full_name user.set_password(password) user.update(is_active=is_active, is_admin=is_admin) - user.save() + user.save_as(op_admin) click.echo('Overwritten account with login {0}:{1}'.format(user.email, password)) else: - user = User.create(email=email, full_name=full_name or email, password=password, - is_active=is_active, is_admin=is_admin) + user = User.create_as(op_admin, + email=email, full_name=full_name or email, password=password, + is_active=is_active, is_admin=is_admin) click.echo('Created account with login {0}:{1}'.format(user.email, password)) @@ -454,12 +456,17 @@ def _get_manually_deleted_permissions(): if collection: if collection.is_active != details['is_active']: collection.is_active = details['is_active'] - collection.save() + collection.save_as(admin) print('corrected collection %r: is_active=%s' % (collection.code, collection.is_active)) else: - collection = Collection.create(**details) - collection.save() + collection = Collection(**details) + if collection.created_at: + collection.modified_at = collection.created_at + collection.modified_by = collection.created_by = admin + collection.save(preserve_modified=True) + else: + collection.save_as(admin) # Store users. for email, full_name in deepcopy(bibdb['cataloging_admin_emails_to_names']).items(): @@ -486,11 +493,11 @@ def _get_manually_deleted_permissions(): with current_app.test_request_context(): password_reset = PasswordReset(user) password_reset.send_email() - user.save() + user.save_as(admin) password_reset.save() print('Added inactive user %r (password reset email sent).' % email) else: - user.save() + user.save_as(admin) print('Added inactive user %r (no password reset).' % email) old_permissions = Permission.query.all() @@ -513,15 +520,14 @@ def _get_manually_deleted_permissions(): current_permissions.append(permission) else: if user.email == 'test@kb.se': - permission = Permission.create(user=user, collection=collection, - registrant=collection.code == 'Utb1', - cataloger=collection.code == 'Utb2', - cataloging_admin=collection.code == 'Utb2') + permission = Permission.create_as(admin, user=user, collection=collection, + registrant=collection.code == 'Utb1', + cataloger=collection.code == 'Utb2', + cataloging_admin=collection.code == 'Utb2') else: - permission = Permission.create(user=user, collection=collection, - registrant=True, cataloger=True, - cataloging_admin=True) - permission.save() + permission = Permission.create_as(admin, user=user, collection=collection, + registrant=True, cataloger=True, + cataloging_admin=True) new_permissions.append(permission) # Apply manual additions. @@ -543,9 +549,9 @@ def _get_manually_deleted_permissions(): if verbose: print('Manual permission for %r on %r already exists.' % (email, code)) else: - permission = Permission.create(user=user, collection=collection, registrant=True, - cataloger=True, cataloging_admin=True) - permission.save() + permission = Permission.create_as(admin, user=user, collection=collection, + registrant=True, cataloger=True, + cataloging_admin=True) new_permissions.append(permission) if verbose: print('Manually added permissions for %r on %r.' % (email, code)) diff --git a/xl_auth/database.py b/xl_auth/database.py index 113878e8..78e7f651 100644 --- a/xl_auth/database.py +++ b/xl_auth/database.py @@ -23,22 +23,51 @@ class CRUDMixin(object): """Mixin that adds convenience methods for CRUD (create, read, update, delete) operations.""" + @classmethod + def create_as(cls, current_user, **kwargs): + """Create a new record and save it to the database as 'current_user'.""" + assert hasattr(cls, 'modified_by') and hasattr(cls, 'created_by') + instance = cls(**kwargs) + return instance.save_as(current_user) + @classmethod def create(cls, **kwargs): - """Create a new record and save it the database.""" + """Create a new record and save it to the database.""" instance = cls(**kwargs) return instance.save() - def update(self, commit=True, **kwargs): - """Update specific fields of a record.""" + def update_as(self, current_user, commit=True, preserve_modified=False, **kwargs): + """Update specific fields of the record and save as 'current_user'.""" for attr, value in kwargs.items(): setattr(self, attr, value) - return commit and self.save() or self + return self.save_as(current_user, commit=commit, preserve_modified=preserve_modified) - def save(self, commit=True): + def update(self, commit=True, preserve_modified=False, **kwargs): + """Update specific fields of a record.""" + for attr, value in kwargs.items(): + setattr(self, attr, value) + return self.save(commit=commit, preserve_modified=preserve_modified) + + def save_as(self, current_user, commit=True, preserve_modified=False): + """Save instance as 'current_user'.""" + assert hasattr(self, 'modified_by') and hasattr(self, 'created_by') + # noinspection PyUnresolvedReferences + if current_user and not self.created_at: + # noinspection PyAttributeOutsideInit + self.created_by = current_user + if current_user and not preserve_modified: + # noinspection PyAttributeOutsideInit + self.modified_by = current_user + return self.save(commit=commit, preserve_modified=preserve_modified) + + def save(self, commit=True, preserve_modified=False): """Save the record.""" db.session.add(self) if commit: + if preserve_modified and hasattr(self, 'modified_at'): + modified_dt = self.modified_at + db.session.commit() + self.modified_at = modified_dt db.session.commit() return self diff --git a/xl_auth/oauth/client/models.py b/xl_auth/oauth/client/models.py index 29031174..2f7e095c 100644 --- a/xl_auth/oauth/client/models.py +++ b/xl_auth/oauth/client/models.py @@ -3,10 +3,12 @@ from __future__ import absolute_import, division, print_function, unicode_literals +from datetime import datetime + from six import string_types from sqlalchemy.ext.hybrid import hybrid_property -from ...database import Column, Model, db +from ...database import Column, Model, db, reference_col, relationship class Client(Model): @@ -16,16 +18,23 @@ class Client(Model): client_id = Column(db.String(32), primary_key=True) client_secret = Column(db.String(256), unique=True, nullable=False) - created_by = Column(db.ForeignKey('users.id'), nullable=False) - is_confidential = Column(db.Boolean(), default=True, nullable=False) _redirect_uris = Column(db.Text(), nullable=False) _default_scopes = Column(db.Text(), nullable=False) # Human readable info fields - name = Column(db.String(64)) - description = Column(db.String(400)) + name = Column(db.String(64), nullable=False) + description = Column(db.String(400), nullable=False) + + modified_at = Column(db.DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=False) + modified_by_id = reference_col('users', nullable=False) + modified_by = relationship('User', foreign_keys=modified_by_id) + + created_at = Column(db.DateTime, default=datetime.utcnow, nullable=False) + created_by_id = reference_col('users', nullable=False) + created_by = relationship('User', foreign_keys=created_by_id) def __init__(self, redirect_uris=None, default_scopes=None, **kwargs): """Create instance.""" diff --git a/xl_auth/oauth/client/views.py b/xl_auth/oauth/client/views.py index e226a97a..a57d9ce5 100644 --- a/xl_auth/oauth/client/views.py +++ b/xl_auth/oauth/client/views.py @@ -36,12 +36,13 @@ def register(): register_form = RegisterForm(current_user, request.form) if register_form.validate_on_submit(): - Client.create(name=register_form.name.data, - description=register_form.description.data, - is_confidential=register_form.is_confidential.data, - redirect_uris=register_form.redirect_uris.data, - default_scopes=register_form.default_scopes.data, - created_by=current_user.id).save() + Client.create_as(current_user, + name=register_form.name.data, + description=register_form.description.data, + is_confidential=register_form.is_confidential.data, + redirect_uris=register_form.redirect_uris.data, + default_scopes=register_form.default_scopes.data, + created_by=current_user.id).save() flash(_('Client "%(name)s" created.', name=register_form.name.data), 'success') return redirect(url_for('oauth.client.home')) else: @@ -79,10 +80,11 @@ def edit(client_id): edit_form = EditForm(current_user, request.form) if edit_form.validate_on_submit(): - client.update(name=edit_form.name.data, description=edit_form.description.data, - is_confidential=edit_form.is_confidential.data, - redirect_uris=edit_form.redirect_uris.data, - default_scopes=edit_form.default_scopes.data).save() + client.update_as(current_user, + name=edit_form.name.data, description=edit_form.description.data, + is_confidential=edit_form.is_confidential.data, + redirect_uris=edit_form.redirect_uris.data, + default_scopes=edit_form.default_scopes.data).save() flash(_('Thank you for updating client details for "%(client_id)s".', client_id=client_id), 'success') return redirect(url_for('oauth.client.home')) diff --git a/xl_auth/permission/models.py b/xl_auth/permission/models.py index c913f28f..4c1f0925 100644 --- a/xl_auth/permission/models.py +++ b/xl_auth/permission/models.py @@ -3,7 +3,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import datetime as dt +from datetime import datetime from ..database import Column, Model, SurrogatePK, db, reference_col, relationship @@ -15,17 +15,22 @@ class Permission(SurrogatePK, Model): __tablename__ = 'permissions' user_id = reference_col('users', nullable=False) - user = relationship('User', back_populates='permissions', uselist=False) - + user = relationship('User', back_populates='permissions', foreign_keys=user_id) collection_id = reference_col('collections', nullable=False) - collection = relationship('Collection', back_populates='permissions', uselist=False) + collection = relationship('Collection', back_populates='permissions') registrant = Column(db.Boolean(), default=False, nullable=False) cataloger = Column(db.Boolean(), default=False, nullable=False) cataloging_admin = Column(db.Boolean(), default=False, nullable=False) - modified_at = Column(db.DateTime, default=dt.datetime.utcnow, onupdate=dt.datetime.utcnow) - created_at = Column(db.DateTime, nullable=False, default=dt.datetime.utcnow) + modified_at = Column(db.DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=False) + modified_by_id = reference_col('users', nullable=False) + modified_by = relationship('User', foreign_keys=modified_by_id) + + created_at = Column(db.DateTime, default=datetime.utcnow, nullable=False) + created_by_id = reference_col('users', nullable=False) + created_by = relationship('User', foreign_keys=created_by_id) def __init__(self, **kwargs): """Create instance.""" diff --git a/xl_auth/permission/views.py b/xl_auth/permission/views.py index d1cab820..df998cff 100644 --- a/xl_auth/permission/views.py +++ b/xl_auth/permission/views.py @@ -34,7 +34,8 @@ def register(): register_permission_form = RegisterForm(current_user, request.form) if register_permission_form.validate_on_submit(): - permission = Permission.create( + permission = Permission.create_as( + current_user, user_id=register_permission_form.user_id.data, collection_id=register_permission_form.collection_id.data, registrant=register_permission_form.registrant.data, @@ -64,7 +65,7 @@ def edit(permission_id): edit_permission_form = EditForm(current_user, permission_id, request.form) if edit_permission_form.validate_on_submit(): - permission.update(**edit_permission_form.data).save() + permission.update_as(current_user, **edit_permission_form.data).save() flash(_('Updated permissions for "%(username)s" on collection "%(code)s".', username=permission.user.email, code=permission.collection.code), 'success') return redirect(url_for('permission.home')) diff --git a/xl_auth/public/views.py b/xl_auth/public/views.py index 433ac62e..856c948d 100644 --- a/xl_auth/public/views.py +++ b/xl_auth/public/views.py @@ -79,7 +79,7 @@ def reset_password(email, code): password_reset.user.set_password(reset_password_form.password.data) if not password_reset.user.is_active: password_reset.user.is_active = True - password_reset.user.save() + password_reset.user.save_as(password_reset.user) flash(_('Password for "%(username)s" has been reset.', username=reset_password_form.username.data), 'success') return redirect(url_for('public.home')) diff --git a/xl_auth/user/models.py b/xl_auth/user/models.py index a44ef008..faa8a803 100644 --- a/xl_auth/user/models.py +++ b/xl_auth/user/models.py @@ -23,7 +23,7 @@ class Role(SurrogatePK, Model): __tablename__ = 'roles' name = Column(db.String(80), unique=True, nullable=False) user_id = reference_col('users', nullable=True) - user = relationship('User', back_populates='roles', uselist=False) + user = relationship('User', back_populates='roles') def __init__(self, name, **kwargs): """Create instance.""" @@ -39,7 +39,7 @@ class PasswordReset(SurrogatePK, Model): __tablename__ = 'password_resets' user_id = reference_col('users', nullable=True) - user = relationship('User', back_populates='password_resets', uselist=False) + user = relationship('User', back_populates='password_resets') code = Column(db.String(32), unique=True, nullable=False) is_active = Column(db.Boolean(), default=True, nullable=False) expires_at = Column(db.DateTime, nullable=False, @@ -109,18 +109,26 @@ class User(UserMixin, SurrogatePK, Model): """A user of the app.""" __tablename__ = 'users' + id = db.Column(db.Integer, primary_key=True) email = Column(db.String(255), unique=True, nullable=False) full_name = Column(db.String(255), unique=False, nullable=False) password = Column(db.Binary(128), nullable=False) last_login_at = Column(db.DateTime, default=None) is_active = Column(db.Boolean(), default=False, nullable=False) is_admin = Column(db.Boolean(), default=False, nullable=False) - permissions = relationship('Permission', back_populates='user') + permissions = relationship('Permission', back_populates='user', + foreign_keys='Permission.user_id') roles = relationship('Role', back_populates='user') password_resets = relationship('PasswordReset', back_populates='user') - modified_at = Column(db.DateTime, default=datetime.utcnow, onupdate=datetime.utcnow) + modified_at = Column(db.DateTime, default=datetime.utcnow, onupdate=datetime.utcnow, + nullable=False) + modified_by_id = reference_col('users', nullable=False) + modified_by = relationship('User', remote_side=id, foreign_keys=modified_by_id) + created_at = Column(db.DateTime, default=datetime.utcnow, nullable=False) + created_by_id = reference_col('users', nullable=False) + created_by = relationship('User', remote_side=id, foreign_keys=created_by_id) def __init__(self, email, full_name, password=None, **kwargs): """Create instance.""" @@ -147,13 +155,23 @@ def update_last_login(self, commit=True): """Set 'last_login_at' to current datetime.""" self.last_login_at = datetime.utcnow() if commit: - self.save() + self.save(commit=True, preserve_modified=True) def get_gravatar_url(self, size=32): """Get Gravatar URL.""" hashed_email = hashlib.md5(str(self.email).lower().encode()).hexdigest() return 'https://www.gravatar.com/avatar/{}?d=mm&s={}'.format(hashed_email, size) + def save_as(self, current_user, commit=True, preserve_modified=False): + """Save instance as 'current_user'.""" + if current_user and not self.created_at: + self.created_by = current_user + if current_user and not preserve_modified: + self.modified_by_id = current_user.id + # Using ``self.modified_by = current_user`` yields an error when user modifies itself: + # "sqlalchemy.exc.CircularDependencyError: Circular dependency detected." + return self.save(commit=commit, preserve_modified=preserve_modified) + def __repr__(self): """Represent instance as a unique string.""" return ''.format(email=self.email) diff --git a/xl_auth/user/views.py b/xl_auth/user/views.py index 55adb8e6..40f21f89 100644 --- a/xl_auth/user/views.py +++ b/xl_auth/user/views.py @@ -46,12 +46,12 @@ def register(): if register_user_form.send_password_reset_email.data: password_reset = PasswordReset(user) password_reset.send_email() - user.save() + user.save_as(current_user) password_reset.save() flash(_('User "%(username)s" registered and emailed with a password reset link.', username=user.email), 'success') else: - user.save() + user.save_as(current_user) flash(_('User "%(username)s" registered.', username=user.email), 'success') return redirect(url_for('user.home')) else: @@ -73,9 +73,10 @@ def administer(user_id): administer_form = AdministerForm(current_user, user.email, request.form) if administer_form.validate_on_submit(): - user.update(full_name=administer_form.full_name.data, - is_active=administer_form.is_active.data, - is_admin=administer_form.is_admin.data).save() + user.update_as(current_user, + full_name=administer_form.full_name.data, + is_active=administer_form.is_active.data, + is_admin=administer_form.is_admin.data).save() flash(_('Thank you for updating user details for "%(username)s".', username=user.email), 'success') return redirect(url_for('user.home')) @@ -99,7 +100,7 @@ def edit_details(user_id): edit_details_form = EditDetailsForm(current_user, user.email, request.form) if edit_details_form.validate_on_submit(): - user.update(full_name=edit_details_form.full_name.data).save() + user.update_as(current_user, full_name=edit_details_form.full_name.data).save() flash(_('Thank you for updating user details for "%(username)s".', username=user.email), 'success') if current_user.is_admin: @@ -128,7 +129,7 @@ def change_password(user_id): change_password_form = ChangePasswordForm(current_user, user.email, request.form) if change_password_form.validate_on_submit(): user.set_password(change_password_form.password.data) - user.save() + user.save_as(current_user) flash(_('Thank you for changing password for "%(username)s".', username=user.email), 'success') if current_user.is_admin: