You are viewing a plain text version of this content. The canonical link for it is here.
Posted to scm@geronimo.apache.org by ga...@apache.org on 2010/07/29 07:54:33 UTC
svn commit: r980317 -
/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Author: gawor
Date: Thu Jul 29 05:54:33 2010
New Revision: 980317
URL: http://svn.apache.org/viewvc?rev=980317&view=rev
Log:
restore old code which seems to work better
Modified:
geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Modified: geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
URL: http://svn.apache.org/viewvc/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java?rev=980317&r1=980316&r2=980317&view=diff
==============================================================================
--- geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java (original)
+++ geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java Thu Jul 29 05:54:33 2010
@@ -76,21 +76,20 @@ public class URLPattern {
if (type == EXACT) {
return pattern;
} else {
- //HashSet<String> bucket = new HashSet<String>();
+ HashSet<String> bucket = new HashSet<String>();
StringBuilder result = new StringBuilder(pattern);
+
// Collect a set of qualifying patterns, depending on the type of this pattern.
for (URLPattern p : patterns) {
if (type.check(this, p)) {
- //bucket.add(p.pattern);
- result.append(':');
- result.append(p.pattern);
+ bucket.add(p.pattern);
}
}
// append the set of qualifying patterns
- /*for (String aBucket : bucket) {
+ for (String aBucket : bucket) {
result.append(':');
result.append(aBucket);
- }*/
+ }
return result.toString();
}
}
Re: svn commit: r980317 - /geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Posted by Ivan <xh...@gmail.com>.
Could you please show me any details for it ? I would like to get this
lesson learn. Thanks !
2010/7/29 Jarek Gawor <jg...@gmail.com>
> The end result with the old code was that less Permission objects were
> created which made things easier to debug and it's a bit better from
> performance point of view.
>
> Jarek
>
> On Thu, Jul 29, 2010 at 3:06 AM, Ivan <xh...@gmail.com> wrote:
> > Yes, I have considered this while I did the changes, the URLPattern
> > overrides the equal and hashCode methods, so the result of them are
> totally
> > depending on the String field pattern in the class, also each
> > getQualifiedPattern invocation, a HashSet is passed in. So I am thinking
> > that the initial patterns have already been filtered. There should be no
> > duplicate items. Please correct me if I miss anything :-)
> > I found Jarek opened a JIRA for the web security issue with Equonix
> > platform, is it caused by this change ?
> >
> > 2010/7/29 David Jencks <da...@yahoo.com>
> >>
> >> I'm not sure it would make a difference to the effect of the permission
> >> you end up with, but the code with the hashset eliminates duplicates. I
> >> think you can have duplicates in the "everything leftover" permission
> (IIRC
> >> /:<path1>:<path2>:<path3:....) if some paths have different permissions
> for
> >> different http methods. I don't have an example and haven't looked at
> more
> >> than this much of the code so I could easily be wrong.
> >> thanks
> >> david jencks
> >> On Jul 28, 2010, at 11:12 PM, Ivan wrote:
> >>
> >> Hi, Jarek:
> >> What is the difference between the old one and the new one ? While I
> >> did it in the past, I just feel that there is no need to create an extra
> >> HashSet.
> >>
> >> 2010/7/29 <ga...@apache.org>
> >>>
> >>> Author: gawor
> >>> Date: Thu Jul 29 05:54:33 2010
> >>> New Revision: 980317
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=980317&view=rev
> >>> Log:
> >>> restore old code which seems to work better
> >>>
> >>> Modified:
> >>>
> >>>
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> >>>
> >>> Modified:
> >>>
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> >>> URL:
> >>>
> http://svn.apache.org/viewvc/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java?rev=980317&r1=980316&r2=980317&view=diff
> >>>
> >>>
> ==============================================================================
> >>> ---
> >>>
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> >>> (original)
> >>> +++
> >>>
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> >>> Thu Jul 29 05:54:33 2010
> >>> @@ -76,21 +76,20 @@ public class URLPattern {
> >>> if (type == EXACT) {
> >>> return pattern;
> >>> } else {
> >>> - //HashSet<String> bucket = new HashSet<String>();
> >>> + HashSet<String> bucket = new HashSet<String>();
> >>> StringBuilder result = new StringBuilder(pattern);
> >>> +
> >>> // Collect a set of qualifying patterns, depending on the
> >>> type of this pattern.
> >>> for (URLPattern p : patterns) {
> >>> if (type.check(this, p)) {
> >>> - //bucket.add(p.pattern);
> >>> - result.append(':');
> >>> - result.append(p.pattern);
> >>> + bucket.add(p.pattern);
> >>> }
> >>> }
> >>> // append the set of qualifying patterns
> >>> - /*for (String aBucket : bucket) {
> >>> + for (String aBucket : bucket) {
> >>> result.append(':');
> >>> result.append(aBucket);
> >>> - }*/
> >>> + }
> >>> return result.toString();
> >>> }
> >>> }
> >>>
> >>>
> >>
> >>
> >>
> >> --
> >> Ivan
> >>
> >
> >
> >
> > --
> > Ivan
> >
>
--
Ivan
Re: svn commit: r980317 - /geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Posted by Jarek Gawor <jg...@gmail.com>.
The end result with the old code was that less Permission objects were
created which made things easier to debug and it's a bit better from
performance point of view.
Jarek
On Thu, Jul 29, 2010 at 3:06 AM, Ivan <xh...@gmail.com> wrote:
> Yes, I have considered this while I did the changes, the URLPattern
> overrides the equal and hashCode methods, so the result of them are totally
> depending on the String field pattern in the class, also each
> getQualifiedPattern invocation, a HashSet is passed in. So I am thinking
> that the initial patterns have already been filtered. There should be no
> duplicate items. Please correct me if I miss anything :-)
> I found Jarek opened a JIRA for the web security issue with Equonix
> platform, is it caused by this change ?
>
> 2010/7/29 David Jencks <da...@yahoo.com>
>>
>> I'm not sure it would make a difference to the effect of the permission
>> you end up with, but the code with the hashset eliminates duplicates. I
>> think you can have duplicates in the "everything leftover" permission (IIRC
>> /:<path1>:<path2>:<path3:....) if some paths have different permissions for
>> different http methods. I don't have an example and haven't looked at more
>> than this much of the code so I could easily be wrong.
>> thanks
>> david jencks
>> On Jul 28, 2010, at 11:12 PM, Ivan wrote:
>>
>> Hi, Jarek:
>> What is the difference between the old one and the new one ? While I
>> did it in the past, I just feel that there is no need to create an extra
>> HashSet.
>>
>> 2010/7/29 <ga...@apache.org>
>>>
>>> Author: gawor
>>> Date: Thu Jul 29 05:54:33 2010
>>> New Revision: 980317
>>>
>>> URL: http://svn.apache.org/viewvc?rev=980317&view=rev
>>> Log:
>>> restore old code which seems to work better
>>>
>>> Modified:
>>>
>>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>>>
>>> Modified:
>>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>>> URL:
>>> http://svn.apache.org/viewvc/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java?rev=980317&r1=980316&r2=980317&view=diff
>>>
>>> ==============================================================================
>>> ---
>>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>>> (original)
>>> +++
>>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>>> Thu Jul 29 05:54:33 2010
>>> @@ -76,21 +76,20 @@ public class URLPattern {
>>> if (type == EXACT) {
>>> return pattern;
>>> } else {
>>> - //HashSet<String> bucket = new HashSet<String>();
>>> + HashSet<String> bucket = new HashSet<String>();
>>> StringBuilder result = new StringBuilder(pattern);
>>> +
>>> // Collect a set of qualifying patterns, depending on the
>>> type of this pattern.
>>> for (URLPattern p : patterns) {
>>> if (type.check(this, p)) {
>>> - //bucket.add(p.pattern);
>>> - result.append(':');
>>> - result.append(p.pattern);
>>> + bucket.add(p.pattern);
>>> }
>>> }
>>> // append the set of qualifying patterns
>>> - /*for (String aBucket : bucket) {
>>> + for (String aBucket : bucket) {
>>> result.append(':');
>>> result.append(aBucket);
>>> - }*/
>>> + }
>>> return result.toString();
>>> }
>>> }
>>>
>>>
>>
>>
>>
>> --
>> Ivan
>>
>
>
>
> --
> Ivan
>
Re: svn commit: r980317 - /geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Posted by David Jencks <da...@yahoo.com>.
On Jul 29, 2010, at 12:06 AM, Ivan wrote:
> Yes, I have considered this while I did the changes, the URLPattern overrides the equal and hashCode methods, so the result of them are totally depending on the String field pattern in the class, also each getQualifiedPattern invocation, a HashSet is passed in. So I am thinking that the initial patterns have already been filtered. There should be no duplicate items. Please correct me if I miss anything :-)
that sounds reasonable to me.... maybe Jarek found something we're missing.
> I found Jarek opened a JIRA for the web security issue with Equonix platform, is it caused by this change ?
No, that seems to be because equinox assigns a lot of permissions by default to a bundle, so it's a bit hard to restrict what users can do. I thought I'd see if we can prevent equinox from assigning so many permissions. Do you have a better idea?
thanks
david jencks
>
> 2010/7/29 David Jencks <da...@yahoo.com>
> I'm not sure it would make a difference to the effect of the permission you end up with, but the code with the hashset eliminates duplicates. I think you can have duplicates in the "everything leftover" permission (IIRC /:<path1>:<path2>:<path3:....) if some paths have different permissions for different http methods. I don't have an example and haven't looked at more than this much of the code so I could easily be wrong.
>
> thanks
> david jencks
>
> On Jul 28, 2010, at 11:12 PM, Ivan wrote:
>
>> Hi, Jarek:
>> What is the difference between the old one and the new one ? While I did it in the past, I just feel that there is no need to create an extra HashSet.
>>
>> 2010/7/29 <ga...@apache.org>
>> Author: gawor
>> Date: Thu Jul 29 05:54:33 2010
>> New Revision: 980317
>>
>> URL: http://svn.apache.org/viewvc?rev=980317&view=rev
>> Log:
>> restore old code which seems to work better
>>
>> Modified:
>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>>
>> Modified: geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java?rev=980317&r1=980316&r2=980317&view=diff
>> ==============================================================================
>> --- geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java (original)
>> +++ geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java Thu Jul 29 05:54:33 2010
>> @@ -76,21 +76,20 @@ public class URLPattern {
>> if (type == EXACT) {
>> return pattern;
>> } else {
>> - //HashSet<String> bucket = new HashSet<String>();
>> + HashSet<String> bucket = new HashSet<String>();
>> StringBuilder result = new StringBuilder(pattern);
>> +
>> // Collect a set of qualifying patterns, depending on the type of this pattern.
>> for (URLPattern p : patterns) {
>> if (type.check(this, p)) {
>> - //bucket.add(p.pattern);
>> - result.append(':');
>> - result.append(p.pattern);
>> + bucket.add(p.pattern);
>> }
>> }
>> // append the set of qualifying patterns
>> - /*for (String aBucket : bucket) {
>> + for (String aBucket : bucket) {
>> result.append(':');
>> result.append(aBucket);
>> - }*/
>> + }
>> return result.toString();
>> }
>> }
>>
>>
>>
>>
>>
>> --
>> Ivan
>
>
>
>
> --
> Ivan
Re: svn commit: r980317 - /geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Posted by Ivan <xh...@gmail.com>.
Yes, I have considered this while I did the changes, the URLPattern
overrides the equal and hashCode methods, so the result of them are totally
depending on the String field pattern in the class, also each
getQualifiedPattern invocation, a HashSet is passed in. So I am thinking
that the initial patterns have already been filtered. There should be no
duplicate items. Please correct me if I miss anything :-)
I found Jarek opened a JIRA for the web security issue with Equonix
platform, is it caused by this change ?
2010/7/29 David Jencks <da...@yahoo.com>
> I'm not sure it would make a difference to the effect of the permission you
> end up with, but the code with the hashset eliminates duplicates. I think
> you can have duplicates in the "everything leftover" permission (IIRC
> /:<path1>:<path2>:<path3:....) if some paths have different permissions for
> different http methods. I don't have an example and haven't looked at more
> than this much of the code so I could easily be wrong.
>
> thanks
> david jencks
>
> On Jul 28, 2010, at 11:12 PM, Ivan wrote:
>
> Hi, Jarek:
> What is the difference between the old one and the new one ? While I
> did it in the past, I just feel that there is no need to create an extra
> HashSet.
>
> 2010/7/29 <ga...@apache.org>
>
>> Author: gawor
>> Date: Thu Jul 29 05:54:33 2010
>> New Revision: 980317
>>
>> URL: http://svn.apache.org/viewvc?rev=980317&view=rev
>> Log:
>> restore old code which seems to work better
>>
>> Modified:
>>
>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>>
>> Modified:
>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>> URL:
>> http://svn.apache.org/viewvc/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java?rev=980317&r1=980316&r2=980317&view=diff
>>
>> ==============================================================================
>> ---
>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>> (original)
>> +++
>> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>> Thu Jul 29 05:54:33 2010
>> @@ -76,21 +76,20 @@ public class URLPattern {
>> if (type == EXACT) {
>> return pattern;
>> } else {
>> - //HashSet<String> bucket = new HashSet<String>();
>> + HashSet<String> bucket = new HashSet<String>();
>> StringBuilder result = new StringBuilder(pattern);
>> +
>> // Collect a set of qualifying patterns, depending on the type
>> of this pattern.
>> for (URLPattern p : patterns) {
>> if (type.check(this, p)) {
>> - //bucket.add(p.pattern);
>> - result.append(':');
>> - result.append(p.pattern);
>> + bucket.add(p.pattern);
>> }
>> }
>> // append the set of qualifying patterns
>> - /*for (String aBucket : bucket) {
>> + for (String aBucket : bucket) {
>> result.append(':');
>> result.append(aBucket);
>> - }*/
>> + }
>> return result.toString();
>> }
>> }
>>
>>
>>
>
>
> --
> Ivan
>
>
>
--
Ivan
Re: svn commit: r980317 - /geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Posted by David Jencks <da...@yahoo.com>.
I'm not sure it would make a difference to the effect of the permission you end up with, but the code with the hashset eliminates duplicates. I think you can have duplicates in the "everything leftover" permission (IIRC /:<path1>:<path2>:<path3:....) if some paths have different permissions for different http methods. I don't have an example and haven't looked at more than this much of the code so I could easily be wrong.
thanks
david jencks
On Jul 28, 2010, at 11:12 PM, Ivan wrote:
> Hi, Jarek:
> What is the difference between the old one and the new one ? While I did it in the past, I just feel that there is no need to create an extra HashSet.
>
> 2010/7/29 <ga...@apache.org>
> Author: gawor
> Date: Thu Jul 29 05:54:33 2010
> New Revision: 980317
>
> URL: http://svn.apache.org/viewvc?rev=980317&view=rev
> Log:
> restore old code which seems to work better
>
> Modified:
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>
> Modified: geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java?rev=980317&r1=980316&r2=980317&view=diff
> ==============================================================================
> --- geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java (original)
> +++ geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java Thu Jul 29 05:54:33 2010
> @@ -76,21 +76,20 @@ public class URLPattern {
> if (type == EXACT) {
> return pattern;
> } else {
> - //HashSet<String> bucket = new HashSet<String>();
> + HashSet<String> bucket = new HashSet<String>();
> StringBuilder result = new StringBuilder(pattern);
> +
> // Collect a set of qualifying patterns, depending on the type of this pattern.
> for (URLPattern p : patterns) {
> if (type.check(this, p)) {
> - //bucket.add(p.pattern);
> - result.append(':');
> - result.append(p.pattern);
> + bucket.add(p.pattern);
> }
> }
> // append the set of qualifying patterns
> - /*for (String aBucket : bucket) {
> + for (String aBucket : bucket) {
> result.append(':');
> result.append(aBucket);
> - }*/
> + }
> return result.toString();
> }
> }
>
>
>
>
>
> --
> Ivan
Re: svn commit: r980317 - /geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
Posted by Ivan <xh...@gmail.com>.
Hi, Jarek:
What is the difference between the old one and the new one ? While I did
it in the past, I just feel that there is no need to create an extra
HashSet.
2010/7/29 <ga...@apache.org>
> Author: gawor
> Date: Thu Jul 29 05:54:33 2010
> New Revision: 980317
>
> URL: http://svn.apache.org/viewvc?rev=980317&view=rev
> Log:
> restore old code which seems to work better
>
> Modified:
>
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
>
> Modified:
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> URL:
> http://svn.apache.org/viewvc/geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java?rev=980317&r1=980316&r2=980317&view=diff
>
> ==============================================================================
> ---
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> (original)
> +++
> geronimo/server/trunk/plugins/j2ee/geronimo-web/src/main/java/org/apache/geronimo/web/security/URLPattern.java
> Thu Jul 29 05:54:33 2010
> @@ -76,21 +76,20 @@ public class URLPattern {
> if (type == EXACT) {
> return pattern;
> } else {
> - //HashSet<String> bucket = new HashSet<String>();
> + HashSet<String> bucket = new HashSet<String>();
> StringBuilder result = new StringBuilder(pattern);
> +
> // Collect a set of qualifying patterns, depending on the type
> of this pattern.
> for (URLPattern p : patterns) {
> if (type.check(this, p)) {
> - //bucket.add(p.pattern);
> - result.append(':');
> - result.append(p.pattern);
> + bucket.add(p.pattern);
> }
> }
> // append the set of qualifying patterns
> - /*for (String aBucket : bucket) {
> + for (String aBucket : bucket) {
> result.append(':');
> result.append(aBucket);
> - }*/
> + }
> return result.toString();
> }
> }
>
>
>
--
Ivan