[rfc][icedtea-web] PolicyEditor Parser patch
Andrew Azores
aazores at redhat.com
Thu Jul 30 14:51:23 UTC 2015
On 30/07/15 07:41 AM, Jiri Vanek wrote:
> 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 :)
I can see that the NEWS also has the whitespaces trimmed like the
ChangeLog did previously, but I figured that it's small and contained
enough of a change that it isn't that annoying to let through. For the
properties files though, I'm not seeing it. There are a lot of renamed
keys because of the transition from modeling only Codebases to full
Identifier triples, and many new keys for new UI elements, but I don't
see any of the whitespace trimming or anything suspicious like changed
line endings...
>
>
>>>>>
>>>>> 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)
Yes, this is working as intended.
>>
>> 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?
Sure, I'll look into doing something for this.
> - 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)
Okay, I have no strong feelings either way. Continuing on even with
invalid CLI arguments made sense because it's a GUI tool and the
arguments are just meant to slightly speed up opening the tool, but it
does also make sense to die on invalid input even if only so that it's
more clear to the user why what they asked for wasn't selected when the
tool opened (since it doesn't open).
>
> Thanx for great contribution! And pelase dont forget about "as another
> changesets:) )
>
> J.
>
I have them marked down on my todo notes, I'll get around to it all...
sometime...
>
> If so,m then just idea. Both itw-settings and javaws have headless
> uspport. Do you think to add something like this to this tool?
>
> If above is right, then you are done, and you just need to "save file"
> and not show gui....
>
> hmm?
>
> J.
Yea, I suppose this would be possible... how useful would it be, though?
All it would allow for is specifying a selector and then having a
matching grant entry created in the specified policy file. If that grant
entry already exists then nothing happens. Maybe if someone wants to
build support for adding/removing permissions on the CLI then this would
be an interesting feature to have but right now it seems really, really
limited. Certainly possible though if you think it's worth doing.
This did just give me the idea that there should probably be some
-defaultfile switch that opens the PolicyEditor to the default policy
file, though. There's a button on the menubar for it but no way to
automatically open to that file directly from the CLI. I'll work on that
next, I think.
--
Thanks,
Andrew Azores
More information about the distro-pkg-dev
mailing list