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

[Andrew Azores]

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.
>>
>


-- 
Thanks,

Andrew Azores

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