[rfc][icedtea-web] PolicyEditor Parser patch

[Andrew Azores]

On 29/07/15 11:27 AM, Jiri Vanek wrote:
> On 07/27/2015 09:35 PM, Andrew Azores wrote:
>> On 27/07/15 08:53 AM, Jiri Vanek wrote:
>>> On 07/23/2015 05:55 PM, Andrew Azores wrote:
>>>> Hi,
>>>>
>>>> Attached is the PolicyEditor Parser patch from a few months ago, 
>>>> updated to apply to current HEAD as
>>>> of today. I also made one small modification so that re-opening a 
>>>> "new" file which is actually the
>>>> same file does not count as a change made.
>>>>
>>>
>>> Hi, thanx.
>>>
>>> Please before push reconsider following bugs:
>>>  lunch policy editor
>>>   do some changes
>>>   save them
>>>   close
>>>
>>> File is not saved (do not ecven exists as empty)
>>>
>>> open file like policyeditor [-file] filename
>>> apply [confirmation changes saved appear]
>>> close, file is again nto saved (but at least file exists, an comment 
>>> //generated, timestamp, do
>>> not edit// is present
>>>
>>>
>>> So maybe those "not saved" have cause in some invalid entries aI'm 
>>> putting in.  But no exception
>>> occures, and even iof so, not saving at all and simply close and 
>>> discard all is bad.
>>>
>>>
>>> So actually I was not able to make some "load" nor "selector" 
>>> testing. Becasue I never get the
>>> content saved. I woudl storngl recommad one more iteration with 
>>> above fixed.
>>>
>>
>> I believe I've fixed all of the above.
>>
>>>
>>> If you wont go on and push and fix it as another changset (but 
>>> please do!)
>>> Few another things should be fixed as  additional changesets.
>>>  - when you open policyeditor with filenameArgument of -file 
>>> filename then the file is always
>>> created. Even if not saved
>>
>> Also fixed.
>>
>>>  - when crating rule you can have codebase empty, when editing, you 
>>> can not
>>
>> Ah, nice catch, thank you. Fixed.
>>
>>>  - I'm not saying that current modify schema is wrong, but maybe the 
>>> modify dialogue same as
>>> creation dialogue for all fields in one would be nice. (but I dont 
>>> know the usecase here, so your
>>> current mpl may be more correct)
>
> Can you please push the changes to changelog as separate changeset? Ay 
> you check how ti applies to 1.6? If applies, pelase, push.

As we discussed on IRC, this was an accident. I have my editor set to 
trim trailing whitespace because it's usually a nice thing to have, but 
I definitely didn't mean to include that huge amount of whitespace 
trimming with this patch. I've undone that for the ChangeLog in this patch.

>>>
>>> J.
>>
>> Sure, I suppose. I like the more granular implementation as it is 
>> because it really works nicely
>> with keyboard shortcuts, since for codebase and signedBy there's only 
>> one thing to edit and so when
>> you bring up the edit dialog with a keyboard shortcut, there's only 
>> one text field and it's
>> pre-selected, so you can just type the replacement and hit 
>> Enter/Return. But I can see that it might
>> be a little clearer if the Edit options were merged and presented the 
>> same as the New Entry dialog.
>> I'll think about it and look at submitting a patch for this later on.
>>
>
> So the fact, that it is saving nothing is caused by policies whcih 
> have no rules.
>
> So where Iwould expect eg
> grant signedBy "bb" {};
>
> it saves nothing.
>
> Can you save empty rules?
>
> If no, then I would advice to warn user: "this policy have no ruels - 
> will be discarded" or similar during saving... Thoughts?
>

Done, empty rules get saved now.

>
>
> The default http:// in  creation dialogue have two blades. On one side 
> I'm happy it do not need to e fill, on second.. when yo save only 
> "http://" it is strange... Or is it desired?
> I'm probably for droping it, and adding soem toolbar with "eg 
> https://your.domain/" or os...
> Also infomration that it must be valid URL or empty is worthy....
>
>

I've made it so that the label says "Codebase URL" to make that a little 
more evident and removed the "http://" pre-filled text. I'd like to come 
back to this in a separate changeset to do some more work on making it 
more obvious what exactly needs to be filled in in each of these fields 
in the creation dialog.

>
>
> During loading of balhX fatal runtime exception ocures
> Exception in thread "AWT-EventQueue-0" 
> java.lang.IllegalArgumentException: fromIndex(0) > toIndex(-1)
>     at java.util.ArrayList.subListRangeCheck(ArrayList.java:1006)
>     at java.util.ArrayList.subList(ArrayList.java:996)
>     at 
> net.sourceforge.jnlp.security.policyeditor.PolicyIdentifier.toString(PolicyIdentifier.java:112)
> (very fatal:( )
> (note  5 minutes  later - not only this file many others... but it 
> soemtimes suddenly start to laod...! 0 theis blahX keep doing it)
>

Hmm, odd. I've put a guard in this method to avoid this.

> Please Update the man/help texts. Especially synopsis would like to 
> have example on selectors (maybe another changeset, but soon enough 
> :)) (grep for PEsynopseP1 both in code and proeperties)
>

Another changeset, I think. This one is really, really ballooning out of 
control...

>
> The selectors did not worked:(
>
> [jvanek at jvanek Desktop]$ sh ~/icedtea-web-image/bin/policyeditor 
> -codebase http://aa -file blah
> [jvanek at jvanek Desktop]$
> [jvanek at jvanek Desktop]$ sh ~/icedtea-web-image/bin/policyeditor 
> -codebase http://aa -file blah
> [jvanek at jvanek Desktop]$ sh ~/icedtea-web-image/bin/policyeditor 
> -signedby bb  -file blah
> [jvanek at jvanek Desktop]$ sh ~/icedtea-web-image/bin/policyeditor 
> -signedby bb  -codebase http://aa  -file blah
>
> Never selected anything. And I had
> grant signedBy "bb" { ...
> grant signedBy "bb",  codeBase "http://aa" {...
> grant codeBase "http://aa" {...
>
>
> Proabbly selectors should work like:
>  - find match where all matches
> nope?  - find first matchwhere at elastsomething matches
>
> Or Am I missing smething?
>
>
> Also - I did not check if this is connected with "run in sandbox" 
> button. IS it?
>
> Much better then last time!
>
>  tHANX!
>
>
> j.
>
>

Thanks for the review! I'd forgotten to test the selector stuff. I have 
it fixed now - the problem was that opening and parsing the policy files 
was async, but the selector stuff was being done synchronously. 
Synchronously after the async job was kicked off, at least, but then 
almost guaranteed in real-time to actually happen first. I've fixed that 
now by making it so that PolicyEditor instances don't automatically 
start opening and parsing policy files as soon as they're instantiated, 
and instead the caller has to invoke this step when ready.

And yes, the Run in Sandbox stuff is still connected, as well as the 
itweb-settings launching.

-- 
Thanks,

Andrew Azores

-------------- next part --------------
A non-text attachment was scrubbed...
Name: policyeditor-parser-3.patch
Type: text/x-patch
Size: 272486 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150729/19da37f0/policyeditor-parser-3-0001.patch>