diff --git a/.github/workflows/python-ci.yaml b/.github/workflows/python-ci.yaml deleted file mode 100644 index a6f4c699..00000000 --- a/.github/workflows/python-ci.yaml +++ /dev/null @@ -1,5 +0,0 @@ -name: Python CI - -on: [push] - -jobs: diff --git a/.gitignore b/.gitignore index 8a07be70..c4e3eff4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,9 @@ +.idea +megaqc/static/js/admin* +megaqc/static/js/trend* +venv/ +*.db + # Uploads uploads/* !uploads/README.md diff --git a/docs/changelog.md b/docs/changelog.md new file mode 100644 index 00000000..5151e2ba --- /dev/null +++ b/docs/changelog.md @@ -0,0 +1,25 @@ +# Changelog + +## 0.3.0 + +### Breaking Changes + +- [[#138]](https://github.com/ewels/MegaQC/issues/138) Added `USER_REGISTRATION_APPROVAL` as a config variable, which defaults to true. This means that the admin must explicitly activate new users in the user management page (`/users/admin/users`) before they can login. To disable this feature, you need to create a config file (for example `megaqc.conf.yaml`) with the contents: + ```yaml + STRICT_REGISTRATION: false + ``` + Then, whenever you run MegaQC, you need to `export MEGAQC_CONFIG /path/to/megaqc.conf.yaml +- Much stricter REST API permissions. You now need an API token for almost all requests. One exception is creating a new account, which you can do without a token, but it will be deactivated by default, unless it is the first account created + +### New Features + +- [[#140]](https://github.com/ewels/MegaQC/issues/140) Added a changelog. It's here! You're reading it! + +### Bug Fixes + +- [[#139]](https://github.com/ewels/MegaQC/issues/139) Fixed the user management page (`/users/admin/users`), which lost its JavaScript + +### Internal Changes + +- Tests for the REST API permissions +- Enforce inactive users (by default) in the model layer diff --git a/megaqc/api/views.py b/megaqc/api/views.py index 293c0e55..87212f94 100644 --- a/megaqc/api/views.py +++ b/megaqc/api/views.py @@ -186,6 +186,7 @@ def admin_add_users(user, *args, **kwargs): except: abort(400) new_user = User(**data) + new_user.enforce_admin() password = new_user.reset_password() new_user.active = True new_user.save() diff --git a/megaqc/public/views.py b/megaqc/public/views.py index 1fbacaf0..0856e6b4 100644 --- a/megaqc/public/views.py +++ b/megaqc/public/views.py @@ -10,6 +10,7 @@ Blueprint, Request, abort, + current_app, flash, json, redirect, @@ -108,18 +109,23 @@ def register(): """ form = RegisterForm(request.form) if form.validate_on_submit(): - user_cnt = db.session.query(User).count() - u = User.create( + u = User( username=form.username.data, email=form.email.data, password=form.password.data, first_name=form.first_name.data, last_name=form.last_name.data, - active=True, - is_admin=True if user_cnt == 0 else False, ) - flash("Thanks for registering! You're now logged in.", "success") - login_user(u) + u.enforce_admin() + u.save() + if u.active: + flash("Thanks for registering! You're now logged in.", "success") + login_user(u) + else: + flash( + "Thanks for registering! You will now need to wait for your admin to approve this account.", + "success", + ) return redirect(url_for("public.home")) else: flash_errors(form) diff --git a/megaqc/rest_api/schemas.py b/megaqc/rest_api/schemas.py index 724bf762..b21a9d2c 100644 --- a/megaqc/rest_api/schemas.py +++ b/megaqc/rest_api/schemas.py @@ -363,7 +363,7 @@ class Meta: email = f.String() salt = f.String() password = f.String() - created_at = f.DateTime() + created_at = f.DateTime(dump_only=True) first_name = f.String() last_name = f.String() active = f.Boolean() diff --git a/megaqc/rest_api/utils.py b/megaqc/rest_api/utils.py index 6d045fe5..68bba0eb 100644 --- a/megaqc/rest_api/utils.py +++ b/megaqc/rest_api/utils.py @@ -3,7 +3,8 @@ from functools import wraps from uuid import uuid4 -from flask import request +from flapison.exceptions import JsonApiException +from flask import abort, request from flask.globals import current_app from megaqc.user.models import User @@ -26,39 +27,56 @@ def get_unique_filename(): class Permission(IntEnum): - VIEWER = auto() + NONUSER = auto() USER = auto() ADMIN = auto() -def check_perms(function): +def api_perms(min_level: Permission = Permission.NONUSER): """ - Adds a "user" and "permission" kwarg to the view function. + Adds a "user" and "permission" kwarg to the view function. Also verifies a + minimum permissions level. - :param function: - :return: + :param min_level: If provided, this is the minimum permission level required by this endpoint """ - @wraps(function) - def user_wrap_function(*args, **kwargs): - if not request.headers.has_key("access_token"): - perms = Permission.VIEWER - user = None - else: - user = User.query.filter_by( - api_token=request.headers.get("access_token") - ).first() - if not user: - perms = Permission.VIEWER - elif user.is_anonymous: - perms = Permission.VIEWER - elif user.is_admin: - perms = Permission.ADMIN + def wrapper(function): + @wraps(function) + def user_wrap_function(*args, **kwargs): + extra = None + if not request.headers.has_key("access_token"): + perms = Permission.NONUSER + user = None + extra = "No access token provided. Please add a header with the name 'access_token'." else: - perms = Permission.USER + user = User.query.filter_by( + api_token=request.headers.get("access_token") + ).first() + if not user: + perms = Permission.NONUSER + extra = "The provided access token was invalid." + elif user.is_anonymous: + perms = Permission.NONUSER + elif user.is_admin: + perms = Permission.ADMIN + elif not user.is_active(): + perms = Permission.NONUSER + extra = "User is not active." + else: + perms = Permission.USER - kwargs["user"] = user - kwargs["permission"] = perms - return function(*args, **kwargs) + if perms < min_level: + title = "Insufficient permissions to access this resource" + raise JsonApiException( + title=title, + detail=extra, + status=403, + ) - return user_wrap_function + kwargs["user"] = user + kwargs["permission"] = perms + return function(*args, **kwargs) + + return user_wrap_function + + return wrapper diff --git a/megaqc/rest_api/views.py b/megaqc/rest_api/views.py index 7536225a..37760d11 100644 --- a/megaqc/rest_api/views.py +++ b/megaqc/rest_api/views.py @@ -7,29 +7,57 @@ from http import HTTPStatus from flapison import ResourceDetail, ResourceList, ResourceRelationship -from flask import Blueprint, jsonify, make_response, request -from flask_login import current_user +from flapison.schema import get_nested_fields, get_relationships +from flask import Blueprint, current_app, jsonify, make_response, request +from flask_login import current_user, login_required from marshmallow.utils import EXCLUDE, INCLUDE from marshmallow_jsonapi.exceptions import IncorrectTypeError import megaqc.user.models as user_models -from megaqc.api.views import check_user from megaqc.extensions import db, json_api, restful from megaqc.model import models from megaqc.rest_api import plot, schemas, utils from megaqc.rest_api.content import json_to_csv +from megaqc.rest_api.utils import Permission, api_perms from megaqc.rest_api.webarg_parser import use_args, use_kwargs api_bp = Blueprint("rest_api", __name__, url_prefix="/rest_api/v1") json_api.blueprint = api_bp -class Upload(ResourceDetail): +class PermissionsMixin: + """ + Adds shared config to all views. + + Logged-out users shouldn't be able to access the API at all, logged + in users should be able to only GET, and only admins should be able + to POST, PATCH or DELETE. These decorators can be overriden by child + classes, however + """ + + @api_perms(Permission.USER) + def get(self, **kwargs): + return super().get(**kwargs) + + @api_perms(Permission.ADMIN) + def post(self, **kwargs): + return super().post(**kwargs) + + @api_perms(Permission.ADMIN) + def patch(self, **kwargs): + return super().patch(**kwargs) + + @api_perms(Permission.ADMIN) + def delete(self, **kwargs): + return super().delete(**kwargs) + + +class Upload(PermissionsMixin, ResourceDetail): schema = schemas.UploadSchema data_layer = dict(session=db.session, model=models.Upload) -class UploadList(ResourceList): +class UploadList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.UploadSchema data_layer = dict(session=db.session, model=models.Upload) @@ -42,10 +70,13 @@ def get_schema_kwargs(self, args, kwargs): return {} - @check_user + @api_perms(Permission.USER) def post(self, **kwargs): """ Upload a new report. + + This is rare in that average users *can* do this, even though + they aren't allowed to edit arbitrary data """ # This doesn't exactly follow the JSON API spec, since it doesn't exactly support file uploads: # https://github.com/json-api/json-api/issues/246 @@ -61,61 +92,61 @@ def post(self, **kwargs): return schemas.UploadSchema(many=False).dump(upload_row), HTTPStatus.CREATED -class UploadRelationship(ResourceRelationship): +class UploadRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.UploadSchema data_layer = dict(session=db.session, model=models.Upload) -class ReportList(ResourceList): +class ReportList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.ReportSchema data_layer = dict(session=db.session, model=models.Report) -class Report(ResourceDetail): +class Report(PermissionsMixin, ResourceDetail): schema = schemas.ReportSchema data_layer = dict(session=db.session, model=models.Report) -class ReportRelationship(ResourceRelationship): +class ReportRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.ReportSchema data_layer = dict(session=db.session, model=models.Report) -class ReportMeta(ResourceDetail): +class ReportMeta(PermissionsMixin, ResourceDetail): view_kwargs = True schema = schemas.ReportMetaSchema data_layer = dict(session=db.session, model=models.ReportMeta) -class ReportMetaList(ResourceList): +class ReportMetaList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.ReportMetaSchema data_layer = dict(session=db.session, model=models.ReportMeta) -class ReportMetaRelationship(ResourceRelationship): +class ReportMetaRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.ReportMetaSchema data_layer = dict(session=db.session, model=models.ReportMeta) -class Sample(ResourceDetail): +class Sample(PermissionsMixin, ResourceDetail): schema = schemas.SampleSchema data_layer = dict(session=db.session, model=models.Sample) -class SampleList(ResourceList): +class SampleList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.SampleSchema data_layer = dict(session=db.session, model=models.Sample) -class SampleRelationship(ResourceRelationship): +class SampleRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.SampleSchema data_layer = dict(session=db.session, model=models.Sample) -class ReportMetaTypeList(ResourceList): +class ReportMetaTypeList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.ReportMetaTypeSchema data_layer = dict(session=db.session, model=models.ReportMeta) @@ -133,40 +164,40 @@ def get_collection(self, qs, kwargs, filters=None): return query.count(), query.all() -class SampleData(ResourceDetail): +class SampleData(PermissionsMixin, ResourceDetail): view_kwargs = True schema = schemas.SampleDataSchema data_layer = dict(session=db.session, model=models.SampleData) -class SampleDataList(ResourceList): +class SampleDataList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.SampleDataSchema data_layer = dict(session=db.session, model=models.SampleData) -class SampleDataRelationship(ResourceRelationship): +class SampleDataRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.SampleDataSchema data_layer = dict(session=db.session, model=models.SampleData) -class DataType(ResourceDetail): +class DataType(PermissionsMixin, ResourceDetail): schema = schemas.SampleDataTypeSchema data_layer = dict(session=db.session, model=models.SampleDataType) -class DataTypeList(ResourceList): +class DataTypeList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.SampleDataTypeSchema data_layer = dict(session=db.session, model=models.SampleDataType) -class User(ResourceDetail): +class User(PermissionsMixin, ResourceDetail): schema = schemas.UserSchema data_layer = dict(session=db.session, model=user_models.User) -class UserRelationship(ResourceRelationship): +class UserRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.UserSchema data_layer = dict(session=db.session, model=user_models.User) @@ -176,6 +207,22 @@ class UserList(ResourceList): schema = schemas.UserSchema data_layer = dict(session=db.session, model=user_models.User) + @api_perms(Permission.USER) + def get(self, **kwargs): + return super().get(**kwargs) + + # We allow this endpoint to be hit by a non user, to allow the first user to be created + @api_perms(Permission.NONUSER) + def post(self, **kwargs): + return super().post(**kwargs) + + def post_schema_kwargs(self, args, kwargs): + # None of these fields should be set directly by a user + if kwargs["permission"] < utils.Permission.ADMIN: + return {"dump_only": ["admin", "active", "salt", "api_token"]} + else: + return {} + def get_schema_kwargs(self, args, kwargs): # Only show the filepath if they're an admin if "user" in kwargs and kwargs["permission"] <= utils.Permission.ADMIN: @@ -184,19 +231,27 @@ def get_schema_kwargs(self, args, kwargs): return {} def create_object(self, data, kwargs): + # This is mostly copied from flapison.data_layers.alchemy + relationship_fields = get_relationships(self.schema, model_field=True) + nested_fields = get_nested_fields(self.schema, model_field=True) + join_fields = relationship_fields + nested_fields + new_user = self.data_layer["model"]( + **{key: value for (key, value) in data.items() if key not in join_fields} + ) + self._data_layer.apply_relationships(data, new_user) + self._data_layer.apply_nested_fields(data, new_user) # Creating a user requires generating a password - new_user = super().create_object(data, kwargs) + new_user.enforce_admin() new_user.set_password(data["password"]) - new_user.active = True new_user.save() return new_user -class CurrentUser(ResourceDetail): +class CurrentUser(PermissionsMixin, ResourceDetail): schema = schemas.UserSchema data_layer = dict(session=db.session, model=user_models.User) - @utils.check_perms + @login_required def get(self, **kwargs): """ Get details about the current user. @@ -216,7 +271,7 @@ def get(self, **kwargs): .first_or_404() ) - if kwargs["permission"] >= utils.Permission.ADMIN: + if current_user.is_admin: # If an admin is making this request, give them everything schema_kwargs = {} else: @@ -226,23 +281,23 @@ def get(self, **kwargs): return schemas.UserSchema(many=False, **schema_kwargs).dump(user) -class FilterList(ResourceList): +class FilterList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.SampleFilterSchema data_layer = dict(session=db.session, model=models.SampleFilter) -class Filter(ResourceDetail): +class Filter(PermissionsMixin, ResourceDetail): schema = schemas.SampleFilterSchema data_layer = dict(session=db.session, model=models.SampleFilter) -class FilterRelationship(ResourceRelationship): +class FilterRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.SampleFilterSchema data_layer = dict(session=db.session, model=models.SampleFilter) -class FilterGroupList(ResourceList): +class FilterGroupList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.FilterGroupSchema data_layer = dict(session=db.session, model=models.SampleFilter) @@ -257,40 +312,40 @@ def get_collection(self, qs, kwargs, filters=None): return query.count(), query.all() -class FavouritePlotList(ResourceList): +class FavouritePlotList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.FavouritePlotSchema data_layer = dict(session=db.session, model=models.PlotFavourite) -class FavouritePlot(ResourceDetail): +class FavouritePlot(PermissionsMixin, ResourceDetail): schema = schemas.FavouritePlotSchema data_layer = dict(session=db.session, model=models.PlotFavourite) -class FavouritePlotRelationship(ResourceRelationship): +class FavouritePlotRelationship(PermissionsMixin, ResourceRelationship): schema = schemas.FavouritePlotSchema data_layer = dict(session=db.session, model=models.PlotFavourite) -class DashboardList(ResourceList): +class DashboardList(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.DashboardSchema data_layer = dict(session=db.session, model=models.Dashboard) -class DashboardRelationship(ResourceList): +class DashboardRelationship(PermissionsMixin, ResourceList): view_kwargs = True schema = schemas.DashboardSchema data_layer = dict(session=db.session, model=models.Dashboard) -class Dashboard(ResourceDetail): +class Dashboard(PermissionsMixin, ResourceDetail): schema = schemas.DashboardSchema data_layer = dict(session=db.session, model=models.Dashboard) -class TrendSeries(ResourceList): +class TrendSeries(PermissionsMixin, ResourceList): @use_args(schemas.TrendInputSchema(), locations=("querystring",)) def get(self, args): # We need to give each resource a unique ID so the client doesn't try to cache diff --git a/megaqc/settings.py b/megaqc/settings.py index 494007ec..e6a62b8f 100644 --- a/megaqc/settings.py +++ b/megaqc/settings.py @@ -40,6 +40,8 @@ class Config(object): SQLALCHEMY_USER = "megaqc_user" SQLALCHEMY_PASS = "" SQLALCHEMY_DATABASE = "megaqc" + # If this is true, every user after the first has to be approved before it becomes active + USER_REGISTRATION_APPROVAL = True def __init__(self): if self.EXTRA_CONFIG: diff --git a/megaqc/static/js/user_admin.js b/megaqc/static/js/user_admin.js new file mode 100644 index 00000000..f5b8ca35 --- /dev/null +++ b/megaqc/static/js/user_admin.js @@ -0,0 +1,158 @@ + +$(function(){ + init_buttons(); +}); + + +function init_buttons(){ + $(".update_btn").click(function(e){ + $("#my_alert").remove() + var data = {'csrf_token':$("#csrf_token").val()}; + e.preventDefault(); + $(this).parentsUntil('tbody').find('input').each(function(idx, el){ + if (el.type != 'button'){ + if (el.type=="checkbox"){ + data[el.name] = $(el).prop("checked"); + } else { + data[el.name] = el.value; + } + } + }); + $.ajax({ + url:"/api/update_users", + type: 'post', + data:JSON.stringify(data), + headers : {access_token:window.token}, + dataType: 'json', + contentType:"application/json; charset=UTF-8", + success: function(){ + toastr.success('User updated successfully'); + }, + error: function(ret_data){ + toastr.error('Error: '+ret_data.responseJSON.message); + } + }); + }); + $(".delete_btn").click(function(e){ + $("#my_alert").remove() + var my_btn = $(this); + var data = {}; + e.preventDefault(); + $(this).parentsUntil('tbody').find('input').each(function(idx, el){ + if (el.name == 'user_id'){ + data[el.name] = el.value; + } + }); + $.ajax({ + url:"/api/delete_users", + type: 'post', + data:JSON.stringify(data), + headers : {access_token:window.token}, + dataType: 'json', + contentType:"application/json; charset=UTF-8", + success: function(){ + my_btn.parentsUntil('tbody').remove(); + toastr.success('User deleted'); + }, + error: function(ret_data){ + toastr.error('Error: '+ret_data.responseJSON.message); + } + }); + }); + $(".reset_btn").click(function(e){ + var data = {}; + e.preventDefault(); + $(this).parentsUntil('tbody').find('input').each(function(idx, el){ + if (el.name == 'user_id'){ + data[el.name] = el.value; + } + }); + $.ajax({ + url:"/api/reset_password", + type: 'post', + data:JSON.stringify(data), + headers : {access_token:window.token}, + dataType: 'json', + contentType:"application/json; charset=UTF-8", + success: function(data){ + toastr.success( + 'Password updated successfully!
New password: '+data.password+'', + null, { + "closeButton": true, + "timeOut": 0, + "extendedTimeOut": 0, + "tapToDismiss": false + } + ); + }, + error: function(ret_data){ + toastr.error('Error: '+ret_data.responseJSON.message); + } + }); + }); + $(".add_btn").click(function(e){ + $("#my_alert").remove() + var data = {}; + e.preventDefault(); + $(this).parentsUntil('tbody').find('input').each(function(idx, el){ + if (el.type != 'button'){ + if (el.type=="checkbox"){ + data[el.name] = $(el).prop("checked"); + }else{ + data[el.name] = el.value; + } + } + }); + $.ajax({ + url:"/api/add_user", + type: 'post', + data: JSON.stringify(data), + headers: {access_token:window.token}, + dataType: 'json', + contentType: "application/json; charset=UTF-8", + success: function(ret_data){ + $('#add_table').find('input').each(function (idx, el){ + if (el.type != 'button'){ + if (el.type == "checkbox"){ + $(el).prop("checked", false); + }else{ + el.value = ''; + } + } + }); + $('#user_table tr:last').after( + ''+ + ''+ + ''+ + ''+ + ''+ + ''+ + ''+ + ''+ + ''+ + ''+ + ''+ + ''+ + ' '+ + ' '+ + ' '+ + ''+ + '' + ); + toastr.success( + 'User Added!
New password: '+ret_data.password+'', + null, { + "closeButton": true, + "timeOut": 0, + "extendedTimeOut": 0, + "tapToDismiss": false + } + ); + init_buttons(); + }, + error: function(){ + toastr.error('Error saving new user'); + } + }); + }); +} diff --git a/megaqc/templates/users/manage_users.html b/megaqc/templates/users/manage_users.html index 97afd3cc..ae9ab1c6 100644 --- a/megaqc/templates/users/manage_users.html +++ b/megaqc/templates/users/manage_users.html @@ -67,5 +67,5 @@

Registered users

{% block js %} - + {% endblock %} diff --git a/megaqc/user/models.py b/megaqc/user/models.py index 5ce64fcc..489353ca 100644 --- a/megaqc/user/models.py +++ b/megaqc/user/models.py @@ -7,6 +7,7 @@ import sys from builtins import str +from flask import current_app from flask_login import UserMixin from passlib.hash import argon2 from passlib.utils import getrandstr, rng @@ -21,6 +22,10 @@ Integer, Table, UnicodeText, + event, + func, + select, + update, ) from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.ext.hybrid import hybrid_method, hybrid_property @@ -29,20 +34,8 @@ from megaqc.database import CRUDMixin from megaqc.extensions import db -if sys.version_info.major == 2: - letters = string.letters - digits = string.digits -elif sys.version_info.major == 3: - letters = string.ascii_letters - digits = string.digits -else: - raise ( - Exception( - "Unsupport python version: v{}.{}".format( - sys.version_info.major, sys.version_info.minor - ) - ) - ) +letters = string.ascii_letters +digits = string.digits class Role(db.Model, CRUDMixin): @@ -94,6 +87,11 @@ def __init__(self, password=None, **kwargs): Create instance. """ db.Model.__init__(self, **kwargs) + + # Config adjusts the default active status + if "active" not in kwargs: + self.active = not current_app.config["USER_REGISTRATION_APPROVAL"] + self.salt = getrandstr(rng, digits + letters, 80) self.api_token = getrandstr(rng, digits + letters, 80) if password: @@ -101,8 +99,21 @@ def __init__(self, password=None, **kwargs): else: self.password = None - if self.user_id == 1: + def enforce_admin(self): + """ + Enforce that the first user is an active admin. + + This is included as a method that isn't automatically called, + because there are cases where we don't want this behaviour to + happen, such as during testing. + """ + if db.session.query(User).count() == 0: self.is_admin = True + self.active = True + + # users = User.__table__ + # if target.user_id == 1: + # connection.execute(users.update().where(users.c.user_id == 1).values(is_admin=True, active=True)) @hybrid_property def full_name(self): @@ -140,3 +151,11 @@ def __repr__(self): Represent instance as a unique string. """ return "".format(username=self.username) + + +# @event.listens_for(User, 'after_insert') +# def receive_after_insert(mapper, connection, target): +# # Here we enforce the business rule that the first user should be an active admin +# users = User.__table__ +# if target.user_id == 1: +# connection.execute(users.update().where(users.c.user_id == 1).values(is_admin=True, active=True)) diff --git a/setup.py b/setup.py index 41b16a8e..b2f18637 100644 --- a/setup.py +++ b/setup.py @@ -12,7 +12,7 @@ setup( name="megaqc", - version="0.2.0", + version="0.3.0", author="Phil Ewels", author_email="phil.ewels@scilifelab.se", description="Collect and visualise data across multiple MultiQC runs", diff --git a/tests/api/conftest.py b/tests/api/conftest.py index a0d3f017..714278b5 100644 --- a/tests/api/conftest.py +++ b/tests/api/conftest.py @@ -1,12 +1,14 @@ import pytest from flask import request +from flask.testing import FlaskClient from tests import factories -@pytest.yield_fixture("function") +@pytest.fixture("function") def client(app): with app.test_client() as c: + c: FlaskClient yield c @@ -24,9 +26,3 @@ def admin_token(db): db.session.add(user) db.session.commit() return user.api_token - - -@pytest.fixture() -def session(db): - sess = db.session - return sess diff --git a/tests/api/test_api.py b/tests/api/test_api.py index 7f461a21..d50f6de9 100644 --- a/tests/api/test_api.py +++ b/tests/api/test_api.py @@ -5,162 +5,23 @@ import pytest from flask import url_for from marshmallow import EXCLUDE -from sqlalchemy import inspect -from sqlalchemy.orm import RelationshipProperty -from tests import factories - - -def unset_dump_only(schema): - """ - Unset the "dump_only" property for every field in this schema. - """ - for field in schema.declared_fields.values(): - field.dump_only = False - schema._init_fields() - - -def dump_only_fields(schema): - """ - Returns a list of field names for the dump_only fields. - """ - return [key for key, field in schema._declared_fields.items() if field.dump_only] - - -def object_as_dict(obj, relationships=False): - """ - Converts an SQLAlchemy instance to a dictionary. - - :param relationships: If true, also include relationships in the output dict - """ - properties = inspect(obj).mapper.all_orm_descriptors - - if not relationships: - properties = { - key: value - for key, value in properties.items() - if not hasattr(value, "prop") - or not isinstance(value.prop, RelationshipProperty) - } - - return {key: getattr(obj, key) for key, value in properties.items()} - - -def resource_from_endpoint(app, endpoint): - """ - Given a string endpoint, e.g. "rest_api.upload", returns the Resource - object for that URL. - """ - return app.view_functions[endpoint].view_class - - -all_factories = factories.BaseFactory.__subclasses__() - - -def find_factory(model): - """ - Returns a factory that will build an instance of the provided model. - """ - for factory in all_factories: - if factory._meta.model == model: - return factory - - -def relationship_fields(model): - """ - Returns a list of keys that each correspond to a relationship on this - model. - """ - return [rel.key for rel in inspect(model).relationships.values()] - - -def instance_pk(instance): - """ - Returns a tuple of (column_name, column_value) for the first primary key on - this instance. - """ - column_name = inspect(instance.__class__).primary_key[0].name - return column_name, getattr(instance, column_name) - - -def find_matching_resource(data, instance, model): - """ - Given an array of dictionaries, checks if at least one of the dictionaries - matches the provided instance. - - :param data: A list of dictionaries - :param instance: An SQLAlchemy model instance - :param model: An SQLAlchemy model (subclass of declarative_base()) - """ - for result in data: - if is_matching_resource(result, instance, model): - return True - return False - - -def is_matching_resource(result, instance, model): - """ - Given a single dictionary, checks if it matches the provided SQLAlchemy - model instance. - - :param result: Instance dictionary - :param instance: An SQLAlchemy model instance - :param model: An SQLAlchemy model (subclass of declarative_base()) - """ - # Remove relationships because we can't validate them easily - for field in relationship_fields(model): - if field in result: - del result[field] - - if result.items() <= object_as_dict(instance).items(): - return True - - return False - - -def clone_model(instance): - """ - Clones an SQLAlchemy instance. - """ - # Copy the attributes as a dictionary - dict = object_as_dict(instance, relationships=True) - # Find the primary key and remove the ID - column_name = inspect(instance.__class__).primary_key[0].name - del dict[column_name] - # Create a new instance using this data - new_instance = instance.__class__(**dict) - return new_instance - - -def factory_clone(instance, factory): - """ - Generate a new object using the factory, except that relationships are - copied from the provided instance, ensuring that no new objects are - created. - """ - rels = { - key: getattr(instance, key) for key in relationship_fields(instance.__class__) - } - return factory(**rels) +from .utils import ( + dump_only_fields, + factory_clone, + find_factory, + find_matching_resource, + instance_pk, + is_matching_resource, + list_resource_endpoints, + resource_from_endpoint, + single_resource_endpoints, + unset_dump_only, + url_for, +) -@pytest.mark.parametrize( - "endpoint", - [ - "rest_api.uploadlist", - "rest_api.sampledatalist", - "rest_api.reportlist", - "rest_api.reportmetalist", - "rest_api.samplelist", - "rest_api.metatypelist", - "rest_api.datatypelist", - "rest_api.userlist", - "rest_api.filterlist", - "rest_api.filtergrouplist", - "rest_api.favouriteplotlist", - "rest_api.dashboardlist", - ], -) +@pytest.mark.parametrize("endpoint", list_resource_endpoints) def test_get_many_resources(endpoint, session, client, admin_token, app): """ GET /resource. @@ -258,20 +119,7 @@ def test_get_many_resources_associated( # assert dummy_instance not in data -@pytest.mark.parametrize( - "endpoint", - [ - "rest_api.upload", - "rest_api.sampledata", - "rest_api.report", - "rest_api.sample", - "rest_api.datatype", - "rest_api.user", - "rest_api.filter", - "rest_api.favouriteplot", - "rest_api.dashboard", - ], -) +@pytest.mark.parametrize("endpoint", single_resource_endpoints) def test_get_single_resource(endpoint, session, client, admin_token, app): """ GET /resource/1. diff --git a/tests/api/test_permissions.py b/tests/api/test_permissions.py new file mode 100644 index 00000000..1040d379 --- /dev/null +++ b/tests/api/test_permissions.py @@ -0,0 +1,147 @@ +import pytest +from flask import Flask +from marshmallow import EXCLUDE + +from megaqc.rest_api.schemas import UserSchema +from megaqc.user.models import User +from tests.factories import UserFactory + +from .utils import ( + dump_only_fields, + find_factory, + instance_pk, + list_resource_endpoints, + resource_from_endpoint, + single_resource_endpoints, + url_for, +) + + +def raise_response(resp: Flask.response_class): + if not str(resp.status_code).startswith("2"): + raise Exception( + "Request failed with status {} and body {}".format( + resp.status_code, resp.data + ) + ) + + +@pytest.mark.parametrize( + "endpoint", set(single_resource_endpoints) - {"rest_api.report"} +) +@pytest.mark.parametrize( + "method,success", + [ + ["GET", True], + ["PATCH", False], + ["DELETE", False], + ], +) +def test_single_resource_permissions( + endpoint, method, success, token, admin_token, session, client, app +): + """ + Test that each endpoint can only be accessed by the appropriate permission + level. + """ + resource = resource_from_endpoint(app, endpoint) + model = resource.data_layer["model"] + factory = find_factory(model) + + # Construct an instance of the model + instance = factory() + session.commit() + + # Do the request + pk = instance_pk(instance)[1] + url = url_for(endpoint, id=pk) + + # Check the request had the expected status, given that we're acting as a regular user + rv = client.open( + url, + method=method, + headers={"access_token": token, "Content-Type": "application/json"}, + ) + # Since we only care about access, we don't check if the request succeeded or not, just if it didn't 403 + assert (rv.status_code != 403) == success + + # Check the request worked, given that we're now an admin + rv = client.open( + url, + method=method, + headers={"access_token": admin_token, "Content-Type": "application/json"}, + ) + assert rv.status_code != 403 + + +@pytest.mark.parametrize( + # These two have unusual permissions + "endpoint", + set(list_resource_endpoints) - {"rest_api.uploadlist", "rest_api.userlist"}, +) +@pytest.mark.parametrize( + "method,success", + [ + ["GET", True], + ["POST", False], + ], +) +def test_many_resources_permissions( + endpoint, method, success, session, client, admin_token, token +): + """ + Test that each endpoint can only be accessed by the appropriate permission + level. + """ + # Do the request + url = url_for(endpoint) + + # Check the request had the expected status, given that we're acting as a regular user + rv = client.open( + url, + method=method, + headers={"access_token": token, "Content-Type": "application/json"}, + ) + # Since we only care about access, we don't check if the request succeeded or not, just if it didn't 403 + assert (rv.status_code != 403) == success + + # Check the request worked, given that we're now an admin + rv = client.open( + url, + method=method, + headers={"access_token": admin_token, "Content-Type": "application/json"}, + ) + assert rv.status_code != 403 + + +@pytest.mark.parametrize("strict", [True, False]) +def test_active_inactive(session, strict, app, client): + """ + The first user to register should be an activated admin, and subsequent + users should be inactive and regular users. + """ + app.config["USER_REGISTRATION_APPROVAL"] = strict + + # Non-admins aren't allowed to specify these fields + dump_only = dump_only_fields(UserSchema) + ["admin", "active", "salt", "api_token"] + req_schema = UserSchema(many=False, use_links=False, exclude=dump_only) + res_schema = UserSchema(many=False, unknown=EXCLUDE) + url = url_for("rest_api.userlist") + first = UserFactory.build() + second = UserFactory.build() + + # The first created user should be an active admin + first_res: Flask.response_class = client.post(path=url, json=req_schema.dump(first)) + raise_response(first_res) + first_model: dict = res_schema.load(first_res.json) + assert first_model["active"] + assert first_model["is_admin"] + + # The second and subsequent users should be active only if it's not in strict mode + second_res: Flask.response_class = client.post( + path=url, json=req_schema.dump(second) + ) + raise_response(second_res) + second_model: dict = res_schema.load(second_res.json) + assert second_model["active"] != strict + assert not second_model["is_admin"] diff --git a/tests/api/utils.py b/tests/api/utils.py new file mode 100644 index 00000000..6e0fb74c --- /dev/null +++ b/tests/api/utils.py @@ -0,0 +1,167 @@ +import pytest +from flask import url_for +from marshmallow import EXCLUDE +from sqlalchemy import inspect +from sqlalchemy.orm import RelationshipProperty + +from tests import factories + +single_resource_endpoints = [ + "rest_api.upload", + "rest_api.sampledata", + "rest_api.report", + "rest_api.sample", + "rest_api.datatype", + "rest_api.user", + "rest_api.filter", + "rest_api.favouriteplot", + "rest_api.dashboard", +] + +list_resource_endpoints = [ + "rest_api.uploadlist", + "rest_api.sampledatalist", + "rest_api.reportlist", + "rest_api.reportmetalist", + "rest_api.samplelist", + "rest_api.metatypelist", + "rest_api.datatypelist", + "rest_api.userlist", + "rest_api.filterlist", + "rest_api.filtergrouplist", + "rest_api.favouriteplotlist", + "rest_api.dashboardlist", +] + + +def unset_dump_only(schema): + """ + Unset the "dump_only" property for every field in this schema. + """ + for field in schema.declared_fields.values(): + field.dump_only = False + schema._init_fields() + + +def dump_only_fields(schema): + """ + Returns a list of field names for the dump_only fields. + """ + return [key for key, field in schema._declared_fields.items() if field.dump_only] + + +def object_as_dict(obj, relationships=False): + """ + Converts an SQLAlchemy instance to a dictionary. + + :param relationships: If true, also include relationships in the output dict + """ + properties = inspect(obj).mapper.all_orm_descriptors + + if not relationships: + properties = { + key: value + for key, value in properties.items() + if not hasattr(value, "prop") + or not isinstance(value.prop, RelationshipProperty) + } + + return {key: getattr(obj, key) for key, value in properties.items()} + + +def resource_from_endpoint(app, endpoint): + """ + Given a string endpoint, e.g. "rest_api.upload", returns the Resource + object for that URL. + """ + return app.view_functions[endpoint].view_class + + +all_factories = factories.BaseFactory.__subclasses__() + + +def find_factory(model): + """ + Returns a factory that will build an instance of the provided model. + """ + for factory in all_factories: + if factory._meta.model == model: + return factory + + +def relationship_fields(model): + """ + Returns a list of keys that each correspond to a relationship on this + model. + """ + return [rel.key for rel in inspect(model).relationships.values()] + + +def instance_pk(instance): + """ + Returns a tuple of (column_name, column_value) for the first primary key on + this instance. + """ + column_name = inspect(instance.__class__).primary_key[0].name + return column_name, getattr(instance, column_name) + + +def find_matching_resource(data, instance, model): + """ + Given an array of dictionaries, checks if at least one of the dictionaries + matches the provided instance. + + :param data: A list of dictionaries + :param instance: An SQLAlchemy model instance + :param model: An SQLAlchemy model (subclass of declarative_base()) + """ + for result in data: + if is_matching_resource(result, instance, model): + return True + return False + + +def is_matching_resource(result, instance, model): + """ + Given a single dictionary, checks if it matches the provided SQLAlchemy + model instance. + + :param result: Instance dictionary + :param instance: An SQLAlchemy model instance + :param model: An SQLAlchemy model (subclass of declarative_base()) + """ + # Remove relationships because we can't validate them easily + for field in relationship_fields(model): + if field in result: + del result[field] + + if result.items() <= object_as_dict(instance).items(): + return True + + return False + + +def clone_model(instance): + """ + Clones an SQLAlchemy instance. + """ + # Copy the attributes as a dictionary + dict = object_as_dict(instance, relationships=True) + # Find the primary key and remove the ID + column_name = inspect(instance.__class__).primary_key[0].name + del dict[column_name] + # Create a new instance using this data + new_instance = instance.__class__(**dict) + return new_instance + + +def factory_clone(instance, factory): + """ + Generate a new object using the factory, except that relationships are + copied from the provided instance, ensuring that no new objects are + created. + """ + rels = { + key: getattr(instance, key) for key in relationship_fields(instance.__class__) + } + return factory(**rels) diff --git a/tests/conftest.py b/tests/conftest.py index 5722a18f..f55ac46b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,6 +6,7 @@ from pathlib import Path import pytest +from sqlalchemy.orm.session import Session from webtest import TestApp from megaqc.app import create_app @@ -16,14 +17,14 @@ from .factories import UserFactory -@pytest.yield_fixture(scope="function") +@pytest.fixture(scope="function") def multiqc_data(): here = Path(__file__).parent with (here / "multiqc_data.json").open() as fp: return fp.read() -@pytest.yield_fixture(scope="function") +@pytest.fixture(scope="function") def app(): """ An application for the tests. @@ -47,7 +48,7 @@ def testapp(app): return TestApp(app) -@pytest.yield_fixture(scope="function") +@pytest.fixture(scope="function") def db(app): """ A database for the tests. @@ -71,3 +72,9 @@ def user(db): user = UserFactory(password="myprecious") db.session.commit() return user + + +@pytest.fixture() +def session(db): + sess: Session = db.session + return sess diff --git a/tests/test_docker.py b/tests/test_docker.py index 6c43a1a1..c40483a9 100644 --- a/tests/test_docker.py +++ b/tests/test_docker.py @@ -7,9 +7,12 @@ def raise_response(resp): + """ + :param resp: Requests response object + """ if not str(resp.status_code).startswith("2"): raise Exception( - "Request failed with status {} and body{}".format( + "Request failed with status {} and body {}".format( resp.status_code, resp.text ) ) @@ -19,7 +22,9 @@ def raise_response(resp): def compose_stack(): deploy = (Path(__file__).parent.parent / "deployment").resolve() # Start the stack, and wait for it to start up - subprocess.run(["docker-compose", "up", "-d"], cwd=deploy, check=True) + subprocess.run( + ["docker-compose", "up", "--build", "--detach"], cwd=deploy, check=True + ) time.sleep(15) yield # When we're done, stop the stack and cleanup the volumes @@ -32,11 +37,6 @@ def test_docker(): def test_compose(multiqc_data, compose_stack): - # Initially we should have no reports - result = requests.get(url="http://localhost/rest_api/v1/uploads") - raise_response(result) - assert len(result.json()["data"]) == 0 - # Create a user user = requests.post( "http://localhost/rest_api/v1/users", @@ -66,6 +66,8 @@ def test_compose(multiqc_data, compose_stack): report.raise_for_status() # Finally, we should have 1 report - result = requests.get(url="http://localhost/rest_api/v1/uploads") + result = requests.get( + url="http://localhost/rest_api/v1/uploads", headers={"access_token": token} + ) raise_response(result) assert len(result.json()["data"]) == 1