[rfc][icedtea-web] Permissions manifest attribute fix

[Omair Majid]

* Andrew Azores <aazores at redhat.com> [2014-03-31 16:35]:
> On 03/31/2014 04:10 PM, Andrew Azores wrote:
> >On 03/31/2014 01:28 PM, Andrew Azores wrote:
> >Small refactor. Rather than the new RequestedPermissionLevel being
> >available from SecurityDesc and PluginBridge only, it's also
> >available from JNLPFile. PluginBridge, being a JNLPFile subclass,
> >then overrides the method and provides the correct implementation
> >for HTML applets. This just makes things more coherent IMO.
> >
> >Thanks,
> >
> 
> And another, as discussed with Omair on IRC. Just extracted the
> common checks for HTML and JNLP into a new method and call this
> method once, before the split check for HTML/JNLP divergence. So
> long as the spec doesn't diverge the two any further, then this
> should be okay. Also remove an unnecessary typecast (made
> unnecessary in the last refactor due to new method and override, but
> forgot to fix).

I am far from an expert on this code, so please take this 'review' with
a grain of salt.

Also, I am a little hesitant about such (potentially) invasive code
going in at the last minute.

> +++ b/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java

> +        final RequestedPermissionLevel requested = file.getRequestedPermissionLevel();
> +        validateRequestedPermissionLevelMatchesManifestPermissions(requested, permissions);
> +        if (file instanceof PluginBridge) { // HTML applet
> +            if (requested == RequestedPermissionLevel.NONE) {
> +                if (permissions == ManifestBoolean.TRUE && signing != SigningState.NONE) {
> +                    // http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#permissions
> +                    // FIXME: attempting to follow the spec, but it is too late now to actually set the applet
> +                    // to run in sandbox, so the applet will not be run at all, rather than run sandboxed!
> +                    //securityDelegate.setRunInSandbox();

I read this comment a few times and I couldn't figure out (without
asking you) how it relates to the current state.

The "applet will not be run at all" bit completely threw me off. I read
this comment as: we can not set the sandbox state now (for some reason)
so by leaving this commented out, the applet will not run at all. Can
you clarify that not calling sandbox will let it run without any issues?

> +            if (requested == RequestedPermissionLevel.NONE) {
> +                if (permissions == ManifestBoolean.TRUE && signing != SigningState.NONE) {

Maybe call this variable `sandbox` rather than `permissions` to clarify
that TRUE means sandbox?

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681