[rfc][icedtea-web] Unsigned applet security dialog abstraction

[Jiri Vanek]

On 02/27/2014 09:50 PM, Andrew Azores wrote:
> Hi,
>
> Three patches here. One is just unit tests. The other two need to be applied together to be
> buildable, but are split because one is actually the real work patch, and the other is a lot of
> noisy renaming.
>
> The goal is to make the Unsigned Applet warning dialog, which is really quite nice, into an abstract
> class, so that new dialogs can be created for similar purposes, but with different information
> displayed. Namely, a new Partially Signed dialog, making use of the same layout and logic, as well
> as the same "remember decision" store, etc.
>
> "abstractions.patch" actually does this work of pulling most of the dialog out into an abstract
> parent class, and making the existing class into a subclass of this one in such a way that the
> look/feel and functionality are all preserved. "renames.patch" just renames a lot of classes that
> are related to this dialog, and whose names are no longer very accurate after the abstraction is
> done. "abstractions.patch" does contain some such renaming, however, it is much more limited.
> "tests.patch" adds unit tests intended to be able to test any such subclasses, although each new
> subclass needs to be manually added to the test case setup.
>
> This is in preparation for a "new-dialog.patch" which will introduce a new Partially Signed Dialog,
> to replace the existing Not All Signed Dialog, using the abstract parent class pulled out of the
> Unsigned Applet Dialog.
>
> ChangeLog:
> UnsignedAppletTrustWarningPanel logic moved into new abstract parent class
> AppTrustWarningPanel for reusability.
> * netx/net/sourceforge/jnlp/security/AppTrustWarningDialog.java: new class
> * netx/net/sourceforge/jnlp/security/AppTrustWarningPanel.java: new class
> * netx/net/sourceforge/jnlp/security/UnsignedAppletTrustWarningPanel.java: major refactor into
> subclass of AppTrustWarningPanel
> * netx/net/sourceforge/jnlp/security/SecurityDialogs.java: (UnsignedWarningAction) references
> changed to AppSigningWarningAction
> * netx/net/sourceforge/jnlp/security/UnsignedAppletTrustWarningDialog.java: same
> * tests/netx/unit/net/sourceforge/jnlp/security/AppTrustWarningPanelTest.java: new tests for
> AppTrustWarningPanel
> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/ExecuteUnsignedApplet.java: renamed,
> changed all references
> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/ExecuteAppletAction.java:
> (ExecuteUnsignedApplet) renamed to this
> * netx/net/sourceforge/jnlp/controlpanel/UnsignedAppletActionTableModel.java: (ExecuteAppletAction)
> changed references
> * netx/net/sourceforge/jnlp/controlpanel/UnsignedAppletsTrustingListPanel.java:
> (ExecuteAppletAction) changed references
> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletActionEntry.java:
> (ExecuteAppletAction) changed references
> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletTrustConfirmation.java:
> (ExecuteAppletAction) changed references
> *
> netx/net/sourceforge/jnlp/security/appletextendedsecurity/impl/UnsignedAppletActionStorageExtendedImpl.java:
> (ExecuteAppletAction) changed references
> *
> netx/net/sourceforge/jnlp/security/appletextendedsecurity/impl/UnsignedAppletActionStorageImpl.java:
> (ExecuteAppletAction) changed references
>
>
> Thanks,
>


hi!

Really than you for splitting the patch to  refactroing and actual work. It really makes review 
thousand times easier. Also thank you to extracting it from original patch, yes, those are really 
two different things. Thank you for helping reviewer to help :)
Just note - when refracting patch is posted, its better to use --git format, it strip the patch of 
content of moved classes.
The renaming patch is ok to go, unless you wont to rename it to general ExecuteAction (see bottom of 
email)

Code itself looks good. Only nit:

+        } catch (Exception e) {
+            e.printStackTrace();
+            throw e;
+        }

Whats that? print and rethrow:-) why? only one of it should be enough or not. Anyway, print stack 
trace is forbidden. Use ServerAccess.log* rather.


One general comment to idea itself. I was thinking about reusing this dialogue for 
http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#app_library (And 
maybe some more ) but it forces few more generalizations:
  - it should depend on jnlpfile, not on plugin bridge. (But this should not prevent the dialogue 
from some cations if it is instance of plugin bridge)\
  - it should use also  title of application
  - the help have to be adapted
  (maybe some acces to disable/enable some controls or similar..)

 From the quick glance those should be not hard to achieve. Do you wont to include it in this 
refactoring, or later? Eg when I will use this. Probably something now, something later on demand...

Also in sound of those refactoings, I'm thinking if it is correct to use ExtendedAppletsSecurity 
table as it is now.
  - you wont to reuse it for partially signed yes applets yes?
  - I wont to use it for Application-Library-Allowable-Codebase
  - currently it is used for unsigned applet wont to run

Your usecase is probably ok, and with some compromising it can reuse current table. But definitely 
not the my one:(

So I was thinking about extending the format for some flag "purpose" (rather then  to have new table)
eg:
from current resolution timestamp url-regex jars:
y 1393582488549 \Qhttp://www.walter-fendt.de/ph14e/forceresol.htm\E 
\Qhttp://www.walter-fendt.de/ph14_jar/\E Ph14English.jar,ZerlKraft.jar
to
y ACACA 1393582488549 \Qhttp://www.walter-fendt.de/ph14e/forceresol.htm\E 
\Qhttp://www.walter-fendt.de/ph14_jar/\E Ph14English.jar,ZerlKraft.jar
or
y UNSIGNED 1393582488549 \Qhttp://www.walter-fendt.de/ph14e/forceresol.htm\E 
\Qhttp://www.walter-fendt.de/ph14_jar/\E Ph14English.jar,ZerlKraft.jar
or
y PARTIALLY 1393582488549 \Qhttp://www.walter-fendt.de/ph14e/forceresol.htm\E 
\Qhttp://www.walter-fendt.de/ph14_jar/\E Ph14English.jar,ZerlKraft.jar

so:
resolution purpose timestamp url-regex jars
or similar,


What do you think?





J.