! routing: Render WWW-Authenticate header also for rejected credent…

…ials, fixes #188

When authentication fails, browsers didn't offer a possibilty to re-enter new credentials because of the missing WWW-Authenticate header. RFC2617 states: "If the origin server does not wish to accept the credentials sent with a request, it SHOULD return a 401 (Unauthorized) response. The response MUST include a WWW-Authenticate header field containing at least one (possibly new) challenge applicable to the requested resource." This patch adds the missing header, so users can attempt again to authenticate themselves.

This patch merges the AuthenticationRequiredRejection into the AuthenticationFailedRejection and moves the generation of the headers into the HttpAuthenticator.

spray-routing-tests/src/test/scala/spray/routing/SecurityDirectivesSpec.scala

@@ -21,49 +21,51 @@ import akka.event.NoLogging
 import spray.routing.authentication._
 import spray.http._
 import HttpHeaders._
+import AuthenticationFailedRejection._
 
 class SecurityDirectivesSpec extends RoutingSpec {
 
-  val dontAuth = UserPassAuthenticator[BasicUserContext](_ ⇒ Future.successful(None))
+  val dontAuth = BasicAuth(UserPassAuthenticator[BasicUserContext](_ ⇒ Future.successful(None)), "Realm")
 
-  val doAuth = UserPassAuthenticator[BasicUserContext] { userPassOption ⇒
+  val doAuth = BasicAuth(UserPassAuthenticator[BasicUserContext] { userPassOption ⇒
     Future.successful(Some(BasicUserContext(userPassOption.get.user)))
-  }
+  }, "Realm")
 
   "the 'authenticate(BasicAuth())' directive" should {
-    "reject requests without Authorization header with an AuthenticationRequiredRejection" in {
+    "reject requests without Authorization header with an AuthenticationFailedRejection" in {
       Get() ~> {
-        authenticate(BasicAuth(dontAuth, "Realm")) { echoComplete }
-      } ~> check { rejection === AuthenticationRequiredRejection("Basic", "Realm", Map.empty) }
+        authenticate(dontAuth) { echoComplete }
+      } ~> check { rejection === AuthenticationFailedRejection(CredentialsMissing, dontAuth) }
     }
-    "reject unauthenticated requests with Authorization header with an AuthorizationFailedRejection" in {
+    "reject unauthenticated requests with Authorization header with an AuthenticationFailedRejection" in {
       Get() ~> Authorization(BasicHttpCredentials("Bob", "")) ~> {
-        authenticate(BasicAuth(dontAuth, "Realm")) { echoComplete }
-      } ~> check { rejection === AuthenticationFailedRejection("Realm") }
+        authenticate(dontAuth) { echoComplete }
+      } ~> check { rejection === AuthenticationFailedRejection(CredentialsRejected, dontAuth) }
     }
     "extract the object representing the user identity created by successful authentication" in {
       Get() ~> Authorization(BasicHttpCredentials("Alice", "")) ~> {
-        authenticate(BasicAuth(doAuth, "Realm")) { echoComplete }
+        authenticate(doAuth) { echoComplete }
       } ~> check { entityAs[String] === "BasicUserContext(Alice)" }
     }
     "properly handle exceptions thrown in its inner route" in {
       object TestException extends spray.util.SingletonException
       Get() ~> Authorization(BasicHttpCredentials("Alice", "")) ~> {
         handleExceptions(ExceptionHandler.default) {
-          authenticate(BasicAuth(doAuth, "Realm")) { _ ⇒ throw TestException }
+          authenticate(doAuth) { _ ⇒ throw TestException }
         }
       } ~> check { status === StatusCodes.InternalServerError }
     }
   }
 
   "the 'authenticate(<ContextAuthenticator>)' directive" should {
+    case object AuthenticationRejection extends Rejection
+
     val myAuthenticator: ContextAuthenticator[Int] = ctx ⇒ Future {
-      Either.cond(ctx.request.uri.authority.host == Uri.NamedHost("spray.io"), 42,
-        AuthenticationRequiredRejection("my-scheme", "MyRealm", Map()))
+      Either.cond(ctx.request.uri.authority.host == Uri.NamedHost("spray.io"), 42, AuthenticationRejection)
     }
     "reject requests not satisfying the filter condition" in {
       Get() ~> authenticate(myAuthenticator) { echoComplete } ~>
-        check { rejection === AuthenticationRequiredRejection("my-scheme", "MyRealm", Map.empty) }
+        check { rejection === AuthenticationRejection }
     }
     "pass on the authenticator extraction if the filter conditions is met" in {
       Get() ~> Host("spray.io") ~> authenticate(myAuthenticator) { echoComplete } ~>

spray-routing/src/main/scala/spray/routing/Rejection.scala

@@ -17,6 +17,7 @@
 package spray.routing
 
 import spray.http._
+import spray.routing.authentication.HttpAuthenticator
 
 /**
  * A rejection encapsulates a specific reason why a Route was not able to handle a request. Rejections are gathered
@@ -115,24 +116,38 @@ case class UnacceptedResponseContentTypeRejection(supported: Seq[ContentType]) e
 case class UnacceptedResponseEncodingRejection(supported: HttpEncoding) extends Rejection
 
 /**
- * Rejection created by the 'authenticate' directive.
- * Signals that the request was rejected because the user could not be authenticated.
+ * Rejection created by an [[spray.routing.authentication.HttpAuthenticator]].
+ * Signals that the request was rejected because the user could not be authenticated. The reason for the rejection is
+ * specified in the cause.
  */
-case class AuthenticationFailedRejection(realm: String) extends Rejection
+case class AuthenticationFailedRejection(cause: AuthenticationFailedRejection.Cause,
+                                         authenticator: HttpAuthenticator[_]) extends Rejection
+
+object AuthenticationFailedRejection {
+  /**
+   * Signals the cause of the failed authentication.
+   */
+  sealed trait Cause
+
+  /**
+   * Signals the cause of the rejecting was that the user could not be authenticated, because the `WWW-Authenticate`
+   * header was not supplied.
+   */
+  case object CredentialsMissing extends Cause
+
+  /**
+   * Signals the cause of the rejecting was that the user could not be authenticated, because the supplied credentials
+   * are invalid.
+   */
+  case object CredentialsRejected extends Cause
+}
 
 /**
  * Rejection created by the 'authorize' directive.
  * Signals that the request was rejected because the user is not authorized.
  */
 case object AuthorizationFailedRejection extends Rejection
 
-/**
- * Rejection created by the 'authenticate' directive.
- * Signals that the request was rejected because no authorization was supplied
- */
-case class AuthenticationRequiredRejection(scheme: String, realm: String, params: Map[String, String] = Map.empty)
-  extends Rejection
-
 /**
  * Rejection created by the `cookie` directive.
  * Signals that the request was rejected because a cookie was not found.

spray-routing/src/main/scala/spray/routing/RejectionHandler.scala

@@ -20,6 +20,7 @@ import spray.http._
 import StatusCodes._
 import HttpHeaders._
 import spray.routing.directives.RouteDirectives._
+import AuthenticationFailedRejection._
 
 trait RejectionHandler extends RejectionHandler.PF
 
@@ -35,12 +36,12 @@ object RejectionHandler {
   implicit val Default = apply {
     case Nil ⇒ complete(NotFound, "The requested resource could not be found.")
 
-    case AuthenticationRequiredRejection(scheme, realm, params) :: _ ⇒
-      complete(Unauthorized, `WWW-Authenticate`(HttpChallenge(scheme, realm, params)) :: Nil,
-        "The resource requires authentication, which was not supplied with the request")
-
-    case AuthenticationFailedRejection(realm) :: _ ⇒
-      complete(Unauthorized, "The supplied authentication is invalid")
+    case AuthenticationFailedRejection(cause, authenticator) :: _ ⇒
+      val rejectionMessage = cause match {
+        case CredentialsMissing  ⇒ "The resource requires authentication, which was not supplied with the request"
+        case CredentialsRejected ⇒ "The supplied authentication is invalid"
+      }
+      ctx ⇒ ctx.complete(Unauthorized, authenticator.getChallengeHeaders(ctx.request), rejectionMessage)
 
     case AuthorizationFailedRejection :: _ ⇒
       complete(Forbidden, "The supplied authentication is not authorized to access this resource")

spray-routing/src/main/scala/spray/routing/authentication/HttpAuthenticator.scala

@@ -22,6 +22,7 @@ import scala.concurrent.{ ExecutionContext, Future }
 import spray.http._
 import spray.util._
 import HttpHeaders._
+import AuthenticationFailedRejection._
 
 /**
  * An HttpAuthenticator is a ContextAuthenticator that uses credentials passed to the server via the
@@ -34,19 +35,17 @@ trait HttpAuthenticator[U] extends ContextAuthenticator[U] {
     val credentials = authHeader.map { case Authorization(creds) ⇒ creds }
     authenticate(credentials, ctx) map {
       case Some(userContext) ⇒ Right(userContext)
-      case None ⇒ Left {
-        if (authHeader.isEmpty) AuthenticationRequiredRejection(scheme, realm, params(ctx))
-        else AuthenticationFailedRejection(realm)
-      }
+      case None ⇒
+        val cause = if (authHeader.isEmpty) CredentialsMissing else CredentialsRejected
+        Left(AuthenticationFailedRejection(cause, this))
     }
   }
 
   implicit def executionContext: ExecutionContext
-  def scheme: String
-  def realm: String
-  def params(ctx: RequestContext): Map[String, String]
 
   def authenticate(credentials: Option[HttpCredentials], ctx: RequestContext): Future[Option[U]]
+
+  def getChallengeHeaders(httpRequest: HttpRequest): List[HttpHeader]
 }
 
 /**
@@ -55,9 +54,6 @@ trait HttpAuthenticator[U] extends ContextAuthenticator[U] {
 class BasicHttpAuthenticator[U](val realm: String, val userPassAuthenticator: UserPassAuthenticator[U])(implicit val executionContext: ExecutionContext)
     extends HttpAuthenticator[U] {
 
-  def scheme = "Basic"
-  def params(ctx: RequestContext) = Map.empty
-
   def authenticate(credentials: Option[HttpCredentials], ctx: RequestContext) = {
     userPassAuthenticator {
       credentials.flatMap {
@@ -66,6 +62,10 @@ class BasicHttpAuthenticator[U](val realm: String, val userPassAuthenticator: Us
       }
     }
   }
+
+  def getChallengeHeaders(httpRequest: HttpRequest) =
+    `WWW-Authenticate`(HttpChallenge(scheme = "Basic", realm = realm, params = Map.empty)) :: Nil
+
 }
 
 object BasicAuth {