[rfc][icedtea-web] Temporary permissions

[Andrew Azores]

On 03/26/2014 01:18 PM, Jiri Vanek wrote:
> On 03/25/2014 08:28 PM, Andrew Azores wrote:
>> On 03/25/2014 03:13 PM, Jiri Vanek wrote:
>>> On 03/25/2014 06:34 PM, Andrew Azores wrote:
>>>> On 03/25/2014 05:40 AM, Jiri Vanek wrote:
>>>>> Generally ok, except one important nit:
>>>>>
>>>>> final JMenuItem noCmdExec = new JMenuItem(R("STempPermNoExec"));
>>>>> + noCmdExec.addActionListener(new TemporaryPermissionsListener(
>>>>> + Permissions.READ_LOCAL_FILES, Permissions.WRITE_LOCAL_FILES,
>>>>> + Permissions.READ_TMP_FILES, Permissions.WRITE_TMP_FILES,
>>>>> + Permissions.READ_PROPERTIES, Permissions.WRITE_PROPERTIES,
>>>>> + Permissions.CLIPBOARD_ACCESS, Permissions.PLAY_AUDIO,
>>>>> + Permissions.PRINT_DOCUMENTS, Permissions.JAVA_REFLECTION,
>>>>> + Permissions.NETWORK_ACCESS));
>>>>> + policyMenu.add(noCmdExec);
>>>>>
>>>>> And similar:
>>>>> are - duplicated.
>>>>>
>>>>>
>>>>> You can push, as similar evil is already done.
>>>>>
>>>>> However refactoring, asn asap changeset is needed.
>>>>>
>>>>> the ||| advanced settings button will be encapsulated, so non of 
>>>>> its code is duplicated.
>>>>> You can have your own "extends jbutton" hwhich will do all this 
>>>>> logic. And you add jsut
>>>>> insntance of this button to the panes.
>>>>>
>>>>> Also all the :
>>>>> new TemporaryPermissionsListener(
>>>>> + Permissions.READ_LOCAL_FILES, Permissions.WRITE_LOCAL_FILES,
>>>>> + Permissions.READ_TMP_FILES, Permissions.WRITE_TMP_FILES,
>>>>> + Permissions.READ_PROPERTIES, Permissions.WRITE_PROPERTIES,
>>>>> + Permissions.CLIPBOARD_ACCESS, Permissions.PLAY_AUDIO,
>>>>> + Permissions.PRINT_DOCUMENTS, Permissions.JAVA_REFLECTION,
>>>>> + Permissions.NETWORK_ACCESS));
>>>>> + policyMenu.add(noCmdExec);
>>>>>
>>>>> should be encapsualted. Those permissionListeners may be stored in 
>>>>> some static class as static
>>>>> constantsand just used as singletons. or not?
>>>>> If not then they can be provided by some factory methods. But not 
>>>>> spred in gui code like this.
>>>>>
>>>>> Yes, just code celaning, but how it is now, i do not look nicely.
>>>>>
>>>>> Sorry for troubles:(
>>>>> J.
>>>>>
>>>>
>>>> You're right, it was getting very messy, and the code for the 
>>>> button/menu stuff definitely should
>>>> have been shared.
>>>>
>>>> So now, behold the monster you've asked for... ! :D
>>>>
>>>> 1) Button is pulled out into its own class and used in the two dialogs
>>>> 2) Popup menu is actually even simpler to make appear now (less 
>>>> futzing with position on screen)
>>>> 3) !! PolicyEditorPermissions are reused. It's some slightly scary 
>>>> reflection with a bit of regex
>>>> sprinkled in, but it works perfect.
>>>> 3b) Some of the temp permissions have been removed because 
>>>> PolicyEditorPermissions don't yet have
>>>> the right counterparts. But there's another patch I sent earlier 
>>>> that adds all of those
>>>> permissions as well, so then they can be added in quite quickly 
>>>> here after
>>>> 4) Temp permission types and menu entries are much much cleaner to 
>>>> define
>>>>
>>>
>>>
>>> hmhmhm - just see:
>>> +STempSoundOnly=Play audio
>>>  it is same issue as in case of reflection. it should probably allow 
>>> both recording and playing.
>>> UTwo permissions under one checkbox.
>>
>> Really? I can imagine many scenarios where a user is comfortable with 
>> an applet playing sound back
>> but not recording it. Reflection makes sense to group together but I 
>> think audio should be separate.
>>
>>>
>>> - maybe as tmp hack can serve some better grouping of checkboxes, 
>>> with one main - whcih will
>>> select/unselect all underlying. By this you will have your 
>>> one-permission-per-checkbox - and user
>>> will not be bored by to much on-first-glance-visible permissions. 
>>> Maybe I can do thsis tomorrow? I
>>> wil enjoy it :)
>>
>> Go ahead.
>>
>>>
>>>
>>> +    public static Permission getPermission(final 
>>> PolicyEditorPermissions editorPermission) {
>>>
>>> arrgh thats monster..... Why reflection!!?!??! No way!!!  it is our 
>>> class. Please do something
>>> public or whatever...
>>>
>>>
>>>
>>> Otherwise it is not os monsterous :)
>>
>> Well, I had to decide if I wanted to extend PolicyEditorPermissions 
>> with some way to convert into
>> Permissions, or to just add this translation layer separately. I 
>> liked the new layer better, since
>> PolicyEditorPermissions was really just intended for use within 
>> PolicyEditor, and in there we don't
>> need this kind of logic. So from outside of the model itself, 
>> reflection was actually not a bad way
>> to handle acquiring the correct kind of Permission subclass from the 
>> model - the alternative would
>> have been a switch or long if/else chain, which would have then meant 
>> that any new PermissionTypes
>> would have also needed an update to the TemporaryPermissions 
>> converter. Using reflection like this
>> means that it never has to be updated if the PermissionTypes change. 
>> I don't think this is even that
>> bad of a use of reflection - I don't think it's too hard to read, and 
>> it's not being used somewhere
>> performance critical or where it will be called very frequently.
>>
>> The other good option I've thought of is to add the Permission 
>> conversion logic to the
>> PolicyEditorPermissions model, and move/rename this model so it's 
>> more suitable as something shared
>> between PolicyEditor and these dialogs. The PolicyEditorPermissions 
>> could be constructed with a
>> Class reference, eg RuntimePermission.class, and have a Permission 
>> field that is then populated by
>> the constructor doing a bit less reflection. This eliminates the need 
>> for the Class.forName call,
>> but makes the PolicyEditorPermission model more complicated in a way 
>> that I don't think it really
>> needs to be.
>>
>
> hmm All three ways looks bad:( Both reflection and "any new 
> PermissionTypes  would have " is  very wrong.
>
>
> No advice here :( Sorry. But I belive you will figure it out O:)
>

As you said you preferred on IRC, here's the way where each temporary 
permission is just newly defined, rather than being directly converted 
from PolicyEditorPermissions. I do reuse what I can from them, just to 
eliminate as much extra hardcoding of Strings as possible, but the new 
Permissions are all defined manually. It took a while to figure out a 
way to make this look clean and be easy to extend in the future, as well 
as to be able to nicely define permissions as reductions from the All 
Permissions set, but I think I've managed it.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: temporary-permissions-3.patch
Type: text/x-patch
Size: 42991 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140326/fe2b6f52/temporary-permissions-3-0001.patch>