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

[Andrew Azores]

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:

- APPEXTSECunsetAppletActionNo should probably be renamed, why is the 
'No' there?
-- Should also be reworded, maybe "This applet has not been visited 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.
-- Please move the private empty constructor to the top of the class as well
- ExecuteAppletAction.fromString() - the RuntimeException message here 
should be reworded. "Undefined zero-length ExecuteAppletAction String 
representation" ?
- 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
- Rather than doing 'for (int i = 0; i < str.length(); i++)' and 
's.charAt(i)', how about for (char ch : s.toCharArray())?

Thanks,

-- 
Andrew A