Authorization Header Specification fixes (#110)

* Authorization Header Specification fixes

* The root of this PR stems from this part of the specification: https://tools.ietf.org/html/rfc6749#section-5.2 This section states that in the event a client is authenticating via the “Authorization” header field, then the authorization server MUST respond with a 401 Unauthorized in the event the client does not have access. This validation MUST occur before any other validations occur since this is an authentication state via a standard header. Only after the authorization has validated - assuming via the Authorization header - can the rest of the processing and validation continue.
* Added some documentation comments on the AuthorizationHandler trait further explaining what a client validation request must do.
* Removed some unnecessary overrides for fetching the clientCrential in the request type case classes as they are unnecessary since they don’t override anything.
* Cleaned up tests to reflect changes for Authorization Header changes, added a few new tests to cover the new case.

* changing to `parseClientCredential` and `maybeValidatedClientCred` per convo on PR

Author: rmmeans

scala-oauth2-core/src/main/scala/scalaoauth2/provider/AuthorizationHandler.scala

@@ -55,6 +55,9 @@ trait AuthorizationHandler[U] {
 
   /**
    * Verify proper client with parameters for issue an access token.
+   * Note that per the OAuth Specification, a Client may be valid if it only contains a client ID but no client
+   * secret (common with Public Clients). However, if the registered client has a client secret value the specification
+   * requires that a client secret must always be provided and verified for that client ID.
    *
    * @param request Request sent by client.
    * @return true if request is a regular client, false if request is a illegal client.

scala-oauth2-core/src/main/scala/scalaoauth2/provider/AuthorizationRequest.scala

@@ -12,24 +12,32 @@ class AuthorizationRequest(headers: Map[String, Seq[String]], params: Map[String
 
   def grantType: String = requireParam("grant_type")
 
-  lazy val clientCredential: Option[ClientCredential] =
+  def parseClientCredential: Option[Either[InvalidClient, ClientCredential]] =
     findAuthorization
-      .flatMap(clientCredentialByAuthorization)
-      .orElse(clientCredentialByParam)
-
-  private def findAuthorization = for {
-    authorization <- header("Authorization")
-    matcher <- """^\s*Basic\s+(.+?)\s*$""".r.findFirstMatchIn(authorization)
-  } yield matcher.group(1)
-
-  private def clientCredentialByAuthorization(s: String) =
+      .flatMap(x => Some(x.fold(
+        left => Left(left),
+        header => clientCredentialByAuthorization(header)
+      )))
+      .orElse(clientCredentialByParam.map(Right(_)))
+
+  private def findAuthorization: Option[Either[InvalidClient, String]] = {
+    header("Authorization").map { auth =>
+      val basicAuthCred = for {
+        matcher <- """^\s*Basic\s+(.+?)\s*$""".r.findFirstMatchIn(auth)
+      } yield matcher.group(1)
+
+      basicAuthCred.fold[Either[InvalidClient, String]](Left(new InvalidClient("Authorization header could not be parsed")))(x => Right(x))
+    }
+  }
+
+  private def clientCredentialByAuthorization(s: String): Either[InvalidClient, ClientCredential] =
     Try(new String(Base64.getDecoder.decode(s), "UTF-8"))
       .map(_.split(":", 2))
       .getOrElse(Array.empty) match {
         case Array(clientId, clientSecret) =>
-          Some(ClientCredential(clientId, if (clientSecret.isEmpty) None else Some(clientSecret)))
+          Right(ClientCredential(clientId, if (clientSecret.isEmpty) None else Some(clientSecret)))
         case _ =>
-          None
+          Left(new InvalidClient())
       }
 
   private def clientCredentialByParam = param("client_id").map(ClientCredential(_, param("client_secret")))
@@ -44,8 +52,6 @@ case class RefreshTokenRequest(request: AuthorizationRequest) extends Authorizat
    * @throws InvalidRequest if the parameter is not found
    */
   def refreshToken: String = requireParam("refresh_token")
-
-  override lazy val clientCredential = request.clientCredential
 }
 
 case class PasswordRequest(request: AuthorizationRequest) extends AuthorizationRequest(request.headers, request.params) {
@@ -64,13 +70,9 @@ case class PasswordRequest(request: AuthorizationRequest) extends AuthorizationR
    * @throws InvalidRequest if the parameter is not found
    */
   def password = requireParam("password")
-
-  override lazy val clientCredential = request.clientCredential
 }
 
-case class ClientCredentialsRequest(request: AuthorizationRequest) extends AuthorizationRequest(request.headers, request.params) {
-  override lazy val clientCredential = request.clientCredential
-}
+case class ClientCredentialsRequest(request: AuthorizationRequest) extends AuthorizationRequest(request.headers, request.params)
 
 case class AuthorizationCodeRequest(request: AuthorizationRequest) extends AuthorizationRequest(request.headers, request.params) {
   /**
@@ -87,11 +89,6 @@ case class AuthorizationCodeRequest(request: AuthorizationRequest) extends Autho
    * @return redirect_uri
    */
   def redirectUri: Option[String] = param("redirect_uri")
-
-  override lazy val clientCredential = request.clientCredential
-
 }
 
-case class ImplicitRequest(request: AuthorizationRequest) extends AuthorizationRequest(request.headers, request.params) {
-  override lazy val clientCredential = request.clientCredential
-}
+case class ImplicitRequest(request: AuthorizationRequest) extends AuthorizationRequest(request.headers, request.params)

scala-oauth2-core/src/main/scala/scalaoauth2/provider/GrantHandler.scala

@@ -20,7 +20,7 @@ trait GrantHandler {
    */
   def clientCredentialRequired = true
 
-  def handleRequest[U](request: AuthorizationRequest, authorizationHandler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]]
+  def handleRequest[U](maybeValidatedClientCred: Option[ClientCredential], request: AuthorizationRequest, authorizationHandler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]]
 
   /**
    * Returns valid access token.
@@ -53,51 +53,51 @@ trait GrantHandler {
 
 class RefreshToken extends GrantHandler {
 
-  override def handleRequest[U](request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+  override def handleRequest[U](maybeValidatedClientCred: Option[ClientCredential], request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+    val clientId = maybeValidatedClientCred.getOrElse(throw new InvalidRequest("Client credential is required")).clientId
     val refreshTokenRequest = RefreshTokenRequest(request)
-    val clientCredential = refreshTokenRequest.clientCredential.getOrElse(throw new InvalidRequest("Client credential is required"))
     val refreshToken = refreshTokenRequest.refreshToken
 
     handler.findAuthInfoByRefreshToken(refreshToken).flatMap { authInfoOption =>
       val authInfo = authInfoOption.getOrElse(throw new InvalidGrant("Authorized information is not found by the refresh token"))
-      if (!authInfo.clientId.contains(clientCredential.clientId)) {
-        throw new InvalidClient
-      }
-
+      if (!authInfo.clientId.contains(clientId)) throw new InvalidClient
       handler.refreshAccessToken(authInfo, refreshToken).map(createGrantHandlerResult(authInfo, _))
     }
   }
 }
 
 class Password extends GrantHandler {
 
-  override def handleRequest[U](request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
-    val passwordRequest = PasswordRequest(request)
-    if (clientCredentialRequired && passwordRequest.clientCredential.isEmpty) {
+  override def handleRequest[U](maybeValidatedClientCred: Option[ClientCredential], request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+    /**
+     * Given that client credentials may be optional, if they are required, they must be fully validated before
+     * further processing.
+     */
+    if (clientCredentialRequired && maybeValidatedClientCred.isEmpty) {
       throw new InvalidRequest("Client credential is required")
-    }
-
-    handler.findUser(passwordRequest).flatMap { maybeUser =>
-      val user = maybeUser.getOrElse(throw new InvalidGrant("username or password is incorrect"))
-      val scope = passwordRequest.scope
-      val maybeClientId = passwordRequest.clientCredential.map(_.clientId)
-      val authInfo = AuthInfo(user, maybeClientId, scope, None)
-
-      issueAccessToken(handler, authInfo)
+    } else {
+      val passwordRequest = PasswordRequest(request)
+      handler.findUser(passwordRequest).flatMap { maybeUser =>
+        val user = maybeUser.getOrElse(throw new InvalidGrant("username or password is incorrect"))
+        val scope = passwordRequest.scope
+        val authInfo = AuthInfo(user, maybeValidatedClientCred.map(_.clientId), scope, None)
+
+        issueAccessToken(handler, authInfo)
+      }
     }
   }
 }
 
 class ClientCredentials extends GrantHandler {
 
-  override def handleRequest[U](request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+  override def handleRequest[U](maybeValidatedClientCred: Option[ClientCredential], request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+    val clientId = maybeValidatedClientCred.getOrElse(throw new InvalidRequest("Client credential is required")).clientId
     val clientCredentialsRequest = ClientCredentialsRequest(request)
-    val clientCredential = clientCredentialsRequest.clientCredential.getOrElse(throw new InvalidRequest("Client credential is required"))
     val scope = clientCredentialsRequest.scope
 
     handler.findUser(clientCredentialsRequest).flatMap { optionalUser =>
       val user = optionalUser.getOrElse(throw new InvalidGrant("client_id or client_secret or scope is incorrect"))
-      val authInfo = AuthInfo(user, Some(clientCredential.clientId), scope, None)
+      val authInfo = AuthInfo(user, Some(clientId), scope, None)
 
       issueAccessToken(handler, authInfo)
     }
@@ -107,10 +107,9 @@ class ClientCredentials extends GrantHandler {
 
 class AuthorizationCode extends GrantHandler {
 
-  override def handleRequest[U](request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+  override def handleRequest[U](maybeValidatedClientCred: Option[ClientCredential], request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+    val clientId = maybeValidatedClientCred.getOrElse(throw new InvalidRequest("Client credential is required")).clientId
     val authorizationCodeRequest = AuthorizationCodeRequest(request)
-    val clientCredential = authorizationCodeRequest.clientCredential.getOrElse(throw new InvalidRequest("Client credential is required"))
-    val clientId = clientCredential.clientId
     val code = authorizationCodeRequest.code
     val redirectUri = authorizationCodeRequest.redirectUri
 
@@ -136,14 +135,14 @@ class AuthorizationCode extends GrantHandler {
 
 class Implicit extends GrantHandler {
 
-  override def handleRequest[U](request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+  override def handleRequest[U](maybeValidatedClientCred: Option[ClientCredential], request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[GrantHandlerResult[U]] = {
+    val clientId = maybeValidatedClientCred.getOrElse(throw new InvalidRequest("Client credential is required")).clientId
     val implicitRequest = ImplicitRequest(request)
-    val clientCredential = implicitRequest.clientCredential.getOrElse(throw new InvalidRequest("Client credential is required"))
 
     handler.findUser(implicitRequest).flatMap { maybeUser =>
       val user = maybeUser.getOrElse(throw new InvalidGrant("user cannot be authenticated"))
       val scope = implicitRequest.scope
-      val authInfo = AuthInfo(user, Some(clientCredential.clientId), scope, None)
+      val authInfo = AuthInfo(user, Some(clientId), scope, None)
 
       issueAccessToken(handler, authInfo)
     }

scala-oauth2-core/src/main/scala/scalaoauth2/provider/TokenEndpoint.scala

@@ -7,23 +7,29 @@ trait TokenEndpoint {
 
   def handleRequest[U](request: AuthorizationRequest, handler: AuthorizationHandler[U])(implicit ctx: ExecutionContext): Future[Either[OAuthError, GrantHandlerResult[U]]] = try {
     val grantType = request.grantType
-    val grantHandler = handlers.getOrElse(grantType, throw new UnsupportedGrantType(s"${grantType} is not supported"))
+    val grantHandler = () => handlers.getOrElse(grantType, throw new UnsupportedGrantType(s"$grantType is not supported"))
 
-    request.clientCredential.map { clientCredential =>
-      handler.validateClient(request).flatMap { validClient =>
-        if (!validClient) {
-          Future.successful(Left(new InvalidClient("Invalid client is detected")))
-        } else {
-          grantHandler.handleRequest(request, handler).map(Right(_))
+    request.parseClientCredential.map { maybeCredential =>
+      maybeCredential.fold(
+        invalid => Future.successful(Left(invalid)),
+        clientCredential => {
+          handler.validateClient(request).flatMap { isValidClient =>
+            if (!isValidClient) {
+              Future.successful(Left(new InvalidClient("Invalid client or client is not authorized")))
+            } else {
+              grantHandler().handleRequest(Some(clientCredential), request, handler).map(Right(_))
+            }
+          }.recover {
+            case e: OAuthError => Left(e)
+          }
         }
-      }.recover {
-        case e: OAuthError => Left(e)
-      }
+      )
     }.getOrElse {
-      if (grantHandler.clientCredentialRequired) {
+      val gh = grantHandler()
+      if (gh.clientCredentialRequired) {
         throw new InvalidRequest("Client credential is not found")
       } else {
-        grantHandler.handleRequest(request, handler).map(Right(_)).recover {
+        gh.handleRequest(None, request, handler).map(Right(_)).recover {
           case e: OAuthError => Left(e)
         }
       }

scala-oauth2-core/src/test/scala/scalaoauth2/provider/AuthorizationCodeSpec.scala

@@ -14,7 +14,8 @@ class AuthorizationCodeSpec extends FlatSpec with ScalaFutures with OptionValues
     val authorizationCode = new AuthorizationCode()
     val request = new AuthorizationRequest(Map(), Map("client_id" -> Seq("clientId1"), "client_secret" -> Seq("clientSecret1"), "code" -> Seq("code1"), "redirect_uri" -> Seq("http://example.com/")))
     var codeDeleted: Boolean = false
-    val f = authorizationCode.handleRequest(request, new MockDataHandler() {
+    val clientCred = request.parseClientCredential.fold[Option[ClientCredential]](None)(_.fold(_ => None, c => Some(c)))
+    val f = authorizationCode.handleRequest(clientCred, request, new MockDataHandler() {
 
       override def findAuthInfoByCode(code: String): Future[Option[AuthInfo[User]]] = Future.successful(Some(
         AuthInfo(user = MockUser(10000, "username"), clientId = Some("clientId1"), scope = Some("all"), redirectUri = Some("http://example.com/"))
@@ -42,7 +43,8 @@ class AuthorizationCodeSpec extends FlatSpec with ScalaFutures with OptionValues
   it should "handle request if redirectUrl is none" in {
     val authorizationCode = new AuthorizationCode()
     val request = new AuthorizationRequest(Map(), Map("client_id" -> Seq("clientId1"), "client_secret" -> Seq("clientSecret1"), "code" -> Seq("code1"), "redirect_uri" -> Seq("http://example.com/")))
-    val f = authorizationCode.handleRequest(request, new MockDataHandler() {
+    val clientCred = request.parseClientCredential.fold[Option[ClientCredential]](None)(_.fold(_ => None, c => Some(c)))
+    val f = authorizationCode.handleRequest(clientCred, request, new MockDataHandler() {
 
       override def findAuthInfoByCode(code: String): Future[Option[AuthInfo[MockUser]]] = Future.successful(Some(
         AuthInfo(user = MockUser(10000, "username"), clientId = Some("clientId1"), scope = Some("all"), redirectUri = None)
@@ -63,7 +65,8 @@ class AuthorizationCodeSpec extends FlatSpec with ScalaFutures with OptionValues
   it should "return a Failure Future" in {
     val authorizationCode = new AuthorizationCode()
     val request = new AuthorizationRequest(Map(), Map("client_id" -> Seq("clientId1"), "client_secret" -> Seq("clientSecret1"), "code" -> Seq("code1"), "redirect_uri" -> Seq("http://example.com/")))
-    val f = authorizationCode.handleRequest(request, new MockDataHandler() {
+    val clientCred = request.parseClientCredential.fold[Option[ClientCredential]](None)(_.fold(_ => None, c => Some(c)))
+    val f = authorizationCode.handleRequest(clientCred, request, new MockDataHandler() {
 
       override def findAuthInfoByCode(code: String): Future[Option[AuthInfo[User]]] = Future.successful(Some(
         AuthInfo(user = MockUser(10000, "username"), clientId = Some("clientId1"), scope = Some("all"), redirectUri = Some("http://example.com/"))