From f2fc1b89ec4926b1862470d3f7821f47bea0af4f Mon Sep 17 00:00:00 2001 From: Maciej Pienczyn Date: Thu, 5 Feb 2026 21:36:14 +0100 Subject: [PATCH] refactor(rbac): Complete RBAC migration - 154/154 admin routes protected - Add @role_required to 2 missing routes (krs_api PDF download, zopk milestones) - Add role-based menu visibility in admin bar (hide Users, Security, Benefits, Model Comparison, Debug from OFFICE_MANAGER users) - Inject SystemRole into Jinja2 context processor for template role checks - Replace is_admin checkbox with role select dropdown in user creation form - Migrate routes.py and routes_users_api.py from is_admin to SystemRole-based role assignment via set_role() - Add deprecation notice to is_admin database column - Add 23 RBAC unit tests (hierarchy, has_role, set_role, permissions) Co-Authored-By: Claude Opus 4.6 --- app.py | 1 + blueprints/admin/routes.py | 9 +- blueprints/admin/routes_krs_api.py | 1 + blueprints/admin/routes_users_api.py | 13 +- blueprints/admin/routes_zopk_timeline.py | 1 + database.py | 4 +- templates/admin/users.html | 22 ++- templates/base.html | 8 + tests/unit/test_rbac.py | 226 +++++++++++++++++++++++ 9 files changed, 265 insertions(+), 20 deletions(-) create mode 100644 tests/unit/test_rbac.py diff --git a/app.py b/app.py index 7a0d3b6..e7ea739 100644 --- a/app.py +++ b/app.py @@ -340,6 +340,7 @@ def inject_globals(): 'COMPANY_COUNT': COMPANY_COUNT_MARKETING, # Liczba podmiotów (cel marketingowy) 'is_staging': is_staging, 'staging_features': STAGING_TEST_FEATURES if is_staging else {}, + 'SystemRole': SystemRole, } diff --git a/blueprints/admin/routes.py b/blueprints/admin/routes.py index 9418e12..c5c9143 100644 --- a/blueprints/admin/routes.py +++ b/blueprints/admin/routes.py @@ -208,8 +208,11 @@ def admin_user_add(): ) db.add(new_user) - if data.get('is_admin', False): - new_user.set_role(SystemRole.ADMIN) + role_name = data.get('role', 'MEMBER') + try: + new_user.set_role(SystemRole[role_name]) + except KeyError: + new_user.set_role(SystemRole.MEMBER) db.commit() db.refresh(new_user) @@ -255,7 +258,7 @@ def admin_user_toggle_admin(user_id): return jsonify({ 'success': True, - 'is_admin': is_now_admin, + 'is_admin': is_now_admin, # Backward compat for frontend 'role': user.role, 'message': f"{'Nadano' if is_now_admin else 'Odebrano'} uprawnienia admina" }) diff --git a/blueprints/admin/routes_krs_api.py b/blueprints/admin/routes_krs_api.py index b920e70..75bd0f4 100644 --- a/blueprints/admin/routes_krs_api.py +++ b/blueprints/admin/routes_krs_api.py @@ -440,6 +440,7 @@ def api_krs_audit_batch(): @bp.route('/krs-api/pdf/') @login_required +@role_required(SystemRole.OFFICE_MANAGER) def api_krs_pdf_download(company_id): """ API: Download/serve KRS PDF file for a company. diff --git a/blueprints/admin/routes_users_api.py b/blueprints/admin/routes_users_api.py index 074ab7b..8574bc9 100644 --- a/blueprints/admin/routes_users_api.py +++ b/blueprints/admin/routes_users_api.py @@ -271,10 +271,12 @@ def admin_users_bulk_create(): is_active=True ) db.add(new_user) - # Set role based on AI parse result (supports both old is_admin and new role field) + # Set role based on AI parse result ai_role = user_data.get('role', 'MEMBER') - if ai_role == 'ADMIN' or user_data.get('is_admin', False): - new_user.set_role(SystemRole.ADMIN) + try: + new_user.set_role(SystemRole[ai_role]) + except KeyError: + new_user.set_role(SystemRole.MEMBER) db.flush() # Get the ID created.append({ @@ -336,10 +338,7 @@ def admin_users_change_role(): return jsonify({'success': False, 'error': 'Nie możesz odebrać sobie uprawnień administratora'}), 400 old_role = user.role - user.role = new_role - - # Sync is_admin flag - user.is_admin = (new_role == 'ADMIN') + user.set_role(SystemRole[new_role]) # Update company_role based on new role if new_role in ['MANAGER']: diff --git a/blueprints/admin/routes_zopk_timeline.py b/blueprints/admin/routes_zopk_timeline.py index 845cc8d..09c4b47 100644 --- a/blueprints/admin/routes_zopk_timeline.py +++ b/blueprints/admin/routes_zopk_timeline.py @@ -32,6 +32,7 @@ def admin_zopk_timeline(): @bp.route('/zopk-api/milestones') @login_required +@role_required(SystemRole.OFFICE_MANAGER) def api_zopk_milestones(): """API - lista kamieni milowych ZOPK.""" db = SessionLocal() diff --git a/database.py b/database.py index f7f4ccd..a989c38 100644 --- a/database.py +++ b/database.py @@ -283,10 +283,10 @@ class User(Base, UserMixin): # Role within assigned company (if any) company_role = Column(String(20), default='NONE', nullable=False) - # Status (is_admin kept for backward compatibility, synced with role) + # Status is_active = Column(Boolean, default=True) is_verified = Column(Boolean, default=False) - is_admin = Column(Boolean, default=False) # Deprecated: use role == ADMIN instead + is_admin = Column(Boolean, default=False) # DEPRECATED: synced by set_role() for backward compat. Use has_role(SystemRole.ADMIN) instead. Will be removed in future migration. is_norda_member = Column(Boolean, default=False) is_rada_member = Column(Boolean, default=False) # Member of Rada Izby (Board Council) diff --git a/templates/admin/users.html b/templates/admin/users.html index ea12835..6a378bf 100644 --- a/templates/admin/users.html +++ b/templates/admin/users.html @@ -1274,11 +1274,17 @@ {% endfor %} -
- +
+
+ + +
diff --git a/tests/unit/test_rbac.py b/tests/unit/test_rbac.py new file mode 100644 index 0000000..4c0be8f --- /dev/null +++ b/tests/unit/test_rbac.py @@ -0,0 +1,226 @@ +""" +RBAC Unit Tests +=============== + +Tests for the Role-Based Access Control system: +- SystemRole hierarchy +- has_role() method +- set_role() with is_admin sync +- Permission helper methods (can_access_admin_panel, can_manage_users, etc.) +- role_required decorator +""" + +import os +import sys +from unittest.mock import MagicMock, patch + +import pytest + +# Add project root to path +sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..')) + +from database import SystemRole, CompanyRole, User + + +# ============================================================ +# SystemRole Hierarchy Tests +# ============================================================ + +class TestSystemRoleHierarchy: + """Test that SystemRole enum values enforce correct hierarchy.""" + + def test_role_ordering(self): + assert SystemRole.UNAFFILIATED < SystemRole.MEMBER + assert SystemRole.MEMBER < SystemRole.EMPLOYEE + assert SystemRole.EMPLOYEE < SystemRole.MANAGER + assert SystemRole.MANAGER < SystemRole.OFFICE_MANAGER + assert SystemRole.OFFICE_MANAGER < SystemRole.ADMIN + + def test_role_values(self): + assert SystemRole.UNAFFILIATED == 10 + assert SystemRole.MEMBER == 20 + assert SystemRole.EMPLOYEE == 30 + assert SystemRole.MANAGER == 40 + assert SystemRole.OFFICE_MANAGER == 50 + assert SystemRole.ADMIN == 100 + + def test_from_string_valid(self): + assert SystemRole.from_string('ADMIN') == SystemRole.ADMIN + assert SystemRole.from_string('OFFICE_MANAGER') == SystemRole.OFFICE_MANAGER + assert SystemRole.from_string('MEMBER') == SystemRole.MEMBER + + def test_from_string_case_insensitive(self): + assert SystemRole.from_string('admin') == SystemRole.ADMIN + assert SystemRole.from_string('member') == SystemRole.MEMBER + + def test_from_string_invalid_defaults_to_unaffiliated(self): + assert SystemRole.from_string('INVALID') == SystemRole.UNAFFILIATED + assert SystemRole.from_string('') == SystemRole.UNAFFILIATED + + def test_enum_by_name(self): + assert SystemRole['ADMIN'] == SystemRole.ADMIN + assert SystemRole['MEMBER'] == SystemRole.MEMBER + with pytest.raises(KeyError): + SystemRole['INVALID'] + + +# ============================================================ +# User.has_role() Tests +# ============================================================ + +def _make_user(role_name='MEMBER', is_admin=False): + """Create a fake user object with User methods for testing RBAC logic. + + Cannot use User() directly since SA instrumented attributes require + a mapped session. Instead, use a plain object with User's methods bound. + """ + class FakeUser: + pass + + user = FakeUser() + user.role = role_name + user.is_admin = is_admin + user.company_role = 'NONE' + user.company_id = None + + # Bind User's property and methods + user.system_role = User.system_role.fget(user) + user.has_role = lambda required_role: User.has_role(user, required_role) + user.can_access_admin_panel = lambda: User.can_access_admin_panel(user) + user.can_manage_users = lambda: User.can_manage_users(user) + user.can_moderate_forum = lambda: User.can_moderate_forum(user) + return user + + +class TestHasRole: + """Test User.has_role() hierarchical check.""" + + def test_admin_has_all_roles(self): + user = _make_user('ADMIN') + assert user.has_role(SystemRole.ADMIN) + assert user.has_role(SystemRole.OFFICE_MANAGER) + assert user.has_role(SystemRole.MANAGER) + assert user.has_role(SystemRole.EMPLOYEE) + assert user.has_role(SystemRole.MEMBER) + assert user.has_role(SystemRole.UNAFFILIATED) + + def test_office_manager_cannot_access_admin(self): + user = _make_user('OFFICE_MANAGER') + assert not user.has_role(SystemRole.ADMIN) + assert user.has_role(SystemRole.OFFICE_MANAGER) + assert user.has_role(SystemRole.MANAGER) + assert user.has_role(SystemRole.MEMBER) + + def test_member_minimal_access(self): + user = _make_user('MEMBER') + assert not user.has_role(SystemRole.ADMIN) + assert not user.has_role(SystemRole.OFFICE_MANAGER) + assert not user.has_role(SystemRole.MANAGER) + assert not user.has_role(SystemRole.EMPLOYEE) + assert user.has_role(SystemRole.MEMBER) + assert user.has_role(SystemRole.UNAFFILIATED) + + def test_unaffiliated_lowest_access(self): + user = _make_user('UNAFFILIATED') + assert not user.has_role(SystemRole.MEMBER) + assert user.has_role(SystemRole.UNAFFILIATED) + + def test_none_role_defaults_to_unaffiliated(self): + user = _make_user(None) + assert user.has_role(SystemRole.UNAFFILIATED) + assert not user.has_role(SystemRole.MEMBER) + + +# ============================================================ +# User.set_role() Tests +# ============================================================ + +class TestSetRole: + """Test User.set_role() with is_admin sync. + + Uses a simple namespace object to test set_role logic directly, + since SQLAlchemy instrumented attributes don't work outside a session. + """ + + def _make_settable_user(self, role_name='MEMBER', is_admin=False): + """Create an object that can receive set_role() assignments.""" + class FakeUser: + pass + user = FakeUser() + user.role = role_name + user.is_admin = is_admin + # Bind set_role method from User class + user.set_role = lambda new_role, sync_is_admin=True: User.set_role(user, new_role, sync_is_admin) + return user + + def test_set_admin_syncs_is_admin_true(self): + user = self._make_settable_user('MEMBER', is_admin=False) + user.set_role(SystemRole.ADMIN) + assert user.role == 'ADMIN' + assert user.is_admin is True + + def test_set_member_syncs_is_admin_false(self): + user = self._make_settable_user('ADMIN', is_admin=True) + user.set_role(SystemRole.MEMBER) + assert user.role == 'MEMBER' + assert user.is_admin is False + + def test_set_office_manager_is_admin_false(self): + user = self._make_settable_user('ADMIN', is_admin=True) + user.set_role(SystemRole.OFFICE_MANAGER) + assert user.role == 'OFFICE_MANAGER' + assert user.is_admin is False + + def test_set_role_without_sync(self): + user = self._make_settable_user('MEMBER', is_admin=False) + user.set_role(SystemRole.ADMIN, sync_is_admin=False) + assert user.role == 'ADMIN' + assert user.is_admin is False # Not synced + + +# ============================================================ +# Permission Helper Methods Tests +# ============================================================ + +class TestPermissionHelpers: + """Test can_access_admin_panel, can_manage_users, can_moderate_forum.""" + + def test_admin_can_access_admin_panel(self): + assert _make_user('ADMIN').can_access_admin_panel() + + def test_office_manager_can_access_admin_panel(self): + assert _make_user('OFFICE_MANAGER').can_access_admin_panel() + + def test_manager_cannot_access_admin_panel(self): + assert not _make_user('MANAGER').can_access_admin_panel() + + def test_member_cannot_access_admin_panel(self): + assert not _make_user('MEMBER').can_access_admin_panel() + + def test_only_admin_can_manage_users(self): + assert _make_user('ADMIN').can_manage_users() + assert not _make_user('OFFICE_MANAGER').can_manage_users() + assert not _make_user('MANAGER').can_manage_users() + + def test_office_manager_can_moderate_forum(self): + assert _make_user('ADMIN').can_moderate_forum() + assert _make_user('OFFICE_MANAGER').can_moderate_forum() + assert not _make_user('MANAGER').can_moderate_forum() + + +# ============================================================ +# CompanyRole Tests +# ============================================================ + +class TestCompanyRole: + """Test CompanyRole enum and hierarchy.""" + + def test_role_ordering(self): + assert CompanyRole.NONE < CompanyRole.VIEWER + assert CompanyRole.VIEWER < CompanyRole.EMPLOYEE + assert CompanyRole.EMPLOYEE < CompanyRole.MANAGER + + def test_from_string(self): + assert CompanyRole.from_string('MANAGER') == CompanyRole.MANAGER + assert CompanyRole.from_string('EMPLOYEE') == CompanyRole.EMPLOYEE + assert CompanyRole.from_string('INVALID') == CompanyRole.NONE