Allowing email for password recovery #27168

Merged
merged 4 commits into from Feb 27, 2017

Projects

None yet

5 participants

@imujjwal96
Contributor
imujjwal96 commented Feb 15, 2017 edited

Description

The function sendEmail in core/LostController.php was checking if a user with the given name exists or not. What I did is, in the else condition used the function getByEmail() and checked if the count of users is 0, 1 or else. In the first case, the following error was logged

{"reqId":"kPpSJNX5ONqosvDdN/QO","remoteAddr":"119.82.86.231","app":"core","message":"Couldnot send reset email because User does not exist. User: jhghjgj","level":3,"time":"2017-02-15T19:31:21+00:00","method":"POST","url":"\/core\/index.php\/lostpassword\/email","u$

and in the third case

{"reqId":"iuivYECI2anrgAXQP4Wx","remoteAddr":"119.82.86.231","app":"core","message":"Couldnot send reset email because the email id is not unique. User: ujjwalb1996@gmail.com","level":3,"time":"2017-02-15T19:31:32+00:00","method":"POST","url":"\/core\/index.php\/los$

A recovery mail was sent only when the count of email was singular in this case.

Related Issue

#27111

Motivation and Context

It solves the problem raised in issue: #27111

How Has This Been Tested?

I forked and clone the repository in my VPS and tested their with all the changes. Recovery mails were received by providing either username or an email address(unique) associated to that user. If a recovery mail was requested for an incorrect username(non existing) or for an email address that was not unique, proper error messages were logged and check in the core/owncloud.log file.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@imujjwal96 imujjwal96 Allowing email for password recovery
74f42a8
@mention-bot

@imujjwal96, thanks for your PR! By analyzing the history of the files in this pull request, we identified @VicDeo, @DeepDiver1975 and @Peter-Prochaska to be potential reviewers.

@CLAassistant
CLAassistant commented Feb 15, 2017 edited

CLA assistant check
All committers have signed the CLA.

@tomneedham
Member

Hey @imujjwal96 thanks for the PR! Just checking out the code, we're introducing a fair bit of duplicated code here. Perhaps you could alter this to either find the user by name, or by email, and once you have the user, pass that to the code that sends the email.

So maybe it should be like this:

  • Try find by username
    • No user => error/log
    • Do we have an email? No => error/log
  • Try find by email
    • Is it unique? No => error/log
  • Send email code
imujjwal96 added some commits Feb 17, 2017
@imujjwal96 imujjwal96 Remove redundancy following PR: #27168
d38b082
@imujjwal96 imujjwal96 Update to remove Test failure
fc97b16
core/Controller/LostController.php
+ $this->logger->info('User with input as email address found. User: {user}', ['app' => 'core', 'user' => $user]);
+ $email = $user;
+ $user = $users[0]->getUID();
+ break;
@PVince81
PVince81 Feb 23, 2017 Collaborator

return false ?

@PVince81
PVince81 Feb 23, 2017 Collaborator

Never mind, I misunderstood the logic.

core/Controller/LostController.php
+ return false;
+ case 1:
+ $this->logger->info('User with input as email address found. User: {user}', ['app' => 'core', 'user' => $user]);
+ $email = $user;
@PVince81
PVince81 Feb 23, 2017 Collaborator

I suggest using the Email address as delivered by $users[0]->getEmail(), this way we are sure it will use the same casing and not rely on direct input. It could also help reduce the likelines of security issues by not directly using whatever was input as a target email (in case of security holes)

@imujjwal96
imujjwal96 Feb 23, 2017 Contributor

Ok. I will do that.
Are there any other changes that you want me to do? I will club them all into a single and final commit.

@PVince81
Collaborator

The code looks good otherwise. @tomneedham

@imujjwal96 imujjwal96 Take email from user info instead of input
49a1e3a
@PVince81 PVince81 added this to the 10.0 milestone Feb 27, 2017
@PVince81
Collaborator

I have tested this and it works fine, nice work! 👍

@PVince81 PVince81 merged commit c9fa309 into owncloud:master Feb 27, 2017

4 checks passed

Scrutinizer 1 new issues
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81
Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment