Backup and restore of Passwords#436
Conversation
mmerklinger
requested review from
a team,
daringer,
james-knippes,
mmerklinger,
robin-nitrokey and
sosthene-nitrokey
and removed request for
a team
Backup and restore. Edit to sign
Fix pin perms during backup
|
The Backup and Restore buttons look like this. The SVG icons are under MIT License from VMWare in SVG repo. Would that be ok? 
|
|
Moved backup and restore Logic to the SDK, with the logic in PR 126. Run against that branch to test |
robin-nitrokey
left a comment
robin-nitrokey
left a comment
There was a problem hiding this comment.
Looks good, thank you! I’ve added some minor comments, questions and suggestions.
| action_layout.addWidget(QLabel("Action:")) | ||
| action_layout.addWidget(self.action_edit) | ||
|
|
||
| self.cleartext_checkbox = QCheckBox("Cleartext") |
There was a problem hiding this comment.
I think it would be useful to add tooltips to these fields that describe:
- Cleartext: that this option disables encryption of the generated backup and is thus discouraged
- Passphrase: that the passphrase is automatically generated when creating a backup
- Failed and skipped: more details on when a credential ends up in this list
There was a problem hiding this comment.
self.failed_name = "Not passwords" if name == BackupRestoreAction.BACKUP else "Already exists"
Failed is not shown as failed. During backup, failed means it is not a password credential. During restore failed means another credential with same label already exists on the key
There was a problem hiding this comment.
Cleartext and passphrase tooltips added
There was a problem hiding this comment.
Thanks. For us it is clear what Not passwords means but I think without context it could be confusing. That’s why I’d also add a tooltip here, something like: Credentials without a password (for example OTP only) are skipped during the backup as they cannot be extracted from the device.
| self.status_edit = QLineEdit() | ||
| self.status_edit.setReadOnly(True) |
There was a problem hiding this comment.
What do you think about using QStatusBar for this?
| </property> | ||
| <property name="icon"> | ||
| <iconset> | ||
| <normaloff>icons/backup.svg</normaloff>icons/backup.svg</iconset> |
There was a problem hiding this comment.
I’m a bit confused by the icons. I think originally the arrow pointing to the server is the backup icon and the arrow pointing from the server is the restore icon. You reversed that, right?
There was a problem hiding this comment.
Yes. Because when I was making this UI, I was originally naming it 'Export' and 'Import', and my logic was 'export' means 'taking out of the box' and 'import' means 'putting it into the box'. I'll fix it
There was a problem hiding this comment.
Or should I rename the buttons to Export and Import? Because in Pynitrokey we are naming it export and import
There was a problem hiding this comment.
Right, it probably makes sense to use the same terms both in pynitrokey and nitrokey-app2. Do you have a preference?
There was a problem hiding this comment.
I would prefer calling it export and import because we offer interoperability with other password managers too which support CXF
2 participants
Backup and Restore of passwords functionality added.
Known issue: Sometimes the 'Press Nitrokey prompt is not appearing'