From 4e4f2b8037c15d8ed43151baed55352a1716dcbb Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Sun, 28 Aug 2016 15:23:57 +0200
Subject: [PATCH 1/8] 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/<admin>', 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/<domain_name>', 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/<domain_name>', 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/<alias>', 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/<alias>', 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)

From ee9a4166960285a0de6e7a0dbb0f49967bc747a7 Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Mon, 29 Aug 2016 19:24:39 +0200
Subject: [PATCH 2/8] Implement the decorator-based access control for all
 views

---
 admin/freeposte/admin/access.py         |  7 ++++-
 admin/freeposte/admin/views/base.py     |  8 +++---
 admin/freeposte/admin/views/domains.py  | 25 +++++++---------
 admin/freeposte/admin/views/fetches.py  | 20 +++++++------
 admin/freeposte/admin/views/managers.py | 11 +++----
 admin/freeposte/admin/views/users.py    | 38 ++++++++++++++-----------
 6 files changed, 59 insertions(+), 50 deletions(-)

diff --git a/admin/freeposte/admin/access.py b/admin/freeposte/admin/access.py
index 4c9e2764..8a8f2447 100644
--- a/admin/freeposte/admin/access.py
+++ b/admin/freeposte/admin/access.py
@@ -53,7 +53,12 @@ def domain_admin(args, kwargs, model, key):
 
 @permissions_wrapper
 def owner(args, kwargs, model, key):
-    obj = model.query.get(kwargs[key])
+    # if no key is provided but the model is User, then return the current
+    # user
+    if kwargs[key] is None and model == models.User:
+        obj = model.query.get(flask_login.current_user.email)
+    else:
+        obj = model.query.get(kwargs[key])
     if not obj:
         flask.abort(404)
     else:
diff --git a/admin/freeposte/admin/views/base.py b/admin/freeposte/admin/views/base.py
index 33e5d48d..2c32a832 100644
--- a/admin/freeposte/admin/views/base.py
+++ b/admin/freeposte/admin/views/base.py
@@ -1,5 +1,5 @@
 from freeposte import dockercli
-from freeposte.admin import app, db, models, forms, utils
+from freeposte.admin import app, db, models, forms, utils, access
 
 import os
 import flask
@@ -7,7 +7,7 @@ import flask_login
 
 
 @app.route('/', methods=["GET"])
-@flask_login.login_required
+@access.authenticated
 def index():
     return flask.redirect(flask.url_for('.user_settings'))
 
@@ -26,14 +26,14 @@ def login():
 
 
 @app.route('/logout', methods=['GET'])
-@flask_login.login_required
+@access.authenticated
 def logout():
     flask_login.logout_user()
     return flask.redirect(flask.url_for('.index'))
 
 
 @app.route('/services', methods=['GET'])
-@flask_login.login_required
+@access.global_admin
 def services():
     utils.require_global_admin()
     containers = {}
diff --git a/admin/freeposte/admin/views/domains.py b/admin/freeposte/admin/views/domains.py
index 93b0449e..b6c05c6d 100644
--- a/admin/freeposte/admin/views/domains.py
+++ b/admin/freeposte/admin/views/domains.py
@@ -1,4 +1,4 @@
-from freeposte.admin import app, db, models, forms, utils
+from freeposte.admin import app, db, models, forms, utils, access
 from freeposte import app as flask_app
 
 import os
@@ -8,15 +8,14 @@ import wtforms_components
 
 
 @app.route('/domain', methods=['GET'])
-@flask_login.login_required
+@access.authenticated
 def domain_list():
     return flask.render_template('domain/list.html')
 
 
 @app.route('/domain/create', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.global_admin
 def domain_create():
-    utils.require_global_admin()
     form = forms.DomainForm()
     if form.validate_on_submit():
         if models.Domain.query.get(form.name.data):
@@ -32,10 +31,9 @@ def domain_create():
 
 
 @app.route('/domain/edit/<domain_name>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.domain_admin(models.Domain, 'domain_name')
 def domain_edit(domain_name):
-    utils.require_global_admin()
-    domain = utils.get_domain_admin(domain_name)
+    domain = models.Domain.query.get(domain_name) or flask.abort(404)
     form = forms.DomainForm(obj=domain)
     wtforms_components.read_only(form.name)
     if form.validate_on_submit():
@@ -48,11 +46,10 @@ def domain_edit(domain_name):
 
 
 @app.route('/domain/delete/<domain_name>', methods=['GET', 'POST'])
+@access.global_admin
 @utils.confirmation_required("delete {domain_name}")
-@flask_login.login_required
 def domain_delete(domain_name):
-    utils.require_global_admin()
-    domain = utils.get_domain_admin(domain_name)
+    domain = models.Domain.query.get(domain_name) or flask.abort(404)
     db.session.delete(domain)
     db.session.commit()
     flask.flash('Domain %s deleted' % domain)
@@ -60,18 +57,18 @@ def domain_delete(domain_name):
 
 
 @app.route('/domain/details/<domain_name>', methods=['GET'])
-@flask_login.login_required
+@access.domain_admin(models.Domain, 'domain_name')
 def domain_details(domain_name):
-    domain = utils.get_domain_admin(domain_name)
+    domain = models.Domain.query.get(domain_name) or flask.abort(404)
     return flask.render_template('domain/details.html', domain=domain,
         config=flask_app.config)
 
 
 @app.route('/domain/genkeys/<domain_name>', methods=['GET', 'POST'])
+@access.domain_admin(models.Domain, 'domain_name')
 @utils.confirmation_required("regenerate keys for {domain_name}")
-@flask_login.login_required
 def domain_genkeys(domain_name):
-    domain = utils.get_domain_admin(domain_name)
+    domain = models.Domain.query.get(domain_name) or flask.abort(404)
     domain.generate_dkim_key()
     return flask.redirect(
         flask.url_for(".domain_details", domain_name=domain_name))
diff --git a/admin/freeposte/admin/views/fetches.py b/admin/freeposte/admin/views/fetches.py
index 5870ef2a..649d8576 100644
--- a/admin/freeposte/admin/views/fetches.py
+++ b/admin/freeposte/admin/views/fetches.py
@@ -1,4 +1,4 @@
-from freeposte.admin import app, db, models, forms, utils
+from freeposte.admin import app, db, models, forms, utils, access
 
 import os
 import flask
@@ -8,17 +8,19 @@ import wtforms_components
 
 @app.route('/fetch/list', methods=['GET', 'POST'], defaults={'user_email': None})
 @app.route('/fetch/list/<user_email>', methods=['GET'])
-@flask_login.login_required
+@access.owner(models.User, 'user_email')
 def fetch_list(user_email):
-    user = utils.get_user(user_email)
+    user_email = user_email or flask_login.current_user.email
+    user = models.User.query.get(user_email) or flask.abort(404)
     return flask.render_template('fetch/list.html', user=user)
 
 
 @app.route('/fetch/create', methods=['GET', 'POST'], defaults={'user_email': None})
 @app.route('/fetch/create/<user_email>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.owner(models.User, 'user_email')
 def fetch_create(user_email):
-    user = utils.get_user(user_email)
+    user_email = user_email or flask_login.current_user.email
+    user = models.User.query.get(user_email) or flask.abort(404)
     form = forms.FetchForm()
     if form.validate_on_submit():
         fetch = models.Fetch(user=user)
@@ -32,9 +34,9 @@ def fetch_create(user_email):
 
 
 @app.route('/fetch/edit/<fetch_id>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.owner(models.Fetch, 'fetch_id')
 def fetch_edit(fetch_id):
-    fetch = utils.get_fetch(fetch_id)
+    fetch = models.Fetch.query.get(fetch_id) or flask.abort(404)
     form = forms.FetchForm(obj=fetch)
     if form.validate_on_submit():
         form.populate_obj(fetch)
@@ -48,9 +50,9 @@ def fetch_edit(fetch_id):
 
 @app.route('/fetch/delete/<fetch_id>', methods=['GET', 'POST'])
 @utils.confirmation_required("delete a fetched account")
-@flask_login.login_required
+@access.owner(models.Fetch, 'fetch_id')
 def fetch_delete(fetch_id):
-    fetch = utils.get_fetch(fetch_id)
+    fetch = models.Fetch.query.get(fetch_id) or flask.abort(404)
     db.session.delete(fetch)
     db.session.commit()
     flask.flash('Fetch configuration delete')
diff --git a/admin/freeposte/admin/views/managers.py b/admin/freeposte/admin/views/managers.py
index b19789df..ec6e19e3 100644
--- a/admin/freeposte/admin/views/managers.py
+++ b/admin/freeposte/admin/views/managers.py
@@ -1,4 +1,4 @@
-from freeposte.admin import app, db, models, forms, utils
+from freeposte.admin import app, db, models, forms, utils, access
 
 import os
 import flask
@@ -7,16 +7,16 @@ import wtforms_components
 
 
 @app.route('/manager/list/<domain_name>', methods=['GET'])
-@flask_login.login_required
+@access.domain_admin(models.Domain, 'domain_name')
 def manager_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('manager/list.html', domain=domain)
 
 
 @app.route('/manager/create/<domain_name>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.domain_admin(models.Domain, 'domain_name')
 def manager_create(domain_name):
-    domain = utils.get_domain_admin(domain_name)
+    domain = models.Domain.query.get(domain_name) or flask.abort(404)
     form = forms.ManagerForm()
     form.manager.choices = [
         (user.email, user.email) for user in
@@ -40,6 +40,7 @@ def manager_create(domain_name):
 @utils.confirmation_required("remove manager {manager}")
 @flask_login.login_required
 def manager_delete(manager):
+    # TODO fix this behaviour
     user = utils.get_user(manager, admin=True)
     domain = utils.get_domain_admin(user.domain_name)
     if user in domain.managers:
diff --git a/admin/freeposte/admin/views/users.py b/admin/freeposte/admin/views/users.py
index 0c6c6374..37cab07e 100644
--- a/admin/freeposte/admin/views/users.py
+++ b/admin/freeposte/admin/views/users.py
@@ -1,4 +1,4 @@
-from freeposte.admin import app, db, models, forms, utils
+from freeposte.admin import app, db, models, forms, utils, access
 
 import os
 import flask
@@ -7,16 +7,16 @@ import wtforms_components
 
 
 @app.route('/user/list/<domain_name>', methods=['GET'])
-@flask_login.login_required
+@access.domain_admin(models.Domain, 'domain_name')
 def user_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('user/list.html', domain=domain)
 
 
 @app.route('/user/create/<domain_name>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.domain_admin(models.Domain, 'domain_name')
 def user_create(domain_name):
-    domain = utils.get_domain_admin(domain_name)
+    domain = models.Domain.query.get(domain_name) or flask.abort(404)
     if domain.max_users and len(domain.users) >= domain.max_users:
         flask.flash('Too many users for domain %s' % domain, 'error')
         return flask.redirect(
@@ -39,9 +39,9 @@ def user_create(domain_name):
 
 
 @app.route('/user/edit/<user_email>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.domain_admin(models.User, 'user_email')
 def user_edit(user_email):
-    user = utils.get_user(user_email, True)
+    user = models.User.query.get(user_email) or flask.abort(404)
     form = forms.UserForm(obj=user)
     wtforms_components.read_only(form.localpart)
     form.pw.validators = []
@@ -57,10 +57,10 @@ def user_edit(user_email):
 
 
 @app.route('/user/delete/<user_email>', methods=['GET', 'POST'])
+@access.domain_admin(models.User, 'user_email')
 @utils.confirmation_required("delete {user_email}")
-@flask_login.login_required
 def user_delete(user_email):
-    user = utils.get_user(user_email, True)
+    user = models.User.query.get(user_email) or flask.abort(404)
     db.session.delete(user)
     db.session.commit()
     flask.flash('User %s deleted' % user)
@@ -70,9 +70,10 @@ def user_delete(user_email):
 
 @app.route('/user/settings', methods=['GET', 'POST'], defaults={'user_email': None})
 @app.route('/user/usersettings/<user_email>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.owner(models.User, 'user_email')
 def user_settings(user_email):
-    user = utils.get_user(user_email)
+    user_email = user_email or flask_login.current_user.email
+    user = models.User.query.get(user_email) or flask.abort(404)
     form = forms.UserSettingsForm(obj=user)
     if form.validate_on_submit():
         form.populate_obj(user)
@@ -86,9 +87,10 @@ def user_settings(user_email):
 
 @app.route('/user/password', methods=['GET', 'POST'], defaults={'user_email': None})
 @app.route('/user/password/<user_email>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.owner(models.User, 'user_email')
 def user_password(user_email):
-    user = utils.get_user(user_email)
+    user_email = user_email or flask_login.current_user.email
+    user = models.User.query.get(user_email) or flask.abort(404)
     form = forms.UserPasswordForm()
     if form.validate_on_submit():
         if form.pw.data != form.pw2.data:
@@ -105,9 +107,10 @@ def user_password(user_email):
 
 @app.route('/user/forward', methods=['GET', 'POST'], defaults={'user_email': None})
 @app.route('/user/forward/<user_email>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.owner(models.User, 'user_email')
 def user_forward(user_email):
-    user = utils.get_user(user_email)
+    user_email = user_email or flask_login.current_user.email
+    user = models.User.query.get(user_email) or flask.abort(404)
     form = forms.UserForwardForm(obj=user)
     if form.validate_on_submit():
         form.populate_obj(user)
@@ -121,9 +124,10 @@ def user_forward(user_email):
 
 @app.route('/user/reply', methods=['GET', 'POST'], defaults={'user_email': None})
 @app.route('/user/reply/<user_email>', methods=['GET', 'POST'])
-@flask_login.login_required
+@access.owner(models.User, 'user_email')
 def user_reply(user_email):
-    user = utils.get_user(user_email)
+    user_email = user_email or flask_login.current_user.email
+    user = models.User.query.get(user_email) or flask.abort(404)
     form = forms.UserReplyForm(obj=user)
     if form.validate_on_submit():
         form.populate_obj(user)

From 713318f097701b74932930373b388922d3a79190 Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Mon, 29 Aug 2016 19:35:09 +0200
Subject: [PATCH 3/8] Clean imports and remove calls to the utils module

---
 admin/freeposte/admin/views/admins.py   |  3 ---
 admin/freeposte/admin/views/aliases.py  |  2 --
 admin/freeposte/admin/views/base.py     |  4 +--
 admin/freeposte/admin/views/domains.py  |  8 +++---
 admin/freeposte/admin/views/fetches.py  |  6 ++---
 admin/freeposte/admin/views/managers.py | 33 +++++++++++++------------
 admin/freeposte/admin/views/users.py    |  5 ++--
 7 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/admin/freeposte/admin/views/admins.py b/admin/freeposte/admin/views/admins.py
index ccb159da..0e314f4b 100644
--- a/admin/freeposte/admin/views/admins.py
+++ b/admin/freeposte/admin/views/admins.py
@@ -1,10 +1,7 @@
 from freeposte.admin import app, db, models, forms, access
 
-import os
-import pprint
 import flask
 import flask_login
-import json
 
 
 @app.route('/admin/list', methods=['GET'])
diff --git a/admin/freeposte/admin/views/aliases.py b/admin/freeposte/admin/views/aliases.py
index 3d046f90..aad93945 100644
--- a/admin/freeposte/admin/views/aliases.py
+++ b/admin/freeposte/admin/views/aliases.py
@@ -1,8 +1,6 @@
 from freeposte.admin import app, db, models, forms, access
 
-import os
 import flask
-import flask_login
 import wtforms_components
 
 
diff --git a/admin/freeposte/admin/views/base.py b/admin/freeposte/admin/views/base.py
index 2c32a832..d2e1ef8a 100644
--- a/admin/freeposte/admin/views/base.py
+++ b/admin/freeposte/admin/views/base.py
@@ -1,7 +1,6 @@
 from freeposte import dockercli
-from freeposte.admin import app, db, models, forms, utils, access
+from freeposte.admin import app, db, models, forms, access
 
-import os
 import flask
 import flask_login
 
@@ -35,7 +34,6 @@ def logout():
 @app.route('/services', methods=['GET'])
 @access.global_admin
 def services():
-    utils.require_global_admin()
     containers = {}
     for brief in dockercli.containers(all=True):
         if brief['Image'].startswith('freeposte/'):
diff --git a/admin/freeposte/admin/views/domains.py b/admin/freeposte/admin/views/domains.py
index b6c05c6d..a1e66bb0 100644
--- a/admin/freeposte/admin/views/domains.py
+++ b/admin/freeposte/admin/views/domains.py
@@ -1,9 +1,7 @@
-from freeposte.admin import app, db, models, forms, utils, access
+from freeposte.admin import app, db, models, forms, access
 from freeposte import app as flask_app
 
-import os
 import flask
-import flask_login
 import wtforms_components
 
 
@@ -47,7 +45,7 @@ def domain_edit(domain_name):
 
 @app.route('/domain/delete/<domain_name>', methods=['GET', 'POST'])
 @access.global_admin
-@utils.confirmation_required("delete {domain_name}")
+@access.confirmation_required("delete {domain_name}")
 def domain_delete(domain_name):
     domain = models.Domain.query.get(domain_name) or flask.abort(404)
     db.session.delete(domain)
@@ -66,7 +64,7 @@ def domain_details(domain_name):
 
 @app.route('/domain/genkeys/<domain_name>', methods=['GET', 'POST'])
 @access.domain_admin(models.Domain, 'domain_name')
-@utils.confirmation_required("regenerate keys for {domain_name}")
+@access.confirmation_required("regenerate keys for {domain_name}")
 def domain_genkeys(domain_name):
     domain = models.Domain.query.get(domain_name) or flask.abort(404)
     domain.generate_dkim_key()
diff --git a/admin/freeposte/admin/views/fetches.py b/admin/freeposte/admin/views/fetches.py
index 649d8576..d9cdf7c2 100644
--- a/admin/freeposte/admin/views/fetches.py
+++ b/admin/freeposte/admin/views/fetches.py
@@ -1,9 +1,7 @@
-from freeposte.admin import app, db, models, forms, utils, access
+from freeposte.admin import app, db, models, forms, access
 
-import os
 import flask
 import flask_login
-import wtforms_components
 
 
 @app.route('/fetch/list', methods=['GET', 'POST'], defaults={'user_email': None})
@@ -49,7 +47,7 @@ def fetch_edit(fetch_id):
 
 
 @app.route('/fetch/delete/<fetch_id>', methods=['GET', 'POST'])
-@utils.confirmation_required("delete a fetched account")
+@access.confirmation_required("delete a fetched account")
 @access.owner(models.Fetch, 'fetch_id')
 def fetch_delete(fetch_id):
     fetch = models.Fetch.query.get(fetch_id) or flask.abort(404)
diff --git a/admin/freeposte/admin/views/managers.py b/admin/freeposte/admin/views/managers.py
index ec6e19e3..793b974e 100644
--- a/admin/freeposte/admin/views/managers.py
+++ b/admin/freeposte/admin/views/managers.py
@@ -1,9 +1,7 @@
-from freeposte.admin import app, db, models, forms, utils, access
+from freeposte.admin import app, db, models, forms, access
 
-import os
 import flask
 import flask_login
-import wtforms_components
 
 
 @app.route('/manager/list/<domain_name>', methods=['GET'])
@@ -18,13 +16,16 @@ def manager_list(domain_name):
 def manager_create(domain_name):
     domain = models.Domain.query.get(domain_name) or flask.abort(404)
     form = forms.ManagerForm()
+    available_users = flask_login.current_user.get_managed_emails(
+        include_aliases=False)
     form.manager.choices = [
-        (user.email, user.email) for user in
-        flask_login.current_user.get_managed_emails(include_aliases=False)
+        (user.email, user.email) for user in available_users
     ]
     if form.validate_on_submit():
-        user = utils.get_user(form.manager.data, admin=True)
-        if user in domain.managers:
+        user = models.User.query.get(form.manager.data)
+        if user not in available_users:
+            flask.abort(403)
+        elif user in domain.managers:
             flask.flash('User %s is already manager' % user, 'error')
         else:
             domain.managers.append(user)
@@ -36,18 +37,18 @@ def manager_create(domain_name):
         domain=domain, form=form)
 
 
+# TODO For now the deletion behaviour is broken and reserved to
+# global admins.
 @app.route('/manager/delete/<manager>', methods=['GET', 'POST'])
-@utils.confirmation_required("remove manager {manager}")
-@flask_login.login_required
+@access.confirmation_required("remove manager {manager}")
+@access.global_admin
 def manager_delete(manager):
-    # TODO fix this behaviour
-    user = utils.get_user(manager, admin=True)
-    domain = utils.get_domain_admin(user.domain_name)
-    if user in domain.managers:
-        domain.managers.remove(user)
+    user = models.User.query.get(manager)
+    if user in user.domain.managers:
+        user.domain.managers.remove(user)
         db.session.commit()
-        flask.flash('User %s can no longer manager %s' % (user, domain))
+        flask.flash('User %s can no longer manager %s' % (user, user.domain))
     else:
         flask.flash('User %s is not manager' % user, 'error')
     return flask.redirect(
-        flask.url_for('.manager_list', domain_name=domain.name))
+        flask.url_for('.manager_list', domain_name=user.domain.name))
diff --git a/admin/freeposte/admin/views/users.py b/admin/freeposte/admin/views/users.py
index 37cab07e..524d0804 100644
--- a/admin/freeposte/admin/views/users.py
+++ b/admin/freeposte/admin/views/users.py
@@ -1,6 +1,5 @@
-from freeposte.admin import app, db, models, forms, utils, access
+from freeposte.admin import app, db, models, forms, access
 
-import os
 import flask
 import flask_login
 import wtforms_components
@@ -58,7 +57,7 @@ def user_edit(user_email):
 
 @app.route('/user/delete/<user_email>', methods=['GET', 'POST'])
 @access.domain_admin(models.User, 'user_email')
-@utils.confirmation_required("delete {user_email}")
+@access.confirmation_required("delete {user_email}")
 def user_delete(user_email):
     user = models.User.query.get(user_email) or flask.abort(404)
     db.session.delete(user)

From f541a951de99da9ee8997ec7aa5f97123f528e17 Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Mon, 29 Aug 2016 19:36:37 +0200
Subject: [PATCH 4/8] Remove obsolete utils module

---
 admin/freeposte/admin/utils.py | 69 ----------------------------------
 1 file changed, 69 deletions(-)
 delete mode 100644 admin/freeposte/admin/utils.py

diff --git a/admin/freeposte/admin/utils.py b/admin/freeposte/admin/utils.py
deleted file mode 100644
index 6deaf53c..00000000
--- a/admin/freeposte/admin/utils.py
+++ /dev/null
@@ -1,69 +0,0 @@
-from freeposte.admin import models, forms
-
-import flask
-import flask_login
-import functools
-
-
-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
-
-
-def get_domain_admin(domain_name):
-    domain = models.Domain.query.get(domain_name)
-    if not domain:
-        flask.abort(404)
-    if not domain in flask_login.current_user.get_managed_domains():
-        flask.abort(403)
-    return domain
-
-
-def require_global_admin():
-    if not flask_login.current_user.global_admin:
-        flask.abort(403)
-
-
-def get_user(user_email, admin=False):
-    if user_email is None:
-        user_email = flask_login.current_user.email
-    user = models.User.query.get(user_email)
-    if not user:
-        flask.abort(404)
-    if not user.domain in flask_login.current_user.get_managed_domains():
-        if admin:
-            flask.abort(403)
-        elif not user.email == flask_login.current_user.email:
-            flask.abort(403)
-    return user
-
-
-def get_alias(alias):
-    alias = models.Alias.query.get(alias)
-    if not alias:
-        flask.abort(404)
-    if not alias.domain in flask_login.current_user.get_managed_domains():
-        return 403
-    return alias
-
-
-def get_fetch(fetch_id):
-    fetch = models.Fetch.query.get(fetch_id)
-    if not fetch:
-        flask.abort(404)
-    if not fetch.user.domain in flask_login.current_user.get_managed_domains():
-        if not fetch.user.email == flask_login.current_user.email:
-            flask.abort(403)
-    return fetch

From f8dcef22efce2f87f2ec8b80aa856f3c6cf15145 Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Mon, 29 Aug 2016 19:40:18 +0200
Subject: [PATCH 5/8] Fix the manager deletion behaviour

---
 .../admin/templates/manager/list.html         |  2 +-
 admin/freeposte/admin/views/managers.py       | 21 +++++++++----------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/admin/freeposte/admin/templates/manager/list.html b/admin/freeposte/admin/templates/manager/list.html
index 23ef317f..d1c9da2d 100644
--- a/admin/freeposte/admin/templates/manager/list.html
+++ b/admin/freeposte/admin/templates/manager/list.html
@@ -22,7 +22,7 @@ Manager list
     {% for manager in domain.managers %}
     <tr>
       <td>
-        <a href="{{ url_for('.manager_delete', manager=manager.email) }}" title="Delete"><i class="fa fa-trash"></i></a>
+        <a href="{{ url_for('.manager_delete', domain_name=domain.name, user_email=manager.email) }}" title="Delete"><i class="fa fa-trash"></i></a>
       </td>
       <td>{{ manager }}</td>
     </tr>
diff --git a/admin/freeposte/admin/views/managers.py b/admin/freeposte/admin/views/managers.py
index 793b974e..7e464c48 100644
--- a/admin/freeposte/admin/views/managers.py
+++ b/admin/freeposte/admin/views/managers.py
@@ -37,18 +37,17 @@ def manager_create(domain_name):
         domain=domain, form=form)
 
 
-# TODO For now the deletion behaviour is broken and reserved to
-# global admins.
-@app.route('/manager/delete/<manager>', methods=['GET', 'POST'])
-@access.confirmation_required("remove manager {manager}")
-@access.global_admin
-def manager_delete(manager):
-    user = models.User.query.get(manager)
-    if user in user.domain.managers:
-        user.domain.managers.remove(user)
+@app.route('/manager/delete/<domain_name>/<user_email>', methods=['GET', 'POST'])
+@access.confirmation_required("remove manager {user_email}")
+@access.domain_admin(models.Domain, 'domain_name')
+def manager_delete(domain_name, user_email):
+    domain = models.Domain.query.get(domain_name) or flask.abort(404)
+    user = models.User.query.get(user_email) or flask.abort(404)
+    if user in domain.managers:
+        domain.managers.remove(user)
         db.session.commit()
-        flask.flash('User %s can no longer manager %s' % (user, user.domain))
+        flask.flash('User %s can no longer manager %s' % (user, domain))
     else:
         flask.flash('User %s is not manager' % user, 'error')
     return flask.redirect(
-        flask.url_for('.manager_list', domain_name=user.domain.name))
+        flask.url_for('.manager_list', domain_name=domain_name))

From c1f9b61dacaaa982ec3496ffb48bc85b91e143aa Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Mon, 29 Aug 2016 20:18:00 +0200
Subject: [PATCH 6/8] Add a simple permission audit script

---
 admin/audit.py                  | 43 +++++++++++++++++++++++++++++++++
 admin/freeposte/admin/access.py |  2 ++
 admin/requirements.txt          |  1 +
 3 files changed, 46 insertions(+)
 create mode 100644 admin/audit.py

diff --git a/admin/audit.py b/admin/audit.py
new file mode 100644
index 00000000..fdd997f6
--- /dev/null
+++ b/admin/audit.py
@@ -0,0 +1,43 @@
+from freeposte import app
+
+import sys
+import tabulate
+
+
+# Known endpoints without permissions
+known_missing_permissions = [
+    "index",
+    "static", "bootstrap.static",
+    "admin.static", "admin.login"
+]
+
+
+# Compute the permission table
+missing_permissions = []
+permissions = {}
+for endpoint, function in app.view_functions.items():
+    audit = function.__dict__.get("_audit_permissions")
+    if audit:
+        handler, args = audit
+        if args:
+            model = args[0].__name__
+            key = args[1]
+        else:
+            model = key = None
+        permissions[endpoint] = [endpoint, handler.__name__, model, key]
+    elif endpoint not in known_missing_permissions:
+        missing_permissions.append(endpoint)
+
+
+# Fail if any endpoint is missing a permission check
+if missing_permissions:
+    print("The following endpoints are missing permission checks:")
+    print(missing_permissions.join(","))
+    sys.exit(1)
+
+
+# Display the permissions table
+print(tabulate.tabulate([
+    [route, *permissions[route.endpoint]]
+    for route in app.url_map.iter_rules() if route.endpoint in permissions
+]))
diff --git a/admin/freeposte/admin/access.py b/admin/freeposte/admin/access.py
index 8a8f2447..58ea4b6e 100644
--- a/admin/freeposte/admin/access.py
+++ b/admin/freeposte/admin/access.py
@@ -25,6 +25,7 @@ def permissions_wrapper(handler):
                 @functools.wraps(function)
                 def wrapper(*args, **kwargs):
                     return callback(function, args, kwargs, dargs, dkwargs)
+                wrapper._audit_permissions = handler, dargs
                 return flask_login.login_required(wrapper)
             return inner
     else:
@@ -32,6 +33,7 @@ def permissions_wrapper(handler):
             @functools.wraps(function)
             def wrapper(*args, **kwargs):
                 return callback(function, args, kwargs, (), {})
+            wrapper._audit_permissions = handler, []
             return flask_login.login_required(wrapper)
     return decorator
 
diff --git a/admin/requirements.txt b/admin/requirements.txt
index acd52c92..a7076531 100644
--- a/admin/requirements.txt
+++ b/admin/requirements.txt
@@ -10,3 +10,4 @@ PyOpenSSL
 passlib
 gunicorn
 docker-py
+tabulate

From 09bec055fd8ec80e0ec9b38c60b30022b09ef42f Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Mon, 29 Aug 2016 20:22:44 +0200
Subject: [PATCH 7/8] Fix domain deletion permissions

---
 admin/freeposte/admin/views/domains.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/admin/freeposte/admin/views/domains.py b/admin/freeposte/admin/views/domains.py
index a1e66bb0..248423ee 100644
--- a/admin/freeposte/admin/views/domains.py
+++ b/admin/freeposte/admin/views/domains.py
@@ -29,7 +29,7 @@ def domain_create():
 
 
 @app.route('/domain/edit/<domain_name>', methods=['GET', 'POST'])
-@access.domain_admin(models.Domain, 'domain_name')
+@access.global_admin
 def domain_edit(domain_name):
     domain = models.Domain.query.get(domain_name) or flask.abort(404)
     form = forms.DomainForm(obj=domain)

From e24da96e583d6793017e43ba3645f38cb41ed00c Mon Sep 17 00:00:00 2001
From: Pierre Jaury <pierre@jaury.eu>
Date: Mon, 29 Aug 2016 20:30:59 +0200
Subject: [PATCH 8/8] Add some documentation to access decorators

---
 admin/freeposte/admin/access.py | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/admin/freeposte/admin/access.py b/admin/freeposte/admin/access.py
index 58ea4b6e..34dbbfdd 100644
--- a/admin/freeposte/admin/access.py
+++ b/admin/freeposte/admin/access.py
@@ -40,11 +40,21 @@ def permissions_wrapper(handler):
 
 @permissions_wrapper
 def global_admin(args, kwargs):
+    """ The view is only available to global administrators.
+    """
     return flask_login.current_user.global_admin
 
 
 @permissions_wrapper
 def domain_admin(args, kwargs, model, key):
+    """ The view is only available to specific domain admins.
+    Global admins will still be able to access the resource.
+
+    A model and key must be provided. The model will be queries
+    based on the query parameter named after the key. The model may
+    either be Domain or an Email subclass (or any class with a
+    ``domain`` attribute which stores a related Domain instance).
+    """
     obj = model.query.get(kwargs[key])
     if not obj:
         flask.abort(404)
@@ -55,12 +65,20 @@ def domain_admin(args, kwargs, model, key):
 
 @permissions_wrapper
 def owner(args, kwargs, model, key):
-    # if no key is provided but the model is User, then return the current
-    # user
+    """ The view is only available to the resource owner or manager.
+
+    A model and key must be provided. The model will be queries
+    based on the query parameter named after the key. The model may
+    either be User or any model with a ``user`` attribute storing
+    a user instance (like Fetch).
+
+    If the query parameter is empty and the model is User, then
+    the resource being accessed is supposed to be the current
+    logged in user and access is obviously authorized.
+    """
     if kwargs[key] is None and model == models.User:
-        obj = model.query.get(flask_login.current_user.email)
-    else:
-        obj = model.query.get(kwargs[key])
+        return True
+    obj = model.query.get(kwargs[key])
     if not obj:
         flask.abort(404)
     else:
@@ -73,6 +91,8 @@ def owner(args, kwargs, model, key):
 
 @permissions_wrapper
 def authenticated(args, kwargs):
+    """ The view is only available to logged in users.
+    """
     return True