Showing posts with label pac4j. Show all posts
Showing posts with label pac4j. Show all posts

Monday, 27 February 2017

Solving A Chicken/Egg DI problem in Play Framework - Part 2

In Part 1 of this post, I outlined how I was facing a chicken-and-egg problem in moving away from using the deprecated current static reference in a Pac4j Authenticator. Play-Pac4j needs me to wire up any custom Authenticators in a Play Module - and Modules get run very early on in the application boot process - long before dependency injection occurs.

So how can I get a dependency-injected UserService into my custom Authenticator.

Well, it turns out the answer was already staring me right in the face. As a reminder, here's how the "legacy code" obtained a UserService reference:
  lazy val userService:UserService = 
    current.injector.instanceOf[UserService]


And as I mentioned, that lazy was no mere Scala sugar - without it, the "injection" would invariably fail, as again, the DI process had not run yet.

And then it hit me - the lazy keyword was essentially allowing the resolution of a UserService instance to be deferred. So why not use Scala's preferred mechanism for dealing with asynchrony, the Future[T] to formally declare this need to wait for something to happen?

So here's what I did to my Authenticator:

class MyAuthn(fUserService:Future[UserService])
              extends Authenticator[UsernamePasswordCredentials] 

  def validate(creds: UsernamePasswordCredentials,
               ctx: WebContext):Unit = {

    ...
    for {
        userService <- fUserService
        maybeUser <- userService.findByUsername(creds.getUsername)
      } yield {
        ...
      }
    }

So it just comes down to one extra Future[T] to be resolved - and of course once fUserService does get successfully resolved, it's essentially instant after that. So that's the consumption of the Future[UserService] taken care of, but how do we actually produce it?

Well, it turns out that Module implementations get access to a whole load of methods to help them "listen" to the DI process - and then you've just got to implement some Google Guice interfaces to be notified about "provisioning" events, and away you go. Notice how I use a Promise[UserService] which is kinda the "chicken" and use the promise's .future method to produce the "egg":
override def configure(): Unit = {
  ...
  val futuristicProvisionListener = new ProvisionListener {

    private val thePromise = Promise[UserService]
    val theFuture = thePromise.future

    override def onProvision[T](provision: ProvisionInvocation[T]) = {

      if (provision.getBinding.getKey.getTypeLiteral.getRawType 
          == classOf[UserService]) {

        logger.info(s"**onProvision - ${provision.getBinding.getKey}")
        val instance = provision.provision()
        logger.info(s"UserService instance: $instance")
        if (!thePromise.isCompleted) {
          logger.info(s"Completing with UserService instance: $instance")
          thePromise.success(instance.asInstanceOf[UserService])
        }
      }
    }
  }

  // This hooks our listener into the Guice binding process
  bindListener(Matchers.any(), futuristicProvisionListener)

  // And finally, pass the (as-yet unresolved) future 
  // UserService to the authenticator:
  val formClient = new FormClient(
    baseUrl + "/login", 
    new MyAuthn(futuristicProvisionListener.theFuture)
  )
  ...
}


Something that I noticed straight away via the log output was that Guice was creating a vast number of UserService instances - basically it was creating a new one for each place an injection was required. I mopped that up by adding the @Singleton annotation to my UserService, and everything was great. I could probably thus remove the .isCompleted check but it seemed like a good safety-net to leave in, just in case.

Friday, 27 January 2017

Solving A Chicken/Egg DI problem in Play Framework - Part 1

Still loving the Play Framework - I get more productive with it every day, and I'm lucky enough to be using it in my day job, income-generating side projects and fun experiments. Really helps in becoming familiar with every corner of the ecosystem.

One of those ecosystem libraries I've been using a bit is the Pac4j Play integration, which builds on the strong foundation of the Pac4j security library to give a comprehensive authentication/authorization platform on top of Play. It's extremely configurable and extensible, supports all the "modern" ways of logging in (e.g. OAuth2 via social providers) and is reasonably well-documented to boot.

One challenge I came across reared its ugly head when I migrated a Play-Pac4j-based app from Play 2.4 to 2.5. Here's a snippet from my MyAuthn - an implementation of Pac4j's Authenticator interface that performs the validation of credentials that come in from a login form (I've actually featured an earlier version of this class before - it's not really the greatest part of Pac4j):
class MyAuthn extends Authenticator[UsernamePasswordCredentials] {

  ...
  lazy val userService:UserService = 
    current.injector.instanceOf[UserService]

  def validate(creds: UsernamePasswordCredentials,
               ctx: WebContext):Unit = {

    ...
    userService.findByUsername(creds.getUsername).map { maybeUser =>
      ...
    }
  }
}
Ignoring the (above-documented) nastiness of the Unit-returning method, we see that we use a userService that is obtained by asking the current application's injector for a UserService instance.

This works because Play has had dependency injection (via Google Guice) since 2.4. It's obviously not the ideal way to do the injection (constructor injection is far neater in my opinion) but it's needed here because of the way we have to wire up Pac4j in a Module that gets run early on in the application boot sequence:
class SecurityModule (environment: Environment, 
                      config: Configuration) extends AbstractModule {

  override def configure(): Unit = {
    val baseUrl = config.getString("baseUrl").get

    val formClient = new FormClient(baseUrl + "/login", new MyAuthn())

    ...
  }
}
Notice how that at this point, we need to create a MyAuthn but in a Module there's no DI "context" (to use a Spring term) to inject the UserService it needs. Hence the unorthodox use of current.injector and the extremely iffy use of the lazy val to defer access until it's actually needed - the whole thing would fall in a heap if we couldn't defer access like that.

So that works, but in Play 2.5, statically accessing the current running application using the current handle is deprecated. And I hate deprecation warnings - they tell me I'm not using the framework the way the designers (who are far smarter than I) have determined is optimal. And thus I have a problem.

Read Part 2 of this post for the solution!

Friday, 26 February 2016

Making better software with Github

The first time I extracted a library from a private project and open-sourced it to Github was a purely practical decision; the project was simply getting too large for the puny build box I was using to build it with (an OpenShift free node*). The library was Arallon - you can read a bit more about what it does in my blog series about Strongly-Typed Time.

This solved my problem, in that I no longer ran out of PermGen on my build slave. But the repercussions were far-reaching. Any decent public-facing library needs documentation, and Github's README.md is an incredibly convenient place to put it all. I've lost count of the number of times I've found myself reading my own documentation up there on Github; if Arallon was still a hodge-podge of classes within my application, I'd have spent hours trying to deduce my own functionality ...

Of course, a decent open-source library must also have excellent tests and test coverage. Splitting Arallon into its own library gave the tests a new-found focus and similarly the test coverage (measured with JaCoCo) was much more significant.

Since that first library split, I've peeled off many other utility libraries from private projects; almost always things to make Play2 app development a little quicker and/or easier:

As a shameless plug, I use yet another of my own projects (I love my own dogfood!), sbt-skeleton to set up a brand new SBT project with tons of useful defaults like dependencies, repository locations, plugins etc as well as a skeleton directory structure. This helps make the decision to extract a library a no-brainer; I can have a library up-and-building, from scratch, in minutes. This includes having it build and publish to BinTray, which is simply just a matter of cloning an existing Jenkins job and changing the name of the source Github repo.

I've found the implied peer-pressure of having code "out there" for public scrutiny has a strong positive effect on my overall software quality. I'm sure I'm not the only one. I highly recommend going through the process of extracting something re-usable from private code and open-sourcing it into a library you are prepared to stand behind. It will make you a better software developer in many ways.

* This is not a criticism of OpenShift; I love them and would gladly pay them money if they would only take my puny Australian dollars :-(

Monday, 11 January 2016

*Facepalm* 2016

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 ;-)