has_module_permission set to False on ModelAdmin raises 403 Forbidden error when visiting its app page

[bhch]

Suppose an app has 2 (or more) models, and we hide one model from the admin index page by returning False in has_module_permission method of ModelAdmin. Now, if we visit the admin index page, we see all the models under our app except the one we hid. So far so good.

But if we visit the app page (at /admin/<app_label>/) we get a 403 Forbidden error. Whereas one would expect to see the app page with the models listed there, except of course the hidden ones.

[Keryn Knight]

Chiming in as I believe I encouraged bhch to open this. I've not tested the ticket's validity, but I'm familiar with the start of the whole changeset, and looking at the code over time I can see how it may be correct. YMMV.

The notion of raising PermissionDenied originated in #21063 and landed in commit 0d74f9553c6acb8b53a502ca5e39542dcc4412c1, probably off the back of #21056 (which landed in 170f72136758add6c9c0c59240cfce73d5672cc2). Both are my doing.

The idea was that, knowing an app label was valid (either because of the urlconf changes having landed, or because Django held an idea of what constituted a good app label anyway) it could run once before doing the loop over the app's ModelAdmins and check whether there were any reason to carry on.

In 504c89e8008c557a1e83c45535b549f77a3503b2 (referencing #6327) the check was hoisted to be a member of a given ModelAdmin, which meant it had to happen within the loop of ModelAdmins. This subtley regressed the original check if one of the enumerated ModelAdmins returned a falsy value for has_module_permission such that now a single ModelAdmin may be responsible for causing a 403 for the whole app's index.

This looks to have happened between 1.7 and 1.8 and was ultimately brought forward when the app dicts were combined in bcf700b4e3393d8e72920ae64aa421c5651f8935 in 1.9. I can't find a ticket number for this last change, though.

I'd argue that the original check (mine, regrettably, as it suggests a bias :)) is correct, and the subsequent ones are incorrect for the purposes of displaying the app index.

[Halil Kaya]

I have opened a pull request for this ticket:
​https://github.com/django/django/pull/6999

[Tim Graham]

Please uncheck "Needs tests" if you can add one.

[Tim Graham]

​Updated PR. Keryn, can you check it?

[Tim Graham <timograham@…>]

In 2027d6a:

Fixed #26954 -- Prevented ModelAdmin.has_module_permission()=False from blocking access to the app index page.