Store forum token instead of password #6487

RoiEXLab · 2020-05-17T23:00:57Z

This is what I was able to come up with from #6452
This PR is a WIP, but I wanted to hear your thoughts on the UI decisions.

Basically the user never really gets to see the token and we only store the username for display purposes, so the user can tell their settings have some sort of credentials stored.
We also store a uid to be able to properly revoke the token again.

Whenever a user clicks "save" now in engine preferences, we make a request to the proper forum and create a new token. There's no UI mechanism in NodeBB for this outside of the admin panel. Tokens are revoked if

A User creates a new token aka changes their username in the UI and clicks save
The Setting is cleared.

This has 2 benefits:

It works with 2FA which is the main goal of this PR
We no longer have to store any forum password, just a random token which is huge for security (maybe we want to consider explicitly clearing stored passwords to prevent them from floating around in the registry?)

Open question: How should we implement this on the "on screen"/not-engine-preferences login fields? Same UI like the engine preferences or just a pre-filled token field?

Functional Changes

[] New map or map update
[] New Feature
[x] Feature update or enhancement
[] Feature Removal
[] Code Cleanup or refactor
[] Configuration Change
[] Problem fix:
[] Other:

Testing

I verified basic posting works with this new approach.

Release Note

Feature|Support for 2FA for PbF
Enhancement|PbF now stores login tokens instead of passwords
Fix|Usernames with spaces work for PbF

codeclimate · 2020-05-17T23:02:25Z

game-core/src/main/java/games/strategy/triplea/settings/SelectionComponentFactory.java

+                  context.setValue(tokenSetting, tokenInfo.getToken().toCharArray());
+
+                  context.setValue(usernameSetting, usernameField.getText().toCharArray());
+                  // TODO error reporting


codeclimate · 2020-05-17T23:02:27Z

Code Climate has analyzed commit ab9c619 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Bug Risk	1

View more on Code Climate.

...a/games/strategy/engine/framework/startup/ui/posted/game/pbf/ForumPosterEditorViewModel.java

RoiEXLab · 2020-05-17T23:10:50Z

game-core/src/main/java/games/strategy/triplea/settings/SelectionComponentFactory.java

-          new JPasswordField(credentialToString(passwordSetting::getValue), 20);
+          new JTextFieldBuilder()
+              .columns(20)
+              .text(usernameSetting.getValue().map(String::new).orElse(""))


I will properly clear any char arrays in the final form as well, even though I'm not sure if actually encrypting everything is that important now

DanVanAtta

The concept of storing a token is good; I like how this is coming along.

Off-topic, this makes me wonder if we can do the same for email. Handling any form of email password just really worries me. So far there is not much reason to attack TripleA or the players, if you can get someone email then suddenly there is a real financial reward for doing so.

...a/games/strategy/engine/framework/startup/ui/posted/game/pbf/ForumPosterEditorViewModel.java

game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbTokenGenerator.java

game-core/src/main/java/games/strategy/triplea/settings/SelectionComponentFactory.java

DanVanAtta · 2020-05-18T00:55:38Z

game-core/src/main/java/games/strategy/triplea/settings/SelectionComponentFactory.java

      }

      @Override
      public void reset() {
-        usernameField.setText(credentialToString(usernameSetting::getValue));
-        passwordField.setText(credentialToString(passwordSetting::getValue));
+        usernameField.setText(usernameSetting.getValue().map(String::new).orElse(""));


I don't think username should be a char[], IMO it's just not sensitive enough. Does it make sense changing username to be a String value before, after or in this PR?

Yes and no.
It would make sense changing it to a normal string, but for usernames that are actually stored already it would fill the username field with a cryptic string for the first time, so we'd have to change the name of the setting to something different.

Regardless I think we should offer a button that performs a "factory reset" on the preferences store so users have a way to clear out settings that might no longer be there at all.

IF we change datatype of a client setting, we only need to change the name of the setting. Having something clear out old settings would be nice, but I think certainly nice and not required.

RoiEXLab · 2020-05-18T09:39:26Z

Off-topic, this makes me wonder if we can do the same for email. Handling any form of email password just really worries me.

For certain services that offern an API like google, the answer is probably yes, but in general the answer is probably no, because every email provider handles things differently

stale · 2020-06-01T11:40:33Z

This pull request has been automatically marked as stale because it has not had recent activity. We are eager to see this work completed, please update and re-open as soon as possible.

stale · 2020-06-15T12:45:38Z

This pull request has been automatically marked as stale because it has not had recent activity. We are eager to see this work completed, please update and re-open as soon as possible.

DanVanAtta · 2020-06-16T03:40:53Z

@RoiEXLab any thoughts when you might be able to wrap this up?

RoiEXLab · 2020-06-16T08:38:03Z

Coding can probably be done in a matter of half an hour or something.
My hesitation comes partly from my weird weekly schedule partly because I'm still not sure how to "redesign" the UI elements inside the PbF Menu so they work with the new system.
Pre-filling a password doesn't work anymore, so how do we indicate a token has already been provided? Do we simply disable the forum and add a tooltip? Do we fill in some stars to indicate some password, just to confuse people that they entered a password with a different amount of characters?
Apart from that I'm not that happy with the temporary storage of the currently password, then token.
Resetting the value requires us to "unregister" the token, which might not be an ideal thing to do in a shutdown hook.

If you think you have a good solution for all of those questions I'd be happy to implement them on the weekend or something, but I don't want to spend 2 hours just trying things until I get to a point that somehow works

DanVanAtta · 2020-06-18T05:35:10Z

IMO setting a long 'dummy' password value is reasonable (ballpark 15~20 characters). I think in the case of saved passwords we are already pre-filling a dummy value in the password field to show that it has been set (so that is not without precedent).

Resetting the value requires us to "unregister" the token

I'm not sure I have enough context to really suggest a solution to this. The 'unregister' is with client settings, or with the forum?

Fixes #6737

RoiEXLab

Ok, I think this PR should be in a fine spot now.

A couple of questions remain open though: What about dummy passwords, as mentioned above and how important is clearing the char array token if we end up converting it into a String anyways?
For convenience I used the String constructor and toCharArray() everywhere

RoiEXLab · 2020-06-26T13:57:41Z

...a/games/strategy/engine/framework/startup/ui/posted/game/pbf/ForumPosterEditorViewModel.java

 import org.triplea.java.Postconditions;
 import org.triplea.java.StringUtils;
 import org.triplea.java.ViewModelListener;

 class ForumPosterEditorViewModel {
+  private static final int DUMMY_PASSWORD_LENGTH = 4;


We no longer store passwords, so we have to go for something. Thoughts regarding the recent events? @DanVanAtta

Oh.. the UX should be changed then probably. Perhaps a button to log in to the forum, clicking that asks for username and password, and once done then the user sees a "credentials saved" label somewhere and are given a button to clear their credentials. When entering in their credentials, that would be a good place to have a checkbox for "clear credentials when the application closes".

This way we never need to enter in a dummy password at all, we don't have to worry about a remember password checkbox, and if the credentials are already entered we don't show needless text fields on the UI.

RoiEXLab · 2020-06-26T14:07:51Z

game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbTokenGenerator.java

+
+  private Map<?, ?> queryUserInfo(final CloseableHttpClient client, final String username)
+      throws IOException {
+    final String encodedUsername = URLEncoder.encode(username, StandardCharsets.UTF_8);


Note this fix for #6737 I added

@RoiEXLab we should probably put the bug fix to its own PR:

when this gets squashed, a bug fix being mixed in with a feature update will be somewhat confusing

the bugfix needs to go out with the next release, which could be as soon as tonight or tomorrow.

Would be nice to see a test case for this as well considering it was a bug. Tests are good for preventing future regressions, ensuring bugs are fixed well, and for giving feedback if the code is designed well, which should drive generally better code design (or at least prevent completely ad-hoc and chaotic design).

DanVanAtta

A quick comment or two for you to work on if you get the time @RoiEXLab

I'll need a few more days and iterations to properly review this.

DanVanAtta · 2020-06-29T00:37:38Z

.../main/java/games/strategy/engine/framework/startup/ui/posted/game/pbf/ForumPosterEditor.java

@@ -41,13 +41,15 @@
  private final JTextField usernameField = new JTextField();
  private final JLabel passwordLabel = new JLabel("Forum Password");
  private final JPasswordField passwordField = new JPasswordField();
+  private final JLabel otpLabel = new JLabel("2FA OTP-Code (optional)");


otp is not that well known of an acronym. What is it spelled out?

Can we add a help text or box next to this as well to explain what this is for a user? We do want people to use this after all. If we look at the 1.9 PBF menu, we can see that the game explained pretty much all the options quite well. This meant any user could learn how to use PBF, even if it was kinda clunky.

2FA OTP-code is a bit cryptic to even me. I think we'll need to help users a bit more, and/or explain when/why they'd want to use this. I'm a bit curious myself when we would want to use OTP code and not just use passwords.

OTP is short for "One-Time-Password".

This Field is only required for people that have 2FA enabled in the Forum. Up until recently the "write-api" for NodeBB didn't check if 2FA was enabled and just ignored it. Because @panther2 pointed this out I created an issue which lead to a fix. Currently people with 2FA enabled can't use PbF at all and this extra field should fix this

Also perhaps worth mentioning: The OTP isn't a replacement for the password, it is a necessary addition

Cool, that is good context. Do you have a plan for how to convey that information to users? Specifically, what the field is, when it is needed, and if then not already clear - perhaps where to get their OTP when it is needed.

Not really, but I think this is a problem that isn't limited to TripleA. How can we explain or even advertise 2FA for users?
For most people this just adds another inconvenient step to the login process, and I'd assume most people already using 2FA know what to look out for, they don't need any extra explanation, they just want a field to enter their newly generated code.

If you have any good explanation, please let me know. I'd also like to know how to explain such an "abstract" concept in such a limited screen space 🤔

To clarify: Obviously we could implement a wall of text, but I bet most people aren't going to read it if it's more than a couple of lines

A help button that says this field is used if "2FA is enabled. That this value can be found in forum settings. Doesn't have to be anything to complicated. Leaving it as 'OTP' though is going to be cryptic.

DanVanAtta · 2020-06-29T00:39:00Z

.../main/java/games/strategy/engine/framework/startup/ui/posted/game/pbf/ForumPosterEditor.java

@@ -63,6 +65,10 @@
    super(new GridBagLayout());
    this.viewModel = viewModel;
    viewModel.setView(this);
+    // If password is already stored we already remember
+    if (viewModel.isForumPasswordValid()) {


nit, view should not be doing logic if it has a view model. The view model should be the one that owns the full logic of when 'rememberPassword' is selected.

Would recommend to fix by moving the logic to view model, EG:

rememberPassword.setSelected(viewModel.isRememberPasswordSelected());

To be honest this is more like a hack.
If the user already has a token stored, it makes no sense to have "Remember Password" unchecked, which would cause the credentials to be removed on close by default.
On the other hand if no token is stored it "defaults" to the ClientSetting that stores the last state. That's why the code is the way it is.

I think "Remember Password" shouldn't be a client setting at all. The checkbox should be selected by default if and only if the token is already stored and leave it up to the user to alter this behaviour.

The 'remember password' is a setting as we are currently supporting passwords, and if you select the box then you want it checked, and if you unselect it then it'll be annoying to have to unselect it every time. Those users are probably very worried about their password and if they forget to unselect the value then they'll be unhappy with the software for allowing the error.

It sounds like if 'remember password' does not apply with tokens then it should be hidden.

Either way, any if statements should be removed from the view and moved to the model. The view should not know how to compute the UI state, it should ask the model what its appropriate state should be and render that.

DanVanAtta · 2020-06-29T00:40:09Z

game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbTokenGenerator.java

+import org.snakeyaml.engine.v2.api.LoadSettings;
+
+@AllArgsConstructor
+public class NodeBbTokenGenerator {


Could use a javadoc on this class IMO to explain it's general usage and how it works.

RoiEXLab · 2020-06-29T23:56:58Z

@DanVanAtta I added documentation and updated the PR description. Let me know what you think so far. I only had a brief look at #6925 so far but I think most of the few remaining open questions can be dicussed there

DanVanAtta · 2020-06-30T00:39:13Z

@RoiEXLab I want the URI decode bug fix for 2.1, sooner. To get there we should create a test for it, but to test it we should be extracting the HTTP client portion so that we can get at the logic that needs to be tested. I am thinking we probably should first convert the HTTP client to feign so it's uniform and testable, then do this work.

On another front, I also think the completable future portions can be removed as we are simply calling get on the future and blocking anyways. I think then the error handling is perhaps not the best as we catch IllegalStateException (a smell) and use the message from the exception as an error message to user. Since you can trigger the exception by simply using the system in a very normal way, it's control-flow-by-exception handling. A user should never be able to trigger an exception via normal usage and standard clicks (pulling the network cable is questionable and probably an okay exception, but simply not completing registration or typo in username are not exceptions, those are normal, they are "user-error" cases, not exceptions).

So, if we convert the return value from the http client to be a result and result code, we can remove the completable futures and an exception would be an IOException only (which we would perhaps want reported and/or retried). I digress though, one main point is if we don't convert to Feign now and test now, I don't think we'll do either one ever.

DanVanAtta · 2020-07-10T13:52:30Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 3
           

Complexity increasing per file
==============================
- game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbTokenGenerator.java  7
- game-core/src/main/java/games/strategy/engine/framework/startup/ui/posted/game/pbf/ForumPosterEditor.java  1

See the complete overview on Codacy

DanVanAtta · 2020-07-10T13:52:34Z

game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbTokenGenerator.java

+      final int userId = getUserId(client, username);
+      return new TokenInfo(getToken(client, userId, password, otp), userId);
+    } catch (final IOException e) {
+      throw new RuntimeException("Failed to retrieve login token", e);


Codacy found an issue: Avoid throwing raw exception types.

DanVanAtta · 2020-07-10T13:52:35Z

game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbTokenGenerator.java

+    try (CloseableHttpClient client = HttpClients.custom().disableCookieManagement().build()) {
+      deleteToken(client, userId, token);
+    } catch (final IOException e) {
+      throw new RuntimeException("Failed to revoke login token", e);


Codacy found an issue: Avoid throwing raw exception types.

DanVanAtta · 2020-07-10T13:52:36Z

game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbForumPoster.java

-        Arrays.withSensitiveArrayAndReturn(() -> forumPostingParameters.username, String::new);
-    this.password =
-        Arrays.withSensitiveArrayAndReturn(() -> forumPostingParameters.password, String::new);
+    this.token = new String(forumPostingParameters.token);


Codacy found an issue: Avoid instantiating String objects; this is usually unnecessary.

RoiEXLab · 2020-07-11T18:52:48Z

Going to merge this now. Explanation can be found in #6925

Ultimate goal is to migrate this to feign to allow for less logic and nicer tests

RoiEXLab requested a review from DanVanAtta May 17, 2020 23:00

RoiEXLab mentioned this pull request May 17, 2020

Implementing 2FA for NodeBB Forum posters #6452

Closed

codeclimate bot reviewed May 17, 2020

View reviewed changes

RoiEXLab commented May 17, 2020

View reviewed changes

...a/games/strategy/engine/framework/startup/ui/posted/game/pbf/ForumPosterEditorViewModel.java Outdated Show resolved Hide resolved

RoiEXLab commented May 17, 2020

View reviewed changes

RoiEXLab added the PR: Work In Progress label May 17, 2020

DanVanAtta reviewed May 18, 2020

View reviewed changes

stale bot added the Stale label Jun 1, 2020

RoiEXLab removed the Stale label Jun 1, 2020

stale bot added Stale and removed Stale labels Jun 15, 2020

DanVanAtta removed the PR: Work In Progress label Jun 22, 2020

RoiEXLab and others added 8 commits June 26, 2020 14:28

Store Token instead of password

eb093cc

Auto-Formatting

95687b7

Properly reset value

5b4d5b1

Correctly encode username

203ff3b

Fixes #6737

Fix band-aided code

2348728

Request Token when actually requiring it

2091b68

Add 2FA support

b109b6a

Pre-Fill remember password checkbox if already remembered

2135f08

RoiEXLab commented Jun 26, 2020

View reviewed changes

RoiEXLab marked this pull request as ready for review June 26, 2020 14:06

RoiEXLab commented Jun 26, 2020

View reviewed changes

RoiEXLab added 4 commits June 26, 2020 16:12

Fix test compilation errors

0648341

Fix test cases

a904cc6

Make tests headless friendly

61057e7

Make var final

73faf15

RoiEXLab assigned DanVanAtta Jun 26, 2020

DanVanAtta reviewed Jun 29, 2020

View reviewed changes

RoiEXLab added 2 commits June 30, 2020 01:49

Use isBlank() instead of isEmpty()

5890bde

Add javadoc

2619e0b

DanVanAtta mentioned this pull request Jun 30, 2020

Fix invalid URI error for forum username that can contain a space #6937

Merged

DanVanAtta removed their assignment Jul 1, 2020

RoiEXLab mentioned this pull request Jul 10, 2020

Begin extracting nodeBB forum http interface #6925

Closed

Merge branch 'master' into store-forum-token

3e21fda

DanVanAtta reviewed Jul 10, 2020

View reviewed changes

RoiEXLab merged commit 81ca160 into triplea-game:master Jul 11, 2020

RoiEXLab deleted the store-forum-token branch July 11, 2020 18:53

Store forum token instead of password #6487

Store forum token instead of password #6487

Conversation

RoiEXLab commented May 17, 2020 • edited Loading

Functional Changes

Testing

Release Note

codeclimate bot May 17, 2020

Choose a reason for hiding this comment

codeclimate bot commented May 17, 2020

Choose a reason for hiding this comment

DanVanAtta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoiEXLab May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoiEXLab commented May 18, 2020

stale bot commented Jun 1, 2020

stale bot commented Jun 15, 2020

DanVanAtta commented Jun 16, 2020

RoiEXLab commented Jun 16, 2020

DanVanAtta commented Jun 18, 2020

RoiEXLab left a 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

DanVanAtta left a 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

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

RoiEXLab commented Jun 29, 2020

DanVanAtta commented Jun 30, 2020

DanVanAtta commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoiEXLab commented Jul 11, 2020

RoiEXLab commented May 17, 2020 •

edited

Loading

RoiEXLab May 18, 2020 •

edited

Loading