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

Implement Optional Private Keys #161

Merged
merged 15 commits into from
Mar 13, 2022
Merged

Conversation

ferrine
Copy link
Contributor

@ferrine ferrine commented Feb 22, 2022

Address #160

  • creation form update
image
  • validate public key
image
  • validate preshared key
image
  • check QR code creation, disable QR if no private key
image
  • check public key duplication
image
  • make the additional fields hidden by default
image

@ferrine
Copy link
Contributor Author

ferrine commented Feb 22, 2022

Step 1: Implement Public / Preshared keys manual input and validation
image
If can't be read with parseKey
image
image

@ferrine ferrine changed the title WIP Implement Optional Private Keys Implement Optional Private Keys Feb 22, 2022
@ferrine
Copy link
Contributor Author

ferrine commented Feb 22, 2022

Ready for review

Copy link

@SomeoneSerge SomeoneSerge left a comment

Choose a reason for hiding this comment

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

Private keys seem to be a part of the persisted state, but I cannot see how their presence reflects in GUI, or how are they ever accessed for anything after the user has been set up.

  • Is there still any need for storing the private key at all?
  • When the private key is persisted, consider making this explicit for the user: maybe a red "Forget private key" button

templates/base.html Outdated Show resolved Hide resolved
@ferrine
Copy link
Contributor Author

ferrine commented Feb 22, 2022

Private keys seem to be a part of the persisted state, but I cannot see how their presence reflects in GUI, or how are they ever accessed for anything after the user has been set up.

  • Is there still any need for storing the private key at all?
  • When the private key is persisted, consider making this explicit for the user: maybe a red "Forget private key" button
  • Private key is used to include in email config
  • Not storing private key at all means we can't send client config (if he doesn't share public key first)
  • Like the idea to add a button, but not as a part of this PR

@SomeoneSerge
Copy link

Like the idea to add a button, but not as a part of this PR

With this PR, the private key becomes Maybe String rather than String. This rather should reflect in the GUI

Private key is used to include in email config

Oh

@ferrine ferrine changed the title Implement Optional Private Keys WIP Implement Optional Private Keys Feb 22, 2022
@ferrine ferrine changed the title WIP Implement Optional Private Keys Implement Optional Private Keys Feb 23, 2022
@ferrine
Copy link
Contributor Author

ferrine commented Feb 23, 2022

@ngoduykhanh can you please have a look?

@ngoduykhanh
Copy link
Owner

@ferrine thank you for your contribution. The code changes look good to me but I have a few concerns/comments:

  • What is the purpose of this feature? Is it for importing the existing WG config? If so, how do we manage the private key? We need it to generate a proper QR code/config file.
  • I see you use ParseKey to validate the public keys and preshared keys. How about validating the duplication? We should not allow importing a public key that already exists.
  • [UI] Since the Public Key and Preshared Key are optional, I think we should adjust the form to hide them by default (using Collapse for example) because it is a bit long now.

@ferrine
Copy link
Contributor Author

ferrine commented Mar 3, 2022

What is the purpose of this feature? Is it for importing the existing WG config?

The user does not want to reveal his private key

If so, how do we manage the private key?

User manages his private key

We need it to generate a proper QR code/config file.

No QR can be generated

How about validating the duplication?

Did not think about this. Seems to be important but treated as a corner case.

[UI] Since the Public Key and Preshared Key are optional, I think we should adjust the form to hide them by default (using Collapse for example) because it is a bit long now.

Yes, I can try to make that more pretty

TODOs:

  • check QR code creation, raise an error if private key is missing
  • check public key duplication
  • make the additional fields hidden by default

@ferrine
Copy link
Contributor Author

ferrine commented Mar 13, 2022

@ngoduykhanh
The only feature I decided not to implement is

check QR code creation, raise an error if the private key is missing

It still generates the QR code but the config is incomplete (it was supposed to be so). Incomplete config is an expected behavior so I treat this as a feature.

UPD: I checked to import the no private key config. It raises an error in the wireguard app. So I'll remove QR code in this case

@ferrine
Copy link
Contributor Author

ferrine commented Mar 13, 2022

This is a simple way to tell there is no QR
image

@ferrine
Copy link
Contributor Author

ferrine commented Mar 13, 2022

an updated version disables Scan button
image

Copy link
Owner

@ngoduykhanh ngoduykhanh left a comment

Choose a reason for hiding this comment

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

Thanks @ferrine for addressing the issues. I have one more comment below. Other changes look good.

templates/clients.html Outdated Show resolved Hide resolved
@ferrine
Copy link
Contributor Author

ferrine commented Mar 13, 2022

Done

@ngoduykhanh ngoduykhanh merged commit 037a6c5 into ngoduykhanh:master Mar 13, 2022
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.

None yet

3 participants