views:

16318

answers:

9

I have a Spring MVC web app which uses Spring Security. I want to know the username of the currently logged in user. The code snippet below is what I'm doing. My question is, is this the accepted way?

I don't like having a call to a static method inside this controller - that defeats the whole purpose of Spring, IMHO. Is there a way to configure the app to have the current SecurityContext, or current Authentication, injected instead?

  @RequestMapping(method = RequestMethod.GET)
  public ModelAndView showResults(final HttpServletRequest request...) {
    final String currentUser = SecurityContextHolder.getContext().getAuthentication().getName();
    ...
  }
+3  A: 

For the last Spring MVC app I wrote, I didn't inject the SecurityContext holder, but I did have a base controller that I had two utility methods related to this ... isAuthenticated() & getUsername(). Internally they do the static method call you described.

At least then it's only in once place if you need to later refactor.

RichH
+2  A: 

You could use Spring AOP aproach. For example if you have some service, that needs to know current principal. You could introduce custom annotation i.e. @Principal , which indicate that this Service should be principal dependent.

public class SomeService {
    private String principal;
    @Principal
    public setPrincipal(String principal){
        this.principal=principal;
    }
}

Then in your advice, which I think needs to extend MethodBeforeAdvice, check that particular service has @Principal annotation and inject Principal name, or set it to 'ANONYMOUS' instead.

Pavel Rodionov
+1  A: 

The only problem is that even after authenticating with Spring Security, the user/principal bean doesn't exist in the container, so dependency-injecting it will be difficult. Before we used Spring Security we would create a session-scoped bean that had the current Principal, inject that into an "AuthService" and then inject that Service into most of the other services in the Application. So those Services would simply call authService.getCurrentUser() to get the object. If you have a place in your code where you get a reference to the same Principal in the session, you can simply set it as a property on your session-scoped bean.

cliff.meyers
+2  A: 

I agree that having to query the SecurityContext for the current user stinks, it seems a very un-Spring way to handle this problem.

I wrote a static "helper" class to deal with this problem; it's dirty in that it's a global and static method, but I figured this way if we change anything related to Security, at least I only have to change the details in one place:

/**
* Returns the domain User object for the currently logged in user, or null
* if no User is logged in.
* 
* @return User object for the currently logged in user, or null if no User
*         is logged in.
*/
public static User getCurrentUser() {

    Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal()

    if (principal instanceof MyUserDetails) return ((MyUserDetails) principal).getUser();

    // principal object is either null or represents anonymous user -
    // neither of which our domain User object can represent - so return null
    return null;
}


/**
 * Utility method to determine if the current user is logged in /
 * authenticated.
 * <p>
 * Equivalent of calling:
 * <p>
 * <code>getCurrentUser() != null</code>
 * 
 * @return if user is logged in
 */
public static boolean isLoggedIn() {
    return getCurrentUser() != null;
}
matt b
This is not thread safe.
HDave
it is as long as SecurityContextHolder.getContext() is, and the latter is threadsafe since it keeps the security details in a threadLocal. This code maintains no state.
matt b
+13  A: 

This is the solution I've ended up going with. Instead of using SecurityContextHolder in my controller, I want to inject something which uses SecurityContextHolder under the hood but abstracts away that singleton-like class from my code. I've found no way to do this other than rolling my own interface, like so:

public interface SecurityContextFacade {

  SecurityContext getContext();

  void setContext(SecurityContext securityContext);

}

Now, my controller (or whatever POJO) would look like this:

public class FooController {

  private final SecurityContextFacade securityContextFacade;

  public FooController(SecurityContextFacade securityContextFacade) {
    this.securityContextFacade = securityContextFacade;
  }

  public void doSomething(){
    SecurityContext context = securityContextFacade.getContext();
    // do something w/ context
  }

}

And, because of the interface being a point of decoupling, unit testing is straightforward. In this example I use Mockito:

public class FooControllerTest {

  private FooController controller;
  private SecurityContextFacade mockSecurityContextFacade;
  private SecurityContext mockSecurityContext;

  @Before
  public void setUp() throws Exception {
    mockSecurityContextFacade = mock(SecurityContextFacade.class);
    mockSecurityContext = mock(SecurityContext.class);
    stub(mockSecurityContextFacade.getContext()).toReturn(mockSecurityContext);
    controller = new FooController(mockSecurityContextFacade);
  }

  @Test
  public void testDoSomething() {
    controller.doSomething();
    verify(mockSecurityContextFacade).getContext();
  }

}

The default implementation of the interface looks like this:

public class SecurityContextHolderFacade implements SecurityContextFacade {

  public SecurityContext getContext() {
    return SecurityContextHolder.getContext();
  }

  public void setContext(SecurityContext securityContext) {
    SecurityContextHolder.setContext(securityContext);
  }

}

And, finally, the production Spring config looks like this:

<bean id="myController" class="com.foo.FooController">
     ...
  <constructor-arg index="1">
    <bean class="com.foo.SecurityContextHolderFacade">
  </constructor-arg>
</bean>

It seems more than a little silly that Spring, a dependency injection container of all things, has not supplied a way to inject something similar. I understand SecurityContextHolder was inherited from acegi, but still. The thing is, they're so close - if only SecurityContextHolder had a getter to get the underlying SecurityContextHolderStrategy instance (which is an interface), you could inject that. In fact, I even opened a Jira issue to that effect.

One last thing - I've just substantially changed the answer I had here before. Check the history if you're curious but, as a coworker pointed out to me, my previous answer would not work in a multi-threaded environment. The underlying SecurityContextHolderStrategy used by SecurityContextHolder is, by default, an instance of ThreadLocalSecurityContextHolderStrategy, which stores SecurityContexts in a ThreadLocal. Therefore, it is not necessarily a good idea to inject the SecurityContext directly into a bean at initialization time - it may need to be retrieved from the ThreadLocal each time, in a multi-threaded environment, so the correct one is retrieved.

Scott Bale
I like your solution -- it's a clever use of the factory-method support in Spring. That said, this is working for you because the controller object is scoped to the web request. If you changed the scope of the controller bean in the wrong way, this would break.
Paul Morie
Good point, thanks.
Scott Bale
The previous two comments refer to an old, incorrect answer which I've just replaced.
Scott Bale
+1 for opening a JIRA ticket! :)
Tuukka Mustonen
Is this still the recommended solution with the current Spring release? I can't believe it needs so much code just to retrieve just the username.
If you're using Spring Security 3.0.x, they implemented my suggestion in the JIRA issue I logged https://jira.springsource.org/browse/SEC-1188 so you can now inject the SecurityContextHolderStrategy instance (from SecurityContextHolder) directly into your bean via standard Spring configuration.
Scott Bale
+1  A: 

Yes, statics are generally bad - generally, but in this case, the static is the most secure code you can write. Since the security context associates a Principal with the currently running thread, the most secure code would access the static from the thread as directly as possible. Hiding the access behind a wrapper class that is injected provides an attacker with more points to attack. They wouldn't need access to the code (which they would have a hard time changing if the jar was signed), they just need a way to override the configuration, which can be done at runtime or slipping some XML onto the classpath. Even using annotation injection in the signed code would be overridable with external XML. Such XML could inject the running system with a rogue principal. This is probably why Spring is doing something so un-Spring-like in this case.

Michael Bushe
A: 

I would just do this:

request.getRemoteUser();

Dan
That might work, but not reliably. From javadoc: "Whether the user name is sent with each subsequent request depends on the browser and type of authentication." -http://download-llnw.oracle.com/javaee/6/api/javax/servlet/http/HttpServletRequest.html#getRemoteUser()
Scott Bale
+2  A: 

I get authenticated user by HttpServletRequest.getUserPrincipal();

Example:

import javax.servlet.http.HttpServletRequest;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.web.authentication.preauth.RequestHeaderAuthenticationFilter;
import org.springframework.stereotype.Controller;
import org.springframework.ui.Model;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.servlet.support.RequestContext;

import foo.Form;

@Controller
@RequestMapping(value="/welcome")
public class IndexController {

    @RequestMapping(method=RequestMethod.GET)
    public String getCreateForm(Model model, HttpServletRequest request) {

        if(request.getUserPrincipal() != null) {
            String loginName = request.getUserPrincipal().getName();
            System.out.println("loginName : " + loginName );
        }

        model.addAttribute("form", new Form());
        return "welcome";
    }
}
digz6666
A: 

I honestly don't know why people use Spring. Well - maybe it might be viable for some solutions - but consider others before attempting.

Tamer Salama
For large (or even small) JavaEE code bases... the code and complexity reduction from Spring is very worth it.
chotchki
My question got to -1 (so far) :)If it's a full JavaEE stack - I can't speak of that. There might be a case. But, if you mean JavaEE (sans EJBs, Services) - then spring IS the source of code complexities (not the other way around).I'm maintaining a plethora of Spring apps, and I can't think of a single case where code/scope scattering between XML, Jar and Java files are a good practice. Not to mention the learning curve, the troubleshooting difficulties, the maintainability. All of that just to glue some layers together (mostly MVC).
Tamer Salama