OpenEnergyPlatform / OpenEnergyPlatform/oeplatform

OEDB endpoints enforce permissions outside DRF, so authorisation cannot be audited — and there is no deny-by-default

Open
#2,482 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
65
Forks
29
Avg merge
15h 25m
Merged PRs (30d)
32

Description

Description of the issue

The OEDB half of api/v0 enforces permissions in ways that nothing outside the function
body can see
. The result is that authorisation cannot be audited, reviewed, or tested as a
property of a view — only by reading each method and every function it calls.

Three mechanisms are in use across api/views.py, and only the third is introspectable:

  1. Hand-rolled method decorators@require_write_permission,
    @require_delete_permission, @require_admin_permission (api/helper.py:243-261),
    which call assert_permission(user=request.user, …).
  2. Checks inside the action function — the advanced/ family. create_ajax_handler
    (api/helper.py:293) builds an APIView with no permission classes and passes
    context = {"user": request.user} down; the action does the check, e.g.
    data_delete (api/actions.py:1784).
  3. DRF permission classes — used by the dataset endpoints (IsAuthenticated,
    IsAuthenticatedOrReadOnly) and by the OEKG scenario-bundle endpoints
    (get_permissions()).

Only the third is visible to anything but a human reader.

Why this is worth an issue of its own

There is no DEFAULT_PERMISSION_CLASSES in REST_FRAMEWORK
(oeplatform/settings.py:417-431). DRF's fallback is therefore AllowAny, so a view is
public unless someone remembered to add one of the three mechanisms above. Nothing fails
when they do not:

  • Review cannot catch it — an unguarded write method looks exactly like a guarded one
    minus a line, and the decorators sit among @api_exception and @method_decorator(...)
    that carry no security meaning.
  • The schema cannot catch it — drf-spectacular reads permission classes, so mechanisms 1
    and 2 are invisible to it. This is the root cause of #2481: 61 write operations are
    declared anonymous in the generated document because the generator genuinely cannot see
    their guards. A deliberately public read and a write whose guard was forgotten
    both render as {}, and are indistinguishable in the artifact.
  • Tests do not catch it — there is no test asserting that writes require
    authentication, and six existing tests hedge assertIn(code, (401, 403)) rather than
    pinning either, which is defensive rather than evidential.

So the safety of the OEDB API rests entirely on each contributor remembering an unenforced
convention, in a file where the three conventions are mixed.

Ideas of solution

1. Deny by default

Set DEFAULT_PERMISSION_CLASSES to IsAuthenticated and make every genuinely public
endpoint say so explicitly with AllowAny. This inverts the failure mode: forgetting a
guard produces a 401 that a test or a user reports, instead of an open endpoint nobody
notices. It is the single highest-value change here and it is a few lines plus an audit of
which reads are meant to be public — the generated schema already lists exactly those
(28 reads carry {}).

2. Express the existing guards as permission classes

The decorators already compute the right answer; wrapping them as DRF permission classes
(or declaring them with extend_schema(auth=...)) makes them visible to the generator, to
the browsable API, and to anything else reading DRF metadata. The advanced/ family is one
factory (create_ajax_handler), so it is one change for all 31 of its operations.

This is what closes #2481 properly — not by editing the artifact, which is generated and
guarded, but by making the document able to tell the truth.

3. A test that fails when a write is anonymous

Assert over the generated schema that no write operation declares anonymous access unless
it is on a short, explicit allowlist. This is the check that would turn "somebody forgot"
from an invisible state into a red build, and it is cheap because the artifact is already
committed and already regenerated in CI by #2457's guard.

Acceptance criteria
  • DEFAULT_PERMISSION_CLASSES set to a deny-by-default, with every intentionally public
    endpoint marked AllowAny explicitly.
  • The existing permission decorators and in-action checks are expressed so that DRF —
    and therefore the schema generator — can see them.
  • A test fails if a write operation in the generated schema permits anonymous access
    outside an explicit allowlist.
  • The 28 genuinely public reads keep their public declaration; this must not be "fixed"
    by requiring authentication everywhere.
  • Artifact regenerated, api/tests/test_openapi_schema.py green, changelog entry.

Relationship to other issues

  • #2481 is the symptom in the generated document. This issue is why it happens; fixing
    this fixes that.
  • #2454 / #2475 are the documentation effort, and this is not part of it — it is a
    change to how api/ expresses authorisation, not to how the API is described.

Note

Filed after an audit of every write method in api/views.py against its guard. That audit
produced one finding that is not described here and has been reported to the maintainer
separately rather than in a public issue.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with REST_FRAMEWORK in oeplatform/settings.py:417-431, then compare the permission decorators in api/helper.py:243-261, create_ajax_handler at api/helper.py:293, and the checks in api/actions.py:1784. Review api/views.py and api/tests/test_openapi_schema.py to understand existing endpoint declarations and schema checks. Done means intentional public reads remain explicit, writes are not anonymous, and the schema test and regenerated artifact pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
django, python
Domain
api, authentication, backend, security
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.