Skip to content

Commit 79cef36

Browse files
authored
Merge pull request #830 from DataIntegrationGroup/chore/harden-api-authorization
chore(auth): harden Authentik authorization
2 parents ad37f78 + e7e89ba commit 79cef36

13 files changed

Lines changed: 687 additions & 125 deletions

.env.example

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,17 +80,28 @@ MODE=development
8080
# ENABLE_PG_CRON=0
8181

8282
# disable authentication (for development only)
83+
#
84+
# Honored ONLY when MODE=development. With any other MODE (including unset or
85+
# "staging"), the app refuses to start -- core.permissions.assert_auth_configuration()
86+
# raises AuthConfigurationError rather than serving every endpoint anonymously.
8387
AUTHENTIK_DISABLE_AUTHENTICATION=1
8488

8589
# erase and rebuild the database for step tests
8690
REBUILD_DB=1
8791

8892
# authentik
93+
# AUTHENTIK_URL is both the JWKS base and the expected `iss` claim; trailing
94+
# slash optional, both spellings are accepted.
8995
AUTHENTIK_URL=
9096
AUTHENTIK_CLIENT_ID=
9197
AUTHENTIK_AUTHORIZE_URL=
9298
AUTHENTIK_TOKEN_URL=
9399

100+
# How long a fetched JWKS document is trusted, in seconds (default 3600).
101+
# An unrecognized `kid` forces one immediate refresh regardless, so this only
102+
# bounds how long a revoked key stays usable.
103+
# AUTHENTIK_JWKS_TTL_SECONDS=3600
104+
94105

95106
# feedback endpoint (POST /feedback) — bug reports and feature requests
96107
JIRA_BASE_URL=https://nmbgmr.atlassian.net

CLAUDE.md

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,8 +155,31 @@ The system uses **Authentik** for OAuth2 authentication with role-based access c
155155
- **Editor**: Can modify existing records (includes Viewer permissions)
156156
- **Admin**: Can create new records (includes Editor + Viewer permissions)
157157

158+
The hierarchy is enforced in code, via `authenticated(any_of=[...])` group lists —
159+
`Admin` satisfies an editor- or viewer-gated route without needing all three
160+
Authentik groups granted.
161+
158162
**AMP-Specific Roles**: `AMPAdmin`, `AMPEditor`, `AMPViewer` for legacy AMPAPI integration
159163

164+
**Role families are orthogonal**: general `Admin` confers nothing in the AMP or
165+
Lexicon families. Only tiers *within* a family nest.
166+
167+
**Authorization is opt-in per endpoint** — a `user: <role>_dependency` parameter
168+
in the signature, not a router-level `dependencies=[...]`. Omitting it produces a
169+
fully public endpoint with no error. `tests/test_authorization.py` holds the
170+
allowlist of intentionally anonymous routes and fails on anything else. Note the
171+
annotation must be a *type annotation* (`user: viewer_dependency`), never a
172+
default value (`user=viewer_dependency`) — the latter silently disables the
173+
dependency and FastAPI treats it as a query parameter.
174+
175+
**Development bypass**: `AUTHENTIK_DISABLE_AUTHENTICATION=1` is honored only when
176+
`MODE=development`. Any other `MODE` (including unset) makes
177+
`assert_auth_configuration()` abort startup.
178+
179+
**`@in_public_schema`** (`core/app.py`) controls anonymous OpenAPI visibility
180+
only — it grants no access and removes no dependency. Apply it only to routes
181+
that genuinely have none.
182+
160183
### Database Configuration
161184

162185
The application supports two database modes (configured via `DB_DRIVER` in `.env`):

api/disclaimer.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
from fastapi import APIRouter, Query, Request
3232
from fastapi.responses import HTMLResponse, JSONResponse
3333

34+
from core.app import in_public_schema
3435
from core.disclaimer import (
3536
DISCLAIMER_CONTACT_EMAIL,
3637
DISCLAIMER_PARAGRAPHS,
@@ -84,6 +85,7 @@ def _render_html() -> str:
8485
)
8586

8687

88+
@in_public_schema
8789
@router.get(
8890
"/disclaimer",
8991
response_class=HTMLResponse,

api/feedback.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ def _build_slack_payload(payload: FeedbackCreate, jira_key: str, jira_url: str)
225225
@router.post("", response_model=FeedbackResponse)
226226
async def create_feedback(
227227
payload: FeedbackCreate,
228-
_user=viewer_dependency,
228+
_user: viewer_dependency,
229229
):
230230
jira_base = os.environ["JIRA_BASE_URL"]
231231
jira_email = os.environ["JIRA_EMAIL"]

api/ngwmn.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
from fastapi import APIRouter
1717
from starlette.responses import Response
1818

19+
from core.app import in_public_schema
1920
from core.dependencies import session_dependency
2021
from services.ngwmn_helper import (
2122
make_waterlevels_response,
@@ -25,7 +26,13 @@
2526

2627
router = APIRouter(prefix="/ngwmn", tags=["NGWMN"])
2728

29+
# These three routes are intentionally anonymous: the federal NGWMN harvester
30+
# polls them without credentials. @in_public_schema documents that (and lists
31+
# them in /openapi.json) so tests/test_authorization.py can tell an intentional
32+
# public route from an endpoint that simply forgot its `user:` dependency.
2833

34+
35+
@in_public_schema
2936
@router.get(
3037
"/waterlevels/{pointid}",
3138
summary="Get waterlevels for a given pointid in the NGWMN format",
@@ -35,6 +42,7 @@ def read_ngwmn_waterlevels(pointid: str, db: session_dependency):
3542
return Response(content=data, media_type="application/xml")
3643

3744

45+
@in_public_schema
3846
@router.get(
3947
"/wellconstruction/{pointid}",
4048
summary="Get wellconstruction for a given pointid in the NGWMN format",
@@ -44,6 +52,7 @@ def read_ngwmn_wellconstruction(pointid: str, db: session_dependency):
4452
return Response(content=data, media_type="application/xml")
4553

4654

55+
@in_public_schema
4756
@router.get(
4857
"/lithology/{pointid}",
4958
summary="Get lithology for a given pointid in the NGWMN format",

api/thing.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
)
2828

2929
from api.pagination import CustomPage
30-
from core.app import public_route
3130
from core.dependencies import (
3231
session_dependency,
3332
admin_dependency,
@@ -349,7 +348,6 @@ def get_thing_id_links(
349348
return paginate(query=sql, conn=session)
350349

351350

352-
@public_route
353351
@router.get("/id-link/{link_id}", summary="Get thing links by link ID")
354352
def get_thing_id_links(
355353
user: viewer_dependency,
@@ -362,7 +360,6 @@ def get_thing_id_links(
362360
return simple_get_by_id(session, ThingIdLink, link_id)
363361

364362

365-
@public_route
366363
@router.get("", summary="Get all things", status_code=HTTP_200_OK)
367364
def get_things(
368365
user: viewer_dependency,

core/app.py

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
get_swagger_ui_oauth2_redirect_html,
2828
)
2929
from fastapi.openapi.utils import get_openapi
30+
from fastapi.routing import iter_route_contexts
3031
from sqlalchemy import text
3132
from sqlalchemy.orm import Session
3233

@@ -123,27 +124,32 @@ def public_openapi():
123124
routes=app.routes,
124125
)
125126

126-
# Keep only operations where the endpoint function is marked public.
127+
# Collect the operations whose endpoint carries @in_public_schema.
128+
#
129+
# This walks iter_route_contexts() rather than app.routes. Routes added
130+
# via app.include_router() are not flattened into app.routes -- they
131+
# live inside opaque _IncludedRouter branches -- so the previous
132+
# `next(r for r in app.routes if r.path == path)` lookup matched
133+
# nothing but the few endpoints declared directly on `app`, and
134+
# silently dropped every decorated router route from the public schema.
135+
# iter_route_contexts() is the same helper get_openapi() itself walks,
136+
# so prefixes resolve identically to the paths in `schema`.
137+
public_operations = set()
138+
for route_context in iter_route_contexts(app.routes):
139+
if not getattr(route_context.endpoint, "_in_public_schema", False):
140+
continue
141+
route_path = route_context.path_format or route_context.path
142+
for route_method in route_context.methods or ():
143+
public_operations.add((route_path, route_method.lower()))
144+
127145
new_paths = {}
128146
for path, path_item in schema["paths"].items():
129147
new_methods = {}
130148
for method, operation in path_item.items():
131-
route = next(
132-
(
133-
r
134-
for r in app.routes
135-
if getattr(r, "path", None) == path
136-
and method.upper() in getattr(r, "methods", set())
137-
),
138-
None,
139-
)
140-
if not route:
149+
if (path, method.lower()) not in public_operations:
141150
continue
142-
143-
endpoint = getattr(route, "endpoint", None)
144-
if getattr(endpoint, "_is_public", False):
145-
operation["security"] = []
146-
new_methods[method] = operation
151+
operation["security"] = []
152+
new_methods[method] = operation
147153

148154
if new_methods:
149155
new_paths[path] = new_methods
@@ -224,7 +230,7 @@ async def warmup():
224230
return {"status": "ok"}
225231

226232
@app.get("/health", tags=["meta"])
227-
@public_route
233+
@in_public_schema
228234
def health(response: Response, session: Session = Depends(get_db_session)):
229235
# Ping the database so a 200 actually proves PostGIS is reachable, not
230236
# just that the process is up. Uptime monitors / status pages assert on
@@ -248,9 +254,18 @@ def health(response: Response, session: Session = Depends(get_db_session)):
248254
return app
249255

250256

251-
def public_route(func):
252-
"""Mark a route as public for OpenAPI filtering."""
253-
setattr(func, "_is_public", True)
257+
def in_public_schema(func):
258+
"""Advertise a route in the anonymous OpenAPI schema (/openapi.json).
259+
260+
Schema visibility only -- this grants no access and removes no dependency.
261+
It was previously named `public_route`, which read like an authorization
262+
decorator; two `/thing` endpoints carried it *and* a `viewer_dependency`,
263+
so the public schema advertised operations that 401 for anonymous callers.
264+
265+
Apply it only to routes that genuinely have no auth dependency.
266+
tests/test_authorization.py asserts the two sets match exactly.
267+
"""
268+
setattr(func, "_in_public_schema", True)
254269
return func
255270

256271

core/dependencies.py

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,26 +34,35 @@
3434
Admin, can do everything Editor and Viewer can do
3535
+ create new objects
3636
37+
That hierarchy is enforced here, by `any_of=` group lists rather than by
38+
Authentik group membership overlap: an Admin-only account satisfies an
39+
editor- or viewer-gated route because "Admin" appears in those lists. Before
40+
this was explicit, `authenticated(permissions=["Viewer"])` required the
41+
literal Viewer group, so the hierarchy held only as long as whoever
42+
provisioned the Authentik groups granted all three tiers to every admin.
43+
44+
The three families below are deliberately orthogonal -- general `Admin` does
45+
not confer `AMPAdmin` or `LexiconAdmin`. Only tiers *within* a family nest.
3746
"""
3847

3948
# General Purpose Authentication/Permissions -----------------------------------
4049

41-
admin_function = authenticated(permissions=["Admin"])
42-
editor_function = authenticated(permissions=["Editor"])
43-
viewer_function = authenticated(permissions=["Viewer"])
50+
admin_function = authenticated(any_of=["Admin"])
51+
editor_function = authenticated(any_of=["Admin", "Editor"])
52+
viewer_function = authenticated(any_of=["Admin", "Editor", "Viewer"])
4453

4554

4655
# AMP-Specific Authentication/Permissions --------------------------------------
4756

48-
amp_admin_function = authenticated(permissions=["AMPAdmin"])
49-
amp_editor_function = authenticated(permissions=["AMPEditor"])
50-
amp_viewer_function = authenticated(permissions=["AMPViewer"])
57+
amp_admin_function = authenticated(any_of=["AMPAdmin"])
58+
amp_editor_function = authenticated(any_of=["AMPAdmin", "AMPEditor"])
59+
amp_viewer_function = authenticated(any_of=["AMPAdmin", "AMPEditor", "AMPViewer"])
5160

5261

5362
# Lexicon-Specific Authentication/Permissions ----------------------------------
5463

55-
lexicon_admin_function = authenticated(permissions=["LexiconAdmin"])
56-
lexicon_editor_function = authenticated(permissions=["LexiconEditor"])
64+
lexicon_admin_function = authenticated(any_of=["LexiconAdmin"])
65+
lexicon_editor_function = authenticated(any_of=["LexiconAdmin", "LexiconEditor"])
5766

5867

5968
# OGC-Internal Authentication/Permissions --------------------------------------
@@ -63,7 +72,9 @@
6372

6473

6574
# Testing-Specific Authentication/Permissions ----------------------------------
66-
no_permission_function = authenticated(permissions=["NoPermission"])
75+
# A group nobody is ever granted, so this dependency always 403s. Used to
76+
# assert that group enforcement is actually wired up.
77+
no_permission_function = authenticated(any_of=["NoPermission"])
6778

6879

6980
# Permissions Dependencies -----------------------------------------------------

core/factory.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,15 @@ def initialize_runtime() -> None:
4040

4141
def create_api_app():
4242
initialize_runtime()
43+
44+
# After initialize_runtime()'s load_dotenv(), so MODE and
45+
# AUTHENTIK_DISABLE_AUTHENTICATION are both resolved. Raises
46+
# AuthConfigurationError -- boot fails loudly rather than serving every
47+
# endpoint anonymously.
48+
from core.permissions import assert_auth_configuration
49+
50+
assert_auth_configuration()
51+
4352
app = create_base_app()
4453
register_api_routes(app)
4554
from core.pygeoapi import mount_pygeoapi, mount_pygeoapi_internal

core/internal_ogc_auth.py

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
"""
3030

3131
import json
32-
import os
3332

3433
from starlette.types import ASGIApp, Receive, Scope, Send
3534

@@ -77,18 +76,15 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
7776
await self.app(scope, receive, send)
7877
return
7978

80-
if int(os.environ.get("AUTHENTIK_DISABLE_AUTHENTICATION", 0)):
81-
if settings.mode == "production":
79+
if permissions.authentication_disabled():
80+
if settings.mode != permissions.BYPASS_ALLOWED_MODE:
8281
# HTTPException(424) (what core.permissions.authenticated()
8382
# raises for this same misconfiguration) means nothing from
8483
# raw ASGI code -- send the response directly so a
85-
# misconfigured production box degrades to "internal mount
86-
# always 424s" rather than crashing the worker.
84+
# misconfigured box degrades to "internal mount always 424s"
85+
# rather than crashing the worker.
8786
await _send_json(
88-
send,
89-
424,
90-
"Authentication is disabled in production mode. Set "
91-
"AUTHENTIK_DISABLE_AUTHENTICATION=0 to enable authentication.",
87+
send, 424, permissions.bypass_misconfiguration_detail()
9288
)
9389
return
9490
await self.app(scope, receive, send)

0 commit comments

Comments
 (0)