Re: Code review: 8010464: Evolve java networking same origin policy

[Alan Bateman]

On 15/05/2013 12:46, Michael McMahon wrote:


updated here:

http://cr.openjdk.java.net/~michaelm/8010464/webrev.3/

Michael.


This looks much better.

In HttpURLConnection then odd formatting in the declaration of 
followRedirect0 (which probably needs a better name, maybe 
implFollowRedirect?) A couple of indentation issues that probably should 
be fixed too, L980 and L2951  for example.


I don't see any other issues.

-Alan

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

On 15/05/13 12:12, Michael McMahon wrote:

On 15/05/13 12:02, Alan Bateman wrote:

On 15/05/2013 11:34, Michael McMahon wrote:



...


I'll post one more webrev, and push it soon afterwards as I'd like 
to make the

code freeze today.

Okay.




updated here:

http://cr.openjdk.java.net/~michaelm/8010464/webrev.3/

Michael.

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

On 15/05/13 12:02, Alan Bateman wrote:

On 15/05/2013 11:34, Michael McMahon wrote:

:

On MessageHeader then getHeaderNamesInList could use 
java.util.StringJoiner to avoid rolling your own.




I can see the benefit of using StringJoiner (and a lambda) if I am 
starting off from a Collection
and this is something that only struck me when looking at this. I was 
surprised
to see that Arrays.asList() doesn't have a variant that limits the 
number of
elements coming from the array. So, I can't use it here. Maybe this 
is something

we could look at again later, with the other changes we're contemplating?
I think getHeaderNamesInList needs to be checked anyway as it looks to 
me that it just appends the keys and will never insert a comma.


The only reason I suggested StringJoiner here is because I thought 
this method was trying to do this:


StringJoiner joiner = new StringJoiner(",");
for (int i=0; i

well spotted and that's a simpler way of doing it. I messed the change 
up from the previous revision.
It's unfortunate the actual end to end HttpURLConnection tests only use 
one header in each

request. So, they didn't catch that. I'll update the test as well.

Thanks
Michael




I'll post one more webrev, and push it soon afterwards as I'd like to 
make the

code freeze today.

Okay.

Re: Code review: 8010464: Evolve java networking same origin policy

[Alan Bateman]

On 15/05/2013 11:34, Michael McMahon wrote:

:

On MessageHeader then getHeaderNamesInList could use 
java.util.StringJoiner to avoid rolling your own.




I can see the benefit of using StringJoiner (and a lambda) if I am 
starting off from a Collection
and this is something that only struck me when looking at this. I was 
surprised
to see that Arrays.asList() doesn't have a variant that limits the 
number of
elements coming from the array. So, I can't use it here. Maybe this is 
something

we could look at again later, with the other changes we're contemplating?
I think getHeaderNamesInList needs to be checked anyway as it looks to 
me that it just appends the keys and will never insert a comma.


The only reason I suggested StringJoiner here is because I thought this 
method was trying to do this:


StringJoiner joiner = new StringJoiner(",");
for (int i=0; i

I'll post one more webrev, and push it soon afterwards as I'd like to 
make the

code freeze today.

Okay.

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

On 15/05/13 11:04, Alan Bateman wrote:

On 14/05/2013 12:54, Michael McMahon wrote:

I have updated the webrev for this at:

http://cr.openjdk.java.net/~michaelm/8010464/webrev.2/

I took a pass over the updated  webrev and it mostly looks good to me.

In HttpURLConnection then I wonder if it would be better if 
getInputStream, getOutputStream and followRedirect set the connecting 
flag rather than URLtoSocketPermission. The side effect on the 
protocol handler state is a bit surprising in this method. 
Alternatively then maybe the method needs a better name.




Yes, that did looks a little incongruous. So, I think moving it to 
getInputStream, getOutputStream

and connect() would make sense.

On MessageHeader then getHeaderNamesInList could use 
java.util.StringJoiner to avoid rolling your own.




I can see the benefit of using StringJoiner (and a lambda) if I am 
starting off from a Collection
and this is something that only struck me when looking at this. I was 
surprised

to see that Arrays.asList() doesn't have a variant that limits the number of
elements coming from the array. So, I can't use it here. Maybe this is 
something

we could look at again later, with the other changes we're contemplating?


Are you planning to add @bug to the tests?



Yes, will do that.

I wonder if about dependency on SimpleSSLContext in the test as I 
don't think that test was originally intended to be a "test library".




SimpleSSLContext isn't a test. All it does is provide the glue to build 
an SSLContext.

I've used it as a kind of library before.

I'll post one more webrev, and push it soon afterwards as I'd like to 
make the

code freeze today.

Thanks
Michael

Re: Code review: 8010464: Evolve java networking same origin policy

[Alan Bateman]

On 14/05/2013 12:54, Michael McMahon wrote:

I have updated the webrev for this at:

http://cr.openjdk.java.net/~michaelm/8010464/webrev.2/

I took a pass over the updated  webrev and it mostly looks good to me.

In HttpURLConnection then I wonder if it would be better if 
getInputStream, getOutputStream and followRedirect set the connecting 
flag rather than URLtoSocketPermission. The side effect on the protocol 
handler state is a bit surprising in this method. Alternatively then 
maybe the method needs a better name.


On MessageHeader then getHeaderNamesInList could use 
java.util.StringJoiner to avoid rolling your own.


Are you planning to add @bug to the tests?

I wonder if about dependency on SimpleSSLContext in the test as I don't 
think that test was originally intended to be a "test library".


-Alan.

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

On 15/05/13 10:09, Chris Hegarty wrote:

What you have in the latest version of this webrev look fine to me.



Thanks Chris!

Michael


-Chris.

On 14/05/2013 12:54, Michael McMahon wrote:

I have updated the webrev for this at:

http://cr.openjdk.java.net/~michaelm/8010464/webrev.2/

Thanks
Michael.

Re: Code review: 8010464: Evolve java networking same origin policy

[Chris Hegarty]

What you have in the latest version of this webrev look fine to me.

-Chris.

On 14/05/2013 12:54, Michael McMahon wrote:

I have updated the webrev for this at:

http://cr.openjdk.java.net/~michaelm/8010464/webrev.2/

Thanks
Michael.

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

I have updated the webrev for this at:

http://cr.openjdk.java.net/~michaelm/8010464/webrev.2/

Thanks
Michael.

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

No, the class is used for both request and response headers.
Also, Arrays.asList() doesn't work in this case because we have to 
account for the number
of elements used. So, something like what you are suggesting below 
should work.


Michael

On 13/05/13 16:10, Vitaly Davidovich wrote:


If the headers are supposed to be parsed and then readonly, then the 
class can be made immutable? Don't know how much work that would entail.


To minimize synch overhead, I think you can assign the array and nkeys 
to locals inside a synch region and then do the copying outside of it.


Sent from my phone

On May 13, 2013 10:43 AM, "Michael McMahon" 
mailto:michael.x.mcma...@oracle.com>> 
wrote:


Okay, adding or removing elements would do that (though it won't
happen in practice)
as opposed to reset(). So, I guess it's better practice to make it
synchronized.

Thanks
Michael.

On 13/05/13 15:19, Vitaly Davidovich wrote:


If the array or nkeys is modified while getHeaders is running,
you can get NPE or ArrayIndexOutOfBoundsException.  If you
synchronize, the method returns some list of headers, and it's
true that as soon as it returns some other thread can mutate the
headers and thus the returned list isn't "in-sync" with current
headers, but there's no exception.

Sent from my phone

On May 13, 2013 9:45 AM, "Michael McMahon"
mailto:michael.x.mcma...@oracle.com>> wrote:

On 13/05/13 14:12, Vitaly Davidovich wrote:


I get what you're saying about before/after, but the
difference would be that if it's called during then you get
an exception purely due to missing synchronization; in the
before/after case, caller may observe "stale" entries but
that's fine, as you say.



How would that be? The only effect of synchronization is to
ensure that the other call
occurs before or after (so to speak).


Maybe headers aren't reset in practice, but this code looks
suspect to someone reading it. :)



Right, we shouldn't be depending on caller behavior, but I
still don't see a problem to be fixed.

Michael


Sent from my phone

On May 13, 2013 9:05 AM, "Michael McMahon"
mailto:michael.x.mcma...@oracle.com>> wrote:

Hi Vitali,

I was going to switch to use Arrays.asList() as you and
Alan suggested. So getHeaderNames() would look like:

public List getHeaderNames() {
return Arrays.asList(keys);
}

So, it turns out synchronizing  nkeys and keys is no
longer necessary.
It's true that reset() could be called during the call.
But, it could (in theory) be called
before or after either. In practice that won't happen,
since the request headers
aren't ever reset.

Michael



On 13/05/13 13:36, Vitaly Davidovich wrote:


Actually, local may not work since getHeaders uses
nkeys as well - can run into AIOBE.  Probably best to
just synchronize given current implementation.

Sent from my phone

On May 13, 2013 8:30 AM, "Vitaly Davidovich"
mailto:vita...@gmail.com>> wrote:

Hi Michael,

On the synchronized issue, I think you do need it;
if someone, e.g., calls reset() while this method
is running, you'll get NPE.  Maybe pull the keys
array into a local then and iterate over the local
instead?

Also, why LinkedList instead of ArrayList(or
Arrays.asList, as Alan mentioned, although maybe
caller is expected to modify the returned list)?

Thanks

Sent from my phone

On May 13, 2013 6:42 AM, "Michael McMahon"
mailto:michael.x.mcma...@oracle.com>> wrote:

Thanks for the review. On the javadoc comments,
there are a couple
of small spec changes that will probably happen
after feature freeze anyway.
So, that might be the best time to address the
other javadoc issues.

I agree with your other comments. On the
synchronized method in MessageHeader,
I don't believe it needs to be synchronized
since the method is not relying on
consistency between object fields, and the
returned object can be
modified before, during or after the method is
called anyway.

Michael

On 12/05/13 08:13, Alan Bateman wrote:

On 10/05/2013 12:34, Michael McMahon wrote:

Re: Code review: 8010464: Evolve java networking same origin policy

[Vitaly Davidovich]

If the headers are supposed to be parsed and then readonly, then the class
can be made immutable? Don't know how much work that would entail.

To minimize synch overhead, I think you can assign the array and nkeys to
locals inside a synch region and then do the copying outside of it.

Sent from my phone
On May 13, 2013 10:43 AM, "Michael McMahon" 
wrote:

>  Okay, adding or removing elements would do that (though it won't happen
> in practice)
> as opposed to reset(). So, I guess it's better practice to make it
> synchronized.
>
> Thanks
> Michael.
>
> On 13/05/13 15:19, Vitaly Davidovich wrote:
>
> If the array or nkeys is modified while getHeaders is running, you can get
> NPE or ArrayIndexOutOfBoundsException.  If you synchronize, the method
> returns some list of headers, and it's true that as soon as it returns some
> other thread can mutate the headers and thus the returned list isn't
> "in-sync" with current headers, but there's no exception.
>
> Sent from my phone
> On May 13, 2013 9:45 AM, "Michael McMahon" 
> wrote:
>
>>  On 13/05/13 14:12, Vitaly Davidovich wrote:
>>
>> I get what you're saying about before/after, but the difference would be
>> that if it's called during then you get an exception purely due to missing
>> synchronization; in the before/after case, caller may observe "stale"
>> entries but that's fine, as you say.
>>
>>
>> How would that be? The only effect of synchronization is to ensure that
>> the other call
>> occurs before or after (so to speak).
>>
>>  Maybe headers aren't reset in practice, but this code looks suspect to
>> someone reading it. :)
>>
>>
>> Right, we shouldn't be depending on caller behavior, but I still don't
>> see a problem to be fixed.
>>
>> Michael
>>
>>  Sent from my phone
>> On May 13, 2013 9:05 AM, "Michael McMahon" 
>> wrote:
>>
>>>  Hi Vitali,
>>>
>>> I was going to switch to use Arrays.asList() as you and Alan suggested.
>>> So getHeaderNames() would look like:
>>>
>>> public List getHeaderNames() {
>>> return Arrays.asList(keys);
>>> }
>>>
>>> So, it turns out synchronizing  nkeys and keys is no longer necessary.
>>> It's true that reset() could be called during the call. But, it could
>>> (in theory) be called
>>> before or after either. In practice that won't happen, since the request
>>> headers
>>> aren't ever reset.
>>>
>>> Michael
>>>
>>>
>>>
>>> On 13/05/13 13:36, Vitaly Davidovich wrote:
>>>
>>> Actually, local may not work since getHeaders uses nkeys as well - can
>>> run into AIOBE.  Probably best to just synchronize given current
>>> implementation.
>>>
>>> Sent from my phone
>>> On May 13, 2013 8:30 AM, "Vitaly Davidovich"  wrote:
>>>
 Hi Michael,

 On the synchronized issue, I think you do need it; if someone, e.g.,
 calls reset() while this method is running, you'll get NPE.  Maybe pull the
 keys array into a local then and iterate over the local instead?

 Also, why LinkedList instead of ArrayList(or Arrays.asList, as Alan
 mentioned, although maybe caller is expected to modify the returned list)?

 Thanks

 Sent from my phone
 On May 13, 2013 6:42 AM, "Michael McMahon" <
 michael.x.mcma...@oracle.com> wrote:

> Thanks for the review. On the javadoc comments, there are a couple
> of small spec changes that will probably happen after feature freeze
> anyway.
> So, that might be the best time to address the other javadoc issues.
>
> I agree with your other comments. On the synchronized method in
> MessageHeader,
> I don't believe it needs to be synchronized since the method is not
> relying on
> consistency between object fields, and the returned object can be
> modified before, during or after the method is called anyway.
>
> Michael
>
> On 12/05/13 08:13, Alan Bateman wrote:
>
>> On 10/05/2013 12:34, Michael McMahon wrote:
>>
>>> Hi,
>>>
>>> This is the webrev for the HttpURLPermission addition.
>>> As well as the new permission class, the change
>>> includes the use of the permission in java.net.HttpURLConnection.
>>>
>>> The code basically checks for a HttpURLPermission in plainConnect(),
>>> getInputStream() and getOutputStream() for the request and if
>>> the caller has permission the request is executed in a doPrivileged()
>>> block. When the limited doPrivileged feature is integrated, I will
>>> change the doPrivileged() call to limit the privilege elevation to a
>>> single
>>> SocketPermission (as shown in the code comments).
>>>
>>> The webrev is at
>>> http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
>>>
>> A partial review, focusing mostly on the spec as we've been through a
>> few rounds on that part already. Overall I think the javadoc looks quite
>> good. I realize someone suggested using lowercase "url" in the javadoc 
>> but
>> as the usage is as an acronym then it might be cleare

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

Okay, adding or removing elements would do that (though it won't happen 
in practice)
as opposed to reset(). So, I guess it's better practice to make it 
synchronized.


Thanks
Michael.

On 13/05/13 15:19, Vitaly Davidovich wrote:


If the array or nkeys is modified while getHeaders is running, you can 
get NPE or ArrayIndexOutOfBoundsException.  If you synchronize, the 
method returns some list of headers, and it's true that as soon as it 
returns some other thread can mutate the headers and thus the returned 
list isn't "in-sync" with current headers, but there's no exception.


Sent from my phone

On May 13, 2013 9:45 AM, "Michael McMahon" 
mailto:michael.x.mcma...@oracle.com>> 
wrote:


On 13/05/13 14:12, Vitaly Davidovich wrote:


I get what you're saying about before/after, but the difference
would be that if it's called during then you get an exception
purely due to missing synchronization; in the before/after case,
caller may observe "stale" entries but that's fine, as you say.



How would that be? The only effect of synchronization is to ensure
that the other call
occurs before or after (so to speak).


Maybe headers aren't reset in practice, but this code looks
suspect to someone reading it. :)



Right, we shouldn't be depending on caller behavior, but I still
don't see a problem to be fixed.

Michael


Sent from my phone

On May 13, 2013 9:05 AM, "Michael McMahon"
mailto:michael.x.mcma...@oracle.com>> wrote:

Hi Vitali,

I was going to switch to use Arrays.asList() as you and Alan
suggested. So getHeaderNames() would look like:

public List getHeaderNames() {
return Arrays.asList(keys);
}

So, it turns out synchronizing  nkeys and keys is no longer
necessary.
It's true that reset() could be called during the call. But,
it could (in theory) be called
before or after either. In practice that won't happen, since
the request headers
aren't ever reset.

Michael



On 13/05/13 13:36, Vitaly Davidovich wrote:


Actually, local may not work since getHeaders uses nkeys as
well - can run into AIOBE.  Probably best to just
synchronize given current implementation.

Sent from my phone

On May 13, 2013 8:30 AM, "Vitaly Davidovich"
mailto:vita...@gmail.com>> wrote:

Hi Michael,

On the synchronized issue, I think you do need it; if
someone, e.g., calls reset() while this method is
running, you'll get NPE.  Maybe pull the keys array into
a local then and iterate over the local instead?

Also, why LinkedList instead of ArrayList(or
Arrays.asList, as Alan mentioned, although maybe caller
is expected to modify the returned list)?

Thanks

Sent from my phone

On May 13, 2013 6:42 AM, "Michael McMahon"
mailto:michael.x.mcma...@oracle.com>> wrote:

Thanks for the review. On the javadoc comments,
there are a couple
of small spec changes that will probably happen
after feature freeze anyway.
So, that might be the best time to address the other
javadoc issues.

I agree with your other comments. On the
synchronized method in MessageHeader,
I don't believe it needs to be synchronized since
the method is not relying on
consistency between object fields, and the returned
object can be
modified before, during or after the method is
called anyway.

Michael

On 12/05/13 08:13, Alan Bateman wrote:

On 10/05/2013 12:34, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission
addition.
As well as the new permission class, the change
includes the use of the permission in
java.net.HttpURLConnection.

The code basically checks for a
HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for
the request and if
the caller has permission the request is
executed in a doPrivileged()
block. When the limited doPrivileged feature
is integrated, I will
change the doPrivileged() call to limit the
privilege elevation to a single
SocketPermission (as shown in the code
comments).

The webrev is at

Re: Code review: 8010464: Evolve java networking same origin policy

[Vitaly Davidovich]

If the array or nkeys is modified while getHeaders is running, you can get
NPE or ArrayIndexOutOfBoundsException.  If you synchronize, the method
returns some list of headers, and it's true that as soon as it returns some
other thread can mutate the headers and thus the returned list isn't
"in-sync" with current headers, but there's no exception.

Sent from my phone
On May 13, 2013 9:45 AM, "Michael McMahon" 
wrote:

>  On 13/05/13 14:12, Vitaly Davidovich wrote:
>
> I get what you're saying about before/after, but the difference would be
> that if it's called during then you get an exception purely due to missing
> synchronization; in the before/after case, caller may observe "stale"
> entries but that's fine, as you say.
>
>
> How would that be? The only effect of synchronization is to ensure that
> the other call
> occurs before or after (so to speak).
>
>  Maybe headers aren't reset in practice, but this code looks suspect to
> someone reading it. :)
>
>
> Right, we shouldn't be depending on caller behavior, but I still don't see
> a problem to be fixed.
>
> Michael
>
>  Sent from my phone
> On May 13, 2013 9:05 AM, "Michael McMahon" 
> wrote:
>
>>  Hi Vitali,
>>
>> I was going to switch to use Arrays.asList() as you and Alan suggested.
>> So getHeaderNames() would look like:
>>
>> public List getHeaderNames() {
>> return Arrays.asList(keys);
>> }
>>
>> So, it turns out synchronizing  nkeys and keys is no longer necessary.
>> It's true that reset() could be called during the call. But, it could (in
>> theory) be called
>> before or after either. In practice that won't happen, since the request
>> headers
>> aren't ever reset.
>>
>> Michael
>>
>>
>>
>> On 13/05/13 13:36, Vitaly Davidovich wrote:
>>
>> Actually, local may not work since getHeaders uses nkeys as well - can
>> run into AIOBE.  Probably best to just synchronize given current
>> implementation.
>>
>> Sent from my phone
>> On May 13, 2013 8:30 AM, "Vitaly Davidovich"  wrote:
>>
>>> Hi Michael,
>>>
>>> On the synchronized issue, I think you do need it; if someone, e.g.,
>>> calls reset() while this method is running, you'll get NPE.  Maybe pull the
>>> keys array into a local then and iterate over the local instead?
>>>
>>> Also, why LinkedList instead of ArrayList(or Arrays.asList, as Alan
>>> mentioned, although maybe caller is expected to modify the returned list)?
>>>
>>> Thanks
>>>
>>> Sent from my phone
>>> On May 13, 2013 6:42 AM, "Michael McMahon" 
>>> wrote:
>>>
 Thanks for the review. On the javadoc comments, there are a couple
 of small spec changes that will probably happen after feature freeze
 anyway.
 So, that might be the best time to address the other javadoc issues.

 I agree with your other comments. On the synchronized method in
 MessageHeader,
 I don't believe it needs to be synchronized since the method is not
 relying on
 consistency between object fields, and the returned object can be
 modified before, during or after the method is called anyway.

 Michael

 On 12/05/13 08:13, Alan Bateman wrote:

> On 10/05/2013 12:34, Michael McMahon wrote:
>
>> Hi,
>>
>> This is the webrev for the HttpURLPermission addition.
>> As well as the new permission class, the change
>> includes the use of the permission in java.net.HttpURLConnection.
>>
>> The code basically checks for a HttpURLPermission in plainConnect(),
>> getInputStream() and getOutputStream() for the request and if
>> the caller has permission the request is executed in a doPrivileged()
>> block. When the limited doPrivileged feature is integrated, I will
>> change the doPrivileged() call to limit the privilege elevation to a
>> single
>> SocketPermission (as shown in the code comments).
>>
>> The webrev is at
>> http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
>>
> A partial review, focusing mostly on the spec as we've been through a
> few rounds on that part already. Overall I think the javadoc looks quite
> good. I realize someone suggested using lowercase "url" in the javadoc but
> as the usage is as an acronym then it might be clearer if it were in
> uppercase, maybe "URL string" to avoid any confusion with java.net.URL.
>
> I assume you'll add a copyright header to HttpURLPermission before
> pushing this.
>
> A minor comment on the javadoc tags is that you probably should use
> @throws instead of @exception.
>
> At a high-level it would be nice if the fields were final but I guess
> the parsing of actions and being serialized complicates this.
>
> setURI - this parses the URI rather than "sets" it so maybe it should
> be renamed. If you use URI.create then it would avoid needing to catch the
> URISyntaxException.
>
> normalizeMethods/normalizeHeaders- I assume these could use an
> ArrayList.
>
> HttpURLConnection

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

Okay, I get you now. I think this is just a (complicating) consequence
of the syntax. I don't think there is anything we can do about it.

BTW, I probably should warn you that we are looking at changing the spec 
again

to allow port number ranges, the same as SocketPermission...

- Michael

On 13/05/13 14:54, Weijun Wang wrote:



On 5/13/13 9:38 PM, Michael McMahon wrote:

On 13/05/13 14:29, Weijun Wang wrote:

Hi Michael

Until now, for all types of permissions, the "actions" property takes
the form of a comma separated list, and it's always accumulative. For
example, it can be "read", or "write", or "read, write". In fact, the
policytool makes use of this style so that you can click on single
actions items to create a combined one.

Now with HttpURLPermission, this is no longer true.

Of course, there is no way to list single items for HttpURLPermission
(no wellknown header name) so the new style doesn't really affect
policytool that much. It's just that I realized this difference when
trying to support this new Permission type in policytool.


Max

If I understand you right, the difference isn't the fact that the list
elements "accumulate", but that
the range of possible values is unlimited. Of course, there is a syntax
difference (with the ":" char) also.


The components on the two sides of ":" have different meanings, so you 
cannot make "A:B:C:D" from "A:B" and "C:D". That's what I mean "not 
accumulative", at least, cannot be accumulated use simple string 
concatenation.


Yes, for existing Permission types, actions only have limited possible 
values, but it's not restrictive to 1 or 2.





To use the old style, "GET,POST:Header1,Header2" will have to be
rewritten to "GET:Header1, GET:Header2, POST: Header1, POST:Header2".
The more the items are, the more complicated it will be. I am not sure
how many methods and headers you would need in a typical use case.



I'm not sure I follow this. The second example isn't supported in
HttpURLPermission.


Yes, I can see that from the CCC.


You would
just create four separate permission objects if that's what you want to
grant. I imagine that typically
only small numbers of headers (5 or less?) would typically be used.


And you normally need multiple methods as well?

Thanks
Max



Michael


Thanks
Max


On 5/10/13 7:34 PM, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a
single
SocketPermission (as shown in the code comments).

The webrev is at 
http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/


Thanks
Michael

Re: Code review: 8010464: Evolve java networking same origin policy

[Weijun Wang]

On 5/13/13 9:38 PM, Michael McMahon wrote:

On 13/05/13 14:29, Weijun Wang wrote:

Hi Michael

Until now, for all types of permissions, the "actions" property takes
the form of a comma separated list, and it's always accumulative. For
example, it can be "read", or "write", or "read, write". In fact, the
policytool makes use of this style so that you can click on single
actions items to create a combined one.

Now with HttpURLPermission, this is no longer true.

Of course, there is no way to list single items for HttpURLPermission
(no wellknown header name) so the new style doesn't really affect
policytool that much. It's just that I realized this difference when
trying to support this new Permission type in policytool.


Max

If I understand you right, the difference isn't the fact that the list
elements "accumulate", but that
the range of possible values is unlimited. Of course, there is a syntax
difference (with the ":" char) also.


The components on the two sides of ":" have different meanings, so you 
cannot make "A:B:C:D" from "A:B" and "C:D". That's what I mean "not 
accumulative", at least, cannot be accumulated use simple string 
concatenation.


Yes, for existing Permission types, actions only have limited possible 
values, but it's not restrictive to 1 or 2.





To use the old style, "GET,POST:Header1,Header2" will have to be
rewritten to "GET:Header1, GET:Header2, POST: Header1, POST:Header2".
The more the items are, the more complicated it will be. I am not sure
how many methods and headers you would need in a typical use case.



I'm not sure I follow this. The second example isn't supported in
HttpURLPermission.


Yes, I can see that from the CCC.


You would
just create four separate permission objects if that's what you want to
grant. I imagine that typically
only small numbers of headers (5 or less?) would typically be used.


And you normally need multiple methods as well?

Thanks
Max



Michael


Thanks
Max


On 5/10/13 7:34 PM, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a
single
SocketPermission (as shown in the code comments).

The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/

Thanks
Michael

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

On 13/05/13 14:12, Vitaly Davidovich wrote:


I get what you're saying about before/after, but the difference would 
be that if it's called during then you get an exception purely due to 
missing synchronization; in the before/after case, caller may observe 
"stale" entries but that's fine, as you say.




How would that be? The only effect of synchronization is to ensure that 
the other call

occurs before or after (so to speak).

Maybe headers aren't reset in practice, but this code looks suspect to 
someone reading it. :)




Right, we shouldn't be depending on caller behavior, but I still don't 
see a problem to be fixed.


Michael


Sent from my phone

On May 13, 2013 9:05 AM, "Michael McMahon" 
mailto:michael.x.mcma...@oracle.com>> 
wrote:


Hi Vitali,

I was going to switch to use Arrays.asList() as you and Alan
suggested. So getHeaderNames() would look like:

public List getHeaderNames() {
return Arrays.asList(keys);
}

So, it turns out synchronizing  nkeys and keys is no longer necessary.
It's true that reset() could be called during the call. But, it
could (in theory) be called
before or after either. In practice that won't happen, since the
request headers
aren't ever reset.

Michael



On 13/05/13 13:36, Vitaly Davidovich wrote:


Actually, local may not work since getHeaders uses nkeys as well
- can run into AIOBE.  Probably best to just synchronize given
current implementation.

Sent from my phone

On May 13, 2013 8:30 AM, "Vitaly Davidovich" mailto:vita...@gmail.com>> wrote:

Hi Michael,

On the synchronized issue, I think you do need it; if
someone, e.g., calls reset() while this method is running,
you'll get NPE.  Maybe pull the keys array into a local then
and iterate over the local instead?

Also, why LinkedList instead of ArrayList(or Arrays.asList,
as Alan mentioned, although maybe caller is expected to
modify the returned list)?

Thanks

Sent from my phone

On May 13, 2013 6:42 AM, "Michael McMahon"
mailto:michael.x.mcma...@oracle.com>> wrote:

Thanks for the review. On the javadoc comments, there are
a couple
of small spec changes that will probably happen after
feature freeze anyway.
So, that might be the best time to address the other
javadoc issues.

I agree with your other comments. On the synchronized
method in MessageHeader,
I don't believe it needs to be synchronized since the
method is not relying on
consistency between object fields, and the returned
object can be
modified before, during or after the method is called anyway.

Michael

On 12/05/13 08:13, Alan Bateman wrote:

On 10/05/2013 12:34, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission
addition.
As well as the new permission class, the change
includes the use of the permission in
java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission
in plainConnect(),
getInputStream() and getOutputStream() for the
request and if
the caller has permission the request is executed
in a doPrivileged()
block. When the limited doPrivileged feature is
integrated, I will
change the doPrivileged() call to limit the
privilege elevation to a single
SocketPermission (as shown in the code comments).

The webrev is at
http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/


A partial review, focusing mostly on the spec as
we've been through a few rounds on that part already.
Overall I think the javadoc looks quite good. I
realize someone suggested using lowercase "url" in
the javadoc but as the usage is as an acronym then it
might be clearer if it were in uppercase, maybe "URL
string" to avoid any confusion with java.net.URL.

I assume you'll add a copyright header to
HttpURLPermission before pushing this.

A minor comment on the javadoc tags is that you
probably should use @throws instead of @exception.

At a high-level it would be nice if the fields were
final but I guess the parsing of actions and being
serialized complicates this.

s

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

On 13/05/13 14:29, Weijun Wang wrote:

Hi Michael

Until now, for all types of permissions, the "actions" property takes 
the form of a comma separated list, and it's always accumulative. For 
example, it can be "read", or "write", or "read, write". In fact, the 
policytool makes use of this style so that you can click on single 
actions items to create a combined one.


Now with HttpURLPermission, this is no longer true.

Of course, there is no way to list single items for HttpURLPermission 
(no wellknown header name) so the new style doesn't really affect 
policytool that much. It's just that I realized this difference when 
trying to support this new Permission type in policytool.



Max

If I understand you right, the difference isn't the fact that the list 
elements "accumulate", but that
the range of possible values is unlimited. Of course, there is a syntax 
difference (with the ":" char) also.


To use the old style, "GET,POST:Header1,Header2" will have to be 
rewritten to "GET:Header1, GET:Header2, POST: Header1, POST:Header2". 
The more the items are, the more complicated it will be. I am not sure 
how many methods and headers you would need in a typical use case.




I'm not sure I follow this. The second example isn't supported in 
HttpURLPermission. You would
just create four separate permission objects if that's what you want to 
grant. I imagine that typically

only small numbers of headers (5 or less?) would typically be used.

Michael


Thanks
Max


On 5/10/13 7:34 PM, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a 
single

SocketPermission (as shown in the code comments).

The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/

Thanks
Michael

Re: Code review: 8010464: Evolve java networking same origin policy

[Weijun Wang]

Hi Michael

Until now, for all types of permissions, the "actions" property takes 
the form of a comma separated list, and it's always accumulative. For 
example, it can be "read", or "write", or "read, write". In fact, the 
policytool makes use of this style so that you can click on single 
actions items to create a combined one.


Now with HttpURLPermission, this is no longer true.

Of course, there is no way to list single items for HttpURLPermission 
(no wellknown header name) so the new style doesn't really affect 
policytool that much. It's just that I realized this difference when 
trying to support this new Permission type in policytool.


To use the old style, "GET,POST:Header1,Header2" will have to be 
rewritten to "GET:Header1, GET:Header2, POST: Header1, POST:Header2". 
The more the items are, the more complicated it will be. I am not sure 
how many methods and headers you would need in a typical use case.


Thanks
Max


On 5/10/13 7:34 PM, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a single
SocketPermission (as shown in the code comments).

The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/

Thanks
Michael

Re: Code review: 8010464: Evolve java networking same origin policy

[Vitaly Davidovich]

I get what you're saying about before/after, but the difference would be
that if it's called during then you get an exception purely due to missing
synchronization; in the before/after case, caller may observe "stale"
entries but that's fine, as you say.

Maybe headers aren't reset in practice, but this code looks suspect to
someone reading it. :)

Sent from my phone
On May 13, 2013 9:05 AM, "Michael McMahon" 
wrote:

>  Hi Vitali,
>
> I was going to switch to use Arrays.asList() as you and Alan suggested. So
> getHeaderNames() would look like:
>
> public List getHeaderNames() {
> return Arrays.asList(keys);
> }
>
> So, it turns out synchronizing  nkeys and keys is no longer necessary.
> It's true that reset() could be called during the call. But, it could (in
> theory) be called
> before or after either. In practice that won't happen, since the request
> headers
> aren't ever reset.
>
> Michael
>
>
>
> On 13/05/13 13:36, Vitaly Davidovich wrote:
>
> Actually, local may not work since getHeaders uses nkeys as well - can run
> into AIOBE.  Probably best to just synchronize given current implementation.
>
> Sent from my phone
> On May 13, 2013 8:30 AM, "Vitaly Davidovich"  wrote:
>
>> Hi Michael,
>>
>> On the synchronized issue, I think you do need it; if someone, e.g.,
>> calls reset() while this method is running, you'll get NPE.  Maybe pull the
>> keys array into a local then and iterate over the local instead?
>>
>> Also, why LinkedList instead of ArrayList(or Arrays.asList, as Alan
>> mentioned, although maybe caller is expected to modify the returned list)?
>>
>> Thanks
>>
>> Sent from my phone
>> On May 13, 2013 6:42 AM, "Michael McMahon" 
>> wrote:
>>
>>> Thanks for the review. On the javadoc comments, there are a couple
>>> of small spec changes that will probably happen after feature freeze
>>> anyway.
>>> So, that might be the best time to address the other javadoc issues.
>>>
>>> I agree with your other comments. On the synchronized method in
>>> MessageHeader,
>>> I don't believe it needs to be synchronized since the method is not
>>> relying on
>>> consistency between object fields, and the returned object can be
>>> modified before, during or after the method is called anyway.
>>>
>>> Michael
>>>
>>> On 12/05/13 08:13, Alan Bateman wrote:
>>>
 On 10/05/2013 12:34, Michael McMahon wrote:

> Hi,
>
> This is the webrev for the HttpURLPermission addition.
> As well as the new permission class, the change
> includes the use of the permission in java.net.HttpURLConnection.
>
> The code basically checks for a HttpURLPermission in plainConnect(),
> getInputStream() and getOutputStream() for the request and if
> the caller has permission the request is executed in a doPrivileged()
> block. When the limited doPrivileged feature is integrated, I will
> change the doPrivileged() call to limit the privilege elevation to a
> single
> SocketPermission (as shown in the code comments).
>
> The webrev is at
> http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
>
 A partial review, focusing mostly on the spec as we've been through a
 few rounds on that part already. Overall I think the javadoc looks quite
 good. I realize someone suggested using lowercase "url" in the javadoc but
 as the usage is as an acronym then it might be clearer if it were in
 uppercase, maybe "URL string" to avoid any confusion with java.net.URL.

 I assume you'll add a copyright header to HttpURLPermission before
 pushing this.

 A minor comment on the javadoc tags is that you probably should use
 @throws instead of @exception.

 At a high-level it would be nice if the fields were final but I guess
 the parsing of actions and being serialized complicates this.

 setURI - this parses the URI rather than "sets" it so maybe it should
 be renamed. If you use URI.create then it would avoid needing to catch the
 URISyntaxException.

 normalizeMethods/normalizeHeaders- I assume these could use an
 ArrayList.

 HttpURLConnection - "if a security manager is installed", should this
 be "set"?

 MessageHeader - some of the methods are synchronized, some are not. I
 can't quite tell if getHeaderNames needs to be synchronized. Also is there
 any reason why this can't use Arrays.asList?

 HttpURLConnection.setRequestMethod - "connection being open" ->
 "connect in progress"?

 That's all I have for now but I think there is further review work
 required on HttpURLConnection as some of that is tricky.

 -Alan.

>>>
>>>
>

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

Hi Vitali,

I was going to switch to use Arrays.asList() as you and Alan suggested. 
So getHeaderNames() would look like:


public List getHeaderNames() {
return Arrays.asList(keys);
}

So, it turns out synchronizing  nkeys and keys is no longer necessary.
It's true that reset() could be called during the call. But, it could 
(in theory) be called
before or after either. In practice that won't happen, since the request 
headers

aren't ever reset.

Michael



On 13/05/13 13:36, Vitaly Davidovich wrote:


Actually, local may not work since getHeaders uses nkeys as well - can 
run into AIOBE.  Probably best to just synchronize given current 
implementation.


Sent from my phone

On May 13, 2013 8:30 AM, "Vitaly Davidovich" > wrote:


Hi Michael,

On the synchronized issue, I think you do need it; if someone,
e.g., calls reset() while this method is running, you'll get NPE. 
Maybe pull the keys array into a local then and iterate over the

local instead?

Also, why LinkedList instead of ArrayList(or Arrays.asList, as
Alan mentioned, although maybe caller is expected to modify the
returned list)?

Thanks

Sent from my phone

On May 13, 2013 6:42 AM, "Michael McMahon"
mailto:michael.x.mcma...@oracle.com>> wrote:

Thanks for the review. On the javadoc comments, there are a couple
of small spec changes that will probably happen after feature
freeze anyway.
So, that might be the best time to address the other javadoc
issues.

I agree with your other comments. On the synchronized method
in MessageHeader,
I don't believe it needs to be synchronized since the method
is not relying on
consistency between object fields, and the returned object can be
modified before, during or after the method is called anyway.

Michael

On 12/05/13 08:13, Alan Bateman wrote:

On 10/05/2013 12:34, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in
java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in
plainConnect(),
getInputStream() and getOutputStream() for the request
and if
the caller has permission the request is executed in a
doPrivileged()
block. When the limited doPrivileged feature is
integrated, I will
change the doPrivileged() call to limit the privilege
elevation to a single
SocketPermission (as shown in the code comments).

The webrev is at
http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/


A partial review, focusing mostly on the spec as we've
been through a few rounds on that part already. Overall I
think the javadoc looks quite good. I realize someone
suggested using lowercase "url" in the javadoc but as the
usage is as an acronym then it might be clearer if it were
in uppercase, maybe "URL string" to avoid any confusion
with java.net.URL.

I assume you'll add a copyright header to
HttpURLPermission before pushing this.

A minor comment on the javadoc tags is that you probably
should use @throws instead of @exception.

At a high-level it would be nice if the fields were final
but I guess the parsing of actions and being serialized
complicates this.

setURI - this parses the URI rather than "sets" it so
maybe it should be renamed. If you use URI.create then it
would avoid needing to catch the URISyntaxException.

normalizeMethods/normalizeHeaders- I assume these could
use an ArrayList.

HttpURLConnection - "if a security manager is installed",
should this be "set"?

MessageHeader - some of the methods are synchronized, some
are not. I can't quite tell if getHeaderNames needs to be
synchronized. Also is there any reason why this can't use
Arrays.asList?

HttpURLConnection.setRequestMethod - "connection being
open" -> "connect in progress"?

That's all I have for now but I think there is further
review work required on HttpURLConnection as some of that
is tricky.

-Alan.

Re: Code review: 8010464: Evolve java networking same origin policy

[Vitaly Davidovich]

Actually, local may not work since getHeaders uses nkeys as well - can run
into AIOBE.  Probably best to just synchronize given current implementation.

Sent from my phone
On May 13, 2013 8:30 AM, "Vitaly Davidovich"  wrote:

> Hi Michael,
>
> On the synchronized issue, I think you do need it; if someone, e.g., calls
> reset() while this method is running, you'll get NPE.  Maybe pull the keys
> array into a local then and iterate over the local instead?
>
> Also, why LinkedList instead of ArrayList(or Arrays.asList, as Alan
> mentioned, although maybe caller is expected to modify the returned list)?
>
> Thanks
>
> Sent from my phone
> On May 13, 2013 6:42 AM, "Michael McMahon" 
> wrote:
>
>> Thanks for the review. On the javadoc comments, there are a couple
>> of small spec changes that will probably happen after feature freeze
>> anyway.
>> So, that might be the best time to address the other javadoc issues.
>>
>> I agree with your other comments. On the synchronized method in
>> MessageHeader,
>> I don't believe it needs to be synchronized since the method is not
>> relying on
>> consistency between object fields, and the returned object can be
>> modified before, during or after the method is called anyway.
>>
>> Michael
>>
>> On 12/05/13 08:13, Alan Bateman wrote:
>>
>>> On 10/05/2013 12:34, Michael McMahon wrote:
>>>
 Hi,

 This is the webrev for the HttpURLPermission addition.
 As well as the new permission class, the change
 includes the use of the permission in java.net.HttpURLConnection.

 The code basically checks for a HttpURLPermission in plainConnect(),
 getInputStream() and getOutputStream() for the request and if
 the caller has permission the request is executed in a doPrivileged()
 block. When the limited doPrivileged feature is integrated, I will
 change the doPrivileged() call to limit the privilege elevation to a
 single
 SocketPermission (as shown in the code comments).

 The webrev is at http://cr.openjdk.java.net/~**
 michaelm/8010464/webrev.1/

>>> A partial review, focusing mostly on the spec as we've been through a
>>> few rounds on that part already. Overall I think the javadoc looks quite
>>> good. I realize someone suggested using lowercase "url" in the javadoc but
>>> as the usage is as an acronym then it might be clearer if it were in
>>> uppercase, maybe "URL string" to avoid any confusion with java.net.URL.
>>>
>>> I assume you'll add a copyright header to HttpURLPermission before
>>> pushing this.
>>>
>>> A minor comment on the javadoc tags is that you probably should use
>>> @throws instead of @exception.
>>>
>>> At a high-level it would be nice if the fields were final but I guess
>>> the parsing of actions and being serialized complicates this.
>>>
>>> setURI - this parses the URI rather than "sets" it so maybe it should be
>>> renamed. If you use URI.create then it would avoid needing to catch the
>>> URISyntaxException.
>>>
>>> normalizeMethods/**normalizeHeaders- I assume these could use an
>>> ArrayList.
>>>
>>> HttpURLConnection - "if a security manager is installed", should this be
>>> "set"?
>>>
>>> MessageHeader - some of the methods are synchronized, some are not. I
>>> can't quite tell if getHeaderNames needs to be synchronized. Also is there
>>> any reason why this can't use Arrays.asList?
>>>
>>> HttpURLConnection.**setRequestMethod - "connection being open" ->
>>> "connect in progress"?
>>>
>>> That's all I have for now but I think there is further review work
>>> required on HttpURLConnection as some of that is tricky.
>>>
>>> -Alan.
>>>
>>
>>

Re: Code review: 8010464: Evolve java networking same origin policy

[Vitaly Davidovich]

Hi Michael,

On the synchronized issue, I think you do need it; if someone, e.g., calls
reset() while this method is running, you'll get NPE.  Maybe pull the keys
array into a local then and iterate over the local instead?

Also, why LinkedList instead of ArrayList(or Arrays.asList, as Alan
mentioned, although maybe caller is expected to modify the returned list)?

Thanks

Sent from my phone
On May 13, 2013 6:42 AM, "Michael McMahon" 
wrote:

> Thanks for the review. On the javadoc comments, there are a couple
> of small spec changes that will probably happen after feature freeze
> anyway.
> So, that might be the best time to address the other javadoc issues.
>
> I agree with your other comments. On the synchronized method in
> MessageHeader,
> I don't believe it needs to be synchronized since the method is not
> relying on
> consistency between object fields, and the returned object can be
> modified before, during or after the method is called anyway.
>
> Michael
>
> On 12/05/13 08:13, Alan Bateman wrote:
>
>> On 10/05/2013 12:34, Michael McMahon wrote:
>>
>>> Hi,
>>>
>>> This is the webrev for the HttpURLPermission addition.
>>> As well as the new permission class, the change
>>> includes the use of the permission in java.net.HttpURLConnection.
>>>
>>> The code basically checks for a HttpURLPermission in plainConnect(),
>>> getInputStream() and getOutputStream() for the request and if
>>> the caller has permission the request is executed in a doPrivileged()
>>> block. When the limited doPrivileged feature is integrated, I will
>>> change the doPrivileged() call to limit the privilege elevation to a
>>> single
>>> SocketPermission (as shown in the code comments).
>>>
>>> The webrev is at http://cr.openjdk.java.net/~**
>>> michaelm/8010464/webrev.1/
>>>
>> A partial review, focusing mostly on the spec as we've been through a few
>> rounds on that part already. Overall I think the javadoc looks quite good.
>> I realize someone suggested using lowercase "url" in the javadoc but as the
>> usage is as an acronym then it might be clearer if it were in uppercase,
>> maybe "URL string" to avoid any confusion with java.net.URL.
>>
>> I assume you'll add a copyright header to HttpURLPermission before
>> pushing this.
>>
>> A minor comment on the javadoc tags is that you probably should use
>> @throws instead of @exception.
>>
>> At a high-level it would be nice if the fields were final but I guess the
>> parsing of actions and being serialized complicates this.
>>
>> setURI - this parses the URI rather than "sets" it so maybe it should be
>> renamed. If you use URI.create then it would avoid needing to catch the
>> URISyntaxException.
>>
>> normalizeMethods/**normalizeHeaders- I assume these could use an
>> ArrayList.
>>
>> HttpURLConnection - "if a security manager is installed", should this be
>> "set"?
>>
>> MessageHeader - some of the methods are synchronized, some are not. I
>> can't quite tell if getHeaderNames needs to be synchronized. Also is there
>> any reason why this can't use Arrays.asList?
>>
>> HttpURLConnection.**setRequestMethod - "connection being open" ->
>> "connect in progress"?
>>
>> That's all I have for now but I think there is further review work
>> required on HttpURLConnection as some of that is tricky.
>>
>> -Alan.
>>
>
>

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

On 13/05/13 11:08, Chris Hegarty wrote:


On 12/05/2013 08:13, Alan Bateman wrote:


At a high-level it would be nice if the fields were final but I guess
the parsing of actions and being serialized complicates this.


This would be my preference too. You could use the serialization proxy 
pattern, and with some restructuring I think the fields could be made 
final. But, I ok with what you have. We can re-visit later to see if 
this makes sense.




Yes, maybe revisit later, especially if this means using sun.misc.Unsafe 
again. I'm still not

convinced about the benefit of that (being worth the ugliness)

I don't see where the socketPermission field is used in 
HttpURLConnection, I suspect it should be set in URLtoSocketPermission?




right, good catch.

I think it should be possible to add the GPL header to the new test 
policy files.




okay

Thanks!
Michael

Re: Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

Thanks for the review. On the javadoc comments, there are a couple
of small spec changes that will probably happen after feature freeze anyway.
So, that might be the best time to address the other javadoc issues.

I agree with your other comments. On the synchronized method in 
MessageHeader,
I don't believe it needs to be synchronized since the method is not 
relying on

consistency between object fields, and the returned object can be
modified before, during or after the method is called anyway.

Michael

On 12/05/13 08:13, Alan Bateman wrote:

On 10/05/2013 12:34, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a 
single

SocketPermission (as shown in the code comments).

The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
A partial review, focusing mostly on the spec as we've been through a 
few rounds on that part already. Overall I think the javadoc looks 
quite good. I realize someone suggested using lowercase "url" in the 
javadoc but as the usage is as an acronym then it might be clearer if 
it were in uppercase, maybe "URL string" to avoid any confusion with 
java.net.URL.


I assume you'll add a copyright header to HttpURLPermission before 
pushing this.


A minor comment on the javadoc tags is that you probably should use 
@throws instead of @exception.


At a high-level it would be nice if the fields were final but I guess 
the parsing of actions and being serialized complicates this.


setURI - this parses the URI rather than "sets" it so maybe it should 
be renamed. If you use URI.create then it would avoid needing to catch 
the URISyntaxException.


normalizeMethods/normalizeHeaders- I assume these could use an ArrayList.

HttpURLConnection - "if a security manager is installed", should this 
be "set"?


MessageHeader - some of the methods are synchronized, some are not. I 
can't quite tell if getHeaderNames needs to be synchronized. Also is 
there any reason why this can't use Arrays.asList?


HttpURLConnection.setRequestMethod - "connection being open" -> 
"connect in progress"?


That's all I have for now but I think there is further review work 
required on HttpURLConnection as some of that is tricky.


-Alan.

Re: Code review: 8010464: Evolve java networking same origin policy

[Chris Hegarty]

On 12/05/2013 08:13, Alan Bateman wrote:


At a high-level it would be nice if the fields were final but I guess
the parsing of actions and being serialized complicates this.


This would be my preference too. You could use the serialization proxy 
pattern, and with some restructuring I think the fields could be made 
final. But, I ok with what you have. We can re-visit later to see if 
this makes sense.


I don't see where the socketPermission field is used in 
HttpURLConnection, I suspect it should be set in URLtoSocketPermission?


I think it should be possible to add the GPL header to the new test 
policy files.


Otherwise, nice to see this moving forward.

-Chris.

Re: Code review: 8010464: Evolve java networking same origin policy

[Chris Hegarty]

On 12/05/2013 08:13, Alan Bateman wrote:


A partial review, focusing mostly on the spec as we've been through a
few rounds on that part already. Overall I think the javadoc looks quite
good. I realize someone suggested using lowercase "url" in the javadoc
but as the usage is as an acronym then it might be clearer if it were in
uppercase, maybe "URL string" to avoid any confusion with java.net.URL.


This may be as a result of my comments. You are right, lowercase url is 
not quite right.


I like "URL string", or
 1) {code url}, since the given arg is named 'url'
 2) "the given/specified {@code url}"

-Chris.

Re: Code review: 8010464: Evolve java networking same origin policy

[Dmitry Samersoff]

Michael,

It might be better to narrow permissions right now with code like below:

private static
AccessControlContext withPermissions(Permission ... perms){
  Permissions col = new Permissions();
  for (Permission thePerm : perms ) {
col.add(thePerm);
  }
 final ProtectionDomain pd = new ProtectionDomain(null, col);
 return new AccessControlContext( new ProtectionDomain[] { pd });
}


AccessController.doPrivileged(
  new PrivilegedExceptionAction() {
  public Void run() throws IOException {
  plainConnect0();
  return null;

   }, withPermissions(p)
 );

-Dmitry

On 2013-05-10 15:34, Michael McMahon wrote:
> Hi,
> 
> This is the webrev for the HttpURLPermission addition.
> As well as the new permission class, the change
> includes the use of the permission in java.net.HttpURLConnection.
> 
> The code basically checks for a HttpURLPermission in plainConnect(),
> getInputStream() and getOutputStream() for the request and if
> the caller has permission the request is executed in a doPrivileged()
> block. When the limited doPrivileged feature is integrated, I will
> change the doPrivileged() call to limit the privilege elevation to a single
> SocketPermission (as shown in the code comments).
> 
> The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
> 
> Thanks
> Michael


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.

Re: Code review: 8010464: Evolve java networking same origin policy

[Alan Bateman]

On 10/05/2013 12:34, Michael McMahon wrote:

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a 
single

SocketPermission (as shown in the code comments).

The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
A partial review, focusing mostly on the spec as we've been through a 
few rounds on that part already. Overall I think the javadoc looks quite 
good. I realize someone suggested using lowercase "url" in the javadoc 
but as the usage is as an acronym then it might be clearer if it were in 
uppercase, maybe "URL string" to avoid any confusion with java.net.URL.


I assume you'll add a copyright header to HttpURLPermission before 
pushing this.


A minor comment on the javadoc tags is that you probably should use 
@throws instead of @exception.


At a high-level it would be nice if the fields were final but I guess 
the parsing of actions and being serialized complicates this.


setURI - this parses the URI rather than "sets" it so maybe it should be 
renamed. If you use URI.create then it would avoid needing to catch the 
URISyntaxException.


normalizeMethods/normalizeHeaders- I assume these could use an ArrayList.

HttpURLConnection - "if a security manager is installed", should this be 
"set"?


MessageHeader - some of the methods are synchronized, some are not. I 
can't quite tell if getHeaderNames needs to be synchronized. Also is 
there any reason why this can't use Arrays.asList?


HttpURLConnection.setRequestMethod - "connection being open" -> "connect 
in progress"?


That's all I have for now but I think there is further review work 
required on HttpURLConnection as some of that is tricky.


-Alan.

Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

Hi,

This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.

The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a single
SocketPermission (as shown in the code comments).

The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/

Thanks
Michael