Skip to content

Added http basic authentication to results resources#214

Merged
tobami merged 7 commits into
tobami:masterfrom
mwatts15:34-add-security-measures-to-posting
Mar 1, 2017
Merged

Added http basic authentication to results resources#214
tobami merged 7 commits into
tobami:masterfrom
mwatts15:34-add-security-measures-to-posting

Conversation

@mwatts15
Copy link
Copy Markdown
Contributor

@mwatts15 mwatts15 commented Feb 27, 2017

This PR is intended to resolve #34 .

HTTP Basic Auth is enabled for the results/add and results/add/json resources using a decorator. I also included a check for request.is_secure, conditioned on a setting so it's easier to test in deployment. Tests get an override to turn the setting off.

I added two settings, REQUIRE_SECURE_AUTH as mentioned above, and ALLOW_ANONYMOUS_POST to toggle the auth check.

@mwatts15 mwatts15 force-pushed the 34-add-security-measures-to-posting branch from 4e99009 to 4fe2f99 Compare February 27, 2017 04:19
@mwatts15
Copy link
Copy Markdown
Contributor Author

I'm aware of the failure in tests due to the missing override_settings in Django 1.6.11 . I'll attempt to port that portion of the test_utils unless there's a better option.

@tobami
Copy link
Copy Markdown
Owner

tobami commented Feb 27, 2017

I have a separate request I wanted to merge that removes DJango 1.6 from the CI build, dropping support basically. Let me merge that branch and you can the rebase.

@tobami
Copy link
Copy Markdown
Owner

tobami commented Feb 27, 2017

Al-right, you can rebase on current master

Copy link
Copy Markdown
Owner

@tobami tobami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would work, though you are kind of re-implementing proper basic auth, which is usually not a good idea considering Django provides most of what you would need for that out of the box.

Ideally we can do something simpler that uses the Django backend. For example user_passes_test
or a custom backend that inherits from the default one

@mwatts15 mwatts15 force-pushed the 34-add-security-measures-to-posting branch from 2eca4c3 to fa4e9e8 Compare February 28, 2017 01:52
@mwatts15
Copy link
Copy Markdown
Contributor Author

A little Googling turned up implementations of decorators similar to the one in this PR. Other options involve modifying the webserver hosting the app which isn't an option in my case.

I modified my PR to allow for other authentication methods to be used by adding the request.user.is_authenticated() branch in my wrapper.

- Also, named the logger and added some helpful logging
Copy link
Copy Markdown
Owner

@tobami tobami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, let's do this.
One last thing, to be backwards compatible we'd have to set ALLOW_ANONYMOUS_POST to True.

@mwatts15
Copy link
Copy Markdown
Contributor Author

mwatts15 commented Mar 1, 2017

I can do that. I'd like to output a warning at startup though, and I would ask that you plan to break backwards compatibility in a near-future release. There are too many web applications that are insecure by default as it is.

- Also added warnings for potentially insecure configuration
@tobami tobami merged commit 7955cd2 into tobami:master Mar 1, 2017
@tobami
Copy link
Copy Markdown
Owner

tobami commented Mar 1, 2017

Yeah, it makes sense to require that in a future release. I'd prefer to do that once there is a proper API, that for example allows API tokens.

In any case, as-is, great addition, thanks!

@mwatts15 mwatts15 deleted the 34-add-security-measures-to-posting branch March 3, 2017 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add security measures for POSTing

2 participants