RFR 8071660 :URLPermission not handling empty method lists correctly

[Chris Hegarty]

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 ) ?


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