Opened 13 years ago

Closed 12 years ago

#16366 closed Bug (fixed)

contrib.auth.tests.context_processors.AuthContextProcessorTests.test_session_not_accessed fails when project loads other middleware that access the session.

Reported by: Jim Dalton Owned by: Aymeric Augustin
Component: contrib.auth Version: dev
Severity: Release blocker Keywords: test
Cc: Jim Dalton, calvinx, dev@… 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

AuthContextProcessorTests.test_session_not_accessed()tests whether the session is accessed when a template is loaded but the auth context processor is not actually accessed in the template. Presumably it's a test that ensures unnecessary session access is avoided.

However, if you have middleware eanbled that access the session at some point in the request cycle, then request.session.accessed will be True going into the test and the test will fail -- even though the code being exercised did not actually hit the session.

As I understand it, the context processors in auth were not actually running until very recently (thanks to r16304), and I"m not sure when this test was actually introduced. I don't have an easy fix for it though. I thought setting request.session.accessed to False prior to calling rendor_to_response in auth.tests.urls.auth_processor_no_attr_access() would do the trick -- but that just sets up a false pass. If you set it the same way in auth.tests.urls.auth_processor_attr_access() then *that* test fails because it checks out to be False.

Does this test have to run in every project? Can it be moved to the main test suite instead of having it be part of the application tests? I'm sure it's valuable to ensure the session is not hit if the context processor is not accessed, but I'm not convinced this behavior will be influenced by any project specific code, so I'm thinking the main test suite is a better place for it. It's definitely a problem to have a test failure in contrib.auth solely because you have middleware that accesses the session.

Attachments (4)

fix_auth_context_processor_tests.diff (1.8 KB ) - added by Jim Dalton 13 years ago.
16366-2.diff (2.0 KB ) - added by Claude Paroz 12 years ago.
Patch with override_settings
16366-3.diff (2.1 KB ) - added by Claude Paroz 12 years ago.
Added TEMPLATE_CONTEXT_PROCESSORS to override
16366-4.diff (2.5 KB ) - added by Ryan Kaskel 12 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 by Jim Dalton, 13 years ago

Easy pickings: set
Has patch: set

Here's a patch that fixes the tests. To test, run project tests with contrib.auth in INSTALLED_APPS.

If we're going to include these tests in the project tests as now, then the views auth_processor_no_attr_access() and auth_processor_attr_access() in auth.tests.urls must have a "clean" request object in order for the tests to be meaningful in anyway. By clean I mean no other middleware can have accessed the session (or request.user, which amounts to the same thing) -- otherwise we aren't actually testing what we say we are testing.

To resolve this, I did a bit of setup before each of these methods. Basically we reset session.accessed to False, clear the _cache_user from request if it has been set, then rerun the auth middleware on the request object, so that a new lazy user object is added correctly into it. This allows the template calls to do what they claim they are doing and the tests to pass.

I'm marking this as easy-pickings, because it's a pretty straightforward fix and hopefully a straightforward issue.

by Jim Dalton, 13 years ago

comment:2 by Aymeric Augustin, 13 years ago

Easy pickings: unset
Triage Stage: UnreviewedAccepted

The problem looks valid.

To resolve it, I would probably try to override settings.CONTEXT_PROCESSOR in the test. Your patch takes a different route. I can't say which approach is the best.

comment:3 by calvinx, 12 years ago

Cc: calvinx added
Has patch: unset

aaugustin: can you clarify what you mean by "override settings.CONTEXT_PROCESSOR" in this django test?

comment:4 by Julien Phalip, 12 years ago

See also #17194.

comment:5 by Aymeric Augustin, 12 years ago

That was a slip, I meant "override settings.MIDDLEWARE_CLASSES".

See https://docs.djangoproject.com/en/dev/topics/testing/#overriding-settings

comment:6 by Aymeric Augustin, 12 years ago

#17342 was a duplicate.

comment:7 by fbeaufort, 12 years ago

I just tried override_settings with django 1.4-alpha-1 as below.
I dont know if it comes from override_settings but this test alone is ok but fails when

./manage.py test aauth.AuthContextProcessorTests
from django.conf import global_settings
...

    @override_settings(MIDDLEWARE_CLASSES=global_settings.MIDDLEWARE_CLASSES)
    @override_settings(TEMPLATE_CONTEXT_PROCESSORS=global_settings.TEMPLATE_CONTEXT_PROCESSORS)
    def test_session_not_accessed(self):
        """
        Tests that the session is not accessed simply by including
        the auth context processor
        """
        response = self.client.get('/auth_processor_no_attr_access/')
        self.assertContains(response, "Session not accessed")

by Claude Paroz, 12 years ago

Attachment: 16366-2.diff added

Patch with override_settings

comment:8 by Claude Paroz, 12 years ago

Has patch: set
Keywords: test added

by Claude Paroz, 12 years ago

Attachment: 16366-3.diff added

Added TEMPLATE_CONTEXT_PROCESSORS to override

comment:9 by Claude Paroz, 12 years ago

New patch uploaded, as in one of my own projects the test was failing due to a custom context processor.

comment:10 by fbeaufort, 12 years ago

Is this patch going to be into django-1.4 at some point?

in reply to:  10 comment:11 by Carl Meyer, 12 years ago

Replying to fbeaufort:

Is this patch going to be into django-1.4 at some point?

If you'd like it to be, you can help. As documented in the contributing docs, the next step for this ticket would be for someone other than the patch author to review the ticket and patch, verify that the code looks correct, the patch applies cleanly, there are no outstanding objections to the approach, and it fixes the reported problem. If so, mark it Ready for Checkin and it will get onto the core devs' radar for commit.

comment:12 by Ryan Kaskel, 12 years ago

Triage Stage: AcceptedReady for checkin

The previous patch was almost there.

The problem was that django.template.context caches template context processors that it's seen. Overriding the settings didn't have any effect if other tests that didn't override the settings were run beforehand.

I've updated the patch to clear the cached context processors for those two tests.

I also removed the settings import because it's not needed.

It would be great if this could be slipped in before the release.

Last edited 12 years ago by Ryan Kaskel (previous) (diff)

by Ryan Kaskel, 12 years ago

Attachment: 16366-4.diff added

comment:13 by Claude Paroz, 12 years ago

I'd better see the context._standard_context_processors cache emptied in override_settings itself, instead of in each test. Let the committer decide what's the preferred approach.

comment:14 by Ryan Kaskel, 12 years ago

Severity: NormalRelease blocker

I'm marking this as a "release blocker" on the basis of Aymeric Augustin's recent message (http://groups.google.com/group/django-developers/browse_thread/thread/cdd0f2b6e2d1141e).

It's not a major issue but I would consider it a regression because Django's tests are failing for some users and a reasonable fix is ready.

Please let me know if this is not the right action.

comment:15 by Ryan Kaskel, 12 years ago

Cc: dev@… added

comment:16 by Aymeric Augustin, 12 years ago

Owner: changed from nobody to Aymeric Augustin

in reply to:  13 comment:17 by Aymeric Augustin, 12 years ago

Replying to claudep:

I'd better see the context._standard_context_processors cache emptied in override_settings itself, instead of in each test. Let the committer decide what's the preferred approach.

I've created #17787 to track the more general problem. Until a decision is made on that ticket, we can use an ad-hoc reset — we're already doing that in several tests.

comment:18 by Aymeric Augustin, 12 years ago

Resolution: fixed
Status: newclosed

In [17598]:

Fixed #16366 -- Prevented some failures of the django.contrib.auth tests when run within a project. Thanks to everyone who contributed to the patch.

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