[rfc][icedtea-web] (PR1264) Run in Sandbox button

[Andrew Azores]

On 02/26/2014 01:02 PM, Omair Majid wrote:
> JCV was designed in a layered approach (JNLPClassLoader calls JCV and
> not the other way aroun) so we would be able to unit test it completely
> and sanely without having to instantiate JNLPClassLoader. This change
> breaks that :(
>
> Thanks,
> Omair
>
>
> The JNLPClassLoader class is hairy enough and this is making it more
> complex. Would it be possible to extract all the
> security-decision-making code into a new class that JNLPClassLoader can
> delegate to, instead?
>
> I mean it is probably okay to have conditionals based on
> getRunInSandbox() this time, but one more conditional like this and the
> code will be next to impossible to maintain.

SecurityDelegate (interface) and SecurityDelegateImpl handle this now. I 
kept them both inside JNLPClassLoader because they don't really make any 
sense outside of that context. I specifically made the interface so that 
for testing other components, eg the JCV, a "faked" implementation could 
be made that doesn't actually require a JNLPClassLoader instance, which 
SecurityDialogImpl does.

I would've liked to add tests for SecurityDialogImpl, but the logic it 
encapsulates was not really separately testable before being extracted, 
and now that it has been extracted and can be tested, it requires a 
JNLPClassLoader mock object first :/ I've run all reproducers with and 
without the delegate though and the results looked the same, and also 
did some manual testing, and it seems to work fine.

> I would create a method named `isUserAlreadyPrompted()` and use that
> instead of `loader.getRunInSandbox()`
>
> All in all, I am quite happy how this patch is not very invasive, given
> that it touches one of the more sensitive classes in the codebase.
>
> Thanks,
> Omair

I added this to SecurityDelegate too.

These patches only really differ from the previous ones by 
SecurityDelegate. I'm not even sure 'delegate' is quite the right term 
for what this is - close enough? It doesn't really simplify any of the 
security decision logic, but it at least consolidates it into one place. 
There are probably other things that can be moved into the 
SecurityDelegate, but for now I've just moved stuff that I've personally 
worked on, in particular the Run In Sandbox stuff, of course.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dialog_with_delegate.patch
Type: text/x-patch
Size: 13340 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140228/c3ed72b5/dialog_with_delegate-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implementation_with_delegate.patch
Type: text/x-patch
Size: 19764 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140228/c3ed72b5/implementation_with_delegate-0001.patch