Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this code in LogInService

public User isValid(User user) {
        if(user == null)
            return user;

        User db_user = userDao.getUserByUsername(user.getUsername());
        if (db_user != null) {
            try {
                if (PasswordHash.validatePassword(user.getPassword(),db_user.getPassword())) {
                    return db_user;
                }
            } catch(Exception e){ e.printStackTrace(); }
        }

        return null;
    }

    public Administrator isValid(Administrator admin){
        if(admin == null)
            return admin;

        Administrator db_admin = adminDao.getAdminByUsername(admin.getUsername());

        if(db_admin != null){
            try{
                if (PasswordHash.validatePassword(admin.getPassword(),db_admin.getPassword())) {
                    return db_admin;
                }
            }catch(Exception e){ e.printStackTrace(); }
        }

        return null;
    }

Where we use a null to check if the user is valid.

My problem now is this. Users and Admins are a different entities but for them to be logged in into the system they have to login. both user and admin has the same method of logging in checking.

My question is there anyway I can refactor this repetitive code inside my Service layer class? or should create a different service layer for Admins to logged in?

share|improve this question
add comment

2 Answers 2

You could use a common interface for User and Admin and a factory/service for the getByUserName method. So you have only a switch in the factory, and all the other duplication is gone.

Btw. I thinks it is a better practice to return null if you know your user is null in your guard condition. A method with the name **is*Something* should return a boolean value. As you will see below, I'm a friend of guard conditions, so I restructured your if.


interface Validable 
{
     String getUsername();
     String getPassword();
}
...
    public boolean isValid(Validable user) {
        if(user == null) return false;

        Validable db_user = validableFactory.getByValidable(user);
        if (db_user == null) return false

        try {
            return PasswordHash.validatePassword(user.getPassword(),db_user.getPassword());
        } 
        catch(Exception e){
            e.printStackTrace(); return false;
        }
    }
...

class ValidableFactory
{
    ...        
    public Validable getByValidable(Validable user)
    {
         if (user instanceOf User) return userDao.getUserByUsername(user.getUsername());
         if (user instanceOf Administrator) return dminDao.getAdminByUsername(admin.getUsername());
         return null;
    }
}

P.S.: In the case you can influence this, why is User not the base class of Administrator? P.P.S.: Why is the password validator throwing an Exception if it has a boolean return value?

share|improve this answer
    
because the ValidatePassword throws an exception with regards to password decryption –  user962206 Jan 17 '13 at 11:28
    
btw Where would I implement the Validable interface? in the ServiceLayer ? or in the dao class? –  user962206 Jan 17 '13 at 11:29
    
Both new classes could be added to the Service as inner static. If you want to use them at an other location, you can also create standalone classes as usual. –  mnhg Jan 17 '13 at 12:03
    
inner static? it still not clear to me on where would I implement the Validable interface –  user962206 Jan 17 '13 at 12:06
    
If you can't remove the exception, than you should at least catch the "minimal" exception (EncrytionException or so). Never catch the raw Exception as you will also catch RuntimeException and all the other stuff you might have missed. –  mnhg Jan 17 '13 at 12:06
show 1 more comment
  1. Fix your method name getUser, getAdmin for starters. If you do not use the returned User object for example except for checking if it is null then consider returning boolean. But I suspect there are more fields in User and Admin.

  2. Fix your parameter types. Why are you passing in User if you get the user from the data store. If it is only for grouping user name and password try defining a value object.

  3. Once you saved your User type from the burden of holding the password remove password field from the User object. The returned type of the authentication method should not contain the password but contain roles of the user etc. Passwords are an authentication concern, whereas Roles etc are an authorization concern.

  4. Unsuccessful authentication attempt is an exceptional case. Returning null from a service in an exceptional case burdens the user of the service with remembering the null check. In this case having a NullPointerException is not the worst that could happen. Not having the exception and letting unauthenticated users access your secure resources is worse.

  5. In the below code:

    if(user == null)
         return user;
    

    If you want to return null say so. Do not confuse the reader of your code. (Reader of the code is probably yourself in a few months.)

    Why are you returning null. If null is not a valid argument, as in this case, throw an IllegalArgumentException. I will give you an example to demonstrate the importance of this. Since you did not provide the code that uses this service it will be an imaginary usage:

    User user = new User(); 
    user.setUsername(request.getParameter("username"); 
    user.setPassword(request.getParameter("pass"); 
    
    
    UserDb = null;
    user = loginService.isValid(user); // I'm guessing usage is as horrible as this
    
    if (userDb == null) {
       return UNSUCCESSFUL_LOGIN; 
    }
    
    saveUserToSession(userDb);
    return WELCOME_PAGE; 
    

    Programmer made a mistake but the user sees "Login Unsuccessfull" message for correct username and password.

  6. Do not get the real password hash out of your datastore and do not pass around the cleartext password.

    User db_user = userDao.getUserByUsername(user.getUsername());
    

    Instead your dao method should be something like this:

    User db_user = userDao.getUserByUsername(username, passwordHash);
    

    which eventually translates to some sql or ldap or hql or whatever query like this

    select blah1, blah2, blah3 
     from myschema.myusertable 
     where username=:username and hashedpass=:hashedpass
    

    This reduces the chance of your hashed passwords to leak into logs or such, removes the need to pass around passwords in the clear makes it harder for you to let users know if a username is valid without knowing the password. I am not a security person, so I will not comment on its effect on the timing attacks. But need to know is a good Object Oriented design principle.

  7. Do a shortcut return here:

        if (db_user != null) {
        //several lines of code
         }
         return null;
    
  8. This should return a boolean. If it doesn't and you cannot change it or find one that does you can always wrap it.

         PasswordHash.validatePassword(user.getPassword(),db_user.getPassword())
    
  9. Do not catch Exception as it is too general and include unchecked exceptions such as nullpointerexception and illegalargumentexception.

     } catch(Exception e){ e.printStackTrace(); }
    
  10. Exception.printStackTrace(); is not proper exception logging. It has no use case in any production environment.

My suggestion

 public User getUser(UserCredential credential) throws SecurityException {
    if (credential == null) throw new IllegalArgumentException("credential required");

    User user = userDao.getUser(credential);

    if (user == null) throw new InvalidCredentialException();

    return user;
  }

static class UserCredential {
    //fields, all of them final
   // ..........
     UserCredential(String userName, String hashedPass) {
        //You can use org.apache.commons.lang.Validate to shorten
       //some common validations
         Validate.notNull(userName);
         Validate.notNull(hashedPass);
          this.userName = userName;
          this.hashedPass = hashedPass;
     }
    //getters no setters
   // ..........
 }

By validating the constructor arguments you catch some errors more easily and remove the need to check them again when used as a parameter type.

String userName = request.getParameter("username");

// case 1
user.setUsername(userName); 
User userDb = loginService.isValid(user);

// case 2
User user = loginService.getUserValid(new UserCredential(userName, hashedPassword));

Note what happens in each case if you misspell the request parameter name. In the second case you will get an illegal argument exception close to the source of the illegal argument whereas in the first case you may tell user his password wrong or you may get some unspecified error in the depths of your ORM or JDBC driver or something.

share|improve this answer
    
You are also assuming Admin inherits from User? –  mnhg Jan 21 '13 at 20:07
    
No. But things that need to be changed are exactly the same, so no need to repeat. UserCredential is a value object, it can be reused straightforward w/o changing the user or admin classes. isValid/getUser is reduced to 4 lines from 9. so it should be clear how getAdmin method will be implemented. –  abuzittin gillifirca Jan 21 '13 at 20:25
add comment

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.