Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

usability improvements to security token Dialog #7112 #7134

Merged
merged 1 commit into from
Dec 11, 2017
Merged

usability improvements to security token Dialog #7112 #7134

merged 1 commit into from
Dec 11, 2017

Conversation

bnvk
Copy link
Contributor

@bnvk bnvk commented Nov 24, 2017

Old Dialog

screenshot-2017-11-21 parity

New Dialog

screenshot-2017-11-24 parity

The changes I've made include

  • Simpler, more human friendly text
  • Cleaner handling of text, inputs, instructions, etc...
  • Changed icon to better reflect context

@parity-cla-bot
Copy link

It looks like @bnvk hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 24, 2017
@tomusdrw
Copy link
Collaborator

Could we add an information that if you are connecting to a remote host the time needs to be synchronized? I remember that many issues where actually caused by non-synchronized time.

I would add this additional info when user provides the token, but we still can't connect.

@5chdn 5chdn added this to the 1.9 milestone Nov 24, 2017
@5chdn 5chdn added the B0-patch label Nov 24, 2017
@jacogr
Copy link
Contributor

jacogr commented Nov 24, 2017

@tomusdrw We have the "big scary red box" on top of the screen screen now when the time is out - cannot be closed. Do we still need it one here?

@jacogr
Copy link
Contributor

jacogr commented Nov 24, 2017

@5chdn Not back-portable without re-doing, structure is completely different between old and new. Should stick to bugs in 1.8.

@tomusdrw
Copy link
Collaborator

It's different though, this is displayed when node time is off (which means that it won't synchronize with the network).
The sync i'm talking about is between the computer running the browser and the machine running a node (in case you are connecting to a remote host) - if the time is off between them you won't even be able to connect (the token will be constantly rejected even if valid).

@bnvk
Copy link
Contributor Author

bnvk commented Nov 24, 2017

Could we add an information that if you are connecting to a remote host the time needs to be synchronized? I remember that many issues where actually caused by non-synchronized time.

It is my understanding that this remote host scenario @tomusdrw describes is a minority of use cases, and the same machine / localhost configuration is the vast majority of users. The previous message about this was EXTREMELY confusing to an end user who would be the later (majority) type of install.

I suggest use some lower level config value (or host detection) that gets exposed to the interface and ONLY shows such a time clock message for remote host instances.

@tomusdrw
Copy link
Collaborator

@bnvk Completely agree that the dialog should be improved, not suggesting to put more text on the current dialog at all.
I also agree that remote wallet configuration is not main use case for this (although regular users should never see this dialog anyway, cause the tray applications are always supplying the token).
Unfortunatelly we cannot easily detect the remote wallet configuration - people may use ssh tunneling and in such cases they are still connecting to localhost.

What I suggest is that when user enters the token, but we still cannot connect instead of just displaying a misleading "Invalid token" error message we should also give them a hint that the token might be valid and if they are running a remote wallet scenario they should also check time synchronization.

@bnvk
Copy link
Contributor Author

bnvk commented Nov 24, 2017

people may use ssh tunneling and in such cases they are still connecting to localhost.

That's a pretty edge case user, no? IMHO, having info that pertains to such a small amount of users is best left to documentation or a wiki.

when user enters the token, but we still cannot connect instead

Ok, but as @jacogr points out "We have the "big scary red box" on top of the screen screen now " is this the same case? Do you have any read on how to best determine this in the codebase?

@tomusdrw
Copy link
Collaborator

tomusdrw commented Nov 27, 2017

That's a pretty edge case user, no?

That's the recommended way of doing remote wallet, I don't know exactly how many people does that though, just judging from the amount of requests we got for this.

is this the same case?

It's a different case, when the time of the node is not synchronized.

As mentioned earlier, I'm not being petty about the dialog, I'm really happy that you changed it.
Just asking to additionally re-phrase the error message here:
https://github.com/paritytech/parity/blob/d105bc272eb8d91b67b01b0fba8e8244b08d0f12/js/src/Connection/connection.js#L116

to something like "The token is invalid or the time between browser and remote node is not synchronized" to cover that use case.

@bnvk
Copy link
Contributor Author

bnvk commented Nov 29, 2017

IIUC, the line you highlighted defaultMessage='invalid signer token' is an error that gets kicked back when clocks are out of sync. Making that show a hidden message would make sense. Does that sound ok to you @tomusdrw

@tomusdrw
Copy link
Collaborator

Yeah, the message is rendered when you can't connect to the node after typing the token and it may indicate two possibilities:

  1. The token you entered is invalid
  2. The clocks are not in sync between client/host machine

@jacogr jacogr removed the B0-patch label Nov 30, 2017
@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 7, 2017
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, still would like to see time sync info incuded in error message, but don't want to postpone this PR any more.

@jacogr jacogr merged commit b25f93d into openethereum:master Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants