Simpler forgot password functionality #2393

woogerboy21 · 2017-01-28T20:39:57Z

Here is a re-submission of the forgot password functionality. This version is much simpler. Here is how it works:

client->server (send request)
server-> verify functionality enabled
server-> validate user exists
server-> verify user isnt banned
server-> check if challenge questions enabled

if ( checks pass AND challenge enabled )
server->client (request client to answer question)
client->server (send challenge answer)
server-> verify challenge questions
if (challenge questions pass)
server-> mark account for reset request
server-> email client token
server->client (request token + new pass)
client->server (send token+pass)
server-> check token
if (token correct)
server-> update pass
server->client (notify of success)

if ( checks pass AND challenge disabled )
server-> mark account for reset request
server-> email client token
server->client (request token + new pass)
client->server (send token+pass)
server-> check token
if (token correct)
server-> update pass
server->client (notify of success)

Currently, if at any time the checks fail, the user is informed of a failure. Now I know at one time or another there was some discussion about possibly changing the behavior on how things are presented to the client. If things need adjusted they should be pretty easily changed. If I remember correctly the only behavior change that may need to be made is if the user types the email in wrong that instead of telling them "fail" do a "succeed" so if that's needed let me know (its just a change on which signal to emit during the response processing).

Some other things to note, unlike the last revision the client now handles storing the inputted information on the dialog and stores it throughout the process in new settings that are cleared upon success. With this configuration dialog(s) can be closed and reopened later and the process continue.

Check it out and let me know what things need "rejiggered".

Updated missed slotname for button trigger

Implemented basic server socket interface command handling

Created most logic for server side request function.

woogerboy21 · 2017-01-28T20:41:33Z

Also, this should be progress (if not a fix) for #1184

woogerboy21 · 2017-01-28T21:35:22Z

If users would like to test this PR I have a server up and running with the codebase on it.
Host: testing.woogerworks.com
Port: 4747

tooomm · 2017-01-29T21:02:05Z

For comparison, the former C-S communication concept in the old pr!
Changes mostly dropping the clientID and ip check and restructuring the challenge questions?

We don't always want to validate the reset request in one way or the other?
Just asking for the username (public information) seems odd and could be abused, allows for user harassment (token spamming) or server stress. Are you not concerned here?

woogerboy21 · 2017-01-29T21:11:30Z

We don't always want to validate the reset request in one way or the other?
Just asking for the username (public information) seems odd and could be abused, allows for user harassment (token spamming) or server stress. Are you not concerned here?

The functionality direction given was to accept a request from a user for forgot password, allow for challenge question functionality (not requiring it) and then validate users forgot password request with a generated token sent via email. This version accomplishes those tasks. The operator has the ability to turn on / off the challenge question (in this case prompts the user for the email on file but can be expanded in the future) and the server flags the user account for password reset, generating a token that is only valid for the period of time indicated by the operator and emails the users email on file the token. Flagged accounts are also "un-flagged" when the user successfully logs in. However during each step of validation (either challenge or token verification) the check for if the user account is flagged for password reset is verified. If the account is not flagged it doesn't perform any actions and returns a failure to the user making the request.

I would agree there should be more checks in place, but from my understanding thats the "expand on in the future" part of the concept. What to me isn't clear is how / when the user should be told "there is an error" or "move on to the next step". From past discussions I believe the argument was to either silently continue regardless of issue with the thought of to not give away user account information in the db (which i dont think is the correct path personally but more than willing to change it and give it a try). Personally I feel the generic "failed" answer by the server is enough ambiguity. The user has no idea when a failure is returned from the server on if the functionality is simply disabled, if the account exist (or not), or something else like if the account is banned. The only place they may figure information out would be if the current challenge question is enabled and they type the email in wrong a failure happens and they could assume the email they typed in is incorrect. But thats not to say something as simple as a server failed to process the challenge for internal issue's of some sort (like db query failures). Not to mention if the challenge expands and there is more than a single item there would not be any understanding on which of the multiple challenge components failed.

As far as server stress, right now there is nothing to stop anyone from performing the actions you say for any of the command request. Some one could simply spam the server over and over with buddy list queries or registration requests and the server does nothing about it.

Edit:
We do actually have a check for the number of game commands that come in over a period of time. So people can't flood the server with game commands. But other commands (such as session) do not have such check.

woogerboy21 · 2017-01-29T21:24:24Z

FYI - I also intend on making an audit table PR in a different change since it was of interest through the last discussion but wanted to keep it separate at this time for clarity.

ZeldaZach

Looks good, simpler then before and we can always change things around in the future for added security and such server-sided.

woogerboy21 · 2017-02-09T01:25:07Z

I committed some of the other PR's that were approved and then re-based this one. It took one minor conflict resolution so I recompiled the test box if anyone is wanting to retry things on this. Otherwise if no comments are made over the next couple days I'll go ahead and merge (unless some one else wishes to merge it).

ZeldaZach · 2017-02-11T05:33:03Z

Just remember to tabs->spaces your work.

Daenyth

I don't intend to review the UI portion or client-side workflow interaction - I trust that Zach already has that covered with manual testing. I'm just focusing on architecture and backend implementation.

Can you squash down commits and please make sure tabs are all converted to spaces?

Daenyth · 2017-02-12T14:45:32Z

cockatrice/src/dlg_forgotpasswordchallenge.cpp

+{
+
+	QString lastfphost; QString lastfpport; QString lastfpplayername;
+	lastfphost = settingsCache->servers().getHostname("cockatrice.woogerworks.com");


Why is this server name hardcoded? This should come from the server info list

This line of code was simply pulled from the registration dialog. It is actually pulling the name from the servers info list (which is stored in the settings cache of the client), it however is pulling woogerworks name. By default the settings cache returns an empty server name. Should I could remove the name altogether?

BTW, I havent looked at the code for the getHostname function but the behavior I am seeing in my windows client is the dialog's present the last host I connected to and if there is none listed it returns the server name "cockatrice.woogerworks.com". Not sure if that is what the expected behavior is nor do I know if something like a mac client behaves differently.

Daenyth · 2017-02-12T14:46:00Z

cockatrice/src/dlg_forgotpasswordchallenge.cpp

+
+	if (settingsCache->servers().getFPHostname().isEmpty() && settingsCache->servers().getFPPort().isEmpty() && settingsCache->servers().getFPPlayerName().isEmpty())
+	{
+		QMessageBox::warning(this, tr("Forgot Password Challenge Warning"), tr("Opps, looks like something has gone wrong.  Please re-start the forgot password process by using the forgot password button on the connection screen."));


typo: Opps should be Oops. re-start should be restart

Daenyth · 2017-02-12T14:46:24Z

cockatrice/src/dlg_forgotpasswordrequest.cpp

+{
+
+	QString lastfphost; QString lastfpport; QString lastfpplayername;
+	lastfphost = settingsCache->servers().getHostname("cockatrice.woogerworks.com");


Hardcoded server here again.

See: #2393 (comment)

Daenyth · 2017-02-12T14:46:36Z

cockatrice/src/dlg_forgotpasswordreset.cpp

+{
+
+	QString lastfphost; QString lastfpport; QString lastfpplayername;
+	lastfphost = settingsCache->servers().getHostname("cockatrice.woogerworks.com");


See: #2393 (comment)

Daenyth · 2017-02-12T14:48:35Z

common/pb/session_commands.proto

+    extend SessionCommand {
+        optional Command_ForgotPasswordReset ext = 1022;
+    }
+    optional string user_name = 1;


Why are these all optional? At least user_name makes sense to be required, probably token also and new_password

I can make any of them required that you like, I didnt want to cause any issue's with future protocol manipulations on this process that's all. I'll make user_name for sure.

Edit: user_name is now required, should I make any other? My $0.02, no.

Daenyth · 2017-02-12T14:55:24Z

servatrice/src/servatrice_database_interface.cpp

@@ -1128,3 +1130,91 @@ int Servatrice_DatabaseInterface::checkNumberOfUserAccounts(const QString &email

    return 0;
 }
+
+bool Servatrice_DatabaseInterface::addForgotPassword(const QString &user)


Store the requestor ip address and client id

See comment : #2393 (comment)
Since everyone seemed really interested in a table for auditing , the very next PR I intend to submit (but left it out of this PR for simplicity) is to create an audit table that will start with auditing information on registration, forgot password, and log queries done by admins. I can see the possibility of expanding to other aspects at some point as well. Have to start some were though. The audit table was in the first "forgot password" PR but was not moved over to this PR.

Daenyth · 2017-02-12T14:57:31Z

servatrice/src/servatrice_database_interface.cpp

+	if (token.isEmpty() || user.isEmpty())
+		return false;
+
+	QSqlQuery *query = prepareQuery("update {prefix}_users set token = :token where name = :user_name");


Can you tell me how this update method is used? I'm a little leery of reusing the users token field - I thought that was used only for non-activated accounts? It seems like we should store the token in the forgot password table, and then only check there when we get the reset-with-token proto message.

Currently the token field is used to store activation tokens generated for users when registering. Once the account is active the code is no longer used any were else in the system. Since the token is no longer needed after activation I see no issue with generating a new token, storing it here and sending it to the user for forgot password reasons. Any reason you can think of?

Daenyth · 2017-02-12T14:58:59Z

servatrice/src/serversocketinterface.cpp

+	if (!sqlInterface->doesForgotPasswordExist(QString::fromStdString(cmd.user_name())))
+		return Response::RespFunctionNotAllowed;
+
+	// if (!sqlInterface->validateUserToken(QString::fromStdString(cmd.token()), QString::fromStdString(cmd.user_name())))


Don't leave in commented code

Yup, this was commented out when I built the challenge questions. I'll remove them.

Daenyth · 2017-02-12T15:00:01Z

servatrice/src/serversocketinterface.cpp

+	if (sqlInterface->doesForgotPasswordExist(QString::fromStdString(cmd.user_name())))
+		return Response::RespOk;
+
+	//if (sqlInterface->validateUserEmail(QString::fromStdString(cmd.email()), QString::fromStdString(cmd.user_name())))


Yup, this was commented out when I built the challenge questions. I'll remove them.

Daenyth · 2017-02-12T15:00:50Z

servatrice/src/smtpclient.cpp

+	message.setSubject(subject);
+	message.setBody(body.replace("%username", nickname).replace("%token", token));
+
+	int id = smtp->send(message);


Is there an error state you can check for after calling send? If one exists we should make sure it's OK before returning true

For starters, this works the exact same way the registration works (so not sure why we want it to work differently), the other thing to mention is all this does is add the email to the que that is later processed at the next timer countdown. I could look to see if the function in the smtp client returns something different or a value to check but I'm not sure if that should really hold the process up. The end result would be the user looks for an email that simply never comes and then contacts the server operator for help. Inconvenience sure, but its not really any different than the current behavior when a user goes to register.

woogerboy21 · 2017-02-12T17:30:16Z

I'll make the changes @Daenyth, check out my responses when you get a chance @Daenyth . Pretty sure when I remove tabs it will squash some of the questions asked in the thread.

…ionality to use smtp settings section.

woogerboy21 · 2017-02-12T20:37:24Z

Alright, changes made (with the exception to the questions I posed back to @Daenyth for things like the dialog). The testing server mentioned above in the thread has been updated to have the new code on it so feel free to test away.

Also @Daenyth , I'll squash the commits prior to merge. No need for a bunch of commits when its in the code base.

ZeldaZach · 2017-02-15T09:05:56Z

I've done my review and I do plan on over-ruling @Daenyth on this case. We will have an audit table added in a future PR (Which is not in the so far future) so there's no need to do it here. Repetitive data is not fully needed, so if @woogerboy21 agrees that this is good to go I'm ready to

woogerboy21 · 2017-02-15T15:27:02Z

The only thing I can see is maybe the questions to this stuff: #2393 (comment)

I don't believe it needs changed but would like confirmation of behavior on other OS's (like mac).

Changes have been made and agreed upon for change in another PR

woogerboy21 added 12 commits January 27, 2017 08:11

Client side request functionality built

94291b4

Added featureset item

56d95b3

Updated missed slotname for button trigger

Fixed client side request code path

c0e006e

Implemented basic server socket interface command handling

Progress save.

b75e638

Created most logic for server side request function.

Progress save

b7eae0b

Progress Save

7766aff

Added missing header

3c35128

Progress save....

224bf6e

Fixed server side reset checks

702e73f

Added challenge question functionality

f526ae0

Implemented email functionality for forgot password

f2d9945

Finished troubleshooting and resolved issues. Functionality working!

5868f11

woogerboy21 added App - Cockatrice Tickets relating to the cockatrice application App - Protocol / API Tickets that will require protocol level support in addition to client and server changes App - Servatrice Tickets relating to the servatrice application labels Jan 28, 2017

Forgot to update the .sql DB creation script. Fixed.

81c45a2

woogerboy21 added 2 commits January 28, 2017 16:41

Fixed currently unused rc container's.

30eba0a

Refactored queries for validating column data

535de6c

ZeldaZach approved these changes Feb 8, 2017

View reviewed changes

Merge branch 'master' into simpler_forgot_password

3576e48

Daenyth self-assigned this Feb 12, 2017

Daenyth previously requested changes Feb 12, 2017

View reviewed changes

woogerboy21 added 6 commits February 12, 2017 13:13

Minor requested changes

90391ab

De-tab

b86faf8

De-tab server side stuff

db28298

More Tab Fixes

c771eba

Unified the email,name of smtp messages from the forgotpassword funct…

f0d3cb4

…ionality to use smtp settings section.

Typo fix.

7e60290

ZeldaZach merged commit 0cfa686 into Cockatrice:master Feb 15, 2017

tooomm mentioned this pull request Mar 4, 2017

login / forgot pw dialog bug (since #2393) #2445

Closed

woogerboy21 deleted the simpler_forgot_password branch July 24, 2017 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simpler forgot password functionality #2393

Simpler forgot password functionality #2393

woogerboy21 commented Jan 28, 2017 •

edited

Loading

woogerboy21 commented Jan 28, 2017

woogerboy21 commented Jan 28, 2017

tooomm commented Jan 29, 2017

woogerboy21 commented Jan 29, 2017 •

edited

Loading

woogerboy21 commented Jan 29, 2017

ZeldaZach left a comment

woogerboy21 commented Feb 9, 2017

ZeldaZach commented Feb 11, 2017

Daenyth left a comment

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017 •

edited

Loading

woogerboy21 Feb 13, 2017

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017 •

edited

Loading

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017 •

edited

Loading

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017

Daenyth Feb 12, 2017

woogerboy21 Feb 12, 2017

woogerboy21 commented Feb 12, 2017

woogerboy21 commented Feb 12, 2017 •

edited

Loading

ZeldaZach commented Feb 15, 2017

woogerboy21 commented Feb 15, 2017

Simpler forgot password functionality #2393

Simpler forgot password functionality #2393

Conversation

woogerboy21 commented Jan 28, 2017 • edited Loading

woogerboy21 commented Jan 28, 2017

woogerboy21 commented Jan 28, 2017

tooomm commented Jan 29, 2017

woogerboy21 commented Jan 29, 2017 • edited Loading

woogerboy21 commented Jan 29, 2017

ZeldaZach left a comment

Choose a reason for hiding this comment

woogerboy21 commented Feb 9, 2017

ZeldaZach commented Feb 11, 2017

Daenyth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woogerboy21 Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woogerboy21 Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woogerboy21 Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woogerboy21 commented Feb 12, 2017

woogerboy21 commented Feb 12, 2017 • edited Loading

ZeldaZach commented Feb 15, 2017

woogerboy21 commented Feb 15, 2017

woogerboy21 commented Jan 28, 2017 •

edited

Loading

woogerboy21 commented Jan 29, 2017 •

edited

Loading

woogerboy21 Feb 12, 2017 •

edited

Loading

woogerboy21 Feb 12, 2017 •

edited

Loading

woogerboy21 Feb 12, 2017 •

edited

Loading

woogerboy21 commented Feb 12, 2017 •

edited

Loading