SPARK-1944 #346
SPARK-1944 #346
Conversation
Alameyo
requested review from
speedy01 and
guusdk
and removed request for
speedy01
There was a problem hiding this comment.
It works! :)
I would make a couple of improvements:
- I don't think you need the text field in front of the button. If someone wants to add a path to the file, they can do that in the file chooser dialog
- When someone selects a file and presses OK, they should first see the content of the certificate, so that they can verify that they are indeed adding the certificate that they were planning to. There should also be a short explanation of what adding the certificate to the store means as well as a "ok" and "cancel" button. Something like: "Would you like to add this certificate to the keystore? By adding the certificate, Spark will agree to establish secured communications with servers that are identified by this certificate." (or, something better)
- The UI asks for an alias, but it does not show these aliases anywhere. We should add them as the first column in the table (or: auto-generate aliases, and show them no-where).
| certControll.addCertificateToKeystore(file, certAlias); | ||
| } catch (KeyStoreException | CertificateException | NoSuchAlgorithmException | IOException ex) { | ||
| Log.error("Cannot upload certificate file", ex); | ||
| } catch (IllegalArgumentException ex) { |
There was a problem hiding this comment.
An IllegalArgumentException is a runtime exception - which is an exception that is intended to be used when something occurs that is considered to be a programming error (not user in put error). You should not use these for things like User-input validation. For that, use a checked exception instead.
Runtime exceptions are generally not 'catched', but bubble their way up all the way to a very core class, which generically processes them in a big 'catch-all' block.
For some background, read https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html
There was a problem hiding this comment.
Perhaps we can prevent having an exception completely. When a user dies not fill out a correct alias, we could simply ask the user again, with some kind of warning (assuming that the user did not press 'cancel').
| } catch (KeyStoreException | CertificateException | NoSuchAlgorithmException | IOException ex) { | ||
| Log.error("Cannot upload certificate file", ex); | ||
| } catch (IllegalArgumentException ex) { | ||
| Log.warning("Certificate alias cannot be null"); |
There was a problem hiding this comment.
Please, always log the exception if you have one. Instead of Log.warning("blah") do Log.warning("blah",ex)
| throw new IllegalArgumentException(); | ||
| } | ||
| CertificateFactory cf = CertificateFactory.getInstance("X509"); | ||
| InputStream inputStream = new FileInputStream(file); |
There was a problem hiding this comment.
You can use try-with-resources here, I think.
|
I have changed it as you requested. I am not sure what I should do about that IlegalArgumentException. Maybe I should make one, combined dialog asking about alias and showing certificate but I also want it easy distinguishable and not hidden somewhere among certificate options (which there might me more soon). I also moved showCertificate() methods to controller. |
| @Override | ||
| public void actionPerformed(ActionEvent e) { | ||
| if(e.getSource() == okButton){ | ||
| //controller should be passed to this class only if there is need to modification content of Keystore. | ||
| System.out.println(certControll); |
| @@ -46,7 +47,7 @@ | |||
| private final static Insets DEFAULT_INSETS = new Insets(5, 5, 5, 5); | |||
| private final LocalPreferences localPreferences; | |||
| private CertificateController certControll; | |||
| private JTable certTable = new JTable(); | |||
| private static JTable certTable = new JTable(); | |||
There was a problem hiding this comment.
Why did you make this static? It results that all instances of this Panel will share the same instance of the table. I'm not sure if that's desirable.
There was a problem hiding this comment.
You can attach only one Truststore and it is somehow direct representation of it so that shouldn't be a problem but real reason why I changed it into static is I wanted to do this:
public void showCertificate() { CertificateDialog certDialog = new CertificateDialog(localPreferences, certificates.get(CertificatesManagerSettingsPanel.getCertTable().getSelectedRow())); }
There was a problem hiding this comment.
Now I left it as is but if you still feel that I should change it I will come back to this.
| @@ -58,6 +63,8 @@ public CertificateController(LocalPreferences localPreferences) { | |||
| case 2: | |||
| return String.class; | |||
| case 3: | |||
| return String.class; | |||
| case 4: | |||
| return Boolean.class; | |||
| default: | |||
| return String.class; | |||
There was a problem hiding this comment.
You have exactly 5 columns now, correct? The default block should never be executed, I think?
If that's correct, then instead of running a value, you should throw an unchecked exception, as it would clearly be a bug in the code if that state was reached!
There was a problem hiding this comment.
This is already a great improvement, but I think you need some more.
On the first screen, there is no real explanation of what is going on. There is a lot of that on the second screen (where you provide the alias). Perhaps that text should go to the first screen.
Also, the certificate screen looks weird for me:
- The content is wider than the dialog itself. Try to make them fit - or, alternatively, use scrollbars, but that's ugly I think.
- The "Distrust" label is not showing completely. It says "Dist..."
- The OK and Cancel buttons are weirdly separated - it's quite different from other screens.
| certControll.addCertificateToKeystore(file, certAlias); | ||
| } catch (KeyStoreException | CertificateException | NoSuchAlgorithmException | IOException ex) { | ||
| Log.error("Cannot upload certificate file", ex); | ||
| } catch (IllegalArgumentException ex) { |
There was a problem hiding this comment.
Perhaps we can prevent having an exception completely. When a user dies not fill out a correct alias, we could simply ask the user again, with some kind of warning (assuming that the user did not press 'cancel').
|
Also (I have not tried this), have you considered a user:
What would happen in those cases? What should happen? |
|
|
I don't think that overwriting is acceptable - at least not without a very clear warning. It will lead to very unexpected behavior otherwise. An alternative here might be to generate aliases automatically. If you do that, you can ensure that they're unique. Also, you then don't need to show them to the user at all. It should not be possible to have two times the same certificate (with a different alias) either. Again, this will lead to a lot of confusion. People that remove one of the duplicates will think that the certificate now no longer is used - while in reality it still is. Note that Spark is used by a lot of people, and that you're more of an expert on certificates than any of them. Even if things are obvious to you, you should always wonder if the average user (for example, think of your parents) can use the UI without your help. |
|
I tested it more and the most of the certificates are shown perfectly fine. While some are missing labels as on your screen and here: I will look what cause it but now I have no idea. |
… have alias as existing certificate/certificate is already in keystore/provided alias is empty.
|
|
Now we're getting somewhere! Here's a screenshot of your latest code, as how it showed for me:
There's a weird empty space on top - can you fix that? Also, the certificate wasn't scrolled to the top - that's odd too. I was able to press "ok" without making a selection whether to trust or distrust the certificate. When trying to test the error message for having no alias, I pressed "ok" without adding an alias. That, to my surprise, showed me "you already have this certificate" (which was true). Then I got back to the screen where I had to fill in an alias. Even when filling out an alias, I still got there. That's silly. When the user is adding a certificate that's already in the store, we can know this on the very first screen. There's no point in having the user to fill in an alias at all - it's going to fail anyway. |
I will try to fix that spacing at top , but that's problematic for me as I don't see it. This is how it appear for me.
I think I can setup it so it will be by default set as trust when adding certificate (why someone would like add certificate that he distrust?) unless it's invalid. User will be able to change it to distrust during adding or at any point later.
Ok, I will fix that. |
|
You might tey to both use same skin. Also, it might look differently on different OS i suppose. Although that should be cross platform :) |
…o set up certificate as trusted or distrusted depending on validity.
|
|
This looks good! The certificate is scrolled all the way to the top, and the weird empty space above the certificate is gone.
|
|
Oh, and I'd make the 'subject' column the first column, as that's more important than the issuer. Perhaps we shouldn't even show the issuer at all. That will only confuse people. What do you think? |
I decided I will make it trusted/distrusted depending on it's validity (which I still need to implement). If user still want to trust it he can change it by clicking trust/adding it to the exception list. End user might doesn't know that his certificate is invalid, this way we will make him sure that it is. I might add dialog that will say "This certificate is invalid, if you still want to use it check "trust"."
I am going to implement it with next PR, here is issue for that.
Maybe I should add it at least in certificate dialog?
Ok, I will try to do it, but I see that whole dialog with table is more stretched for you than for me.
I have no strong opinion which fields are the most important to show here, so if you think that I should remove issuer then I will do it. |
I like that idea!
Ok!
If you don't have a strong opinion, that lets drop it from the table, and try to show the 'subject' better. Less is more! :) |
|
Oh - and pressing the "Show Certificate" button gives a nullpointer when you don't have a certificate selected. Easy fix: don't allow the button to be pressed when there is no selection in the table. |
|
Those buttons doesn't have to be so wide. This looks weird :) "Show certificate", "Choose file". |
|
@wrooot For me it is wide but not as wide as on Guus's screenshot. |
|
I don't have this PR branch merged, so can't test how it looks now, but in the previous PR maybe window was a bit narrower and buttons looked ok. It seems that the window is stretched now and buttons gridbags have been stretched along. |
…ed show certificate button until user click on table. Added dialog that shows up when certificate is invalid.
|
So that's how it look like now for me: |
|
looks nice. are the columns sort-able? that would be nice if they could be. That could save someone a lot of time having to look for exceptions or for a cert to make a change to. |
|
This columns are sortable but currently after sorting there is displayed wrong certificate if you click on them. |
|
There's something weird with the table. Its content does not use all space on the right side. Could you have a look at that?
Also, when a certificate has a "CN" value, I think we can show only that. So, instead of a list like this:
We'd see a list like this:
That's a lot easier on the eyes. Obviously, when you open th certificate, you should still see everything. Also, certificates are not required to have a Common Name. In such case, let's still show everything. Finally, in the 'valid' column, let's add a a description of why something is not valid (possibly in a different color - red or orange). Possible values:
Oh, and could you center those values? That also looks a lot nicer, I think. |
|
Guus are you using some unusual wide resolution? |
|
Nope - but I'm using a non-Windows desktop manager, so that might account for some of the weirdness. This is actually good - Swing does allow the developer to prevent this, when applied properly. It's somewhat of a hassle to get it right, but we're better of seeing these issues now, than in production. :) |
|
For me on Windows it opens without this gap on the right. But I'm not using this PR. Last one merged. |
…y, centred values.
|
I hope now it stretching good (this took me to long). Validity cells are green if certificate is valid, red if invalid, grey if self signed (in some situations, for end certificates it's not fine, on the other hand root CA are self signed and it is fine so I give to it distinguished color). Validity column values are centered. I left Subject column as is because length of it's value can differ and when centered it is almost unreadable. I also added some basic validation (by date and self signed) in CertificateModel and I done some changes in i18n values. |
|
This is looking great! I have a couple of small remarks, but I'll raise them as JIRA issues. That will allow us to merge this PR. Issues:
|
Implementation of uploading certificates into Truststore. I also changed some of access modifiers to private.
I am not sure if I am doing good use of text field next to choose file. Maybe it would be better if I put there text "Certificate has been uploaded" or delete that text field at all.