User ID instead of email address for play-silhouette-seed

[Michael Slinn]

play-silhouette-seed is set up to use email address and password for identifying and logging in users. I would like to have users login by user id instead, and I don't want to provide any other means of authentication. Here are the changes that I have made:

1. In signIn.scala.html, changed the field b3.email to b3.text so so a user id will be accepted
2. Changed SignInForm so the first mapped field is "userId" with type nonEmptyText instead of a field called "email" with type email, also renamed Data.email to Data.userId 
   
3. In SignUpForm, added a field "userId" of type nonEmptyText, and added a new userId field to SignUpForm.Data
4. Changed DBAuthInfoDAO.find to look up the user by userId (which is actually loginInfo.providerKey) instead of searching by email address
   
5. Changed UserServiceImpl.retrieve to search by userId instead of by email address
6. In SignInController.submit now use Credentials(data.userId, data.password) instead of Credentials(data.email, data.password)
7. Changed SilhouetteModule.provideAuthInfoRepository to just return new DelegableAuthInfoRepository(passwordInfoDAO) because I don't need oauth1InfoDAO, oauth2InfoDAO or openIDInfoDAO
8. Changed trait AuthInfoRepository.find's signature to:
   
def find[T <: AuthInfo](loginInfo: LoginInfo)(implicit tag: ClassTag[T]): Future[Option[T]]

I think the last change is incorrect because logins always fail when evaluating credentialsProvider.authenticate(credentials in SignInController.authenticate, and returns this exception:

authInfoRepository.find[PasswordInfo](loginInfo)

Method threw 'com.mohiva.play.silhouette.api.exceptions.ConfigurationException' exception. Cannot find auth info of type: class java.lang.Object; Please configure the DAO for this type

I made the change to AuthInfoRepository.find because the implementation had to allow for finding by userId. This is what I have:

class DelegableAuthInfoRepository(daos: DelegableAuthInfoDAO[_]*)(implicit ec: ExecutionContext) extends AuthInfoRepository {
  override def find[T <: AuthInfo](loginInfo: LoginInfo)(implicit tag: ClassTag[T]): Future[Option[T]] = {
    daos.find(_.classTag == tag) match {
      case Some(dao) => dao.find(loginInfo).map(_.map(_.asInstanceOf[T]))
      case _ => throw new ConfigurationException(FindError.format(tag.runtimeClass))
    }
  }
}

Suggestions?

Mike

[Michael Slinn]

The last bit I posted showed the wrong file. This is what I meant to show:

class DBAuthInfoDAO[T <: AuthInfo](implicit mkT: PasswordInfo => T) extends DelegableAuthInfoDAO[PasswordInfo] {

  override def find(loginInfo: LoginInfo) = Future.successful {
    for {
      user  <- Users.findByUserId(loginInfo.providerKey)
    } yield PasswordInfo(BCryptPasswordHasher.ID, user.password)
  }

Mike

[Christian Kaps]

Hi Mike,

I do not understand why do you try to change the DAO! In my opinion this isn't necessary, because during registration you can store the the LoginInfo with the user ID instead of the email. Then you have the mapping:

LoginInfo(providerID, userID.toString) -> PasswordInfo(...)

directly in the database.

This should solve all your problems.

Best regards,

Christian

[Michael Slinn]

The comments say to replace InMemoryAuthInfoDAO with a threadsafe version, and presumably the replacement should be backed by a database. If I preserve the InMemoryAuthInfoDAO signatures, like this:

class DBAuthInfoDAO[T <: AuthInfo: ClassTag] extends DelegableAuthInfoDAO[T] {

  override def find(loginInfo: LoginInfo): Future[Option[T]] = Future.successful {

    for {
      user  <- Users.findByUserId(loginInfo.providerKey)
    } yield PasswordInfo(BCryptPasswordHasher.ID, user.password)
  }
}

Then I get:

[error] DBAuthInfoDAO.scala:44: type mismatch;
[error]  found   : com.mohiva.play.silhouette.api.util.PasswordInfo
[error]  required: T
[error]     } yield PasswordInfo(BCryptPasswordHasher.ID, user.password)

Thanks,
Mike

[Michael Slinn]

Cheating (casting result to Option[T]) works:

  override def find(loginInfo: LoginInfo): Future[Option[T]] = Future.successful {

    val result: Option[PasswordInfo] = for {

      user <- Users.findByUserId(loginInfo.providerKey)
    } yield PasswordInfo(BCryptPasswordHasher.ID, user.password)

    result.asInstanceOf[Option[T]]
  }

It would be nice if there was a way to make this work without calling asInstanceOf.

Next issue: I see LoginController.submit obtains a CookieAuthenticator and publishes a LoginEvent, then embeds the CookieAuthenticator in the redirected response. This tells me the user authenticates properly. However, the UserAware request handler for the redirect does not obtain an Authenticator (userAwareRequest.authenticator is None).

The CookieAuthenticator's expirationDateTime is a month in the future, maxAge is Some(2592000000 milliseconds) and idleTimeout is Some(432000000 milliseconds). In silhouette.conf: silhouette.authenticator { secureCookie = false, httpOnlyCookie = true, cookieName = "authenticator", cookiePath = "/" }

I've disabled all the filters to simplify debugging. What should I look for to track this problem down?

Thanks,
Mike

[Michael Slinn]

I solved the problem, it was an improperly generated cookie. I found that silhouette.env.authenticatorService.init(cookieAuthenticator) always returns a secure cookie, which means the cookie will be rejected unless https is used. For testing on localhost, setting Domain=None was also required. This is my kludge for LoginController:

silhouette.env.authenticatorService.init(cookieAuthenticator).flatMap { cookie =>
  val cookieMightBeSecure = cookie.copy(secure=request.secure, domain=None)
  silhouette.env.authenticatorService.embed(cookieMightBeSecure, result)
}

Seems like authenticatorService.init should not always returns a secure cookie.

Next I need to figure out the best incantation for configuring silhouette.authenticator.cookieDomain so I get proper values back when running on localhost and in production. The Silhouette docs say "This should be disabled for testing on localhost without SSL, otherwise cookie couldn't be set" but the docs do not the configuration value(s) that disable cookieDomain. I'll try false, the empty string, etc in the hope that I'll eventually stumble upon a way to disable cookieDomain.

Mike

[Christian Kaps]

Hi,

Users.findByUserId(loginInfo.providerKey)

I would not do that because you use global state which is not mockable for testing. It can also have unexpected side-effects. I also do not understand why do you override the DAO. Please could you describe your scenario in a few sentences? Because, I think you are doing it in the wrong way.

Next I need to figure out the best incantation for configuring silhouette.authenticator.cookieDomain so I get proper values back when running on localhost and in production. The Silhouette docs say "This should be disabled for testing on localhost without SSL, otherwise cookie couldn't be set" but the docs do not the configuration value(s) that disablecookieDomain. I'll try false, the empty string, etc in the hope that I'll eventually stumble upon a way to disable cookieDomain.

If you do not set the cookieDomain in the config, then it will be None. You could also provide different config files when running in development or production. The seed template already uses this approach.

Best regards,

Christian

[Michael Slinn]

On Sunday, August 28, 2016 at 3:28:43 AM UTC-7, Christian Kaps wrote:

Hi,

Users.findByUserId(loginInfo.providerKey)

I would not do that because you use global state which is not mockable for testing. It can also have unexpected side-effects. I also do not understand why do you override the DAO. Please could you describe your scenario in a few sentences? Because, I think you are doing it in the wrong way.

This code predates Silhouette, is battle tested and has extensive unit tests. There is a User type and a RichUser type, so there is no requirement for mocking. The combination is flexible, robust and efficient. It is merely a different approach, and I'm not going to completely rewrite the entire application for consistency with how Silhouette is written. Let's move on to the issue at hand.

 

Next I need to figure out the best incantation for configuring silhouette.authenticator.cookieDomain so I get proper values back when running on localhost and in production. The Silhouette docs say "This should be disabled for testing on localhost without SSL, otherwise cookie couldn't be set" but the docs do not the configuration value(s) that disablecookieDomain. I'll try false, the empty string, etc in the hope that I'll eventually stumble upon a way to disable cookieDomain.

If you do not set the cookieDomain in the config, then it will be None. You could also provide different config files when running in development or production. The seed template already uses this approach.

It is better to have only one configuration, and to give the required value. I'll deal with this another way, and will continue to use the kludge I described to overcome the authenticatorService.init bug.

Mike

[Christian Kaps]

This code predates Silhouette, is battle tested and has extensive unit tests. There is a User type and a RichUser type, so there is no requirement for mocking. The combination is flexible, robust and efficient. It is merely a different approach, and I'm not going to completely rewrite the entire application for consistency with how Silhouette is written. Let's move on to the issue at hand.

 

This has nothing to do with how Silhouette is written. This are common design principles used for object oriented programming. I think you are following the Play development. And I think you know that the Play team tries to eliminate global state across the whole framework since a year now. I would rather say, that dependency injection only exists to solve the issue of global state. 

Anyway, if you do not have the time to change this immediately, you should put it on your to-do list.

It is better to have only one configuration, and to give the required value.

When you say that it's better, do you say it because it's your personal preference? Or do you say it because the Play team advises it?

I'll deal with this another way, and will continue to use the kludge I described to overcome the authenticatorService.init bug.

The authenticatorService.init method doesn't have a bug with secure cookies. The init method uses the value from the config. And it works completely correct with the seed template on localhost. Maybe you have a configuration issue?

Best regards,

Christian