Skip to content

Throttle protection for login/password views #54

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.

Sign up for GitHub

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

Jump to bottom
Merged
LilyFirefly merged 8 commits into from
Jun 2, 2014
Merged

Throttle protection for login/password views #54

LilyFirefly merged 8 commits into from
Jun 2, 2014

Conversation

@mlen108
Copy link
Contributor

@mlen108 mlen108 commented May 30, 2014

Using the DRM throttling functionality.

@mlen108 mlen108 changed the title Throttle login/password views Throttle protection for login/password views May 29, 2014
@mlen108 mlen108 self-assigned this May 30, 2014
@mlen108
Copy link
Contributor Author

mlen108 commented May 30, 2014

@meshy @ian-foote review?

@coveralls
Copy link

coveralls commented May 30, 2014

Coverage Status

Coverage remained the same when pulling 81ab260 on throttle-the-troll into 8f9e914 on master.

meshy
meshy reviewed May 30, 2014
View reviewed changes
user_management/tests/run.py Outdated
Copy link
Contributor

@meshy meshy May 30, 2014

Choose a reason for hiding this comment

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

Not sure it's fair to limit user login attempts to 10 in a day

Copy link
Contributor

@meshy meshy May 30, 2014

Choose a reason for hiding this comment

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

10 in an hour, perhaps

Copy link
Contributor Author

@mlen108 mlen108 May 30, 2014

Choose a reason for hiding this comment

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

Seems it's not required for the runner at all :)

Copy link
Contributor Author

@mlen108 mlen108 May 30, 2014

Choose a reason for hiding this comment

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

Going to remove it.

Copy link
Contributor

@meshy meshy May 30, 2014

Choose a reason for hiding this comment

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

Yeah I missed that this was tests-only. I think it may be good to define a default, however, as I believe that left as-is this has no effect at all.

@coveralls
Copy link

coveralls commented May 30, 2014

Coverage Status

Coverage remained the same when pulling a2fece4 on throttle-the-troll into 8f9e914 on master.

@LilyFirefly
Copy link

LilyFirefly commented May 30, 2014

Can you test the throttle? Something like setting the THROTTLE_RATE to something low, calling the view one extra time and asserting the final call returns the appropriate status code. I think that would be HTTP_429_TOO_MANY_REQUESTS.

@mlen108
Copy link
Contributor Author

mlen108 commented May 30, 2014

@ian-foote quick review?

@coveralls
Copy link

coveralls commented May 30, 2014

Coverage Status

Coverage remained the same when pulling c4c3157 on throttle-the-troll into 8f9e914 on master.

meshy
meshy reviewed May 30, 2014
View reviewed changes
user_management/api/tests/test_views.py Outdated
Copy link
Contributor

@meshy meshy May 30, 2014

Choose a reason for hiding this comment

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

Should you be testing with anon users too?

Copy link
Contributor Author

@mlen108 mlen108 May 30, 2014

Choose a reason for hiding this comment

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

SimpleRateThrottle is the base class for both, anon and non-anon users.

Copy link
Contributor

@meshy meshy May 30, 2014

Choose a reason for hiding this comment

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

Why do you need to pass anon and user?

@coveralls
Copy link

coveralls commented May 30, 2014

Coverage Status

Coverage remained the same when pulling b763471 on throttle-the-troll into 8f9e914 on master.

@coveralls
Copy link

coveralls commented Jun 2, 2014

Coverage Status

Coverage remained the same when pulling 1a448ae on throttle-the-troll into 8f9e914 on master.

meshy
meshy reviewed Jun 2, 2014
View reviewed changes
user_management/api/tests/test_views.py Outdated
Copy link
Contributor

@meshy meshy Jun 2, 2014

Choose a reason for hiding this comment

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

If this is only testing "AnonRate", then do you need the user in this dict?

Copy link
Contributor Author

@mlen108 mlen108 Jun 2, 2014

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

@meshy meshy Jun 2, 2014

Choose a reason for hiding this comment

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

I don't see why. scope equals 'anon', so that will pass without failing if you only pass 'anon': '1/day', without the user key.

Copy link
Contributor Author

@mlen108 mlen108 Jun 2, 2014

Choose a reason for hiding this comment

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

Ahh I've removed the config from wrong test case ;-) All good now.

@coveralls
Copy link

coveralls commented Jun 2, 2014

Coverage Status

Coverage remained the same when pulling fcb0267 on throttle-the-troll into 8f9e914 on master.

@mlen108
Copy link
Contributor Author

mlen108 commented Jun 2, 2014

Updated.

@coveralls
Copy link

coveralls commented Jun 2, 2014

Coverage Status

Coverage remained the same when pulling 9fbeb57 on throttle-the-troll into 8f9e914 on master.

LilyFirefly pushed a commit that referenced this pull request Jun 2, 2014
Merge pull request #54 from incuna/throttle-the-troll
729e6eb 
Throttle protection for login/password views
@LilyFirefly LilyFirefly merged commit 729e6eb into master Jun 2, 2014
@LilyFirefly LilyFirefly deleted the throttle-the-troll branch June 2, 2014 10:44
@mlen108
Copy link
Contributor Author

mlen108 commented Jun 2, 2014

Thanks!

@mlen108
Copy link
Contributor Author

mlen108 commented Jun 2, 2014

Released to PyPI too.

@LilyFirefly
Copy link

LilyFirefly commented Jun 2, 2014

✨ 👍 ✨

@mlen108 mlen108 removed their assignment Dec 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mlen108 @coveralls @LilyFirefly @meshy