[rfc][icedtea-web] PolicyEditor Parser patch

[Jiri Vanek]

On 07/29/2015 06:40 PM, Andrew Azores wrote:
> 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.
>

Now NEWS and all properties files are affected. For properties,  by plain eye I can not see why.

However - both changes to news,  changelog(especially changelog and news) and to properties  looks 
ok anyway.

+1 to push changes to changelog as they are, and +1 to push chnages to proeprtiesfiles when you find 
what the changes actually are :)


>>>>
>>>> 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.

ok.
>
>>
>>
>> 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...

agree.

>
>>
>> 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

works now.

> 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.

When selector is set, and it do not exists in target file - new item is crerated. Is it expected 
bahvior?
(-me guess yes)
>
> And yes, the Run in Sandbox stuff is still connected, as well as the itweb-settings launching.
>


I had not found some serious flaws... Ok for head.


Few more its to another chnagesets (except what was already told)
- order f loaded items is alwasy random.. Maybe some sort?
- when invalid argumenr is sent:
eg
[jvanek at jvanek icedtea-web]$ ~/icedtea-web-image/bin/javaws  -codebase http://aa -signedby bb
netx: Invalid argument: net.sourceforge.jnlp.util.optionparser.InvalidArgumentException: [-codebase, 
http://aa, -signedby, bb]

netx should die.... policyeditor dont :) how come?

I'm+1 for dying (as another changeset)

Thanx for great contribution! And pelase dont forget about "as another changesets:) )

   J.