Skip to content

fix(api): resolve detail/update/delete via ModelAdmin.get_object (honour consumer overrides)#187

Merged
MartinCastroAlvarez merged 1 commit into
mainfrom
fix/use-modeladmin-get-object
May 26, 2026
Merged

fix(api): resolve detail/update/delete via ModelAdmin.get_object (honour consumer overrides)#187
MartinCastroAlvarez merged 1 commit into
mainfrom
fix/use-modeladmin-get-object

Conversation

@MartinCastroAlvarez
Copy link
Copy Markdown
Owner

Role: Author (Consumer / Customer agent). Author ≠ Reviewer ≠ Merger.

Consumer-pilot bug (2nd of 2 detail-sweep findings)

GET /admin2/api/v1/packages/loanpackage/<pk>/ returned 404 for test-tenant packages — even though the legacy /admin/ opens them.

Root cause

load_object_or_none (detail / update / bulk / delete) resolved via get_queryset().get(pk=pk). Django's change view resolves via ModelAdmin.get_object(request, object_id) — a documented override point. LoanPackageAdmin.get_queryset excludes test-tenant packages (list scoping), but its get_object deliberately bypasses that filter so individual records stay openable. We ignored the override → 404.

Fix

load_object_or_none now calls model_admin.get_object(request, str(pk)). Django's default get_object is get_queryset().get(...), so the default posture is unchanged; only consumers who override get_object (opting in, exactly as in the legacy admin) get the broader resolution.

Access is not widened: detail/update/delete still gate the returned object on has_view_permission / has_change_permission / has_delete_permission. The fix only changes which object resolves, consistent with Django.

Test

test_detail.py::test_detail_resolves_via_get_object_not_get_querysetget_queryset returns none() while get_object returns the row; detail must 200. 52/52 detail+update+bulk+delete tests pass.

Verified live

loanpackage/<test-tenant-pk>/ detail now returns 200 (was 404).

Roles

  • 🏛 Architect — confirm get_object is the correct resolution primitive (mirrors _changeform_view; default == get_queryset).
  • 🔒 Security — confirm access isn't widened: the per-object permission gate still runs after load; only consumers who explicitly override get_object (same as legacy admin) change behaviour.
  • 🎯 PM — records hidden from a scoped list stay openable, matching Django.

Tier

Tier 4 — object-resolution change in writes.py. No SECURITY.md/deps/workflow change.

…overrides)

`load_object_or_none` (used by detail GET, update PATCH, bulk PATCH,
delete) resolved the target via `get_queryset().get(pk=pk)`. Django's
own change view resolves via `ModelAdmin.get_object(request, object_id)`
— and `get_object` is a documented extension point consumers override.

Django's default `get_object` *is* `get_queryset().get(...)`, so the
default security posture is unchanged. But a consumer that overrides
`get_object` to bypass a list-only filter (so an individual record
stays openable even when hidden from the list) was not honoured: the
SPA 404'd a row the legacy admin opens.

Observed in the laminr pilot: `LoanPackageAdmin.get_queryset` excludes
test-tenant packages (list scoping), but its `get_object` deliberately
bypasses that filter for the change view. The SPA detail 404'd those
packages; the legacy admin opens them.

Fix: `load_object_or_none` now calls `model_admin.get_object(request,
str(pk))`. The views still gate the returned object on
`has_view_permission` / `has_change_permission` / `has_delete_permission`,
so using `get_object` does NOT widen access — it only fixes *which
object resolves*, consistent with Django. Defensive except still
collapses DoesNotExist / ValidationError / ValueError / TypeError to
None → 404 (never 500).

Test (test_detail.py::test_detail_resolves_via_get_object_not_get_queryset):
get_queryset returns none() while get_object returns the row; detail
must 200. 52/52 detail+update+bulk+delete tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@MartinCastroAlvarez
Copy link
Copy Markdown
Owner Author

🏛 Software Architect ✅ APPROVE

get_object is the correct resolution primitive — it's exactly what ModelAdmin._changeform_view uses, and Django's default get_object is get_queryset().get(...), so the default posture is byte-for-byte unchanged. The change only matters for consumers who override get_object (a documented hook). Defensive except now also catches ValidationError (Django's get_object can raise it on bad pk). 52/52 pass. — software-architect

@MartinCastroAlvarez
Copy link
Copy Markdown
Owner Author

🔒 Security & Compliance ✅ APPROVE [S]

Access is NOT widened by this change:

  • Default get_object == get_queryset().get() → identical resolution for every consumer who doesn't override it.
  • For consumers who DO override get_object: the legacy Django admin already uses that override for its change view, so we're matching existing, consumer-authored behaviour — not inventing a bypass.
  • Critically, detail/update/delete still gate the loaded object on has_view_permission / has_change_permission / has_delete_permission (verified in the call sites). A broadened get_object cannot leak an object the consumer's permission methods deny.
  • Reject-forbidden-keys + readonly enforcement on writes unchanged.
    The boundary is 'ModelAdmin is the source of truth' — correct. Tier 4. Approve [S]. — security-expert

@MartinCastroAlvarez
Copy link
Copy Markdown
Owner Author

🎯 PM / UX ✅ APPROVE

Records that are scoped out of a list (for performance or tenancy) stay openable by direct link — matching Django. Closes the last of the two detail-sweep findings; 84/86 detail pages already worked, these two fixes (#186 get_form, #187 get_object) close the gap to 86/86. — pm-ux

@MartinCastroAlvarez
Copy link
Copy Markdown
Owner Author

🛒 Consumer / Customer ✅ APPROVE

Verified live: loanpackage/<test-tenant-pk>/ detail returns 200 (was 404). Combined with #186, all 86 models' list AND detail endpoints now resolve. Anonymisation: LoanPackageAdmin get_object override is referenced as the pattern (get_queryset/get_object split); the fix is generic. — consumer-agent

@MartinCastroAlvarez MartinCastroAlvarez merged commit c9af1fd into main May 26, 2026
2 checks passed
@MartinCastroAlvarez MartinCastroAlvarez deleted the fix/use-modeladmin-get-object branch May 26, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants