Enable user app routes when DEBUG is True only

[demestav]

Description

The user app exposes all users' details to any registered user. This was of course intentional, since the purpose of the user app is to demonstrate the functionality and not to be deployed in production as-is. However, a developer who is not familiar with the cookiecutter-django's structure, may overlook this and expose user information.

Therefore, the user app should only be enabled when DEBUG == True in the settings.

Closes #1739, Closes #1752.

Rationale

Since DEBUG = False in production settings, even if the user app is left as-is, it will not be enabled and users' information will not be exposed.

[webyneter]

You forgot about https://github.com/pydanny/cookiecutter-django/blob/master/%7B%7Bcookiecutter.project_slug%7D%7D/config/urls.py#L18 : gotta decide how it should be taken care of.

[demestav]

@webyneter Indeed I have. Also the LOGIN_REDIRECT_URL points to the users app, which also needs to be fixed.

[luzfcb]

I agree UserListView may be useful only for anyone who has explicitly permission to access it (maybe we should include some permission by default.) or for debugging.

I think it would be best just to check if the user of the request is the same as the user obtained in self.object or super().get_object() on UserDetailView, anything like

from django.http import Http404
from django.utils.translation import gettext as _

class OnlyMePermission:
    def get_object(self, queryset=None):
        obj = super().get_object(queryset=queryset)
        if not obj == self.request.user:
            if queryset is None:
                queryset = self.get_queryset()
            raise Http404(_("No %(verbose_name)s found matching the query") %
                          {'verbose_name': queryset.model._meta.verbose_name})
        return obj

class UserDetailView(LoginRequiredMixin, OnlyMePermission, DetailView):

    model = User
    slug_field = "username"
    slug_url_kwarg = "username"

UserUpdateView has no problem, because

    def get_object(self):
        return User.objects.get(username=self.request.user.username)

will always return the correct user.

[demestav]

@luzfcb Thank you for your input. What you propose does indeed resolve the exposure issue. What we can achieve with that, is to make the user app operational, in its current state, to only the authorized user.

However, since this app's purpose is to demonstrate the functionality, and a developer is expected to modify it to his/her own needs, I believe that just keeping it operational in the local environment only is enough. Functionality like the user list, only makes sense in the local environment anyway.

[browniebroke]

I agree UserListView may be useful only for anyone who has explicitly permission to access it (maybe we should include some permission by default.) or for debugging.

On this, maybe we could restrict the list view to staff using django-brances' StaffuserRequiredMixin?

[luzfcb]

On this, maybe we could restrict the list view to staff using django-brances' StaffuserRequiredMixin?

LGTM

[browniebroke]

The build fails currently, looks like it's not run in debug mode?

[browniebroke]

This is unused, please remove.

[demestav]

Indeed. Will do.

[demestav]

@browniebroke Yes, tests fail because DEBUG=False in settings.test. I tried and using @override_settings(DEBUG=True) in the user's tests and it works.

However, I am wondering if this is the preferred way to do this. If I am not mistaken, I noticed that the effects of @override_settings remain effective, even after a specific test finishes. In other words, I used @override_settings(DEBUG=True) only in users/test_models.py and all tests passed, even though before this change, there were 7 failures.

[browniebroke]

That's most likely because we use pytest and that decorator is for unittests. Could you try using pytest-django fixture for editing settings instead?

https://pytest-django.readthedocs.io/en/latest/helpers.html#settings

[demestav]

@browniebroke I see. I have spent some time to fix this, but I am having some trouble since I never used pytest before. What does user: settings.AUTH_USER_MODEL part does in test_models.py? I can't find any documentation about that.

[browniebroke]

Ah, yes, that can be confusing if you never used pytest. It's a pytest fixture, and the user one is defined here:

https://github.com/pydanny/cookiecutter-django/blob/320c957f4ea90626e29c3f06626f3db8811e8dcc/%7B%7Bcookiecutter.project_slug%7D%7D/%7B%7Bcookiecutter.project_slug%7D%7D/conftest.py#L13-L15

When a test function argument matches a fixture name, pytest injects it into your test as argument.

The settings helper is also a fixture, there is no need to import it in the test file. If you add an argument to the test function called settings, pytest takes care of finding it from pytest-django.

By convention, custom fixtures are defined in a file called conftest.py. You can read more about them here: https://docs.pytest.org/en/latest/fixture.html

[demestav]

@browniebroke Thank you for the detailed explanation. The only issue I see here is that the settings is already imported because of the user fixture:
https://github.com/pydanny/cookiecutter-django/blob/3aa289e79b44a8bd207fe0273b16c64b08a3b199/%7B%7Bcookiecutter.project_slug%7D%7D/%7B%7Bcookiecutter.project_slug%7D%7D/users/tests/test_models.py#L2

So I believe that would overwrite the settings fixture of pytest.

[browniebroke]

Have you tried to change it to:

from django.conf import settings as real_settings

[demestav]

@browniebroke Actually I have, but I named it project_settings 😄 . I wasn't sure if the core team would approve it though. Anyway, it still gives me trouble. Adding settings.DEBUG = True to only test_models.py causes all the tests to pass, even though they shouldn't. I have checked the status of the settings.DEBUG within the other tests (the ones that should have failed) and the value returns as False, which is the expected. So, I am not sure at the moment why this happens.

[browniebroke]

Stupid question: is there anything preventing us from setting DEBUG=True for the entire test run? 🤔

[demestav]

@browniebroke Well, the default settings for testing define DEBUG = False so I guess the tests need to run in that state? On the other hand, if user application tests are the only tests of the project, then why not.

[browniebroke]

I genuinely don't know why or if DEBUG = False is actually required for tests, so maybe change it to True and see if anything else breaks?

As a side note, the build status is green although the tests failed in Travis, I don't think it's related to this PR, but it's something we'll need to look at...

[webyneter]

I hoped to get some time to finally fix this in mainline, but... My guess is, you're stumbling upon the wrong configuration of pytest.ini -- to work as expected, pytest.ini's contents must rather be:

[pytest]
; https://pytest-django.readthedocs.io/en/latest/configuring_django.html#order-of-choosing-settings
addopts = --ds=config.settings.test

[demestav]

@browniebroke Indeed, I also noticed that Travis reports OK!

A small update regarding this issue. I tried the following:

1. Putting DEBUG = True in config.settings.test
2. Changing pytest.ini as @webyneter suggested
3. To double check, I also appended --ds=config.settings.test in the testing command of tests/test_docker.sh file

In all cases the tests, failed. I went ahead and printed out the settings.DEBUG value in test_models.py. Here are the file contents for you to confirm:

import pytest
from django.conf import settings

pytestmark = pytest.mark.django_db


def test_user_get_absolute_url(user: settings.AUTH_USER_MODEL):
    print(settings.DEBUG)
    print(str(settings))
    assert False
    assert user.get_absolute_url() == f"/users/{user.username}/"

In all cases, the return value I got was False. Also the name of the settings is <UserSettingsHolder>. Is there a way to get the settings location?

[browniebroke]

It looks like pytest is using the right setting file, though:

Test session starts (platform: linux, Python 3.6.8, pytest 4.4.0, pytest-sugar 0.9.2)
Django settings: config.settings.test (from ini file)

[demestav]

@browniebroke Thanks for helping out. Does your commit resolve the testing issue? I will try and get back on this PR as soon as possible. It shouldn't need much more effort.

[browniebroke]

No, it doesn't (confirms one of your previous comment).

[browniebroke]

Turns out pytest-django has a fixture that sets DEBUG=False and overrides the value from our test settings.

[browniebroke]

After this last finding, I think it might be better to create a new setting, something along the lines of USER_APP_URLS_ENABLED/USER_APP_INSECURE_URLS_ENABLED (better names welcome).

How about it being True in config.settings.base and False in config.settings.production?

We should probably revert my last commit to set DEBUG=True in test config, this is misleading.

[demestav]

@browniebroke Agreed. Regarding the commit revert, I am not sure which is the best way to do it.

[browniebroke]

I did a git reset to the prior commit and pushed, so it's now reverted.

[demestav]

Even though we resolved the testing issue, I have to say that having that extra option seems like a workaround. Why would anyone want the User app as is in production? They shouldn't unless they modify it. If they do modify it, why would they need to enable it through the production.py settings?

[personjerry]

Would it be better to address the privacy issues specifically and directly instead of turning the whole app off? In particular I mean something along the lines of request.user.username == username check for UserDetailView and just removing the UserListView.

[browniebroke]

I agree, looking at how this pull request now, it doesn't feel like the right fix. I agree that the UserListView should be removed at this point, but the UserDetailView is a profile page. If you don't own the profile, you see a limited view of it, it's not necessarily a privacy issue (it depends on your application).

We can fix one easily, but the other requires further discussion.

[demestav]

@browniebroke I think we should close this due to #2062

[browniebroke]

Agreed. Thanks for putting through the work in this anyway!