[rfc][icedtea-web] Backport unsigned applet confirmation to icedtea-web 1.2

[Adam Domurad]

On 02/28/2013 12:01 PM, Jiri Vanek wrote:
> On 02/27/2013 10:02 PM, Adam Domurad wrote:
>> On 02/27/2013 12:01 PM, Jiri Vanek wrote:
>>> On 02/26/2013 10:59 PM, Adam Domurad wrote:
>>>> NB: This is a work in progress, posting for early feedback.
>>>>
>>>> So a back-port was requested for 1.2 for systems that still use it. 
>>>> I tried to use as much
>>>> already-reviewed material as possible while retaining simplicity.
>>>>
>>>> The whitelist attempts to be forwards compatible with the patch 
>>>> aimed at HEAD, but generally it does
>>>> plain string matching without regex.
>>>>
>>>> The icedtea-web settings panel simply has a box that changes the 
>>>> security level, and a button to
>>>> clear the current whitelist.
>>>>
>>>> I *think* everything is working, but I was in a rush posting this, 
>>>> so let me know. There are a few
>>>> println statements left in, but again, rush.
>>>>
>>>> This is of course meant to be applied to the 1.2 release branch.
>>>>
>>>> Cheers,
>>>> -Adam
>>>
>>> Hi!
>>>
>>> I like this backport. Looks like it is working....
>>> I like that you have  addapted  as much code as possible, and you 
>>> are keeping forward compatibility.
>>> => Maybe it is also worthy to backport 
>>> UnsignedAppletActionEntry.java , which is handling the loading of 
>>> lines.
>>
>> I looked at it but decided to go for the simplest route possible with 
>> the matching/storing/loading, which was treating the file as a set of 
>> strings.
>>
>>> Also I like the url normlaization stuff.
>>>
>>> mayor nit:
>>> The checkbox is not working. Decision is always saved.
>>
>> Sorry, this is a remnant of the behaviour of storing y/n in HEAD. 
>> Fixed now.
>>
>>> Proceed/cancel is not saved correctly  - in all cases "N" is saved
>>> The values in .applettrustSettings are overriding main policy. Is 
>>> this even in head?
>>
>> Really ? I couldn't reproduce this. Still doing as much testing as I 
>> can. Let me know if you have concrete reproduction steps. Note that 
>> the browser will have to be restarted after changing setting in 
>> itweb-settings
>>
>>> This file should be checked only (in case of this backport) only 
>>> during "high security"
>>>
>>> Minor nits - in head the items in combobobx are sorted in reversed 
>>> order.
>>
>> Fixed
>>
>>> Sometimes I noted that the combobozx have not setup its value 
>>> accordingly user's value
>>
>> I am not sure what you mean. Do you have steps to reproduce this?
>
> I was not able to reproduce again.
>>
>>> The exeption should be maybe more clear (eg distinguish custompolicy 
>>> and "by applet| policy in its text) - I think this is valid also fo 
>>> head
>>
>> The one thrown when applet is denied ? Good point. Added 'The applet 
>> was unsigned, and was not trusted.' for when user rejects / blacklist 
>> rejects the applet. They are now 
>> LUnsignedAppletPolicyDenied/LUnsignedAppletUserDenied. I will include 
>> it in my next version for HEAD.
>
> Thanx!
>
> Maybe for head you can think about even more verbose errors:
> policy middle, but applet untrusted
> well, appelt trusted, but policy to strict....
> or so on... There is a lot of cases:)
>
>>
>>>
>>> imho both
>>> User file : /home/jvanek/.icedtea/deployment.properties
>>> System file : null
>>>  outputs should be gone...
>>
>> Sorry! Those were the 'println's that I noted.
>>
> sure ;)
>
>>>
>>> For 1.3 I would ratehr  like to have full backport of head.
>>
>> I'm not too strongly opinionated either way.
>
> good :) Then head to 1.3 :)
>>
>>>
>>>
>>> Thanx for backport!
>>>
>>>    J.
>>
>> Attached is new version, also added missing copyright headers.
>
> thanx!
>
> Two mayor nits:
> the code can not be transleted by jdk6:
>         final JComboBox<String>  comboBox = new 
> JComboBox<String>(items); - >         final JComboBox comboBox = new 
> JComboBox(items);

Very good catch, fixed

>
>
> You have switched the huma readable strings  in combobox, nut not the 
> input values, so extra high is actually assigned to ALLOW_ALL and low 
> is assigned to DANY_ALL_UNSIGNED
> both discussed on irc...

Also very good catch, fixed now

>
> minor -  you are saving "1" to .applets file - is new Date().getTime() 
> to problematic?

  I think it is -- it would be defeat my simple-as-can-be matching 
method, and I don't see a significant benefit.

> minor - your combobox is aligned to second side then rest of controls 
> (try to maximize it)

I didn't quite understand this ? What do you mean by aligned to the 
second side ? It's currently center aligned but looks fine to me.

>
> I'm actually fine with above fixed for 1.2....
>
> I would like to backport also the locking file unittests and (adapted) 
> mathching unittests which are both in head. But do as you want.

It's not trivial unfortunately, since 1. there is no 
LockingReaderWriter, 2. the matching always looks .appletTrustSettings. 
I can still do it if you think its worth the effort/re-engineering.

>
>
> Feel free to turn one more round on distro-list if you feel so.
>
> J.
>


Thanks,
-Adam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dialogue-1.2-backport4.patch
Type: text/x-patch
Size: 50304 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130228/d3936aa6/dialogue-1.2-backport4.patch