Add support for optional login_required decorator to require user authentication

[blag]

This should allow Django's sessions, OAuth, HTTP Auth, etc. to be utilized to protect the status view.

[landscape-bot]

Repository health decreased by 0.62% when pulling 922c2ba on blag:add-login-required-decorator into e443d84 on mwarkentin:master.

• 1 new problem was found (including 0 errors and 1 code smell).
• No problems were fixed.

[mwarkentin]

Thanks for opening these PRs - I'll try to take a look soon!

[blag]

I just want to point out that all three states of this feature are fully tested. Thanks for putting this together!

[mwarkentin]

Does it make sense to move these statements into a small reload_watchman_modules utility function since we're doing the same thing over and over in these tests?

Secondly, I'm curious as to why this is necessary - is it to pick up the @override_settings decorator you're using? Or is it just to guarantee that settings aren't being mixed and matched between tests?

[blag]

Yes, I think we could refactor this into a function, but I don't think that's strictly necessary because the three tests are the only times these modules will be reloaded. Let me know what you want me to do with this.

Reloading the modules is necessary to pick up the overridden settings, because simply overriding the settings doesn't re-run the

WATCHMAN_LOGIN = getattr(settings, 'WATCHMAN_LOGIN', False)

logic in settings.py, or the

if settings.WATCHMAN_LOGIN:
    ....

logic in decorators.py, or the

from watchman.decorators import token_required, login_required

logic in views.py, but re-importing all of those modules in that specific order does.

The @override_settings decorator automatically ensures that the settings do not propagate outside of the decorated test/s (it can also decorate classes).

[mwarkentin]

Ah, makes sense why it's needed now.

I'd be +1 to pulling it out into a small function (generally I like doing that the third time I find myself copy / pasting some code). I wouldn't be surprised if more tests are added which require the reload as well (eg. once this and #31 are merged in, we may want to test the login_required functionality on that view as well.

[blag]

I'll pull the reload()s out into their own functions then.

I'm not sure what we gain by testing the @login_required decorator on different endpoints. The three states that test the decorator are:

1. WATCHMAN_LOGIN = False, bypass the @login_required check entirely for unauthenticated users
2. WATCHMAN_LOGIN = True, which for AnonymousUser should HTTP redirect to the site's login page
3. WATCHMAN_LOGIN = True, which for authenticated users should return an HTTP 200 status and the normal status JSON

If we know the decorator works the way we want it to (eg: all three settings states are tested) on one endpoint, testing the decorator on an additional endpoint doesn't serve to test the decorator logic any more than is already done. We don't gain any more information about the decorator logic, and we have duplicated a part of our testing burden.

However, once #31 is merged in, we can switch these tests over to use that endpoint instead.

Your thoughts?

[mwarkentin]

Makes sense, thanks!

[mwarkentin]

I'm a bit curious on your use-case for this change - we use automated monitoring of these endpoints heavily, and requiring a user to be logged in would make that difficult. Do you find yourself using watchman manually a lot (eg. checking in the browser)?

[blag]

I was planning on utilizing an OAuth client to check the status, which necessitates using the normal Django @login_required decorator. Incidentally, using that decorator allows us to use normal passwords/cookies, JWT, HTTP headers, etc. to authenticate users).

Put simply, I feel much more comfortable using a default, highly flexible, widely-deployed decorator (eg: @login_required) that's already used in production systems than a static, settings-defined token to authenticate users.

There might also be interest in using the @login_required decorator for the dashboard as well, because a static token-based authentication mechanism doesn't really fit well for that. That means we might want to have different authentication mechanisms for the JSON status endpoint and the dashboard endpoint.

I'll fix up the current issues.

[mwarkentin]

Makes sense - we configure our token w/ an env var, and just load that in the settings file, however I can see the use of adding the built-in django auth as well.

Just thinking about that a little bit, I guess that means that right now any user logged in to the site would be able to access the watchman status endpoint (or dashboard from #31) - I wonder if there should be a way to restrict it further to staff / superuser accounts?

[blag]

Yep, you are correct. There would probably have to be a corresponding permission check added in, which I can write if you like.

[mwarkentin]

It looks like there's a django.contrib.admin.view.decorators.staff_member_required decorator, which could be used, or you could write it using the @user_passes_test decorator: @user_passes_test(lambda u: u.is_staff)

The built-in decorator may be "unofficial" (and subject to change, although it seems unlikely).

Do you think there are use cases for all 3 levels (logged in, staff, superuser), or does it make more sense to pick the most common (I would imagine staff), and move forward with that?

[blag]

Hmmm...well, now I'm thinking I might want to refactor the code to allow users to simply point to their own authentication/authorization decorator in their settings, and then default to pointing to your @token_required decorator by default. That way we don't have to try to guess what users will want, it's easy to override to a built-in decorator, and easy to override to a custom decorator.

Rough example:

<project>/settings.py:

...
WATCHMAN_AUTH_DECORATOR = 'django.contrib.admin.view.decorators.staff_member_required'
...

watchman/settings.py:

...
AUTH_DECORATOR = getattr(settings, 'WATCHMAN_AUTH_DECORATOR', '.decorators.token_required')

# Actually import the decorator as auth_user
# Do not allow an empty decorator, because that's a security risk
# if systems go down and tracebacks are exposed to untrusted users
...

watchman/views.py:

from .decorators import auth_user
...
@auth_user
@json_view
def status(request):
...

How does that sound? An additional benefit is that it doesn't actually change the existing behavior (eg: using @token_required by default.

[mwarkentin]

Yeah, that's interesting.. we could even provide our own auth decorators (eg. watchman.decorators.staff_required) to make it simple to swap in a few of the common types.

[blag]

I would say simply test the @token_required decorator as we do now, and then use one of the built-in decorators from Django to test that setting a different auth decorator works.

Aside from Django deprecating one of their decorators (which I doubt will happen frequently, if at all), I would be against vendoring or wrapping any built-in decorators, simply because it would increase our testing burden for no real benefit. I am assuming that Django already has tests for the @login_required and @staff_member_required decorators, so there's no need to duplicate testing the behavior of those decorators.

Does that make sense?

[mwarkentin]

Agree from a testing perspective, but I think that providing a few built-in decorators could be nicer from a UX point of view for devs who just want a basic Django auth decorator.

Anyways, adding built in decorators would be simple enough to do in the future, so if you want to update the PR as you've described above, I'm good with that.

Will need some doc updates as well about how to configure your own auth decorator.

[landscape-bot]

Code quality remained the same when pulling 402a5d1 on blag:add-login-required-decorator into e443d84 on mwarkentin:master.

[blag]

The runtests.py commit is to get Travis tests to run again - the 68244246 test errored on out installing requirements with pip...

Update: Not a transient error, but I'm not sure what the issue is.

Update 2: Not sure what happened, but Travis tests passed.

[landscape-bot]

Code quality remained the same when pulling 6f04895 on blag:add-login-required-decorator into e443d84 on mwarkentin:master.

[mwarkentin]

LGTM