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

Add: Warn players that company passwords are not truly secure #7351

Merged
merged 4 commits into from Apr 24, 2019

Conversation

@nielsmh
Copy link
Contributor

nielsmh commented Mar 9, 2019

Add a warning text to password entry windows, to remind players that the protocol is not truly secure.

image

Currently has an issue with sizing/spacing in the more generic text entry window, without the "Default password" button:
image

@nielsmh nielsmh force-pushed the nielsmh:password-warning branch 2 times, most recently from e3d4271 to 072342c Mar 11, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Mar 11, 2019

Fixed the window sizing issue of the second case. It's not perfect (the string needs to have manual line breaks and it might not behave well with large text sizes) but the window sizing system is not good to work with.

image

Also, the warning shows when setting the password for a server, which is silly. ("You, the server owner, may be able to see what you enter here!") Is it worth bothering to not show it in this case?

@nielsmh nielsmh marked this pull request as ready for review Mar 11, 2019
src/misc_gui.cpp Outdated Show resolved Hide resolved
@michicc

This comment has been minimized.

Copy link
Member

michicc commented Mar 24, 2019

Is it worth bothering to not show it in this case?

Unless it is super complicated I'd say yes. Security warnings that are made too often just lead to people ignoring them every time.

@nielsmh nielsmh force-pushed the nielsmh:password-warning branch from 072342c to 6e0e5de Mar 31, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Mar 31, 2019

No warning:
image

Warning, automatically line broken:
image
image
image

@LordAro LordAro added the needs review label Apr 9, 2019
assert(this->nested_root->smallest_x > 0);
this->warning_size.width = this->nested_root->current_x - (WD_FRAMETEXT_LEFT + WD_FRAMETEXT_RIGHT + WD_FRAMERECT_LEFT + WD_FRAMERECT_RIGHT);
this->warning_size.height = GetStringHeight(STR_WARNING_PASSWORD_SECURITY, this->warning_size.width);
this->warning_size.height += WD_FRAMETEXT_TOP + WD_FRAMETEXT_BOTTOM + WD_FRAMERECT_TOP + WD_FRAMERECT_BOTTOM;

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Apr 13, 2019

Member

Is it useful if we put this "algorithm" in a function? You now use it at two places, but if I understand it correctly, it can also be useful for other places where we want this kind of behaviour? (honest question btw, no is a valid answer :D)

This comment has been minimized.

Copy link
@nielsmh

nielsmh Apr 13, 2019

Author Contributor

I think the best would be to add a flag to label controls or similar, that they have height dependent on width (and width is not measured for the widget), which then causes them to wrap and size after the main size calculations are done.

@nielsmh nielsmh merged commit dd35a43 into OpenTTD:master Apr 24, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190331.13 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@nielsmh nielsmh deleted the nielsmh:password-warning branch Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.