The newest entry in my (very)
occasional series of career facepalm moments comes from this new year. My current project is using Scala, Play, MongoDB and the
Pac4J library for authentication/authorization with social providers like Google, Facebook, Twitter etc. It's a good library that I've used successfully on a couple of previous projects, but purely in the "select a provider to auth with" mode. For
this project, I needed to use the so-called
HTTP Module to allow a traditional username/password form to also be used, for people who (for whatever reason) don't want to use social login. As an aside, this does actually seem to be a reasonably significant portion of users, even though it is actually placing more trust in an "unknown" website than delegating off to a well-known auth provider like Facebook. But users will be users; I digress.
Setup for Failure
The key integration point between your existing user-access code and pac4j's form handling is your implementation of the
UsernamePasswordAuthenticator interface which is where credentials coming from the input form get checked over and the go/no-go decision is made. Here's what it looks like:
public interface UsernamePasswordAuthenticator
extends Authenticator<UsernamePasswordCredentials> {
/**
* Validate the credentials.
* It should throw a CredentialsException in case of failure.
*
* @param credentials the given credentials.
*/
@Override
void validate(UsernamePasswordCredentials credentials);
}
An apparently super-simple interface, but slightly lacking in documentation, this little method cost me over a day of futzing around debugging, followed by a monstrous facepalm.
Side-effects for the lose
The reasons for this method being
void are not apparent, but such things are not as generally frowned-upon in the Java world as they are in Scala-land. Here's what a basic working implementation (that just checks that the username is the same as the password) looks like as-is in Scala:
object MyUsernamePasswordAuthenticator
extends UsernamePasswordAuthenticator {
val badCredsException =
new BadCredentialsException("Incorrect username/password")
def validate(credentials: UsernamePasswordCredentials):Unit = {
if (credentials.getUsername == credentials.getPassword) {
credentials.setUserProfile(new EmailProfile(u.emailAddress))
} else {
throw badCredsException
}
}
}
So straight away we see that on the happy path, there's an
undocumented incredibly-important
side-effect that is
needed for the whole login flow to work - the
Authenticator must
mutate the incoming
credentials, populating them with a profile that can then be used to load a full user object. Whoa. That's
three pretty-big no-nos just in the
description! The only way I found out about this mutation path was by studying
some test/throwaway code that also ships with the project.
Not great. I think a better Scala implementation might look more like this:
object MyUsernamePasswordAuthenticator
extends ScalaUsernamePasswordAuthenticator[EmailProfile] {
val badCredsException =
new BadCredentialsException("Incorrect username/password")
/** Return a Success containing an instance of EmailProfile if
* successful, otherwise a Failure around an appropriate
* Exception if invalid credentials were provided
*/
def validate(credentials: UsernamePasswordCredentials):Try[EmailProfile] = {
if (credentials.getUsername == credentials.getPassword) {
Success(new EmailProfile(u.emailAddress))
} else {
Failure(badCredsException)
}
}
}
We've added strong typing with a self-documenting return-type, and lost the object mutation side-effect. If I'd been coding to that interface, I wouldn't have needed to go spelunking through test code.
But this wasn't my facepalm.
Race to the bottom
Of course my real
Authenticator instance is going to need to hit the database to verify the credentials. As a longtime
Play Reactive-Mongo fan, I have a nice little asynchronous service layer to do that. My
UserService offers the following method:
class UserService extends MongoService[User]("users") {
...
def findByEmailAddress(emailAddress:String):Future[Option[User]] = {
...
}
I've left out quite a lot of details, but you can probably imagine that plenty of boilerplate can be stuffed into the strongly-typed
MongoService superclass (as well as providing the basic CRUD operations) and subclasses can just add handy extra methods appropriate to their domain object.
The signature of the
findByEmailAddress method encapsulates the fact that the query both a) takes time and b) might not find anything. So let's see how I employed it:
def validate(credentials: UsernamePasswordCredentials):Unit = {
userService.findByEmailAddress(credentials.getUsername).map { maybeUser =>
maybeUser.fold(throw badCredsException) { u =>
if (!User.isValidPassword(u, credentials.getPassword)) {
logger.warn(s"Password for ${u.displayName} did not match!")
throw badCredsException
} else {
logger.info(s"Credentials for ${u.displayName} OK!")
credentials.setUserProfile(new EmailProfile(u.emailAddress))
}
}
}
}
It all looks reasonable right? Failure to find the user means an instant fail; finding the user but not matching the (
BCrypted) passwords also results in an exception being thrown. Otherwise, we perform the necessary mutation and get out.
So here's what happened at runtime:
- A valid username/password combo would appear to get accepted (log entries etc) but not actually be logged in
- Invalid combos would be logged as such but the browser would not redisplay the login form with errors
Have you spotted the problem yet?
The signature of
findByEmailAddress is
Future[Option[User]] - but I've completely forgotten the
Future part (probably because most of the time I'm writing code in Play controllers where returning a
Future is actually encouraged). The signature of the surrounding method, being
Unit, means Scala won't bother type-checking anything. So my method ends up returning nothing almost-instantaneously, which makes pac4j think everything is good. Then it tries to use the
UserProfile of the passed-in object to actually load the user in question, but of course the mutation code hasn't run yet so it's
null- we're almost-certainly still waiting for the result to come back from Mongo!
**Facepalm**
An
Await.ready() around the whole lot fixed this one for me. But I think I might need to offer a refactor to the pac4j team ;-)
