From 33f2bebe1f2fb1f314eb6eee6e423927169aa711 Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Sat, 3 Jul 2021 12:12:58 +0200 Subject: [PATCH] more relaxed legacy oauth validation (#9327) --- app/controllers/OAuth.scala | 32 ++++++++++++++----- .../oauth/src/main/AccessTokenRequest.scala | 32 +++++++++++++------ modules/oauth/src/main/AuthorizationApi.scala | 12 +++---- modules/oauth/src/main/LegacyClientApi.scala | 14 ++++++-- 4 files changed, 64 insertions(+), 26 deletions(-) diff --git a/app/controllers/OAuth.scala b/app/controllers/OAuth.scala index 9707847a30..1818bb34e7 100644 --- a/app/controllers/OAuth.scala +++ b/app/controllers/OAuth.scala @@ -67,9 +67,9 @@ final class OAuth(env: Env) extends LilaController(env) { "grant_type" -> optional(text), "code" -> optional(text), "code_verifier" -> optional(text), - "client_secret" -> optional(text), + "client_id" -> optional(text), "redirect_uri" -> optional(text), - "client_id" -> optional(text) + "client_secret" -> optional(text) )(AccessTokenRequest.Raw.apply)(AccessTokenRequest.Raw.unapply) ) @@ -86,11 +86,6 @@ final class OAuth(env: Env) extends LilaController(env) { "token_type" -> "Bearer", "access_token" -> token.id.value ) - .add( - "refresh_token" -> prepared.clientSecret.map(_ => - s"invalid_for_bc_${lila.common.ThreadLocalRandom.nextString(17)}" - ) - ) .add("expires_in" -> token.expires.map(_.getSeconds - nowSeconds)) ) } @@ -100,7 +95,28 @@ final class OAuth(env: Env) extends LilaController(env) { } } - def legacyTokenApply = tokenApply + def legacyTokenApply = + Action.async(parse.form(accessTokenRequestForm)) { implicit req => + req.body.prepareLegacy match { + case Validated.Valid(prepared) => + env.oAuth.authorizationApi.consume(prepared) flatMap { + case Validated.Valid(granted) => + env.oAuth.tokenApi.create(granted) map { token => + Ok( + Json + .obj( + "token_type" -> "Bearer", + "access_token" -> token.id.value, + "refresh_token" -> s"invalid_for_bc_${lila.common.ThreadLocalRandom.nextString(17)}" + ) + .add("expires_in" -> token.expires.map(_.getSeconds - nowSeconds)) + ) + } + case Validated.Invalid(err) => BadRequest(err.toJson).fuccess + } + case Validated.Invalid(err) => BadRequest(err.toJson).fuccess + } + } def tokenRevoke = Scoped() { implicit req => _ => diff --git a/modules/oauth/src/main/AccessTokenRequest.scala b/modules/oauth/src/main/AccessTokenRequest.scala index 386d0630c7..000e9b32ff 100644 --- a/modules/oauth/src/main/AccessTokenRequest.scala +++ b/modules/oauth/src/main/AccessTokenRequest.scala @@ -10,26 +10,38 @@ object AccessTokenRequest { grantType: Option[String], code: Option[String], codeVerifier: Option[String], - clientSecret: Option[String], + clientId: Option[String], redirectUri: Option[String], - clientId: Option[String] + clientSecret: Option[String] ) { def prepare: Validated[Error, Prepared] = for { - redirectUri <- redirectUri.map(UncheckedRedirectUri.apply).toValid(Error.RedirectUriRequired) + grantType <- grantType.toValid(Error.GrantTypeRequired).andThen(GrantType.from) + code <- code.map(AuthorizationCode.apply).toValid(Error.CodeRequired) + codeVerifier <- codeVerifier + .toValid(Protocol.Error.CodeVerifierRequired) + .andThen(Protocol.CodeVerifier.from) clientId <- clientId.map(ClientId.apply).toValid(Error.ClientIdRequired) - grantType <- grantType.toValid(Error.GrantTypeRequired).andThen(GrantType.from) - code <- code.map(AuthorizationCode.apply).toValid(Error.CodeRequired) - } yield Prepared(grantType, code, codeVerifier, clientSecret, redirectUri, clientId) + redirectUri <- redirectUri.map(UncheckedRedirectUri.apply).toValid(Error.RedirectUriRequired) + } yield Prepared(grantType, code, codeVerifier.some, clientId.some, redirectUri.some, None) + + def prepareLegacy: Validated[Error, Prepared] = + for { + grantType <- grantType.toValid(Error.GrantTypeRequired).andThen(GrantType.from) + code <- code.map(AuthorizationCode.apply).toValid(Error.CodeRequired) + clientSecret <- clientSecret + .map(LegacyClientApi.ClientSecret) + .toValid(LegacyClientApi.ClientSecretRequired) + } yield Prepared(grantType, code, None, clientId.map(ClientId.apply), None, clientSecret.some) } case class Prepared( grantType: GrantType, code: AuthorizationCode, - codeVerifier: Option[String], - clientSecret: Option[String], - redirectUri: UncheckedRedirectUri, - clientId: ClientId + codeVerifier: Option[CodeVerifier], + clientId: Option[ClientId], + redirectUri: Option[UncheckedRedirectUri], + clientSecret: Option[LegacyClientApi.ClientSecret] ) case class Granted( diff --git a/modules/oauth/src/main/AuthorizationApi.scala b/modules/oauth/src/main/AuthorizationApi.scala index 3d10f5b32b..2fcb810f7d 100644 --- a/modules/oauth/src/main/AuthorizationApi.scala +++ b/modules/oauth/src/main/AuthorizationApi.scala @@ -33,19 +33,19 @@ final class AuthorizationApi(val coll: Coll)(implicit ec: scala.concurrent.Execu .result[PendingAuthorization] .toValid(Protocol.Error.AuthorizationCodeInvalid) .ensure(Protocol.Error.AuthorizationCodeExpired)(_.expires.isAfter(DateTime.now())) - .ensure(Protocol.Error.MismatchingRedirectUri)(_.redirectUri.matches(request.redirectUri)) - .ensure(Protocol.Error.MismatchingClient)(_.clientId == request.clientId) + .ensure(Protocol.Error.MismatchingRedirectUri)(p => + request.redirectUri.forall(p.redirectUri.matches) + ) + .ensure(Protocol.Error.MismatchingClient)(p => request.clientId.forall(_ == p.clientId)) _ <- pending.challenge match { case Left(hashedClientSecret) => request.clientSecret - .map(LegacyClientApi.ClientSecret) - .toValid(LegacyClientApi.ClientSecretRequired) + .toValid(LegacyClientApi.ClientSecretIgnored) .ensure(LegacyClientApi.MismatchingClientSecret)(_.matches(hashedClientSecret)) .map(_.unit) case Right(codeChallenge) => request.codeVerifier - .toValid(Protocol.Error.CodeVerifierRequired) - .andThen(Protocol.CodeVerifier.from) + .toValid(LegacyClientApi.CodeVerifierIgnored) .ensure(Protocol.Error.MismatchingCodeVerifier)(_.matches(codeChallenge)) .map(_.unit) } diff --git a/modules/oauth/src/main/LegacyClientApi.scala b/modules/oauth/src/main/LegacyClientApi.scala index 6a769dafa2..a04724f1a6 100644 --- a/modules/oauth/src/main/LegacyClientApi.scala +++ b/modules/oauth/src/main/LegacyClientApi.scala @@ -35,7 +35,17 @@ object LegacyClientApi { } case object MismatchingClientSecret - extends Protocol.Error.InvalidGrant("fix mismatching client secret (or update to pkce)") + extends Protocol.Error.InvalidGrant( + "fix mismatching client secret (or update to pkce and update endpoints)" + ) case object ClientSecretRequired - extends Protocol.Error.InvalidRequest("client_secret required (or update to pkce)") + extends Protocol.Error.InvalidRequest("client_secret required (or update to pkce and update endpoints)") + case object ClientSecretIgnored + extends Protocol.Error.InvalidRequest( + "cannot finish legacy flow with pkce token request (client_secret ignored, update to pkce or check endpoint)" + ) + case object CodeVerifierIgnored + extends Protocol.Error.InvalidRequest( + "cannot finish pkce flow with legacy token request (code_verifier ignored, update endpoint)" + ) }