RFR 8071660 :URLPermission not handling empty method lists correctly

[Chris Hegarty]

On 21 Jun 2016, at 11:21, Chris Hegarty <chris.hegarty at oracle.com> wrote:
> 
> The code changes look fine, but the bigger question is whether we 
> want to support actions without any ‘method’ being specified,  like
> “:X-Bar”? Should the constructor throw IAE, or is that even possible
> now, it would require a spec clarification ( would that invalidate existing
> code already doing this ) ?

Seems like a corner case and not worth trying to contort the spec to
cover it. Such a permission is effectively useless anyway.

I’ll sponsor this change for you.

-Chris.

> 
> -Chris.
> 
>> On 21 Jun 2016, at 11:16, Vyom Tewari <vyom.tewari at oracle.com> wrote:
>> 
>> Hi Pavel,
>> 
>> Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.4/index.html <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.4/index.html>). I had incorporated the review comments.
>> 
>> Thanks,
>> Vyom
>> 
>> 
>> On Monday 20 June 2016 07:27 PM, Pavel Rappo wrote:
>>> Vyom, please consider the following changes:
>>> 
>>> 1. Append 8071660 to the lists of tests here:
>>> 
>>> 	* @bug 8010464 8027570 8027687 8029354 8071660
>>>                                               ^
>>> 2. Reformat the code the way it's in the rest of the file:
>>> 
>>> from:
>>> 
>>> 266         URLPermission that = (URLPermission)p;
>>> 267
>>> 268         if(this.methods.isEmpty() && !that.methods.isEmpty())
>>> 269             return false;
>>> 270
>>> 271         if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") &&
>>> 272                 Collections.indexOfSubList(this.methods, that.methods) == -1) {
>>> 273             return false;
>>> 274         }
>>> 
>>> to:
>>> 
>>> 266         URLPermission that = (URLPermission)p;
>>> 267
>>> 268         if (this.methods.isEmpty() && !that.methods.isEmpty()) {
>>> 269             return false;
>>> 270         }
>>> 271
>>> 272         if (!this.methods.isEmpty() &&
>>> 273             !this.methods.get(0).equals("*") &&
>>> 274             Collections.indexOfSubList(this.methods,
>>> 275                                        that.methods) == -1) {
>>> 276             return false;
>>> 277         }
>>> 
>>> Other than these minor things, looks good.
>>> 
>>> Thanks,
>>> -Pavel
>>> 
>>>> On 20 Jun 2016, at 12:36, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>>> 
>>>> On 17/06/16 15:09, Vyom Tewari wrote:
>>>>> Hi,
>>>>> 
>>>>> Please find the latest
>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html
>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.3/index.html>). I
>>>>> fixed the typo along with spaces issue.
>>>> Looks good!
>>>> 
>>>> -- daniel
>>>> 
>>>>> Thanks,
>>>>> Vyom
>>>>> 
>>>>> 
>>>>> On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:
>>>>>> Hi Vyom,
>>>>>> 
>>>>>> URLPermissionTest.java:
>>>>>> 
>>>>>> line 125:  it looks odd to assign the same variable twice with the
>>>>>> same value.
>>>>>>  Perhaps it should have been assigning this.url2 = url2.
>>>>>> 
>>>>>> line 330-332: please add a space after "," in argument lists.
>>>>>> 
>>>>>> Roger
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 6/17/2016 8:42 AM, Daniel Fuchs wrote:
>>>>>>> On 17/06/16 13:25, Vyom Tewari wrote:
>>>>>>>> Hi Daniel,
>>>>>>>> 
>>>>>>>> thanks for review please find the latest
>>>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.2/index.html>) .
>>>>>>>> I added some more tests where base url is different.
>>>>>>> Thanks Vyom,
>>>>>>> Looks good to me.
>>>>>>> 
>>>>>>> See if you can get someone more experienced in the area
>>>>>>> to give a thumb up too :-)
>>>>>>> 
>>>>>>> best regards,
>>>>>>> 
>>>>>>> -- daniel
>>>>>>> 
>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Vyom
>>>>>>>> 
>>>>>>>> On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:
>>>>>>>>> On 17/06/16 09:21, Vyom Tewari wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>> 
>>>>>>>>>> Please find the new
>>>>>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html
>>>>>>>>>> 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.1/index.html>).
>>>>>>>>>> 
>>>>>>>>>> I  addressed the review comments given by Daniel.
>>>>>>>>> Hi Vyom,
>>>>>>>>> 
>>>>>>>>> Looks good to me - but I'm a bit concerned that the previous
>>>>>>>>> mistake was not caught that by any test.
>>>>>>>>> Could you add a test that fails with you previous fix
>>>>>>>>> but passes with the new one?
>>>>>>>>> 
>>>>>>>>> best regards,
>>>>>>>>> 
>>>>>>>>> -- daniel
>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> Vyom
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:
>>>>>>>>>>> Hi Vyom,
>>>>>>>>>>> 
>>>>>>>>>>> This looks strange to me:
>>>>>>>>>>> 
>>>>>>>>>>> 268         if(!this.methods.isEmpty() && that.methods.isEmpty())
>>>>>>>>>>> 269             return true;
>>>>>>>>>>> 270         if(this.methods.isEmpty() && !that.methods.isEmpty())
>>>>>>>>>>> 271             return false;
>>>>>>>>>>> 272         if(this.methods.isEmpty() && that.methods.isEmpty())
>>>>>>>>>>> 273             return true;
>>>>>>>>>>> 
>>>>>>>>>>> Namely, lines 269 & 273 will return true before the URL part
>>>>>>>>>>> of the permission has been checked.
>>>>>>>>>>> Is that really the expected behavior?
>>>>>>>>>>> 
>>>>>>>>>>> best regards,
>>>>>>>>>>> 
>>>>>>>>>>> -- daniel
>>>>>>>>>>> 
>>>>>>>>>>> On 11/06/16 05:50, vyom wrote:
>>>>>>>>>>>> Hi All,
>>>>>>>>>>>> 
>>>>>>>>>>>> Please review the below fix.
>>>>>>>>>>>> 
>>>>>>>>>>>> Bug       :         JDK-8071660 URLPermission not handling empty
>>>>>>>>>>>> method
>>>>>>>>>>>> lists correctly
>>>>>>>>>>>> Webrev :
>>>>>>>>>>>> http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.0/index.html>
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Vyom
>> 
>