[rfc][icedtea-web] Temporary permissions

[Jiri Vanek]

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:)