[rfc][icedtea-web] Temporary Permissions generated via reflection (re-proposal)

[Deepak Bhole]

* Andrew Azores <aazores at redhat.com> [2014-06-13 16:41]:
> On 06/13/2014 04:26 PM, Deepak Bhole wrote:
> >* Andrew Azores <aazores at redhat.com> [2014-06-13 11:19]:
> >>On 06/13/2014 11:09 AM, Andrew Azores wrote:
> >>>Hi,
> >>>
> >>>When Temporary Permissions on the security dialogs were first introduced,
> >>>I proposed to have them generated from the PolicyEditorPermissions
> >>>dynamically using reflection, so that "grantable" permissions were only
> >>>defined in one place and so maintenance would be easy, and the
> >>>TemporaryPermissions class could be kept small. This was rejected due to
> >>>the required use of reflection, but I am re-proposing this patch because I
> >>>do believe it is a much cleaner solution, especially now that the number
> >>>of grantable permissions has grown and the temporary permissions menu has
> >>>become a multi-select UI rather than preset groups.
> >>>
> >>>ChangeLog:
> >>>    TemporaryPermissions generated via reflection rather than hard coded
> >>>    * netx/net/sourceforge/jnlp/resources/Messages.properties
> >>>    (STempPermNoFile, STempPermNoNetwork, STempPermNoExec,
> >>>    STempNoFileOrNetwork, STempNoExecOrNetwork, STempNoFileOrExec,
> >>>    STempNoFileOrNetworkOrExec, STempAllMedia, STempSoundOnly,
> >>>    STempClipboardOnly, STempPrintOnly, STempAllFileAndPropertyAccess,
> >>>    STempReadLocalFilesAndProperties, STempReflectionOnly): removed
> >>>    (STempNetwork, STempReadFilesAndProperties,
> >>>STempWriteFilesAndProperties,
> >>>    STempReflectionAndExternal, STempAllMedia): new messages
> >>>    * netx/net/sourceforge/jnlp/resources/Messages_cs.properties
> >>>    (STempPermNoFile, STempPermNoNetwork, STempPermNoExec,
> >>>    STempNoFileOrNetwork, STempNoExecOrNetwork, STempNoFileOrExec,
> >>>    STempNoFileOrNetworkOrExec, STempAllMedia, STempSoundOnly,
> >>>    STempClipboardOnly, STempPrintOnly, STempAllFileAndPropertyAccess,
> >>>    STempReadLocalFilesAndProperties, STempReflectionOnly): removed
> >>>    *
> >>>netx/net/sourceforge/jnlp/security/dialogs/TemporaryPermissions.java:
> >>>    rewrite to use reflection to generate permissions from
> >>>    PolicyEditorPermissions rather than exposing statically defined values
> >>>    *
> >>>netx/net/sourceforge/jnlp/security/dialogs/TemporaryPermissionsButton.java:
> >>>    refactor to use new TemporaryPermissions change
> >>>    * tests/netx/unit/net/sourceforge/jnlp/security/dialogs/TemporaryPermissionsTest.java:
> >>>    new tests for TemporaryPermissions
> >>>
> >>>
> >>>Thanks,
> >>>
> >>Lightning-quick update thanks to Jie Kang's keen eye: now using multi-catch
> >>in TemporaryPermissions.getPermission. Semantically the same and no other
> >>changes.
> >>
> >Hi Andrew,
> >
> >I agree, reflection is preferable over an ever growing list of static
> >permission objects.
> >
> >Right now the code seems to be boxed such that the reflected object
> >creation is always done dynamically (via the new getPermission method).
> >Is there is a chance that this could later be utilized with user input?
> >
> >In its current static state it is fine, but my concern is that there is
> >no arg sanitization when we try to create permission objects with the
> >given string args in getPermission. In addition to potential bugs, there
> >could also be security implications caused by incorrect permission
> >creation/granting.
> >
> >Rest looks fine to me.
> >
> >Cheers,
> >Deepak
> 
> I don't anticipate this particular code being used to accept user input,
> although I suppose it would be possible. For me at least the purpose of
> these temporary permissions is just to give users a faster way to quickly
> test applets with various restricted permission configurations, rather than
> having to use PolicyEditor or policytool, which is a bit more involved. If
> the user needs a custom-defined permission, then PolicyEditor allows this
> through CustomPolicyViewer.
> 
> As it is, the PolicyEditorPermissions argument is pretty restrictive
> already. It's an enum with two String fields providing simple human-readable
> descriptions, and three more enum fields that model the permission type (eg
> java.io.FilePermission), target (eg "${user.home}"), and actions (eg
> "read"). So there isn't any actual sanitization on the input, but it's just
> because the input is restricted to come from these pre-defined enum values
> anyway.
> 

Ah okay, fair enough. Thanks for clearing it up.

Assuming you have tested this, approved for HEAD.

Deepak

> Thanks,
> 
> -- 
> Andrew A
>