feat: implement threshold for resetting password #6035

shreyanshdwivedi · 2019-06-12T13:45:01Z

Checklist

I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream development branch.
The unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

All the functions created/modified in this PR contain relevant docstrings.

Changes proposed in this pull request:

A threshold for an account reset email should be 3 times per hour. After that, the user receives a message: "You have reached the threshold for this action. Please try again in one hour."

shreyanshdwivedi · 2019-06-12T13:46:58Z

@uds5501 @mrsaicharan1 @kushthedude I'm not getting the time in the same format as it is getting stored in the db. For example, the time getting stored in db is - 2019-06-12 13:36:29.233271+05:30 and one I'm getting is 2019-06-12 13:36:29.227708+00:00. Please note the +5:30 and 00:00 as difference. I'm not able to get correct timezone, I guess. Can you guys help me out to find what am I missing here?

app/api/auth.py

kushthedude · 2019-06-12T13:50:17Z

@shreyanshdwivedi We are not using pytz for time now , As recent change made by @prateekj117 we are now using SQLalchemy inbuilt time stamp function, Check there if you find a way to retireve time from db

app/api/auth.py

codecov · 2019-06-12T14:05:57Z

Codecov Report

Merging #6035 into development will increase coverage by 0.01%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##           development    #6035      +/-   ##
===============================================
+ Coverage        66.12%   66.13%   +0.01%     
===============================================
  Files              285      285              
  Lines            14089    14095       +6     
===============================================
+ Hits              9316     9322       +6     
  Misses            4773     4773

Impacted Files	Coverage Δ
app/api/auth.py	`22.64% <100%> (+1.34%)`	⬆️
app/__init__.py	`86.93% <100%> (+0.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d78839b...64f0593. Read the comment docs.

niranjan94

Related comment: #6033 (comment)

Explore using https://flask-limiter.readthedocs.io/en/stable/ for this. That way we can extend it to other endpoints too.

shreyanshdwivedi · 2019-06-12T14:09:14Z

@niranjan94 so should I use flask_limiter here rather than this approach for password-reset?

app/api/auth.py

niranjan94 · 2019-06-12T14:16:48Z

so should I use flask_limiter here rather than this approach for password-reset?

Yep. Correct. And the frontend can show an appropriate message to the user if it gets a 429 response code.

shreyanshdwivedi · 2019-06-12T14:19:25Z

@niranjan94 ok cool. But one thing, the issue mentioned -

A threshhold for an account reset email should be 3 times per hour. After that the user should receive a message: "You have reached the threshhold for this action. Please try again in one hour."

But I don't think this type of message for a particular user can be achieved by flask_limiter.

niranjan94 · 2019-06-12T14:20:13Z

But I don't think this type of message can be achieved by flask_limiter.

@shreyanshdwivedi

#6035 (comment)

app/api/auth.py

app/__init__.py

app/api/auth.py

shreyanshdwivedi · 2019-06-17T16:21:51Z

@iamareebjamal @niranjan94 I've updated the key_func and finalized the PR. I've checked it on local and it works for me. The error message produced -

429 Too Many Requests
Too Many Requests
Limit for this action exceeded

Please review

iamareebjamal · 2019-06-18T15:50:42Z

app/api/auth.py


 @auth_routes.route('/reset-password', methods=['POST'])
+@limiter.limit(
+    '3/hour', key_func=lambda: request.json['data']['email'], error_message='Limit for this action exceeded'


jamal.areeb@gmail.com
jamalareeb@gmail.com
ja.mal.are.eb@gmail.com

Are all same email addresses and will be allowed by this method

@iamareebjamal I just tested the above three email addresses.
Each one is distinguished perfectly and failed when the email if entered fourth time. I jumbled the order and tested, the reset request for one email is not affecting other in any way

I jumbled the order and tested, the reset request for one email is not affecting other in any way

It should

The point is that I can keep adding dots and sending emails at a rate of more than 3/hour

I can loop through a list of emails and send password reset request on them. Essentially brute forcing

But there is a check which raises error if user is not registered and so no mail is sent out. If a user loops by adding dots, it doesn't mean the email is registered and so error will be raised. I hope it makes sense to you.

A hacker doesn't care if the error is raised. He'll just continue looping through emails, and our resources will get exhausted. And users will still get spammed.

Also, raising error if user is not registered is a pretty big security issue, it should be fixed ASAP - #6069

@iamareebjamal from what I understood, I should create a function which treats similar emails as same email. Right?
Also can you please guide me which pattern should I consider ?

Instead, you should add one more limiter which prevents same IP to access the unprotected resource at a lower rate, like '1/minute'

@iamareebjamal I've updated the PR. Please review

uds5501

This looks good!

* feat: Implement flask limiter on password-reset route * adds limiter to limit the request from same IP

houndci-bot reviewed Jun 12, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

prateekj117 reviewed Jun 12, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

niranjan94 suggested changes Jun 12, 2019

View reviewed changes

houndci-bot reviewed Jun 12, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

app/api/auth.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the limitEmail branch from 44aa4eb to 1fe99a2 Compare June 12, 2019 14:48

iamareebjamal reviewed Jun 12, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

iamareebjamal reviewed Jun 12, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

fossasia deleted a comment Jun 12, 2019

shreyanshdwivedi force-pushed the limitEmail branch from 1fe99a2 to 61a4a9d Compare June 17, 2019 15:50

houndci-bot reviewed Jun 17, 2019

View reviewed changes

app/__init__.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the limitEmail branch 2 times, most recently from 262ab94 to aa9782e Compare June 17, 2019 15:58

houndci-bot reviewed Jun 17, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the limitEmail branch 2 times, most recently from ba748fc to 5700b69 Compare June 17, 2019 16:15

houndci-bot reviewed Jun 17, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the limitEmail branch from 5700b69 to e8030d8 Compare June 17, 2019 16:18

houndci-bot reviewed Jun 17, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the limitEmail branch from e8030d8 to 16d9b2b Compare June 17, 2019 16:18

shreyanshdwivedi changed the title ~~[WIP] feat: implement threshold for resetting password~~ feat: implement threshold for resetting password Jun 17, 2019

auto-label bot added the feature label Jun 17, 2019

iamareebjamal reviewed Jun 18, 2019

View reviewed changes

shreyanshdwivedi force-pushed the limitEmail branch from 16d9b2b to 64f0593 Compare June 19, 2019 07:33

shreyanshdwivedi added 2 commits June 19, 2019 13:03

feat: Implement flask limiter on password-reset route

8171102

adds limiter to limit the request from same IP

64f0593

iamareebjamal approved these changes Jun 19, 2019

View reviewed changes

uds5501 approved these changes Jun 19, 2019

View reviewed changes

kushthedude approved these changes Jun 19, 2019

View reviewed changes

iamareebjamal merged commit 329e586 into fossasia:development Jun 19, 2019

iamareebjamal pushed a commit to iamareebjamal/open-event-server that referenced this pull request Aug 2, 2019

feat: implement threshold for resetting password (fossasia#6035)

c40c4e7

* feat: Implement flask limiter on password-reset route * adds limiter to limit the request from same IP

feat: implement threshold for resetting password #6035

feat: implement threshold for resetting password #6035

Uh oh!

Conversation

shreyanshdwivedi commented Jun 12, 2019

Checklist

Changes proposed in this pull request:

Uh oh!

shreyanshdwivedi commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kushthedude commented Jun 12, 2019

Uh oh!

Uh oh!

codecov bot commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

niranjan94 left a comment

Choose a reason for hiding this comment

Uh oh!

shreyanshdwivedi commented Jun 12, 2019

Uh oh!

Uh oh!

Uh oh!

niranjan94 commented Jun 12, 2019

Uh oh!

shreyanshdwivedi commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niranjan94 commented Jun 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shreyanshdwivedi commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamareebjamal Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uds5501 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shreyanshdwivedi commented Jun 12, 2019 •

edited

Loading

codecov bot commented Jun 12, 2019 •

edited

Loading

shreyanshdwivedi commented Jun 12, 2019 •

edited

Loading

iamareebjamal Jun 18, 2019 •

edited

Loading