Opened 3 years ago

Closed 3 years ago

#19057 closed Bug (fixed)

mod_wsgi authentication handlers fail with custom user model

Reported by: russellm Owned by: nobody
Component: contrib.auth Version: master
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The mod_wsgi authentication handler introduced in r373932fa don't work if you have a custom User model.

I can't see any fundamental reason that they shouldn't be able to be made to work for the general case. At the very least, the tests should be marked @skipIfCustom

Marking release blocker because it's a bug in a new feature.

Change History (10)

comment:1 Changed 3 years ago by ptone

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

comment:2 Changed 3 years ago by Preston Holmes <preston@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 5f8b97f9fb058e5e02f1f99423fc3b0020ecdeb0:

Fixed #19057 -- support custom user models in mod_wsgi auth handler

thanks @freakboy3742 for the catch and review

comment:3 Changed 3 years ago by russellm

  • Resolution fixed deleted
  • Status changed from closed to reopened

Work is still required; the committed test:

1) doesn't check the behavior for custom users, and
2) assumes that User is always available.

comment:4 Changed 3 years ago by ptone

  • Triage Stage changed from Ready for checkin to Accepted

This should hopefully address point 1:

But I'm unclear as to point 2

There are lots of places in the test suite that still refer to a hard coded django.contrib.auth.models.User

We still need to have our coverage for the default user model, and it doesn't harmful to leave those concrete references in tests as long as the custom user option is also tested.

Are you suggesting we adopt the same UserModel = auth.get_user_model pattern for all test code that test user behavior? That would seem to leave the default user model untested if run in a project with a custom auth user.

comment:5 Changed 3 years ago by ptone

ref #19050 - I think that actually clarifies the issue for point 2 from comment 3 above

will investigate with test project I have on hand that sets auth user model setting

comment:6 Changed 3 years ago by ptone

seeing the light now on point 2 after running it with a sample project where user model swapped out - this should be fixed with:

I'm wondering if we should have something like this set up on the CI server - not too many tickets will bump against this issue - but until you get into the mindset that User isn't always available in the test suite (though User._meta.installed == True), its hard to remember that it is not available in tests the way it always has been.

also I'm going to look at if we can improve the error, which currently is AttributeError: type object 'User' has no attribute 'objects' vs DatabaseError: no such table: <...> for non-installed models.

OK so the issue the manager not being there is due to a check of 'swapped' in setting up the manager:

Though I haven't run the whole suite I can take out those, checks, restoring the standard manager behavior and no tests fail that I might expect to fail.

Obviously these tests on this issue will change depending on the outcome of #19061 - currently I do NOT assume is_valid is true - since there might be some other way a user is invalid, and I want to default prevent access. Of course raising a NotImplemented would be viable here as well.

comment:7 Changed 3 years ago by russellm

Ok - I put *in* the swapped checks by request during the review process because the feedback I got was that "table does not exist" wasn't a meaningful error. Admittedly, the error messages that are now generated aren't really much better.

Instead of removing the checks, a better approach would be to install a dummy manager object that returns an error when it's used.

comment:8 Changed 3 years ago by ptone

  • Triage Stage changed from Accepted to Ready for checkin

updated and hopefully final patch:

pending final resolution on #19061

comment:9 Changed 3 years ago by Russell Keith-Magee <russell@…>

In 81f5d4a1a7b955afe530d5292726b3a8a93d7038:

Added some test guards for some recently added auth tests.

Refs #19061, #19057.

comment:10 Changed 3 years ago by ptone

  • Resolution set to fixed
  • Status changed from reopened to closed

This feature and it's tests should now be custom user compatible:

github-trac didn't seem to auto-reclose this ticket.

Note: See TracTickets for help on using tickets.
Back to Top