[ENG-607][ENG-787] Send redirect URL's to Akismet and validate URLs #9087

Johnetordoff · 2019-07-09T19:54:11Z

Purpose

Currently some spammer have taken to using our redirect link feature to funnel traffic to their site. This PR sends the links off to askismet to be vetted.

Changes

SpamMixin can now have joined fields, so we can query on addons_forward_node_settings__url
tests for v1 and v2

QA Notes

Add spammy redirect links, see what happens.

Also verify spammy titles and spammy descriptions behavior still works as expected.

Documentation

No user face, no docs.

Side Effects

None that I know of.

Ticket

https://openscience.atlassian.net/browse/ENG-607

corbinSanders · 2019-07-15T13:49:29Z

Looks good!

pattisdr

interesting approach @Johnetordoff! a few changes, and I would recommend moving the spam checking back to the model instead of individually checking on each of the serializers if possible.

addons/forward/tests/test_views.py

api/nodes/serializers.py

pattisdr · 2019-07-24T00:04:21Z

api/nodes/serializers.py

@@ -890,7 +890,9 @@ def create(self, validated_data):
 class ForwardNodeAddonSettingsSerializer(NodeAddonSettingsSerializerBase):


oh my goodness. We have two separate v2 views/serializers which handle updating the forward addon.

PATCH http://localhost:8000/v2/nodes/9bknu/addons/forward/ implemented by Matt in 2016 #6122

PATCH http://localhost:8000/v2/nodes/9bknu/settings/. Erin and I implemented this last year as part of the NodeSettings endpoint #8536.

John's work adds the spam check in Matt's v2 endpoint, but not in ours - NodeSettingsUpdateSerializer. At minimum, both v2 serializers should handle spam checking on the forward addon, but it's my opinion we should deprecate one of them in a follow-up PR, after Emberization occurs.

Matt's: added first, current legacy page uses this v1 view, appears to be documented

Dawn's/Erin's: appears to be documented, if I remember right, this was planned as part of the emberized work? Does Emberized page plan to use to the NodeSettingsSerializer to update the forward addon? (Can you weigh in tomorrow @brianjgeiger ?)

api/nodes/serializers.py

osf/models/mixins.py

into askismet-redirect-url * 'develop' of https://github.com/CenterForOpenScience/osf.io: (59 commits) Update FAQ link Bump version and update changelog. add license check move comment to docstring fix waffle test fixture make task check DOIs in batches of 20 DRY up row encoding move tests into TCI/linting range gotta pin to trusty for now, my default build xenial is bugged with ES add check for DOI to CeleryConfigs fix/clean-up crossref DOI checking exclude preprints that have only been "stuck" a short time from unsticking process change crossref pending period and fix type make dry_run default to true for DOIs fix test spruce up emails for stuck crossref DOIs save DOI identifier values clean up logging statement for DOIs Bump version and update changelog. use sync_manuscript insteand ...

api_tests/nodes/views/test_node_wiki_list.py

pattisdr

hey @Johnetordoff - passing back because some of my comments weren't addressed - I think we're pretty close though!

.travis.yml

api/nodes/serializers.py

addons/forward/tests/test_views.py

api/addons/forward/test_views.py

osf/models/mixins.py

into askismet-redirect-url * 'develop' of https://github.com/CenterForOpenScience/osf.io: (24 commits) Bump version and update changelog. leave Travis confused, but completely unharmed isolate bug feat: expose preprint request actions to requester and preprint admins try new travis config for waffle cookies make osftestcase into pytest for waffle cookie getcwd for Travis remove thrown exception for travis more fixes for travis remove exception catching for request context error move __future__ import to test files only try to fix for travis remove out of context request checking fix for travis fix for travis Remove outdated migration Rename migration and fix dependency Add migration for ab_testing_home_page_version_b waffle flag move and fix waffle tests remove wafflejs urls and comments ...

brianjgeiger

Clarification based on my understanding

pattisdr · 2019-08-07T13:13:46Z

addons/forward/models.py

+            else:
+                user = request.user
+
+            self.owner.check_spam(user, {'addons_forward_node_settings__url'}, get_headers_from_request(request))


wondering if we should only check spam if the url has actually been changed - instead of every time the forward addon is saved.

Hmmm. From a performance standpoint, I doubt it matters much, since I don't think the forward addon is used much. If we trust the spam filter a lot, then it's better to check each time, in case we modify the spam filter to have a better understanding of what is spam. If we are concerned about false positives (which I don't believe we are), then not checking as frequently is better.

It's a bit of a trade off, if we only want to run this when the user changes something it should probably go back in the serializer.

I agree with @brianjgeiger, i'm not super concerned about false positives and this isn't going to be run a lot. I do prefer having it on the model since it's being called in 3 places -

👍 agreed I think this is better from a data integrity standpoint and also easier to remove if we depreciate the Forward addon.

pattisdr

LGTM @Johnetordoff 🚢

pattisdr · 2019-08-28T19:10:10Z

branch merged into feature/staging-1 for testing

Johnetordoff force-pushed the askismet-redirect-url branch from d4d025d to 97782a1 Compare July 9, 2019 20:15

hack together fix to send redirect url to akismet

1db904b

Johnetordoff force-pushed the askismet-redirect-url branch from 97782a1 to 1db904b Compare July 9, 2019 21:30

pattisdr suggested changes Jul 24, 2019

View reviewed changes

Johnetordoff added 5 commits July 24, 2019 13:16

seperate v1 and v2 tests

99b8e18

exclude null values from spam content look up

a6bd6ac

add for travis to pass

a310456

add functionality for node setting Forward route

847a93f

Johnetordoff force-pushed the askismet-redirect-url branch from 994f867 to 21ff9af Compare July 29, 2019 13:22

de-randomize testmon value

7bbac65

Johnetordoff commented Jul 29, 2019

View reviewed changes

api_tests/nodes/views/test_node_wiki_list.py Show resolved Hide resolved

pattisdr suggested changes Jul 31, 2019

View reviewed changes

Johnetordoff added 2 commits July 31, 2019 09:53

add more docs and remove line for askismet redirect link

647c1dc

Johnetordoff force-pushed the askismet-redirect-url branch from e956e35 to 0c7fd56 Compare July 31, 2019 14:05

remove travis trusty dist

92bca4a

brianjgeiger reviewed Jul 31, 2019

View reviewed changes

Johnetordoff changed the title ~~[ENG-607] Send redirect URL's to Akismet~~ [ENG-607] Send redirect URL's to Akismet and validate URLs Jul 31, 2019

Johnetordoff added 2 commits July 31, 2019 12:26

validate url for NodeAddonSettingsSerializerBase

cccf63f

move check for spam into nade addon model

d5549c3

Johnetordoff force-pushed the askismet-redirect-url branch from b384a49 to d5549c3 Compare July 31, 2019 17:07

pattisdr reviewed Aug 7, 2019

View reviewed changes

pattisdr approved these changes Aug 7, 2019

View reviewed changes

Johnetordoff changed the title ~~[ENG-607] Send redirect URL's to Akismet and validate URLs~~ [ENG-607][ENG-787] Send redirect URL's to Akismet and validate URLs Aug 28, 2019

Merge branch 'develop' into askismet-redirect-url

dc040e9

pattisdr merged commit 064c48b into CenterForOpenScience:develop Sep 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-607][ENG-787] Send redirect URL's to Akismet and validate URLs #9087

[ENG-607][ENG-787] Send redirect URL's to Akismet and validate URLs #9087

Johnetordoff commented Jul 9, 2019 •

edited by pattisdr

Loading

corbinSanders commented Jul 15, 2019

pattisdr left a comment

pattisdr Jul 24, 2019

pattisdr left a comment

brianjgeiger left a comment

pattisdr Aug 7, 2019

brianjgeiger Aug 7, 2019

Johnetordoff Aug 7, 2019

pattisdr Aug 7, 2019

Johnetordoff Aug 7, 2019

pattisdr left a comment

pattisdr commented Aug 28, 2019

		@@ -890,7 +890,9 @@ def create(self, validated_data):
		class ForwardNodeAddonSettingsSerializer(NodeAddonSettingsSerializerBase):

[ENG-607][ENG-787] Send redirect URL's to Akismet and validate URLs #9087

[ENG-607][ENG-787] Send redirect URL's to Akismet and validate URLs #9087

Conversation

Johnetordoff commented Jul 9, 2019 • edited by pattisdr Loading

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

corbinSanders commented Jul 15, 2019

pattisdr left a comment

Choose a reason for hiding this comment

pattisdr Jul 24, 2019

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

pattisdr Aug 7, 2019

Choose a reason for hiding this comment

brianjgeiger Aug 7, 2019

Choose a reason for hiding this comment

Johnetordoff Aug 7, 2019

Choose a reason for hiding this comment

pattisdr Aug 7, 2019

Choose a reason for hiding this comment

Johnetordoff Aug 7, 2019

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

pattisdr commented Aug 28, 2019

Johnetordoff commented Jul 9, 2019 •

edited by pattisdr

Loading