[rfc][icedtea-web] policytool in itweb-settings
Jiri Vanek
jvanek at redhat.com
Fri Jan 17 02:35:24 PST 2014
snip
>>>
>>> Okay, the "couldNotOpenFileDialog" is more informative about the specific reason that it's appearing
>>> now, and I'm using a DirectoryValidator to check that we'll be able to make/edit a policy file. I've
>>> also removed the requirement that the file be writeable - you're right, it does still make sense to
>>> let the tool launch, even if the file is read-only. And it's policytool's responsibility to deal
>>> with the problem if it turns out the file is read-only. Besides, I did label the button as "View
>>> Policy", not "Edit Policy" ;)
>> Please remove unnecessary html from:
>> + JLabel aboutLabel = new JLabel("<html>" + R("CPPolicyDetail") + "</html>");
>
> The html tags are there to enable line wrapping in the JLabel. We do the same thing in the
> AboutPanel, otherwise the label just shows one incredibly long line of text. We could also make it
> into a JTextArea and enable line wrapping directly without using html tags, but then the text area
> isn't going to actually look the same and it'll take extra work to make it fit in.
>
>>
>> Also I would like two things here:
>> - If file is not writeable, warn user and lunch policy tool anyway
>> - otherwise show proper mesasge end do nothing as you do now.
>
> Great idea, done.
>
Ugh. It still do not work.
In 000 or 222 permissions - the policy tool executed with "read only" message. It should not opena
at all, and correct error message should apear (can not read)
so - rw - no message, open policy toool
- r- - read only warning, lucnh
- -w or --, can not read/write, not luching
>>
>> Please rename button to: "view/edit by policy tool" - it is what is really does.
>
> Also done.
>
>>
>>>
>>>> few more nits:
>>>>
>>>> Now the polici file is visibl ein policy tool, in plolicy file in readonly textfield (I'm
>>>> wondering why it was not visible before). Thats good, but I would like to see it in itw settings
>>>> itself:( Please add similar readonly jtextfield to police settings pane.
>>>> You can keep also current tool tip text;)
>>>
>>> I have done this, but it's kind of ugly looking. And I really don't know how to make things look
>>> pretty. :)
>>
>> My suggesttion:
>>
>> ****text***t*t*t*t*t*t
>> **text**t**tt*tt*t*t*t
>> ****text***t*t*t*t*t*t
>> **text**t**tt*tt*t*t*t
>> [textfield with path] [button]
>>
>> What do you mean?
>>
>> I think that in future we may end up with something like
>>
>> ****text***t*t*t*t*t*t
>> **text**t**tt*tt*t*t*t
>> ****text***t*t*t*t*t*t
>> **text**t**tt*tt*t*t*t
>> [textfield with path to local file ] [simple edit button][policy tool button]
>> [textfield with path to global file] [simple edit button][policy tool button]
>>
>> Maybe ot soon, nor ever. But stil it will be nicer :) And you will discover a bit of swing O:)
>
> I have no idea if what I've done is a good way to do this, but it seems to have worked and looks
> reasonable...
yes! Nice (IMO)! Tahnkx!
>
>>
Unluckily I found one overlooked flaw - when policy tool is closed, it close ITW-settiings. It
should not happen.
If it will be to complex* to fix, then I'm for this fix as another changeset. (as I would really
like this changeset (in simplest form) bubble to 1.4)
* I'm afraid You will have to owerride something or modify it via reflection or use some another
nasty ahck to fix it :(
>>>> + private static File policy = new File(System.getProperty("user.home") +
>>>> "/.config/icedtea-web/security/java.policy"),
>>>> + policyBackup = new File(System.getProperty("user.home") +
>>>> "/.config/icedtea-web/security/java.policy.bak");
>>>>
>>>> nope - you have to use value from deployment configuration
>>>
>>> Yea, this was just carelessness :( wrote it as a quick hack and then forgot to go back and actually
>>> fix it properly. Classic story, isn't it?
>>>
>>>>
>>>> + if (policy.isFile()) {
>>>> + if (policyBackup.exists()) {
>>>> + throw new RuntimeException("Backup policy file already exists");
>>>> + }
>>>>
>>>>
>>>> Instead of this (this can simple become blocker if tests are killed) I would recommand to use some
>>>> random-suffixed.bak file. Of course feel free to throw out if backuping fail and log out worning
>>>> if old backups exists.
>>>>
>>>> try {
>>>> + out = new FileWriter(policy, false /*no append*/);
>>>> + } catch (IOException e) {
>>>> + throw new RuntimeException(e);
>>>> + }
>>>> +... and much similar
>>>>
>>>> Its not worthy to catch. Please let already thrown exception flow. Of course try to close in
>>>> finally. (especially you will avoid try{}finally{try{}catch{}} nastyness.
>>>>
>>>> Both your testcases should share shared code (namely backupPolicy and restorePolicy ;) ) And also
>>>> yor assersert* methods (just with negation ;) )
>>>>
>>>> Otherwise the tests looks really good!
>>>>
>>>
>>> Well, the reason I had the tests split into two testcases files like that was because one set of
>>
>> Oh I did not wont you to join files, nor reuse before/after method themselves. Just logic of them.
>> I'm terribly sorry for not being clear!
>>
>> Please split the files again. They are compiled together, so they can reuse thirs package
>> private/public methods or extends themselves.
>>
>> The previous before/after backup was nicer then current one.
>>
>>
>> if (policy.isFile()) {
>> + for (int i = 0; i < MAX_BACKUPS; ++i) {
>> + if (!policyBackup.exists()) {
>> + break;
>> + } else {
>> + policyBackup = getRandomizedBackup();
>> + }
>> + }
>>
>>
>> :DDD (sorry :) )
>>
>> Please use File.createTmpfile(dir,pattern) instead :)
>> It guarants you creation of the file, so you do not need to throw.
>
> I didn't do this in the attached new patches, because I'm not sure about it. This is going to create
> temp files, eg in /tmp - do we really want to back up to here? If the power goes out during a test
nope :)
Se again: File.createTmpfile(dir,pattern). You can let it crate file in any dir -
http://docs.oracle.com/javase/7/docs/api/java/io/File.html#createTempFile%28java.lang.String,%20java.lang.String,%20java.io.File%29
Pls really use this instead of your schema.
Also - I have just found :
policyBackup.renameTo(..)
policyBackup.delete()
It jsut do not have much sense to rename and then delte.. ORr what have I missed?
> run then you might just lose your user policy file entirely. The patch now does the backup simply by
> renaming the policy file without moving it to any other directory, so that it should generally be
> safe and remain recoverable even if the machine dies during the test (barring problems out of our
> hands like filesystem corruption). The thrown exception is only there if we can't back up the policy
> at all, probably because there are too many existing backups. Using File.createTempFile() would be
> nice because it would simplify the backup/restore process but I don't really like the idea of
> backing up policy files to non-persistent and possibly volatile storage... but maybe I'm worrying
> too much about this.
>
Otherwise I agree with your statement. The security dir is where it shold be.
>>
J.
More information about the distro-pkg-dev
mailing list