Yes - this is just preparation for a request scoped AuthInfo. I believe we should start modifying most classes to not use HttpServletRequest - classes should be injecting the specific objects they need. This will cut down on a lot of dependencies and help avoid needing to mock out HttpServletRequest. Currently for many classes to function properly, we need to orchestrate a complex set of interactions on the HttpServletRequest.
On Tue, Sep 2, 2008 at 4:42 PM, Cassie <[EMAIL PROTECTED]> wrote: > For now, because the req is not injectable I was thinking it would be: > > (authInfo gets injected in the constructor of JsonRpcServlet lets say) > authInfo.getSecurityToken(req); > > Then you can mock out authInfo when testing to return whatever you want by > just stuffing a stub class in the constructor. > > - Cassie > > > On Tue, Sep 2, 2008 at 4:35 PM, Kevin Brown <[EMAIL PROTECTED]> wrote: > > > This doesn't give any obvious improvement to testability, since the code > is > > still executed inline and can't be modified by the callers. I don't see > > how: > > > > new AuthInfo(req).getSecurityToken() > > > > is any different from: > > > > AuthInfo.getSecurityToken(req); > > > > If AuthInfo was RequestScoped or at least passed to relevant ctors here, > > that would certainly be an improvement, but here it doesn't seem to be > > changing anything. > > > > On Tue, Sep 2, 2008 at 4:17 PM, <[EMAIL PROTECTED]> wrote: > > > > > Author: evan > > > Date: Tue Sep 2 16:17:39 2008 > > > New Revision: 691426 > > > > > > URL: http://svn.apache.org/viewvc?rev=691426&view=rev > > > Log: > > > AuthInfo is now instance based for testability and injectability. > > > > > > Modified: > > > > > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthInfo.java > > > > > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java > > > > > > > > > > incubator/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/AuthInfoTest.java > > > > > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpGadgetContext.java > > > > > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java > > > > > > > > > > incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/ApiServlet.java > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthInfo.java > > > URL: > > > > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthInfo.java?rev=691426&r1=691425&r2=691426&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthInfo.java > > > (original) > > > +++ > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthInfo.java > > > Tue Sep 2 16:17:39 2008 > > > @@ -17,16 +17,33 @@ > > > */ > > > package org.apache.shindig.auth; > > > > > > +import com.google.inject.Inject; > > > + > > > import javax.servlet.http.HttpServletRequest; > > > > > > /** > > > - * Class to get/set authorization information on a servlet request. > > > - * Used by auth filters. > > > + * Class to get authorization information on a servlet request. > > > + * > > > + * Information is set by adding an AuthentiationServletFilter, and > there > > > + * is no way to set in a public API. This can be added in the future > for > > > testing > > > + * purposes. > > > */ > > > public class AuthInfo { > > > + private HttpServletRequest req; > > > + > > > + /** > > > + * Create AuthInfo from a given HttpServletRequest > > > + * @param req > > > + */ > > > + @Inject > > > + public AuthInfo(HttpServletRequest req) { > > > + this.req = req; > > > + } > > > > > > /** > > > * Constants for request attribute keys > > > + * > > > + * This is only public for testing. > > > */ > > > public enum Attribute { > > > /** The security token */ > > > @@ -42,41 +59,41 @@ > > > /** > > > * Get the security token for this request. > > > * > > > - * @param req The request > > > * @return The security token > > > */ > > > - public static SecurityToken getSecurityToken(HttpServletRequest req) > { > > > + public SecurityToken getSecurityToken() { > > > return getRequestAttribute(req, Attribute.SECURITY_TOKEN); > > > } > > > > > > /** > > > * Get the hosted domain for this request. > > > * > > > - * @param req The request > > > * @return The domain, or [EMAIL PROTECTED] null} if no domain was found > > > */ > > > - public static String getAuthType(HttpServletRequest req) { > > > + public String getAuthType() { > > > return getRequestAttribute(req, Attribute.AUTH_TYPE); > > > } > > > > > > /** > > > * Set the security token for the request. > > > * > > > - * @param req The request > > > * @param token The security token > > > + * @return This object > > > */ > > > - public static void setSecurityToken(HttpServletRequest req, > > > SecurityToken token) { > > > + AuthInfo setSecurityToken(SecurityToken token) { > > > setRequestAttribute(req, Attribute.SECURITY_TOKEN, token); > > > + return this; > > > } > > > > > > /** > > > * Set the auth type for the request. > > > * > > > - * @param req The request > > > * @param authType The named auth type > > > + * @return This object > > > */ > > > - public static void setAuthType(HttpServletRequest req, String > > authType) > > > { > > > + AuthInfo setAuthType(String authType) { > > > setRequestAttribute(req, Attribute.AUTH_TYPE, authType); > > > + return this; > > > } > > > > > > /** > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java > > > URL: > > > > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java?rev=691426&r1=691425&r2=691426&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java > > > (original) > > > +++ > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java > > > Tue Sep 2 16:17:39 2008 > > > @@ -62,8 +62,7 @@ > > > for (AuthenticationHandler handler : handlers) { > > > SecurityToken token = handler.getSecurityTokenFromRequest(req); > > > if (token != null) { > > > - AuthInfo.setAuthType(req, handler.getName()); > > > - AuthInfo.setSecurityToken(req, token); > > > + new > > > AuthInfo(req).setAuthType(handler.getName()).setSecurityToken(token); > > > chain.doFilter(req, response); > > > return; > > > } > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/AuthInfoTest.java > > > URL: > > > > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/AuthInfoTest.java?rev=691426&r1=691425&r2=691426&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > incubator/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/AuthInfoTest.java > > > (original) > > > +++ > > > > > > incubator/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/AuthInfoTest.java > > > Tue Sep 2 16:17:39 2008 > > > @@ -17,6 +17,10 @@ > > > */ > > > package org.apache.shindig.auth; > > > > > > +import com.google.inject.AbstractModule; > > > +import com.google.inject.Guice; > > > +import com.google.inject.Injector; > > > + > > > import org.apache.shindig.common.testing.FakeGadgetToken; > > > import org.apache.shindig.common.testing.FakeHttpServletRequest; > > > > > > @@ -29,13 +33,45 @@ > > > public void testToken() throws Exception { > > > HttpServletRequest req = new FakeHttpServletRequest(); > > > SecurityToken token = new FakeGadgetToken(); > > > - AuthInfo.setSecurityToken(req, token); > > > - assertEquals(token, AuthInfo.getSecurityToken(req)); > > > + > > > + AuthInfo info = new AuthInfo(req).setSecurityToken(token); > > > + > > > + assertEquals(token, info.getSecurityToken()); > > > + // This should work when creating a new AuthInfo from the same > > request > > > + assertEquals(token, new AuthInfo(req).getSecurityToken()); > > > } > > > > > > public void testAuthType() throws Exception { > > > HttpServletRequest req = new FakeHttpServletRequest(); > > > - AuthInfo.setAuthType(req, "FakeAuth"); > > > - assertEquals("FakeAuth", AuthInfo.getAuthType(req)); > > > + > > > + AuthInfo info = new AuthInfo(req).setAuthType("FakeAuth"); > > > + > > > + assertEquals("FakeAuth", info.getAuthType()); > > > + // This should work when creating a new AuthInfo from the same > > request > > > + assertEquals("FakeAuth", new AuthInfo(req).getAuthType()); > > > + } > > > + > > > + public void testBinding() throws Exception { > > > + HttpServletRequest req = new FakeHttpServletRequest(); > > > + SecurityToken token = new FakeGadgetToken(); > > > + new AuthInfo(req).setSecurityToken(token).setAuthType("FakeAuth"); > > > + > > > + Injector injector = Guice.createInjector(new TestModule(req)); > > > + AuthInfo injected = injector.getInstance(AuthInfo.class); > > > + assertEquals(token, injected.getSecurityToken()); > > > + assertEquals("FakeAuth", injected.getAuthType()); > > > + } > > > + > > > + private static class TestModule extends AbstractModule { > > > + private HttpServletRequest req; > > > + > > > + public TestModule(HttpServletRequest req) { > > > + this.req = req; > > > + } > > > + @Override > > > + protected void configure() { > > > + bind(HttpServletRequest.class).toInstance(req); > > > + } > > > + > > > } > > > } > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpGadgetContext.java > > > URL: > > > > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpGadgetContext.java?rev=691426&r1=691425&r2=691426&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpGadgetContext.java > > > (original) > > > +++ > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpGadgetContext.java > > > Tue Sep 2 16:17:39 2008 > > > @@ -121,7 +121,7 @@ > > > > > > @Override > > > public SecurityToken getToken() { > > > - return AuthInfo.getSecurityToken(request); > > > + return new AuthInfo(request).getSecurityToken(); > > > } > > > > > > @Override > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java > > > URL: > > > > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java?rev=691426&r1=691425&r2=691426&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java > > > (original) > > > +++ > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java > > > Tue Sep 2 16:17:39 2008 > > > @@ -235,7 +235,7 @@ > > > * @return A valid token for the given input. > > > */ > > > private SecurityToken extractAndValidateToken(HttpServletRequest > > request) > > > throws GadgetException { > > > - SecurityToken token = AuthInfo.getSecurityToken(request); > > > + SecurityToken token = new AuthInfo(request).getSecurityToken(); > > > if (token == null) { > > > throw new > > > GadgetException(GadgetException.Code.INVALID_SECURITY_TOKEN); > > > } > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/ApiServlet.java > > > URL: > > > > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/ApiServlet.java?rev=691426&r1=691425&r2=691426&view=diff > > > > > > > > > ============================================================================== > > > --- > > > > > > incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/ApiServlet.java > > > (original) > > > +++ > > > > > > incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/ApiServlet.java > > > Tue Sep 2 16:17:39 2008 > > > @@ -66,7 +66,7 @@ > > > } > > > > > > protected SecurityToken getSecurityToken(HttpServletRequest > > > servletRequest) { > > > - return AuthInfo.getSecurityToken(servletRequest); > > > + return new AuthInfo(servletRequest).getSecurityToken(); > > > } > > > > > > protected abstract void sendError(HttpServletResponse > servletResponse, > > > ResponseItem responseItem) > > > > > > > > > > > >
