tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: svn commit: r1408739 - in /tomcat/trunk: java/org/apache/catalina/startup/RealmRuleSet.java webapps/docs/config/systemprops.xml
Date Sun, 18 Nov 2012 02:26:34 GMT
Mark,

On 11/17/12 8:04 PM, Christopher Schultz wrote:
> Apologies for the late reply. Please see comments inline.
> 
> On 11/13/12 9:17 AM, markt@apache.org wrote:
>> Author: markt
>> Date: Tue Nov 13 14:17:42 2012
>> New Revision: 1408739
>>
>> URL: http://svn.apache.org/viewvc?rev=1408739&view=rev
>> Log:
>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54141
>> Increase the maximum number of supported nested realm levels from 2 to 3 and make
the maximum configurable via a system property.
>>
>> Modified:
>>     tomcat/trunk/java/org/apache/catalina/startup/RealmRuleSet.java
>>     tomcat/trunk/webapps/docs/config/systemprops.xml
>>
>> Modified: tomcat/trunk/java/org/apache/catalina/startup/RealmRuleSet.java
>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/startup/RealmRuleSet.java?rev=1408739&r1=1408738&r2=1408739&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/catalina/startup/RealmRuleSet.java (original)
>> +++ tomcat/trunk/java/org/apache/catalina/startup/RealmRuleSet.java Tue Nov 13 14:17:42
2012
>> @@ -34,6 +34,10 @@ import org.apache.tomcat.util.digester.R
>>  public class RealmRuleSet extends RuleSetBase {
>>  
>>  
>> +    private static final int MAX_NESTED_REALM_LEVELS = Integer.getInteger(
>> +            "org.apache.catalina.startup.RealmRuleSet.MAX_NESTED_REALM_LEVELS",
>> +            3).intValue();
>> +
>>      // ----------------------------------------------------- Instance Variables
>>  
>>  
>> @@ -83,23 +87,28 @@ public class RealmRuleSet extends RuleSe
>>      @Override
>>      public void addRuleInstances(Digester digester) {
>>  
>> -        digester.addObjectCreate(prefix + "Realm",
>> -                                 null, // MUST be specified in the element,
>> -                                 "className");
>> -        digester.addSetProperties(prefix + "Realm");
>> -        digester.addSetNext(prefix + "Realm",
>> -                            "setRealm",
>> -                            "org.apache.catalina.Realm");
>> -
>> -        digester.addObjectCreate(prefix + "Realm/Realm",
>> -                                 null, // MUST be specified in the element
>> -                                 "className");
>> -        digester.addSetProperties(prefix + "Realm/Realm");
>> -        digester.addSetNext(prefix + "Realm/Realm",
>> -                            "addRealm",
>> -                            "org.apache.catalina.Realm");
>> -
>> -    }
>> +        String pattern = prefix;
>>  
>> +        for (int i = 0; i < MAX_NESTED_REALM_LEVELS; i++) {
>>  
>> +            if (i > 0) {
>> +                pattern += "/";
>> +            }
>> +            pattern += "Realm";
>> +
>> +            digester.addObjectCreate(pattern,
>> +                                     null, // MUST be specified in the element,
>> +                                     "className");
>> +            digester.addSetProperties(pattern);
>> +            if (i == 0) {
>> +                digester.addSetNext(pattern,
>> +                                    "setRealm",
>> +                                    "org.apache.catalina.Realm");
>> +            } else {
>> +                digester.addSetNext(pattern,
>> +                                    "addRealm",
>> +                                    "org.apache.catalina.Realm");
>> +            }
>> +        }
>> +    }
> 
> The code above might be a bit more straightforward if you did "step 1"
> (prefix/Realm + setRealm) above the loop and then loop for each "nested"
> realm.
> 
> Also, I think most users would expect setting MAX_NESTED_REALM_LEVELS
> to, say, 3, would allow Realm/Realm/Realm while I believe the code above
> will only allow Realm/Realm. If you want Realm/Realm/Realm, you have to
> set MAX_NESTED_REALM_LEVELS to 4 because of the strictly-less-than that
> you have in the loop control statement.

That last part is, of course, nonsense. That's what I get for reading
code while quite exhausted.

I still think that the code would be more straightforward without a
special-case when i=0.

-chris


Mime
View raw message