Skip to content
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.

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

Send password reset link when creating new users with e-mail #18485

Closed
wants to merge 1 commit into from

Conversation

Takuto88
Copy link
Contributor

This implements #17398.

Instead of the Login-URL the user is sent to the reset password form where he can set his initial password.

Important note about unit test
I've done my best to update the unit test but I am unable to get the unit tests working on my system at all so this is blind.

Running ./autotest.sh sqlite as described in the developer manual just produces this output on my system:

Using PHP executable /usr/local/bin/php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

Using database oc_autotest
Setup environment for sqlite testing ...
Installing ....
ownCloud is not installed - only a limited number of commands are available
Mac OS X is not supported and ownCloud will not work properly on this platform. Use it at your own risk! 
 -> For the best results, please consider using a GNU/Linux server instead.

PHPUnit is installed, running OS X 10.10.5 which seems to be unsupported?. Any pointers are welcome.

ISecureRandom::CHAR_DIGITS.
ISecureRandom::CHAR_LOWER.
ISecureRandom::CHAR_UPPER);
$this->config->setUserValue($username, 'owncloud', 'lostpassword', $token);
Copy link
Member

Choose a reason for hiding this comment

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

This is error-prone as it duplicates the logic from the controller and will lead to problems once this gets changed, please make this a little bit centrally.

(that said this will collide with #18491 and one of those two needs to get rebased)

Copy link
Member

Choose a reason for hiding this comment

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

a little bit centrally => a central class for this stuff but not twice please ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LukasReschke Good catch. But a new class just for that would probably be too a bit too much. Is there an existing place where this should go?

We could add something like IUser::createPasswordResetToken() to encapsulate this and return the token from it. I don't know the codebase of ownCloud that well (yet) so there probably is something more appropriate that I have missed.

Copy link
Member

Choose a reason for hiding this comment

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

Mhm… IUser would be public API. I would love to avoid having it in the public API, nothing that app authors should mess with in any way.

Let me think about something. I'm anyways refactoring some auth stuff at the moment 🙊

@LukasReschke
Copy link
Member

PHPUnit is installed, running OS X 10.10.5 which seems to be unsupported?. Any pointers are welcome.

The whole unit test suite on OS X will fail. cd into test and do phpunit lib/foo.php on single files or use a Linux VM to run the test suite.

@Takuto88
Copy link
Contributor Author

Running a single test should be fine for now. I just want to make sure that I don't break stuff and make Mr. Vader unhappy ;-).

phpunit settings/controller/userscontrollertest.php just gives me:

PHP Fatal error:  Uncaught exception 'Doctrine\DBAL\DBALException' with message 'Failed to connect to the database: An exception occured in driver: SQLSTATE[HY000] [2002] No such file or directory' in /Users/lennart/git/owncloud/core/lib/private/db/connection.php:52
Stack trace:
#0 /Users/lennart/git/owncloud/core/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php(429): OC\DB\Connection->connect()
#1 /Users/lennart/git/owncloud/core/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php(389): Doctrine\DBAL\Connection->getDatabasePlatformVersion()
#2 /Users/lennart/git/owncloud/core/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php(328): Doctrine\DBAL\Connection->detectDatabasePlatform()
#3 /Users/lennart/git/owncloud/core/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php(621): Doctrine\DBAL\Connection->getDatabasePlatform()
#4 /Users/lennart/git/owncloud/core/lib/private/db/connection.php(135): Doctrine\DBAL\Connection->setTransactionIsolation(2)
#5 /Users/lennart/git/owncloud/core/3rdparty/doctrine/dbal/lib/ in /Users/lennart/git/owncloud/core/lib/private/db/connection.php on line 52

I am missing something here. It probably does not use the correct config.php and tries something like an sqlite db or a socket file that does not exist. I did export CONFIG_DIR=tests/config and have a valid config.php in there which is otherwise known working when accessing owncloud through Apache.

Thanks for your patience with me.

@xylo
Copy link

xylo commented Oct 7, 2015

Any updates on this issue?

@DeepDiver1975 DeepDiver1975 added this to the 9.0-next milestone Oct 7, 2015
@hutchic hutchic mentioned this pull request Nov 7, 2015
@hutchic
Copy link
Contributor

hutchic commented Nov 7, 2015

Put up a PR that fixes the failing test #20379

@PVince81
Copy link
Contributor

Please rebase, there are conflicts.

Also, this probably needs a config.php setting disabled by default to make it optional behavior.

@MorrisJobke MorrisJobke modified the milestones: 9.1-next, 9.0-current Mar 4, 2016
@PVince81
Copy link
Contributor

@Takuto88 can you rebase ? Note that master now has some new APIs and with a bit of luck this might become easier / cleaner to implement.

CC @ChristophWurst

@MorrisJobke
Copy link
Contributor

00005462

@ghost
Copy link

ghost commented May 30, 2016

@Takuto88

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@DeepDiver1975
Copy link
Member

@Takuto88

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

funny - looks like @Takuto88 got remove from the core developer team.

@Takuto88
Copy link
Contributor Author

@DeepDiver1975: I've left the organisation because sadly, I have little time to contribute to this project. I have signed the contribution agreement therefore any contributions by me can be used by the owncoud project.

@DeepDiver1975
Copy link
Member

I've left the organisation because sadly, I have little time to contribute to this project. I have signed the contribution agreement therefore any contributions by me can be used by the owncoud project.

Ah - this explains this ... well ... we use the team membership to identify if the agreement was signed or not.

@PVince81
Copy link
Contributor

PVince81 commented Jun 9, 2016

Would move to 9.2 as this is a new feature and we're past feature freeze.

Also I think this needs some kind of config switch for admins who don't want to automatically send these temp passwords.

CC @felixboehm

@xylo
Copy link

xylo commented Jun 13, 2016

@PVince81
I'm not sure if an configuration option is needed. The mail template has to be edited anyway if you want to activate/deactivate the email reset link. So if an admin doesn't want the password reset link in the user registration email he/she just hast to remove it from the mail template (or the other way around to be backward compatible)

@brknkfr
Copy link

brknkfr commented Jun 15, 2016

This exactly what I was looking for, but there is a problem. This won't work with enabled encryption app, because keys aren't yet initialized.

@AlexVrg
Copy link

AlexVrg commented Jul 4, 2016

Doesn't seem to work with version 9.0.2. When trying to setup the password it reports that token is invalid.

@PVince81 PVince81 modified the milestones: 10.0.1, 10.0 Apr 6, 2017
@DeepDiver1975 DeepDiver1975 modified the milestones: 10.0.1, 10.1 May 17, 2017
@PVince81
Copy link
Contributor

PVince81 commented Jul 4, 2017

closing due to lack of feedback / abandonned.

there is a new version here: #28113

@PVince81 PVince81 closed this Jul 4, 2017
@PVince81 PVince81 deleted the implement-17398 branch July 4, 2017 15:01
@DeepDiver1975 DeepDiver1975 modified the milestones: 10.1, development Oct 10, 2017
@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants