[rfc][icedtea-web] PolicyEditor dies on invalid CLI arguments

[Andrew Azores]

On 17/08/15 02:29 PM, Andrew Azores wrote:
> On 17/08/15 04:41 AM, Jiri Vanek wrote:
>> On 07/31/2015 12:19 AM, Andrew Azores wrote:
>>> Hi,
>>>
>>> This patch makes it so that PolicyEditor dies immediately if given 
>>> invalid arguments via -codebase,
>>> -signedby, or -principals CLI switches.
>>>
>>> -- 
>>> Thanks,
>>>
>>> Andrew Azores
>>>
>>>
>>> exception-on-invalid-selector.patch
>>>
>>>
>>> # HG changeset patch
>>> # User Andrew Azores<aazores at redhat.com>
>>> # Date 1438294716 14400
>>> #      Thu Jul 30 18:18:36 2015 -0400
>>> # Node ID 5f4260d54edaed95ece92ba7934f003275250919
>>> # Parent  09792ebffb27b328f47e5ab5f5e075f2c2fe0a18
>>> PolicyEditor dies when given invalid -codebase or -principals arguments
>>>
>>> * netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> (main): extracted helper method for getting codebase argument. Throw
>>> IllegalArgumentException when given invalid arguments to selector 
>>> switches
>>> (getCodebaseArgument, getSignedByArgument, getPrincipalsArgument): new
>>> methods
>>> (cleanFilePathArgument): make parameter final for consistent style
>>> * 
>>> tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorTest.java
>>> (testGetCodebaseArgument, testGetCodebaseArgument2,
>>> testGetCodebaseArgument3, testGetCodebaseArgumentWhenNotProvided,
>>> testGetPrincipalsArgument, testGetPrincipalsArgument2,
>>> testGetPrincipalsArgument3, testGetPrincipalsArgumentWhenNotProvided,
>>> testGetSignedByArgument, testGetSignedByArgumentWhenNotProvided): 
>>> new tests
>>>
>>> diff --git a/ChangeLog b/ChangeLog
>>> --- a/ChangeLog
>>> +++ b/ChangeLog
>>> @@ -1,3 +1,19 @@
>>> +2015-07-30  Andrew Azores<aazores at redhat.com>
>>> +
>>> +    PolicyEditor dies when given invalid -codebase or -principals 
>>> arguments
>>> +    * 
>>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> +    (main): extracted helper method for getting codebase argument. 
>>> Throw
>>> +    IllegalArgumentException when given invalid arguments to 
>>> selector switches
>>> +    (getCodebaseArgument, getSignedByArgument, 
>>> getPrincipalsArgument): new
>>> +    methods
>>> +    (cleanFilePathArgument): make parameter final for consistent style
>>> +    * 
>>> tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorTest.java
>>> +    (testGetCodebaseArgument, testGetCodebaseArgument2,
>>> +    testGetCodebaseArgument3, testGetCodebaseArgumentWhenNotProvided,
>>> +    testGetPrincipalsArgument, testGetPrincipalsArgument2,
>>> +    testGetPrincipalsArgument3, 
>>> testGetPrincipalsArgumentWhenNotProvided,
>>> +    testGetSignedByArgument, 
>>> testGetSignedByArgumentWhenNotProvided): new tests
>>> +
>>>   2015-07-30  Andrew Azores<aazores at redhat.com>
>>>
>>>       Add tests for PolicyEditor.getFilePathArgument
>>> diff --git 
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java 
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> --- a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> @@ -1759,18 +1759,16 @@
>>>               // not really important, so just ignore
>>>           }
>>>
>>> +
>>> +        final String filepath = getFilePathArgument(optionParser);
>>> +        final String codebase = getCodebaseArgument(optionParser);
>>> +        final String signedBy = getSignedByArgument(optionParser);
>>> +        final Set<PolicyParser.PrincipalEntry> principals = 
>>> getPrincipalsArgument(optionParser);
>>> +
>>>           SwingUtilities.invokeLater(new Runnable() {
>>>               @Override
>>>               public void run() {
>>> -                final String filepath = 
>>> getFilePathArgument(optionParser);
>>>                   final PolicyEditorWindow frame = 
>>> getPolicyEditorFrame(filepath);
>>> -                final String codebase = 
>>> optionParser.getParam(OptionsDefinitions.OPTIONS.CODEBASE);
>>> -                final String signedBy = 
>>> optionParser.getParam(OptionsDefinitions.OPTIONS.SIGNEDBY);
>>> -                final List<String> rawPrincipals = 
>>> optionParser.getParams(OptionsDefinitions.OPTIONS.PRINCIPALS);
>>> -                final Set<PolicyParser.PrincipalEntry> principals = 
>>> new HashSet<>();
>>> -                for (int i = 0; i < rawPrincipals.size(); i+= 2) {
>>> -                    principals.add(new 
>>> PolicyParser.PrincipalEntry(rawPrincipals.get(i), 
>>> rawPrincipals.get(i + 1)));
>>> -                }
>>> frame.getPolicyEditor().openPolicyFileSynchronously();
>>>                   frame.getPolicyEditor().addNewEntry(new 
>>> PolicyIdentifier(signedBy, principals, codebase));
>>>                   frame.asWindow().setVisible(true);
>>> @@ -1778,7 +1776,50 @@
>>>           });
>>>       }
>>>
>>> -    static String getFilePathArgument(OptionParser optionParser) {
>>> +    static String getCodebaseArgument(final OptionParser 
>>> optionParser) {
>>> +        if 
>>> (optionParser.hasOption(OptionsDefinitions.OPTIONS.CODEBASE)) {
>>> +            final String codebase = 
>>> optionParser.getParam(OptionsDefinitions.OPTIONS.CODEBASE);
>>> +            try {
>>> +                new URL(codebase);
>>> +            } catch (final MalformedURLException e) {
>>> +                throw new IllegalArgumentException("Invalid URL", e);
>>> +            }
>>> +            return codebase;
>>> +        } else {
>>> +            return null;
>>> +        }
>>> +    }
>>> +
>>> +    static String getSignedByArgument(final OptionParser 
>>> optionParser) {
>>> +        if 
>>> (optionParser.hasOption(OptionsDefinitions.OPTIONS.SIGNEDBY)) {
>>> +            final String signedBy = 
>>> optionParser.getParam(OptionsDefinitions.OPTIONS.SIGNEDBY);
>>> +            if (signedBy.isEmpty()) {
>>> +                throw new IllegalArgumentException("SignedBy cannot 
>>> be empty");
>>> +            } else {
>>> +                return signedBy;
>>> +            }
>>> +        } else {
>>> +            return null;
>>> +        }
>>> +    }
>>> +
>>> +    static Set<PolicyParser.PrincipalEntry> 
>>> getPrincipalsArgument(final OptionParser optionParser) {
>>> +        if 
>>> (optionParser.hasOption(OptionsDefinitions.OPTIONS.PRINCIPALS)) {
>>> +            final List<String> rawPrincipals = 
>>> optionParser.getParams(OptionsDefinitions.OPTIONS.PRINCIPALS);
>>> +            if (rawPrincipals.size() % 2 != 0) {
>>> +                throw new IllegalArgumentException("List of 
>>> principals must have an even number of elements: " + rawPrincipals);
>>
>> This does not seems right.p
>> ITs deffined as:
>> PRINCIPALS("-principals", "class_name principal_name", 
>> "PBOPrincipals", NumberOfArguments.EVEN_NUMBER_SUPPORTS_EQUALS_CHAR);
>>
>> So even should be already thrown.
>
> Yes, the OptionParser does actually throw an UnevenParameterException 
> in this case (which 
> PolicyEditorTest.testGetPrincipalsArgumentWhenUnevenArgumentsProvided 
> actually expects), but I added the check there as well just as a 
> defensive measure.
>
>> Also the test for case like
>>         String[] args = new String[] { "-principals", "aa", "bb" };
>> is missing.
>>
> Ah good catch, thank you. Added.
>
>> Thanx!
>>   J.
>>>
>>
>
>

Sorry, this is the correct patch.

-- 
Thanks,

Andrew Azores

-------------- next part --------------
A non-text attachment was scrubbed...
Name: exception-on-invalid-selector-2.patch
Type: text/x-patch
Size: 11397 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150817/5180e3ed/exception-on-invalid-selector-2-0001.patch>