[rfc][icedtea-web] Temporary permissions

[Andrew Azores]

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.

Thanks,

-- 
Andrew A