[Freeipa-devel] [PATCH] 076 UI support for ssh keys

[Petr Vobornik]

On 02/11/2012 12:36 AM, Endi Sukma Dewata wrote:
> On 2/6/2012 8:15 AM, Petr Vobornik wrote:
>> ipasshpubkey attribute was added to user and host details pages.
>>
>> New widget for ssh public keys was created.
>>
>> Static preview:
>> http://pvoborni.fedorapeople.org/ssh/#identity=user&navigation=identity&user-facet=default&user-pkey=aortiz
>>
>>
>> https://fedorahosted.org/freeipa/ticket/2340
>
> Everything works, so it's ACKed. I have some comments:

Pushed to master, ipa-2-2.

>
> 1. When you click Add to add a new SSH public key the UI creates a new
> space for the key. The space is still empty, but it shows an undo
> button, implying there's a change that can be saved. However, clicking
> Update will generate an error "no modifications to be performed". Some
> possible solutions:
>
> (a) Show the "Set SSH key" dialog when you click Add. Once you entered
> the key, it creates the new space that already contains the new key.
>
> (b) When you click Update, remove all empty new space and do not
> generate the modify operation unless there are other changes.

This should be addressed in save and test_dirty_method (omit empty keys 
in save). 1a is a nice idea but user can still set an empty key - it 
doesn't solve the problem.
>
> 2. If we decided to do #1a, the dialog title would be "Add SSH Public
> Key" and the button would be Add. For edit the title would be "Edit SSH
> Public Key" and the button would be Update.
>
> 3. Instead of showing "New: key set" or "New: key not set" we probably
> can show the first few characters of the key itself. This way if you're
> entering multiple keys you know which ones are already added.

Like it.
>
> 4. It's also possible to use a table widget, which will work like
> solution #1a except that all changes are saved immediately so we don't
> need to show the partial key like in #3.

I don't think table fits in the facet. Table would be better candidate 
for attribute with more than one displayed column.
>
> 5. Instead of using a "Show/Set key" link, we probably can link the
> fingerprint (for existing keys) or the partial key like in #3 (for new
> keys). If we did the table like in #4, it can be an Edit link.

Might work.
>
> 6. When modifying a key, changing the fingerprint to "Modified" is
> probably not necessary because the undo button will imply that the key
> has been modified and we can't see the original fingerprint anymore.

Maybe. I didn't want to show the fingerprint because it is not valid for 
the modified value.
>
> 7. When you edit an existing key and set it into empty, it will say
> "Modified: key not set". I think we can just strikeout the fingerprint
> because it's the same as deleting the key.

Agree.
>
> 8. This is a minor server issue, not sure whether it's already fixed in
> the latest patch. When adding a new user with a key, the output of the
> user-add command contains the key too. In user-show the key is only
> shown with the --all option.
>

1-7 - I'll address them later.


-- 
Petr Vobornik