-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add endpoints to set user notification preference #338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| NOTIFICATION_PREF_KEY = "notification_pref" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| Feature: One-click unsubscribe | ||
| As a user with notifications enabled | ||
| I want to be able to unsubscribe from notifications | ||
|
|
||
| Scenario: Unsubscribe when not logged in | ||
| Given I am an edX user | ||
| And I am not logged in | ||
| And I have notifications enabled | ||
| When I access my unsubscribe url | ||
| Then my notifications should be disabled | ||
| And I should see "Unsubscribe Successful!" somewhere on the page | ||
| And I should see "Click here to return to your dashboard" somewhere on the page | ||
| And I should see a link to "/dashboard" with the text "here" | ||
|
|
||
| Scenario: Unsubscribe when logged in | ||
| Given I am a logged in user | ||
| And I have notifications enabled | ||
| When I access my unsubscribe url | ||
| Then my notifications should be disabled | ||
| And I should see "Unsubscribe Successful!" somewhere on the page | ||
| And I should see "Click here to return to your dashboard" somewhere on the page | ||
| And I should see a link to "/dashboard" with the text "here" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| from django.contrib.auth.models import User | ||
| from lettuce import step, world | ||
| from notification_prefs import NOTIFICATION_PREF_KEY | ||
| from user_api.models import UserPreference | ||
|
|
||
|
|
||
| USERNAME = "robot" | ||
| UNSUB_TOKEN = "av9E-14sAP1bVBRCPbrTHQ==" | ||
|
|
||
|
|
||
| @step(u"I have notifications enabled") | ||
| def enable_notifications(step): | ||
| user = User.objects.get(username=USERNAME) | ||
| UserPreference.objects.create(user=user, key=NOTIFICATION_PREF_KEY, value=UNSUB_TOKEN) | ||
|
|
||
|
|
||
| @step(u"I access my unsubscribe url") | ||
| def access_unsubscribe_url(step): | ||
| world.visit("/notification_prefs/unsubscribe/{0}/".format(UNSUB_TOKEN)) | ||
|
|
||
|
|
||
| @step(u"my notifications should be disabled") | ||
| def notifications_should_be_disabled(step): | ||
| user = User.objects.get(username=USERNAME) | ||
| assert not UserPreference.objects.filter(user=user, key=NOTIFICATION_PREF_KEY).exists() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,185 @@ | ||
| from django.contrib.auth.models import AnonymousUser | ||
| from django.http import Http404 | ||
| from django.test import TestCase | ||
| from django.test.client import Client, RequestFactory | ||
| from django.test.utils import override_settings | ||
| from mitxmako.middleware import MakoMiddleware | ||
| from student.tests.factories import UserFactory | ||
| from user_api.models import UserPreference | ||
| from notification_prefs import NOTIFICATION_PREF_KEY | ||
| from notification_prefs.views import ajax_enable, ajax_disable, unsubscribe | ||
|
|
||
|
|
||
| @override_settings(SECRET_KEY="test secret key") | ||
| class NotificationPrefViewTest(TestCase): | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| # Make sure global state is set up appropriately | ||
| Client().get("/") | ||
|
|
||
| def setUp(self): | ||
| self.user = UserFactory.create(username="testuser") | ||
| # Username with length equal to AES block length to test padding | ||
| self.aes_block_length_user = UserFactory.create(username="sixteencharsuser") | ||
| # Tokens are intentionally hard-coded instead of computed to help us | ||
| # avoid breaking existing links. | ||
| self.tokens = { | ||
| # Encrypted value: "testuser" + "\x08" * 8 | ||
| self.user: "DyYxCj3oVl9vVgq_VHlfqw==", | ||
| # Encrypted value: "sixteencharsuser" + "\x10" * 16 | ||
| self.aes_block_length_user: "_E9YK4jYDL1MBMFWd_Dt4tRGw8HDEmlcLVFawgY9wI8=", | ||
| } | ||
| self.request_factory = RequestFactory() | ||
|
|
||
| def create_prefs(self): | ||
| for (user, token) in self.tokens.items(): | ||
| UserPreference.objects.create(user=user, key=NOTIFICATION_PREF_KEY, value=token) | ||
|
|
||
| def assertPrefValid(self, user): | ||
| self.assertEqual( | ||
| UserPreference.objects.get(user=user, key=NOTIFICATION_PREF_KEY).value, | ||
| self.tokens[user] | ||
| ) | ||
|
|
||
| def assertNotPrefExists(self, user): | ||
| self.assertFalse( | ||
| UserPreference.objects.filter(user=user, key=NOTIFICATION_PREF_KEY).exists() | ||
| ) | ||
|
|
||
| # AJAX enable view | ||
|
|
||
| def test_ajax_enable_get(self): | ||
| request = self.request_factory.get("dummy") | ||
| request.user = self.user | ||
| response = ajax_enable(request) | ||
| self.assertEqual(response.status_code, 405) | ||
| self.assertNotPrefExists(self.user) | ||
|
|
||
| def test_ajax_enable_anon_user(self): | ||
| request = self.request_factory.post("dummy") | ||
| request.user = AnonymousUser() | ||
| response = ajax_enable(request) | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertNotPrefExists(self.user) | ||
|
|
||
| def test_ajax_enable_success(self): | ||
| def test_user(user): | ||
| request = self.request_factory.post("dummy") | ||
| request.user = user | ||
| response = ajax_enable(request) | ||
| self.assertEqual(response.status_code, 204) | ||
| self.assertPrefValid(user) | ||
|
|
||
| test_user(self.user) | ||
| test_user(self.aes_block_length_user) | ||
|
|
||
| def test_ajax_enable_already_enabled(self): | ||
| self.create_prefs() | ||
| request = self.request_factory.post("dummy") | ||
| request.user = self.user | ||
| response = ajax_enable(request) | ||
| self.assertEqual(response.status_code, 204) | ||
| self.assertPrefValid(self.user) | ||
|
|
||
| def test_ajax_enable_distinct_values(self): | ||
| request = self.request_factory.post("dummy") | ||
| request.user = self.user | ||
| ajax_enable(request) | ||
| other_user = UserFactory.create() | ||
| request.user = other_user | ||
| ajax_enable(request) | ||
| self.assertNotEqual( | ||
| UserPreference.objects.get(user=self.user, key=NOTIFICATION_PREF_KEY).value, | ||
| UserPreference.objects.get(user=other_user, key=NOTIFICATION_PREF_KEY).value | ||
| ) | ||
|
|
||
| # AJAX disable view | ||
|
|
||
| def test_ajax_disable_get(self): | ||
| self.create_prefs() | ||
| request = self.request_factory.get("dummy") | ||
| request.user = self.user | ||
| response = ajax_disable(request) | ||
| self.assertEqual(response.status_code, 405) | ||
| self.assertPrefValid(self.user) | ||
|
|
||
| def test_ajax_disable_anon_user(self): | ||
| self.create_prefs() | ||
| request = self.request_factory.post("dummy") | ||
| request.user = AnonymousUser() | ||
| response = ajax_disable(request) | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertPrefValid(self.user) | ||
|
|
||
| def test_ajax_disable_success(self): | ||
| self.create_prefs() | ||
| request = self.request_factory.post("dummy") | ||
| request.user = self.user | ||
| response = ajax_disable(request) | ||
| self.assertEqual(response.status_code, 204) | ||
| self.assertNotPrefExists(self.user) | ||
|
|
||
| def test_ajax_disable_already_disabled(self): | ||
| request = self.request_factory.post("dummy") | ||
| request.user = self.user | ||
| response = ajax_disable(request) | ||
| self.assertEqual(response.status_code, 204) | ||
| self.assertNotPrefExists(self.user) | ||
|
|
||
| # Unsubscribe view | ||
|
|
||
| def test_unsubscribe_post(self): | ||
| request = self.request_factory.post("dummy") | ||
| response = unsubscribe(request, "dummy") | ||
| self.assertEqual(response.status_code, 405) | ||
|
|
||
| def test_unsubscribe_invalid_token(self): | ||
| def test_invalid_token(token): | ||
| request = self.request_factory.get("dummy") | ||
| self.assertRaises(Http404, unsubscribe, request, token) | ||
|
|
||
| # Invalid base64 encoding | ||
| test_invalid_token("Non-ASCII\xff") | ||
| test_invalid_token("ZOMG INVALID BASE64 CHARS!!!") | ||
| test_invalid_token(self.tokens[self.user][:-1]) | ||
|
|
||
| # Token of wrong length | ||
| test_invalid_token(self.tokens[self.user][:-4]) | ||
|
|
||
| # Invalid padding (ends in 0 byte) | ||
| # Encrypted value: "testuser" + "\x00" * 8 | ||
| test_invalid_token("yhrNEjt48uMRZc3U3uR4vA==") | ||
|
|
||
| # Invalid padding (ends in byte > 16) | ||
| # Encrypted value: "testusertestuser" | ||
| test_invalid_token("LqItcaGOQXK0mglIElnMng==") | ||
|
|
||
| # Invalid padding (entire string is padding) | ||
| # Encrypted value: "\x10" * 16 | ||
| test_invalid_token("1EbDwcMSaVwtUVrCBj3Ajw==") | ||
|
|
||
| # Nonexistent user | ||
| # Encrypted value: "nonexistentuser\x01" | ||
| test_invalid_token("KnJTFMYitSOem5Sw2LuYBg==") | ||
|
|
||
| def test_unsubscribe_success(self): | ||
| self.create_prefs() | ||
|
|
||
| def test_user(user): | ||
| request = self.request_factory.get("dummy") | ||
| request.user = AnonymousUser() | ||
| response = unsubscribe(request, self.tokens[user]) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertNotPrefExists(user) | ||
|
|
||
| test_user(self.user) | ||
| test_user(self.aes_block_length_user) | ||
|
|
||
| def test_unsubscribe_twice(self): | ||
| self.create_prefs() | ||
| request = self.request_factory.get("dummy") | ||
| request.user = AnonymousUser() | ||
| unsubscribe(request, self.tokens[self.user]) | ||
| response = unsubscribe(request, self.tokens[self.user]) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertNotPrefExists(self.user) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| from base64 import urlsafe_b64encode, urlsafe_b64decode | ||
| from Crypto.Cipher import AES | ||
| from django.conf import settings | ||
| from django.contrib.auth.models import User | ||
| from django.http import Http404, HttpResponse, HttpResponseForbidden, HttpResponseNotAllowed | ||
| from hashlib import sha256 | ||
| from mitxmako.shortcuts import render_to_response | ||
| from notification_prefs import NOTIFICATION_PREF_KEY | ||
| from user_api.models import UserPreference | ||
|
|
||
|
|
||
| class UsernameCodec(object): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docstring please.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do |
||
| AES_BLOCK_LEN = 16 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I don't know how consequential it is for this, but it seems like this is easy to fake the way the code is written now. For instance, if I want to get the token for user "dave", can't I just make a user "aaaaaaaaaaaaaaaadave" and use the second half of the token generated for it?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess just switching to CBC mode would take care of this, though I'm by no means an expert.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch; I will switch to CBC. |
||
|
|
||
| def __init__(self): | ||
| hash_ = sha256() | ||
| hash_.update(settings.SECRET_KEY) | ||
| self.cipher = AES.new(hash_.digest()) | ||
|
|
||
| def _add_padding(self, str): | ||
| """Return str with PKCS#7 padding added""" | ||
| padding_len = self.AES_BLOCK_LEN - (len(str) % self.AES_BLOCK_LEN) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens to users with usernames > 16 chars?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, nvm, that still works, doesn't it?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I will augment the unit tests accordingly. |
||
| return str + (padding_len * chr(padding_len)) | ||
|
|
||
| def _remove_padding(self, str): | ||
| """Return str with PKCS#7 padding trimmed""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't shadow the built-in function str.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will fix |
||
| num_pad_bytes = ord(str[-1]) | ||
| if num_pad_bytes < 1 or num_pad_bytes > self.AES_BLOCK_LEN or num_pad_bytes >= len(str): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the username is exactly 16 chars long?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 16 bytes of padding will be added, and this case is explicitly tested; see https://github.com/edx/edx-platform/pull/338/files#L5R23 |
||
| return None | ||
| return str[:-num_pad_bytes] | ||
|
|
||
| def encode(self, username): | ||
| return urlsafe_b64encode(self.cipher.encrypt(self._add_padding(username))) | ||
|
|
||
| def decode(self, encoded): | ||
| return self._remove_padding(self.cipher.decrypt(urlsafe_b64decode(encoded))) | ||
|
|
||
|
|
||
| def _validate_ajax(request): | ||
| """ | ||
| Ensure that `request` is valid | ||
|
|
||
| If the request is invalid, an appropriate response is returned. Otherwise, | ||
| None is returned. | ||
| """ | ||
| if request.method != "POST": | ||
| return HttpResponseNotAllowed(["POST"]) | ||
|
|
||
| if not request.user.is_authenticated(): | ||
| return HttpResponseForbidden() | ||
|
|
||
|
|
||
| def ajax_enable(request): | ||
| """ | ||
| A view that enables notifications for the authenticated user | ||
|
|
||
| This view should be invoked by an AJAX POST call. It returns status 204 | ||
| (no content) or an error. If notifications were already enabled for this | ||
| user, this has no effect. Otherwise, a preference is created with the | ||
| unsubscribe token (an ecnryption of the username) as the value.unsernam | ||
| """ | ||
| validation_response = _validate_ajax(request) | ||
| if validation_response is not None: | ||
| return validation_response | ||
|
|
||
| UserPreference.objects.get_or_create( | ||
| user=request.user, | ||
| key=NOTIFICATION_PREF_KEY, | ||
| defaults={ | ||
| "value": UsernameCodec().encode(request.user.username) | ||
| } | ||
| ) | ||
|
|
||
| return HttpResponse(status=204) | ||
|
|
||
|
|
||
| def ajax_disable(request): | ||
| """ | ||
| A view that disables notifications for the authenticated user | ||
|
|
||
| This view should be invoked by an AJAX POST call. It returns status 204 | ||
| (no content) or an error. | ||
| """ | ||
| validation_response = _validate_ajax(request) | ||
| if validation_response is not None: | ||
| return validation_response | ||
|
|
||
| UserPreference.objects.filter( | ||
| user=request.user, | ||
| key=NOTIFICATION_PREF_KEY | ||
| ).delete() | ||
|
|
||
| return HttpResponse(status=204) | ||
|
|
||
|
|
||
| def unsubscribe(request, token): | ||
| """ | ||
| A view that disables notifications for a user who may not be authenticated | ||
|
|
||
| This view is meant to be the target of an unsubscribe link. The request | ||
| must be a GET, and the `token` parameter must decrypt to a valid username. | ||
|
|
||
| A 405 will be returned if the request method is not GET. A 404 will be | ||
| returned if the token parameter is missing or if the given token does not | ||
| decrypt to a valid username. On success, the response will contain a page | ||
| indicating success. | ||
| """ | ||
| if request.method != "GET": | ||
| return HttpResponseNotAllowed(["GET"]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the require_GET() decorator instead?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely. I just didn't know that existed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, there's also a similar decorator for
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't use login_required, because that returns a redirect if the requirement is not satisfied. Do we have a library somewhere for general-purpose view decorators? |
||
|
|
||
| try: | ||
| username = UsernameCodec().decode(token.encode()) | ||
| user = User.objects.get(username=username) | ||
| except Exception as e: | ||
| raise Http404(e.message) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you differentiate between "it's not here" and "something exploded somewhere", and add logging in the latter case?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose so, but the only thing that would really tell us is that somebody has tried plugging in a random token.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured out a better way to do this |
||
|
|
||
| UserPreference.objects.filter(user=user, key=NOTIFICATION_PREF_KEY).delete() | ||
|
|
||
| return render_to_response("unsubscribe.html", {}) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please split the imports by type (stdlib, 3rd party, our own)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that was the preferred style. I will fix it.