[rfc][icedtea-web] PolicyEditor gains a real parser

[Andrew Azores]

Hi,

On 01/29/2015 06:23 AM, Jiri Vanek wrote:
> I was not reading whole thread (lazyyY!)
>
> So maybe those were already answered:
>
> Why OPTIONS.CODEBASE disapeared??? I can not see its replacement.

Before this patch, PolicyEditor's internal model assumes that codebases 
can be used as unique identifiers of sorts - each entry in a policy file 
always has exactly zero or one codebases, and does not have principals, 
or signedby. With this patch, the internal model becomes full-featured, 
like upstream policytool's, and PolicyEditor can actually handle policy 
entries with any combination of codebase/signedby/principal, just like 
policytool. The PolicyEditor GUI just doesn't have a way to display them 
(yet). So a -codebase selector is no longer guaranteed to return exactly 
one (or none) result, and I don't know what the expected behaviour would 
be if there were multiple matches in the policy file, especially when 
some of them may be "hidden".

>
> Also the patch do not apply   the  -        FILE("-file", "policy_file",
> "PBOFile"), and CODEBASE("-codebase", "url", "PBOCodebase"); definitions
> changed.

Yes, I'll make that change this weekend I hope.

>
> The code itself is pretty god - especially via using the
> import sun.security.provider.PolicyParser;
>
> *however*
>
> http://mail.openjdk.java.net/pipermail/awt-dev/2014-June/008072.html
> "In JDK-9 we would have modules and you will not be able to use the
> sun.* packages at all, so this changeset is useless for JDK9. "
>
> Makes me worrying about this approach. Especially also because of
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-October/029989.html
>
>
> So if we are going to use it, it sholdbe changed to public api. So you
> should have some adapter, which will allow simply switch between old and
> new api. And ofcourse, somebody have to negotiate it with upstream.. oh
> dear....

Yea, this sounds problematic. I have no idea how to move forward with 
this kind of forward compatibility in mind, honestly.

>
> Also, I.m aware of
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-November/030101.html
> thread. It died without push. Or am I wrong?
>

Not wrong, it did die without push, but only because it was around that 
time that I discovered these sun parser bits, and found out that the 
PolicyIdentifier I was making there could be easily adapted and made 
much more useful by doing some rewrites and using the sun stuff, and 
then this whole patch happened. So really, that old patch just became 
this one :)

>
>
>
> To code:
>
>
>   Why DisplayablePermission isnt regualr class?

You mean why is it an inner static class rather than its own top level? 
Mostly just because the only context I intend it to be used it is within 
the Custom Policy Viewer. Its only reason to exist is because the Sun 
PolicyParser.PermissionEntry doesn't implement toString nicely, really, 
but I need the CustomPolicyViewer to be able to display its permissions, 
obviously. It seemed nicer to subclass the class I actually wanted to 
use (PermissionEntry) and have this implement toString than to make all 
my CustomPolicyViewer Swing stuff use custom adapters to render the 
collection of permission entries.

>
> the
>
> +    /**
> +     * PolicyEditor is only designed to deal with grant entries with a
> single codeBase -
> +     * no signedBy and no principals. Any "identifiers" attached to
> grant entries which
> +     * do not strictly match this will be ignored and not displayed in
> the UI.
> +     */
> +    static boolean isCodeBaseIdentifier(final PolicyIdentifier
> identifier) {
> +        return (identifier.getSignedBy() == null ||
> identifier.getSignedBy().isEmpty())
> +                && (identifier.getPrincipals() == null ||
> identifier.getPrincipals().isEmpty())
> +                && identifier.getCodebase() != null;
>
>
> MEans, that all other policies will be unvisible, but not lost.
>
> What about showing them at least read only? Aka as plain text or
> something like it... It wouldbe suspicious if they are "just not there"
>
>

Yes, I've been intending to work on this somehow but have not yet had 
time. Do you have any suggestions on how to display them read-only? I 
was thinking maybe (temporarily, until somebody makes the PolicyEditor 
GUI properly handle the full permission entry feature set) just have 
another child window like the CustomPolicyViewer... or maybe just have 
them displayed in the CustomPolicyViewer itself, honestly. What do you 
think?

>
> Why
>   +public enum PolicyEditorPermissions implements Serializable {
> is now serializable?
> +public class PolicyEntry implements Serializable, Transferable {
> again.. why so?
> Because of: public static final DataFlavor DATA_FLAVOR
> What is it for? For clipboard actions only?

Clipboard actions and also dragging actions. I think the Serializable 
and Transferable all come from that, but it's been so long at this point 
that I can't remember for sure. I would need to go back and do some API 
diving probably :) again, I'm hoping to have some time to work on this 
this weekend, so I'll take note to look into why this is.

>
> MAybe rneame the DATA_FLAVOR o POLICY_ENTRY_DATA_FLAVOR or similar.
>

Sure thing.

>
> Why   public static class PolicyEntryBuilder { is initernal class?
>

Well, it's a Builder, this one *definitely* doesn't have any good 
context as a top-level IMO :)

>
> May you please higligh usage of import
> sun.security.provider.PolicyParser; ?
>

What do you mean? Just put a comment next to it I suppose?

>
> The net.sourceforge.jnlp.security.policyeditor package have grown a lot.
> Maybe separate it to data/gui/rest/ ? Can be doen as another (following)
> changeset.
>
>
>
> J.

Sure, as a separate changeset.

Thanks,
-- 
Andrew Azores