Opened 11 years ago

Closed 11 years ago

#19057 closed Bug (fixed)

mod_wsgi authentication handlers fail with custom user model

Reported by: Russell Keith-Magee Owned by: nobody
Component: contrib.auth Version: dev
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

Description

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 by Preston Holmes, 11 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:2 by Preston Holmes <preston@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 5f8b97f9fb058e5e02f1f99423fc3b0020ecdeb0:

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

thanks @freakboy3742 for the catch and review

comment:3 by Russell Keith-Magee, 11 years ago

Resolution: fixed
Status: closedreopened

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 by Preston Holmes, 11 years ago

Triage Stage: Ready for checkinAccepted

This should hopefully address point 1:

https://github.com/ptone/django/commit/a1da92acb9fb0fa095ad850a3e575071804cdffc

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 by Preston Holmes, 11 years ago

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 by Preston Holmes, 11 years ago

seeing the light now on point 2 after running it with a sample project where user model swapped out - this should be fixed with:
https://github.com/ptone/django/commit/fc2c4be763088687765abfdee5563e58ee279c86

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 by Russell Keith-Magee, 11 years ago

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 by Preston Holmes, 11 years ago

Triage Stage: AcceptedReady for checkin

updated and hopefully final patch:

https://github.com/ptone/django/compare/modwsgi-customuser

pending final resolution on #19061

comment:9 by Russell Keith-Magee <russell@…>, 11 years ago

In 81f5d4a1a7b955afe530d5292726b3a8a93d7038:

Added some test guards for some recently added auth tests.

Refs #19061, #19057.

comment:10 by Preston Holmes, 11 years ago

Resolution: fixed
Status: reopenedclosed

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

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

https://github.com/django/django/commit/2b5f848207b1dab35afd6f63d0107629c76d4d9a

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