From 4e4f2b8037c15d8ed43151baed55352a1716dcbb Mon Sep 17 00:00:00 2001 From: Pierre Jaury Date: Sun, 28 Aug 2016 15:23:57 +0200 Subject: [PATCH] First shot at improving access control, related to #42 A couple of things are important to note for this commit: - it only implements the new access control for alias and admin management - the access control code is located in access.py The idea behind simpler access control is auditability. There have been a couple of bugs related to functions not checking permissions properly. If checking permissions is as simple as decorating a function, exporting the permission scheme for an audit should be simple. Also, this still does not address the information leakage related to 404 errors when an object does not exist, independently of permissions the user has over the domain. --- admin/freeposte/admin/access.py | 87 ++++++++++++++++++++++++++ admin/freeposte/admin/views/admins.py | 13 ++-- admin/freeposte/admin/views/aliases.py | 20 +++--- 3 files changed, 102 insertions(+), 18 deletions(-) create mode 100644 admin/freeposte/admin/access.py diff --git a/admin/freeposte/admin/access.py b/admin/freeposte/admin/access.py new file mode 100644 index 00000000..4c9e2764 --- /dev/null +++ b/admin/freeposte/admin/access.py @@ -0,0 +1,87 @@ +from freeposte.admin import db, models, forms + +import flask +import flask_login +import functools + + +def permissions_wrapper(handler): + """ Decorator that produces a decorator for checking permissions. + """ + def callback(function, args, kwargs, dargs, dkwargs): + authorized = handler(args, kwargs, *dargs, **dkwargs) + if not authorized: + flask.abort(403) + elif type(authorized) is int: + flask.abort(authorized) + else: + return function(*args, **kwargs) + # If the handler has no argument, declare a + # simple decorator, otherwise declare a nested decorator + # There are at least two mandatory arguments + if handler.__code__.co_argcount > 2: + def decorator(*dargs, **dkwargs): + def inner(function): + @functools.wraps(function) + def wrapper(*args, **kwargs): + return callback(function, args, kwargs, dargs, dkwargs) + return flask_login.login_required(wrapper) + return inner + else: + def decorator(function): + @functools.wraps(function) + def wrapper(*args, **kwargs): + return callback(function, args, kwargs, (), {}) + return flask_login.login_required(wrapper) + return decorator + + +@permissions_wrapper +def global_admin(args, kwargs): + return flask_login.current_user.global_admin + + +@permissions_wrapper +def domain_admin(args, kwargs, model, key): + obj = model.query.get(kwargs[key]) + if not obj: + flask.abort(404) + else: + domain = obj if type(obj) is models.Domain else obj.domain + return domain in flask_login.current_user.get_managed_domains() + + +@permissions_wrapper +def owner(args, kwargs, model, key): + obj = model.query.get(kwargs[key]) + if not obj: + flask.abort(404) + else: + user = obj if type(obj) is models.User else obj.user + return ( + user.email == flask_login.current_user.email + or user.domain in flask_login.current_user.get_managed_domains() + ) + + +@permissions_wrapper +def authenticated(args, kwargs): + return True + + + +def confirmation_required(action): + """ View decorator that asks for a confirmation first. + """ + def inner(function): + @functools.wraps(function) + def wrapper(*args, **kwargs): + form = forms.ConfirmationForm() + if form.validate_on_submit(): + return function(*args, **kwargs) + return flask.render_template( + "confirm.html", action=action.format(*args, **kwargs), + form=form + ) + return wrapper + return inner diff --git a/admin/freeposte/admin/views/admins.py b/admin/freeposte/admin/views/admins.py index 3e4cc3f7..ccb159da 100644 --- a/admin/freeposte/admin/views/admins.py +++ b/admin/freeposte/admin/views/admins.py @@ -1,4 +1,4 @@ -from freeposte.admin import app, db, models, forms, utils +from freeposte.admin import app, db, models, forms, access import os import pprint @@ -8,17 +8,15 @@ import json @app.route('/admin/list', methods=['GET']) -@flask_login.login_required +@access.global_admin def admin_list(): - utils.require_global_admin() admins = models.User.query.filter_by(global_admin=True) return flask.render_template('admin/list.html', admins=admins) @app.route('/admin/create', methods=['GET', 'POST']) -@flask_login.login_required +@access.global_admin def admin_create(): - utils.require_global_admin() form = forms.AdminForm() form.admin.choices = [ (user.email, user.email) @@ -38,10 +36,9 @@ def admin_create(): @app.route('/admin/delete/', methods=['GET', 'POST']) -@utils.confirmation_required("delete admin {admin}") -@flask_login.login_required +@access.global_admin +@access.confirmation_required("delete admin {admin}") def admin_delete(admin): - utils.require_global_admin() user = models.User.query.get(admin) if user: user.global_admin = False diff --git a/admin/freeposte/admin/views/aliases.py b/admin/freeposte/admin/views/aliases.py index b60ff7c0..3d046f90 100644 --- a/admin/freeposte/admin/views/aliases.py +++ b/admin/freeposte/admin/views/aliases.py @@ -1,4 +1,4 @@ -from freeposte.admin import app, db, models, forms, utils +from freeposte.admin import app, db, models, forms, access import os import flask @@ -7,16 +7,16 @@ import wtforms_components @app.route('/alias/list/', methods=['GET']) -@flask_login.login_required +@access.domain_admin(models.Domain, 'domain_name') def alias_list(domain_name): - domain = utils.get_domain_admin(domain_name) + domain = models.Domain.query.get(domain_name) or flask.abort(404) return flask.render_template('alias/list.html', domain=domain) @app.route('/alias/create/', methods=['GET', 'POST']) -@flask_login.login_required +@access.domain_admin(models.Domain, 'domain_name') def alias_create(domain_name): - domain = utils.get_domain_admin(domain_name) + domain = models.Domain.query.get(domain_name) or flask.abort(404) if domain.max_aliases and len(domain.aliases) >= domain.max_aliases: flask.flash('Too many aliases for domain %s' % domain, 'error') return flask.redirect( @@ -38,9 +38,9 @@ def alias_create(domain_name): @app.route('/alias/edit/', methods=['GET', 'POST']) -@flask_login.login_required +@access.domain_admin(models.Alias, 'alias') def alias_edit(alias): - alias = utils.get_alias(alias) + alias = models.Alias.query.get(alias) or flask.abort(404) form = forms.AliasForm(obj=alias) wtforms_components.read_only(form.localpart) if form.validate_on_submit(): @@ -54,10 +54,10 @@ def alias_edit(alias): @app.route('/alias/delete/', methods=['GET', 'POST']) -@utils.confirmation_required("delete {alias}") -@flask_login.login_required +@access.domain_admin(models.Alias, 'alias') +@access.confirmation_required("delete {alias}") def alias_delete(alias): - alias = utils.get_alias(alias) + alias = models.Alias.query.get(alias) or flask.abort(404) db.session.delete(alias) db.session.commit() flask.flash('Alias %s deleted' % alias)