From b783e443011f5879fa45657e3d3e8009ba51bb71 Mon Sep 17 00:00:00 2001 From: Peter Weidenbach Date: Tue, 10 Apr 2018 11:50:02 +0200 Subject: [PATCH 1/7] show admin panal only if user is superuser --- src/web_interface/components/analysis_routes.py | 13 ++++++++++++- src/web_interface/templates/show_analysis.html | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/web_interface/components/analysis_routes.py b/src/web_interface/components/analysis_routes.py index 2b1e5145a..704a74eee 100644 --- a/src/web_interface/components/analysis_routes.py +++ b/src/web_interface/components/analysis_routes.py @@ -5,6 +5,9 @@ from common_helper_files import get_binary_from_file from flask import render_template, request, render_template_string +from flask_security.core import AnonymousUser +from flask_login.utils import current_user +import logging from helperFunctions.dataConversion import none_to_none from helperFunctions.fileSystem import get_src_dir @@ -47,6 +50,13 @@ def _get_firmware_ids_including_this_file(fo): else: return list(fo.get_virtual_file_paths().keys()) + @staticmethod + def _is_superuser(user): + if isinstance(user._get_current_object(), AnonymousUser): + return True + else: + return user.has_role('superuser') + @roles_accepted(*PRIVILEGES['view_analysis']) def _show_analysis_results(self, uid, selected_analysis=None, root_uid=None): root_uid = none_to_none(root_uid) @@ -78,7 +88,8 @@ def _show_analysis_results(self, uid, selected_analysis=None, root_uid=None): firmware_including_this_fo=firmware_including_this_fo, analysis_plugin_dict=analysis_plugins, other_versions=other_versions, - uids_for_comparison=uids_for_comparison) + uids_for_comparison=uids_for_comparison, + show_admin_panal=self._is_superuser(current_user)) else: return render_template('uid_not_found.html', uid=uid) diff --git a/src/web_interface/templates/show_analysis.html b/src/web_interface/templates/show_analysis.html index ae9eb77b2..29bdd4ae6 100644 --- a/src/web_interface/templates/show_analysis.html +++ b/src/web_interface/templates/show_analysis.html @@ -407,7 +407,7 @@

Analysis for {{ firmware.get_hid(root_uid=root_ui {# admin buttons #} - {% if firmware.vendor %} + {% if firmware.vendor and show_admin_panal %}
admin options: From 518c1adb9b4f43fa575ca43c388fb987e9a9839e Mon Sep 17 00:00:00 2001 From: Peter Weidenbach Date: Tue, 10 Apr 2018 12:45:55 +0200 Subject: [PATCH 2/7] refactoring --- src/helperFunctions/web_interface.py | 8 ++++++++ src/web_interface/components/analysis_routes.py | 14 ++------------ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/helperFunctions/web_interface.py b/src/helperFunctions/web_interface.py index caffb78a7..00ec0ac45 100644 --- a/src/helperFunctions/web_interface.py +++ b/src/helperFunctions/web_interface.py @@ -3,6 +3,7 @@ import os import re +from flask_security.core import AnonymousUser from common_helper_files import get_binary_from_file from itertools import chain @@ -77,3 +78,10 @@ def __exit__(self, *args): def get_template_as_string(view_name): path = os.path.join(get_template_dir(), view_name) return get_binary_from_file(path).decode('utf-8') + + +def is_superuser(user): + if isinstance(user._get_current_object(), AnonymousUser): # auth disabled + return True + else: + return user.has_role('superuser') diff --git a/src/web_interface/components/analysis_routes.py b/src/web_interface/components/analysis_routes.py index 704a74eee..bbf6ffaf5 100644 --- a/src/web_interface/components/analysis_routes.py +++ b/src/web_interface/components/analysis_routes.py @@ -5,15 +5,12 @@ from common_helper_files import get_binary_from_file from flask import render_template, request, render_template_string -from flask_security.core import AnonymousUser from flask_login.utils import current_user -import logging from helperFunctions.dataConversion import none_to_none from helperFunctions.fileSystem import get_src_dir from helperFunctions.mongo_task_conversion import check_for_errors, convert_analysis_task_to_fw_obj, create_re_analyze_task -from helperFunctions.web_interface import ConnectTo, get_template_as_string -from helperFunctions.web_interface import overwrite_default_plugins +from helperFunctions.web_interface import ConnectTo, get_template_as_string, is_superuser, overwrite_default_plugins from intercom.front_end_binding import InterComFrontEndBinding from objects.firmware import Firmware from storage.db_interface_admin import AdminDbInterface @@ -50,13 +47,6 @@ def _get_firmware_ids_including_this_file(fo): else: return list(fo.get_virtual_file_paths().keys()) - @staticmethod - def _is_superuser(user): - if isinstance(user._get_current_object(), AnonymousUser): - return True - else: - return user.has_role('superuser') - @roles_accepted(*PRIVILEGES['view_analysis']) def _show_analysis_results(self, uid, selected_analysis=None, root_uid=None): root_uid = none_to_none(root_uid) @@ -89,7 +79,7 @@ def _show_analysis_results(self, uid, selected_analysis=None, root_uid=None): analysis_plugin_dict=analysis_plugins, other_versions=other_versions, uids_for_comparison=uids_for_comparison, - show_admin_panal=self._is_superuser(current_user)) + show_admin_panal=is_superuser(current_user)) else: return render_template('uid_not_found.html', uid=uid) From d93fa7d93eed3dfd4bf984cc87955cb8a2e15e01 Mon Sep 17 00:00:00 2001 From: Peter Weidenbach Date: Tue, 10 Apr 2018 14:04:08 +0200 Subject: [PATCH 3/7] tests added --- src/test/acceptance/test_analyze_firmware.py | 1 + .../helperFunctions/test_web_interface.py | 46 +++++++++++++++---- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/test/acceptance/test_analyze_firmware.py b/src/test/acceptance/test_analyze_firmware.py index cda9e14de..947a93768 100644 --- a/src/test/acceptance/test_analyze_firmware.py +++ b/src/test/acceptance/test_analyze_firmware.py @@ -77,6 +77,7 @@ def _show_analysis_page(self): self.assertIn(b'test_vendor', rv.data) self.assertIn(b'unknown', rv.data) self.assertIn(self.test_fw_a.file_name.encode(), rv.data, 'file name not found') + self.assertIn(b'admin options:', rv.data, 'admin options not shown with disabled auth') def _check_ajax_file_tree_routes(self): rv = self.test_client.get('/ajax_tree/{}/{}'.format(self.test_fw_a.uid, self.test_fw_a.uid)) diff --git a/src/test/unit/helperFunctions/test_web_interface.py b/src/test/unit/helperFunctions/test_web_interface.py index 0edc4e018..65b8bd045 100644 --- a/src/test/unit/helperFunctions/test_web_interface.py +++ b/src/test/unit/helperFunctions/test_web_interface.py @@ -1,13 +1,41 @@ -import unittest +# -*- coding: utf-8 -*- +import pytest -from helperFunctions.web_interface import filter_out_illegal_characters +from helperFunctions.web_interface import filter_out_illegal_characters, is_superuser +from flask_security.core import AnonymousUser, UserMixin, RoleMixin +from werkzeug.local import LocalProxy -class TestHelperFunctionsWebInterface(unittest.TestCase): +@pytest.mark.parametrize('input_data, expected', [ + ('', ''), + ('abc', 'abc'), + ('Größer 2', 'Größer 2'), + ('{"$test": ["test"]}', 'test test'), + (None, None) +]) +def test_filter_out_illegal_characters(input_data, expected): + assert filter_out_illegal_characters(input_data) == expected - def test_filter_out_illegal_characters(self): - self.assertEqual(filter_out_illegal_characters(''), '') - self.assertEqual(filter_out_illegal_characters('abc'), 'abc') - self.assertEqual(filter_out_illegal_characters('Größer 2'), 'Größer 2') - self.assertEqual(filter_out_illegal_characters('{"$test": ["test"]}'), 'test test') - self.assertEqual(filter_out_illegal_characters(None), None) + +class role_superuser(RoleMixin): + name = 'superuser' + + +class superuser_user(UserMixin): + id = 1 + roles = [role_superuser] + + +class normal_user(UserMixin): + id = 2 + roles = [] + + +@pytest.mark.parametrize('input_data, expected', [ + (AnonymousUser, True), + (superuser_user, True), + (normal_user, False) +]) +def test_is_superuser_without_auth(input_data, expected): + proxied_object = LocalProxy(input_data) + assert is_superuser(proxied_object) == expected From c437d02eadcc669f0d86b8889bdb037a01560466 Mon Sep 17 00:00:00 2001 From: Peter Weidenbach Date: Tue, 10 Apr 2018 14:06:43 +0200 Subject: [PATCH 4/7] cleanup --- src/test/unit/helperFunctions/test_web_interface.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/unit/helperFunctions/test_web_interface.py b/src/test/unit/helperFunctions/test_web_interface.py index 65b8bd045..9d4c8cbd5 100644 --- a/src/test/unit/helperFunctions/test_web_interface.py +++ b/src/test/unit/helperFunctions/test_web_interface.py @@ -1,10 +1,11 @@ # -*- coding: utf-8 -*- import pytest -from helperFunctions.web_interface import filter_out_illegal_characters, is_superuser from flask_security.core import AnonymousUser, UserMixin, RoleMixin from werkzeug.local import LocalProxy +from helperFunctions.web_interface import filter_out_illegal_characters, is_superuser + @pytest.mark.parametrize('input_data, expected', [ ('', ''), From 93d546dff24c2464afd1da80bb637972c25065ff Mon Sep 17 00:00:00 2001 From: Peter Weidenbach Date: Tue, 10 Apr 2018 14:07:21 +0200 Subject: [PATCH 5/7] cleanup --- src/test/unit/helperFunctions/test_web_interface.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/unit/helperFunctions/test_web_interface.py b/src/test/unit/helperFunctions/test_web_interface.py index 9d4c8cbd5..99c99c672 100644 --- a/src/test/unit/helperFunctions/test_web_interface.py +++ b/src/test/unit/helperFunctions/test_web_interface.py @@ -37,6 +37,6 @@ class normal_user(UserMixin): (superuser_user, True), (normal_user, False) ]) -def test_is_superuser_without_auth(input_data, expected): +def test_is_superuser(input_data, expected): proxied_object = LocalProxy(input_data) assert is_superuser(proxied_object) == expected From be44f61a718fa4332488f7c43e50519234999e2e Mon Sep 17 00:00:00 2001 From: Peter Weidenbach Date: Thu, 12 Apr 2018 10:04:36 +0200 Subject: [PATCH 6/7] changes requested by reviewer --- src/helperFunctions/web_interface.py | 11 ++++++----- src/test/unit/helperFunctions/test_web_interface.py | 4 ++-- src/web_interface/components/analysis_routes.py | 4 ++-- src/web_interface/templates/show_analysis.html | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/helperFunctions/web_interface.py b/src/helperFunctions/web_interface.py index 00ec0ac45..db3369e84 100644 --- a/src/helperFunctions/web_interface.py +++ b/src/helperFunctions/web_interface.py @@ -80,8 +80,9 @@ def get_template_as_string(view_name): return get_binary_from_file(path).decode('utf-8') -def is_superuser(user): - if isinstance(user._get_current_object(), AnonymousUser): # auth disabled - return True - else: - return user.has_role('superuser') +def _auth_is_disabled(user): + return isinstance(user._get_current_object(), AnonymousUser) + + +def user_has_admin_clearance(user): + return _auth_is_disabled(user) or user.has_role('superuser') diff --git a/src/test/unit/helperFunctions/test_web_interface.py b/src/test/unit/helperFunctions/test_web_interface.py index 99c99c672..d7225270d 100644 --- a/src/test/unit/helperFunctions/test_web_interface.py +++ b/src/test/unit/helperFunctions/test_web_interface.py @@ -4,7 +4,7 @@ from flask_security.core import AnonymousUser, UserMixin, RoleMixin from werkzeug.local import LocalProxy -from helperFunctions.web_interface import filter_out_illegal_characters, is_superuser +from helperFunctions.web_interface import filter_out_illegal_characters, user_has_admin_clearance @pytest.mark.parametrize('input_data, expected', [ @@ -39,4 +39,4 @@ class normal_user(UserMixin): ]) def test_is_superuser(input_data, expected): proxied_object = LocalProxy(input_data) - assert is_superuser(proxied_object) == expected + assert user_has_admin_clearance(proxied_object) == expected diff --git a/src/web_interface/components/analysis_routes.py b/src/web_interface/components/analysis_routes.py index bbf6ffaf5..15bc9b15d 100644 --- a/src/web_interface/components/analysis_routes.py +++ b/src/web_interface/components/analysis_routes.py @@ -10,7 +10,7 @@ from helperFunctions.dataConversion import none_to_none from helperFunctions.fileSystem import get_src_dir from helperFunctions.mongo_task_conversion import check_for_errors, convert_analysis_task_to_fw_obj, create_re_analyze_task -from helperFunctions.web_interface import ConnectTo, get_template_as_string, is_superuser, overwrite_default_plugins +from helperFunctions.web_interface import ConnectTo, get_template_as_string, user_has_admin_clearance, overwrite_default_plugins from intercom.front_end_binding import InterComFrontEndBinding from objects.firmware import Firmware from storage.db_interface_admin import AdminDbInterface @@ -79,7 +79,7 @@ def _show_analysis_results(self, uid, selected_analysis=None, root_uid=None): analysis_plugin_dict=analysis_plugins, other_versions=other_versions, uids_for_comparison=uids_for_comparison, - show_admin_panal=is_superuser(current_user)) + user_has_admin_clearance=user_has_admin_clearance(current_user)) else: return render_template('uid_not_found.html', uid=uid) diff --git a/src/web_interface/templates/show_analysis.html b/src/web_interface/templates/show_analysis.html index 29bdd4ae6..987d526bf 100644 --- a/src/web_interface/templates/show_analysis.html +++ b/src/web_interface/templates/show_analysis.html @@ -407,7 +407,7 @@

Analysis for {{ firmware.get_hid(root_uid=root_ui

{# admin buttons #} - {% if firmware.vendor and show_admin_panal %} + {% if firmware.vendor and user_has_admin_clearance %}
admin options: From 8dd44b55a9924dc00621af2746e5a1eff0c5ddfc Mon Sep 17 00:00:00 2001 From: Peter Weidenbach Date: Thu, 12 Apr 2018 10:58:35 +0200 Subject: [PATCH 7/7] role generalized to privilege --- src/helperFunctions/web_interface.py | 5 +++-- src/test/unit/helperFunctions/test_web_interface.py | 4 ++-- src/web_interface/components/analysis_routes.py | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/helperFunctions/web_interface.py b/src/helperFunctions/web_interface.py index db3369e84..fd69a0d0f 100644 --- a/src/helperFunctions/web_interface.py +++ b/src/helperFunctions/web_interface.py @@ -8,6 +8,7 @@ from itertools import chain from helperFunctions.fileSystem import get_template_dir +from web_interface.security.privileges import PRIVILEGES SPECIAL_CHARACTERS = 'ÄäÀàÁáÂâÃãÅåǍǎĄąĂăÆæĀāÇçĆćĈĉČčĎđĐďðÈèÉéÊêËëĚěĘęĖėĒēĜĝĢģĞğĤĥÌìÍíÎîÏïıĪīĮįĴĵĶķĹĺĻļŁłĽľÑñŃńŇňŅņÖöÒòÓóÔôÕõŐőØøŒœŔŕŘřẞߌśŜŝŞşŠšȘș' \ @@ -84,5 +85,5 @@ def _auth_is_disabled(user): return isinstance(user._get_current_object(), AnonymousUser) -def user_has_admin_clearance(user): - return _auth_is_disabled(user) or user.has_role('superuser') +def user_has_privilege(user, privilege='delete'): + return _auth_is_disabled(user) or any(user.has_role(role) for role in PRIVILEGES[privilege]) diff --git a/src/test/unit/helperFunctions/test_web_interface.py b/src/test/unit/helperFunctions/test_web_interface.py index d7225270d..a7d6aa956 100644 --- a/src/test/unit/helperFunctions/test_web_interface.py +++ b/src/test/unit/helperFunctions/test_web_interface.py @@ -4,7 +4,7 @@ from flask_security.core import AnonymousUser, UserMixin, RoleMixin from werkzeug.local import LocalProxy -from helperFunctions.web_interface import filter_out_illegal_characters, user_has_admin_clearance +from helperFunctions.web_interface import filter_out_illegal_characters, user_has_privilege @pytest.mark.parametrize('input_data, expected', [ @@ -39,4 +39,4 @@ class normal_user(UserMixin): ]) def test_is_superuser(input_data, expected): proxied_object = LocalProxy(input_data) - assert user_has_admin_clearance(proxied_object) == expected + assert user_has_privilege(proxied_object) == expected diff --git a/src/web_interface/components/analysis_routes.py b/src/web_interface/components/analysis_routes.py index 15bc9b15d..8042ab834 100644 --- a/src/web_interface/components/analysis_routes.py +++ b/src/web_interface/components/analysis_routes.py @@ -10,7 +10,7 @@ from helperFunctions.dataConversion import none_to_none from helperFunctions.fileSystem import get_src_dir from helperFunctions.mongo_task_conversion import check_for_errors, convert_analysis_task_to_fw_obj, create_re_analyze_task -from helperFunctions.web_interface import ConnectTo, get_template_as_string, user_has_admin_clearance, overwrite_default_plugins +from helperFunctions.web_interface import ConnectTo, get_template_as_string, overwrite_default_plugins, user_has_privilege from intercom.front_end_binding import InterComFrontEndBinding from objects.firmware import Firmware from storage.db_interface_admin import AdminDbInterface @@ -79,7 +79,7 @@ def _show_analysis_results(self, uid, selected_analysis=None, root_uid=None): analysis_plugin_dict=analysis_plugins, other_versions=other_versions, uids_for_comparison=uids_for_comparison, - user_has_admin_clearance=user_has_admin_clearance(current_user)) + user_has_admin_clearance=user_has_privilege(current_user, privilege='delete')) else: return render_template('uid_not_found.html', uid=uid)