-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#96 Add protocols parameter on Tomcat valves #97
Conversation
@@ -5,7 +5,7 @@ | |||
<groupId>com.github.dblock.waffle</groupId> | |||
<artifactId>waffle-demo-parent</artifactId> | |||
<version>1.7-SNAPSHOT</version> | |||
<relativePath>../waffle-demo-parent</relativePath> | |||
<relativePath>..</relativePath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because relative path of waffle-demo-parent is just the parent directory. And build fails with the ./waffle-demo-parent relativePath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! This is a pretty good indication that some refactoring is still needed to bring this into a better maven structure. I'll look at addressing that soon. The layout now is a little misleading especially around the demo. Waffle-pom suffers the same design flaw but was setup properly.
I wonder if there's any way we can nest this XML configuration and provide a list, instead of a comma-separated thing? |
} | ||
|
||
@Test | ||
public void should_accept_Negociate_protocol() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negotiate with a T
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arghh. C is in french :(
Can you please make the changes for the other valve versions? Also see my note on XML nesting, maybe we can avoid having a list to parse. |
I assume you are referring to doing something like this to get the XML nesting. <Valve className="waffle.apache.MixedAuthenticator" principalFormat="fqn" roleFormat="both" allowGuestLogin="false">
<protocols>
<protocol>Negotiate</protocol>
<protocol>NTLM</protocol>
</protocols>
</Valve I do not think this is easy without modifying tomcats digester rules. I'm not certain how that can even be accomplished. I was able to code something up and got only as far as getting No Rules Matching Found 'Context/Valve/protocols/protocol'. I think if I can figure out the rules, this would be possible but not sure how much effort it would take and how worth it the results are. For now, I think this is good enough. This site was very useful in getting this far. http://www.javacodegeeks.com/2012/09/apache-digester-example-make-easy.html |
Makes sense. LMK when this is ready to merge with what I'm asking for above. Also please try to squash these commits. Thx for the good work! |
The goal of the protocols attribute is mainly to limit the valve to one single protocol. I'm convinced that having a protocols sub-element isn't worth. |
Merged & squashed commits. |
If this one is OK, I'll propose an other PR for Tomcat 8 (and Tomcat 6 ?)