revsys / django-tos

A small app to ensure your users re-agree to Terms of Service changes
BSD 3-Clause "New" or "Revised" License
153 stars 42 forks source link

Unable to consistently redirect after user accepts #40

Closed magyarn closed 4 years ago

magyarn commented 4 years ago

Hello,

I'm using Django 2.2.7, Wagtail 2.7, and Django Allauth 0.40.0. I downloaded a copy of the django-tos repo and placed it my root project directory. Because I am using Django Allauth I am following the middleware installation approach. I have created an active TermsOfService object and am successfully sent to the TosView after logging in. However, when a user accepts the terms, they are not always passed to the next page. The UserAgreement object gets succcessfully created on the first try, but the user remains on the TosView after the tos_check form is submitted. It seems as though the user can only move onto the "next" page after I make a change to my project and save the file, causing everything to reload.

I have tried setting request.session['tos_user'] = user.pk and request.session['tos_backend'] = user.backend after auth_login(request, user) is called in check_tos(), but that had no effect. I also logged user and UserAgreement.objects.all() inside check_tos() to make sure that information is there, and it is.

My value for redirect_to is /, which I thought might be problematic. So I changed the value of the hidden next field to a different relative path in the inspector, but that also did not work. I remain on the TosView page.

Below are my settings/base.py and root urls.py. Thank you for any help you can provide.

### settings/base.py
"""
Django settings for whyyoumatter project.

Generated by 'django-admin startproject' using Django 2.2.7.

For more information on this file, see
https://docs.djangoproject.com/en/2.2/topics/settings/

For the full list of settings and their values, see
https://docs.djangoproject.com/en/2.2/ref/settings/
"""

# Build paths inside the project like this: os.path.join(BASE_DIR, ...)
import os

PROJECT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
BASE_DIR = os.path.dirname(PROJECT_DIR)

# Quick-start development settings - unsuitable for production
# See https://docs.djangoproject.com/en/2.2/howto/deployment/checklist/

# Application definition

INSTALLED_APPS = [
    "tos",
    "accounts",
    "articles",
    "base",
    "partners",
    "search",
    "testimonials",
    "threads",
    "wagtail.contrib.forms",
    "wagtail.contrib.modeladmin",
    "wagtail.contrib.redirects",
    "wagtail.contrib.routable_page",
    "wagtail.embeds",
    "wagtail.sites",
    "wagtail.users",
    "wagtail.snippets",
    "wagtail.documents",
    "wagtail.images",
    "wagtail.search",
    "wagtail.admin",
    "wagtail.core",
    "wagtailfontawesome",
    "wagtailmenus",
    "modelcluster",
    "taggit",
    "widget_tweaks",
    "allauth",
    "allauth.account",
    "allauth.socialaccount",
    "allauth.socialaccount.providers.google",
    "django_extensions",
    "django.contrib.admin",
    "django.contrib.auth",
    "django.contrib.contenttypes",
    "django.contrib.humanize",
    "django.contrib.sessions",
    "django.contrib.sitemaps",
    "django.contrib.messages",
    "django.contrib.sites",
    "django.contrib.staticfiles",
    "sass_processor",
]

MIDDLEWARE = [
    "django.contrib.sessions.middleware.SessionMiddleware",
    "django.middleware.common.CommonMiddleware",
    "django.middleware.csrf.CsrfViewMiddleware",
    "django.contrib.auth.middleware.AuthenticationMiddleware",
    "django.contrib.messages.middleware.MessageMiddleware",
    "django.middleware.clickjacking.XFrameOptionsMiddleware",
    "django.middleware.security.SecurityMiddleware",
    "wagtail.core.middleware.SiteMiddleware",
    "wagtail.contrib.redirects.middleware.RedirectMiddleware",
    "tos.middleware.UserAgreementMiddleware",
]

ROOT_URLCONF = "whyyoumatter.urls"

TEMPLATES = [
    {
        "BACKEND": "django.template.backends.django.DjangoTemplates",
        "DIRS": [os.path.join(PROJECT_DIR, "templates"),],
        "APP_DIRS": True,
        "OPTIONS": {
            "context_processors": [
                "django.template.context_processors.debug",
                "django.template.context_processors.request",
                "django.contrib.auth.context_processors.auth",
                "django.contrib.messages.context_processors.messages",
                "wagtailmenus.context_processors.wagtailmenus",
            ],
        },
    },
]

AUTHENTICATION_BACKENDS = (
    # Needed to login by username in Django admin, regardless of `allauth`
    "django.contrib.auth.backends.ModelBackend",
    # `allauth` specific authentication methods, such as login by e-mail
    "allauth.account.auth_backends.AuthenticationBackend",
)

SITE_ID = 2

WSGI_APPLICATION = "whyyoumatter.wsgi.application"

# Database
# https://docs.djangoproject.com/en/2.2/ref/settings/#databases

# Password validation
# https://docs.djangoproject.com/en/2.2/ref/settings/#auth-password-validators

AUTH_PASSWORD_VALIDATORS = [
    {
        "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator",
    },
    {"NAME": "django.contrib.auth.password_validation.MinimumLengthValidator",},
    {"NAME": "django.contrib.auth.password_validation.CommonPasswordValidator",},
    {"NAME": "django.contrib.auth.password_validation.NumericPasswordValidator",},
]

AUTH_USER_MODEL = "accounts.Account"

# Internationalization
# https://docs.djangoproject.com/en/2.2/topics/i18n/

LANGUAGE_CODE = "en-us"

TIME_ZONE = "America/Detroit"

USE_I18N = True

USE_L10N = True

USE_TZ = True

# Static files (CSS, JavaScript, Images)
# https://docs.djangoproject.com/en/2.2/howto/static-files/

STATICFILES_FINDERS = (
    "django.contrib.staticfiles.finders.FileSystemFinder",
    "django.contrib.staticfiles.finders.AppDirectoriesFinder",
    "sass_processor.finders.CssFinder",
)

SASS_PROCESSOR_ROOT = os.path.join(BASE_DIR, "static")

STATICFILES_DIRS = [
    os.path.join(PROJECT_DIR, "static"),
]

# ManifestStaticFilesStorage is recommended in production, to prevent outdated
# Javascript / CSS assets being served from cache (e.g. after a Wagtail upgrade).
# See https://docs.djangoproject.com/en/2.2/ref/contrib/staticfiles/#manifeststaticfilesstorage

STATIC_ROOT = os.path.join(BASE_DIR, "static")
STATIC_URL = "/static/"

# Wagtail settings

WAGTAIL_SITE_NAME = "#WhyYouMatter"

# Base URL to use when referring to full URLs within the Wagtail admin backend -
# e.g. in notification emails. Don't include '/admin' or a trailing slash
BASE_URL = "https://whyyoumatter.pythonanywhere.com"

ACCOUNT_FORMS = {"signup": "accounts.forms.MySignUpForm"}
LOGIN_URL = "/login/"
LOGIN_REDIRECT_URL = "/"
ACCOUNT_AUTHENTICATION_METHOD = "email"
ACCOUNT_CONFIRM_EMAIL_ON_GET = True
ACCOUNT_EMAIL_REQUIRED = True
ACCOUNT_UNIQUE_EMAIL = True
ACCOUNT_EMAIL_VERIFICATION = "mandatory"
ACCOUNT_LOGIN_ON_EMAIL_CONFIRMATION = True
ACCOUNT_LOGOUT_ON_GET = True
ACCOUNT_LOGIN_ON_PASSWORD_RESET = True
ACCOUNT_SESSION_REMEMBER = False  # Change to true once cookie policy is displayed
ACCOUNT_USERNAME_REQUIRED = False
ACCOUNT_USER_MODEL_USERNAME_FIELD = None
ACCOUNT_DEFAULT_HTTP_PROTOCOL = "https"
SOCIALACCOUNT_PROVIDERS = {
    'google': {
        'SCOPE': [
            'profile',
            'email',
        ],
        'AUTH_PARAMS': {
            'access_type': 'online',
        }
    }
}

WAGTAIL_USER_EDIT_FORM = "accounts.forms.CustomUserEditForm"
WAGTAIL_USER_CREATION_FORM = "accounts.forms.CustomUserCreationForm"
WAGTAIL_USER_CUSTOM_FIELDS = ["school"]
WAGTAIL_FRONTEND_LOGIN_URL = LOGIN_URL
### root urls.py
from django.conf import settings
from django.conf.urls import url
from django.contrib import admin
from django.urls import include, path

from wagtail.admin import urls as wagtailadmin_urls
from wagtail.contrib.sitemaps.views import sitemap
from wagtail.core import urls as wagtail_urls
from wagtail.documents import urls as wagtaildocs_urls

from search import views as search_views

urlpatterns = [
    url(r"^sitemap\.xml$", sitemap),
    url(r"^django-admin/", admin.site.urls),
    url(r"^admin/", include(wagtailadmin_urls)),
    url(r"^documents/", include(wagtaildocs_urls)),
    url(r"^search/$", search_views.search, name="search"),
    url(r"^my-account/", include("accounts.urls")),
    url(r'^terms-of-service/', include('tos.urls')),
    url(r"", include("allauth.urls")),
    # For anything not caught by a more specific rule above, hand over to
    # Wagtail's page serving mechanism. This should be the last pattern in
    # the list:
    url(r"", include(wagtail_urls)),
    # Alternatively, if you want Wagtail pages to be served from a subpath
    # of your site, rather than the site root:
    #    url(r'^pages/', include(wagtail_urls)),
]

if settings.DEBUG:
    from django.conf.urls.static import static
    from django.contrib.staticfiles.urls import staticfiles_urlpatterns
    import debug_toolbar

    urlpatterns = [
        path('__debug__/', include(debug_toolbar.urls)),
    ] + urlpatterns
    # Serve static and media files from development server
    urlpatterns += staticfiles_urlpatterns()
    urlpatterns += static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT)
nicholasserra commented 4 years ago

What cache backend are you using? Middleware install requires cache set up.

magyarn commented 4 years ago

Thanks for your quick response, @nicholasserra. I didn't have a CACHES setting before, so I added the following to my settings/base.py:

CACHES = {
    'default': {
        'BACKEND': 'django.core.cache.backends.locmem.LocMemCache',
        'LOCATION': 'unique-snowflake',
    }
}

I encountered the same problem, though. So I tried creating a separate cache, like so:

CACHES = {
    'default': {
        'BACKEND': 'django.core.cache.backends.locmem.LocMemCache',
        'LOCATION': 'unique-snowflake',
    },
    'tos': {
        'BACKEND': 'django.core.cache.backends.locmem.LocMemCache',
        'LOCATION': 'unique-snowflake-2',
        'NAME': 'tos-cache',
    }
}

TOS_CACHE_NAME = 'tos'

But that also did not change anything. Thanks for your time and help.

nicholasserra commented 4 years ago

At this point i'd be throwing logging into the check_tos view within the package and see what's getting called.

magyarn commented 4 years ago

After some digging around and changing to python-memcached instead of LocMemCache, I received this traceback error:

Watching for file changes with StatReloader
Exception in thread django-main-thread:
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/threading.py", line 917, in _bootstrap_inner
    self.run()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/utils/autoreload.py", line 54, in wrapper
    fn(*args, **kwargs)
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/core/management/commands/runserver.py", line 109, in inner_run
    autoreload.raise_last_exception()
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/utils/autoreload.py", line 77, in raise_last_exception
    raise _exception[1]
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/core/management/__init__.py", line 337, in execute
    autoreload.check_errors(django.setup)()
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/utils/autoreload.py", line 54, in wrapper
    fn(*args, **kwargs)
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/apps/registry.py", line 122, in populate
    app_config.ready()
  File "/Users/nathanmagyar/Documents/why-you-matter/whyyoumatter/tos/apps.py", line 27, in ready
    invalidate_cached_agreements(TermsOfService, None)
  File "/Users/nathanmagyar/Documents/why-you-matter/whyyoumatter/tos/signal_handlers.py", line 18, in invalidate_cached_agreements
    cache.incr('django:tos:key_version')
  File "/Users/nathanmagyar/Documents/why-you-matter/env/lib/python3.7/site-packages/django/core/cache/backends/memcached.py", line 110, in incr
    raise ValueError("Key '%s' not found" % key)
ValueError: Key ':1:django:tos:key_version' not found

I looked in signal_handlers.py and it seemed like the cache key wasn't getting set.

# signal_handlers.py
...
cache = get_cache(getattr(settings, 'TOS_CACHE_NAME', 'default'))
# Logging cache produces: <django.core.cache.backends.memcached.MemcachedCache object at 0x107a91198>

def invalidate_cached_agreements(TermsOfService, instance, **kwargs):
    if kwargs.get('raw', False):
        return

    # Set the key version to 0 if it doesn't exist and leave it
    # alone if it does
    cache.get('django:tos:key_version', 0)
    # Logging cache.get('django:tos:key_version') produces: None

    # This key will be used to version the rest of the TOS keys
    # Incrementing it will effectively invalidate all previous keys
    cache.incr('django:tos:key_version')

I tried using cache.get_or_set('django:tos:key_version, 0) (documentation) and that set the key, but ultimately didn't solve the issue because memcache.py is looking for ':1:django:tos:key_version'.

How does the :1: portion of the cache key get added? Is that the incrementing piece?

nicholasserra commented 4 years ago

the :1: is injected via django's internal tracking of cache VERSION. This is abstracted while using any of django's built in cache handling methods. If you zip up some kind of testable codebase I can take a better look. Sorry you're still having issues.

magyarn commented 4 years ago

Ok, thanks. Here is a test project with Wagtail, Allauth, and TOS. Thanks for taking a look.

nicholasserra commented 4 years ago

@magyarn Did you end up finding a solution to this?

magyarn commented 4 years ago

@nicholasserra Unfortunately, no.

nicholasserra commented 4 years ago

Turns out this was a cache invalidation issue. There was an incomplete PR for it a bit before this issue was raised. Just pushed a fix. Thanks!