diff --git a/docs/02-iam/rbac-model.md b/docs/02-iam/rbac-model.md index 851da2a..f6b7df6 100644 --- a/docs/02-iam/rbac-model.md +++ b/docs/02-iam/rbac-model.md @@ -25,6 +25,51 @@ grants are how an access-control system becomes unauditable: within a year nobod revoke an offer?" without a full-table scan and a guess. If someone needs a capability, either their role gets it or a new role exists. +### One basic role, any number of additional roles + +Roles are not interchangeable, and treating them as one flat list is what lets a student be made Dean +Academic. + +| | Basic role | Additional roles | +|---|---|---| +| How many | exactly one | any number, including none | +| Where from | ERP `globals_extrainfo.user_type` | ERP `globals_holdsdesignation` | +| Assignable | no — it is what a person *is* | yes | +| Values | `student`, `faculty`, `staff` | the 113 designations | + +The basic role is a role like any other in the grant tables — `RolePermission(designation="faculty", …)` +resolves for every faculty member without an ERP row saying so. Before this rule existed, 14 permissions +and 6 module grants were declared against `faculty` and reached nobody, and 121 staff who hold no +designation had no role at all. + +`active_role` defaults to the first additional role and falls back to the basic role, so a Junior +Assistant lands in their office rather than on a generic staff view. + +### Who may hold what + +`iam_role` catalogues each designation as a **rank**, an **office** or a **functional** role, and records +which basic roles may hold it: + +| Category | Example | May be held by | +|---|---|---| +| `basic` | `student` | itself | +| `rank` | Professor, Associate Professor | faculty | +| `office` | Dean Academic, HOD (CSE) | faculty | +| `office` | acadadmin, Registrar, Junior Assistant | faculty or staff | +| `functional` | co-ordinator, Convenor, mess_committee | anyone, students included | + +The functional row is the point: a student cannot be a Professor but can be a club co-ordinator. + +`sync_identity` checks every projected assignment against the catalogue and writes what fails to +`iam_role_violation`. It **reports** by default and only withholds the role when +`IAM_ENFORCE_ROLE_POLICY` is on — the catalogue is a claim about institute practice, and refusing before +that claim is confirmed would revoke access from whoever it is wrong about. An uncatalogued designation +is allowed, because the academic office adds designations without telling this service. + +Two live violations exist today: `23BCS265` (student) holds Dean Academic, which grants 54 permissions +including `curriculum.course.manage`; `ntripathi` (staff) holds Assistant Professor. Enforcing reduces +the first to 18. + **Two independent checks on every request:** | Check | Question | Source | Cost | diff --git a/modules/accesscontrol/management/commands/permission_manifest.py b/modules/accesscontrol/management/commands/permission_manifest.py index c26da99..31ea148 100644 --- a/modules/accesscontrol/management/commands/permission_manifest.py +++ b/modules/accesscontrol/management/commands/permission_manifest.py @@ -4,6 +4,7 @@ manage.py permission_manifest --check fail if stale or ungrantable """ import json +import re from importlib import import_module from pathlib import Path @@ -37,11 +38,24 @@ def collect() -> dict: for code, label in getattr(reg, "PERMISSIONS", []) ], "system_permissions": sorted(getattr(reg, "SYSTEM_PERMISSIONS", [])), + # Enforced by narrowing a queryset rather than by refusing a + # request. Holding one widens what you see; not holding it shows + # you less. There is no endpoint to check it in, so the + # unenforced check has to be told. + "scope_permissions": sorted(getattr(reg, "SCOPE_PERMISSIONS", [])), "grants": { designation: sorted(set(codes)) for designation, codes in sorted(getattr(reg, "ROLE_GRANTS", {}).items()) }, + # Derived, never listed separately. A designation holding any of a + # module's permissions must be able to enter the module — the two + # gates are separate checks, and seeding only one leaves every screen + # 403 with the permissions apparently correct. + "module_grants": sorted( + designation + for designation, codes in getattr(reg, "ROLE_GRANTS", {}).items() + if codes), } return {"version": VERSION, "modules": modules} @@ -51,6 +65,7 @@ def problems(manifest: dict) -> list[str]: for code, spec in manifest["modules"].items(): declared = {p["code"] for p in spec["permissions"]} system = set(spec["system_permissions"]) + scoped = set(spec.get("scope_permissions", [])) granted = {c for codes in spec["grants"].values() for c in codes} found.extend( @@ -71,6 +86,97 @@ def problems(manifest: dict) -> list[str]: f"{code}: {both} is both a system permission and granted to a " f"designation. Pick one." for both in sorted(system & granted)) + # None means the module's code could not be located, so there is + # nothing to judge. Reporting every permission as unenforced in that + # case is exactly the failure this check is meant to prevent. + # A system permission is enforced by whichever service calls it, which + # need not be inside this module, so it is out of scope here. + enforced = _enforced(code) + if enforced is not None: + found.extend( + f"{code}: {unused} is declared and granted, but no endpoint or " + f"service checks it — so the thing it names cannot be done at " + f"all. Either the feature is missing, or the permission is." + for unused in sorted( + declared - system - scoped - enforced - KNOWN_UNENFORCED)) + return found + + +def known_gaps(manifest: dict) -> list[str]: + """The allowlisted ones that are still true, so they stay countable. + + Reported on every run rather than suppressed. An allowlist nobody sees is + how a temporary exception becomes permanent. + """ + return sorted( + code for spec_code, spec in manifest["modules"].items() + for code in ({p["code"] for p in spec["permissions"]} + & KNOWN_UNENFORCED) - (_enforced(spec_code) or set())) + + + + + +#: Permissions declared before the check existed, whose feature has not +#: been built yet. Each is a real gap, listed so a NEW one fails CI +#: immediately instead of joining a pile nobody counts. Deleting a line +#: here is the definition of done for that feature. +KNOWN_UNENFORCED = frozenset({ +}) + + +def _module_paths() -> dict[str, Path]: + """Module code -> the directory its code lives in. + + Resolved through the app registry, not by joining the code onto a path: the + placement module's code is `placement_cell` while its package is + `modules.placement`, and guessing the directory found nothing and reported + every one of its 21 permissions as unenforced. + """ + out: dict[str, Path] = {} + for cfg in apps.get_app_configs(): + if not cfg.name.startswith("modules."): + continue + try: + reg = import_module(f"{cfg.name}.registry") + except ModuleNotFoundError: + continue + spec = getattr(reg, "MODULE", None) + out[spec["code"] if spec else cfg.label] = Path(cfg.path) + return out + + +def _enforced(module_code: str) -> set[str] | None: + """Permission codes that appear anywhere in the module's own code. + + A grep, deliberately: a permission is enforced by being passed to + HasPermission or authz.require, and both take a plain string. Anything + cleverer would be a static analysis that the next way of checking defeats. + + `selectors/` does not count. That is the architecture's own line: a + selector decides what you can see and authorises nothing, so a permission + appearing only there hid three `*.manage` permissions whose write path did + not exist. + + `domain/` does count. It holds the tables that say which permission a step + needs — STEP_PERMISSIONS, OFFICE_PERMISSIONS — and the service reads them + to make the check. That is authority stated once rather than inlined. + + registry.py is skipped because that is where they are declared, and tests + because a test naming a permission is not an endpoint enforcing it. + """ + root = _module_paths().get(module_code) + if root is None: + return None + found: set[str] = set() + for path in root.rglob("*.py"): + parts = set(path.parts) + if "tests" in parts or path.name == "registry.py": + continue + if "selectors" in parts: + continue + found.update(re.findall(rf"{re.escape(module_code)}\.[a-z_]+\.[a-z_]+", + path.read_text())) return found diff --git a/registry/permissions.json b/registry/permissions.json index 0f0ef21..8c2469c 100644 --- a/registry/permissions.json +++ b/registry/permissions.json @@ -18,12 +18,20 @@ "directory.user.search" ] }, + "module_grants": [ + "Dean Academic", + "acadadmin", + "placement_chairman", + "placement_coordinator", + "placement_officer" + ], "permissions": [ { "code": "directory.user.search", "label": "Search the institute directory" } ], + "scope_permissions": [], "system_permissions": [] }, "placement_cell": { @@ -81,6 +89,14 @@ "placement_cell.registration.self" ] }, + "module_grants": [ + "Dean Academic", + "acadadmin", + "placement_chairman", + "placement_coordinator", + "placement_officer", + "student" + ], "permissions": [ { "code": "placement_cell.job_posting.view", @@ -167,6 +183,7 @@ "label": "Browse every student's declared CPI" } ], + "scope_permissions": [], "system_permissions": [ "placement_cell.application.auto_withdraw", "placement_cell.offer.expire"