Re: [PATCH proxmox-backup v2 19/27] ui: define and expose encryption key management menu item and windows

[Christian Ebner <c.ebner@proxmox.com>]

On 4/11/26 10:50 AM, Thomas Lamprecht wrote:
> Am 10.04.26 um 18:54 schrieb Christian Ebner:
>> diff --git a/www/config/EncryptionKeysView.js b/www/config/EncryptionKeysView.js
>> @@ -0,0 +1,324 @@
>>
>> +        archiveEncryptionKey: function () {
>> +            ...
>> +            if (selection[0].data.type === 'tape') {
>> +                Ext.Msg.alert(gettext('Error'), gettext('cannot archive tape key'));
> 
> Missing `return;` here - the code falls through to the API2Request below even
> after showing the error alert for tape keys.  The enableFn on the button should
> prevent reaching this in practice, but it is still wrong.

fixed for the next version of the patches, thanks.

> 
>> +            }
> 
>> +        reload: async function () {
>> +            let [syncKeys, tapeKeys] = await Promise.all([syncKeysFuture, tapeKeysFuture]);
> 
> Might profit from wrapping this in a try/catch - if the user lacks permissions
> for one of the two endpoints the entire panel dies with an unhandled promise
> rejection instead of showing partial results.

True, will wrap these as suggested.

> 
> nit: Type column with width 50 px might be rather narrow for icon + label, but
> did not checked out the result yet, so YMMV.

will double check.
  > nit: the synthetic tape key IDs are not super nice/telling on their 
own. The
> tape keys have a hint field that would be more meaningful to display (in
> addition?).

Currently the hint field is already being displayed as column in the 
grid, so this should already cover your idea?

Just to give a bit more context here:

I refrained from using the hint field to be part of the generate id, 
since this is not as restrictive, allowing e.g. white-spaces, not 
allowed otherwise. Also, the hint is not unique. The fingerprint should 
in theory be unique, however it is to long as valid id field value, even 
if encoded differently (e.g. some form of baseXX encoding). Also, that 
makes it even worse for handling.

Since the likelihood of creating the same key at the same second should 
be almost 0, a combination of time and fingerprint seemed a good fit. 
Currently the id for tape keys are only used for display and nowhere 
enforced, so a migration to a different derivation method should still 
be possible also in the future if config files should be merged.

> 
>> diff --git a/www/window/EncryptionKeysEdit.js b/www/window/EncryptionKeysEdit.js
>> @@ -0,0 +1,383 @@
>>
>> +                            if (key.data === undefined) {
>> +                                return 'Does not seems like a valid Proxmox Backup key!';
> 
> nit: missing gettect for both validator strings, also: s/seems/seem/

acked, will adapt.

>> +        {
>> +            xtype: 'displayfield',
>> +            name: 'crypt-key-fp',
>> +            fieldLabel: gettext('Key Source'),
>> +            padding: '2 0',
>> +        },
> 
> Is this set anywhere?  I could not find a matching field name in the API
> response nor this getting manually set. In fact, `rg -i 'crypt.?key.?fp'` shows
> only this lone instance here.

No, this should only be a simple text label for the radio-fields below, 
will adapt.