From f43cd19910a1052b4142eb1aaed764ad4c0e2af9 Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 00:23:19 +0200 Subject: [PATCH 1/9] start preparing legacy oauth bc --- app/controllers/OAuth.scala | 3 +- .../oauth/src/main/AccessTokenRequest.scala | 15 ++-- modules/oauth/src/main/AuthorizationApi.scala | 72 +++++++++------- .../oauth/src/main/AuthorizationRequest.scala | 82 ++++++++++--------- modules/oauth/src/main/Protocol.scala | 28 ++++--- 5 files changed, 114 insertions(+), 86 deletions(-) diff --git a/app/controllers/OAuth.scala b/app/controllers/OAuth.scala index c80f7dd7b7..5cd3426a92 100644 --- a/app/controllers/OAuth.scala +++ b/app/controllers/OAuth.scala @@ -45,7 +45,7 @@ final class OAuth(env: Env) extends LilaController(env) { def authorizeApply = Auth { implicit ctx => me => withPrompt { prompt => - prompt.authorize(me) match { + prompt.authorize(me, ???) flatMap { case Validated.Valid(authorized) => env.oAuth.authorizationApi.create(authorized) map { code => Redirect(authorized.redirectUrl(code)) @@ -60,6 +60,7 @@ final class OAuth(env: Env) extends LilaController(env) { "grant_type" -> optional(text), "code" -> optional(text), "code_verifier" -> optional(text), + "client_secret" -> optional(text), "redirect_uri" -> optional(text), "client_id" -> optional(text) )(AccessTokenRequest.Raw.apply)(AccessTokenRequest.Raw.unapply) diff --git a/modules/oauth/src/main/AccessTokenRequest.scala b/modules/oauth/src/main/AccessTokenRequest.scala index 8b12693d86..386d0630c7 100644 --- a/modules/oauth/src/main/AccessTokenRequest.scala +++ b/modules/oauth/src/main/AccessTokenRequest.scala @@ -10,23 +10,24 @@ object AccessTokenRequest { grantType: Option[String], code: Option[String], codeVerifier: Option[String], + clientSecret: Option[String], redirectUri: Option[String], clientId: Option[String] ) { def prepare: Validated[Error, Prepared] = for { - grantType <- grantType.toValid(Error.GrantTypeRequired).andThen(GrantType.from) - code <- code.map(AuthorizationCode.apply).toValid(Error.CodeRequired) - codeVerifier <- codeVerifier.toValid(Error.CodeVerifierRequired).andThen(CodeVerifier.from) - redirectUri <- redirectUri.map(UncheckedRedirectUri.apply).toValid(Error.RedirectUriRequired) - clientId <- clientId.map(ClientId.apply).toValid(Error.ClientIdRequired) - } yield Prepared(grantType, code, codeVerifier, redirectUri, clientId) + redirectUri <- redirectUri.map(UncheckedRedirectUri.apply).toValid(Error.RedirectUriRequired) + 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) } case class Prepared( grantType: GrantType, code: AuthorizationCode, - codeVerifier: CodeVerifier, + codeVerifier: Option[String], + clientSecret: Option[String], redirectUri: UncheckedRedirectUri, clientId: ClientId ) diff --git a/modules/oauth/src/main/AuthorizationApi.scala b/modules/oauth/src/main/AuthorizationApi.scala index 0504320bec..084f7d63cc 100644 --- a/modules/oauth/src/main/AuthorizationApi.scala +++ b/modules/oauth/src/main/AuthorizationApi.scala @@ -17,7 +17,7 @@ final class AuthorizationApi(val coll: Coll)(implicit ec: scala.concurrent.Execu request.clientId, request.user, request.redirectUri, - request.codeChallenge, + request.challenge, request.scopes, DateTime.now().plusSeconds(120) ) @@ -27,30 +27,42 @@ final class AuthorizationApi(val coll: Coll)(implicit ec: scala.concurrent.Execu def consume( request: AccessTokenRequest.Prepared ): Fu[Validated[Protocol.Error, AccessTokenRequest.Granted]] = - coll.findAndModify($doc(F.hashedCode -> request.code.hashed), coll.removeModifier) map { - _.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.MismatchingCodeVerifier(request.codeVerifier))( - _.codeChallenge.matches(request.codeVerifier) - ) - .map { pending => - AccessTokenRequest.Granted(pending.userId, pending.scopes, pending.redirectUri) + coll.findAndModify($doc(F.hashedCode -> request.code.hashed), coll.removeModifier) map { doc => + for { + pending <- doc + .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) + _ <- pending.challenge match { + case Left(hashedClientSecret) => + request.clientSecret + .map(Protocol.ClientSecret) + .toValid(Protocol.Error.ClientSecretRequired) + .ensure(Protocol.Error.MismatchingClientSecret)(_.matches(hashedClientSecret)) + .map(_.unit) + case Right(codeChallenge) => + request.codeVerifier + .toValid(Protocol.Error.CodeVerifierRequired) + .andThen(Protocol.CodeVerifier.from) + .ensure(Protocol.Error.MismatchingCodeVerifier)(_.matches(codeChallenge)) + .map(_.unit) } + } yield AccessTokenRequest.Granted(pending.userId, pending.scopes, pending.redirectUri) } } private object AuthorizationApi { object BSONFields { - val hashedCode = "_id" - val clientId = "clientId" - val userId = "userId" - val redirectUri = "redirectUri" - val codeChallenge = "codeChallenge" - val scopes = "scopes" - val expires = "expires" + val hashedCode = "_id" + val clientId = "clientId" + val userId = "userId" + val redirectUri = "redirectUri" + val codeChallenge = "codeChallenge" + val hashedClientSecret = "hashedClientSecret" + val scopes = "scopes" + val expires = "expires" } case class PendingAuthorization( @@ -58,7 +70,7 @@ private object AuthorizationApi { clientId: Protocol.ClientId, userId: User.ID, redirectUri: Protocol.RedirectUri, - codeChallenge: Protocol.CodeChallenge, + challenge: Either[Protocol.HashedClientSecret, Protocol.CodeChallenge], scopes: List[OAuthScope], expires: DateTime ) @@ -75,20 +87,24 @@ private object AuthorizationApi { clientId = Protocol.ClientId(r.str(F.clientId)), userId = r.str(F.userId), redirectUri = Protocol.RedirectUri.unchecked(r.str(F.redirectUri)), - codeChallenge = Protocol.CodeChallenge(r.str(F.codeChallenge)), + challenge = r.strO(F.hashedClientSecret) match { + case Some(hashedClientSecret) => Left(Protocol.HashedClientSecret(hashedClientSecret)) + case None => Right(Protocol.CodeChallenge(r.str(F.codeChallenge))) + }, scopes = r.get[List[OAuthScope]](F.scopes), expires = r.get[DateTime](F.expires) ) def writes(w: BSON.Writer, o: PendingAuthorization) = $doc( - F.hashedCode -> o.hashedCode, - F.clientId -> o.clientId.value, - F.userId -> o.userId, - F.redirectUri -> o.redirectUri.value.toString, - F.codeChallenge -> o.codeChallenge.value, - F.scopes -> o.scopes, - F.expires -> o.expires + F.hashedCode -> o.hashedCode, + F.clientId -> o.clientId.value, + F.userId -> o.userId, + F.redirectUri -> o.redirectUri.value.toString, + F.codeChallenge -> o.challenge.toOption.map(_.value), + F.hashedClientSecret -> o.challenge.swap.toOption.map(_.value), + F.scopes -> o.scopes, + F.expires -> o.expires ) } } diff --git a/modules/oauth/src/main/AuthorizationRequest.scala b/modules/oauth/src/main/AuthorizationRequest.scala index 5d0d2389f9..7772f7152a 100644 --- a/modules/oauth/src/main/AuthorizationRequest.scala +++ b/modules/oauth/src/main/AuthorizationRequest.scala @@ -23,28 +23,25 @@ object AuthorizationRequest { ) { // In order to show a prompt and redirect back with error codes a valid // redirect_uri is absolutely required. Ignore all other errors for now. - def prompt: Validated[Error, Prompt] = { - redirectUri - .toValid(Error.RedirectUriRequired) - .andThen(RedirectUri.from) - .map { redirectUri => - Prompt( - redirectUri, - state.map(State.apply), - clientId = clientId, - responseType = responseType, - codeChallenge = codeChallenge, - codeChallengeMethod = codeChallengeMethod, - scope = scope - ) - } - } + def prompt: Validated[Error, Prompt] = + for { + redirectUri <- redirectUri.toValid(Error.RedirectUriRequired).andThen(RedirectUri.from) + clientId <- clientId.map(ClientId).toValid(Error.ClientIdRequired) + } yield Prompt( + redirectUri, + state.map(State.apply), + clientId = clientId, + responseType = responseType, + codeChallenge = codeChallenge, + codeChallengeMethod = codeChallengeMethod, + scope = scope + ) } case class Prompt( redirectUri: RedirectUri, state: Option[State], - clientId: Option[String], + clientId: ClientId, responseType: Option[String], codeChallenge: Option[String], codeChallengeMethod: Option[String], @@ -66,35 +63,42 @@ object AuthorizationRequest { def maybeScopes: List[OAuthScope] = validScopes.getOrElse(Nil) - def authorize(user: User): Validated[Error, Authorized] = { - for { - clientId <- clientId.map(ClientId.apply).toValid(Error.ClientIdRequired) - scopes <- validScopes - codeChallenge <- codeChallenge.map(CodeChallenge.apply).toValid(Error.CodeChallengeRequired) - responseType <- responseType.toValid(Error.ResponseTypeRequired).andThen(ResponseType.from) - codeChallengeMethod <- codeChallengeMethod - .toValid(Error.CodeChallengeMethodRequired) - .andThen(CodeChallengeMethod.from) - } yield Authorized( - clientId, - redirectUri, - state, - codeChallenge, - codeChallengeMethod, - user.id, - scopes - ) - } + def authorize( + user: User, + legacy: ClientId => Fu[Option[HashedClientSecret]] + ): Fu[Validated[Error, Authorized]] = + (codeChallengeMethod match { + case None => + legacy(clientId).dmap( + _.toValid[Error](Error.CodeChallengeMethodRequired).map(Left.apply) + ) + case Some(method) => + fuccess(CodeChallengeMethod.from(method).andThen { _ => + codeChallenge.map(CodeChallenge).toValid[Error](Error.CodeChallengeRequired).map(Right.apply) + }) + }) dmap { challenge => + for { + challenge <- challenge + scopes <- validScopes + responseType <- responseType.toValid(Error.ResponseTypeRequired).andThen(ResponseType.from) + } yield Authorized( + clientId, + redirectUri, + state, + user.id, + scopes, + challenge + ) + } } case class Authorized( clientId: ClientId, redirectUri: RedirectUri, state: Option[State], - codeChallenge: CodeChallenge, - codeChallengeMethod: CodeChallengeMethod, user: User.ID, - scopes: List[OAuthScope] + scopes: List[OAuthScope], + challenge: Either[HashedClientSecret, CodeChallenge] ) { def redirectUrl(code: AuthorizationCode) = redirectUri.code(code, state) } diff --git a/modules/oauth/src/main/Protocol.scala b/modules/oauth/src/main/Protocol.scala index 4b7a169dbf..ca4ed5461c 100644 --- a/modules/oauth/src/main/Protocol.scala +++ b/modules/oauth/src/main/Protocol.scala @@ -11,7 +11,7 @@ import lila.common.SecureRandom object Protocol { case class AuthorizationCode(secret: String) extends AnyVal { - def hashed = Algo.sha256(secret).hex + def hashed = Algo.sha256(secret).hex override def toString = "AuthorizationCode(***)" } object AuthorizationCode { @@ -22,10 +22,6 @@ object Protocol { case class State(value: String) extends AnyVal - case class CodeChallenge(value: String) extends AnyVal { - def matches(verifier: CodeVerifier) = verifier.challenge == this - } - case class CodeChallengeMethod() object CodeChallengeMethod { def from(codeChallengeMethod: String): Validated[Error, CodeChallengeMethod] = @@ -35,10 +31,11 @@ object Protocol { } } + case class CodeChallenge(value: String) extends AnyVal + case class CodeVerifier(value: String) extends AnyVal { - def challenge = CodeChallenge( - Base64.getUrlEncoder().withoutPadding().encodeToString(Algo.sha256(value).bytes) - ) + def matches(challenge: CodeChallenge) = + Base64.getUrlEncoder().withoutPadding().encodeToString(Algo.sha256(value).bytes) == challenge.value } object CodeVerifier { def from(value: String): Validated[Error, CodeVerifier] = @@ -48,6 +45,13 @@ object Protocol { .map(CodeVerifier.apply) } + case class HashedClientSecret(value: String) extends AnyVal + + case class ClientSecret(secret: String) extends AnyVal { + def matches(hash: HashedClientSecret) = Algo.sha256(secret).hex == hash.value + override def toString = "ClientSecret(***)" + } + case class ResponseType() object ResponseType { def from(responseType: String): Validated[Error, ResponseType] = @@ -133,6 +137,7 @@ object Protocol { case object CodeRequired extends InvalidRequest("code required") case object CodeVerifierRequired extends InvalidRequest("code_verifier required") case object CodeVerifierTooShort extends InvalidRequest("code_verifier too short") + case object ClientSecretRequired extends InvalidRequest("client_secret required (or switch to pkce)") case class InvalidScope(val key: String) extends Error("invalid_scope") { def description = s"invalid scope: ${URLEncoder.encode(key, "UTF-8")}" @@ -150,8 +155,9 @@ object Protocol { extends InvalidGrant("authorization code was issued for a different redirect_uri") case object MismatchingClient extends InvalidGrant("authorization code was issued for a different client_Id") - case class MismatchingCodeVerifier(val verifier: CodeVerifier) extends Error("invalid_grant") { - def description = s"hash '${verifier.challenge.value}' of code_verifier does not match code_challenge" - } + case object MismatchingCodeVerifier + extends InvalidGrant("hash of code_verifier does not match code_challenge") + case object MismatchingClientSecret + extends InvalidGrant("fix mismatching client secret (or switch to pkce)") } } From cb68a03ceaa8b35a78ce60b079e2840b3280ab87 Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 09:38:07 +0200 Subject: [PATCH 2/9] add legacy oauth client collection --- app/controllers/OAuth.scala | 2 +- modules/oauth/src/main/Env.scala | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/OAuth.scala b/app/controllers/OAuth.scala index 5cd3426a92..23bf168e15 100644 --- a/app/controllers/OAuth.scala +++ b/app/controllers/OAuth.scala @@ -45,7 +45,7 @@ final class OAuth(env: Env) extends LilaController(env) { def authorizeApply = Auth { implicit ctx => me => withPrompt { prompt => - prompt.authorize(me, ???) flatMap { + prompt.authorize(me, env.oAuth.legacyClientApi.apply) flatMap { case Validated.Valid(authorized) => env.oAuth.authorizationApi.create(authorized) map { code => Redirect(authorized.redirectUrl(code)) diff --git a/modules/oauth/src/main/Env.scala b/modules/oauth/src/main/Env.scala index adcf452ed9..a75cd9be2d 100644 --- a/modules/oauth/src/main/Env.scala +++ b/modules/oauth/src/main/Env.scala @@ -47,9 +47,9 @@ final class Env( none } - lazy val tokenApi = wire[AccessTokenApi] - + lazy val tokenApi = wire[AccessTokenApi] lazy val authorizationApi = new AuthorizationApi(lilaDb(CollName("oauth2_authorization"))) + lazy val legacyClientApi = new LegacyClientApi(lilaDb(CollName("oauth2_legacy_client"))) def forms = OAuthForm } From e73f471908f9c79cca837619decdb50dfaecd8c5 Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 10:36:57 +0200 Subject: [PATCH 3/9] add legacy oauth redirects --- app/controllers/OAuth.scala | 11 ++++++++++- app/views/oAuth/app/authorize.scala | 4 ++-- conf/routes | 4 +++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/app/controllers/OAuth.scala b/app/controllers/OAuth.scala index 23bf168e15..5a98e1912a 100644 --- a/app/controllers/OAuth.scala +++ b/app/controllers/OAuth.scala @@ -37,11 +37,18 @@ final class OAuth(env: Env) extends LilaController(env) { Open { implicit ctx => withPrompt { prompt => fuccess(ctx.me.fold(Redirect(routes.Auth.login.url, Map("referrer" -> List(ctx.req.uri)))) { me => - Ok(html.oAuth.app.authorize(prompt, me)) + Ok( + html.oAuth.app.authorize(prompt, me, s"${routes.OAuth.authorizeApply}?${ctx.req.rawQueryString}") + ) }) } } + def legacyAuthorize = + Action { req => + MovedPermanently(s"${routes.OAuth.authorize}?${req.rawQueryString}") + } + def authorizeApply = Auth { implicit ctx => me => withPrompt { prompt => @@ -88,6 +95,8 @@ final class OAuth(env: Env) extends LilaController(env) { } } + def legacyTokenApply = tokenApply + def tokenRevoke = Scoped() { implicit req => _ => HTTPRequest.bearer(req) ?? { token => diff --git a/app/views/oAuth/app/authorize.scala b/app/views/oAuth/app/authorize.scala index f0054b4327..e81305ea06 100644 --- a/app/views/oAuth/app/authorize.scala +++ b/app/views/oAuth/app/authorize.scala @@ -10,7 +10,7 @@ import lila.user.User import lila.oauth.AuthorizationRequest object authorize { - def apply(prompt: AuthorizationRequest.Prompt, me: User)(implicit ctx: Context) = + def apply(prompt: AuthorizationRequest.Prompt, me: User, authorizeUrl: String)(implicit ctx: Context) = views.html.base.layout( title = "Authorization", moreCss = cssTag("oauth"), @@ -19,7 +19,7 @@ object authorize { ) { main(cls := "oauth box box-pad")( h1(dataIcon := "", cls := "text")("Authorize third party"), - postForm( + postForm(action := authorizeUrl)( p( strong(code(prompt.redirectUri.clientOrigin)), " wants to access your ", diff --git a/conf/routes b/conf/routes index 0a8607b9c5..98ff69d638 100644 --- a/conf/routes +++ b/conf/routes @@ -717,7 +717,9 @@ GET /account/now-playing controllers.Account.nowPlaying # OAuth GET /oauth controllers.OAuth.authorize -POST /oauth controllers.OAuth.authorizeApply +POST /oauth controllers.OAuth.legacyTokenApply +GET /oauth/authorize controllers.OAuth.legacyAuthorize +POST /oauth/authorize controllers.OAuth.authorizeApply POST /oauth/revoke-client controllers.OAuth.revokeClient POST /api/token controllers.OAuth.tokenApply DELETE /api/token controllers.OAuth.tokenRevoke From 378abab8f7f9a386e9f9c2c4e14141b3d6803d08 Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 11:57:44 +0200 Subject: [PATCH 4/9] add deprecation warning to legacy prompt --- app/controllers/OAuth.scala | 2 +- app/views/oAuth/app/authorize.scala | 5 +++++ modules/oauth/src/main/AuthorizationRequest.scala | 8 +++++--- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/app/controllers/OAuth.scala b/app/controllers/OAuth.scala index 5a98e1912a..04e8fe94a6 100644 --- a/app/controllers/OAuth.scala +++ b/app/controllers/OAuth.scala @@ -21,8 +21,8 @@ final class OAuth(env: Env) extends LilaController(env) { responseType = get("response_type", req), redirectUri = get("redirect_uri", req), state = get("state", req), - codeChallenge = get("code_challenge", req), codeChallengeMethod = get("code_challenge_method", req), + codeChallenge = get("code_challenge", req), scope = get("scope", req) ) diff --git a/app/views/oAuth/app/authorize.scala b/app/views/oAuth/app/authorize.scala index e81305ea06..950bef7538 100644 --- a/app/views/oAuth/app/authorize.scala +++ b/app/views/oAuth/app/authorize.scala @@ -52,6 +52,11 @@ object authorize { a(href := prompt.cancelUrl)("Cancel"), submitButton(cls := "button", disabled := true, id := "oauth-authorize")("Authorize") ) + ), + prompt.maybeLegacy option p( + "Note for developers: The OAuth flow without PKCE is ", + a(href := "https://github.com/ornicar/lila/issues/9214")("deprecated"), + ". Consider updating your app." ) ) } diff --git a/modules/oauth/src/main/AuthorizationRequest.scala b/modules/oauth/src/main/AuthorizationRequest.scala index 7772f7152a..acc461af08 100644 --- a/modules/oauth/src/main/AuthorizationRequest.scala +++ b/modules/oauth/src/main/AuthorizationRequest.scala @@ -17,8 +17,8 @@ object AuthorizationRequest { state: Option[String], redirectUri: Option[String], responseType: Option[String], - codeChallenge: Option[String], codeChallengeMethod: Option[String], + codeChallenge: Option[String], scope: Option[String] ) { // In order to show a prompt and redirect back with error codes a valid @@ -32,8 +32,8 @@ object AuthorizationRequest { state.map(State.apply), clientId = clientId, responseType = responseType, - codeChallenge = codeChallenge, codeChallengeMethod = codeChallengeMethod, + codeChallenge = codeChallenge, scope = scope ) } @@ -43,8 +43,8 @@ object AuthorizationRequest { state: Option[State], clientId: ClientId, responseType: Option[String], - codeChallenge: Option[String], codeChallengeMethod: Option[String], + codeChallenge: Option[String], scope: Option[String] ) { def errorUrl(error: Error) = redirectUri.error(error, state) @@ -63,6 +63,8 @@ object AuthorizationRequest { def maybeScopes: List[OAuthScope] = validScopes.getOrElse(Nil) + def maybeLegacy: Boolean = codeChallengeMethod.isEmpty && codeChallenge.isEmpty + def authorize( user: User, legacy: ClientId => Fu[Option[HashedClientSecret]] From 118f6dfaa65423787bb2755f6b7bccf8f29258ee Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 15:49:59 +0200 Subject: [PATCH 5/9] forgot to commit LegacyClientApi.scala --- modules/oauth/src/main/LegacyClientApi.scala | 24 ++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 modules/oauth/src/main/LegacyClientApi.scala diff --git a/modules/oauth/src/main/LegacyClientApi.scala b/modules/oauth/src/main/LegacyClientApi.scala new file mode 100644 index 0000000000..81f79ac41b --- /dev/null +++ b/modules/oauth/src/main/LegacyClientApi.scala @@ -0,0 +1,24 @@ +package lila.oauth + +import org.joda.time.DateTime + +import lila.db.dsl._ + +final class LegacyClientApi(val coll: Coll)(implicit ec: scala.concurrent.ExecutionContext) { + import LegacyClientApi.{ BSONFields => F } + + def apply(clientId: Protocol.ClientId): Fu[Option[Protocol.HashedClientSecret]] = + coll + .findAndUpdate($id(clientId.value), $set(F.usedAt -> DateTime.now())) + .map { + _.result[Bdoc].flatMap(_.getAsOpt[String](F.hashedSecret)).map(Protocol.HashedClientSecret) + } +} + +object LegacyClientApi { + object BSONFields { + val id = "_id" + val hashedSecret = "secret" + val usedAt = "used" + } +} From d51f39fe84cb96c336a76b64c207e21ef9cd42cb Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 15:58:46 +0200 Subject: [PATCH 6/9] group legacy protocol types --- modules/oauth/src/main/AuthorizationApi.scala | 10 +++++----- .../oauth/src/main/AuthorizationRequest.scala | 4 ++-- modules/oauth/src/main/LegacyClientApi.scala | 17 ++++++++++++++--- modules/oauth/src/main/Protocol.scala | 10 ---------- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/modules/oauth/src/main/AuthorizationApi.scala b/modules/oauth/src/main/AuthorizationApi.scala index 084f7d63cc..3d10f5b32b 100644 --- a/modules/oauth/src/main/AuthorizationApi.scala +++ b/modules/oauth/src/main/AuthorizationApi.scala @@ -38,9 +38,9 @@ final class AuthorizationApi(val coll: Coll)(implicit ec: scala.concurrent.Execu _ <- pending.challenge match { case Left(hashedClientSecret) => request.clientSecret - .map(Protocol.ClientSecret) - .toValid(Protocol.Error.ClientSecretRequired) - .ensure(Protocol.Error.MismatchingClientSecret)(_.matches(hashedClientSecret)) + .map(LegacyClientApi.ClientSecret) + .toValid(LegacyClientApi.ClientSecretRequired) + .ensure(LegacyClientApi.MismatchingClientSecret)(_.matches(hashedClientSecret)) .map(_.unit) case Right(codeChallenge) => request.codeVerifier @@ -70,7 +70,7 @@ private object AuthorizationApi { clientId: Protocol.ClientId, userId: User.ID, redirectUri: Protocol.RedirectUri, - challenge: Either[Protocol.HashedClientSecret, Protocol.CodeChallenge], + challenge: Either[LegacyClientApi.HashedClientSecret, Protocol.CodeChallenge], scopes: List[OAuthScope], expires: DateTime ) @@ -88,7 +88,7 @@ private object AuthorizationApi { userId = r.str(F.userId), redirectUri = Protocol.RedirectUri.unchecked(r.str(F.redirectUri)), challenge = r.strO(F.hashedClientSecret) match { - case Some(hashedClientSecret) => Left(Protocol.HashedClientSecret(hashedClientSecret)) + case Some(hashedClientSecret) => Left(LegacyClientApi.HashedClientSecret(hashedClientSecret)) case None => Right(Protocol.CodeChallenge(r.str(F.codeChallenge))) }, scopes = r.get[List[OAuthScope]](F.scopes), diff --git a/modules/oauth/src/main/AuthorizationRequest.scala b/modules/oauth/src/main/AuthorizationRequest.scala index acc461af08..962b69d26f 100644 --- a/modules/oauth/src/main/AuthorizationRequest.scala +++ b/modules/oauth/src/main/AuthorizationRequest.scala @@ -67,7 +67,7 @@ object AuthorizationRequest { def authorize( user: User, - legacy: ClientId => Fu[Option[HashedClientSecret]] + legacy: ClientId => Fu[Option[LegacyClientApi.HashedClientSecret]] ): Fu[Validated[Error, Authorized]] = (codeChallengeMethod match { case None => @@ -100,7 +100,7 @@ object AuthorizationRequest { state: Option[State], user: User.ID, scopes: List[OAuthScope], - challenge: Either[HashedClientSecret, CodeChallenge] + challenge: Either[LegacyClientApi.HashedClientSecret, CodeChallenge] ) { def redirectUrl(code: AuthorizationCode) = redirectUri.code(code, state) } diff --git a/modules/oauth/src/main/LegacyClientApi.scala b/modules/oauth/src/main/LegacyClientApi.scala index 81f79ac41b..bbc07ce985 100644 --- a/modules/oauth/src/main/LegacyClientApi.scala +++ b/modules/oauth/src/main/LegacyClientApi.scala @@ -1,17 +1,18 @@ package lila.oauth import org.joda.time.DateTime +import com.roundeights.hasher.Algo import lila.db.dsl._ final class LegacyClientApi(val coll: Coll)(implicit ec: scala.concurrent.ExecutionContext) { - import LegacyClientApi.{ BSONFields => F } + import LegacyClientApi.{ BSONFields => F, _ } - def apply(clientId: Protocol.ClientId): Fu[Option[Protocol.HashedClientSecret]] = + def apply(clientId: Protocol.ClientId): Fu[Option[HashedClientSecret]] = coll .findAndUpdate($id(clientId.value), $set(F.usedAt -> DateTime.now())) .map { - _.result[Bdoc].flatMap(_.getAsOpt[String](F.hashedSecret)).map(Protocol.HashedClientSecret) + _.result[Bdoc].flatMap(_.getAsOpt[String](F.hashedSecret)).map(HashedClientSecret) } } @@ -21,4 +22,14 @@ object LegacyClientApi { val hashedSecret = "secret" val usedAt = "used" } + + case class HashedClientSecret(value: String) extends AnyVal + + case class ClientSecret(secret: String) extends AnyVal { + def matches(hash: HashedClientSecret) = Algo.sha256(secret).hex == hash.value + override def toString = "ClientSecret(***)" + } + + case object MismatchingClientSecret extends Protocol.Error.InvalidGrant("fix mismatching client secret (or update to pkce)") + case object ClientSecretRequired extends Protocol.Error.InvalidRequest("client_secret required (or update to pkce)") } diff --git a/modules/oauth/src/main/Protocol.scala b/modules/oauth/src/main/Protocol.scala index ca4ed5461c..c8697d588e 100644 --- a/modules/oauth/src/main/Protocol.scala +++ b/modules/oauth/src/main/Protocol.scala @@ -45,13 +45,6 @@ object Protocol { .map(CodeVerifier.apply) } - case class HashedClientSecret(value: String) extends AnyVal - - case class ClientSecret(secret: String) extends AnyVal { - def matches(hash: HashedClientSecret) = Algo.sha256(secret).hex == hash.value - override def toString = "ClientSecret(***)" - } - case class ResponseType() object ResponseType { def from(responseType: String): Validated[Error, ResponseType] = @@ -137,7 +130,6 @@ object Protocol { case object CodeRequired extends InvalidRequest("code required") case object CodeVerifierRequired extends InvalidRequest("code_verifier required") case object CodeVerifierTooShort extends InvalidRequest("code_verifier too short") - case object ClientSecretRequired extends InvalidRequest("client_secret required (or switch to pkce)") case class InvalidScope(val key: String) extends Error("invalid_scope") { def description = s"invalid scope: ${URLEncoder.encode(key, "UTF-8")}" @@ -157,7 +149,5 @@ object Protocol { extends InvalidGrant("authorization code was issued for a different client_Id") case object MismatchingCodeVerifier extends InvalidGrant("hash of code_verifier does not match code_challenge") - case object MismatchingClientSecret - extends InvalidGrant("fix mismatching client secret (or switch to pkce)") } } From 25ac2e11381f12326aede95021b196e7ffc20476 Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 19:08:38 +0200 Subject: [PATCH 7/9] whitelist some custom url schemes for bc --- modules/oauth/src/main/LegacyClientApi.scala | 6 ++++-- modules/oauth/src/main/Protocol.scala | 19 ++++++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/modules/oauth/src/main/LegacyClientApi.scala b/modules/oauth/src/main/LegacyClientApi.scala index bbc07ce985..ef099d6de2 100644 --- a/modules/oauth/src/main/LegacyClientApi.scala +++ b/modules/oauth/src/main/LegacyClientApi.scala @@ -30,6 +30,8 @@ object LegacyClientApi { override def toString = "ClientSecret(***)" } - case object MismatchingClientSecret extends Protocol.Error.InvalidGrant("fix mismatching client secret (or update to pkce)") - case object ClientSecretRequired extends Protocol.Error.InvalidRequest("client_secret required (or update to pkce)") + case object MismatchingClientSecret + extends Protocol.Error.InvalidGrant("fix mismatching client secret (or update to pkce)") + case object ClientSecretRequired + extends Protocol.Error.InvalidRequest("client_secret required (or update to pkce)") } diff --git a/modules/oauth/src/main/Protocol.scala b/modules/oauth/src/main/Protocol.scala index c8697d588e..ecf564b9b4 100644 --- a/modules/oauth/src/main/Protocol.scala +++ b/modules/oauth/src/main/Protocol.scala @@ -90,7 +90,24 @@ object Protocol { .parseOption(redirectUri) .toValid(Error.RedirectUriInvalid) .ensure(Error.RedirectSchemeNotAllowed)(url => - List("http", "https", "ionic", "capacitor").has(url.scheme) + List( + // standard + "http", + "https", + "ionic", + "capacitor", + // bc + "squareoffapp", + "anichess", + "lichessmac", + "chessrtx", + "chesscomopse", + // whitelist (consider automating) + "no.rieck.chess.dgt", + "net.developerfluid.darkknight", + "com.guykn.chessboard3", + "com.georgdotorg.catur" + ).has(url.scheme) ) .map(RedirectUri.apply) From 5f275307d04d6c02313f0ae54083e5df690310e1 Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 19:40:00 +0200 Subject: [PATCH 8/9] do not whitelist apps that would have leaked their client secret --- modules/oauth/src/main/Protocol.scala | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/modules/oauth/src/main/Protocol.scala b/modules/oauth/src/main/Protocol.scala index ecf564b9b4..c8697d588e 100644 --- a/modules/oauth/src/main/Protocol.scala +++ b/modules/oauth/src/main/Protocol.scala @@ -90,24 +90,7 @@ object Protocol { .parseOption(redirectUri) .toValid(Error.RedirectUriInvalid) .ensure(Error.RedirectSchemeNotAllowed)(url => - List( - // standard - "http", - "https", - "ionic", - "capacitor", - // bc - "squareoffapp", - "anichess", - "lichessmac", - "chessrtx", - "chesscomopse", - // whitelist (consider automating) - "no.rieck.chess.dgt", - "net.developerfluid.darkknight", - "com.guykn.chessboard3", - "com.georgdotorg.catur" - ).has(url.scheme) + List("http", "https", "ionic", "capacitor").has(url.scheme) ) .map(RedirectUri.apply) From 981173dceadaa60b28d81fd6f5f3b585a0cbd0bd Mon Sep 17 00:00:00 2001 From: Niklas Fiekas Date: Mon, 28 Jun 2021 19:50:33 +0200 Subject: [PATCH 9/9] also match redirectUri for legacy clients --- modules/oauth/src/main/AuthorizationRequest.scala | 4 ++-- modules/oauth/src/main/LegacyClientApi.scala | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/modules/oauth/src/main/AuthorizationRequest.scala b/modules/oauth/src/main/AuthorizationRequest.scala index 962b69d26f..d5f2078e4b 100644 --- a/modules/oauth/src/main/AuthorizationRequest.scala +++ b/modules/oauth/src/main/AuthorizationRequest.scala @@ -67,11 +67,11 @@ object AuthorizationRequest { def authorize( user: User, - legacy: ClientId => Fu[Option[LegacyClientApi.HashedClientSecret]] + legacy: (ClientId, RedirectUri) => Fu[Option[LegacyClientApi.HashedClientSecret]] ): Fu[Validated[Error, Authorized]] = (codeChallengeMethod match { case None => - legacy(clientId).dmap( + legacy(clientId, redirectUri).dmap( _.toValid[Error](Error.CodeChallengeMethodRequired).map(Left.apply) ) case Some(method) => diff --git a/modules/oauth/src/main/LegacyClientApi.scala b/modules/oauth/src/main/LegacyClientApi.scala index ef099d6de2..6a769dafa2 100644 --- a/modules/oauth/src/main/LegacyClientApi.scala +++ b/modules/oauth/src/main/LegacyClientApi.scala @@ -8,9 +8,12 @@ import lila.db.dsl._ final class LegacyClientApi(val coll: Coll)(implicit ec: scala.concurrent.ExecutionContext) { import LegacyClientApi.{ BSONFields => F, _ } - def apply(clientId: Protocol.ClientId): Fu[Option[HashedClientSecret]] = + def apply(clientId: Protocol.ClientId, redirectUri: Protocol.RedirectUri): Fu[Option[HashedClientSecret]] = coll - .findAndUpdate($id(clientId.value), $set(F.usedAt -> DateTime.now())) + .findAndUpdate( + $doc(F.id -> clientId.value, F.redirectUri -> redirectUri.value.toString), + $set(F.usedAt -> DateTime.now()) + ) .map { _.result[Bdoc].flatMap(_.getAsOpt[String](F.hashedSecret)).map(HashedClientSecret) } @@ -19,6 +22,7 @@ final class LegacyClientApi(val coll: Coll)(implicit ec: scala.concurrent.Execut object LegacyClientApi { object BSONFields { val id = "_id" + val redirectUri = "redirectUri" val hashedSecret = "secret" val usedAt = "used" }