Persist lobby terms agreement checkbox if the terms didn't change
ClosedPublic
Actions

Authored by elexis on Jun 12 2018, 12:33 PM.

Details

Reviewers

vladislavbelov

Commits

rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in…

Summary

Since its introduction in rP15019, the lobby terms never changed.
As they need to be updated now, refs rP21847 / D1568 / #5218 added the possibility to accept new terms without registrating again and prevented a login if the updated terms were not accepted.

Since one physically cannot read the lobby terms of use and service each time one may want to login,
the user choice to agree with the terms shall be persisted. If the terms changed, the agreement is reset.

Test Plan

Notice that the checkbox is persisted between logins. Change a byte in one of the files and see that this specific page needs to be read again and the checkbox ticked again.
See also https://gdpr-info.eu/art-7-gdpr/.

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

elexis created this revision.Jun 12 2018, 12:33 PM

elexis added inline comments.Jun 12 2018, 12:37 PM

binaries/data/mods/public/gui/prelobby/common/terms/terms.js
65 ↗	(On Diff #6754)	This line should be redundant with the other code but I'd rather have this doublecheck, mostly since the controller shall be able to demonstrate that the data subject has consented https://gdpr-info.eu/art-7-gdpr/
source/ps/scripting/JSInterface_Main.cpp
26 ↗	(On Diff #6754)	This diff was written 1.5 years ago already for #4399...

Successful build - Chance fights ever on the side of the prudent.

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

Harbormaster completed remote builds in B6246: Diff 6754.Jun 12 2018, 12:39 PM

Stan added a subscriber: Stan.Jun 12 2018, 2:18 PM

Stan added inline comments.

source/ps/scripting/JSInterface_Main.cpp
26 ↗	(On Diff #6754)	Isn't it better to use something more safe than MD5 ? Nothing in libsodium ?

vladislavbelov requested changes to this revision.Jun 12 2018, 2:28 PM

vladislavbelov added a subscriber: vladislavbelov.

vladislavbelov added inline comments.

binaries/data/config/default.cfg
413 ↗	(On Diff #6754)	Why not empty?
source/ps/scripting/JSInterface_Main.cpp
118 ↗	(On Diff #6754)	`static_cast`. Ehm: `strlen(input.c_str())`? Why not `input.size()`? And then `static_cast<const u8*>(input.data())`.
123 ↗	(On Diff #6754)	It's a duplication of code (and not really good). Use the implementation from `JSInterface_Lobby.cpp`, it's pretty simple and nice. It'd be good to make a common helper.
source/ps/scripting/JSInterface_Main.h
36 ↗	(On Diff #6754)	`UNUSED` is not used in headers as you can see above :)

This revision now requires changes to proceed.Jun 12 2018, 2:28 PM

vladislavbelov added inline comments.Jun 12 2018, 2:30 PM

source/ps/scripting/JSInterface_Main.cpp
26 ↗	(On Diff #6754)	We have `SHA256` in the mentioned `JSInterface_Lobby.cpp`.

vladislavbelov added inline comments.Jun 12 2018, 2:31 PM

source/ps/scripting/JSInterface_Main.cpp
26 ↗	(On Diff #6754)	But `MD5` is pretty enough for this purpose.

elexis added inline comments.Jun 12 2018, 2:36 PM

binaries/data/config/default.cfg
413 ↗	(On Diff #6754)	#3990
source/ps/scripting/JSInterface_Main.cpp
123 ↗	(On Diff #6754)	I'm aware there is some duplication, but I won't change existing code now before the rerelease unless I have to (I had to in D1568). Interfaces shouldn't contain implementation and I don't see any in `JSInterface_Lobby.cpp` (nor a hashing function in `XmppClient.cpp`)
source/ps/scripting/JSInterface_Main.h
36 ↗	(On Diff #6754)	roger

vladislavbelov added inline comments.Jun 12 2018, 2:41 PM

binaries/data/config/default.cfg
413 ↗	(On Diff #6754)	Thanks, forgot about it.
source/ps/scripting/JSInterface_Main.cpp
123 ↗	(On Diff #6754)	So use the right duplication and add a comment to refactor it in future.

Multiple accounts per computer maybe a problem with this config persistance. Perhaps the checkbox should be unticked once the username input field changed?

source/ps/scripting/JSInterface_Main.cpp
26 ↗	(On Diff #6754)	SHA would definitely preferable, because it supposedly is collision resistant and the GDPR controller should be able demonstrate that the license was accepted and not a former one. The likelihood that we will ever have two different license files with the same md5sum is so unlikely that we almost certainly won't have trouble with this aspect if using md5 however. The agreements could certainly be a lot more restrictive, for instance we could send some bytes to the lobby server that the client accepted, thus allowing us to detect people who illegaly connected with custom clients and thus were not able to read the terms. I don't know if we can afford to implement that or if it's a strict requirement by the regulation.
123 ↗	(On Diff #6754)	Ah that. Yeah possible. In fact that `JSI_Lobby::EncryptPassword` function could be used without introducing the md5 function at all. ? (also for the welcome/splash screen). Perhaps it should be renamed, maybe to sha256(text, salt)?

and if we use the currently typed username as the salt of the hash, then we just solved the multiple-users-per-machine problem.

So tried Engine.EncryptPassword, but it's too slow, freezes the main thread for about 1 second on my machine.

So actually we have to use the md5 thing :-/

I've added the username to the hashed content and reloaded the acceptance from the config while typing the username.

It still saves the hash only for one username, so persisting lobby terms acceptance on multi-user account machines is currently not implemented.

source/ps/scripting/JSInterface_Main.cpp
118 ↗	(On Diff #6754)	`length` returns the contents of the std::string, so excludes the null terminating character. So using `data` is misleading no? The use of `strlen` comes from `test_MD5.h`, that was an unneeded import indeed (as we have a string here but a char* in the other place). http://www.cplusplus.com/reference/cstring/strlen/ http://www.cplusplus.com/reference/string/string/data/
123 ↗	(On Diff #6754)	Well, it's 5 lines of code, not that terrible amounts of duplication considering that arguments differ.