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

Better pin handling #76

Closed
wants to merge 4 commits into from
Closed

Better pin handling #76

wants to merge 4 commits into from

Conversation

My1
Copy link
Contributor

@My1 My1 commented Mar 28, 2020

Ask for PIN directly upon request but only if a PIN exists, also remove PINs not being asked for.

My1 added 3 commits March 28, 2020 19:22
check whether or not PIN exists on the device, if it does, ask for PIN, if not remove any PIN entered using --pin
remove debug print
set-pin won't work if a PIN exists, similarly chnage-PIN will error out if a PIN exists, so alert and guide the user beforehand, maybe someday add a unified command for PIN management.
@My1
Copy link
Contributor Author

My1 commented Mar 29, 2020

amended to have set and change PIN alert and exit early.

Copy link
Contributor

@rgerganov rgerganov left a comment

Choose a reason for hiding this comment

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

Squash all commits into one for the sake of better git history

@@ -409,3 +409,7 @@ def isCorrectVersion(current, target):
self.verify_flash(sig)

return sig

def has_pin (self):
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what extra space? when I have spaceds at the end of the line the PR quality checker says bad.

@@ -409,3 +409,7 @@ def isCorrectVersion(current, target):
self.verify_flash(sig)

return sig

def has_pin (self):
haspin = (self.client.info.options["clientPin"])
Copy link
Contributor

Choose a reason for hiding this comment

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

return self.client.info.options["clientPin"]

if not pin:
dev = solo.client.find(serial, udp=udp)
if dev.has_pin() is False:
#remove PIN if no PIN set
Copy link
Contributor

Choose a reason for hiding this comment

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

silently removing the PIN is incorrect and misleading -- if there is no PIN set and the user specifies a PIN, he should get an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay yeah silently is maybe bad, I just wanted to try and resolve this without the user needing to actially do anything extra, making it easier to use

if not pin:
dev = solo.client.find(serial, udp=udp)
if dev.has_pin() is False:
#remove PIN if no PIN set
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

client = solo.client.find(serial)
if client.has_pin():
print("This Device already has a PIN, use change-pin instead")
sys.exit(1)
"""Set pin of current key"""
Copy link
Contributor

Choose a reason for hiding this comment

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

docstring should be first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that's a special string? I thought that was just a comment

cert = solo.client.find(serial, udp=udp).make_credential(pin=pin)
dev = solo.client.find(serial, udp=udp)
if dev.has_pin() is False:
#remove PIN if no PIN set
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@My1
Copy link
Contributor Author

My1 commented Mar 30, 2020

Squash all commits into one for the sake of better git history

no plan how

@nickray nickray closed this Jul 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