Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm using Shiro as my security manager for a Spring MVC web application.

The login basically happens in these lines:

Subject user = SecurityUtils.getSubject();
user.login(new UsernamePasswordToken(username, password));

Where should I put this logic? login() can throw exceptions if the requested user doesn't exist or wrong password was provided, etc. Should I call the code from my controller and do the error handling there or should I call it from my service layer, catch exceptions and rethrow my own up to the controller?

share|improve this question

migrated from codereview.stackexchange.com Feb 11 '13 at 20:31

This question came from our site for peer programmer code reviews.

2 Answers 2

up vote 2 down vote accepted

Either of the two options you present is valid.

Services (the Model layer) are allowed to call other services. So having your service call the authentication service is acceptable.

Likewise, Controllers are allowed to call to services so that option is equally acceptable.

If Shiro is providing your only authentication mechanisms, then it's kind of a toss-up between either approach. Calling from the Controller would seem a little faster to develop and potentially have less error handling code.

However, if you intend to add additional checks / wrappings around that authentication call, then you should have your Model make the call. So if there is a likely additional call for authorization after the authentication check, then I would lean towards calling from the Model.


Quick note, loose definitions:
Authentication: "Am I who I say I am?"
Authorization: "Do I have the authority or permission to access this?"

share|improve this answer

It should be in a Service Layer.

public interface UserService {
    public void findByLogin(final String userName);
}

And in a Service implementation :

public class UserServiceImpl implements UserService {

    @Inject
    private UserRepository userRepository;

    public User findByLogin(final String userName, final String password) {
        Preconditions.checkArgument(!Strings.isNullOrEmpty(userName), "Username is mandatory !");
        return Preconditions.checkNotNull(userRepository.findByLogin(userName), "User not found !");
    }
}

And an Identity provider service :

public class InternalIdentityProviderImpl implements ... {

    @Inject
    private UserService userService;

    public void authenticate(final String login, final String password) throws BadCredentialsException, IdentityNotFoundException {
        try {
            Preconditions.checkArgument(!Strings.isNullOrEmpty(login), "Username is mandatory !");
            Preconditions.checkArgument(!Strings.isNullOrEmpty(password), "Username is mandatory !");
        catch(Exception e) {
            throw new BadCredentialsException(e);
        }

        // Check user 
        try {
            User internal = Preconditions.checkNotNull(userService.findByLogin(username));
        } catch(Exception e) {
            // logs
            throw new IdentityNotFoundException(e);
        }

        // Shiro authentication 
        Subject user = SecurityUtils.getSubject();
        user.login(new UsernamePasswordToken(username, password));
    }

}

You should decorelate how the user is retrieved from the identity provider, and how it is validated by your identification system.

Currently I used RuntimeException thrown by Guava Preconditions, but you can use a specific handcrafted Exception.

ie:

  • IdentityNotFoundException : from your identity provider.
  • BadCredentialsException : from your identification system.

This service could be called from the controller, and handle the exception to display the right message.

share|improve this answer
    
Minor issue: public class UserService should be public interface UserService –  Paul Richter Feb 11 '13 at 22:02
1  
Yep ! i fix it ! –  Zenithar Feb 11 '13 at 22:02
    
When I do user.login(), it internally uses a configured Realm to check the database for the user. A Realm is like a specialized service class used by shiro. Your answer has good points but doesn't fit with the Shiro workflow. –  Sotirios Delimanolis Feb 13 '13 at 17:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.