Code

Opened 3 years ago

Closed 2 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: jsdalton Owned by: aaugustin
Component: contrib.auth Version: master
Severity: Release blocker Keywords: test
Cc: jsdalton, 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 jsdalton 3 years ago.
16366-2.diff (2.0 KB) - added by claudep 2 years ago.
Patch with override_settings
16366-3.diff (2.1 KB) - added by claudep 2 years ago.
Added TEMPLATE_CONTEXT_PROCESSORS to override
16366-4.diff (2.5 KB) - added by ryankask 2 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 years ago by jsdalton

  • Easy pickings set
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

Changed 3 years ago by jsdalton

comment:2 Changed 3 years ago by aaugustin

  • Easy pickings unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 2 years ago by calvinx

  • Cc calvinx added
  • Has patch unset

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

comment:4 Changed 2 years ago by julien

See also #17194.

comment:5 Changed 2 years ago by aaugustin

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

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

comment:6 Changed 2 years ago by aaugustin

#17342 was a duplicate.

comment:7 Changed 2 years ago by fbeaufort

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")

Changed 2 years ago by claudep

Patch with override_settings

comment:8 Changed 2 years ago by claudep

  • Has patch set
  • Keywords test added

Changed 2 years ago by claudep

Added TEMPLATE_CONTEXT_PROCESSORS to override

comment:9 Changed 2 years ago by claudep

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

comment:10 follow-up: Changed 2 years ago by fbeaufort

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

comment:11 in reply to: ↑ 10 Changed 2 years ago by carljm

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 Changed 2 years ago by ryankask

  • Triage Stage changed from Accepted to Ready for checkin

The previous patch was almost there.

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

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.

Version 1, edited 2 years ago by ryankask (previous) (next) (diff)

Changed 2 years ago by ryankask

comment:13 follow-up: Changed 2 years ago by 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.

comment:14 Changed 2 years ago by ryankask

  • Severity changed from Normal to Release 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 Changed 2 years ago by ryankask

  • Cc dev@… added

comment:16 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:17 in reply to: ↑ 13 Changed 2 years ago by aaugustin

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 Changed 2 years ago by aaugustin

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.