From 91c78ec8ed63917f85c7d3695e034589b8e13a4f Mon Sep 17 00:00:00 2001 From: Martin Lettry Date: Thu, 26 Jan 2023 15:41:14 +0100 Subject: [PATCH] datastore: add update_role func * closes https://github.com/inveniosoftware/invenio-app-rdm/issues/2186 * updated cli to pass ids on create role Co-authored-by: jrcastro2 --- ...ange_accountsrole_primary_key_to_string.py | 102 ++++++++++++++++++ invenio_accounts/api.py | 57 +++++++--- invenio_accounts/cli.py | 4 +- invenio_accounts/datastore.py | 24 ++++- invenio_accounts/models.py | 10 +- run-tests.sh | 28 ++++- setup.cfg | 2 +- tests/test_invenio_accounts.py | 98 ++++++++++------- 8 files changed, 256 insertions(+), 69 deletions(-) create mode 100644 invenio_accounts/alembic/f2522cdd5fcd_change_accountsrole_primary_key_to_string.py diff --git a/invenio_accounts/alembic/f2522cdd5fcd_change_accountsrole_primary_key_to_string.py b/invenio_accounts/alembic/f2522cdd5fcd_change_accountsrole_primary_key_to_string.py new file mode 100644 index 00000000..2d21b3b8 --- /dev/null +++ b/invenio_accounts/alembic/f2522cdd5fcd_change_accountsrole_primary_key_to_string.py @@ -0,0 +1,102 @@ +# +# This file is part of Invenio. +# Copyright (C) 2023 CERN. +# +# Invenio is free software; you can redistribute it and/or modify it +# under the terms of the MIT License; see LICENSE file for more details. + +"""Change AccountsRole primary key to string.""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "f2522cdd5fcd" +down_revision = "eb9743315a9d" +branch_labels = () +depends_on = None + + +def upgrade(): + """Upgrade database.""" + # Drop primary key and all foreign keys + op.execute("ALTER TABLE accounts_role DROP CONSTRAINT pk_accounts_role CASCADE") + + op.alter_column( + "accounts_userrole", + "role_id", + existing_type=sa.Integer, + type_=sa.String(80), + postgresql_using="role_id::integer", + ) + # Change primary key type + # server_default=None will remove the autoincrement + op.alter_column( + "accounts_role", + "id", + existing_type=sa.Integer, + type_=sa.String(80), + server_default=None, + ) + op.create_primary_key("pk_accounts_role", "accounts_role", ["id"]) + # Add new column `is_managed` + op.add_column( + "accounts_role", + sa.Column( + "is_managed", sa.Boolean(name="is_managed"), default=True, nullable=True + ), + ) + op.execute("UPDATE accounts_role SET is_managed = true") + op.alter_column("accounts_role", "is_managed", nullable=False) + + # Re-create the foreign key constraint + op.create_foreign_key( + "fk_accounts_userrole_role_id", + "accounts_userrole", + "accounts_role", + ["role_id"], + ["id"], + ) + + +def downgrade(): + """Downgrade database.""" + # Drop new column `is_managed` + op.drop_column("accounts_role", "is_managed") + op.execute("ALTER TABLE accounts_role DROP CONSTRAINT pk_accounts_role CASCADE") + + op.alter_column( + "accounts_userrole", + "role_id", + existing_type=sa.String(80), + type_=sa.Integer, + postgresql_using="role_id::integer", + ) + # Change primary key type + # op.drop_constraint("pk_accounts_role", "accounts_role", type_="primary") + op.alter_column( + "accounts_role", + "id", + existing_type=sa.String(80), + type_=sa.Integer, + postgresql_using="id::integer", + ) + op.create_primary_key("pk_accounts_role", "accounts_role", ["id"]) + op.alter_column( + "accounts_role", + "id", + existing_type=sa.String(80), + type_=sa.Integer, + autoincrement=True, + existing_autoincrement=True, + nullable=False, + ) + + # Re-create the foreign key constraint + op.create_foreign_key( + "fk_accounts_userrole_role_id", + "accounts_userrole", + "accounts_role", + ["role_id"], + ["id"], + ) diff --git a/invenio_accounts/api.py b/invenio_accounts/api.py index f0e67fd8..4e84f90c 100644 --- a/invenio_accounts/api.py +++ b/invenio_accounts/api.py @@ -1,32 +1,57 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2022 CERN. +# Copyright (C) 2022-2023 CERN. # # Invenio is free software; you can redistribute it and/or modify it # under the terms of the MIT License; see LICENSE file for more details. """API objects for Invenio Accounts.""" -from collections import defaultdict + +class Session: + """Session object for DB Users change history.""" + + def __init__(self): + """Constructor.""" + self.updated_users = set() + self.updated_roles = set() + self.deleted_users = set() + self.deleted_roles = set() class DBUsersChangeHistory: """DB Users change history storage.""" def __init__(self): - """constructor.""" - # the keys are going to be the sessions, the values are going to be - # the sets of dirty/deleted models - self.updated_users = defaultdict(lambda: list()) - self.updated_roles = defaultdict(lambda: list()) - self.deleted_users = defaultdict(lambda: list()) - self.deleted_roles = defaultdict(lambda: list()) - - def _clear_dirty_sets(self, session): - """Clear the dirty sets for the given session.""" + """Constructor.""" + self.sessions = {} + + def _get_session(self, session_id): + """Returns or creates a session for a concrete session id.""" + return self.sessions.setdefault(session_id, Session()) + + def add_updated_user(self, session_id, user_id): + """Adds a user to the updated users list.""" + session = self._get_session(session_id) + session.updated_users.add(user_id) + + def add_updated_role(self, session_id, role_id): + """Adds a role to the updated roles list.""" + session = self._get_session(session_id) + session.updated_roles.add(role_id) + + def add_deleted_user(self, session_id, user_id): + """Adds a user to the deleted users list.""" + session = self._get_session(session_id) + session.deleted_users.add(user_id) + + def add_deleted_role(self, session_id, role_id): + """Adds a role to the deleted roles list.""" + session = self._get_session(session_id) + session.deleted_roles.add(role_id) + + def clear_dirty_sets(self, session): + """Removes session object.""" sid = id(session) - self.updated_users.pop(sid, None) - self.updated_roles.pop(sid, None) - self.deleted_users.pop(sid, None) - self.deleted_roles.pop(sid, None) + self.sessions.pop(sid, None) diff --git a/invenio_accounts/cli.py b/invenio_accounts/cli.py index b511a199..05f3c1fd 100644 --- a/invenio_accounts/cli.py +++ b/invenio_accounts/cli.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2015-2018 CERN. +# Copyright (C) 2015-2023 CERN. # # Invenio is free software; you can redistribute it and/or modify it # under the terms of the MIT License; see LICENSE file for more details. @@ -80,7 +80,7 @@ def users_create(email, password, active, confirm, profile): @commit def roles_create(**kwargs): """Create a role.""" - _datastore.create_role(**kwargs) + _datastore.create_role(id=kwargs["name"], **kwargs) click.secho('Role "%(name)s" created successfully.' % kwargs, fg="green") diff --git a/invenio_accounts/datastore.py b/invenio_accounts/datastore.py index 79037fb9..c327d25b 100644 --- a/invenio_accounts/datastore.py +++ b/invenio_accounts/datastore.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2015-2022 CERN. +# Copyright (C) 2015-2023 CERN. # # Invenio is free software; you can redistribute it and/or modify it # under the terms of the MIT License; see LICENSE file for more details. @@ -10,6 +10,7 @@ from flask_security import SQLAlchemyUserDatastore +from .models import Role from .proxies import current_db_change_history from .sessions import delete_user_sessions from .signals import datastore_post_commit, datastore_pre_commit @@ -34,10 +35,27 @@ def commit(self): datastore_pre_commit.send(session=self.db.session) super().commit() datastore_post_commit.send(session=self.db.session) + current_db_change_history.clear_dirty_sets(self.db.session) def mark_changed(self, sid, uid=None, rid=None): """Save a user to the changed history.""" if uid: - current_db_change_history.updated_users[sid].append(uid) + current_db_change_history.add_updated_user(sid, uid) elif rid: - current_db_change_history.updated_roles[sid].append(uid) + current_db_change_history.add_updated_role(sid, rid) + + def update_role(self, role): + """Updates roles.""" + role = self.db.session.merge(role) + self.mark_changed(id(self.db.session), rid=role.id) + return role + + def create_role(self, **kwargs): + """Creates and returns a new role from the given parameters.""" + role = super().create_role(**kwargs) + self.mark_changed(id(self.db.session), rid=role.id) + return role + + def find_role_by_id(self, role_id): + """Fetches roles searching by id.""" + return self.role_model.query.filter_by(id=role_id).one_or_none() diff --git a/invenio_accounts/models.py b/invenio_accounts/models.py index c48e32ab..74d4de30 100644 --- a/invenio_accounts/models.py +++ b/invenio_accounts/models.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2015-2022 CERN. +# Copyright (C) 2015-2023 CERN. # Copyright (C) 2022 KTH Royal Institute of Technology # # Invenio is free software; you can redistribute it and/or modify it @@ -9,6 +9,7 @@ """Database models for accounts.""" +import uuid from datetime import datetime from flask import current_app, session @@ -52,7 +53,7 @@ ), db.Column( "role_id", - db.Integer(), + db.String(80), db.ForeignKey("accounts_role.id", name="fk_accounts_userrole_role_id"), ), ) @@ -64,7 +65,7 @@ class Role(db.Model, Timestamp, RoleMixin): __tablename__ = "accounts_role" - id = db.Column(db.Integer(), primary_key=True) + id = db.Column(db.String(80), primary_key=True, default=str(uuid.uuid4())) name = db.Column(db.String(80), unique=True) """Role name.""" @@ -72,6 +73,9 @@ class Role(db.Model, Timestamp, RoleMixin): description = db.Column(db.String(255)) """Role description.""" + is_managed = db.Column(db.Boolean(), default=True, nullable=False) + """True when the role is managed by Invenio, and not externally provided.""" + # Enables SQLAlchemy version counter version_id = db.Column(db.Integer, nullable=False) """Used by SQLAlchemy for optimistic concurrency control.""" diff --git a/run-tests.sh b/run-tests.sh index af5939eb..4274a184 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -18,14 +18,36 @@ set -o nounset function cleanup() { eval "$(docker-services-cli down --env)" } -trap cleanup EXIT +# Check for arguments +# Note: "-k" would clash with "pytest" +keep_services=0 +pytest_args=() +for arg in $@; do + # from the CLI args, filter out some known values and forward the rest to "pytest" + # note: we don't use "getopts" here b/c of some limitations (e.g. long options), + # which means that we can't combine short options (e.g. "./run-tests -Kk pattern") + case ${arg} in + -K|--keep-services) + keep_services=1 + ;; + *) + pytest_args+=( ${arg} ) + ;; + esac +done + +if [[ ${keep_services} -eq 0 ]]; then + trap cleanup EXIT +fi python -m check_manifest python -m setup extract_messages --output-file /dev/null python -m sphinx.cmd.build -qnN docs docs/_build/html eval "$(docker-services-cli up --db ${DB:-postgresql} --cache ${CACHE:-redis} --env)" -python -m pytest -tests_exit_code=$? +# Note: expansion of pytest_args looks like below to not cause an unbound +# variable error when 1) "nounset" and 2) the array is empty. +python -m pytest ${pytest_args[@]+"${pytest_args[@]}"} python -m sphinx.cmd.build -qnN -b doctest docs docs/_build/doctest +tests_exit_code=$? exit "$tests_exit_code" diff --git a/setup.cfg b/setup.cfg index 50b6afe8..8fa56e8e 100644 --- a/setup.cfg +++ b/setup.cfg @@ -54,7 +54,7 @@ tests = mock>=1.3.0 invenio-app>=1.3.3 pytest-black>=0.3.0 - pytest-invenio>=1.4.6 + pytest-invenio>=2.1.4 sphinx>=4.2.0,<5 [options.entry_points] diff --git a/tests/test_invenio_accounts.py b/tests/test_invenio_accounts.py index 8805bd16..b5c24d48 100644 --- a/tests/test_invenio_accounts.py +++ b/tests/test_invenio_accounts.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2015-2018 CERN. +# Copyright (C) 2015-2023 CERN. # Copyright (C) 2021 TU Wien. # # Invenio is free software; you can redistribute it and/or modify it @@ -114,48 +114,65 @@ def test_datastore_usercreate(app): """Test create user.""" ds = app.extensions["invenio-accounts"].datastore - with app.app_context(): - u1 = ds.create_user( - email="info@inveniosoftware.org", password="1234", active=True - ) - ds.commit() - u2 = ds.find_user(email="info@inveniosoftware.org") - assert u1 == u2 - assert 1 == User.query.filter_by(email="info@inveniosoftware.org").count() + u1 = ds.create_user(email="info@inveniosoftware.org", password="1234", active=True) + ds.commit() + u2 = ds.find_user(email="info@inveniosoftware.org") + assert u1 == u2 + assert 1 == User.query.filter_by(email="info@inveniosoftware.org").count() def test_datastore_rolecreate(app): - """Test create user.""" + """Test create role.""" + ds = app.extensions["invenio-accounts"].datastore + + r1 = ds.create_role(name="superuser", description="1234") + ds.commit() + r2 = ds.find_role("superuser") + assert r1 == r2 + assert 1 == Role.query.filter_by(name="superuser").count() + + +def test_datastore_update_role(app): + """Test update role.""" ds = app.extensions["invenio-accounts"].datastore - with app.app_context(): - r1 = ds.create_role(name="superuser", description="1234") - ds.commit() - r2 = ds.find_role("superuser") - assert r1 == r2 - assert 1 == Role.query.filter_by(name="superuser").count() + r1 = ds.create_role(id="1", name="superuser", description="1234") + ds.commit() + r2 = ds.find_role("superuser") + assert r1 == r2 + assert 1 == Role.query.filter_by(name="superuser").count() + assert r2.is_managed is True + + r1 = ds.update_role( + Role( + id="1", name="megauser", description="updated description", is_managed=False + ) + ) + ds.commit() + r2 = ds.find_role("megauser") + assert r1 == r2 + assert r2.description == "updated description" + assert r2.is_managed is False + assert 1 == Role.query.filter_by(name="megauser").count() + assert 0 == Role.query.filter_by(name="superuser").count() def test_datastore_assignrole(app): """Create and assign user to role.""" ds = app.extensions["invenio-accounts"].datastore - with app.app_context(): - u = ds.create_user( - email="info@inveniosoftware.org", password="1234", active=True - ) - r = ds.create_role(name="superuser", description="1234") - ds.add_role_to_user(u, r) - ds.commit() - u = ds.get_user("info@inveniosoftware.org") - assert len(u.roles) == 1 - assert u.roles[0].name == "superuser" + u = ds.create_user(email="info@inveniosoftware.org", password="1234", active=True) + r = ds.create_role(name="superuser", description="1234") + ds.add_role_to_user(u, r) + ds.commit() + u = ds.get_user("info@inveniosoftware.org") + assert len(u.roles) == 1 + assert u.roles[0].name == "superuser" def test_view(app): """Test view.""" - with app.app_context(): - login_url = url_for_security("login") + login_url = url_for_security("login") with app.test_client() as client: res = client.get(login_url) @@ -208,16 +225,15 @@ def test_headers_info(app, users): """Test if session and user id is set response header.""" u = users[0] url = url_for_security("change_password") - with app.app_context(): - with app.test_client() as client: - response = client.get(url) - # Not logged in, so only session id available - assert not testutils.client_authenticated(client) - assert "X-Session-ID" in response.headers - assert "X-User-ID" not in response.headers - # Login - testutils.login_user_via_session(client, email=u["email"]) - response = client.get(url) - cookie = requests.utils.dict_from_cookiejar(client.cookie_jar) - assert response.headers["X-Session-ID"] == cookie["session"].split(".")[0] - assert int(response.headers["X-User-ID"]) == u["id"] + with app.test_client() as client: + response = client.get(url) + # Not logged in, so only session id available + assert not testutils.client_authenticated(client) + assert "X-Session-ID" in response.headers + assert "X-User-ID" not in response.headers + # Login + testutils.login_user_via_session(client, email=u["email"]) + response = client.get(url) + cookie = requests.utils.dict_from_cookiejar(client.cookie_jar) + assert response.headers["X-Session-ID"] == cookie["session"].split(".")[0] + assert int(response.headers["X-User-ID"]) == u["id"]