[rfc][icedtea-web] conditional disabling of manifes tattributes

[Andrew Azores]

On 05/07/2014 04:26 AM, Jiri Vanek wrote:
> On 04/16/2014 04:01 PM, Andrew Azores wrote:
>> On 04/16/2014 03:08 AM, Jiri Vanek wrote:
>>> On 04/15/2014 05:48 PM, Jiri Vanek wrote:
>>>> On 04/11/2014 06:49 PM, Andrew Azores wrote:
>>>>> On 04/11/2014 09:07 AM, Jiri Vanek wrote:
>>>>>> On 04/10/2014 04:44 PM, Andrew Azores wrote:
>>>>>>> On 04/10/2014 08:43 AM, Jiri Vanek wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> This patch can disable the check for new manifest attributes.
>>>>>>>> sevral motivations
>>>>>>>>   -  I hate this feature
>>>>>>>>   - the remembering of this actions may take time to implement
>>>>>>>>   - and the most important  - the testsuite...
>>>>>>>
>>>>>>> Most important indeed.
>>>>>>>
>>>>>>>>
>>>>>>>> will go both head and 1.5.
>>>>>>>>
>>>>>>>> I'm hesitating whether to make this property public via 
>>>>>>>> itw-settings... Well if so, then as
>>>>>>>> another changeset. Any opinion about publish it in itw-settings?
>>>>>>>>
>>>>>>>> J
>>>>>>>
>>>>>>> Ok to push, so that we can clean up some of the noise in the 
>>>>>>> test suite.
>>>>>>>
>>>>>>> RE putting it in itweb-settings - I think the cleanest way to do 
>>>>>>> this, if it's to be done at
>>>>>>> all, is
>>>>>>> probably to add another security level. 'Minimal' or something 
>>>>>>> perhaps? Because I don't think it
>>>>>>> makes sense to add it in as a checkbox, for example, and have it 
>>>>>>> possible to set security to
>>>>>>> High,
>>>>>>> but disable manifest checks. I'm not sure if we really want to 
>>>>>>> allow this, though. I can imagine
>>>>>>> most users leaving the security level at Minimal once they 
>>>>>>> discover that this gives them the
>>>>>>> least
>>>>>>> amount of clicking to do before getting their applets to run :)
>>>>>>
>>>>>>
>>>>>> Oh yes, that can be correct thing to do. But looking to the issue 
>>>>>> from this perspective, it is
>>>>>> NO-GO  argument for it as it is done. Well all correct 
>>>>>> applications should be updated to support
>>>>>> new attributes.. but the legaxy, and still used ones?
>>>>
>>>> I have pushed at the end. This shortcut for testsuite is needed.
>>>>>>
>>>>>> So I have another idea here - really extend 
>>>>>> ExtendedAppletsSecurity, and to allow "do not check
>>>>>> manifest attributes" on the application/codebase  - So all those 
>>>>>> dialogs will be moreover based on
>>>>>> your abstraction of Extended Applets security. Also remember 
>>>>>> alaca checkbox issue will be solved
>>>>>> by this.
>>>>>>
>>>>>> Bu the extension of ExtendedAppletsSecurity will needed to be 
>>>>>> done really carefully - and prepared
>>>>>> before implementation.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Sounds like it could be a lot of work, but should be worthwhile in 
>>>>> the end. I think it will be very
>>>>> nice if we can unify all of our dialogs like this so that they can 
>>>>> all make use of the same
>>>>> remembered action storage. Or at least, the ones that need to be 
>>>>> able to remember actions. I'm not
>>>>> sure about having the manifest attribute check be an option to be 
>>>>> selected on the dialog though,
>>>>> even with an option to remember it... but maybe it's the right 
>>>>> thing to do. It just seems like the
>>>>> dialogs are getting busier and busier and more confusing looking, 
>>>>> but you're right that this option
>>>>> will (hopefully) be used mostly for allowing some old applets 
>>>>> which haven't been updated to be used
>>>>> still. In that kind of situation, which I think is the target, 
>>>>> then applying the setting to all
>>>>> applets/globally maybe isn't the right choice. So as ugly and 
>>>>> cluttered as the UI might become, I
>>>>> suppose this really should be done on a per-applet basis (maybe in 
>>>>> addition to having it
>>>>> configurable globally?).
>>>>>
>>>>
>>>> Hi attached patch is adding the basic support for multiple types of 
>>>> actions. Well it apeared that
>>>> the implementation itself will be quit easy (if this approach will 
>>>> be used) but most troubles will
>>>> be in itw-settings. The table will requires serious revork on level 
>>>> of filtering, sorting, delting
>>>> and especially editing of first - action - column.
>>>>
>>>> General idea is - instead of one   ExecuteAppletAction 
>>>> unsignedAppletAction will be list of them,
>>>> encapsualted in  AppletSecurityActions unsignedAppletAction;
>>>> In file it will be encoded instead of current one char, as one-word 
>>>> string - see the tests.
>>>> To read/write the correct action for given type, there will be set 
>>>> of getters/setters  - tight now
>>>> there is
>>>>   public ExecuteAppletAction getUnsignedAppletAction() {
>>>> +        return getAction(0);
>>>> +    }
>>>> +
>>>> +    public void setUnsignedAppletAction(ExecuteAppletAction a) {
>>>> +        actions.set(0,a);
>>>> +    }
>>>>
>>>> And they will encapsulate the order in list.
>>>>
>>>>
>>>> If nothing else...This is nicely backward compatible, and scaleable 
>>>> for growth of "remember me"
>>>> dialogs :)
>>>>
>>>> Once this is aproved -if ever -  I will add few more "on top of it" 
>>>> work like -table or rember for
>>>> ALACAto se hw it works - before push itself.
>>>> J.
>>>>
>>>>
>>>
>>> This is a bit better - get rid of paranoid constant of MAX_LENGTH.
>>>
>>> J.
>>>
>>
>> Just a few nits:
>
> Here we go!
>
>>
>> - APPEXTSECunsetAppletActionNo should probably be renamed, why is the 
>> 'No' there?
> fixed
>
>> -- Should also be reworded, maybe "This applet has not been visited 
>> before"
>
> kept - es expalined before
>
>> - AppletSecurityActions.fromString() - why 'break' on whitespace? I 
>> can understand if it was only
>> internal whitespace, but even leading whitespace? eg the string " A" 
>> means UNSET? Maybe do a
>> String#trim() first.
>
> well, fixed, but see the comment in code
>
>> -- Please move the private empty constructor to the top of the class 
>> as well
> removed completely
>
>> - ExecuteAppletAction.fromString() - the RuntimeException message 
>> here should be reworded.
>> "Undefined zero-length ExecuteAppletAction String representation" ?
> fixed
>
>> - ExecuteAppletAction should be refactored now that it has both a 
>> toChar and fromChar. I would
>> suggest retaining the corresponding char as a field (so make a 
>> one-arg constructor), then toChar
>> simply returns this and fromChar iterates over the enum values. If 
>> there is a matching enum value's
>> char, return that enum value, otherwise return UNSET
>
>
> Unless you really insist, I kept my original approach. I like it more.

I don't insist, I just think doing it my way might be nice because 
adding new kinds of action in the future doesn't require remembering to 
add a corresponding case to the switch. Admittedly it does make the enum 
itself maybe a little more difficult to read. Anyway, I'm fine with 
keeping it as is.

>
>> - Rather than doing 'for (int i = 0; i < str.length(); i++)' and 
>> 's.charAt(i)', how about for (char
>> ch : s.toCharArray())?
>
> ok :)
>>
>> Thanks,
>>
>
>
> The unittest was enhanced to test backward compatibility and new feature.
>
>
> Thank you again.
>
> J.

APPEXTSECunsetAppletAction: s/have/has/
and some of the license headers say 2013, maybe they should say 2014 ;)

Okay to push after that though.

Thanks,

-- 
Andrew A