Skip to content

fix: updates message on reset password if user isn't registered #6079

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
merged 1 commit into from
Jun 26, 2019
Merged

fix: updates message on reset password if user isn't registered #6079

merged 1 commit into from
Jun 26, 2019

Conversation

@shreyanshdwivedi
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi commented Jun 20, 2019

Fixes #6069

Short description of what this resolves:

Raising an error on password reset if the user is not registered is like gifting hackers a list of users present in the database.

Changes proposed in this pull request:

  • Updates the message is user is not registered on reset password

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.

@auto-label auto-label bot added the fix label Jun 20, 2019
@codecov
Copy link

codecov bot commented Jun 20, 2019 •
edited
Loading

Codecov Report

Merging #6079 into development will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #6079      +/-   ##
==============================================
+ Coverage        66.19%   66.2%   +<.01%     
==============================================
  Files              285     285              
  Lines            14195   14197       +2     
==============================================
+ Hits              9397    9399       +2     
  Misses            4798    4798
Impacted Files Coverage Δ
app/api/auth.py 24.22% <50%> (+0.67%) ⬆️

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 af2ee84...2f04bc2. Read the comment docs.

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Jun 20, 2019

@iamareebjamal the server responds with message If your email was registered with us, you'll get an email with reset link shortly and status 200 always. Do you want to update FE message too? Currently it shows - Please go to the link sent to your email to reset your password

iamareebjamal
iamareebjamal reviewed Jun 20, 2019
View reviewed changes
iamareebjamal
iamareebjamal reviewed Jun 20, 2019
View reviewed changes
@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Jun 20, 2019

@iamareebjamal I've updated the PR. Please review

iamareebjamal
iamareebjamal reviewed Jun 20, 2019
View reviewed changes
@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Jun 21, 2019

@iamareebjamal I'm logging email for both the cases so that user cannot differentiate from response that whether email is registered or not. Please review

@iamareebjamal
Copy link
Member

iamareebjamal commented Jun 21, 2019

Logs are for us and not the user. I want to differentiate between valid and non valid emails

@shreyanshdwivedi shreyanshdwivedi force-pushed the errorOnReset branch 2 times, most recently from eb44044 to 22cabff Compare June 21, 2019 11:21
@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Jun 21, 2019

@iamareebjamal I have logged email in case of NoResultFound exception. Please have a look

iamareebjamal
iamareebjamal reviewed Jun 25, 2019
View reviewed changes
app/api/auth.py Outdated
user = User.query.filter_by(email=email).one()
except NoResultFound:
return NotFoundError({'source': ''}, 'User not found').respond()
logging.info(email)
Copy link
Member

@iamareebjamal iamareebjamal Jun 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Create a logger. Never use root logging module.

Also, logging just the email makes no sense. What will I know from that. Properly log Tried to reset password not existing email

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Jun 26, 2019

@iamareebjamal created a logger and logged the proper info. Please review

mrsaicharan1
mrsaicharan1 approved these changes Jun 26, 2019
View reviewed changes
Copy link
Member

@mrsaicharan1 mrsaicharan1 left a comment

Choose a reason for hiding this comment

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

Previously logging info wasn't there but now it's fine. LGTM

@iamareebjamal iamareebjamal merged commit 88d1b13 into fossasia:development Jun 26, 2019
iamareebjamal pushed a commit to iamareebjamal/open-event-server that referenced this pull request Aug 2, 2019
@shreyanshdwivedi @iamareebjamal
fix: updates message on reset password if user isn't registered (foss…
f7820d3 
…asia#6079)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iamareebjamal iamareebjamal iamareebjamal approved these changes

+3 more reviewers

@abhinavk96 abhinavk96 abhinavk96 approved these changes

@uds5501 uds5501 uds5501 approved these changes

@mrsaicharan1 mrsaicharan1 mrsaicharan1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error raised on reset password if user is not registered

5 participants

@shreyanshdwivedi @iamareebjamal @abhinavk96 @uds5501 @mrsaicharan1