Page MenuHomeWildfire Games

Add remember password checkbox
ClosedPublic

Authored by vladislavbelov on Aug 21 2017, 8:33 PM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP21311: Adds remember password checkbox
Trac Tickets
#4708
Summary

Some users might not want to save the password at all, so please add a tick box (maybe off by default, so an opt-out) "Remember my password".

This is also somewhat security-critical. E.g. taking the following example as a thread model: On a shared, public computer (internet cafe) the admin is so nice to install 0ad (don't ask me whether this is realistic). There, obviously I do not want the next user of the system to login as me. This is only an example! There may be other reasons and situations (multiuser setup, no disk encryption, paranoia, whatever…) why users do not want to have their passwords saved.

Also another UX aspect: Currently I do not even know that my password is saved. So probably, I think I have to use an easy one or have to save it somewhere else, so that I do not lose access. However, when I see that checkbox, I see "Ah, right, it is saving my password. How nice.".

Test Plan
  1. Run the game
  2. Turn on the checkbox
  3. Check that the password was saved
  4. Turn off the checkbox
  5. Check that the password wasn't saved

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.

Added storing.

elexis edited edge metadata.Aug 21 2017, 8:45 PM

Rest looks good.

binaries/data/mods/public/gui/lobby/prelobby.js
195 ↗(On Diff #3266)

The implementation regulates how many reports for forgotton passwords I get, so I'd prefer a confirmation message box before deleting user passwords while not having a forgotton-pw function. (In fact I'd prefer three messsage boxes before deletion, maybe a 5 minute consideration timer xD)

Agree that a "delete-password" button isn't better, because people who dont want the PW saved, don't have their use case addressed.

Added confirmation box.

vladislavbelov added inline comments.Aug 21 2017, 9:18 PM
binaries/data/mods/public/gui/lobby/prelobby.js
283 ↗(On Diff #3269)

I'm not sure about function names, I'm open to suggestions.

Vulcan added a subscriber: Vulcan.Aug 21 2017, 9:38 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1897/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1898/ for more details.

leper rescinded a token.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1899/ for more details.

vladislavbelov added a reviewer: Restricted Owners Package.Oct 22 2017, 6:39 PM
vladislavbelov added a subscriber: Restricted Owners Package.
elexis added inline comments.Oct 22 2017, 7:36 PM
binaries/data/mods/public/gui/lobby/prelobby.js
193 ↗(On Diff #3269)

(Some prefer to have the most common case first, others prefer to avoid the double negation by checking for positives in if-else statements)

283 ↗(On Diff #3269)

Name functions according to what they do. So maybe toggleRemeberPassword.

binaries/data/mods/public/gui/lobby/prelobby.xml
55 ↗(On Diff #3269)

Either remove the unused name property, or probably better do not pass this as an argument, since the function will never be called for a different checkbox and instead use let checkbox = Engine.GetGUIObjectByName("rememberPassword")`

57 ↗(On Diff #3269)

XML is a markup language and in our case deals with formatting the GUI layout. XML by definition is not supposed to contain code, while JS files should. There have been GUI files that accumulated a lot of JS code, it should be avoided as much as possible. So move it to init IMO.

64 ↗(On Diff #3269)

Not sure if Password woldn't suit better.

Thank you for the overview!

binaries/data/mods/public/gui/lobby/prelobby.xml
64 ↗(On Diff #3269)

I've met both cases on different sites, so I'm not sure too.

Adds @elexis suggestions.

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsScene]’:
FCollada/FCDocument/FCDLibrary.cpp:163:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
In file included from FCollada/FUtils/FUSemaphore.cpp:10:0:
FCollada/FUtils/FUSemaphore.h:36:2: warning: #warning "FUSemaphore: Semaphore not implemented for non Windows" [-Wcpp]
 #warning "FUSemaphore: Semaphore not implemented for non Windows"
  ^
FCollada/FUtils/FUStringConversion.cpp: In function ‘void TrickLinkerFUStringConversion()’:
FCollada/FUtils/FUStringConversion.cpp:281:8: warning: variable ‘f’ set but not used [-Wunused-but-set-variable]
  float f = FUStringConversion::ToFloat(&c);
        ^
FCollada/FUtils/FUStringConversion.cpp:283:7: warning: variable ‘b’ set but not used [-Wunused-but-set-variable]
  bool b = FUStringConversion::ToBoolean(c);
       ^
FCollada/FUtils/FUStringConversion.cpp:285:8: warning: variable ‘i32’ set but not used [-Wunused-but-set-variable]
  int32 i32 = FUStringConversion::ToInt32(&c);
        ^
FCollada/FUtils/FUStringConversion.cpp:287:9: warning: variable ‘u32’ set but not used [-Wunused-but-set-variable]
  uint32 u32 = FUStringConversion::ToUInt32(&c);
         ^
In file included from FCollada/FUtils/FUThread.cpp:10:0:
FCollada/FUtils/FUThread.h:30:2: warning: #warning "Threads not yet implemented for non Windows." [-Wcpp]
 #warning "Threads not yet implemented for non Windows."
  ^
FCollada/FCDocument/FCDAnimationCurve.cpp: In member function ‘float FCDAnimationCurve::Evaluate(float) const’:
FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::x’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   FMVector2 inTangent;
             ^
FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::y’ may be used uninitialized in this function [-Wmaybe-uninitialized]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FC

http://jenkins-master:8080/job/phabricator/2153/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/prelobby/prelobby.js
| 159| »   while·((message·=·Engine.LobbyGuiPollNewMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/functions_global_object.js"/>

http://jenkins-master:8080/job/phabricator_lint/621/ for more details.

Build is green

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2154/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/prelobby/prelobby.js
| 159| »   while·((message·=·Engine.LobbyGuiPollNewMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/functions_global_object.js"/>

http://jenkins-master:8080/job/phabricator_lint/622/ for more details.

Imarok added a subscriber: Imarok.Oct 25 2017, 10:55 AM
Imarok added inline comments.
binaries/data/config/default.cfg
384 ↗(On Diff #3952)

Shouldn't that be true by default?

binaries/data/config/default.cfg
384 ↗(On Diff #3952)

I agree, that it's good for usual players. But pedantic users can say: "Aha! You want to steal my password!". If it's ok, then no problem - I can change it.

elexis added inline comments.Oct 25 2017, 2:40 PM
binaries/data/config/default.cfg
384 ↗(On Diff #3952)

We had one of these players that complained about the password being saved.
Consider that about everyone types a password and then forgets it immediately after the registration and we don't have a way to restore a forgotton password (moderators can only delete accounts which allows recreation of an account, but that still requires the trust that the one claiming to be the account owner actually is the account owner).

So if you really want it to be false, then there should be a dialog upon registration that asks after the registration whether the user wants to store the encrypted password in the config IMO

binaries/data/mods/public/gui/lobby/prelobby.xml
64 ↗(On Diff #3269)

rP20000 changed a lot to Title Case.

vladislavbelov added inline comments.Oct 25 2017, 3:20 PM
binaries/data/config/default.cfg
384 ↗(On Diff #3952)

Ok, I'll make it true. Until we don't have this dialog.

binaries/data/mods/public/gui/lobby/prelobby.xml
64 ↗(On Diff #3269)

Good, then I'll change it.

Adds @elexis 's suggestions.

vladislavbelov marked 2 inline comments as done.Oct 25 2017, 11:15 PM

Build is green

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2167/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/prelobby/prelobby.js
| 159| »   while·((message·=·Engine.LobbyGuiPollNewMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/functions_global_object.js"/>

http://jenkins-master:8080/job/phabricator_lint/632/ for more details.

Tested, works so far, code looks correct.

We have a lobby tab in the options dialog, so it's cheap to add this as a setting there.

binaries/data/config/default.cfg
384 ↗(On Diff #3952)

"Whether to store the encrypted password in the user config"?

binaries/data/mods/public/gui/prelobby/prelobby.js
209 ↗(On Diff #3979)

(One might also think about keeping the password still in the config after login, just not writing it to the config again. But then we would have to ensure that whenever saving a new config, i.e. nope.)

300 ↗(On Diff #3979)

after connecting
your password -> the password

elexis accepted this revision.Feb 21 2018, 10:28 PM

Tested works, all requests addressed, thanks for the patch!

binaries/data/mods/public/gui/prelobby/prelobby.js
312 ↗(On Diff #5864)

We have a new function for this: saveSettingAndWriteToUserConfig(setting, value)

313 ↗(On Diff #5864)

newline

This revision is now accepted and ready to land.Feb 21 2018, 10:28 PM
This revision was automatically updated to reflect the committed changes.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/61/display/redirect