Skip to content

fix StudyMixin: avoid leaking study existance in API views

Bengfort requested to merge fix-study-mixin-404-leak into main

A typical view can respond with 403 when a users is not authenticated or does not have the necessary permissions and with 404 if the requested object does not exist.

It is important to get the order of those checks right. Responding with 404 too early can leak the existance of objects.

Studies are special because we need them in order to check permissions in study context. So the correct order is:

  1. authentication
  2. get study
  3. check permissions
  4. get other objects

The order of (2)-(4) works fine. (1) is also guarenteed to be first thanks to django-stronghold. However, for API views we disable django-stronghold and use custom authentication.

That custom authentication is run during dispatch() while the study is fetched in setup(), so this is actually the wrong way around.

I have some ideas how to implement this:

  • in dispatch
    • guaranteed to run (and potentially raise 404)
    • order of checks depends on order of mixins
    • StudyMixin must be in between APIAuthMixin und PermissionRequiredMixin
  • as a cached property
    • not guaranteed to run
    • order of checks depends on first use of self.study (probably correct, but no guarantees)
    • order of mixins doesn't matter
  • hook into PermissionRequiredMixin methods
    • guaranteed to run (as long as PermissionRequiredMixin is used)
    • run immediately before permission checks
    • order of mixins doesn't matter

I first went with the dispatch approach and did some related refactoring. In the end I switched to the third approach but liked the refactoring anyway, so I left it in.

Edited by Bengfort

Merge request reports