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 password options to the UI of the proxy and vpn client apps #520

Merged
merged 2 commits into from
Sep 16, 2020
Merged

Add password options to the UI of the proxy and vpn client apps #520

merged 2 commits into from
Sep 16, 2020

Conversation

Senyoret1
Copy link
Contributor

Did you run make format && make check?
The go code was not changed. npm run lint and npm run build were used.

Fixes #519

Changes:

  • Now the user can add a password when configuring the vpn-client and skysocks-client apps.
  • When selecting a history entry created with a password, a modal window is shown for the user to enter the password again, as it is not saved.

How to test this PR:
Using the manager open the app configuration modal window for the vpn-client and skysocks-client apps. You will be able to enter a password manually.

NOTE: the changes in this PR don’t work totally well because of 2 things:

  • When changing the password of the skysocks-client app, the server returns App skysocks-client is not allowed to change password as error. For the PR work well, the backend has to be changed to avoid that error. The password field could be removed when changing the configuration of the skysocks-client app, to avoid the problem, but that does not appear to be a good solution, as it is possible to set a password for the skysocks app, so the situation would be similar to Missing frontend support for setting VPN client passcode arg #519

  • The hypervisor API does not allow to remove the -passcode setting. Because of that, if the user does not enter a password the manager sends an empty string to the backend as the password, so the app configuration will have an empty string as password. If that solution is ok, then there is no problem with this point.

@nkryuchkov
Copy link
Contributor

@Senyoret1

There's no password setting for skysocks-client app, it just redirects credentials to skysocks, therefore it's present in the list of apps that are allowed to change the password.

I've implemented removing args if their passed value is empty: #522

Copy link
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Works great. Just minor requests.

* Makes the app use the data from a history entry.
* @param entry Entry to be used.
*/
useFronHistory(entry: HistoryEntry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean useFromHistory?

"password-dialog": {
"title": "Enter Password",
"password": "Password",
"info": "You are being asked for a password because a a password was set when the selected entry was created, but the it was not saved for security reasons. You can leave the password empty if needed.",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems there's a redundant a in because a a password was set when the selected entry was created

"password-dialog": {
"title": "Enter Password",
"password": "Password",
"info": "You are being asked for a password because a a password was set when the selected entry was created, but the it was not saved for security reasons. You can leave the password empty if needed.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@Senyoret1
Copy link
Contributor Author

@nkryuchkov Thanks for the code review. I removed the password field for the skysocks-client app and made the changes you mentioned, so everything should be ready after merging #522

@jdknives jdknives merged commit 68d6e59 into skycoin:develop Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants