You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Evan Gilbert <ui...@google.com> on 2008/09/03 01:56:29 UTC
Re: svn commit: r691426 - in /incubator/shindig/trunk/java:
common/src/main/java/org/apache/shindig/auth/ common/src/test/java/org/apache/shindig/auth/
gadgets/src/main/java/org/apache/shindig/gadgets/servlet/
social-api/src/main/java/org/apache/shin
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 <do...@apache.org> 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 <et...@google.com> 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, <ev...@apache.org> 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 {@code 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)
> > >
> > >
> > >
> >
>